feat: add oauth2 token exchange #11778
Conversation
9d071ac to
a60af2c
Compare
a41182d to
6a157e9
Compare
6a157e9 to
becb1a3
Compare
7d10b9e to
29a9e26
Compare
becb1a3 to
80fa017
Compare
There was a problem hiding this comment.
Consolidating and adding some hash function notes.
We should use userpasword.Hash()
The user secret is currently 40 random characters, let's spruce that up.
- Add a
coder_prefix so a random key in a text file is more easily recognized. - Add an
idsection of 10(?) random characters with a unique constraint. - Optionally... if you want to be really cool, add a 2 character checksum at the end, so if someone hand types this we can detect typos 😉
coderd/httpapi/queryparams.go
Outdated
| @@ -233,7 +246,7 @@ func ParseCustomList[T any](parser *QueryParamParser, vals url.Values, def []T, | |||
| func parseQueryParam[T any](parser *QueryParamParser, vals url.Values, parse func(v string) (T, error), def T, queryParam string) (T, error) { | |||
| parser.addParsed(queryParam) | |||
| // If the query param is required and not present, return an error. | |||
| if parser.RequiredParams[queryParam] && (!vals.Has(queryParam)) { | |||
| if parser.RequiredParams[queryParam] && (!vals.Has(queryParam) || vals.Get(queryParam) == "") { | |||
There was a problem hiding this comment.
Why did we add vals.Get(queryParam) == ""? The empty string is a valid value for parsing params here. If it is not valid for your use case, we should solve it somewhere else.
I think this could break something 🤔
| if parser.RequiredParams[queryParam] && (!vals.Has(queryParam) || vals.Get(queryParam) == "") { | |
| if parser.RequiredParams[queryParam] && (!vals.Has(queryParam)) { |
There was a problem hiding this comment.
Yeah I had mixed feelings about this; it felt surprising to me that I could write Required("client_id") and it could come through empty. My thinking was that if I want to require a query param I also want a value.
I did check existing usages; it is used for reconnect on the terminal (which is invalid if blank) and for start_time and end_time on an insights endpoint which also cannot be blank (it will fail to be parsed into a time).
But at the same time I see the distinction between required and empty. An empty query value is probably only useful as a boolean where the presence of the key means true, and I could see requiring a true for something like ?acceptTerms or something like that before you can use the endpoint.
So let me rename this to RequiredNotEmpty, that will leave room for Required if we do need that some day.
There was a problem hiding this comment.
Some new ones popped up after I merged main: template_id, workspace_id, and agent_id on some endpoints, they all make database queries with these values so I think they would just return "no rows" or a 404 or whatever, so this change will make it return "invalid query params, template_id is required" instead which I think is fair, but it does change the semantics a little from a 404 to a 400.
|
Moved to a draft while I work on:
|
a6b9de7 to
3623f69
Compare
765f71c to
d530bb0
Compare
3623f69 to
9e8200b
Compare
f862651 to
911660a
Compare
0cf4f55 to
6f03c33
Compare
911660a to
eb4ca9e
Compare
And rename another, it is not being used incorrectly, but the name makes it clear.
cd5fb8f to
19fa030
Compare
e6d7102 to
cae4e61
Compare
|
Rebased, but only commits from Delete some oidc helper comments onward are new. They should all be self-contained enough to review individually, if that appeals. I really like the idea of a checksum but I have not implemented it yet. I was reading up some check digit algorithms and thinking maybe of using crc8 or something but got hit with decision paralysis so I am leaving it for the moment. 😓 |
The referer check was not quite right either.
|
|
||
| // RevokeOAuth2ProviderApp completely revokes an app's access for the user. | ||
| func (c *Client) RevokeOAuth2ProviderApp(ctx context.Context, appID uuid.UUID) error { | ||
| res, err := c.Request(ctx, http.MethodDelete, "/login/oauth2/tokens", nil, func(r *http.Request) { |
There was a problem hiding this comment.
I wonder if we should hardcode this to /login/oauth2/me/tokens for now to allow us supporting revoking other user's in the future.
There was a problem hiding this comment.
Oh really good point, let me change it now real quick.
There was a problem hiding this comment.
Oh right I had it here to have the post/delete paradigm on the same endpoint. Maybe I should actually move this back to /api/v2 since it is our own thing, not part of the whole oauth flow.
There was a problem hiding this comment.
This is not an oauth2 spec endpoint right?
Eh just keep it here and comment that it is not oauth 2 spec. If you feel this has to be a static endpoint though, then just comment why it should be. I'll defer to your judgement
There was a problem hiding this comment.
Yup exactly, it is just our own thing and not part of the spec. I do not feel strongly about it, just felt kinda right to have posts and deletes on the same endpoint. Hmmm lemme give it some thought!
This adds a new identity provider package and hooks it into coderd routes along with some test refactoring. See commit messages for more details.
There is more work to be done (tracking in #11609) but I think this is fully featured enough to be merged in. All the routes are only accessible in dev mode.
It is getting pretty big, although 60% is HTML, tests, and generated code!
Blocked by:
Blocks: