feat: implement standardized OAuth2 endpoints and token revocation by ThomasK33 · Pull Request #18809 · coder/coder

ThomasK33 · 2025-07-09T12:02:57Z

feat: standardize OAuth2 endpoints and add token revocation

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 libraries

feat: implement OAuth2 Device Authorization Grant (RFC 8628)

Add device authorization endpoint /oauth2/device
Add device verification UI at /oauth2/device/verify
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 flow

chore: add OAuth2 device flow test scripts

Change-Id: Ic232851727e683ab3d8b7ce970c505588da2f827
Change-Id: I7a7eebeb23a4f28718ebed2994d01dc21b49315b
Signed-off-by: Thomas Kosiewski tk@coder.com

Summary by CodeRabbit

New Features
- Added full support for OAuth2 Device Authorization Grant (device code flow), including device authorization, verification, and token exchange endpoints.
- Introduced OAuth2 token revocation endpoint, allowing secure revocation of refresh tokens and access tokens.
- Updated API documentation and schemas to include device authorization and revocation endpoints.
- Enhanced UI with new device authorization and result pages, including dedicated CSS for OAuth2 device flows.
Bug Fixes
- Corrected OAuth2 token endpoint paths to align with standard specifications.
Documentation
- Expanded API and developer documentation to cover OAuth2 device flow and token revocation processes.
Tests
- Added comprehensive tests for device flow, token revocation, and RBAC permissions related to OAuth2.
Style
- Introduced new CSS for OAuth2 device authorization pages and updated HTML templates for improved user experience.
Chores
- Updated scripts and fixtures to support and test the new OAuth2 device flow and revocation features.

ThomasK33 · 2025-07-09T12:03:16Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

aslilac

I'm afraid this pr is large enough to be very 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.

aslilac · 2025-08-12T17:03:26Z

+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"
+)


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.

aslilac · 2025-08-12T17:05:39Z

+			}
+			_ = resp.Body.Close()
+
+			if errorCode, ok := result["error"].(string); ok {


this should probably use codersdk.AsError

aslilac · 2025-08-12T17:09:04Z

 // NOTE: OAuth2 client registration validation tests have been migrated to
 // oauth2provider/validation_test.go for better separation of concerns


feels weird to leave this comment in the middle of such a huge file. maybe move it to the top actually?

aslilac · 2025-08-12T17:11:16Z

+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)';


I think we generally tend to be more conservative than this with SQL comments. some of these seem a bit overkill, like status which is an enum type and pretty self explanatory.

aslilac · 2025-08-12T17:12:03Z

+    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)


similarly here, I don't think this needs to be rationalized. enums are a great choice!

aslilac · 2025-08-12T17:14:01Z

@@ -0,0 +1,8 @@
+-- Remove OAuth2 Device Authorization Grant support (RFC 8628)
+
+-- Remove constraints added for data integrity


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 · 2025-09-05T13:58:19Z

@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 libraries feat: 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 flow Change-Id: I7a7eebeb23a4f28718ebed2994d01dc21b49315b Signed-off-by: Thomas Kosiewski <tk@coder.com>

Change-Id: Ic232851727e683ab3d8b7ce970c505588da2f827 Signed-off-by: Thomas Kosiewski <tk@coder.com>

Emyrk · 2025-10-17T14:21:18Z

+type parsedAPIKey struct {
+	keyID  string // The API key ID for database lookup
+	secret string // The secret part for verification
+}
+
+// parseAPIKeyToken parses an API key token following the encoder/decoder pattern
+func parseAPIKeyToken(token string) (parsedAPIKey, error) {
+	parts := strings.SplitN(token, "-", 2)
+	if len(parts) != 2 {
+		return parsedAPIKey{}, xerrors.Errorf("incorrect number of parts: %d", len(parts))
+	}
+	if parts[0] == "" || parts[1] == "" {
+		return parsedAPIKey{}, xerrors.New("empty key ID or secret")
+	}
+	return parsedAPIKey{
+		keyID:  parts[0],
+		secret: parts[1],
+	}, nil
+}


We should not reimplement

coder/coderd/httpmw/apikey.go

Lines 746 to 767 in f3d950d

// SplitAPIToken verifies the format of an API key and returns the split ID and

// secret.

//

// APIKeys are formatted: ${ID}-${SECRET}

