From 663f41ffa9da8b8fae83844ff7ba44160c956293 Mon Sep 17 00:00:00 2001 From: Bobby Ho Date: Tue, 18 Aug 2026 21:12:44 -0700 Subject: [PATCH] feat: derive OAuth2 client type from token_endpoint_auth_method (#28043) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an OAuth2 client type (public vs confidential, RFC 7591 §2) derived from the requested auth method instead of hardcoded confidential. The type is stored and guarded here, but no endpoint enforces on it yet; public behavior at the token endpoint follows in the next PR in the stack. - Client type is derived once and reused by both registration and redirect URI validation, so they can't disagree - IsPublic() fails closed: an unrecognized or missing value reads as confidential - RFC 7592 update (PUT) now rejects moving a client between public and confidential (400) instead of silently flipping it when the auth method is omitted - Discovery still doesn't advertise "none"; follows once the token endpoint honors it ### Behavior by client shape `client_type` is derived from `token_endpoint_auth_method` at POST and pinned at PUT. RFC 7592 GET/PUT authenticate with the registration access token, not the client secret, so neither endpoint reads a secret. | Registered with | Stored `client_type` / method | GET reports | PUT that flips the method | |------------------------------------|---------------------------------------|-----------------------|-----------------------------------------| | omitted, or `client_secret_basic` | `confidential` / `client_secret_basic` | `client_secret_basic` | `none` → 400 `invalid_client_metadata` | | `none` (new) | `public` / `none` | `none` | `client_secret_*` → 400 `invalid_client_metadata` | | `none` (before this PR) | `confidential` / `none` | `none` | either → 200, type stays `confidential` | - PUT still replaces every other RFC 7591 field. `client_type` is the only pinned one; the method may move within a type (`client_secret_basic` ↔ `client_secret_post`). - Row 3 is the only shape where the two columns disagree. The guard fires only on a method change that crosses the type line, so those clients keep managing themselves instead of being locked out of their own configuration endpoint. - The token endpoint does not consult `client_type` yet, so every client still authenticates with a secret and registration still issues one. Split out of #27873, second in the stack (on top of #28041). Refs https://linear.app/codercom/issue/ENG-3029/oauth2-support-public-client --- coderd/database/constants.go | 20 ++ coderd/database/modelmethods.go | 8 + coderd/database/modelmethods_internal_test.go | 32 +++ coderd/oauth2provider/apps.go | 2 +- coderd/oauth2provider/registration.go | 48 +++- coderd/oauth2provider/registration_test.go | 271 ++++++++++++++++++ coderd/oauth2provider/tokens.go | 2 +- codersdk/oauth2.go | 63 +++- codersdk/oauth2_test.go | 88 ++++++ codersdk/oauth2_validation.go | 14 +- site/src/api/typesGenerated.ts | 5 + 11 files changed, 526 insertions(+), 27 deletions(-) create mode 100644 codersdk/oauth2_test.go diff --git a/coderd/database/constants.go b/coderd/database/constants.go index 34ad1005ee..96663b4e92 100644 --- a/coderd/database/constants.go +++ b/coderd/database/constants.go @@ -10,3 +10,23 @@ import ( // for use as a uuid.UUID. Both must agree; tests pin the value to the // codersdk constant so the two cannot drift. var PrebuildsSystemUserID = uuid.MustParse(codersdk.PrebuildsSystemUserID) + +// Values stored in oauth2_provider_apps.client_type, as plain strings for +// comparison against the sqlc-generated string column. +// +// Converted from the codersdk constants rather than redeclared, so the value +// registration writes and the value OAuth2ProviderApp.IsPublic reads back +// cannot disagree. That divergence would fail closed anyway (the app would read +// as confidential and demand a secret it was never issued), but it would fail +// visibly to a client rather than here. +// +// What this does not protect against is the two constants colliding on the same +// value, which would make IsPublic true for confidential apps. Nothing in the +// type system can catch that; the tests that pin these spellings to the wire +// values do, so do not delete them as redundant: +// TestOAuth2ClientRegistrationRequest_DetermineClientType (codersdk) and +// TestOAuth2ProviderAppIsPublic (coderd/database). +const ( + OAuth2ProviderAppClientTypeConfidential = string(codersdk.OAuth2ClientTypeConfidential) + OAuth2ProviderAppClientTypePublic = string(codersdk.OAuth2ClientTypePublic) +) diff --git a/coderd/database/modelmethods.go b/coderd/database/modelmethods.go index fae247adbf..927c352ebf 100644 --- a/coderd/database/modelmethods.go +++ b/coderd/database/modelmethods.go @@ -685,6 +685,14 @@ func (OAuth2ProviderApp) RBACObject() rbac.Object { return rbac.ResourceOauth2App } +// IsPublic reports whether the app is a public (secretless, PKCE-only) +// OAuth2 client per RFC 7591 §2 / OAuth 2.1 §2.1, as opposed to confidential. +// An unset or unrecognized client type reads as confidential, so an app can +// never skip client authentication by accident. +func (a OAuth2ProviderApp) IsPublic() bool { + return a.ClientType == OAuth2ProviderAppClientTypePublic +} + func (a GetOAuth2ProviderAppsByUserIDRow) RBACObject() rbac.Object { return a.OAuth2ProviderApp.RBACObject() } diff --git a/coderd/database/modelmethods_internal_test.go b/coderd/database/modelmethods_internal_test.go index 090e1141b2..f7b35e1a00 100644 --- a/coderd/database/modelmethods_internal_test.go +++ b/coderd/database/modelmethods_internal_test.go @@ -221,6 +221,38 @@ func TestWorkspaceACLDisabled(t *testing.T) { }) } +// TestOAuth2ProviderAppIsPublic pins IsPublic's contract directly, since it is +// what decides whether the token endpoint validates a client secret at all. +// Only the exact string "public" may read as public: anything else, including +// an unset column or a differently-cased value, must read as confidential so +// that a garbled value cannot silently skip client authentication. +func TestOAuth2ProviderAppIsPublic(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + clientType string + want bool + }{ + {name: "Public", clientType: "public", want: true}, + {name: "Confidential", clientType: "confidential", want: false}, + {name: "Empty", clientType: "", want: false}, + {name: "MixedCasePublic", clientType: "Public", want: false}, + {name: "AllCapsPublic", clientType: "PUBLIC", want: false}, + {name: "LeadingSpace", clientType: " public", want: false}, + {name: "TrailingSpace", clientType: "public ", want: false}, + {name: "Bogus", clientType: "bogus", want: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + app := OAuth2ProviderApp{ClientType: tt.clientType} + require.Equal(t, tt.want, app.IsPublic()) + }) + } +} + // Helpers func requirePermission(t *testing.T, s rbac.Scope, resource string, action policy.Action) { t.Helper() diff --git a/coderd/oauth2provider/apps.go b/coderd/oauth2provider/apps.go index da590ab0cd..046f615670 100644 --- a/coderd/oauth2provider/apps.go +++ b/coderd/oauth2provider/apps.go @@ -92,7 +92,7 @@ func CreateApp(db database.Store, accessURL *url.URL, auditor *audit.Auditor, lo Icon: req.Icon, CallbackURL: req.CallbackURL, RedirectUris: []string{}, - ClientType: "confidential", + ClientType: database.OAuth2ProviderAppClientTypeConfidential, DynamicallyRegistered: sql.NullBool{Bool: false, Valid: true}, ClientIDIssuedAt: sql.NullTime{}, ClientSecretExpiresAt: sql.NullTime{}, diff --git a/coderd/oauth2provider/registration.go b/coderd/oauth2provider/registration.go index 2261a5c5b5..67cacfd32c 100644 --- a/coderd/oauth2provider/registration.go +++ b/coderd/oauth2provider/registration.go @@ -101,7 +101,7 @@ func CreateDynamicClientRegistration(db database.Store, accessURL *url.URL, audi Icon: req.LogoURI, CallbackURL: req.RedirectURIs[0], // Primary redirect URI RedirectUris: req.RedirectURIs, - ClientType: req.DetermineClientType(), + ClientType: string(req.DetermineClientType()), DynamicallyRegistered: sql.NullBool{Bool: true, Valid: true}, ClientIDIssuedAt: sql.NullTime{Time: now, Valid: true}, ClientSecretExpiresAt: sql.NullTime{}, // No expiration for now @@ -311,17 +311,49 @@ func UpdateClientConfiguration(db database.Store, auditor *audit.Auditor, logger return } + // A client's type is fixed at registration (RFC 7592 §2.2 permits + // rejecting metadata the server will not accept). Flipping it would + // either drop the secret requirement for a client that has one, or mark + // a client confidential when it has no secret and no way to be issued + // one. + // + // Requiring authMethodChanged means an update that leaves the auth + // method alone is never rejected, so a legacy row whose two columns + // disagree can still manage itself. IsPublic is the reader for the + // stored column so an unrecognized value is treated as confidential + // here exactly as it is at the token endpoint. + storedMethod := codersdk.OAuth2TokenEndpointAuthMethod(existingApp.TokenEndpointAuthMethod.String) + authMethodChanged := req.TokenEndpointAuthMethod != storedMethod + clientTypeChanged := (req.DetermineClientType() == codersdk.OAuth2ClientTypePublic) != existingApp.IsPublic() + if authMethodChanged && clientTypeChanged { + logger.Warn(ctx, "rejected oauth2 client type change", + slog.F("client_id", clientID.String()), + slog.F("stored_token_endpoint_auth_method", existingApp.TokenEndpointAuthMethod.String), + slog.F("requested_token_endpoint_auth_method", string(req.TokenEndpointAuthMethod)), + slog.F("stored_client_type", existingApp.ClientType)) + writeOAuth2RegistrationError(ctx, rw, http.StatusBadRequest, + "invalid_client_metadata", + fmt.Sprintf("token_endpoint_auth_method cannot move an existing client between public and confidential (stored %q, requested %q); the client type is fixed at registration, so register a new client instead", + existingApp.TokenEndpointAuthMethod.String, string(req.TokenEndpointAuthMethod))) + return + } + // Update app in database now := dbtime.Now() //nolint:gocritic // OAuth2 system context — RFC 7592 client configuration endpoint updatedApp, err := db.UpdateOAuth2ProviderAppByClientID(dbauthz.AsSystemOAuth2(ctx), database.UpdateOAuth2ProviderAppByClientIDParams{ - ID: clientID, - UpdatedAt: now, - Name: req.GenerateClientName(), - Icon: req.LogoURI, - CallbackURL: req.RedirectURIs[0], // Primary redirect URI - RedirectUris: req.RedirectURIs, - ClientType: req.DetermineClientType(), + ID: clientID, + UpdatedAt: now, + Name: req.GenerateClientName(), + Icon: req.LogoURI, + CallbackURL: req.RedirectURIs[0], // Primary redirect URI + RedirectUris: req.RedirectURIs, + // Carried through unchanged. The guard above rejects a request that + // would change the type, so re-deriving it here could only ever + // differ for a legacy row whose stored type and auth method + // disagree, silently converting it to public while it still holds a + // secret. + ClientType: existingApp.ClientType, ClientSecretExpiresAt: sql.NullTime{}, // No expiration for now GrantTypes: slice.ToStrings(req.GrantTypes), ResponseTypes: slice.ToStrings(req.ResponseTypes), diff --git a/coderd/oauth2provider/registration_test.go b/coderd/oauth2provider/registration_test.go index f23e82dbf7..7c7ccd0748 100644 --- a/coderd/oauth2provider/registration_test.go +++ b/coderd/oauth2provider/registration_test.go @@ -2,16 +2,22 @@ package oauth2provider_test import ( "bytes" + "context" + "database/sql" "encoding/json" "net/http" "net/http/httptest" "net/url" "testing" + "github.com/go-chi/chi/v5" + "github.com/google/uuid" "github.com/stretchr/testify/require" "cdr.dev/slog/v3/sloggers/slogtest" "github.com/coder/coder/v2/coderd/audit" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbgen" "github.com/coder/coder/v2/coderd/database/dbtestutil" "github.com/coder/coder/v2/coderd/oauth2provider" "github.com/coder/coder/v2/coderd/tracing" @@ -97,3 +103,268 @@ func TestCreateDynamicClientRegistration_DCREnabled(t *testing.T) { }) } } + +// TestUpdateClientConfiguration_ClientTypeIsImmutable verifies that an +// RFC 7592 update cannot move a registered client between public and +// confidential. Allowing it would either drop the secret requirement for a +// client that has a secret, or mark a client confidential when it has no +// secret and no way to be issued one, permanently breaking its token +// exchange. Switching between the two confidential auth methods stays +// allowed, since it changes nothing about how the client authenticates. +func TestUpdateClientConfiguration_ClientTypeIsImmutable(t *testing.T) { + t.Parallel() + + accessURL, err := url.Parse("https://oauth2-registration-immutable-type-test.example.com") + require.NoError(t, err) + + tests := []struct { + name string + registerAs codersdk.OAuth2TokenEndpointAuthMethod + updateTo codersdk.OAuth2TokenEndpointAuthMethod + // omitAuthMethod sends the update with no token_endpoint_auth_method at + // all, which ApplyDefaults rewrites to client_secret_basic before the + // guard sees it. updateTo is ignored when set. + omitAuthMethod bool + wantStatus int + wantFinalCallback string + // wantClientType is deliberately a bare literal rather than the + // database constant: it pins the value actually stored in the column, + // so it must fail if that spelling ever changes. Fixtures that set + // state use the constant instead. + wantClientType string + }{ + { + name: "ConfidentialToPublicIsRejected", + registerAs: codersdk.OAuth2TokenEndpointAuthMethodClientSecretBasic, + updateTo: codersdk.OAuth2TokenEndpointAuthMethodNone, + wantStatus: http.StatusBadRequest, + wantClientType: "confidential", + }, + { + name: "PublicToConfidentialIsRejected", + registerAs: codersdk.OAuth2TokenEndpointAuthMethodNone, + updateTo: codersdk.OAuth2TokenEndpointAuthMethodClientSecretBasic, + wantStatus: http.StatusBadRequest, + wantClientType: "public", + }, + { + // RFC 7592 makes PUT a full replacement, so an omitted auth method + // defaults to client_secret_basic and moves a public client to + // confidential, which is rejected. The rejection is correct; what + // matters is that it is reported in terms the caller can act on, + // since they never sent the field named in the error. + name: "PublicWithOmittedAuthMethodIsRejected", + registerAs: codersdk.OAuth2TokenEndpointAuthMethodNone, + omitAuthMethod: true, + wantStatus: http.StatusBadRequest, + wantClientType: "public", + }, + { + // Both are confidential, so the guard must not fire. + name: "BasicToPostIsAllowed", + registerAs: codersdk.OAuth2TokenEndpointAuthMethodClientSecretBasic, + updateTo: codersdk.OAuth2TokenEndpointAuthMethodClientSecretPost, + wantStatus: http.StatusOK, + wantFinalCallback: "https://example.com/updated-callback", + wantClientType: "confidential", + }, + { + // A confidential client omitting the field is unaffected, because + // the default it lands on is also confidential. Pinned so the + // asymmetry with the public case above stays visible. + name: "ConfidentialWithOmittedAuthMethodIsAllowed", + registerAs: codersdk.OAuth2TokenEndpointAuthMethodClientSecretPost, + omitAuthMethod: true, + wantStatus: http.StatusOK, + wantFinalCallback: "https://example.com/updated-callback", + wantClientType: "confidential", + }, + { + name: "PublicToPublicIsAllowed", + registerAs: codersdk.OAuth2TokenEndpointAuthMethodNone, + updateTo: codersdk.OAuth2TokenEndpointAuthMethodNone, + wantStatus: http.StatusOK, + wantFinalCallback: "https://example.com/updated-callback", + wantClientType: "public", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + db, _ := dbtestutil.NewDB(t) + require.NoError(t, db.UpsertOAuth2DCREnabled(ctx, true)) + + logger := slogtest.Make(t, nil) + auditor := audit.NewNop() + + // Register the client first, so the update runs against a real + // persisted client_type rather than a hand-built fixture. + createHandler := tracing.StatusWriterMiddleware(oauth2provider.CreateDynamicClientRegistration(db, accessURL, &auditor, logger)) + createBody, err := json.Marshal(codersdk.OAuth2ClientRegistrationRequest{ + RedirectURIs: []string{"https://example.com/callback"}, + TokenEndpointAuthMethod: tt.registerAs, + }) + require.NoError(t, err) + + createReq := httptest.NewRequest(http.MethodPost, "/oauth2/register", bytes.NewReader(createBody)).WithContext(ctx) + createReq.Header.Set("Content-Type", "application/json") + createRW := httptest.NewRecorder() + createHandler.ServeHTTP(createRW, createReq) + require.Equal(t, http.StatusCreated, createRW.Code) + + var created codersdk.OAuth2ClientRegistrationResponse + require.NoError(t, json.Unmarshal(createRW.Body.Bytes(), &created)) + clientID, err := uuid.Parse(created.ClientID) + require.NoError(t, err) + + updateHandler := tracing.StatusWriterMiddleware(oauth2provider.UpdateClientConfiguration(db, &auditor, logger)) + updateReqBody := codersdk.OAuth2ClientRegistrationRequest{ + RedirectURIs: []string{"https://example.com/updated-callback"}, + } + if !tt.omitAuthMethod { + updateReqBody.TokenEndpointAuthMethod = tt.updateTo + } + updateBody, err := json.Marshal(updateReqBody) + require.NoError(t, err) + + // The handler reads client_id via chi.URLParam, which normally + // comes from the router in coderd.go. + rctx := chi.NewRouteContext() + rctx.URLParams.Add("client_id", clientID.String()) + updateCtx := context.WithValue(ctx, chi.RouteCtxKey, rctx) + + updateReq := httptest.NewRequest(http.MethodPut, "/oauth2/clients/"+clientID.String(), bytes.NewReader(updateBody)).WithContext(updateCtx) + updateReq.Header.Set("Content-Type", "application/json") + updateRW := httptest.NewRecorder() + updateHandler.ServeHTTP(updateRW, updateReq) + require.Equal(t, tt.wantStatus, updateRW.Code) + + app, err := db.GetOAuth2ProviderAppByClientID(ctx, clientID) + require.NoError(t, err) + + // client_type is what IsPublic() reads to decide whether the token + // endpoint validates a secret, so it must be unchanged whether the + // update was accepted or rejected. + require.Equal(t, tt.wantClientType, app.ClientType) + + if tt.wantStatus != http.StatusOK { + var errResp map[string]string + require.NoError(t, json.Unmarshal(updateRW.Body.Bytes(), &errResp)) + require.Equal(t, "invalid_client_metadata", errResp["error"]) + // The error code alone cannot distinguish this guard from + // req.Validate() failing, which returns the same one, and + // neither can the untouched row below. The description is the + // only field that tells them apart, so assert on it: a change + // that made "none" fail validation outright would otherwise + // leave these cases green while testing something else. + require.Contains(t, errResp["error_description"], "cannot move an existing client between public and confidential") + // It must also name what the server actually compared, since + // the caller may never have sent the field. + require.Contains(t, errResp["error_description"], "client_secret_basic") + // The rejection must leave the whole update unapplied, not + // just the client_type field. + require.Equal(t, "https://example.com/callback", app.CallbackURL) + return + } + + require.Equal(t, tt.wantFinalCallback, app.CallbackURL) + wantMethod := tt.updateTo + if tt.omitAuthMethod { + // ApplyDefaults substitutes the RFC 7591 default. + wantMethod = codersdk.OAuth2TokenEndpointAuthMethodClientSecretBasic + } + require.Equal(t, string(wantMethod), app.TokenEndpointAuthMethod.String) + }) + } +} + +// TestUpdateClientConfiguration_LegacyAuthMethodMismatch covers clients that +// registered before client_type was derived from token_endpoint_auth_method. +// Registration persisted whatever auth method was requested while hardcoding +// client_type to "confidential", and "none" has always passed validation, so +// apps stored as confidential with an auth method of "none" exist in any +// deployment where a native or MCP client self-registered. That is the exact +// population public clients are for. +// +// Such a client must still be able to manage its registration. Comparing only +// the derived client type would reject it forever, including when it resends +// the metadata GET reports, leaving re-registration as the only recovery. It +// must also not be silently converted to public, since it holds a secret that +// would stop being required. +func TestUpdateClientConfiguration_LegacyAuthMethodMismatch(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + updateTo codersdk.OAuth2TokenEndpointAuthMethod + }{ + { + // The read-modify-write shape: echo back what GET reports. + name: "ResendingStoredAuthMethodIsAccepted", + updateTo: codersdk.OAuth2TokenEndpointAuthMethodNone, + }, + { + // Moving to a secret-based method matches the stored confidential + // type, so it is allowed and repairs the divergence. + name: "MovingToSecretBasedMethodIsAccepted", + updateTo: codersdk.OAuth2TokenEndpointAuthMethodClientSecretBasic, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + db, _ := dbtestutil.NewDB(t) + require.NoError(t, db.UpsertOAuth2DCREnabled(ctx, true)) + + legacy := dbgen.OAuth2ProviderApp(t, db, database.OAuth2ProviderApp{ + CallbackURL: "https://example.com/callback", + RedirectUris: []string{"https://example.com/callback"}, + ClientType: database.OAuth2ProviderAppClientTypeConfidential, + TokenEndpointAuthMethod: sql.NullString{String: "none", Valid: true}, + DynamicallyRegistered: sql.NullBool{Bool: true, Valid: true}, + }) + // Registration issued a secret unconditionally back then. + _ = dbgen.OAuth2ProviderAppSecret(t, db, database.OAuth2ProviderAppSecret{AppID: legacy.ID}) + + logger := slogtest.Make(t, nil) + auditor := audit.NewNop() + handler := tracing.StatusWriterMiddleware(oauth2provider.UpdateClientConfiguration(db, &auditor, logger)) + + body, err := json.Marshal(codersdk.OAuth2ClientRegistrationRequest{ + RedirectURIs: []string{"https://example.com/updated-callback"}, + TokenEndpointAuthMethod: tt.updateTo, + }) + require.NoError(t, err) + + rctx := chi.NewRouteContext() + rctx.URLParams.Add("client_id", legacy.ID.String()) + r := httptest.NewRequest(http.MethodPut, "/oauth2/clients/"+legacy.ID.String(), + bytes.NewReader(body)).WithContext(context.WithValue(ctx, chi.RouteCtxKey, rctx)) + r.Header.Set("Content-Type", "application/json") + rw := httptest.NewRecorder() + + handler.ServeHTTP(rw, r) + require.Equal(t, http.StatusOK, rw.Code, "body: %s", rw.Body.String()) + + app, err := db.GetOAuth2ProviderAppByClientID(ctx, legacy.ID) + require.NoError(t, err) + require.Equal(t, "https://example.com/updated-callback", app.CallbackURL) + require.Equal(t, string(tt.updateTo), app.TokenEndpointAuthMethod.String) + + // The update must not convert the client to public. It still holds + // a secret, and IsPublic() reading "public" here would stop the + // token endpoint from requiring it. + require.Equal(t, "confidential", app.ClientType) + require.False(t, app.IsPublic()) + secrets, err := db.GetOAuth2ProviderAppSecretsByAppID(ctx, legacy.ID) + require.NoError(t, err) + require.Len(t, secrets, 1) + }) + } +} diff --git a/coderd/oauth2provider/tokens.go b/coderd/oauth2provider/tokens.go index 1beedf8cc3..bb4afbc290 100644 --- a/coderd/oauth2provider/tokens.go +++ b/coderd/oauth2provider/tokens.go @@ -288,7 +288,7 @@ func authorizationCodeGrant(ctx context.Context, db database.Store, app database // The secret must belong to the app identified by the request's // client_id, which is otherwise unauthenticated at this point (it is // parsed straight from the request with no verification). Without this - // check, a valid secret for one app could mint a token attributed to a + // check, a valid secret for one app could issue a token attributed to a // different app. if dbSecret.AppID != app.ID { return codersdk.OAuth2TokenResponse{}, errBadSecret diff --git a/codersdk/oauth2.go b/codersdk/oauth2.go index 679e5eea11..a9c2993dc4 100644 --- a/codersdk/oauth2.go +++ b/codersdk/oauth2.go @@ -7,6 +7,7 @@ import ( "fmt" "net/http" "net/url" + "slices" "strings" "time" @@ -269,11 +270,44 @@ const ( OAuth2TokenEndpointAuthMethodNone OAuth2TokenEndpointAuthMethod = "none" ) -func (m OAuth2TokenEndpointAuthMethod) Valid() bool { - switch m { - case OAuth2TokenEndpointAuthMethodClientSecretBasic, +// AllOAuth2TokenEndpointAuthMethods returns every accepted token endpoint auth +// method. Valid() is defined in terms of it, so what registration accepts +// cannot drift from what this function reports. +// +// Discovery metadata does not yet derive from it: +// coderd/oauth2provider/metadata.go's TokenEndpointAuthMethodsSupported is +// hardcoded to {client_secret_basic, client_secret_post} and does not +// advertise "none", even though "none" is accepted here. A follow-up PR +// wires the token endpoint to honor "none"; only once that lands should +// discovery advertise it too. +func AllOAuth2TokenEndpointAuthMethods() []OAuth2TokenEndpointAuthMethod { + return []OAuth2TokenEndpointAuthMethod{ + OAuth2TokenEndpointAuthMethodClientSecretBasic, OAuth2TokenEndpointAuthMethodClientSecretPost, - OAuth2TokenEndpointAuthMethodNone: + OAuth2TokenEndpointAuthMethodNone, + } +} + +func (m OAuth2TokenEndpointAuthMethod) Valid() bool { + return slices.Contains(AllOAuth2TokenEndpointAuthMethods(), m) +} + +// OAuth2ClientType is how a client authenticates at the token endpoint +// (RFC 7591 §2, OAuth 2.1 §2.1). A confidential client authenticates with a +// secret; a public client authenticates with PKCE alone. It is derived from +// the requested token_endpoint_auth_method and stored on the app. A +// follow-up PR wires the token endpoint to read it when deciding whether to +// require a client secret. +type OAuth2ClientType string + +const ( + OAuth2ClientTypeConfidential OAuth2ClientType = "confidential" + OAuth2ClientTypePublic OAuth2ClientType = "public" +) + +func (t OAuth2ClientType) Valid() bool { + switch t { + case OAuth2ClientTypeConfidential, OAuth2ClientTypePublic: return true } return false @@ -527,14 +561,19 @@ func (req OAuth2ClientRegistrationRequest) ApplyDefaults() OAuth2ClientRegistrat return req } -// DetermineClientType determines if client is public or confidential -func (*OAuth2ClientRegistrationRequest) DetermineClientType() string { - // For now, default to confidential - // In the future, we might detect based on: - // - token_endpoint_auth_method == "none" -> public - // - application_type == "native" -> might be public - // - Other heuristics - return "confidential" +// DetermineClientType determines if client is public or confidential, based +// on the requested token_endpoint_auth_method (RFC 7591 §2, OAuth 2.1 §2.1). +// +// Only "none" reads as public; every other value, including an omitted one, +// reads as confidential, so this is safe to call before ApplyDefaults(). A +// caller that also compares the request's auth method against a stored one must +// apply defaults first, or an omitted field compares as "" and looks like a +// change the client did not request. +func (req *OAuth2ClientRegistrationRequest) DetermineClientType() OAuth2ClientType { + if req.TokenEndpointAuthMethod == OAuth2TokenEndpointAuthMethodNone { + return OAuth2ClientTypePublic + } + return OAuth2ClientTypeConfidential } // GenerateClientName generates a client name if not provided diff --git a/codersdk/oauth2_test.go b/codersdk/oauth2_test.go new file mode 100644 index 0000000000..e75a5e3b52 --- /dev/null +++ b/codersdk/oauth2_test.go @@ -0,0 +1,88 @@ +package codersdk_test + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/codersdk" +) + +// TestOAuth2ClientRegistrationRequest_DetermineClientType verifies that the +// client type is derived from the requested token_endpoint_auth_method +// (RFC 7591 §2, OAuth 2.1 §2.1), not hardcoded to "confidential". +func TestOAuth2ClientRegistrationRequest_DetermineClientType(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + authMethod codersdk.OAuth2TokenEndpointAuthMethod + // applyDefaults runs ApplyDefaults() before DetermineClientType(), + // matching the real request path where an omitted auth method is + // defaulted to "client_secret_basic" before this check ever runs. + applyDefaults bool + // wantAuthMethodAfterDefaults pins what ApplyDefaults() does to + // authMethod, so a case that runs applyDefaults also verifies + // ApplyDefaults left (or changed) the field as expected before + // DetermineClientType() reads it. Only checked when applyDefaults + // is true. + wantAuthMethodAfterDefaults codersdk.OAuth2TokenEndpointAuthMethod + expectedType string + }{ + { + name: "NoneIsPublic", + authMethod: codersdk.OAuth2TokenEndpointAuthMethodNone, + expectedType: "public", + }, + { + name: "ClientSecretBasicIsConfidential", + authMethod: codersdk.OAuth2TokenEndpointAuthMethodClientSecretBasic, + expectedType: "confidential", + }, + { + name: "ClientSecretPostIsConfidential", + authMethod: codersdk.OAuth2TokenEndpointAuthMethodClientSecretPost, + expectedType: "confidential", + }, + { + // ApplyDefaults only fills an empty auth method; it must not + // touch an explicit "none". If it ever grew a rule that did, + // the pre-defaults Validate() call and the post-defaults + // storage call would disagree about this client's type. + name: "NoneStaysPublicAfterApplyDefaults", + authMethod: codersdk.OAuth2TokenEndpointAuthMethodNone, + applyDefaults: true, + wantAuthMethodAfterDefaults: codersdk.OAuth2TokenEndpointAuthMethodNone, + expectedType: "public", + }, + { + // An omitted auth method must not be read as public. Without + // ApplyDefaults the empty string also falls through to + // confidential, so this is safe in either order, but the real + // path always defaults first. + name: "OmittedDefaultsToConfidentialAfterApplyDefaults", + applyDefaults: true, + wantAuthMethodAfterDefaults: codersdk.OAuth2TokenEndpointAuthMethodClientSecretBasic, + expectedType: "confidential", + }, + { + name: "OmittedIsConfidentialWithoutApplyDefaults", + expectedType: "confidential", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + req := codersdk.OAuth2ClientRegistrationRequest{ + TokenEndpointAuthMethod: tt.authMethod, + } + if tt.applyDefaults { + req = req.ApplyDefaults() + require.Equal(t, tt.wantAuthMethodAfterDefaults, req.TokenEndpointAuthMethod) + } + require.Equal(t, tt.expectedType, string(req.DetermineClientType())) + }) + } +} diff --git a/codersdk/oauth2_validation.go b/codersdk/oauth2_validation.go index 34d0e37665..9c61739c79 100644 --- a/codersdk/oauth2_validation.go +++ b/codersdk/oauth2_validation.go @@ -16,7 +16,9 @@ func (req *OAuth2ClientRegistrationRequest) Validate() error { return xerrors.New("redirect_uris is required for authorization code flow") } - if err := validateRedirectURIs(req.RedirectURIs, req.TokenEndpointAuthMethod); err != nil { + // The client type is derived once, by DetermineClientType, so which RFC 8252 + // rules apply here cannot drift from what gets stored in client_type. + if err := validateRedirectURIs(req.RedirectURIs, req.DetermineClientType()); err != nil { return xerrors.Errorf("invalid redirect_uris: %w", err) } @@ -118,8 +120,11 @@ func validateScheme(u *url.URL) error { return nil } -// validateRedirectURIs validates redirect URIs according to RFC 7591, 8252 -func validateRedirectURIs(uris []string, tokenEndpointAuthMethod OAuth2TokenEndpointAuthMethod) error { +// validateRedirectURIs validates redirect URIs according to RFC 7591, 8252. +// clientType selects which rules apply and is derived by DetermineClientType, +// the single owner of that mapping, so this cannot disagree with the type the +// app is stored as. +func validateRedirectURIs(uris []string, clientType OAuth2ClientType) error { if len(uris) == 0 { return xerrors.New("at least one redirect URI is required") } @@ -144,8 +149,7 @@ func validateRedirectURIs(uris []string, tokenEndpointAuthMethod OAuth2TokenEndp continue } - // Determine if this is a public client based on token endpoint auth method - isPublicClient := tokenEndpointAuthMethod == OAuth2TokenEndpointAuthMethodNone + isPublicClient := clientType == OAuth2ClientTypePublic // Handle different validation for public vs confidential clients if uri.Scheme == "http" || uri.Scheme == "https" { diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index b9c456118b..56b0f4825b 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -6534,6 +6534,11 @@ export interface OAuth2ClientRegistrationResponse { readonly registration_client_uri: string; } +// From codersdk/oauth2.go +export type OAuth2ClientType = "confidential" | "public"; + +export const OAuth2ClientTypes: OAuth2ClientType[] = ["confidential", "public"]; + // From codersdk/deployment.go export interface OAuth2Config { readonly github: OAuth2GithubConfig;