Skip to content

feat: Add GitHub OAuth #1050

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Apr 23, 2022
Merged

feat: Add GitHub OAuth #1050

merged 14 commits into from
Apr 23, 2022

Conversation

kylecarbs
Copy link
Member

@kylecarbs kylecarbs commented Apr 17, 2022 •
edited
Loading

This adds scaffolding for future OAuth providers and adds a real implementation for GitHub.

  • /users/authmethods endpoint to detect whether GitHub authentication is enabled. It returns password as well, which is currently statically set to true.
  • Renames the built-in login type to password. It felt like a better name (happy to discuss this).
  • Implements the frontend to optionally display a GitHub button if enabled. This is really basic right now, but figured we could pretty it up in a future PR (happy to do it here if y'all would like).

This intentionally avoids allowing multiple GitHub OAuth configs. External providers like Okta should be recommended in that scenario. This has worked for Sourcegraph, so I think it's fair to say we'll be fine for a while.

I've added configuration as CLI flags for now. In the future, we'll be moving this to a config that would look something like:

auth:
  password: false
  github:
    client-id: xxxxxx
    client-secret: xxxxxx
  gitlab:
    client-id: xxxxxx
    client-secret: xxxxxx
  oidc:
    client-id: xxxxxx
    client-secret: xxxxxx

In a future PR, I'll be separating access and refresh tokens from API keys. They should be attached to a user, not a specific token.

image

@codecov
Copy link

codecov bot commented Apr 17, 2022 •
edited
Loading

Codecov Report

Merging #1050 (aa9e5b7) into main (3976994) will decrease coverage by 9.08%.
The diff coverage is 63.13%.

@@            Coverage Diff             @@
##             main    #1050      +/-   ##
==========================================
- Coverage   66.56%   57.47%   -9.09%     
==========================================
  Files         255      257       +2     
  Lines       15743    16011     +268     
  Branches      152      156       +4     
==========================================
- Hits        10479     9202    -1277     
- Misses       4196     5815    +1619     
+ Partials     1068      994      -74
Flag Coverage Δ
unittest-go-macos-latest 53.56% <61.48%> (+0.23%) ⬆️
unittest-go-postgres- ?
unittest-go-ubuntu-latest 56.04% <61.48%> (+0.14%) ⬆️
unittest-go-windows-2022 53.02% <61.48%> (+0.06%) ⬆️
unittest-js 67.28% <100.00%> (+0.45%) ⬆️
Impacted Files Coverage Δ
coderd/database/queries.sql.go 0.00% <0.00%> (-83.24%) ⬇️
cli/server.go 54.33% <39.68%> (-7.72%) ⬇️
codersdk/users.go 65.67% <50.00%> (-1.00%) ⬇️
coderd/userauth.go 58.33% <58.33%> (ø)
coderd/users.go 60.65% <71.92%> (+1.16%) ⬆️
coderd/httpmw/apikey.go 81.30% <80.00%> (-3.05%) ⬇️
coderd/httpmw/oauth2.go 82.71% <82.71%> (ø)
cli/cliflag/cliflag.go 100.00% <100.00%> (ø)
coderd/coderd.go 97.47% <100.00%> (+0.12%) ⬆️
coderd/coderdtest/coderdtest.go 93.83% <100.00%> (-5.04%) ⬇️
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3976994...aa9e5b7. Read the comment docs.

@kylecarbs kylecarbs requested a review from deansheather April 17, 2022 20:45
@kylecarbs kylecarbs self-assigned this Apr 17, 2022
@kylecarbs kylecarbs requested review from presleyp and a team as code owners April 17, 2022 22:25
@kylecarbs
Copy link
Member Author

@presleyp this is my first time working with XState, so it's very possible I did something wrong... just let me know!

@kylecarbs kylecarbs mentioned this pull request Apr 18, 2022
Closed
greyscaled
greyscaled reviewed Apr 18, 2022
View reviewed changes
Copy link
Contributor

@greyscaled greyscaled left a comment

Choose a reason for hiding this comment

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

This is really basic right now, but figured we could pretty it up in a future PR (happy to do it here if y'all would like).

I think it's the right call to re-design later, and move things like strings and icons to the backend response. What's here looks awesome for now, nicely done!

This is awesome 🎉 !

@presleyp
Copy link
Contributor

Looks great! Can you add stories to SignInForm.stories.tsx for when github oauth is on? (I'm thinking add the happy case, switch the loading story to show both so we can see how each handles loading, and maybe the error case even though it doesn't do anything, just to prove we still have a working form)

@kylecarbs
Copy link
Member Author

@presleyp I added the stories listed, and now display an error for auth methods. I noticed right now we don't display the backend error for responses, and I'm curious if we should do that instead? Regardless, I followed the existing style and added it to Language.

I also improved the tests with mocks and checked for the error state.

@presleyp
Copy link
Contributor

I noticed right now we don't display the backend error for responses, and I'm curious if we should do that instead?

Yeah, I think so!

Thanks for adding stories!

deansheather
deansheather reviewed Apr 22, 2022
View reviewed changes
@kylecarbs kylecarbs enabled auto-merge (squash) April 23, 2022 19:59
@kylecarbs kylecarbs disabled auto-merge April 23, 2022 20:02
@kylecarbs
Copy link
Member Author

@deansheather approved via Slack ☑

@kylecarbs kylecarbs enabled auto-merge (squash) April 23, 2022 20:02
@kylecarbs kylecarbs merged commit 7496c3d into main Apr 23, 2022
@kylecarbs kylecarbs deleted the oauth branch April 23, 2022 22:58
This was referenced Apr 25, 2022
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greyscaled greyscaled greyscaled left review comments

@deansheather deansheather deansheather left review comments

@presleyp presleyp presleyp approved these changes

Assignees

@kylecarbs kylecarbs

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kylecarbs @presleyp @greyscaled @deansheather