func SplitAPIToken(token string) (id string, secret string, err error) {

parts := strings.Split(token, "-")

if len(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]

if len(keyID) != 10 {

return "", "", xerrors.Errorf("invalid API key ID length, expected 10 got %d", len(keyID))

}

if len(keySecret) != 22 {

return "", "", xerrors.Errorf("invalid API key secret length, expected 22 got %d", len(keySecret))

}

return keyID, keySecret, nil

}

(I am fixing)

Emyrk · 2025-10-17T14:22:40Z

+func revokeAPIKeyInTx(ctx context.Context, db database.Store, token string, appID uuid.UUID) error {
+	// Parse the API key using the structured decoder
+	parsedKey, err := parseAPIKeyToken(token)
+	if err != nil {
+		return ErrInvalidTokenFormat
+	}
+
+	// Get the API key
+	//nolint:gocritic // Using AsSystemOAuth2 for OAuth2 public token revocation endpoint
+	apiKey, err := db.GetAPIKeyByID(dbauthz.AsSystemOAuth2(ctx), parsedKey.keyID)
+	if err != nil {
+		if errors.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
+			return nil
+		}
+		return xerrors.Errorf("get api key by id: %w", err)
+	}
+
+	// Verify the API key was created by OAuth2
+	if apiKey.LoginType != database.LoginTypeOAuth2ProviderApp {
+		return xerrors.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)
+	if err != nil {
+		if errors.Is(err, sql.ErrNoRows) {
+			// No associated OAuth2 token - return success per RFC 7009
+			return nil
+		}
+		return xerrors.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)
+	if err != nil {
+		return xerrors.Errorf("get oauth2 provider app secret for api key verification: %w", err)
+	}
+
+	if appSecret.AppID != appID {
+		return ErrTokenNotBelongsToClient
+	}
+
+	// Delete the API key
+	//nolint:gocritic // Using AsSystemOAuth2 for OAuth2 public token revocation endpoint
+	err = db.DeleteAPIKeyByID(dbauthz.AsSystemOAuth2(ctx), apiKey.ID)
+	if err != nil && !errors.Is(err, sql.ErrNoRows) {
+		return xerrors.Errorf("delete api key for revocation: %w", err)
+	}
+
+	return nil
 }


Don't we need to verify the api key secret is correct?

This only checks the id

Emyrk · 2025-10-17T14:33:40Z

+				if err != 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)
+				}


Why do we not fail the test?

Emyrk · 2025-10-17T14:44:28Z

+				err = client.RevokeOAuth2Token(ctx, s.app.ID.String(), token.RefreshToken, "refresh_token")
+				if err != 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)
+				}


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?

mafredri · 2026-01-26T11:04:15Z

Removing my review due to PR being stale, feel free to add me back if this becomes relevant again.

Emyrk · 2026-01-26T13:24:46Z

some of this is done, I'm going to close this

github-actions Bot assigned ThomasK33 Jul 9, 2025

ThomasK33 mentioned this pull request Jul 9, 2025

refactor: replace OAuth2 callback_url with RFC 6749 compliant redirect_uris #18810

Closed

ThomasK33 changed the title ~~feat: standardize OAuth2 endpoints and add token revocation~~ feat: standardize OAuth2 endpoints and implement token revocation Jul 9, 2025

ThomasK33 force-pushed the thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation branch 7 times, most recently from bc44107 to ab73979 Compare July 9, 2025 22:15

ThomasK33 marked this pull request as ready for review July 10, 2025 09:20

ThomasK33 requested a review from johnstcn July 10, 2025 09:24

ThomasK33 mentioned this pull request Jul 10, 2025

feat: add cleanup for expired OAuth2 provider app codes and tokens #18825

Closed

ThomasK33 requested review from Emyrk and mafredri July 10, 2025 11:10

ThomasK33 force-pushed the thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation branch from ab73979 to b89c367 Compare July 12, 2025 12:55

ThomasK33 mentioned this pull request Jul 12, 2025

feat: add OAuth2 client credentials grant type and user ownership #18841

Closed

ThomasK33 force-pushed the thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation branch from b89c367 to b73b71d Compare July 14, 2025 12:43

This was referenced Jul 14, 2025

feat: add client credentials OAuth2 applications for API access #18846

