Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement ACL Login #215

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Implement ACL Login #215

wants to merge 3 commits into from

Conversation

willnode
Copy link

@willnode willnode commented Feb 2, 2025

This enables the login page to truly check with ACL configuration instead of checking with username/password config.

It also:

  • When default user is off, the web redirects to login page
  • Make logout a bit nicer for Basic Auth
  • Handle access exceptions if ACL denies to query some keys

This works for my need but I need feedback before I can make this changes a bit nicer, as it's currently includes a breaking changes such as

  • Changed from Auth digest to Auth basic
  • config['login'] no longer works
  • Changed cookie login to store it session

@willnode willnode changed the title Add exception handling Implement ACL Login Feb 2, 2025
@erikdubbelboer
Copy link
Owner

I'm sorry but I can't review all these changes mixed though each other. I'm not sure what This enables the login page to truly check with ACL configuration instead of checking with username/password config. means. Can you maybe explain this or reduce the changes to only this? I'm not sure why that would mean Auth Basic needs to be used and switched from cookie to session?

@willnode
Copy link
Author

I ran a shared redis instance where it has many ACL accounts, so when user logged in they require to log in with one of ACL account since the default user is disabled.

This enables the login page to truly check with ACL configuration instead of checking with username/password config. simply means when login credentials provided, it will no longer check user/pass in config, but it will check if AUTH <username> <password> success or not.

I ran some difficulties when implementing this, especially with WWW-Digest as the correct password is unknown and I need the actual password, not the Digest-ed password, so I have to use WWW-Basic auth. I'm not sure if I still can use Digest unless there's a way to recover user-provided password with it.

Also, I have to use $_SESSION because it's easier to control.

Would you like me to resubmit this PR to multiple smaller ones?

@erikdubbelboer
Copy link
Owner

Would it be possible to make this an option to configure? And without it we still use Digest.

And is it really not possible to do with cookies, not everyone will have sessions configured properly. I don't want to make sessions a dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants