- Notifications
You must be signed in to change notification settings - Fork1.1k
feat: implement standardized OAuth2 endpoints and token revocation#18809
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
base:main
Are you sure you want to change the base?
feat: implement standardized OAuth2 endpoints and token revocation#18809
Conversation
ThomasK33 commentedJul 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
bc44107 toab73979Compareab73979 tob89c367Compareb89c367 tob73b71dCompare514f744 to386d77dCompare01f48f3 to9393060Compare9393060 to2c31819Compareb436ec8 toaca2f6aCompareaca2f6a to35d7f5aCompare
aslilac left a comment
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'm afraid this pr is large enough to bevery difficult to review 😅 any ideas on how we could break it up?
I would call out the size even for something half this line count. I think the ideal diff size for getting good reviews is under 1k.
| const ( | ||
| // ANSI color codes | ||
| colorReset="\033[0m" | ||
| colorRed="\033[31m" | ||
| colorGreen="\033[32m" | ||
| colorYellow="\033[33m" | ||
| colorBlue="\033[34m" | ||
| colorPurple="\033[35m" | ||
| colorCyan="\033[36m" | ||
| colorWhite="\033[37m" | ||
| ) |
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 see other places in the codebase where we use github.com/muesli/termenv for this, which will do a better job of not flooding the output with noise when stdout isn't a tty. it's maybe not a big deal for something in scripts/, but we already pull in the dependency. might as well use it.
| } | ||
| _=resp.Body.Close() | ||
| iferrorCode,ok:=result["error"].(string);ok { |
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 should probably usecodersdk.AsError
| // NOTE: OAuth2 client registration validation tests have been migrated to | ||
| // oauth2provider/validation_test.go for better separation of concerns |
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.
feels weird to leave this comment in the middle of such a huge file. maybe move it to the top actually?
| COMMENT ON TABLE oauth2_provider_device_codes IS 'RFC 8628: OAuth2 Device Authorization Grant device codes'; | ||
| COMMENT ON COLUMN oauth2_provider_device_codes.device_code_hash IS 'Hashed device code for security'; | ||
| COMMENT ON COLUMN oauth2_provider_device_codes.device_code_prefix IS 'Device code prefix for lookup (first 8 chars)'; | ||
| COMMENT ON COLUMN oauth2_provider_device_codes.user_code IS 'Human-readable code shown to user (6-8 characters)'; | ||
| COMMENT ON COLUMN oauth2_provider_device_codes.verification_uri IS 'URI where user enters user_code'; | ||
| COMMENT ON COLUMN oauth2_provider_device_codes.verification_uri_complete IS 'Optional complete URI with user_code embedded'; | ||
| COMMENT ON COLUMN oauth2_provider_device_codes.polling_interval IS 'Minimum polling interval in seconds (RFC 8628)'; | ||
| COMMENT ON COLUMN oauth2_provider_device_codes.resource_uri IS 'RFC 8707 resource parameter for audience restriction'; | ||
| COMMENT ON COLUMN oauth2_provider_device_codes.status IS 'Current authorization status: pending (awaiting user action), authorized (user approved), or denied (user rejected)'; |
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 think we generally tend to be more conservative than this with SQL comments. some of these seem a bit overkill, likestatus which is an enum type and pretty self explanatory.
| client_id uuid NOT NULL REFERENCES oauth2_provider_apps(id) ON DELETE CASCADE, | ||
| user_id uuid REFERENCES users(id) ON DELETE CASCADE, -- NULL until authorized | ||
| -- Authorization state (using enum for better data integrity) |
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.
similarly here, I don't think this needs to be rationalized. enums are a great choice!
| @@ -0,0 +1,8 @@ | |||
| -- Remove OAuth2 Device Authorization Grant support (RFC 8628) | |||
| -- Remove constraints added for data integrity | |||
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 also kinda weird. I imagine some agent inserted this? saying "this was added for data integrity" as you destroy it is just tonally odd.
buenos-nachos commentedSep 5, 2025
@ThomasK33@Emyrk Sorry, somehow missed the first ping from two weeks ago. Where are we at on the PR? Do we just need another pass on the frontend? |
- Change /oauth2/tokens → /oauth2/token per RFC 6749 - Move token deletion to POST /oauth2/revoke per RFC 7009 - Update all endpoint URLs and documentation - Maintain backward compatibility in client librariesfeat: implement OAuth2 Device Authorization Grant (RFC 8628) - Add device authorization endpoint /oauth2/device/authorize - Add device verification UI at /oauth2/device - Support device_code grant type in token endpoint - Add database table for device codes with expiration - Implement polling interval and user authorization flow - Add comprehensive test coverage for device flowChange-Id: I7a7eebeb23a4f28718ebed2994d01dc21b49315bSigned-off-by: Thomas Kosiewski <tk@coder.com>
35d7f5a to8b4b6f0CompareChange-Id: Ic232851727e683ab3d8b7ce970c505588da2f827Signed-off-by: Thomas Kosiewski <tk@coder.com>
8b4b6f0 to81adc67Compare| typeparsedAPIKeystruct { | ||
| keyIDstring// The API key ID for database lookup | ||
| secretstring// The secret part for verification | ||
| } | ||
| // parseAPIKeyToken parses an API key token following the encoder/decoder pattern | ||
| funcparseAPIKeyToken(tokenstring) (parsedAPIKey,error) { | ||
| parts:=strings.SplitN(token,"-",2) | ||
| iflen(parts)!=2 { | ||
| returnparsedAPIKey{},xerrors.Errorf("incorrect number of parts: %d",len(parts)) | ||
| } | ||
| ifparts[0]==""||parts[1]=="" { | ||
| returnparsedAPIKey{},xerrors.New("empty key ID or secret") | ||
| } | ||
| returnparsedAPIKey{ | ||
| keyID:parts[0], | ||
| secret:parts[1], | ||
| },nil | ||
| } |
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 should not reimplement
Lines 746 to 767 inf3d950d
| // SplitAPIToken verifies the format of an API key and returns the split ID and | |
| // secret. | |
| // | |
| // APIKeys are formatted: ${ID}-${SECRET} | |
| funcSplitAPIToken(tokenstring) (idstring,secretstring,errerror) { | |
| parts:=strings.Split(token,"-") | |
| iflen(parts)!=2 { | |
| return"","",xerrors.Errorf("incorrect amount of API key parts, expected 2 got %d",len(parts)) | |
| } | |
| // Ensure key lengths are valid. | |
| keyID:=parts[0] | |
| keySecret:=parts[1] | |
| iflen(keyID)!=10 { | |
| return"","",xerrors.Errorf("invalid API key ID length, expected 10 got %d",len(keyID)) | |
| } | |
| iflen(keySecret)!=22 { | |
| return"","",xerrors.Errorf("invalid API key secret length, expected 22 got %d",len(keySecret)) | |
| } | |
| returnkeyID,keySecret,nil | |
| } |
(I am fixing)
| funcrevokeAPIKeyInTx(ctx context.Context,db database.Store,tokenstring,appID uuid.UUID)error { | ||
| // Parse the API key using the structured decoder | ||
| parsedKey,err:=parseAPIKeyToken(token) | ||
| iferr!=nil { | ||
| returnErrInvalidTokenFormat | ||
| } | ||
| // Get the API key | ||
| //nolint:gocritic // Using AsSystemOAuth2 for OAuth2 public token revocation endpoint | ||
| apiKey,err:=db.GetAPIKeyByID(dbauthz.AsSystemOAuth2(ctx),parsedKey.keyID) | ||
| iferr!=nil { | ||
| iferrors.Is(err,sql.ErrNoRows) { | ||
| // API key not found - return success per RFC 7009 (don't reveal token existence) | ||
| // Note: This covers both non-existent keys and invalid key ID formats | ||
| returnnil | ||
| } | ||
| returnxerrors.Errorf("get api key by id: %w",err) | ||
| } | ||
| // Verify the API key was created by OAuth2 | ||
| ifapiKey.LoginType!=database.LoginTypeOAuth2ProviderApp { | ||
| returnxerrors.New("API key is not an OAuth2 token") | ||
| } | ||
| // Find the associated OAuth2 token to verify ownership | ||
| //nolint:gocritic // Using AsSystemOAuth2 for OAuth2 public token revocation endpoint | ||
| dbToken,err:=db.GetOAuth2ProviderAppTokenByAPIKeyID(dbauthz.AsSystemOAuth2(ctx),apiKey.ID) | ||
| iferr!=nil { | ||
| iferrors.Is(err,sql.ErrNoRows) { | ||
| // No associated OAuth2 token - return success per RFC 7009 | ||
| returnnil | ||
| } | ||
| returnxerrors.Errorf("get oauth2 provider app token by api key id: %w",err) | ||
| } | ||
| // Verify the token belongs to the requesting app | ||
| //nolint:gocritic // Using AsSystemOAuth2 for OAuth2 public token revocation endpoint | ||
| appSecret,err:=db.GetOAuth2ProviderAppSecretByID(dbauthz.AsSystemOAuth2(ctx),dbToken.AppSecretID) | ||
| iferr!=nil { | ||
| returnxerrors.Errorf("get oauth2 provider app secret for api key verification: %w",err) | ||
| } | ||
| ifappSecret.AppID!=appID { | ||
| returnErrTokenNotBelongsToClient | ||
| } | ||
| // Delete the API key | ||
| //nolint:gocritic // Using AsSystemOAuth2 for OAuth2 public token revocation endpoint | ||
| err=db.DeleteAPIKeyByID(dbauthz.AsSystemOAuth2(ctx),apiKey.ID) | ||
| iferr!=nil&&!errors.Is(err,sql.ErrNoRows) { | ||
| returnxerrors.Errorf("delete api key for revocation: %w",err) | ||
| } | ||
| returnnil | ||
| } |
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.
Don't we need to verify the api key secret is correct?
This only checks theid
| iferr!=nil { | ||
| // Log the error for debugging, but don't fail the test | ||
| t.Logf("Token revocation error (this is expected for now): %v",err) | ||
| t.Logf("Client ID: %s, Token: %s",s.app.ID.String(),token.RefreshToken) | ||
| } |
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.
Why do we not fail the test?
| err=client.RevokeOAuth2Token(ctx,s.app.ID.String(),token.RefreshToken,"refresh_token") | ||
| iferr!=nil { | ||
| // Log the error for debugging, but don't fail the test | ||
| t.Logf("Token revocation error (this is expected for now): %v",err) | ||
| t.Logf("Client ID: %s, Token: %s",s.app.ID.String(),token.RefreshToken) | ||
| } |
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 test passes if you comment out
err=client.RevokeOAuth2Token(ctx,s.app.ID.String(),token.RefreshToken,"refresh_token")
So I don't think this is actually testing anything?
Uh oh!
There was an error while loading.Please reload this page.
feat: standardize OAuth2 endpoints and add token revocation
feat: implement OAuth2 Device Authorization Grant (RFC 8628)
chore: add OAuth2 device flow test scripts
Change-Id: Ic232851727e683ab3d8b7ce970c505588da2f827
Change-Id: I7a7eebeb23a4f28718ebed2994d01dc21b49315b
Signed-off-by: Thomas Kosiewskitk@coder.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Style
Chores