Skip to content

feat: implement OAuth2 dynamic client registration (RFC 7591/7592) #18645

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

Conversation

ThomasK33
Copy link
Member

Implement OAuth2 Dynamic Client Registration (RFC 7591/7592)

This PR implements OAuth2 Dynamic Client Registration according to RFC 7591 and Client Configuration Management according to RFC 7592. These standards allow OAuth2 clients to register themselves programmatically with Coder as an authorization server.

Key changes include:

  1. Added database schema extensions to support RFC 7591/7592 fields in the oauth2_provider_apps table

  2. Implemented /oauth2/register endpoint for dynamic client registration (RFC 7591)

  3. Added client configuration management endpoints (RFC 7592):

    • GET/PUT/DELETE /oauth2/clients/{client_id}
    • Registration access token validation middleware
  4. Added comprehensive validation for OAuth2 client metadata:

    • URI validation with support for custom schemes for native apps
    • Grant type and response type validation
    • Token endpoint authentication method validation
  5. Enhanced developer documentation with:

    • RFC compliance guidelines
    • Testing best practices to avoid race conditions
    • Systematic debugging approaches for OAuth2 implementations

The implementation follows security best practices from the RFCs, including proper token handling, secure defaults, and appropriate error responses. This enables third-party applications to integrate with Coder's OAuth2 provider capabilities programmatically.

@ThomasK33 ThomasK33 force-pushed the thomask33/06-27-feat_oauth2_implement_rfc_7591_7592_dynamic_client_registration_for_mcp_compliance branch from 3902793 to b37d850 Compare June 27, 2025 17:02
@ThomasK33 ThomasK33 force-pushed the thomask33/06-27-feat_oauth2_implement_rfc_6750_bearer_token_support_for_mcp_compliance branch from ff83df4 to 3665807 Compare June 27, 2025 17:02
@ThomasK33 ThomasK33 force-pushed the thomask33/06-27-feat_oauth2_implement_rfc_7591_7592_dynamic_client_registration_for_mcp_compliance branch from b37d850 to caf974c Compare June 27, 2025 17:11
@ThomasK33 ThomasK33 force-pushed the thomask33/06-27-feat_oauth2_implement_rfc_6750_bearer_token_support_for_mcp_compliance branch from 3665807 to 56126dd Compare June 27, 2025 17:11
@ThomasK33 ThomasK33 force-pushed the thomask33/06-27-feat_oauth2_implement_rfc_7591_7592_dynamic_client_registration_for_mcp_compliance branch from caf974c to 22b8b6d Compare June 27, 2025 17:29
@ThomasK33 ThomasK33 force-pushed the thomask33/06-27-feat_oauth2_implement_rfc_6750_bearer_token_support_for_mcp_compliance branch from 56126dd to fca6b9a Compare June 27, 2025 17:29
@ThomasK33 ThomasK33 force-pushed the thomask33/06-27-feat_oauth2_implement_rfc_7591_7592_dynamic_client_registration_for_mcp_compliance branch 2 times, most recently from 14c7196 to c43b551 Compare June 27, 2025 17:54
@ThomasK33 ThomasK33 force-pushed the thomask33/06-27-feat_oauth2_implement_rfc_6750_bearer_token_support_for_mcp_compliance branch from fca6b9a to 68baa21 Compare June 27, 2025 17:54
@ThomasK33 ThomasK33 force-pushed the thomask33/06-27-feat_oauth2_implement_rfc_7591_7592_dynamic_client_registration_for_mcp_compliance branch 12 times, most recently from 46d50e8 to 61142ba Compare June 30, 2025 08:58
@ThomasK33 ThomasK33 force-pushed the thomask33/06-27-feat_oauth2_implement_rfc_7591_7592_dynamic_client_registration_for_mcp_compliance branch 3 times, most recently from a8a29d4 to f9e693d Compare July 2, 2025 14:06
Copy link
Member Author

In general, should these routes be implemented in their own package? Rather than mostly the oauth2 file in coderd?

Looking at the router/chi.Mux in coderd.go, I see that all the HTTP handlers are implemented on the API struct.

