Watch the htpasswd file for changes and update the htpasswdMap
#1701
Conversation
|
Hi all. Checking into this PR to see if its now ready for merge? |
|
@braunsonm The requested changes were implemented, could you please review it again? |
pkg/authentication/basic/htpasswd.go
Outdated
|
|
||
| err := h.loadHTPasswdFile(path) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Please wrap errors you are returning with additional context
| return nil, err | |
| return nil, fmt.Errorf("could not load htpasswd file: %v", err) |
There was a problem hiding this comment.
We already have the context in the oauthproxy.go file -> https://github.com/oauth2-proxy/oauth2-proxy/blob/master/oauthproxy.go#L114-L117.
We just need the value of the error retrieved from loadHTPasswdFile or WatchFileForUpdates.
There was a problem hiding this comment.
Typically, if you add context at every level, it's easy to see the exact error paths. Else you have to dig into each function call within this function to see which child error came up
There was a problem hiding this comment.
Changed the error message from oauthproxy.go and added the context for load and watch.
pkg/authentication/basic/htpasswd.go
Outdated
| } | ||
| }) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
The context in already in the oauthproxy.go file -- same as above.
There was a problem hiding this comment.
Wrapped the error with context
Co-authored-by: Joel Speed <Joel.speed@hotmail.co.uk>
|
Amazing! |
…auth2-proxy#1701) * dynamically update the htpasswdMap based on the changes made to the htpasswd file * added tests to validate that htpasswdMap is updated after the htpasswd file is changed * refactored `htpasswd` and `watcher` to lower cognitive complexity * returned errors and refactored tests * added `CHANGELOG.md` entry for oauth2-proxy#1701 and fixed the codeclimate issue * Apply suggestions from code review Co-authored-by: Joel Speed <Joel.speed@hotmail.co.uk> * Fix lint issue from code suggestion * Wrap htpasswd load and watch errors with context * add the htpasswd wrapped error context to the test Co-authored-by: Joel Speed <Joel.speed@hotmail.co.uk>
Description
watcher.gowas moved to a package to be accessible globallywatcher.go- code was refactored to accommodate the implemented featurehtpasswd.go- code was refactored to allow updates while the service is up&runninghtpasswd_test.go- new tests were added to validate the added featureBased on these changes basic auth users can be dynamically added or removed
Motivation and Context
Fixes #1320
How Has This Been Tested?
oauth2-proxycharthtpasswd-fileSecretwhile the service was running (used working/malformed/invalid user details)Checklist: