Skip to content

feat: implement standardized OAuth2 endpoints and token revocation #18809

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ThomasK33
Copy link
Member

@ThomasK33 ThomasK33 commented Jul 9, 2025

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 ThomasK33 changed the title feat: standardize OAuth2 endpoints and add token revocation feat: standardize OAuth2 endpoints and implement token revocation Jul 9, 2025
@ThomasK33 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 ThomasK33 marked this pull request as ready for review July 10, 2025 09:20
@ThomasK33 ThomasK33 requested a review from johnstcn July 10, 2025 09:24
@ThomasK33 ThomasK33 requested review from Emyrk and mafredri July 10, 2025 11:10
@ThomasK33 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 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
@ThomasK33 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 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 ThomasK33 requested a review from dannykopping July 17, 2025 12:52
@ThomasK33 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 ThomasK33 force-pushed the thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation branch 3 times, most recently from 8342301 to 3c0603e Compare July 21, 2025 19:09
@ThomasK33 ThomasK33 force-pushed the thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation branch from 3c0603e to 9dfcc8a Compare July 23, 2025 16:53
@github-actions github-actions bot added the stale This issue is like stale bread. label Jul 31, 2025
@ThomasK33 ThomasK33 force-pushed the thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation branch 2 times, most recently from 4ad81e8 to 12117a3 Compare July 31, 2025 16:11
@ThomasK33 ThomasK33 removed the stale This issue is like stale bread. label Jul 31, 2025
@ThomasK33 ThomasK33 force-pushed the thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation branch 2 times, most recently from d10afa7 to b436ec8 Compare July 31, 2025 16:48
Copy link
Member

@aslilac aslilac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw do we have any figma designs for these new pages? is there an easy way for me to take a look at them locally? at the very least I'd like to see some screenshots

@@ -58,6 +58,21 @@ var (
oauthHTML string

oauthTemplate *htmltemplate.Template

//go:embed static/oauth2device.html
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this means all of these files will actually be embedded in the binary twice

  • once here
  • once over here because everything in static/ gets copied to out/ by vite

the version copied by vite is practically useless since it will always be the unfilled template, but it will always be accessible at http:///oauth2device.html, and so on for the other similar html files.

maybe this means we should have a site/templates/ folder to put stuff like this in?

@@ -1149,3 +1179,56 @@ func RenderOAuthAllowPage(rw http.ResponseWriter, r *http.Request, data RenderOA
return
}
}

// RenderOAuthDeviceData contains the variables that are found in
// site/static/oauth2device.html.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we do move the files we should update this path

}

// RenderOAuthDeviceResultData contains the variables that are found in
// site/static/oauth2device_success.html and site/static/oauth2device_denied.html.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and these

@github-actions github-actions bot added the stale This issue is like stale bread. label Aug 8, 2025
@github-actions github-actions bot closed this Aug 11, 2025
@ThomasK33 ThomasK33 reopened this Aug 12, 2025
@ThomasK33 ThomasK33 removed the stale This issue is like stale bread. label Aug 12, 2025
@ThomasK33 ThomasK33 force-pushed the thomask33/07-07-feat_standardize_oauth2_endpoints_and_add_token_revocation branch from b436ec8 to aca2f6a Compare August 12, 2025 16:26
@ThomasK33 ThomasK33 assigned Emyrk and unassigned ThomasK33 Aug 12, 2025
  - 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>
@ThomasK33 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 ThomasK33 changed the title feat: standardize OAuth2 endpoints and implement token revocation feat: implement standardized OAuth2 endpoints and token revocation Aug 12, 2025
Copy link
Member

@aslilac aslilac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +18 to +28
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"
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see other places in the codebase where we use github.com/muesli/termenv for this, which will do a better job of not flooding the output with noise when stdout isn't a tty. it's maybe not a big deal for something in scripts/, but we already pull in the dependency. might as well use it.

}
_ = resp.Body.Close()

if errorCode, ok := result["error"].(string); ok {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably use codersdk.AsError

Comment on lines 1571 to 1572
// NOTE: OAuth2 client registration validation tests have been migrated to
// oauth2provider/validation_test.go for better separation of concerns
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +50 to +58
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)';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

client_id uuid NOT NULL REFERENCES oauth2_provider_apps(id) ON DELETE CASCADE,
user_id uuid REFERENCES users(id) ON DELETE CASCADE, -- NULL until authorized

-- Authorization state (using enum for better data integrity)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

-- Remove constraints added for data integrity
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is also kinda weird. I imagine some agent inserted this? saying "this was added for data integrity" as you destroy it is just tonally odd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants