- Notifications
You must be signed in to change notification settings - Fork928
feat: Add OIDC authentication#3314
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This PR made my day |
| "docker", | ||
| "run", | ||
| "--rm", | ||
| "--network=host", | ||
| "postgres:13", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This was a bit out of scope, but makes everything use Docker, which makes changing our versions simpler!
| @@ -81,71 +81,6 @@ func TestRead(t *testing.T) { | |||
| }) | |||
| } | |||
| func TestReadUsername(t *testing.T) { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
These have been moved to theusername package!
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Ammar Bandukwala <ammar@ammar.io>
Co-authored-by: Ammar Bandukwala <ammar@ammar.io>
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| var user database.User | ||
| user, err = api.Database.GetUserByEmailOrUsername(r.Context(), database.GetUserByEmailOrUsernameParams{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is pretty unlikely to happen but if an existing, unrelated user (say withbuilt-in auth) already has this email address, then it's possible for a new OIDC user to impersonate them by providing the same email address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yup. The issue I attached expands on that a bit. We'll have to make the user-identification with account links better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do we want to add a column to the users table that indicates the sort of authentication they used? Probably not necessary to do in this PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We will in a future PR, this one just gets it out the door
coderd/username/username.go Outdated
| @@ -0,0 +1,45 @@ | |||
| package username | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I like the idea of extracting validation to a separate package but what sort of precedent do we want to be setting if we have other fields that may be suitable for relocation to a new package? Are we expecting each new field to create its own pkg, to refactor into a commonvalidate pkg or do you feel likeusername is sufficiently unique to warrant its own pkg and most everything else should be contained tocoderd?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I feel like this isn't just a case of validation. I extracted this primarily due to the generation that we now need to do with OIDC.
The alternative was exposinghttpapi.ValidateUsername or something of the sort, which I'm also mostly fine with. Keep in mind we'd have to have a separate function ofhttpapi.UsernameFromString or something like it, but I'm also fine with that. I'll think on it, but if you think that sounds cleaner I'll make it happen!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nah I was just bringing it up to get an idea of how we want to structure packages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I actually removed the username package. It felt too much like floaty scope with little benefit. That code is inhttpapi now, which feels better anyways.
Good comment 👍
Uh oh!
There was an error while loading.Please reload this page.
This adds OIDC authentication to the product!
The email and username cannot be used as unique, so we'll have to add an identifier to match a user. See:https://openid.net/specs/openid-connect-core-1_0.html#ClaimStability.#3322 has been created to fix this.
Closes#695.
oidc-2022-07-31_14.28.48.mp4
Most of the diff is extracting the username validation into its package; the OIDC exchange is just a few hundred lines. This was manually tested with Google, but considering the extent of tests, it'll hopefully work with others!