Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add login.gov provider #55

Merged
merged 63 commits into from Mar 20, 2019
Merged

Conversation

timothy-spencer
Copy link
Contributor

Description

This change does three things:

  • Adds the login.gov provider, which is basically an OIDC provider, but doesn't have renewal stuff in it, and it also requires more parameters for the authentication step.
  • Adds environment variables to all config options, so that it is easier to configure the proxy in environments like GCP's App Engine.
  • Updates the README with info about how to use the login.gov provider.

Motivation and Context

We would like to be able to use the proxy with login.gov.

How Has This Been Tested?

I wrote tests into logingov_test.go, and have been using that.
I also have set up a test application using the login.gov integration environment on my local host,
using the instructions in the README.md.

Checklist:

  • My change requires a change to the documentation or CHANGELOG.
  • I have updated the documentation/CHANGELOG accordingly.
  • I have created a feature (non-master) branch for my PR.

fixing bugs now that I think I understand things better
Fixing all dependencies to point at my fork
…d up []byte stuff, removed client_secret if we are using login.gov
Copy link
Member

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delay in getting this reviewed. Between illness, annual leave and doing some research on the login.gov implementation I have been considering this one for a little while.

I've made a few comments throughout, mostly minor nitpicks but am in general happy with this from a code style perspective.

I have no ability to test this though so I have to rely on you for that. On that note, to accept this change I would really like to add you as the CodeOwner for this provider, meaning future PRs to the code in providers/login_gov.go would request review from you, is that ok? Can you add the relevant line to the CodeOwners file as a part of this PR?

CHANGELOG.md Outdated Show resolved Hide resolved
options.go Outdated Show resolved Hide resolved
options.go Outdated Show resolved Hide resolved
options.go Show resolved Hide resolved
providers/logingov.go Show resolved Hide resolved
providers/logingov_test.go Outdated Show resolved Hide resolved
JoelSpeed and others added 6 commits March 6, 2019 10:49
@timothy-spencer
Copy link
Contributor Author

Sorry, didn't mean to harass you! Life is indeed busy. :-) I have updated everything that you requested and did a merge with master, updated CODEOWNERS, etc. Let me know what else you need. Thanks!

providers/logingov.go Show resolved Hide resolved
providers/logingov.go Show resolved Hide resolved
@timothy-spencer
Copy link
Contributor Author

By a TODO, did you mean in the code, or an issue in the repo, or something in the README? I would think that an issue would be the best way to do this, but wanted to make sure in case you had something else in mind. :-)

@JoelSpeed
Copy link
Member

I would tend to put a TODO(@<gh_id>): ... in the code and raise an issue to match it. Helps people reading the code notice the known issues IMO, either is sufficient on its own though

@timothy-spencer
Copy link
Contributor Author

Hey, anything else you need me to do here? I actually have a few more changes that I am maintaining on another branch that I am using to be able to deploy this stuff in GCP that I'd love to clean up and PR, but it would be hard without all the rest of this stuff. :-) Love to get this merged in!

@JoelSpeed JoelSpeed merged commit 8cc5fbf into oauth2-proxy:master Mar 20, 2019
@JoelSpeed
Copy link
Member

@timothy-spencer Thanks for your work on this 😄

@timothy-spencer
Copy link
Contributor Author

Thank you for taking the time to review/merge!

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

Successfully merging this pull request may close these issues.

None yet

3 participants