Pass the access token to the upstream server #80
Conversation
This reflects the apparent intent of TestNewReverseProxy(). Without this change, the test will fail when run without an Internet connection.
mbland
changed the title
| payload := "" | ||
| switch url.Path { | ||
| case "/oauth/token": | ||
| payload = "{\"access_token\": \"my_auth_token\"}" |
There was a problem hiding this comment.
Can you backtick this string so it's a little easier to read?
|
@jehiah I've just pushed changes to set |
|
Changes look good. Let's just error if the cookie secret is <32 char and you enabled passing the access token instead of padding with spaces. |
|
Ah, PTAL == Please Take Another Look. :-) Just pushed a new commit with the changes you asked for. (Actually, the AES cipher isn't created if the |
| @@ -405,7 +419,17 @@ func (p *OauthProxy) ServeHTTP(rw http.ResponseWriter, req *http.Request) { | |||
| // set cookie, or deny | |||
| if p.Validator(email) { | |||
| log.Printf("%s authenticating %s completed", remoteAddr, email) | |||
| p.SetCookie(rw, req, email) | |||
| if p.PassAccessToken { | |||
| access_token, err = encodeAccessToken(p.AesCipher, access_token) | |||
There was a problem hiding this comment.
I think that on the validating request this leaves an encoded access token to be passed in the header because we are re-using this variable.
There was a problem hiding this comment.
It looks like we always return when we hit this case, but I'll make it a new variable to be on the safe side.
There was a problem hiding this comment.
oh duh. i missed that redirect line in looking at the changes. all good here then.
|
Looks good aside from those two small details. Once you've handled those go ahead and squash the commits in this branch down. |
This is accomplished by encoding the access_token in the auth cookie and unpacking it as the X-Forwarded-Access-Token header for upstream requests.
mbland
force-pushed
the
pass-access-token
branch
from
236a753 to
ad3c9a8
Compare
|
Created a new |
|
awesome! |
Pass the access token to the upstream server
|
Sweeet, thanks, @jehiah! Appreciate the thoughtful and timely review. |
Refactor pass_access_token changes from #80
@jehiah We're looking to allow other services that accept the
access_tokento be integrated into the 18F Hub per 18F/hub#241. This PR accomplishes this by encoding theaccess_tokenin the auth cookie and unpacking it as theX-Forwarded-Access-Tokenheader for upstream requests. The changes maintain backward compatibility with the original cookie format, but I'm happy to entertain other notions or refactor the code.@dhcole @afeld @adelevie @yozlet @jackiekazil I've actually got our internal Hub running successfully with these changes to the
google_auth_proxyand the changes from 18F/hub#241. Just need to merge them to make them official.I also included a commit to update TestNewReverseProxy(). Without this change, the test will fail when run without an Internet connection. @jehiah I can split that commit out into a separate PR if you prefer.