Skip to content
This repository has been archived by the owner on Jan 24, 2019. It is now read-only.

various fixes for getting Nginx auth_request mode working #319

Merged
merged 3 commits into from Mar 29, 2017
Merged

various fixes for getting Nginx auth_request mode working #319

merged 3 commits into from Mar 29, 2017

Conversation

ashkulz
Copy link
Contributor

@ashkulz ashkulz commented Nov 16, 2016

This supersedes #173 and #247, with the following changes:

  • adds a new flag --set-xauthrequest which sets response headers for use in auth_request mode (analogous to the X-Forwarded-User and X-Forwarded-Email headers in normal mode).
  • optionally read the redirect URL from request header X-Auth-Request-Redirect as the request URL doesn't correspond to the URL to redirect to in auth_request mode
  • documentation updates

@ashkulz
Copy link
Contributor Author

ashkulz commented Nov 16, 2016

Hopefully @jehiah would merge this PR, as multiple PRs related to this have been submitted and it doesn't become an example of XKCD 927.

@ashkulz
Copy link
Contributor Author

ashkulz commented Nov 22, 2016

@jehiah: anything required to merge this?

@ashkulz
Copy link
Contributor Author

ashkulz commented Dec 4, 2016

Pinging @jehiah ...

@kobiakov
Copy link

kobiakov commented Mar 7, 2017

The last commit in the master is Nov 18, 2016. Probably it's just about the time to fork the project? :)

@ashkulz
Copy link
Contributor Author

ashkulz commented Mar 8, 2017

@akobyakov: I suspect that no one wants to be the maintainer. Maybe trying pinging @bitly on Twitter?

@ploxiln
Copy link
Contributor

ploxiln commented Mar 8, 2017

I think that if someone stepped-up with a fork, which incorporate this PR as well as the websocket support one, and was reasonably well tested and maintained, it could gain traction and then get a link from this repo's README.

I haven't worked at Bitly in a few years, so take that with a grain of salt.

I'm pretty sure the bitly twitter account is the wrong contact for this issue. I'd instead ping @SeanOC and @markrechler

@rcoup
Copy link

rcoup commented Mar 29, 2017

@jehiah you seem to be active again -- is there any interest in merging this?

@jehiah
Copy link
Member

jehiah commented Mar 29, 2017

Yes! sorry for the moving target on master. Can you rebase this?

Also It's worth mentioning that I'm not personally using this configuration, so it'd be really really really nice to get a setup on travis that exercises this config. Would you be interested in trying to add some sort of tests in this PR?

lsiudut and others added 3 commits March 29, 2017 21:28
This is enhancement of #173 to use "Auth Request" consistently in
the command-line option, configuration file and response headers.
It always sets the X-Auth-Request-User response header and if the
email is available, sets X-Auth-Request-Email as well.
This is useful in Nginx auth_request mode, if a 401 handler is
configured to redirect to the sign-in page. As the request URL
does not reflect the actual URL, the value is taken from the
header "X-Auth-Request-Redirect" instead. Based on #247
@ashkulz
Copy link
Contributor Author

ashkulz commented Mar 29, 2017

@jehiah: rebased on top of latest master. Not sure if you noticed the test TestAuthOnlyEndpointSetXAuthRequestHeaders which was added, let me know what you'd like to see in addition.

@jehiah jehiah merged commit af7be2d into bitly:master Mar 29, 2017
@jehiah
Copy link
Member

jehiah commented Mar 29, 2017

@ashkulz I think i'm just musing about getting an end-to-end test running via jenkins that includes actually requesting through nginx. That can happen later though. Thanks for these improvements

@ashkulz
Copy link
Contributor Author

ashkulz commented Mar 29, 2017

Not sure if that can run on Travis CI. BTW, the merge build failed -- maybe a flaky test?

2017/03/29 16:15:53 validator.go:42: failed opening authenticated-emails-file="/tmp/test_auth_emails_964360117", open /tmp/test_auth_emails_964360117: no such file or directory

Also, any ETA for a 2.2 release? 😄

@jehiah
Copy link
Member

jehiah commented Mar 29, 2017

Also, any ETA for a 2.2 release? 😄

soon. I am working on some more complete validation in this build, but I think it's already got enough fixes/changes to warrant a release, even if there are plenty more queued up to do another release in a month or two.

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

Successfully merging this pull request may close these issues.

8 participants