- Notifications
You must be signed in to change notification settings - Fork948
feat: standardize OAuth2 endpoints and implement 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: standardize OAuth2 endpoints and implement 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 to2c31819CompareThere 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 don't think we need to duplicate the styles and svg in each html file like this. we could just put a css file next to them in the static folder and reference it, and we already have icons in there.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
- 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>
2c31819 to212e8ebCompare212e8eb to165e7dcComparecoderd/database/dbauthz/dbauthz.go Outdated
| // Fetch the device code first to check authorization | ||
| deviceCode, err := q.db.GetOAuth2ProviderDeviceCodeByID(ctx, id) | ||
| if err != nil { | ||
| return err |
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.
Wrap these errs please.
| @@ -1275,6 +1275,25 @@ func OAuth2ProviderAppToken(t testing.TB, db database.Store, seed database.OAuth | |||
| return token | |||
| } | |||
| func OAuth2ProviderDeviceCode(t testing.TB, db database.Store, seed database.OAuth2ProviderDeviceCode) database.OAuth2ProviderDeviceCode { | |||
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.
Should probably be marked as at.Helper(), no?
coderd/database/dbgen/dbgen.go Outdated
| ResourceUri: seed.ResourceUri, | ||
| PollingInterval: takeFirst(seed.PollingInterval, 5), | ||
| }) | ||
| require.NoError(t, err, "insert oauth2 device code") |
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.
Probably safer as anassert in case this is called from a goroutine.
BTW both comments here could apply to the other helper funcs in this file, so my suggestions are optional if consistency is what we want, but the others are likely wrong.
| device_code_hash bytea NOT NULL UNIQUE, | ||
| device_code_prefix text NOT NULL UNIQUE CHECK (length(device_code_prefix) = 8), | ||
| -- User code (human-readable, 6-8 chars) |
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 need aCHECK for this?
Can we documentwhy these lengths are specified?
| @@ -129,6 +129,11 @@ SELECT * FROM oauth2_provider_app_codes WHERE id = $1; | |||
| -- name: GetOAuth2ProviderAppCodeByPrefix :one | |||
| SELECT * FROM oauth2_provider_app_codes WHERE secret_prefix = $1; | |||
| -- name: ConsumeOAuth2ProviderAppCodeByPrefix :one | |||
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.
Since you're only expecting one row back, you should add aLIMIT 1 just to be safe.
| // Verify ownership | ||
| // nolint:gocritic // Using AsSystemRestricted is necessary for OAuth2 public token revocation endpoint | ||
| appSecret, err := db.GetOAuth2ProviderAppSecretByID(dbauthz.AsSystemRestricted(ctx), dbToken.AppSecretID) |
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.
Really feels like we need a new role instead of overloading the system one.
coderd/oauth2provider/revoke.go Outdated
| // nolint:gocritic // Using AsSystemRestricted is necessary for OAuth2 public token revocation endpoint | ||
| err = db.DeleteAPIKeyByID(dbauthz.AsSystemRestricted(ctx), dbToken.APIKeyID) | ||
| if err != nil && !errors.Is(err, sql.ErrNoRows) { | ||
| return err |
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.
Wrap errors please.
| func revokeAPIKey(ctx context.Context, db database.Store, token string, appID uuid.UUID) error { | ||
| // Parse the API key ID from the token (format: <id>-<secret>) | ||
| parts := strings.SplitN(token, "-", 2) |
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.
Nit: I'd prefer a key encoder/decoder pattern.
coderd/oauth2provider/tokens.go Outdated
| errBadDeviceCode = xerrors.New("Invalid device code") | ||
| // errAuthorizationPending means the user hasn't authorized the device yet. | ||
| errAuthorizationPending = xerrors.New("authorization pending") |
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.
Nit: use consistent capitalisation.
Uh oh!
There was an error while loading.Please reload this page.
Change-Id: Ic232851727e683ab3d8b7ce970c505588da2f827Signed-off-by: Thomas Kosiewski <tk@coder.com>
165e7dc tod655a2dComparecoderabbitaibot commentedJul 21, 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.
WalkthroughThis update introduces comprehensive support for the OAuth2 Device Authorization Grant (RFC 8628) and OAuth2 Token Revocation (RFC 7009) across the backend, SDK, API documentation, database schema, and frontend. It adds new endpoints, database tables, RBAC permissions, client SDK methods, test suites, and user-facing UI for device authorization, while removing deprecated token deletion endpoints and correcting token endpoint URLs to align with OAuth2 standards. Changes
Sequence Diagram(s)sequenceDiagram participant Device as Device (CLI/IoT) participant User as User (Browser) participant Server as OAuth2 Server participant DB as Database Device->>Server: POST /oauth2/device (client_id, scope, resource) Server->>DB: Store device_code, user_code, status=pending Server-->>Device: device_code, user_code, verification_uri User->>Server: GET /oauth2/device/verify?user_code Server-->>User: Verification page (enter/confirm code) User->>Server: POST /oauth2/device/verify (user_code, action) Server->>DB: Update device_code status (authorized/denied) Server-->>User: Success or denied page Device->>Server: POST /oauth2/token (grant_type=device_code, device_code) Server->>DB: Check device_code status alt status == authorized Server->>DB: Consume device_code, create tokens Server-->>Device: access_token, refresh_token else status == pending Server-->>Device: error=authorization_pending else status == denied Server-->>Device: error=access_denied endsequenceDiagram participant Client as OAuth2 Client participant Server as OAuth2 Server participant DB as Database Client->>Server: POST /oauth2/revoke (client_id, token, token_type_hint) Server->>DB: Lookup token ownership alt Valid and owned by client Server->>DB: Revoke token (delete/cascade) Server-->>Client: 200 OK else Not found or not owned Server-->>Client: 200 OK (silent) endEstimated code review effort5 (>120 minutes) This is a large, high-complexity change set affecting many files across backend, frontend, SDK, tests, documentation, and scripts, with new features, schema, RBAC, and compliance requirements. Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat withCodeRabbit:
SupportNeed help? Create a ticket on oursupport page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File ( |
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.
Actionable comments posted: 13
🔭 Outside diff range comments (1)
docs/reference/api/enterprise.md (1)
19-30:Addrevocation_endpointto AS metadata exampleRFC 8414 lists
revocation_endpointalongsidedevice_authorization_endpoint.
Including it here prevents downstream generators/SDKs from ignoring the revocation URL."device_authorization_endpoint": "string",+ "revocation_endpoint": "string",
♻️ Duplicate comments (11)
site/static/oauth2device_success.html (1)
16-18:Mirror accessibility fix applied to the success pageSame rationale as denial page—hide decorative glyphs from assistive technology.
-<div>✓</div>+<div aria-hidden="true">✓</div>coderd/database/dbgen/dbgen.go (1)
1279-1298:Excellent implementation addressing past review feedback.The new
OAuth2ProviderDeviceCodefunction correctly:
- Uses
t.Helper()as suggested in previous reviews- Uses
assert.NoErrorinstead ofrequire.NoErrorfor safer error handling when called from goroutines- Follows the established pattern of other dbgen functions with sensible defaults
coderd/database/queries/oauth2.sql (3)
288-289:Consider adding LIMIT 1 for consistency.While
device_code_prefixhas a UNIQUE constraint, addingLIMIT 1would be consistent with the codebase patterns and defensive programming practices.-- name: GetOAuth2ProviderDeviceCodeByPrefix :one-SELECT * FROM oauth2_provider_device_codes WHERE device_code_prefix = $1;+SELECT * FROM oauth2_provider_device_codes WHERE device_code_prefix = $1 LIMIT 1;
296-297:Consider adding LIMIT 1 for consistency.While
user_codehas a case-insensitive unique constraint, addingLIMIT 1would be consistent with defensive programming practices.-- name: GetOAuth2ProviderDeviceCodeByUserCode :one-SELECT * FROM oauth2_provider_device_codes WHERE user_code = $1;+SELECT * FROM oauth2_provider_device_codes WHERE user_code = $1 LIMIT 1;
291-294:Add LIMIT 1 to ensure atomic consumption.The consume operation should include
LIMIT 1to ensure exactly one device code is consumed atomically, even in edge cases.-- name: ConsumeOAuth2ProviderDeviceCodeByPrefix :oneDELETE FROM oauth2_provider_device_codes-WHERE device_code_prefix = $1 AND expires_at > NOW() AND status = 'authorized'-RETURNING *;+WHERE device_code_prefix = $1 AND expires_at > NOW() AND status = 'authorized'+RETURNING *+LIMIT 1;coderd/oauth2provider/revoke.go (5)
22-27:Good use of sentinel errors as requested.The error definitions properly implement sentinel errors using
xerrors.New, addressing the previous review feedback.
80-102:Excellent error handling and logging implementation.The error handling properly addresses the previous review comments by:
- Adding debug logs for token ownership mismatch (line 82-84)
- Adding debug logs for invalid token format (line 90-92)
- Adding error logs for internal server errors (line 96-99)
This follows RFC 7009 requirements while providing proper observability.
117-119:Consider documenting the rationale for AsSystemRestricted usage.While the use of
AsSystemRestrictedis justified for this public OAuth2 endpoint (as it needs to access tokens across different users), the previous review suggested using a new role instead. Consider:
- Adding a more detailed comment explaining why system-restricted access is necessary here
- Creating a dedicated OAuth2 provider role in a follow-up PR to address the architectural concern
The current implementation is functionally correct for the OAuth2 revocation endpoint requirements.
Also applies to: 128-130, 139-141
148-167:Good implementation of the key encoder/decoder pattern.This implementation properly addresses the previous review comment requesting a key encoder/decoder pattern. The
parsedAPIKeystruct andparseAPIKeyTokenfunction provide a clean abstraction for API key parsing.
169-223:Well-structured API key revocation with proper error handling.The implementation correctly:
- Uses the new decoder pattern for parsing
- Validates OAuth2 ownership through proper database queries
- Wraps errors with descriptive context as requested in past reviews
- Maintains RFC 7009 compliance by returning success for non-existent tokens
coderd/oauth2provider/tokens.go (1)
31-35:Good fix for the capitalization consistency issue.The comments now follow a consistent lowercase style as previously requested.
🧹 Nitpick comments (19)
.claude/scripts/format.sh (1)
109-114:Passfile_pathexplicitly for better function purity.
run_formattercurrently relies on the globalfile_pathvariable. Passing the path as an argument makes the helper easier to reuse/test and eliminates hidden dependencies.-run_formatter() {-local-local file_type="$2"+run_formatter() {+local+local file_type="$2"+local path="$3" ...-if ! make FILE="$file_path" "$target"; then+if ! make FILE="$path" "$target"; then ... echo "✓ Formatted $file_type file: $path"}And update the call sites, e.g.:
-run_formatter "fmt/go" "Go"+run_formatter "fmt/go" "Go" "$file_path"site/static/oauth2device.html (1)
42-49:Minor accessibility tweak for secondary actionConsider adding
formnovalidateto the “Deny” button so browsers don’t block the negative path when the input field is empty.- <button type="submit" name="action" value="deny">+ <button type="submit" name="action" value="deny" formnovalidate>scripts/oauth2/test-device-flow.sh (2)
1-3:Harden the script error handling flags
set -ealone misses failures in pipes and unbound variables. Strengthen the safety net.-set -e+set -euo pipefail
21-27:Echoing instructions leaks nothing, but exit code 1 loses color resetWhen the credential check fails the script exits while the terminal is still in red. Add a final
echo -e "${NC}"ortput sgr0beforeexit 1so subsequent shell prompts aren’t affected.site/static/oauth2device_denied.html (1)
16-18:Provide an accessible label for the denial iconScreen readers interpret “✗” literally. Either hide it from AT or give it a role/label.
-<div>✗</div>+<div aria-hidden="true">✗</div>coderd/database/dbauthz/dbauthz_test.go (2)
5539-5545:Avoidtime.Now()for test-data uniqueness
uniquePrefixis derived fromtime.Now().UnixNano().
Parallel test execution can still produce identical timestamps at nanosecond
resolution, causing a rare but painful flake when two prefixes collide.-uniquePrefix := fmt.Sprintf("prefix-%s-%d", s.T().Name(), time.Now().UnixNano())+uniquePrefix := uuid.NewString() // 36 chars and collision-free(
uuid.NewString()is already in the test’s imports.)
Same remark applies touniqueUserCode.
5683-5691:Prefix length logic is brittle
fmt.Sprintf("t%07d", time.Now().UnixNano()%10000000)hits the 8-char target,
but the%07dmodulo math obscures intent and relies on the magic constant10 000 000.Consider a clearer approach:
random:=rand.Intn(1_0000_000)// 0-9999 999prefix:=fmt.Sprintf("t%07d",random)Or switch to a cryptographically-safe 8-byte base-32 string generated with
crypto/randfor better test coverage of non-numeric prefixes.coderd/oauth2provider/device.go (1)
206-209:Add audit logging for device authorization actions.Consider adding audit log entries when users authorize or deny device codes for security tracking and compliance.
docs/reference/api/schemas.md (1)
4718-4735:Reference typo & missing normative wording in request schemaThe description “Resource 8707 resource parameter” should reference RFC 8707 explicitly and clarify semantics. also
resourceandscopeare optional but SHOULD follow respective RFCs.-| `resource` | string | false | | Resource 8707 resource parameter |+| `resource` | string | false | | Resource indicator (RFC 8707). |Nit: wrap RFC numbers in parentheses for uniformity.
coderd/oauth2provider/revoke.go (1)
66-68:Consider documenting the token prefix convention.The logic for distinguishing refresh tokens from API keys based on the "coder_" prefix works but could benefit from:
- A comment explaining this is a Coder-specific convention
- Consider defining the prefix as a package-level constant that can be reused across the codebase
This would make the codebase more maintainable if the prefix convention needs to change in the future.
coderd/database/dump.sql (1)
2875-2876:Case-insensitive uniqueness achieved, but considercitext
CREATE UNIQUE INDEX oauth2_device_codes_user_code_ci_idx ON ... (upper(user_code));This works, but Postgres’
citextmodule offers identical semantics with less mental overhead and simpler queries (WHERE user_code = 'ABCD'). Migrating tocitextis optional but improves ergonomics.coderd/oauth2provider/tokens.go (1)
641-648:Consider documenting the app secret selection strategy.The code uses the first (most recent) app secret when multiple secrets exist. This is likely correct, but consider adding a comment explaining this choice for future maintainers.
-// Use the first (most recent) app secret+// Use the first (most recent) app secret - secrets are ordered by creation time DESCappSecret := appSecrets[0]coderd/oauth2provider/revoke_test.go (2)
319-322:Consider implementing API key revocation verification.The TODO comment indicates that actual API key revocation verification is not yet implemented. This could leave a gap in test coverage.
Would you like me to help implement the API key revocation verification logic or create an issue to track this task?
490-512:Good security test coverage with room for enhancement.The test covers important security scenarios including path traversal and XSS attempts. Consider adding a test for SQL injection attempts in token values.
Add SQL injection test case:
maliciousTokens := []string{ "coder_", // Missing prefix and secret "coder__secret", // Empty prefix "coder_prefix_", // Missing secret "../../../etc/passwd", // Path traversal attempt "<script>alert('xss')</script>", // XSS attempt strings.Repeat("a", 10000), // Very long token+ "'; DROP TABLE oauth2_tokens; --", // SQL injection attempt "", // Empty token (already covered but included for completeness) }coderd/database/dbauthz/dbauthz.go (1)
1572-1586:Consider using thedeleteQhelper for consistency.While the current implementation is functionally correct, it could be refactored to use the
deleteQhelper pattern for consistency with other delete methods in the codebase (e.g.,DeleteAPIKeyByID,DeleteGroupByID).func (q *querier) DeleteOAuth2ProviderDeviceCodeByID(ctx context.Context, id uuid.UUID) error {-// Fetch the device code first to check authorization-deviceCode, err := q.db.GetOAuth2ProviderDeviceCodeByID(ctx, id)-if err != nil {-return xerrors.Errorf("get oauth2 provider device code: %w", err)-}-if err := q.authorizeContext(ctx, policy.ActionDelete, deviceCode); err != nil {-return xerrors.Errorf("authorize oauth2 provider device code deletion: %w", err)-}--if err := q.db.DeleteOAuth2ProviderDeviceCodeByID(ctx, id); err != nil {-return xerrors.Errorf("delete oauth2 provider device code: %w", err)-}-return nil+return deleteQ(q.log, q.auth, q.db.GetOAuth2ProviderDeviceCodeByID, q.db.DeleteOAuth2ProviderDeviceCodeByID)(ctx, id) }coderd/oauth2_test.go (1)
741-746:Token revocation test appears incomplete.The error handling for
RevokeOAuth2Tokenincludes a comment indicating "this is expected for now", and the test skips the "app should disappear" check. This suggests the revocation functionality might not be fully implemented yet.Would you like me to help implement the complete token revocation test or create an issue to track this TODO?
coderd/apidoc/swagger.json (1)
2189-2246:Verify that the verification page truly needs an API session token.
RequiringCoderSessionTokenmakes sense only if users must already be signed in.
If anonymous users enter a one-time code (common in device flows), drop the security requirement for GET and rely on CSRF protection for POST.coderd/oauth2_spec_test.go (1)
135-137:Consider removing debug logging from test.The debug logging statements might clutter test output. Consider removing them or using
t.Logfonly when test fails.-// Debug PKCE values-t.Logf("PKCE Test Values:")-t.Logf(" verifier: %q", verifier)-t.Logf("Exchanging code with verifier: %q", verifier)Also applies to: 147-147
codersdk/oauth2.go (1)
475-478:Consider improving request body handling.The pattern of passing
nilbody toRequestand then reassigningr.Bodyin the callback might be confusing. Consider passing the body directly to the Request method.-res, err := c.Request(ctx, http.MethodPost, "/oauth2/device", nil, func(r *http.Request) {+res, err := c.Request(ctx, http.MethodPost, "/oauth2/device", strings.NewReader(form.Encode()), func(r *http.Request) { r.Header.Set("Content-Type", "application/x-www-form-urlencoded")-r.Body = io.NopCloser(strings.NewReader(form.Encode())) })Also applies to: 502-505
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (60)
.claude/scripts/format.sh(1 hunks)coderd/apidoc/docs.go(8 hunks)coderd/apidoc/swagger.json(8 hunks)coderd/coderd.go(2 hunks)coderd/database/db2sdk/db2sdk.go(1 hunks)coderd/database/dbauthz/dbauthz.go(7 hunks)coderd/database/dbauthz/dbauthz_test.go(2 hunks)coderd/database/dbgen/dbgen.go(4 hunks)coderd/database/dbmetrics/querymetrics.go(6 hunks)coderd/database/dbmock/dbmock.go(6 hunks)coderd/database/dump.sql(6 hunks)coderd/database/foreign_key_constraint.go(1 hunks)coderd/database/migrations/000350_oauth2_device_authorization.down.sql(1 hunks)coderd/database/migrations/000350_oauth2_device_authorization.up.sql(1 hunks)coderd/database/migrations/testdata/fixtures/000350_oauth2_device_codes.up.sql(1 hunks)coderd/database/modelmethods.go(1 hunks)coderd/database/models.go(2 hunks)coderd/database/querier.go(6 hunks)coderd/database/queries.sql.go(5 hunks)coderd/database/queries/oauth2.sql(2 hunks)coderd/database/sqlc.yaml(2 hunks)coderd/database/unique_constraint.go(2 hunks)coderd/httpapi/httpapi.go(7 hunks)coderd/httpmw/oauth2.go(2 hunks)coderd/mcp/mcp_e2e_test.go(5 hunks)coderd/oauth2.go(4 hunks)coderd/oauth2_spec_test.go(1 hunks)coderd/oauth2_test.go(13 hunks)coderd/oauth2provider/authorize.go(4 hunks)coderd/oauth2provider/device.go(1 hunks)coderd/oauth2provider/metadata.go(1 hunks)coderd/oauth2provider/oauth2providertest/helpers.go(2 hunks)coderd/oauth2provider/oauth2providertest/oauth2_test.go(1 hunks)coderd/oauth2provider/provider_test.go(2 hunks)coderd/oauth2provider/revoke.go(1 hunks)coderd/oauth2provider/revoke_test.go(1 hunks)coderd/oauth2provider/tokens.go(13 hunks)coderd/rbac/object_gen.go(1 hunks)coderd/rbac/policy/policy.go(1 hunks)coderd/rbac/roles_test.go(1 hunks)codersdk/oauth2.go(9 hunks)codersdk/oauth2_validation.go(1 hunks)codersdk/rbacresources_gen.go(1 hunks)docs/reference/api/enterprise.md(9 hunks)docs/reference/api/schemas.md(5 hunks)scripts/oauth2/README.md(3 hunks)scripts/oauth2/device/server.go(1 hunks)scripts/oauth2/oauth2-test-server.go(1 hunks)scripts/oauth2/setup-test-app.sh(1 hunks)scripts/oauth2/test-device-flow.sh(1 hunks)scripts/oauth2/test-mcp-oauth2.sh(4 hunks)site/site.go(3 hunks)site/src/api/rbacresourcesGenerated.ts(1 hunks)site/src/api/typesGenerated.ts(4 hunks)site/src/testHelpers/entities.ts(1 hunks)site/static/oauth2.css(1 hunks)site/static/oauth2allow.html(2 hunks)site/static/oauth2device.html(1 hunks)site/static/oauth2device_denied.html(1 hunks)site/static/oauth2device_success.html(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (13)
coderd/oauth2provider/oauth2providertest/oauth2_test.go (1)
coderd/util/slice/slice.go (1)
Contains(63-67)
codersdk/rbacresources_gen.go (2)
coderd/rbac/object_gen.go (1)
ResourceOauth2AppCodeToken(190-192)coderd/rbac/policy/policy.go (4)
ActionCreate(9-9)ActionDelete(12-12)ActionRead(10-10)ActionUpdate(11-11)
codersdk/oauth2_validation.go (1)
codersdk/oauth2.go (1)
OAuth2ProviderGrantTypeDeviceCode(200-200)
coderd/database/modelmethods.go (5)
coderd/database/dbgen/dbgen.go (1)
OAuth2ProviderDeviceCode(1279-1297)coderd/database/models.go (1)
OAuth2ProviderDeviceCode(3288-3311)coderd/rbac/object.go (1)
Object(24-42)coderd/rbac/object_gen.go (1)
ResourceOauth2AppCodeToken(190-192)codersdk/rbacresources_gen.go (1)
ResourceOauth2AppCodeToken(27-27)
coderd/rbac/roles_test.go (3)
coderd/database/types.go (1)
Actions(45-45)coderd/rbac/policy/policy.go (5)
Action(6-6)ActionCreate(9-9)ActionRead(10-10)ActionUpdate(11-11)ActionDelete(12-12)codersdk/rbacresources_gen.go (4)
ActionCreate(52-52)ActionRead(56-56)ActionUpdate(60-60)ActionDelete(54-54)
coderd/database/db2sdk/db2sdk.go (1)
coderd/externalauth/externalauth.go (1)
DeviceAuth(348-355)
coderd/oauth2provider/metadata.go (1)
codersdk/oauth2.go (1)
OAuth2ProviderGrantTypeDeviceCode(200-200)
coderd/rbac/policy/policy.go (1)
codersdk/rbacresources_gen.go (1)
ActionUpdate(60-60)
coderd/oauth2provider/authorize.go (1)
site/src/api/typesGenerated.ts (1)
ValidationError(3411-3414)
coderd/database/querier.go (4)
testutil/ctx.go (1)
Context(9-13)coderd/database/dbgen/dbgen.go (1)
OAuth2ProviderDeviceCode(1279-1297)coderd/database/models.go (1)
OAuth2ProviderDeviceCode(3288-3311)coderd/database/queries.sql.go (2)
InsertOAuth2ProviderDeviceCodeParams(6315-6328)UpdateOAuth2ProviderDeviceCodeAuthorizationParams(6612-6616)
site/src/api/typesGenerated.ts (1)
codersdk/oauth2.go (6)
OAuth2AuthorizationServerMetadata(254-265)OAuth2DeviceAuthorizationRequest(573-577)OAuth2DeviceAuthorizationResponse(580-587)OAuth2DeviceFlowCallbackResponse(249-251)OAuth2DeviceVerificationRequest(590-592)OAuth2ProviderGrantType(195-195)
coderd/database/dbmetrics/querymetrics.go (5)
testutil/ctx.go (1)
Context(9-13)coderd/database/dbgen/dbgen.go (1)
OAuth2ProviderDeviceCode(1279-1297)coderd/database/models.go (1)
OAuth2ProviderDeviceCode(3288-3311)coderd/database/dbtime/dbtime.go (1)
Now(6-8)coderd/database/queries.sql.go (2)
InsertOAuth2ProviderDeviceCodeParams(6315-6328)UpdateOAuth2ProviderDeviceCodeAuthorizationParams(6612-6616)
coderd/database/dbmock/dbmock.go (4)
testutil/ctx.go (1)
Context(9-13)coderd/database/dbgen/dbgen.go (2)
OAuth2ProviderAppCode(1246-1261)OAuth2ProviderDeviceCode(1279-1297)coderd/database/models.go (2)
OAuth2ProviderAppCode(3245-3259)OAuth2ProviderDeviceCode(3288-3311)coderd/database/queries.sql.go (2)
InsertOAuth2ProviderDeviceCodeParams(6315-6328)UpdateOAuth2ProviderDeviceCodeAuthorizationParams(6612-6616)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: test-e2e
- GitHub Check: offlinedocs
- GitHub Check: build-dylib
- GitHub Check: chromatic
- GitHub Check: test-go-pg (windows-2022)
- GitHub Check: test-go-pg (macos-latest)
- GitHub Check: test-go-pg-17
- GitHub Check: test-go-race-pg
- GitHub Check: test-go-pg (ubuntu-latest)
- GitHub Check: lint
- GitHub Check: check-docs
🔇 Additional comments (146)
.claude/scripts/format.sh (1)
104-108:Nice extraction of a reusable helper.Encapsulating the repeated
makeinvocation inrun_formatterremoves duplication and makes the switch-case blocks far cleaner.scripts/oauth2/oauth2-test-server.go (1)
184-184:LGTM! OAuth2 token endpoint standardized correctly.The endpoint path has been correctly updated from
/oauth2/tokensto/oauth2/tokento align with RFC 6749 specifications. This change ensures the test server uses the standardized token endpoint.coderd/oauth2provider/oauth2providertest/oauth2_test.go (1)
51-51:LGTM! Test assertion updated for standardized endpoint.The test assertion correctly validates the standardized OAuth2 token endpoint path
/oauth2/token, ensuring the authorization server metadata contains the RFC 6749 compliant endpoint.coderd/oauth2provider/oauth2providertest/helpers.go (1)
211-211:LGTM! Test helper functions updated for standardized endpoint.Both token exchange helper functions (
ExchangeCodeForTokenandPerformTokenExchangeExpectingError) have been correctly updated to use the standardized/oauth2/tokenendpoint. This ensures all OAuth2 test scenarios use the RFC 6749 compliant token endpoint.Also applies to: 278-278
site/src/testHelpers/entities.ts (1)
4253-4258:Confirm type sync & propagate change across mocksGreat to see the mock updated with the new RFC-7009 revocation endpoint.
Please double-check two things:
TypesGen.OAuth2ProviderApp["endpoints"]must now declare therevocationkey (or make it optional). Otherwise this addition will break type-checking / TS build.- If you keep multiple
OAuth2ProviderAppfixtures elsewhere, add the samerevocationfield so tests that iterate over the collection don’t hitundefined.No code change required here if the above already happened; just sanity-check and update the remaining mocks if necessary.
coderd/httpmw/oauth2.go (1)
215-215:LGTM! Endpoint documentation correctly updated to match RFC 6749.The comment corrections from
/oauth2/tokensto/oauth2/tokenproperly align with the OAuth2 RFC 6749 specification, which defines the token endpoint as singular. This documentation update is consistent with the broader endpoint standardization effort in this PR.Also applies to: 292-292
coderd/rbac/object_gen.go (1)
189-189:Please verify RBAC policy source definitions include the new update actionThe generated file now lists
ActionUpdateforResourceOauth2AppCodeToken. Before approving, confirm that the policy source used by the generator (e.g. files undercoderd/rbac/policy/) has been updated to includeActionUpdateforoauth2_app_code_token. If it’s missing there, add it to the source so future code generations don’t overwrite this permission.scripts/oauth2/test-mcp-oauth2.sh (1)
90-96:LGTM! Token endpoint URLs consistently updated throughout the test script.All OAuth2 token endpoint references have been correctly changed from
/oauth2/tokensto/oauth2/tokento align with RFC 6749 standards. The changes are consistent across:
- PKCE token exchange (line 90)
- Invalid PKCE negative test (line 115)
- Resource parameter flow (line 142)
- Token refresh request (line 160)
The test script will now work correctly with the standardized endpoints.
Also applies to: 115-121, 142-148, 160-165
scripts/oauth2/setup-test-app.sh (1)
20-32:LGTM! Improved request formatting and API schema compliance.The changes enhance the OAuth2 app creation request in two positive ways:
- Better formatting: Using here-document with
--data-binary @-improves readability over inline JSON- Enhanced API compliance: Adding
"redirect_uris"as an array reflects updated OAuth2 app registration schema that supports multiple redirect URIsThe script maintains backward compatibility by keeping the existing
"callback_url"field while adding the more standard"redirect_uris"array format.site/src/api/rbacresourcesGenerated.ts (1)
105-105:Generated frontend RBAC definitions correctly updated.The addition of the "update" action for
oauth2_app_code_tokenproperly reflects the backend RBAC policy changes and enables frontend clients to perform update operations on OAuth2 app code tokens. This generated code change is consistent with the comprehensive OAuth2 device flow and token management functionality being added.coderd/rbac/policy/policy.go (1)
294-294:LGTM: Consistent RBAC permission addition.The addition of
ActionUpdatepermission tooauth2_app_code_tokenaligns with the existing pattern where related OAuth2 resources (oauth2_appandoauth2_app_secret) already have full CRUD permissions including update. This change supports the OAuth2 device authorization functionality being implemented.codersdk/rbacresources_gen.go (1)
91-91:LGTM: Generated code correctly reflects policy updates.The addition of
ActionUpdatetoResourceOauth2AppCodeToken's allowed actions is consistent with the policy definition incoderd/rbac/policy/policy.goand maintains the correct mapping of RBAC permissions for OAuth2 app code tokens.coderd/database/migrations/000350_oauth2_device_authorization.down.sql (1)
1-8:LGTM: Well-structured rollback migration.The down migration correctly reverses the OAuth2 device authorization schema changes:
- Uses
IF EXISTSclauses to prevent errors during rollback- Properly drops the device codes table with
CASCADEto handle dependencies- Removes the constraint and enum type in the correct order
- Clear comments explain the purpose of each operation
codersdk/oauth2_validation.go (1)
162-162:LGTM: Enables device code grant type validation.The addition of
OAuth2ProviderGrantTypeDeviceCodeto the valid grant types correctly implements support for RFC 8628 (OAuth2 Device Authorization Grant) in the client registration validation. This aligns with the broader OAuth2 device flow implementation in this PR.coderd/database/modelmethods.go (1)
415-423:LGTM: Well-designed RBAC method for device codes.The
RBACObject()method correctly handles the two states of OAuth2 device codes:
- Authorized codes (with valid UserID) are owned by the authorizing user
- Pending codes (without valid UserID) are treated as system resources without specific ownership
This design aligns with the OAuth2 device flow where codes start as system resources and become user-owned after authorization. The use of
ResourceOauth2AppCodeTokenis appropriate since device codes function similarly to OAuth2 tokens in the RBAC context.coderd/rbac/roles_test.go (1)
645-647:LGTM! Correctly adds Update action to OAuth2Token RBAC test.The addition of
policy.ActionUpdateto the allowed actions for OAuth2 tokens properly expands the RBAC test coverage to include update operations, which aligns with the OAuth2 enhancements being introduced in this PR.coderd/database/db2sdk/db2sdk.go (1)
367-371:LGTM! OAuth2 endpoint corrections align with RFC standards.The changes correctly:
- Fix the token endpoint path from
/oauth2/tokensto/oauth2/tokenper RFC 6749- Add device authorization endpoint
/oauth2/device/authorizeto support RFC 8628 device flowThese updates properly standardize the OAuth2 implementation and enable the new device authorization grant functionality.
coderd/database/migrations/testdata/fixtures/000350_oauth2_device_codes.up.sql (1)
1-20:LGTM! Comprehensive test fixture for OAuth2 device authorization.The fixture provides well-structured test data including:
- Proper UUID values for IDs
- Realistic timestamps with 10-minute expiry
- Device code hash and prefix for security
- User-friendly user code (ABCD1234)
- Complete verification URIs for device flow
- Pending status for testing authorization workflow
This enables thorough testing of the OAuth2 device authorization grant functionality.
coderd/oauth2provider/metadata.go (1)
18-22:LGTM! OAuth2 metadata correctly updated for standards compliance.The changes properly implement OAuth2 standards:
- Token endpoint corrected to
/oauth2/tokenper RFC 6749- Device authorization endpoint added at
/oauth2/deviceper RFC 8628- Device code grant type added to supported grant types using the proper constant
These metadata updates ensure clients can discover and use the OAuth2 endpoints correctly.
site/static/oauth2allow.html (1)
10-25:LGTM! Good refactoring to external CSS and semantic classes.The updates improve the template by:
- Linking to external
/oauth2.cssstylesheet for better maintainability- Adding semantic CSS classes (
narrow allow-page,large,compact) for consistent styling- Aligning with the new OAuth2 device authorization UI design
This creates a more cohesive and maintainable OAuth2 user interface across the application.
coderd/database/foreign_key_constraint.go (1)
34-35:LGTM – new FK constants align with the migrationThe generated constants correctly reflect the new
client_idanduser_idconstraints foroauth2_provider_device_codes. No further action needed.coderd/database/unique_constraint.go (1)
41-43:LGTM! Code-generated unique constraint constants for OAuth2 device codes.The new unique constraint constants follow the established naming convention and properly define database constraints for the OAuth2 device authorization table.
coderd/mcp/mcp_e2e_test.go (5)
455-459:OAuth2 token endpoint URL corrected to comply with RFC 6749.The endpoint has been correctly updated from
/oauth2/tokensto/oauth2/token(singular) as per OAuth2 specification.
611-611:OAuth2 token endpoint URL corrected for authorization code exchange.Consistent update to use the standardized
/oauth2/tokenendpoint.
709-709:OAuth2 token endpoint URL corrected for token refresh request.Properly updated to use the RFC 6749 compliant endpoint path.
934-934:OAuth2 token endpoint URL corrected for dynamic client registration flow.Maintains consistency with the standardized token endpoint.
1035-1035:OAuth2 token endpoint URL corrected for token refresh in dynamic client flow.Final endpoint correction completing the standardization across all OAuth2 flows.
scripts/oauth2/README.md (3)
105-137:Excellent documentation for OAuth2 device flow testing.The new documentation provides comprehensive details about the device authorization flow test script and Go program, including usage instructions, features, and implementation details.
162-177:Well-structured example workflow for device authorization flow.The step-by-step workflow clearly demonstrates how to test the OAuth2 device authorization grant, making it easy for developers to understand and use.
198-199:OAuth2 endpoints documentation updated correctly.The token endpoint has been corrected to
/oauth2/token(singular) per RFC 6749, and the new device authorization endpoint has been added.coderd/database/sqlc.yaml (2)
88-90:Proper security measure for sensitive device code field.The
json:"-"struct tag correctly prevents the sensitivedevice_code_hashfield from being serialized in JSON responses, which is a good security practice.
147-151:Consistent Go type renames for OAuth2 device types.The new type renames follow the established naming convention and properly map the database types to Go types for the OAuth2 device authorization feature.
coderd/oauth2provider/provider_test.go (3)
40-40:Updated test case with appropriately long name for validation testing.The longer test string properly exceeds the 64-character limit to validate the length restriction.
120-170:Comprehensive test coverage for valid OAuth2 app display names.Excellent test cases covering various valid name formats including spaces, special characters, parentheses, dashes, numbers, and single words. This ensures the validation accepts legitimate app names.
172-213:Thorough test coverage for invalid OAuth2 app display names.Well-designed test cases that verify rejection of names with leading/trailing spaces and names exceeding the character limit. This ensures proper validation of edge cases.
coderd/oauth2provider/authorize.go (4)
22-27:LGTM! Constants address previous feedback.The addition of
AccessTypeOnlineandAccessTypeOfflineconstants directly addresses the past review comment requesting constants instead of hardcoded strings. This improves code maintainability and readability.
37-37:Good addition following existing patterns.The
accessTypefield is correctly added to theauthorizeParamsstruct with appropriate documentation and follows the same pattern as other OAuth2 parameters.
55-55:Correct parameter extraction implementation.The
access_typeparameter extraction follows the established pattern and correctly uses an empty string as the default value for this optional OAuth2 parameter.
65-72:Well-implemented validation logic.The validation correctly handles the optional
access_typeparameter by allowing empty values and restricting non-empty values to the defined constants. The error message construction using constants ensures consistency and maintainability.coderd/coderd.go (3)
948-957:Excellent OAuth2 standards compliance.The token endpoint route correction from
/tokensto/tokenproperly aligns with RFC 6749. The middleware configuration and explanatory comments demonstrate a solid understanding of OAuth2 client authentication requirements.
959-968:Proper RFC 7009 token revocation implementation.The new
/revokeendpoint correctly implements OAuth2 token revocation per RFC 7009, using appropriate OAuth2 client authentication middleware and following the standard POST method pattern.
983-992:Correct RFC 8628 device authorization implementation.The device authorization endpoints properly implement the OAuth2 Device Authorization Grant flow per RFC 8628. The middleware choices are appropriate: no authentication for the client-initiated
/deviceendpoint and API key with redirect for user-facing/device/verifyendpoints.coderd/database/dbgen/dbgen.go (2)
20-20:Good addition aligning with past review feedback.Adding the assert import supports the previous review comment suggesting safer error handling using assert instead of require when called from goroutines.
1250-1250:LGTM! Consistent 24-hour expiration defaults.The expiration time defaults for both OAuth2ProviderAppCode and OAuth2ProviderAppToken are now consistently set to 24 hours, which provides a reasonable default for testing.
Also applies to: 1267-1267
coderd/httpapi/httpapi.go (3)
52-57:Validation tag update looks correct.The removal of
oauth2_app_nameand addition ofoauth2_app_display_nameto the display name validator aligns with the OAuth2 enhancements and SDK changes mentioned in the summary.Also applies to: 68-73
191-191:Good modernization usinganytype.Replacing
interface{}withanyfollows modern Go idioms and improves code readability.Also applies to: 214-214, 236-236
344-344:Performance optimization in SSE formatting.Good optimization replacing
fmt.Sprintfwithfmt.Appendfand usingfmt.Fprintfdirectly to the buffer, avoiding unnecessary string allocations.Also applies to: 361-361
site/static/oauth2.css (1)
1-265:Well-structured CSS with good accessibility considerations.The CSS provides a clean, consistent dark theme for OAuth2 device authorization pages with:
- Proper visual hierarchy and spacing
- Clear differentiation between success/denied states
- Good accessibility choices (avoiding confusing characters in the charset comment)
- Responsive design with appropriate max-widths
coderd/oauth2.go (3)
110-110:Good API documentation improvements.Adding explicit content-type annotations and correcting the token endpoint path to
/oauth2/tokenimproves RFC 6749 compliance.Also applies to: 126-127, 150-150
155-166:Correct implementation of RFC 7009 token revocation.The new POST
/oauth2/revokeendpoint properly implements OAuth2 token revocation with appropriate form data parameters and HTTP 200 response on success.
235-271:Comprehensive OAuth2 device flow endpoints.The three new endpoints properly implement RFC 8628:
- Device authorization request with JSON request/response
- Device verification page (GET) and processing (POST) with proper session authentication
- Clear API documentation with appropriate content types
coderd/oauth2provider/device.go (3)
42-144:DeviceAuthorization implementation looks secure and compliant.The implementation correctly:
- Validates content-type as required by RFC 8628
- Uses UUID validation for client_id
- Generates cryptographically secure codes
- Stores device codes with proper expiration
- Returns compliant response format
241-244:Good defensive programming with status verification.The double-check of the updated status helps prevent race conditions where multiple requests might try to process the same device code simultaneously.
330-343:Excellent user code generation with accessibility in mind.The user code generation correctly:
- Avoids confusing characters (0, O, I, 1) for better readability
- Uses cryptographically secure random generation
- Formats as XXXX-XXXX for easy human input
coderd/database/querier.go (1)
68-69:LGTM! OAuth2 device code interface methods are well-structured.The new interface methods for OAuth2 device authorization follow consistent naming patterns and cover the complete device code lifecycle appropriately.
Also applies to: 90-90, 102-102, 243-246, 513-514, 600-600
coderd/database/queries/oauth2.sql (3)
132-136:Good implementation of atomic code consumption.The query correctly implements an atomic consume operation with proper expiration checking and includes the
LIMIT 1clause as previously suggested.
302-308:Excellent implementation of device code authorization update.The query correctly enforces state transitions (only 'pending' codes can be updated) and includes
LIMIT 1for atomic updates.
313-315:Good cleanup implementation for expired device codes.The query correctly targets only expired 'pending' codes for deletion, preserving authorized/denied codes for potential audit purposes.
coderd/database/migrations/000350_oauth2_device_authorization.up.sql (2)
14-14:CHECK constraints are properly implemented with good documentation.The length constraints for
device_code_prefix(8 chars) anduser_code(6-8 chars) are well-documented with comments explaining the rationale, addressing the previous review feedback.Also applies to: 17-17
43-44:Excellent implementation of RFC 8628 case-insensitive user code requirement.The unique index on
UPPER(user_code)correctly enforces case-insensitive uniqueness as required by the OAuth2 device authorization specification.site/site.go (2)
62-104:Device flow template initialization follows established patterns.The template variables and initialization in
init()are consistent with the existing OAuth template setup, with proper error handling.
1183-1234:Device page rendering functions are well-implemented.The rendering functions follow consistent patterns with proper content-type headers and error handling that matches the existing OAuth page rendering.
scripts/oauth2/device/server.go (2)
124-170:Device code request implementation correctly follows RFC 8628.The function properly uses basic authentication for client credentials and handles both OAuth2 error responses and HTTP errors appropriately.
172-279:Excellent implementation of OAuth2 device flow token polling.The function correctly handles all OAuth2 device flow error codes, implements proper backoff behavior for
slow_downresponses, and respects the minimum polling interval as per RFC 8628.coderd/database/models.go (2)
1182-1241:OAuth2DeviceStatus enum implementation looks good.The enum correctly implements the three authorization states (pending, authorized, denied) required by RFC 8628, with proper scanning and validation methods following the established pattern in this file.
3287-3311:OAuth2ProviderDeviceCode struct is well-implemented.The struct properly implements RFC 8628 device authorization with appropriate security measures (DeviceCodeHash with
json:"-"tag) and clear field documentation. All required fields for the device flow are present with correct types.site/src/api/typesGenerated.ts (5)
1539-1539:LGTM: OAuth2 revocation endpoint additionThe addition of the
revocationendpoint to theOAuth2AppEndpointsinterface correctly implements RFC 7009 OAuth2 Token Revocation support.
1547-1547:LGTM: Device authorization endpoint in server metadataThe optional
device_authorization_endpointfield correctly implements RFC 8628 OAuth2 Device Authorization Grant server metadata.
1631-1655:LGTM: OAuth2 device flow interfacesThe new interfaces correctly implement RFC 8628 OAuth2 Device Authorization Grant:
OAuth2DeviceAuthorizationRequestproperly includes requiredclient_idand optionalscope/resourcefieldsOAuth2DeviceAuthorizationResponsecontains all standard device flow response fieldsOAuth2DeviceVerificationRequestprovides the necessary user code verification interfaceAll field names and types align with OAuth2 specifications.
1706-1715:LGTM: Device code grant type additionThe addition of
"urn:ietf:params:oauth:grant-type:device_code"to the OAuth2 grant types correctly implements RFC 8628 Device Authorization Grant specification. The grant type URI matches the standard exactly.
4021-4024:LGTM: Device authorization action constantsThe
oauth2DeviceActionAuthorizeandoauth2DeviceActionDenyconstants provide clean, typed values for the OAuth2 device authorization flow user actions.coderd/database/dump.sql (1)
146-150:Enum definition LGTMThe new
oauth2_device_statustype is concise, self-explanatory, and matches RFC-8628 state terminology.coderd/oauth2provider/tokens.go (9)
61-61:LGTM!The addition of the
deviceCodefield with the RFC 8628 reference is appropriate for device authorization grant support.
66-80:Proper implementation of device code parameter extraction.The changes correctly:
- Add proper form parsing error handling
- Require
client_idanddevice_codefor the device code grant type (RFC 8628 compliant)- Extract the device code parameter consistently with other fields
Also applies to: 91-91
136-145:LGTM!The missing parameters validation now correctly includes
device_codein the list.
158-160:Comprehensive error handling for device code grant.The implementation correctly:
- Adds the device code grant case to the switch statement
- Provides appropriate OAuth2 error responses for all device code states as per RFC 8628
- Uses the correct HTTP status codes and error codes
Also applies to: 186-205
229-232:Good error handling improvement.Properly differentiates between "secret not found" and other database errors.
242-264:Excellent security improvement with atomic code consumption.The refactoring to use
ConsumeOAuth2ProviderAppCodeByPrefixensures authorization codes are consumed atomically, preventing race conditions and replay attacks. The validation after consumption is the correct approach to maintain atomicity.
374-377:Good error handling for token validation and revocation.The changes properly handle:
- Token not found vs other database errors
- API key deletion (e.g., from token revocation) making refresh tokens invalid
Also applies to: 404-408
501-514:Well-structured device code parser.The function correctly validates the device code format and provides clear error messages.
516-678:Comprehensive and secure device code grant implementation.The implementation excellently handles:
- Device code validation and atomic consumption
- Proper authorization status checking (pending/authorized/denied)
- Security validation ensuring client_id matches
- Resource parameter consistency (RFC 8707)
- Expired device code detection
- Proper token generation with user context
The atomic consumption pattern (check status first, then consume if authorized) prevents race conditions while maintaining good UX.
docs/reference/api/enterprise.md (2)
1661-1667:Confirm all docs & samples now use/oauth2/tokenThis example is updated, but stale
/oauth2/tokensstrings may still exist elsewhere in the docs or code comments.
Please grep the repo to avoid confusing integrators.
1692-1696:👍 Device-code grant type includedThe enumerated
grant_typevalues correctly add the device-code URN.coderd/oauth2provider/revoke_test.go (13)
1-21:LGTM! Well-organized imports.The package declaration and imports are appropriate for OAuth2 token revocation testing.
23-26:LGTM! Proper test setup.The test function is well-named and correctly uses
t.Parallel()for concurrent execution.
27-72:LGTM! Comprehensive refresh token revocation test.The test properly validates successful revocation by attempting to use the revoked token and verifying the expected OAuth2 error response.
74-96:LGTM! Correctly implements RFC 7009 behavior.The test properly verifies that revoking non-existent tokens returns 200 OK as required by the specification.
98-139:LGTM! Correctly implements client ownership verification.The test properly verifies that tokens can only be revoked by their owning client, as required by RFC 7009.
141-170:LGTM! Proper validation of required parameters.The test correctly verifies that
client_idis required and returns appropriate error response.
172-200:LGTM! Proper client ID validation.The test correctly verifies that invalid client ID formats result in appropriate OAuth2 error responses.
202-231:LGTM! Good test coverage for client validation.The test properly verifies that non-existent client IDs are rejected with appropriate error response.
233-260:LGTM! Proper token parameter validation.The test correctly verifies that the
tokenparameter is required for revocation requests.
262-289:LGTM! Good token format validation.The test properly validates that tokens must follow the expected format with dash separators.
439-474:LGTM! Correctly implements RFC 7009 token type hint behavior.The test properly verifies that token type hints are advisory only and incorrect hints don't prevent successful revocation.
516-621:LGTM! Well-structured test helper functions.The helper functions are properly implemented with good error handling and appropriate use of test utilities.
426-430:Verify consistency of 405 responses for unsupported HTTP methodsIt’s unclear whether the revoke endpoint’s empty-body shortcut masks a deeper middleware issue, since we couldn’t locate comparable method-validation logic in other handlers automatically. Please manually confirm that all OAuth2 endpoints consistently return a proper 405 response body when an unsupported HTTP method is used.
• File: coderd/oauth2provider/revoke_test.go
Lines 426–430:go // If body is empty, the middleware might not be handling it properly if len(body) == 0 { t.Log("Response body is empty for method not allowed") return }• Action:
- Review other OAuth2 endpoint handlers (e.g., token, authorize) to see how they handle unsupported methods.
- Ensure they produce non-empty 405 responses for API consistency.
coderd/database/dbmetrics/querymetrics.go (5)
190-202:LGTM! OAuth2 consume operations instrumented correctly.The new consume wrapper methods for OAuth2 provider app codes and device codes follow the established pattern correctly, providing proper Prometheus metrics instrumentation for the device authorization grant functionality.
302-307:LGTM! Delete operations instrumented correctly.The delete wrapper methods for expired device codes and device codes by ID are implemented correctly with proper metrics collection.
1121-1147:LGTM! Get operations for device codes instrumented correctly.All the new Get wrapper methods for OAuth2 provider device codes (by ID, prefix, user code, and client ID) follow the established pattern correctly and provide comprehensive metrics coverage for device code retrieval operations.
2311-2316:LGTM! Insert operation instrumented correctly.The
InsertOAuth2ProviderDeviceCodewrapper method correctly instruments the device code insertion operation with proper Prometheus metrics collection.
2815-2820:LGTM! Update operation instrumented correctly.The
UpdateOAuth2ProviderDeviceCodeAuthorizationwrapper method correctly instruments the device code authorization update operation, completing the metrics coverage for the OAuth2 device authorization grant lifecycle.coderd/database/dbauthz/dbauthz.go (6)
420-420:LGTM! Appropriate permissions for OAuth2 app code tokens.The addition of full CRUD permissions for
rbac.ResourceOauth2AppCodeTokento the system-restricted subject is consistent with other OAuth2 resources and aligns with the PR's implementation of OAuth2 device authorization grant support.
1350-1356:LGTM! OAuth2 provider code consumption methods properly implement authorization.Both
ConsumeOAuth2ProviderAppCodeByPrefixandConsumeOAuth2ProviderDeviceCodeByPrefixcorrectly use theupdateWithReturnhelper, which ensures proper authorization checks are performed before consuming the codes.
1482-1488:LGTM! System-level authorization for cleanup operation.The
DeleteExpiredOAuth2ProviderDeviceCodesmethod correctly requires system-level authorization, which is appropriate for automated cleanup operations of expired device codes.
2361-2379:LGTM! OAuth2 provider device code getter methods properly implement authorization.All four getter methods correctly implement authorization:
- The single-item getters use the standard
fetchhelper pattern- The
GetOAuth2ProviderDeviceCodesByClientIDappropriately requires read access toResourceOauth2AppCodeTokensince it returns multiple device codes
3824-3830:LGTM! Appropriate authorization for device code creation.The
InsertOAuth2ProviderDeviceCodemethod correctly requires create permission onResourceOauth2AppCodeToken, and the comment clearly explains the authorization rationale.
4504-4509:LGTM! Device code authorization update properly implements RBAC.The
UpdateOAuth2ProviderDeviceCodeAuthorizationmethod correctly uses theupdateWithReturnhelper pattern, ensuring proper authorization checks before updating the device code's authorization state.coderd/database/dbmock/dbmock.go (6)
251-279:LGTM! Mock methods follow proper gomock patterns.The implementation correctly follows standard gomock patterns with proper type assertions and method call recording for both consume operations.
513-525:LGTM! Cleanup method correctly implemented.The mock method for deleting expired device codes follows proper gomock patterns and correctly handles the single error return value.
682-694:LGTM! Delete method correctly implemented.The mock method for deleting device codes by ID follows proper gomock patterns with correct parameter handling and error return.
2343-2401:LGTM! All getter methods correctly implemented.All four getter methods follow proper gomock patterns with correct type assertions. The variety of lookup methods (by ID, prefix, user code, and client ID) properly supports the OAuth2 device authorization flow requirements.
4933-4946:LGTM! Insert method correctly implemented.The mock method for inserting device codes follows proper gomock patterns with correct parameter and return type handling for the OAuth2 device authorization flow.
6023-6036:LGTM! Update method correctly implemented.The mock method for updating device code authorization follows proper gomock patterns and correctly handles the authorization update parameters for the OAuth2 device flow.
coderd/database/queries.sql.go (5)
5292-5352:LGTM! Well-structured OAuth2 code consumption and cleanup queries.The SQL queries properly implement secure code consumption patterns:
- Use parameterized queries to prevent SQL injection
- Implement appropriate expiration checks with
expires_at > NOW()- Include correct status filtering for device codes
- Follow consistent DELETE...RETURNING pattern for consumption operations
5425-5432:LGTM! Standard secure delete operation.The delete function correctly uses parameterized queries and appropriate type handling for UUID parameters.
5834-5954:LGTM! Comprehensive device code retrieval functions with proper error handling.These GET functions implement secure database access patterns:
- Use parameterized queries throughout
- Include proper error handling for multi-row queries (rows.Close(), rows.Err())
- Maintain consistent field scanning across all functions
- Support key OAuth2 device flow operations (ID, prefix, user code, client listing)
6284-6364:LGTM! Secure device code insertion with proper RFC 8628 implementation.The implementation demonstrates good security practices:
- DeviceCodeHash field correctly excludes JSON serialization with
json:"-"tag- Uses parameterized queries to prevent SQL injection
- Properly handles nullable fields with
sql.NullString- INSERT...RETURNING pattern ensures atomicity
- Field ordering is consistent between parameters and scanning
6605-6638:LGTM! Secure device authorization update with proper state machine enforcement.The implementation correctly enforces OAuth2 device flow state transitions:
- WHERE clause includes
status = 'pending'check to prevent updating already processed codes- Uses
uuid.NullUUIDfor optional user assignment (supports both approval and denial)- UPDATE...RETURNING pattern ensures atomicity
- Prevents race conditions in device authorization flow
coderd/oauth2_test.go (6)
47-47:Good cleanup of unused variable assignments.Removing the unused
ownervariable assignments improves code cleanliness and eliminates potential linter warnings.Also applies to: 67-67, 1267-1267, 1368-1368, 1492-1492
232-232:Correct standardization to RFC 6749 compliant endpoint.The token endpoint URL has been correctly changed from
/oauth2/tokensto/oauth2/tokento comply with OAuth2 standards.
809-818:Excellent improvement to test isolation.Creating a separate server instance for each subtest prevents race conditions and ensures proper test isolation when running tests in parallel.
1573-2141:Excellent comprehensive test coverage for OAuth2 Device Authorization Grant.The test suite thoroughly covers all aspects of RFC 8628 including:
- Device authorization requests with various scenarios
- User verification flows (authorize/deny)
- Token exchange with proper error handling
- Resource parameter validation
- Concurrent access handling
- One-time use enforcement
The tests are well-structured with proper parallelization and error assertions.
2143-2539:Thorough RBAC test coverage ensures proper authorization.The RBAC tests comprehensively verify:
- Public endpoint accessibility without authentication
- Authentication requirements for sensitive operations
- Proper device code ownership enforcement
- System vs user context authorization
- Cross-user authorization scenarios
Great attention to security considerations.
2541-2559:Useful helper functions reduce test code duplication.The helper functions appropriately encapsulate OAuth2 app and secret creation logic, making tests more readable and maintainable.
coderd/apidoc/docs.go (8)
2268-2270:LGTM: Correct content type specification.The addition of
text/htmlas the produced content type for the GET/oauth2/authorizeendpoint is correct, as this endpoint typically returns HTML for user authorization consent forms.
2326-2331:LGTM: Correct content type specifications.The addition of both
consumesandproducescontent type specifications for the POST/oauth2/authorizeendpoint is correct:
- Form-urlencoded input is standard for OAuth2 authorization form submissions
- HTML output is appropriate for post-submission response pages
2474-2576:LGTM: Correct RFC 8628 device authorization implementation.The new device authorization endpoints are well-defined and correctly implement RFC 8628:
/oauth2/devicePOST endpoint properly consumes JSON device authorization requests and produces JSON responses/oauth2/device/verifyGET/POST endpoints appropriately handle the user verification flow with session-based security- Parameter definitions, content types, and schema references are all correct
2611-2649:LGTM: Correct RFC 7009 token revocation implementation.The new
/oauth2/revokeendpoint correctly implements RFC 7009:
- Uses POST method with form-urlencoded content type as specified
- Includes required
client_idandtokenparameters- Includes optional
token_type_hintparameter with proper description- Returns 200 status code as per RFC 7009 requirements
2688-2690:LGTM: Correct device code grant type addition.The addition of
"urn:ietf:params:oauth:grant-type:device_code"to the grant type enum correctly implements RFC 8628's device authorization grant type specification.
13929-13931:LGTM: Correct schema extension for device authorization.The addition of the
device_authorizationfield with proper RFC 8628 description correctly extends the OAuth2 app schema to support device authorization endpoints.
13952-13955:LGTM: Correct authorization server metadata extension.The addition of
device_authorization_endpointfield with RFC 8628 reference correctly extends the authorization server metadata schema per the specification.
14221-14261:LGTM: Well-defined device authorization schemas.The new schema definitions correctly implement RFC 8628:
OAuth2DeviceAuthorizationRequest:
- Properly requires
client_idas per RFC 8628- Includes optional
scopefor standard OAuth2 functionality- Includes optional
resourceparameter with correct RFC 8707 referenceOAuth2DeviceAuthorizationResponse:
- Contains all standard RFC 8628 response fields with appropriate types
- Integer types for
expires_inandintervalare correct for time values- String types for codes and URIs are appropriate
coderd/apidoc/swagger.json (5)
1982-1985:GET /oauth2/authorize now correctly advertises text/html output.
No issues spotted – the change aligns with browser–based flows and RFC 6749 §3.1.
2034-2036:POST /oauth2/authorize: content-type/output declarations look good.application/x-www-form-urlencoded → text/htmlmatches the spec for the form-post response mode.
2160-2186:Missing endpoint-level security definition.
Unlike/oauth2/token, the new/oauth2/devicePOST lacks anysecurityblock.
Clients authenticating with HTTP Basic OR no auth (public clients) must be represented explicitly; otherwise code-gen tools mark the endpoint “no auth”.
Add, e.g.:"security": [ {"OAuth2ClientBasic": [] }]or
"security": []if intentionally unauthenticated.
2342-2346:Grant-type enum extended, but required parameterdevice_codeis missing.
Token requests withgrant_type=urn:ietf:params:oauth:grant-type:device_coderequire adevice_codeform field.
Ensure it is declared alongsideclient_id,grant_type, etc., otherwise generated SDKs cannot call the flow.
12820-12858:New request/response schemas look correct.
Fields and types match RFC 8628. Nice work.coderd/oauth2_spec_test.go (3)
1-30:Well-structured test setup!Good use of external test package and comprehensive OAuth2TestSetup struct for test organization.
31-84:Excellent OAuth2 test setup implementation!The setup function properly discovers metadata, registers clients dynamically, and configures OAuth2. Good use of RFC 8414 (metadata discovery) and RFC 7591 (dynamic registration).
433-535:Well-implemented OAuth2 test helpers!The helper functions properly simulate user interactions, handle redirects correctly, and validate tokens comprehensively. Good handling of both 302 and 307 redirect status codes.
codersdk/oauth2.go (6)
8-21:Good addition of OAuth2 device flow constants!The new imports and constants are appropriate for implementing OAuth2 device flow support.
35-41:Correct OAuth2 endpoint additions!The device authorization and revocation endpoints properly implement RFC 8628 and RFC 7009 requirements.
195-210:Correct RFC 8628 device code grant type implementation!The device code grant type uses the proper URN format as specified in RFC 8628.
226-247:Correct OAuth2 token revocation implementation!The method properly implements RFC 7009 token revocation with form-encoded parameters.
463-593:Comprehensive OAuth2 device flow implementation!The new methods correctly implement RFC 8628 device authorization grant flow with proper error handling, form encoding, and response types.
85-86:Custom validator is properly registeredThe
oauth2_app_display_nametag is already implemented and registered in the HTTP API layer:
- In coderd/httpapi/httpapi.go you’ll find:
for_,displayNameTag:=range []string{"organization_display_name","template_display_name","group_display_name","oauth2_app_display_name",} {err:=Validate.RegisterValidation(displayNameTag,displayNameValidator)iferr!=nil {panic(err) }}No further action needed here.
| "/oauth2/device": { | ||
| "post": { | ||
| "consumes": ["application/json"], | ||
| "produces": ["application/json"], | ||
| "tags": ["Enterprise"], | ||
| "summary": "OAuth2 device authorization request (RFC 8628).", | ||
| "operationId": "oauth2-device-authorization-request", | ||
| "parameters": [ | ||
| { | ||
| "description": "Device authorization request", | ||
| "name": "request", | ||
| "in": "body", | ||
| "required": true, | ||
| "schema": { | ||
| "$ref": "#/definitions/codersdk.OAuth2DeviceAuthorizationRequest" | ||
| } | ||
| } | ||
| ], | ||
| "responses": { | ||
| "200": { | ||
| "description": "OK", | ||
| "schema": { | ||
| "$ref": "#/definitions/codersdk.OAuth2DeviceAuthorizationResponse" | ||
| } | ||
| } | ||
| } | ||
| } |
coderabbitaibotJul 21, 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.
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.
Device-authorization request should be form-urlencoded, not JSON.
RFC 8628 §3.1 mandates the request be sent asapplication/x-www-form-urlencoded with parametersclient_id,scope, … .
Keeping it JSON means:
• Non-compliant clients will break.
• Public tools that assume the RFC default will mis-generate requests.
- "consumes": ["application/json"],+ "consumes": ["application/x-www-form-urlencoded"],
While JSON could be accepted as an extension, advertise both media types if you intend to support it:
- "consumes": ["application/json"],+ "consumes": ["application/x-www-form-urlencoded", "application/json"],
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In coderd/apidoc/swagger.json between lines 2160 and 2186, the OAuth2 deviceauthorization request incorrectly specifies the request content type asapplication/json, but per RFC 8628 §3.1 it must beapplication/x-www-form-urlencoded with parameters like client_id and scope. Tofix this, change the consumes field from application/json toapplication/x-www-form-urlencoded and update the parameters to reflect form datafields instead of a JSON body schema. If JSON support is desired as anextension, list both media types in consumes and adjust parameters accordingly.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.
That's an auto-generated file using swaggo. If you find issues in here, you should comment on them in the Go source files.
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.
@ThomasK33 Understood—I'll make sure to raise API spec issues in the Go source files (not the generated Swagger) in future reviews.
✏️ Learnings added
Learnt from: ThomasK33PR: coder/coder#18809File: coderd/apidoc/swagger.json:2160-2186Timestamp: 2025-07-21T14:30:40.568ZLearning: For the Coder repository, API specification or standards compliance issues should be raised in the Go source files (e.g., handler annotations or swaggo comments), not in auto-generated files like coderd/apidoc/swagger.json.Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configurereview instructions. You can manage existing learnings in theapp.
| "/oauth2/revoke": { | ||
| "post": { | ||
| "consumes": ["application/x-www-form-urlencoded"], | ||
| "tags": ["Enterprise"], | ||
| "summary": "Revoke OAuth2 tokens (RFC 7009).", | ||
| "operationId": "oauth2-token-revocation", | ||
| "parameters": [ | ||
| { | ||
| "type": "string", | ||
| "description": "Client ID for authentication", | ||
| "name": "client_id", | ||
| "in": "formData", | ||
| "required": true | ||
| }, | ||
| { | ||
| "type": "string", | ||
| "description": "The token to revoke", | ||
| "name": "token", | ||
| "in": "formData", | ||
| "required": true | ||
| }, | ||
| { | ||
| "type": "string", | ||
| "description": "Hint about token type (access_token or refresh_token)", | ||
| "name": "token_type_hint", | ||
| "in": "formData" | ||
| } | ||
| ], | ||
| "responses": { | ||
| "200": { | ||
| "description": "Token successfully revoked" | ||
| } | ||
| } | ||
| } | ||
| }, |
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.
/oauth2/revoke endpoint lacks security & incorrectly forces client_id.
RFC 7009 expects client authentication via Basic auth or anoptionalclient_id parameter for public clients.
Action items:
- Add a
securitystanza mirroring/oauth2/token. - Mark
client_idas optional ("required": false). - Advertise
consumesandproduces; most servers returnapplication/json({}) or no body.
🤖 Prompt for AI Agents
In coderd/apidoc/swagger.json between lines 2275 and 2309, the /oauth2/revokeendpoint is missing a security definition and incorrectly marks client_id asrequired. To fix this, add a security stanza similar to the one used in/oauth2/token to enforce client authentication, change the client_id parameter's"required" field to false to reflect its optional nature, and include a"produces" field specifying the response content type, typically"application/json", alongside the existing "consumes" field.| "description": "DeviceAuth is the device authorization endpoint for RFC 8628.", | ||
| "type": "string" | ||
| }, | ||
| "revocation": { | ||
| "type": "string" | ||
| }, |
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.
🛠️ Refactor suggestion
Inconsistent field names between metadata objects.codersdk.OAuth2AppEndpoints uses"device_authorization" whilecodersdk.OAuth2AuthorizationServerMetadata uses"device_authorization_endpoint".
Pick one canonical name (*_endpoint is consistent with existingauthorization_endpoint,token_endpoint).
Also applies to: 12551-12554
🤖 Prompt for AI Agents
In coderd/apidoc/swagger.json around lines 12528 to 12533 and 12551 to 12554,the field names for device authorization endpoints are inconsistent betweenmetadata objects. Change the field name from "device_authorization" to"device_authorization_endpoint" to match the canonical naming convention usedfor other endpoints like "authorization_endpoint" and "token_endpoint". Ensureboth metadata objects use the exact same field name for consistency.| CREATE TABLE oauth2_provider_device_codes ( | ||
| id uuid DEFAULT gen_random_uuid() NOT NULL, | ||
| created_at timestamp with time zone DEFAULT now() NOT NULL, | ||
| expires_at timestamp with time zone NOT NULL, | ||
| device_code_hash bytea NOT NULL, | ||
| device_code_prefix text NOT NULL, | ||
| user_code text NOT NULL, | ||
| client_id uuid NOT NULL, | ||
| user_id uuid, | ||
| status oauth2_device_status DEFAULT 'pending'::oauth2_device_status NOT NULL, | ||
| verification_uri text NOT NULL, | ||
| verification_uri_complete text, | ||
| scope text DEFAULT ''::text, | ||
| resource_uri text, | ||
| polling_interval integer DEFAULT 5 NOT NULL, | ||
| CONSTRAINT oauth2_provider_device_codes_device_code_prefix_check CHECK ((length(device_code_prefix) = 8)) | ||
| ); |
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.
🛠️ Refactor suggestion
Strengthen data-integrity onoauth2_provider_device_codes
- RFC 8628 mandates the client must respect the
intervalvalue and never poll faster than 5 s. A rogue write could accidentally setpolling_intervalto a value < 5. user_codeis compared case-insensitively (see index below). Consider storing it in acitextcolumn to avoid always callingupper()and to keep the functional index smaller.- You may also want a CHECK to ensure
expires_at > created_at.
Example diff:
polling_interval integer DEFAULT 5 NOT NULL,+ CONSTRAINT polling_interval_min CHECK (polling_interval >= 5),+ CONSTRAINT expires_after_creation CHECK (expires_at > created_at));--- BEFORE: user_code text NOT NULL,+-- Optionally, if the citext extension is enabled:+user_code citext NOT NULL,
These simple constraints prevent silent spec violations at the database layer.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In coderd/database/dump.sql around lines 1278 to 1294, enhance data integrity byadding a CHECK constraint to ensure polling_interval is never less than 5,change the user_code column type to citext for case-insensitive storage andindexing, and add a CHECK constraint to guarantee expires_at is always greaterthan created_at. Modify the table definition accordingly to enforce theseconstraints at the database level.| ALTER TABLE ONLY oauth2_provider_device_codes | ||
| ADD CONSTRAINT oauth2_provider_device_codes_device_code_hash_key UNIQUE (device_code_hash); | ||
| ALTER TABLE ONLY oauth2_provider_device_codes | ||
| ADD CONSTRAINT oauth2_provider_device_codes_device_code_prefix_key UNIQUE (device_code_prefix); | ||
| ALTER TABLE ONLY oauth2_provider_device_codes | ||
| ADD CONSTRAINT oauth2_provider_device_codes_pkey PRIMARY KEY (id); | ||
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.
Redundant / duplicate indexes – drop to save ~2× space & write I/O
UNIQUE constraints already create backing B-tree indexes:
device_code_hash(CONSTRAINT oauth2_provider_device_codes_device_code_hash_key)device_code_prefix(CONSTRAINT ..._device_code_prefix_key)
Additional non-unique indexes on the same columns and the duplicate pair onexpires_at add no query benefit but increase disk usage and slow down writes.
Recommended fix (keep only oneexpires_at index, drop the other two):
- CREATE INDEX idx_oauth2_provider_device_codes_device_code_hash- ON oauth2_provider_device_codes(device_code_hash);- CREATE INDEX idx_oauth2_provider_device_codes_expires_at- ON oauth2_provider_device_codes(expires_at);
If the extraexpires_at index is meant for a partial/covered pattern, rename the first one and add the intended predicate instead of duplicating.
Also applies to: 2827-2834
🤖 Prompt for AI Agents
In coderd/database/dump.sql around lines 2594 to 2602, there are redundantindexes created by UNIQUE constraints on device_code_hash and device_code_prefixcolumns, as well as duplicate indexes on expires_at. To fix this, identify anddrop the duplicate non-unique indexes on these columns to save disk space andimprove write performance. For the expires_at indexes, keep only one and if apartial or covered index is needed, rename the retained index and add theappropriate predicate instead of duplicating. Apply the same cleanup to lines2827-2834.| | Name | Type | Required | Restrictions | Description | | ||
| |------------------------|--------|----------|--------------|-------------------------------------------------------------------------| | ||
| | `authorization` | string | false | | | | ||
| | `device_authorization` | string | false | | Device authorization is the device authorization endpoint for RFC 8628. | | ||
| | `revocation` | string | false | | | | ||
| | `token` | string | false | | | |
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.
🛠️ Refactor suggestion
Missing description + inconsistent naming between specs and discovery docs
revocation is introduced with no description, whiledevice_authorization is described yet its “endpoint” suffix is omitted here but used elsewhere (device_authorization_endpoint). That asymmetry makes the docs harder to consume and deviates from RFC-7009 / RFC-8628 discovery field names (revocation_endpoint,device_authorization_endpoint).
-| `device_authorization` | string | false | | Device authorization is the device authorization endpoint for RFC 8628. |-| `revocation` | string | false | | |+| `device_authorization` | string | false | | Device authorization endpoint (RFC 8628). |+| `revocation` | string | false | | Token revocation endpoint (RFC 7009). |
Consider also aligning the field names with the well-known metadata keys (*_endpoint) for consistency across schemas.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | Name| Type| Required| Restrictions| Description| | |
| |------------------------|--------|----------|--------------|-------------------------------------------------------------------------| | |
| |`authorization`| string| false||| | |
| |`device_authorization`| string| false|| Device authorizationis the device authorization endpoint for RFC 8628.| | |
| |`revocation`| string| false||| | |
| |`token`| string| false||| | |
| | Name| Type| Required| Restrictions| Description| | |
| |------------------------|--------|----------|--------------|-------------------------------------------------------------------------| | |
| |`authorization`| string| false||| | |
| |`device_authorization`| string| false|| Device authorizationendpoint (RFC 8628).| | |
| |`revocation`| string| false||Token revocation endpoint (RFC 7009).| | |
| |`token`| string| false||| |
🤖 Prompt for AI Agents
In docs/reference/api/schemas.md around lines 4467 to 4472, the field names`device_authorization` and `revocation` lack consistent naming and descriptionscompared to the RFC specs and discovery docs. Rename these fields to`device_authorization_endpoint` and `revocation_endpoint` respectively to alignwith RFC-8628 and RFC-7009 metadata keys, and add a clear description for`revocation_endpoint` similar to the existing one for`device_authorization_endpoint`.| "code_challenge_methods_supported": [ | ||
| "string" | ||
| ], | ||
| "device_authorization_endpoint": "string", | ||
| "grant_types_supported": [ | ||
| "string" | ||
| ], |
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.
revocation_endpoint missing from server-metadata example
The JSON fragment addsdevice_authorization_endpoint but omits the equally newrevocation_endpoint, leaving implementers guessing.
"code_challenge_methods_supported": [ "string" ], "device_authorization_endpoint": "string",+ "revocation_endpoint": "string", "grant_types_supported": [Add the property to stay faithful to RFC 7009 discovery metadata.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "code_challenge_methods_supported":[ | |
| "string" | |
| ], | |
| "device_authorization_endpoint": "string", | |
| "grant_types_supported":[ | |
| "string" | |
| ], | |
| "code_challenge_methods_supported":[ | |
| "string" | |
| ], | |
| "device_authorization_endpoint": "string", | |
| "revocation_endpoint": "string", | |
| "grant_types_supported":[ | |
| "string" | |
| ], |
🤖 Prompt for AI Agents
In docs/reference/api/schemas.md around lines 4479 to 4485, the server-metadataexample includes device_authorization_endpoint but is missing therevocation_endpoint property. To fix this, add the revocation_endpoint propertywith a string value to the JSON fragment to comply with RFC 7009 discoverymetadata and provide complete information for implementers.| | Name | Type | Required | Restrictions | Description | | ||
| |-----------------------------------------|-----------------|----------|--------------|------------------------------------| | ||
| | `authorization_endpoint` | string | false | | | | ||
| | `code_challenge_methods_supported` | array of string | false | | | | ||
| | `device_authorization_endpoint` | string | false | | Device authorization endpoint 8628 | | ||
| | `grant_types_supported` | array of string | false | | | | ||
| | `issuer` | string | false | | | | ||
| | `registration_endpoint` | string | false | | | | ||
| | `response_types_supported` | array of string | false | | | | ||
| | `scopes_supported` | array of string | false | | | | ||
| | `token_endpoint` | string | false | | | | ||
| | `token_endpoint_auth_methods_supported` | array of string | false | | | | ||
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.
🛠️ Refactor suggestion
Property table out-of-sync with JSON example
revocation_endpoint is not documented; descriptions are terse.
| `device_authorization_endpoint` | string | false | | Device authorization endpoint (RFC 8628) |+| `revocation_endpoint` | string | false | | Token revocation endpoint (RFC 7009) |Please add the row and tidy descriptions.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In docs/reference/api/schemas.md around lines 4503 to 4515, the property tableis missing the `revocation_endpoint` row and the descriptions for existingproperties are too terse. Add a new row for `revocation_endpoint` with typestring, required false, and an appropriate description. Also, improve thedescriptions for all properties to provide clearer and more informative details.| | Name | Type | Required | Restrictions | Description | | ||
| |-----------------------------|---------|----------|--------------|-------------| | ||
| | `device_code` | string | false | | | | ||
| | `expires_in` | integer | false | | | | ||
| | `interval` | integer | false | | | | ||
| | `user_code` | string | false | | | | ||
| | `verification_uri` | string | false | | | | ||
| | `verification_uri_complete` | string | false | | | |
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.
Required flags don’t match RFC 8628
Per §3.2 of RFC 8628,device_code,user_code,verification_uri, andexpires_in are mandatory. Mark themtrue; leaveinterval optional.
-| `device_code` | string | false | | |-| `expires_in` | integer | false | | |-| `interval` | integer | false | | |-| `user_code` | string | false | | |-| `verification_uri` | string | false | | |+| `device_code` | string | true | | |+| `expires_in` | integer | true | | |+| `interval` | integer | false | | |+| `user_code` | string | true | | |+| `verification_uri` | string | true | | |
Updating the “Required” column avoids implementer confusion and test-suite failures.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| | Name| Type| Required| Restrictions| Description| | |
| |-----------------------------|---------|----------|--------------|-------------| | |
| |`device_code`| string|false||| | |
| |`expires_in`| integer|false||| | |
| |`interval`| integer| false||| | |
| |`user_code`| string|false||| | |
| |`verification_uri`| string|false||| | |
| |`verification_uri_complete`| string| false||| | |
| | Name| Type| Required| Restrictions| Description| | |
| |-----------------------------|---------|----------|--------------|-------------| | |
| |`device_code`| string|true||| | |
| |`expires_in`| integer|true||| | |
| |`interval`| integer| false||| | |
| |`user_code`| string|true||| | |
| |`verification_uri`| string|true||| | |
| |`verification_uri_complete`| string| false||| |
🤖 Prompt for AI Agents
In docs/reference/api/schemas.md around lines 4751 to 4758, update the"Required" column for the fields device_code, user_code, verification_uri, andexpires_in to true, as per RFC 8628 section 3.2. Leave the interval field asfalse. This change aligns the schema documentation with the RFC requirements andprevents confusion and test failures.| <form method="post"> | ||
| <div class="form-group"> | ||
| <label for="user_code">Device Code:</label> | ||
| <input | ||
| type="text" | ||
| id="user_code" | ||
| name="user_code" | ||
| value="{{ .UserCode }}" | ||
| placeholder="Enter the code from your device" | ||
| required | ||
| autocomplete="off" | ||
| /> | ||
| </div> |
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.
Add CSRF protection to the verification form
A POST form without an anti-CSRF token is susceptible to cross-site-request-forgery attacks.
Inject the server-generated token as a hidden field so middleware can validate the request.
<form method="post">+ {{- /* CSRF token to protect the POST */}}+ <input type="hidden" name="csrf_token" value="{{ .CSRFToken }}"> <div>
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <formmethod="post"> | |
| <divclass="form-group"> | |
| <labelfor="user_code">Device Code:</label> | |
| <input | |
| type="text" | |
| name="user_code" | |
| value="{{ .UserCode }}" | |
| placeholder="Enter the code from your device" | |
| required | |
| autocomplete="off" | |
| /> | |
| </div> | |
| <formmethod="post"> | |
| {{- /* CSRF token to protect the POST */}} | |
| <inputtype="hidden"name="csrf_token"value="{{ .CSRFToken }}"> | |
| <divclass="form-group"> | |
| <labelfor="user_code">Device Code:</label> | |
| <input | |
| type="text" | |
| name="user_code" | |
| value="{{ .UserCode }}" | |
| placeholder="Enter the code from your device" | |
| required | |
| autocomplete="off" | |
| /> | |
| </div> |
🤖 Prompt for AI Agents
In site/static/oauth2device.html around lines 29 to 41, the POST form lacks CSRFprotection, making it vulnerable to cross-site request forgery. Fix this byadding a hidden input field inside the form that includes the server-generatedCSRF token, typically passed as a template variable, so the middleware canvalidate the authenticity of the request.
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