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

Add support for HTTP Basic authentication #25

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arogge
Copy link

@arogge arogge commented Nov 25, 2024

We run timetagger locally hosted with LDAP authentication. This produces the (probably rather unusual) requirement to authenticate to the reverse-proxy using the LDAP credentials before we can access the API.
For this I added support for that using two new configuration options auth_username and auth_password.

This code currently works for me.

However, I'm unsure where to advertise that feature (i.e. whether to put it in the sample configuration).
Also, I don't think it should be too obvious, because we probably don't want people to configure username/password unless they really need that (after all, you're putting a cleartext password into the configuration)
Maybe we could emit a helpful error when we get an HTTP 401 and not mention it otherwise?

Another thing I noticed is, that maybe I should add a check that either both or none of the parameters are set when we load the config.

Any guidance, hints and suggestions are very welcome!

For normal setups this is not required. However, when using external
authentication (e.g. in setups using LDAP authentication), the API
endpoint might only be accessible after the client authenticated. For
that we add the new parameters `auth_username` and `auth_password`. If
both of these are set the API calls will be made using these
credentials.
@almarklein
Copy link
Owner

Cool!

Would be nice to add dummy variables in the initial_config_text in config.py. plus a comment that explains their use. That should probably enough for people who need it to find it.

I think the code should not only check for presence, but only of it being non-empty. Maybe call them basic_auth_username to be explicit and maybe enable other auth later.

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