Store name of authenticated user in basicauth #1426

Merged
merged 7 commits into from Feb 17, 2017

Projects

None yet

2 participants

@jung-kurt
Collaborator

The fastcgi and (pending) cgi middleware are currently unable to set the REMOTE_USER environment variable of users authenticated with the basicauth plugin. This PR sets the HTTP_AUTHORIZED key in the request header to the name of the authenticated user in order to do that. The password is not retained.

@jung-kurt jung-kurt Store name of authenticated user in basicauth for use by upstream mid…
…dleware such as fastcgi and cgi.
b097ad9
@mholt
Owner
mholt commented Feb 14, 2017

Thanks for the PR, @jung-kurt! This is not a standard field, correct? Do you think this will be likely to overwrite an existing header field?

@mholt
Owner
mholt commented Feb 14, 2017

Actually, how about using request.Context instead?

And can you add a test case? 😇

@jung-kurt
Collaborator

Actually, how about using request.Context instead?

Perfect! That makes much better sense than co-opting a header field.

And can you add a test case? 😇

Will do.

jung-kurt added some commits Feb 14, 2017
@jung-kurt jung-kurt Use request context to transfer name of authorized user from basicaut…
…h to upstream middleware. Test retrieval of name from context.
fd02399
@jung-kurt jung-kurt Merge branch 'master' into remote_user
bc3ee0c
@jung-kurt jung-kurt Remove development code that was inadvertently left in place af96aa7
@jung-kurt jung-kurt Merge branch 'remote_user' of https://github.com/jung-kurt/caddy into…
… remote_user
e257f58
@mholt

I think I'm blocking this one. Oops. Check back after Go 1.8 is released and one other PR is merged! 😅

caddyhttp/basicauth/basicauth.go
+
+ // let upstream middleware (e.g. fastcgi and cgi) know about authenticated
+ // user; this replaces the request with a wrapped instance
+ r = r.WithContext(context.WithValue(r.Context(), "remote_user", username))
@mholt
mholt Feb 16, 2017 Owner

Instead of using a primitive string type as a context key (which is asking for collisions, though I admit, less likely in Caddy-specific packages), we should wrap this in httpserver.CtxKey. It doesn't exist yet but it will when #1430 is merged! I should have done that change separately, sorry. Also see #1331 which may be blocked on the same thing.

@mholt
mholt Feb 17, 2017 Owner

@jung-kurt Okay, I've merged the other branch, so if you want to wrap your string key in httpserver.CtxKey, then I think we're good to go!

@jung-kurt
jung-kurt Feb 17, 2017 Collaborator

Thanks, @mholt!

caddyhttp/basicauth/basicauth_test.go
+ // This handler is registered for tests in which the only authorized user is
+ // "okuser"
+ upstreamHandler := func(w http.ResponseWriter, r *http.Request) (int, error) {
+ remoteUser, _ := r.Context().Value("remote_user").(string)
@mholt
mholt Feb 16, 2017 Owner

The , _ isn't necessary for type assertions, but why ignore the bool/error? Wouldn't it panic then? I guess it's a test case so it's not the end of the world, but perhaps better to check the error and t.Fatal if there's a problem.

Edit: Nevermind, it doesn't panic. Clever.

@jung-kurt
jung-kurt Feb 16, 2017 Collaborator

Admittedly, it could be too clever. Maybe something like this is clearer for maintenance. Does it make sense to have some helper functions for setting and getting context variables?

@mholt
mholt Feb 16, 2017 Owner

The more I think about it, the net result is the same. It's not technically an error value, it's a boolean telling you whether the assertion was successful. I like the brevity of your approach. I just forgot that type assertions don't panic if you assign to two variables, even if one is discarded. This is fine.

jung-kurt added some commits Feb 17, 2017
@jung-kurt jung-kurt Use keys of type httpserver.CtxKey to access Context values
7c40887
@jung-kurt jung-kurt Merge branch 'master' into remote_user
95d937a
@mholt
mholt approved these changes Feb 17, 2017 View changes
@mholt mholt merged commit 977a3c3 into mholt:master Feb 17, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@mholt
Owner
mholt commented Feb 17, 2017

Thank you, @jung-kurt ! If you'd like, I'll make you a collaborator so you can submit changes in branches instead of forks, and help review pull requests and help with issues.

@jung-kurt jung-kurt deleted the jung-kurt:remote_user branch Feb 18, 2017
@jung-kurt
Collaborator

Thanks, @mholt -- I'd be honored to be a collaborator. Caddy is an impressive project -- keep up the great work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment