Add support for HTTP Basic authentication#25
Open
arogge wants to merge 1 commit into
Open
Conversation
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.
Owner
|
Cool! Would be nice to add dummy variables in the I think the code should not only check for presence, but only of it being non-empty. Maybe call them |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_usernameandauth_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!