mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
feat(coderd): support public OAuth2 client tokens at the schema layer (#27712)
Layer 1 of a multi-PR split of #27195 (public/secretless PKCE-only OAuth2 clients), broken up for easier review: **database schema (this PR)** → oauth2provider handler logic → API/e2e integration tests. ## Goal Coder's OAuth2 provider only works correctly for confidential clients today. Public clients — native apps that can't safely hold a shared secret, such as the CLI's browser-based login flow, IDE plugins (VS Code, JetBrains), desktop apps, and MCP clients — cannot complete a real OAuth2 flow against Coder, even though OAuth 2.1 §2.1 explicitly defines this client type and RFC 8252 §8.5 requires PKCE alone to be sufficient authentication for it. Every MCP client, CLI login flow, and IDE plugin is a public client by construction, and none of them can complete a secretless flow against Coder today: dynamic registration always classifies a client as confidential regardless of what it asks for, the token endpoint unconditionally requires a `client_secret`, and discovery metadata never advertises `"none"` as a supported auth method. Full write-up: [ENG-3029](https://linear.app/codercom/issue/ENG-3029/oauth2-support-public-client) ### Overall design (end state across the full PR stack) `[PR2]` marks handler-layer changes landing in the next PR in this stack. The green box is what this PR implements. ```mermaid sequenceDiagram autonumber participant C as Public Client (CLI/MCP/IDE plugin) participant S as coderd (chi router) participant H as oauth2provider handlers participant DB as PostgreSQL Note over C,S: Discovery C->>S: GET /.well-known/oauth-authorization-server S->>H: GetAuthorizationServerMetadata() Note over H: [PR2] add "none" to<br/>the returned auth methods list H-->>C: [PR2] 200 { token_endpoint_auth_methods_supported:<br/>[..., "none"] } Note over C,S: Dynamic Client Registration C->>S: POST /oauth2/register<br/>{redirect_uris, token_endpoint_auth_method: "none"} S->>H: CreateDynamicClientRegistration() Note over H: [PR2] client type now reads<br/>the request -> "public" Note over H: [PR2] skip secret generation<br/>for public clients H->>DB: [PR2] INSERT app row<br/>(client_type = 'public') DB-->>H: app row Note over H: [PR2] skip secret insert entirely H-->>C: [PR2] 201 { client_id }<br/>(no client_secret field) Note over C,S: Authorization Code + PKCE flow C->>S: GET /oauth2/authorize?client_id=...&code_challenge=... C->>S: POST /oauth2/tokens (grant_type=authorization_code)<br/>no client_secret S->>H: extractTokenRequest() Note over H: [PR2] client_secret no longer required<br/>for public clients H->>H: authorizationCodeGrant() Note over H: [PR2] skip secret lookup for public clients Note over H: PKCE verification — already mandatory, unchanged rect rgb(198, 239, 206) Note over H,DB: [THIS PR] oauth2_provider_app_tokens.app_id<br/>column added (NOT NULL, populated at insert<br/>time from app.ID) and app_secret_id loosened<br/>to nullable. Revocation now checks app_id<br/>directly. Confidential-client behavior is<br/>unchanged — no public client can be created yet. H->>DB: [PR2] INSERT refresh token row<br/>(no secret reference, for public clients) end DB-->>H: token row H-->>C: 200 { access_token, refresh_token } ``` ## This PR: database schema A public client has no `client_secret`, so it has nothing to put in `oauth2_provider_app_tokens.app_secret_id`, which was `NOT NULL`. This PR makes that column nullable and instead attributes a token to its owning app through a new, always-populated `app_id` column — so ownership checks (e.g. revocation) work identically for public and confidential clients without joining through a secret that may not exist. | Column | Before | After (this PR) | |---|---|---| | `app_secret_id` | `uuid NOT NULL` | **nullable** | | `app_id` | — | **new**: `uuid NOT NULL`, `FOREIGN KEY → oauth2_provider_apps(id) ON DELETE CASCADE`, backfilled for every existing row and populated on every new insert from that point on | This is a single, complete migration — not staged across multiple PRs. An earlier version of this branch deferred `app_secret_id`'s nullability and the insert-time population of `app_id` to a later PR, keeping this PR's diff limited to `coderd/database`. [Automated review](https://github.com/coder/coder/pull/27712#discussion_r3686851911) correctly flagged that as unsafe: the migration would backfill existing rows once, but nothing would populate `app_id` for rows written afterward, so the moment this PR merged, new tokens would start accumulating a permanently `NULL` app_id — and if a release happened to be cut before the follow-up PR landed, that gap could ship to customers and would need a second, later backfill to close. Doing the full migration now avoids that: `app_id` is correct from the first row written, and the promised `NOT NULL` constraint requires no data repair because it's already enforced. Closing that gap requires a few mechanical, non-branching touches outside `coderd/database`: - `revoke.go`'s two ownership checks now compare `dbToken.AppID` directly instead of looking up the app through `app_secret_id` — a genuine simplification (and slightly less code), not a temporary shim. - `tokens.go`'s two `InsertOAuth2ProviderAppToken` call sites supply the new `app_id` column and wrap `app_secret_id` as a `NullUUID`. - `oauth2_test.go`'s one direct-insert test fixture does the same. None of these introduce client-type branching or new capability — every client today is still confidential-only, still always presents a secret, and behavior is unchanged. The full repo builds, vets, and all existing tests pass unmodified in behavior. ## Coming next - **PR2 (handler layer)**: `codersdk`'s `DetermineClientType()` reading the requested `token_endpoint_auth_method`; `registration.go` skipping secret generation for public clients (and wrapping the app+secret insert in a single transaction, fixing a pre-existing orphan-row/visibility-race gap); `tokens.go` making the secret check conditional so PKCE alone authenticates a public client; `metadata.go` advertising `"none"` in discovery. No further migration is needed — the schema this PR ships is already final. - **PR3 (API/e2e layer)**: integration tests through the real HTTP API (`coderd/oauth2_test.go`), the MCP OAuth2 e2e flow (`coderd/mcp/mcp_e2e_test.go`), and the manual test script (`scripts/oauth2/test-mcp-oauth2.sh`). Depends on: #27195 (original combined PR, being superseded by this stack) --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Bobby Ho
co-authored by
Claude Sonnet 5
Claude Sonnet 5
parent
120ec1f318
commit
d814dfad88
+195
-6
@@ -446,6 +446,16 @@ func TestOAuth2ProviderTokenExchange(t *testing.T) {
|
||||
return err
|
||||
},
|
||||
},
|
||||
{
|
||||
// secret belongs to apps.Default (see the shared "secret" above),
|
||||
// but this app's client_id is apps.NoPort. The token endpoint
|
||||
// must reject a secret that belongs to a different app than the
|
||||
// one identified by client_id, rather than trusting client_id
|
||||
// alone to attribute the resulting token.
|
||||
name: "SecretBelongsToDifferentApp",
|
||||
app: apps.NoPort,
|
||||
tokenError: "The client credentials are invalid",
|
||||
},
|
||||
{
|
||||
name: "OK",
|
||||
app: apps.Default,
|
||||
@@ -531,6 +541,76 @@ func TestOAuth2ProviderTokenExchange(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestOAuth2ProviderTokenExchangeCodeBelongsToDifferentApp covers
|
||||
// authorizationCodeGrant's code-ownership check in isolation. The token
|
||||
// endpoint validates redirect_uri against the app resolved from client_id
|
||||
// (before the grant runs at all) and separately against the redirect_uri
|
||||
// recorded on the code itself (inside the grant). Both must pass for the
|
||||
// request to reach the code-ownership check, which only happens when the
|
||||
// app identified by client_id and the app that originally issued the code
|
||||
// happen to share the exact same callback URL, which is plausible for
|
||||
// native clients that commonly register a conventional localhost
|
||||
// redirect. Two apps with distinct callbacks (as in the table above) can
|
||||
// never reach this check via a redirect_uri mismatch; this test
|
||||
// constructs the one scenario that does.
|
||||
func TestOAuth2ProviderTokenExchangeCodeBelongsToDifferentApp(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
ownerClient := coderdtest.New(t, nil)
|
||||
owner := coderdtest.CreateFirstUser(t, ownerClient)
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
|
||||
const sharedCallback = "http://localhost1:8080/foo/bar"
|
||||
createApp := func(name string) (codersdk.OAuth2ProviderApp, codersdk.OAuth2ProviderAppSecretFull) {
|
||||
//nolint:gocritic // OAauth2 app management requires owner permission.
|
||||
app, err := ownerClient.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{
|
||||
Name: fmt.Sprintf("%s-%d", name, time.Now().UnixNano()),
|
||||
CallbackURL: sharedCallback,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
//nolint:gocritic // OAauth2 app management requires owner permission.
|
||||
secret, err := ownerClient.PostOAuth2ProviderAppSecret(ctx, app.ID)
|
||||
require.NoError(t, err)
|
||||
return app, secret
|
||||
}
|
||||
appA, _ := createApp("code-owner")
|
||||
appB, secretB := createApp("code-thief")
|
||||
|
||||
userClient, _ := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID)
|
||||
|
||||
cfgA := &oauth2.Config{
|
||||
ClientID: appA.ID.String(),
|
||||
Endpoint: oauth2.Endpoint{
|
||||
AuthURL: appA.Endpoints.Authorization,
|
||||
TokenURL: appA.Endpoints.Token,
|
||||
AuthStyle: oauth2.AuthStyleInParams,
|
||||
},
|
||||
RedirectURL: sharedCallback,
|
||||
Scopes: []string{},
|
||||
}
|
||||
code, verifier, err := authorizationFlow(ctx, userClient, cfgA)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Exchange the code issued for appA, but presenting appB's client_id
|
||||
// and appB's own valid secret. redirect_uri is identical for both
|
||||
// apps, so both the request-level check (against the app resolved
|
||||
// from client_id) and the grant's own check (against the code's
|
||||
// recorded redirect_uri) pass, isolating the code-ownership check.
|
||||
cfgB := &oauth2.Config{
|
||||
ClientID: appB.ID.String(),
|
||||
ClientSecret: secretB.ClientSecretFull,
|
||||
Endpoint: oauth2.Endpoint{
|
||||
TokenURL: appB.Endpoints.Token,
|
||||
AuthStyle: oauth2.AuthStyleInParams,
|
||||
},
|
||||
RedirectURL: sharedCallback,
|
||||
Scopes: []string{},
|
||||
}
|
||||
_, err = cfgB.Exchange(ctx, code, oauth2.SetAuthURLParam("code_verifier", verifier))
|
||||
require.Error(t, err)
|
||||
require.ErrorContains(t, err, "The authorization code is invalid or expired")
|
||||
}
|
||||
|
||||
func TestOAuth2ProviderTokenRefresh(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
@@ -552,6 +632,12 @@ func TestOAuth2ProviderTokenRefresh(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
app codersdk.OAuth2ProviderApp
|
||||
// refreshAsApp, if set, performs the refresh request under this
|
||||
// app's client_id/endpoints instead of app, while the token itself
|
||||
// still belongs to app. Used to test that refreshing a token under
|
||||
// a different app's client_id is rejected outright, rather than
|
||||
// silently re-parenting the token to the presented client_id.
|
||||
refreshAsApp *codersdk.OAuth2ProviderApp
|
||||
// If null, assume the token should be valid.
|
||||
defaultToken *string
|
||||
error string
|
||||
@@ -593,6 +679,18 @@ func TestOAuth2ProviderTokenRefresh(t *testing.T) {
|
||||
expires: time.Now().Add(time.Minute * -1),
|
||||
error: "The refresh token is invalid or expired",
|
||||
},
|
||||
{
|
||||
// The token belongs to apps.Default, but the refresh request
|
||||
// presents apps.NoPort's client_id. This must be rejected
|
||||
// outright: silently accepting it (and re-parenting the
|
||||
// token's app_id to whatever client_id is presented) would let
|
||||
// a stolen refresh token be laundered to a different app,
|
||||
// after which the issuing app could no longer revoke it.
|
||||
name: "WrongApp",
|
||||
app: apps.Default,
|
||||
refreshAsApp: &apps.NoPort,
|
||||
error: "The refresh token is invalid or expired",
|
||||
},
|
||||
{
|
||||
name: "OK",
|
||||
app: apps.Default,
|
||||
@@ -630,7 +728,8 @@ func TestOAuth2ProviderTokenRefresh(t *testing.T) {
|
||||
ExpiresAt: expires,
|
||||
HashPrefix: []byte(token.Prefix),
|
||||
RefreshHash: token.Hashed,
|
||||
AppSecretID: secret.ID,
|
||||
AppID: test.app.ID,
|
||||
AppSecretID: uuid.NullUUID{UUID: secret.ID, Valid: true},
|
||||
APIKeyID: newKey.ID,
|
||||
UserID: user.ID,
|
||||
})
|
||||
@@ -643,16 +742,20 @@ func TestOAuth2ProviderTokenRefresh(t *testing.T) {
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, user.ID, gotUser.ID)
|
||||
|
||||
refreshAsApp := test.app
|
||||
if test.refreshAsApp != nil {
|
||||
refreshAsApp = *test.refreshAsApp
|
||||
}
|
||||
cfg := &oauth2.Config{
|
||||
ClientID: test.app.ID.String(),
|
||||
ClientID: refreshAsApp.ID.String(),
|
||||
ClientSecret: secret.ClientSecretFull,
|
||||
Endpoint: oauth2.Endpoint{
|
||||
AuthURL: test.app.Endpoints.Authorization,
|
||||
DeviceAuthURL: test.app.Endpoints.DeviceAuth,
|
||||
TokenURL: test.app.Endpoints.Token,
|
||||
AuthURL: refreshAsApp.Endpoints.Authorization,
|
||||
DeviceAuthURL: refreshAsApp.Endpoints.DeviceAuth,
|
||||
TokenURL: refreshAsApp.Endpoints.Token,
|
||||
AuthStyle: oauth2.AuthStyleInParams,
|
||||
},
|
||||
RedirectURL: test.app.CallbackURL,
|
||||
RedirectURL: refreshAsApp.CallbackURL,
|
||||
Scopes: []string{},
|
||||
}
|
||||
|
||||
@@ -855,6 +958,92 @@ func TestOAuth2ProviderRevoke(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestOAuth2ProviderRevokeCrossApp covers RFC 7009 revocation's ownership
|
||||
// check, which compares a token's app_id directly rather than joining
|
||||
// through app_secret_id. That rewrite had zero test coverage on its
|
||||
// unequal branch: revoking a token while presenting a different app's
|
||||
// client_id than the one that issued it must be rejected (masked as a
|
||||
// success per RFC 7009, since revocation must not reveal whether a token
|
||||
// exists), and must leave the token's session intact. Revoking under the
|
||||
// correct, issuing app must still work.
|
||||
func TestOAuth2ProviderRevokeCrossApp(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
ownerClient := coderdtest.New(t, nil)
|
||||
owner := coderdtest.CreateFirstUser(t, ownerClient)
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
apps := generateApps(ctx, t, ownerClient, "revoke-cross-app")
|
||||
|
||||
//nolint:gocritic // OAauth2 app management requires owner permission.
|
||||
secret, err := ownerClient.PostOAuth2ProviderAppSecret(ctx, apps.Default.ID)
|
||||
require.NoError(t, err)
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
// tokenFor extracts the token under test from a successful exchange,
|
||||
// covering both the refresh-token (revokeRefreshTokenInTx) and
|
||||
// access-token (revokeAPIKeyInTx) revocation branches.
|
||||
tokenFor func(*oauth2.Token) string
|
||||
}{
|
||||
{
|
||||
name: "AccessToken",
|
||||
tokenFor: func(tok *oauth2.Token) string { return tok.AccessToken },
|
||||
},
|
||||
{
|
||||
name: "RefreshToken",
|
||||
tokenFor: func(tok *oauth2.Token) string { return tok.RefreshToken },
|
||||
},
|
||||
}
|
||||
for _, test := range tests {
|
||||
t.Run(test.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
|
||||
userClient, _ := coderdtest.CreateAnotherUser(t, ownerClient, owner.OrganizationID)
|
||||
|
||||
cfg := &oauth2.Config{
|
||||
ClientID: apps.Default.ID.String(),
|
||||
ClientSecret: secret.ClientSecretFull,
|
||||
Endpoint: oauth2.Endpoint{
|
||||
AuthURL: apps.Default.Endpoints.Authorization,
|
||||
DeviceAuthURL: apps.Default.Endpoints.DeviceAuth,
|
||||
TokenURL: apps.Default.Endpoints.Token,
|
||||
AuthStyle: oauth2.AuthStyleInParams,
|
||||
},
|
||||
RedirectURL: apps.Default.CallbackURL,
|
||||
Scopes: []string{},
|
||||
}
|
||||
|
||||
code, verifier, err := authorizationFlow(ctx, userClient, cfg)
|
||||
require.NoError(t, err)
|
||||
token, err := cfg.Exchange(ctx, code, oauth2.SetAuthURLParam("code_verifier", verifier))
|
||||
require.NoError(t, err)
|
||||
|
||||
sessionWorks := func() bool {
|
||||
checkClient := codersdk.New(userClient.URL)
|
||||
checkClient.SetSessionToken(token.AccessToken)
|
||||
_, err := checkClient.User(ctx, codersdk.Me)
|
||||
return err == nil
|
||||
}
|
||||
require.True(t, sessionWorks(), "session should be valid before any revoke attempt")
|
||||
|
||||
tokenUnderTest := test.tokenFor(token)
|
||||
|
||||
// RFC 7009: revoking under a different app than the one that
|
||||
// issued the token must not reveal whether it exists (no
|
||||
// error), and must not actually end the session.
|
||||
err = userClient.RevokeOAuth2Token(ctx, apps.NoPort.ID, tokenUnderTest)
|
||||
require.NoError(t, err, "cross-app revoke must appear to succeed per RFC 7009")
|
||||
require.True(t, sessionWorks(), "cross-app revoke must not actually end the session")
|
||||
|
||||
// Revoking under the correct, issuing app must actually work.
|
||||
err = userClient.RevokeOAuth2Token(ctx, apps.Default.ID, tokenUnderTest)
|
||||
require.NoError(t, err)
|
||||
require.False(t, sessionWorks(), "same-app revoke must end the session")
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
type provisionedApps struct {
|
||||
Default codersdk.OAuth2ProviderApp
|
||||
NoPort codersdk.OAuth2ProviderApp
|
||||
|
||||
Reference in New Issue
Block a user