Closed

feat: add OAuth2 token bulk revocation endpoint #18847

Closed

ThomasK33 force-pushed the thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation branch 2 times, most recently from 514f744 to 386d77d Compare July 14, 2025 16:13

ThomasK33 mentioned this pull request Jul 14, 2025

docs: remove dbmem references from documentation files #18861

Merged

ThomasK33 force-pushed the thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation branch 3 times, most recently from 01f48f3 to 9393060 Compare July 15, 2025 16:30

ThomasK33 mentioned this pull request Jul 15, 2025

fix: allow non-HTTP URIs in OAuth2 provider redirect URIs #18880

Closed

ThomasK33 requested a review from dannykopping July 17, 2025 12:52

ThomasK33 force-pushed the thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation branch from 9393060 to 2c31819 Compare July 17, 2025 14:38

ThomasK33 removed their assignment Aug 12, 2025

ThomasK33 force-pushed the thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation branch from aca2f6a to 35d7f5a Compare August 12, 2025 16:49

ThomasK33 changed the title ~~feat: standardize OAuth2 endpoints and implement token revocation~~ feat: implement standardized OAuth2 endpoints and token revocation Aug 12, 2025

aslilac reviewed Aug 12, 2025

View reviewed changes

github-actions Bot added the stale This issue is like stale bread. label Aug 20, 2025

github-actions Bot closed this Aug 24, 2025

Emyrk reopened this Aug 24, 2025

Emyrk requested a review from buenos-nachos as a code owner August 24, 2025 22:11

github-actions Bot removed the stale This issue is like stale bread. label Aug 25, 2025

blink-so Bot mentioned this pull request Aug 27, 2025

OAuth client registration fails with custom scheme redirect URIs #19581

Open

github-actions Bot added the stale This issue is like stale bread. label Sep 1, 2025

github-actions Bot closed this Sep 5, 2025

buenos-nachos reopened this Sep 5, 2025

ThomasK33 removed the stale This issue is like stale bread. label Sep 24, 2025

ThomasK33 force-pushed the thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation branch from 35d7f5a to 8b4b6f0 Compare October 10, 2025 14:42

chore: add OAuth2 device flow test scripts

81adc67

Change-Id: Ic232851727e683ab3d8b7ce970c505588da2f827 Signed-off-by: Thomas Kosiewski <tk@coder.com>

ThomasK33 force-pushed the thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation branch from 8b4b6f0 to 81adc67 Compare October 10, 2025 14:53

Emyrk reviewed Oct 17, 2025

View reviewed changes

Emyrk mentioned this pull request Oct 17, 2025

feat: implement oauth2 RFC 7009 token revocation endpoint #20362

Merged

mafredri removed request for buenos-nachos and mafredri January 26, 2026 11:04

Emyrk closed this Jan 26, 2026

github-actions Bot deleted the thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation branch April 11, 2026 00:14

		// NOTE: OAuth2 client registration validation tests have been migrated to
		// oauth2provider/validation_test.go for better separation of concerns

		@@ -0,0 +1,8 @@
		-- Remove OAuth2 Device Authorization Grant support (RFC 8628)

		-- Remove constraints added for data integrity

	// SplitAPIToken verifies the format of an API key and returns the split ID and
	// secret.
	//
	// APIKeys are formatted: ${ID}-${SECRET}
	func SplitAPIToken(token string) (id string, secret string, err error) {
	parts := strings.Split(token, "-")
	if len(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]
	if len(keyID) != 10 {
	return "", "", xerrors.Errorf("invalid API key ID length, expected 10 got %d", len(keyID))
	}
	if len(keySecret) != 22 {
	return "", "", xerrors.Errorf("invalid API key secret length, expected 22 got %d", len(keySecret))
	}

	return keyID, keySecret, nil
	}

Conversation

ThomasK33 commented Jul 9, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

ThomasK33 commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aslilac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

buenos-nachos commented Sep 5, 2025

Uh oh!

Emyrk Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mafredri commented Jan 26, 2026

Uh oh!

Emyrk commented Jan 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ThomasK33 commented Jul 9, 2025 •

edited by coderabbitai Bot

Loading

ThomasK33 commented Jul 9, 2025 •

edited

Loading

Emyrk Oct 17, 2025 •

edited

Loading