Are you suggesting moving those handlers into a different package or moving the logic behind them into their own package, which then gets called from those API handlers? (I'm assuming the latter, but I want to be sure.)

@ThomasK33 ThomasK33 force-pushed the thomask33/06-27-feat_oauth2_implement_rfc_6750_bearer_token_support_for_mcp_compliance branch from 3760dd0 to 2a41a65 Compare July 2, 2025 15:50
@ThomasK33 ThomasK33 force-pushed the thomask33/06-27-feat_oauth2_implement_rfc_7591_7592_dynamic_client_registration_for_mcp_compliance branch 2 times, most recently from 9629df1 to df790c7 Compare July 2, 2025 16:35
@ThomasK33 ThomasK33 force-pushed the thomask33/06-27-feat_oauth2_implement_rfc_6750_bearer_token_support_for_mcp_compliance branch 2 times, most recently from 52c88e0 to 4799b4b Compare July 2, 2025 16:44
@ThomasK33 ThomasK33 force-pushed the thomask33/06-27-feat_oauth2_implement_rfc_7591_7592_dynamic_client_registration_for_mcp_compliance branch from df790c7 to 17af791 Compare July 2, 2025 16:45
@ThomasK33 ThomasK33 force-pushed the thomask33/06-27-feat_oauth2_implement_rfc_6750_bearer_token_support_for_mcp_compliance branch 2 times, most recently from a07ba99 to 5c1b9f6 Compare July 2, 2025 16:59
@ThomasK33 ThomasK33 force-pushed the thomask33/06-27-feat_oauth2_implement_rfc_7591_7592_dynamic_client_registration_for_mcp_compliance branch from 17af791 to 3e15358 Compare July 2, 2025 16:59
@ThomasK33 ThomasK33 changed the base branch from thomask33/06-27-feat_oauth2_implement_rfc_6750_bearer_token_support_for_mcp_compliance to graphite-base/18645 July 2, 2025 17:14
@ThomasK33 ThomasK33 force-pushed the graphite-base/18645 branch from 5c1b9f6 to 09c5055 Compare July 2, 2025 17:15
@ThomasK33 ThomasK33 force-pushed the thomask33/06-27-feat_oauth2_implement_rfc_7591_7592_dynamic_client_registration_for_mcp_compliance branch from 3e15358 to 6bea7a6 Compare July 2, 2025 17:15
@graphite-app graphite-app bot changed the base branch from graphite-base/18645 to main July 2, 2025 17:15
@ThomasK33 ThomasK33 force-pushed the thomask33/06-27-feat_oauth2_implement_rfc_7591_7592_dynamic_client_registration_for_mcp_compliance branch from 6bea7a6 to e33d3d6 Compare July 2, 2025 17:15
… MCP compliance

- Add comprehensive OAuth2 dynamic client registration support
- Implement RFC 7591 client registration endpoint with proper validation
- Implement RFC 7592 client management protocol (GET/PUT/DELETE)
- Add RFC 6750 Bearer token authentication support
- Fix authorization context issues with AsSystemRestricted
- Add proper RBAC permissions for OAuth2 resources
- Implement registration access token security per RFC 7592
- Add comprehensive validation for redirect URIs, grant types, response types
- Support custom schemes for native applications
- Add database migration with all RFC-required fields
- Add audit logging support for OAuth2 operations
- Ensure full RFC compliance with proper error handling
- Add comprehensive test coverage for all scenarios

Change-Id: I36c711201d598a117f6bfc381fc74e07fc3cc365
Signed-off-by: Thomas Kosiewski <[email protected]>
@ThomasK33 ThomasK33 force-pushed the thomask33/06-27-feat_oauth2_implement_rfc_7591_7592_dynamic_client_registration_for_mcp_compliance branch from e33d3d6 to 4f4e4f1 Compare July 3, 2025 15:00
Copy link
Member Author

Alright, so I did the refactoring in two PRs:

  • this PR refactors the handlers into a oauth2provider package (merging in the functions from the identityprovider package)
  • this PR refactors the tests from the coderd package into the oauth2provider package.

The main reason for this two PR approach was to make sure that the refactorings didn't break the tests, and that the test refactoring did not break the code.

@ThomasK33 ThomasK33 requested review from johnstcn and Emyrk July 3, 2025 15:34
Copy link
Member

@johnstcn johnstcn left a comment

Choose a reason for hiding this comment

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

I don't have any further blocking comments, but would appreciate a second pair of eyes before merge in case I missed something.

Copy link
Member

Choose a reason for hiding this comment

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

nit: the validation tests could probably live in codersdk alongside the func itself but open to it staying here either

@ThomasK33 ThomasK33 merged commit 74e1d5c into main Jul 3, 2025
40 checks passed
Copy link
Member Author

Merge activity

@ThomasK33 ThomasK33 deleted the thomask33/06-27-feat_oauth2_implement_rfc_7591_7592_dynamic_client_registration_for_mcp_compliance branch July 3, 2025 16:33
@github-actions github-actions bot locked and limited conversation to collaborators Jul 3, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants