feat: Add OIDC authentication #3314
Conversation
|
This PR made my day |
| "docker", | ||
| "run", | ||
| "--rm", | ||
| "--network=host", | ||
| "postgres:13", |
There was a problem hiding this comment.
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.
These have been moved to the username package!
Co-authored-by: Ammar Bandukwala <ammar@ammar.io>
Co-authored-by: Ammar Bandukwala <ammar@ammar.io>
| } | ||
|
|
||
| var user database.User | ||
| user, err = api.Database.GetUserByEmailOrUsername(r.Context(), database.GetUserByEmailOrUsernameParams{ |
There was a problem hiding this comment.
This is pretty unlikely to happen but if an existing, unrelated user (say with built-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.
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.
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.
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.
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 common validate pkg or do you feel like username is sufficiently unique to warrant its own pkg and most everything else should be contained to coderd?
There was a problem hiding this comment.
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 exposing httpapi.ValidateUsername or something of the sort, which I'm also mostly fine with. Keep in mind we'd have to have a separate function of httpapi.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.
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.
I actually removed the username package. It felt too much like floaty scope with little benefit. That code is in httpapi now, which feels better anyways.
Good comment 👍
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!