From d12c47c5349139747d15f9f3eba048556136f5c1 Mon Sep 17 00:00:00 2001 From: Bobby Ho Date: Tue, 11 Aug 2026 15:59:43 -0700 Subject: [PATCH] feat: validate and persist OAuth2 authorization scope /oauth2/authorize ignored the scope parameter entirely and wrote a hardcoded coder:all onto every authorization code. It now negotiates: each requested scope must be in the external scope catalog (rbac.IsExternalScope), and the result must fall within the app's configured allowlist, which is itself filtered through the same catalog. An omitted scope defaults to the filtered allowlist per RFC 6749 section 3.3. The negotiated value is persisted on oauth2_provider_app_codes.scope, replacing the placeholder written when the column was added. Both GET and POST validate, so a request that cannot succeed is rejected before the consent page renders rather than after the user clicks Allow. This matches how the handler already treats PKCE's code_challenge requirement. Two cases produce an empty result and are handled deliberately differently. An app with no allowlist and no requested scope keeps today's unrestricted grant, spelled as the explicit coder:all sentinel because the column is NOT NULL with a non-empty CHECK. An app whose allowlist filters to nothing is rejected instead, since falling back there would grant strictly more than the allowlist ever permitted. NULL and the empty string are one "no allowlist configured" state, unified in a single predicate now that reading the column is an authorization decision. Accepted compatibility break: dynamic client registration performs no catalog validation, so apps registered with scopes such as openid or admin hold allowlists this server cannot grant from. Those apps now fail authorization in both directions with invalid_scope. Grandfathering unknown names through would seed api_keys.scopes with values dbauthz cannot evaluate, trading a visible negotiation-time error for a silent enforcement-time hole. Registration-time expectations are unchanged; the two tests asserting registration accepts these values carried comments promising the opposite of what authorization does, and those were corrected. Issued tokens are not yet restricted: authorizationCodeGrant still mints rbac.ScopeAll and does not read the persisted column. That lands with the grant path. Refs PLAT-479 --- coderd/oauth2_metadata_validation_test.go | 18 +- coderd/oauth2provider/authorize.go | 127 +++++++- .../oauth2provider/authorize_internal_test.go | 182 +++++++++++ coderd/oauth2provider/authorize_test.go | 299 ++++++++++++++++++ coderd/oauth2provider/validation_test.go | 21 +- 5 files changed, 633 insertions(+), 14 deletions(-) diff --git a/coderd/oauth2_metadata_validation_test.go b/coderd/oauth2_metadata_validation_test.go index 01b2143f5a6..3bce27a8afd 100644 --- a/coderd/oauth2_metadata_validation_test.go +++ b/coderd/oauth2_metadata_validation_test.go @@ -541,7 +541,15 @@ func TestOAuth2ClientNameValidation(t *testing.T) { } } -// TestOAuth2ClientScopeValidation tests scope parameter validation +// TestOAuth2ClientScopeValidation tests scope parameter validation at +// registration time, which accepts any syntactically valid scope string. +// +// Registration performs no scope catalog validation, so these values are +// stored verbatim as the app's scope allowlist. Authorization is where the +// catalog is enforced: none of the names below is in rbac.IsExternalScope, so +// an app registered with one can no longer complete an authorization, whether +// it requests that scope or omits scope entirely. See +// TestOAuth2AuthorizeDCRScopeCompatibility in coderd/oauth2provider. func TestOAuth2ClientScopeValidation(t *testing.T) { t.Parallel() @@ -596,9 +604,11 @@ func TestOAuth2ClientScopeValidation(t *testing.T) { expectError: false, }, { - name: "InvalidAdmin", - scope: "admin", - expectError: false, // Admin scope should be allowed but validated during authorization + name: "InvalidAdmin", + scope: "admin", + // Registration accepts it; authorization rejects it with + // invalid_scope, since "admin" is not a grantable scope name. + expectError: false, }, { name: "ValidCustom", diff --git a/coderd/oauth2provider/authorize.go b/coderd/oauth2provider/authorize.go index 7e03c1ab860..d25b7d878b4 100644 --- a/coderd/oauth2provider/authorize.go +++ b/coderd/oauth2provider/authorize.go @@ -19,10 +19,100 @@ import ( "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/httpapi" "github.com/coder/coder/v2/coderd/httpmw" + "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/site" ) +// noScopeAllowlist reports whether an app has no scope allowlist configured. +// NULL and "" are one state, and this is the only place the two are unified: +// admin-created apps store sql.NullString{} (apps.go), while DCR-registered +// apps store Valid: true carrying a possibly-empty req.Scope +// (registration.go). Once the allowlist decides what a token may do, reading +// it is an authorization decision, so the two encodings route through one +// predicate rather than each caller flattening via .String. +// +// A whitespace-only allowlist is deliberately not this state. It is a +// configured value that grants nothing, so it falls through to +// validateRequestedScope's filtered-to-empty rejection instead of the +// unrestricted fallback. +func noScopeAllowlist(appScope sql.NullString) bool { + return !appScope.Valid || appScope.String == "" +} + +// validateRequestedScope checks each requested scope token is a recognized, +// user-requestable scope (RFC 6749 §4.1.2.1 invalid_scope), and that the full +// requested set is covered by the app's configured scope allowlist. If the +// client requested no scope, it defaults to the app's allowlist (RFC 6749 +// §3.3). If the app has no allowlist configured, it preserves today's +// unrestricted behavior. +// +// The return value is written directly to a NOT NULL column whose CHECK +// constraint also rejects the empty string, so it is a string rather than a +// []string, and it is never empty alongside a nil error. +func validateRequestedScope(requested []string, appScope sql.NullString) (string, error) { + // Only names in the external scope catalog (rbac.IsExternalScope) are + // user-requestable. That is a curation, not a validity check: RBAC can + // expand internal-only names such as debug_info:read just fine, and the + // api_key_scope enum would store them, which is exactly why the catalog + // exists as a narrower list. Checking here keeps both an unrecognizable + // name and an internal-only one out of the granted scope, whether or not + // the app has an allowlist to check against. + for _, s := range requested { + if !rbac.IsExternalScope(rbac.ScopeName(s)) { + return "", xerrors.Errorf("unknown or unsupported scope: %q", s) + } + } + + if noScopeAllowlist(appScope) { + if len(requested) == 0 { + // Unrestricted, the same grant this app got before scope + // enforcement existed, but stated explicitly: an empty string + // would violate the column's CHECK. + return database.OAuth2ScopeUnrestricted, nil + } + return strings.Join(requested, " "), nil + } + + // Filter the allowlist through IsExternalScope before it is used for + // anything. The allowlist was stored at registration time and may contain + // a scope name since removed from the curated catalog, or never in it at + // all. Filtering only ever narrows what is granted. + allowed := strings.Fields(appScope.String) + filtered := make([]string, 0, len(allowed)) + for _, a := range allowed { + if rbac.IsExternalScope(rbac.ScopeName(a)) { + filtered = append(filtered, a) + } + } + if len(filtered) == 0 { + // The app has an allowlist, but no entry in it is grantable. + // Returning the unrestricted sentinel here would grant strictly more + // than the allowlist ever permitted, so reject instead. This is the + // all-entries-dropped counterpart to the single-stale-entry case the + // filter above handles, and it must not share the no-allowlist + // branch's fallback. + return "", xerrors.New("this app's allowed scope list contains no grantable scope") + } + + if len(requested) == 0 { + return strings.Join(filtered, " "), nil // RFC 6749 §3.3 default + } + + // The subset check runs against the filtered allowlist, not the raw one, + // so a dropped entry cannot be requested explicitly either. + allowedSet := make(map[string]bool, len(filtered)) + for _, a := range filtered { + allowedSet[a] = true + } + for _, s := range requested { + if !allowedSet[s] { + return "", xerrors.Errorf("scope %q is not in this app's allowed scope list", s) + } + } + return strings.Join(requested, " "), nil +} + type authorizeParams struct { clientID string redirectURL *url.URL @@ -144,6 +234,27 @@ func ShowAuthorizePage(accessURL *url.URL) http.HandlerFunc { return } + // Reject a scope the app can never be granted before the consent page + // renders, rather than after the user clicks Allow. This mirrors how + // the PKCE code_challenge requirement is already handled: inside + // extractAuthorizeParams, which both GET and POST call. + if _, err := validateRequestedScope(params.scope, app.Scope); err != nil { + site.RenderStaticErrorPage(rw, r, site.ErrorPageData{ + Status: http.StatusBadRequest, + HideStatus: false, + Title: "Invalid Scope", + Description: "The requested scope is invalid or exceeds what this application is allowed to request.", + Warnings: []string{err.Error()}, + Actions: []site.Action{ + { + URL: accessURL.String(), + Text: "Back to site", + }, + }, + }) + return + } + cancel := params.redirectURL cancelQuery := params.redirectURL.Query() cancelQuery.Add("error", "access_denied") @@ -222,7 +333,12 @@ func ProcessAuthorize(db database.Store) http.HandlerFunc { return } - // TODO: Ignoring scope for now, but should look into implementing. + grantedScope, err := validateRequestedScope(params.scope, app.Scope) + if err != nil { + httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeInvalidScope, err.Error()) + return + } + code, err := GenerateSecret() if err != nil { httpapi.WriteOAuth2Error(r.Context(), rw, http.StatusInternalServerError, codersdk.OAuth2ErrorCodeServerError, "Failed to generate OAuth2 app authorization code") @@ -259,11 +375,10 @@ func ProcessAuthorize(db database.Store) http.HandlerFunc { CodeChallengeMethod: sql.NullString{String: params.codeChallengeMethod, Valid: params.codeChallengeMethod != ""}, StateHash: hashOAuth2State(params.state), RedirectUri: sql.NullString{String: params.redirectURL.String(), Valid: params.redirectURIProvided}, - // Scope negotiation lands in a later phase. Until the - // requested scope is validated against the app's allowlist, - // persisting it here would store unvalidated client input, so - // the code records an unrestricted grant. - Scope: database.OAuth2ScopeUnrestricted, + // The negotiated scope, not the requested one: it has been + // checked against the scope catalog and the app's allowlist, + // and it is what the token minted from this code will carry. + Scope: grantedScope, }) if err != nil { return xerrors.Errorf("insert oauth2 authorization code: %w", err) diff --git a/coderd/oauth2provider/authorize_internal_test.go b/coderd/oauth2provider/authorize_internal_test.go index 4f2d3fc9937..2654af1fd8a 100644 --- a/coderd/oauth2provider/authorize_internal_test.go +++ b/coderd/oauth2provider/authorize_internal_test.go @@ -2,13 +2,195 @@ package oauth2provider import ( "crypto/sha256" + "database/sql" "encoding/hex" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + "github.com/coder/coder/v2/coderd/database" ) +func TestValidateRequestedScope(t *testing.T) { + t.Parallel() + + // Every scope name below is either in rbac.IsExternalScope's curated + // catalog or deliberately outside it; the test's meaning depends on which, + // so they are named rather than inlined. + const ( + inCatalog = "coder:workspaces.access" + alsoInCatalog = "coder:templates.build" + notInCatalog = "some_removed_scope" + neverInCatalog = "openid" + ) + + noAllowlist := sql.NullString{} + emptyAllowlist := sql.NullString{String: "", Valid: true} + + tests := []struct { + name string + requested []string + appScope sql.NullString + want string + wantErr bool + }{ + { + name: "UnknownRequestedScopeRejected", + requested: []string{"not_a_real_scope"}, + appScope: sql.NullString{String: inCatalog, Valid: true}, + wantErr: true, + }, + { + // The catalog check does not depend on the allowlist, so an + // unknown scope is rejected even where there is nothing to + // check it against. + name: "UnknownRequestedScopeRejectedWithoutAllowlist", + requested: []string{"not_a_real_scope"}, + appScope: noAllowlist, + wantErr: true, + }, + { + // A different rejection from the case above, and the one that + // matters more: debug_info:read is a real scope RBAC can expand + // and the api_key_scope enum can store. Only the catalog's + // curation keeps a client from negotiating an internal-only + // permission for itself. + name: "InternalOnlyScopeRejected", + requested: []string{"debug_info:read"}, + appScope: noAllowlist, + wantErr: true, + }, + { + // AC3/AC16: the literal return value matters. "" is exactly what + // the column's CHECK rejects, so asserting only "no error" would + // let a DB-level 500 through. + name: "NoAllowlistOmittedRequestIsUnrestricted", + requested: nil, + appScope: noAllowlist, + want: database.OAuth2ScopeUnrestricted, + }, + { + // AC16/Edge Case 22: '' is the DCR-registered encoding of the + // same "no allowlist configured" state NULL expresses for + // admin-created apps. Both must reach the same branch. + name: "EmptyAllowlistBehavesAsNoAllowlist", + requested: nil, + appScope: emptyAllowlist, + want: database.OAuth2ScopeUnrestricted, + }, + { + name: "NoAllowlistExplicitRequestPassesThrough", + requested: []string{inCatalog}, + appScope: noAllowlist, + want: inCatalog, + }, + { + name: "EmptyAllowlistExplicitRequestPassesThrough", + requested: []string{inCatalog}, + appScope: emptyAllowlist, + want: inCatalog, + }, + { + // RFC 6749 §3.3: an omitted scope defaults to the app's allowlist. + name: "OmittedRequestDefaultsToAllowlist", + requested: nil, + appScope: sql.NullString{String: inCatalog + " " + alsoInCatalog, Valid: true}, + want: inCatalog + " " + alsoInCatalog, + }, + { + name: "ExactMatchAccepted", + requested: []string{inCatalog}, + appScope: sql.NullString{String: inCatalog, Valid: true}, + want: inCatalog, + }, + { + name: "GenuineSubsetAccepted", + requested: []string{alsoInCatalog}, + appScope: sql.NullString{String: inCatalog + " " + alsoInCatalog, Valid: true}, + want: alsoInCatalog, + }, + { + name: "PartiallyOutOfAllowlistRejected", + requested: []string{inCatalog, "template:read"}, + appScope: sql.NullString{String: inCatalog, Valid: true}, + wantErr: true, + }, + { + // Edge Case 20: catalog drift. The stale entry is dropped by the + // filter, and the surviving entry is still granted. + name: "StaleAllowlistEntryDroppedNotGranted", + requested: nil, + appScope: sql.NullString{String: inCatalog + " " + notInCatalog, Valid: true}, + want: inCatalog, + }, + { + // The filter applies to the subset check too, so a dropped entry + // cannot be reached by requesting it explicitly either. + name: "StaleAllowlistEntryNotRequestableExplicitly", + requested: []string{notInCatalog}, + appScope: sql.NullString{String: inCatalog + " " + notInCatalog, Valid: true}, + wantErr: true, + }, + { + // AC15/Edge Case 19: the all-entries-dropped counterpart to the + // case above. Falling back to the unrestricted sentinel here would + // grant strictly more than this allowlist ever permitted. + name: "AllowlistFilteringToEmptyRejected", + requested: nil, + appScope: sql.NullString{String: "openid profile email", Valid: true}, + wantErr: true, + }, + { + // §4.2.2's compatibility break, in its most direct form: a DCR + // client requesting exactly what it registered. + name: "NonCatalogScopeRequestedAsRegistered", + requested: []string{neverInCatalog}, + appScope: sql.NullString{String: neverInCatalog, Valid: true}, + wantErr: true, + }, + { + // A whitespace-only allowlist is a configured value that grants + // nothing, not an unset one, so it rejects rather than falling + // back to unrestricted. + name: "WhitespaceOnlyAllowlistRejected", + requested: nil, + appScope: sql.NullString{String: " ", Valid: true}, + wantErr: true, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + got, err := validateRequestedScope(test.requested, test.appScope) + if test.wantErr { + require.Error(t, err) + assert.Empty(t, got, "a rejected request must not return a persistable scope") + return + } + require.NoError(t, err) + assert.Equal(t, test.want, got) + // The return value goes straight to a NOT NULL column carrying + // CHECK (scope <> ''), so an empty success is never legal. + assert.NotEmpty(t, got, "a successful negotiation must never return an empty scope") + }) + } +} + +func TestNoScopeAllowlist(t *testing.T) { + t.Parallel() + + // NULL and '' are one state. Both are produced in the tree today: + // sql.NullString{} by admin-created apps, Valid-with-empty-string by DCR + // registration that sent no scope. + assert.True(t, noScopeAllowlist(sql.NullString{})) + assert.True(t, noScopeAllowlist(sql.NullString{String: "", Valid: true})) + assert.False(t, noScopeAllowlist(sql.NullString{String: "coder:workspaces.access", Valid: true})) + assert.False(t, noScopeAllowlist(sql.NullString{String: " ", Valid: true})) +} + func TestHashOAuth2State(t *testing.T) { t.Parallel() diff --git a/coderd/oauth2provider/authorize_test.go b/coderd/oauth2provider/authorize_test.go index 61e037a8a4b..e6b3a353708 100644 --- a/coderd/oauth2provider/authorize_test.go +++ b/coderd/oauth2provider/authorize_test.go @@ -1,15 +1,29 @@ package oauth2provider_test import ( + "context" + "database/sql" + "encoding/json" htmltemplate "html/template" + "io" "net/http" "net/http/httptest" + "net/url" "testing" + "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/coder/coder/v2/coderd/coderdtest" + "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/oauth2provider/oauth2providertest" + "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/site" + "github.com/coder/coder/v2/testutil" ) func TestOAuthConsentFormIncludesCSRFToken(t *testing.T) { @@ -34,3 +48,288 @@ func TestOAuthConsentFormIncludesCSRFToken(t *testing.T) { assert.Contains(t, body, `id="allow-form"`) assert.Contains(t, body, `id="cancel-link"`) } + +// Scope names used by the negotiation tests. Whether a name is in +// rbac.IsExternalScope's curated catalog is the point of each case, so the two +// groups are named rather than inlined. +const ( + scopeInCatalog = "coder:workspaces.access" + scopeAlsoInCatalog = "coder:templates.build" + scopeOutOfCatalog = "some_removed_scope" +) + +func TestOAuth2AuthorizeScopeNegotiation(t *testing.T) { + t.Parallel() + + db, pubsub := dbtestutil.NewDB(t) + client := coderdtest.New(t, &coderdtest.Options{ + Database: db, + Pubsub: pubsub, + }) + _ = coderdtest.CreateFirstUser(t, client) + + // Each sub-test gets its own app: only one code exists per app/user pair at + // a time, and the allowlist is the variable under test. + seedApp := func(t *testing.T, appScope sql.NullString) database.OAuth2ProviderApp { + t.Helper() + return dbgen.OAuth2ProviderApp(t, db, database.OAuth2ProviderApp{ + Name: testutil.GetRandomName(t), + CallbackURL: "https://example.com/callback", + Scope: appScope, + }) + } + + // AC1: a scope outside the app's allowlist is rejected and no code is + // issued. + t.Run("OutOfAllowlistRejected", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + app := seedApp(t, sql.NullString{String: scopeInCatalog, Valid: true}) + resp := authorizeRequest(ctx, t, client, http.MethodPost, app.ID.String(), scopeInCatalog+" template:read") + defer resp.Body.Close() + + requireInvalidScope(t, resp) + }) + + // AC1's catalog half: a scope name the enforcement layer cannot evaluate is + // rejected on its own terms, not because of the allowlist. + t.Run("UnknownScopeRejected", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + app := seedApp(t, sql.NullString{String: scopeInCatalog, Valid: true}) + resp := authorizeRequest(ctx, t, client, http.MethodPost, app.ID.String(), "not_a_real_scope") + defer resp.Body.Close() + + requireInvalidScope(t, resp) + }) + + // AC2: omitting scope grants the app's full allowlist (RFC 6749 §3.3). + t.Run("OmittedScopeDefaultsToAllowlist", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + allowlist := scopeInCatalog + " " + scopeAlsoInCatalog + app := seedApp(t, sql.NullString{String: allowlist, Valid: true}) + resp := authorizeRequest(ctx, t, client, http.MethodPost, app.ID.String(), "") + defer resp.Body.Close() + + require.Equal(t, allowlist, persistedCodeScope(ctx, t, db, resp)) + }) + + t.Run("RequestedSubsetGranted", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + app := seedApp(t, sql.NullString{String: scopeInCatalog + " " + scopeAlsoInCatalog, Valid: true}) + resp := authorizeRequest(ctx, t, client, http.MethodPost, app.ID.String(), scopeAlsoInCatalog) + defer resp.Body.Close() + + require.Equal(t, scopeAlsoInCatalog, persistedCodeScope(ctx, t, db, resp)) + }) + + // AC3: apps with no configured allowlist keep today's unrestricted + // behavior. The persisted value is asserted literally, since '' is what the + // column's CHECK would reject. + t.Run("NoAllowlistStaysUnrestricted", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + app := seedApp(t, sql.NullString{}) + resp := authorizeRequest(ctx, t, client, http.MethodPost, app.ID.String(), "") + defer resp.Body.Close() + + require.Equal(t, database.OAuth2ScopeUnrestricted, persistedCodeScope(ctx, t, db, resp)) + }) + + // AC16: NULL (admin-created apps) and '' (DCR apps that sent no scope) are + // one "no allowlist configured" state and must behave identically. + t.Run("NullAndEmptyAllowlistBehaveIdentically", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + nullApp := seedApp(t, sql.NullString{}) + emptyApp := seedApp(t, sql.NullString{String: "", Valid: true}) + + nullResp := authorizeRequest(ctx, t, client, http.MethodPost, nullApp.ID.String(), "") + defer nullResp.Body.Close() + emptyResp := authorizeRequest(ctx, t, client, http.MethodPost, emptyApp.ID.String(), "") + defer emptyResp.Body.Close() + + nullScope := persistedCodeScope(ctx, t, db, nullResp) + emptyScope := persistedCodeScope(ctx, t, db, emptyResp) + require.Equal(t, database.OAuth2ScopeUnrestricted, nullScope) + require.Equal(t, nullScope, emptyScope) + }) + + // Edge Case 20: an allowlist entry no longer in the catalog is dropped, not + // granted. Paired with AllowlistFilteringToEmptyRejected below, which is the + // same filter with no survivors. + t.Run("StaleAllowlistEntryDropped", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + app := seedApp(t, sql.NullString{String: scopeInCatalog + " " + scopeOutOfCatalog, Valid: true}) + resp := authorizeRequest(ctx, t, client, http.MethodPost, app.ID.String(), "") + defer resp.Body.Close() + + require.Equal(t, scopeInCatalog, persistedCodeScope(ctx, t, db, resp)) + }) + + // AC15: an allowlist whose every entry is dropped rejects rather than + // falling back to unrestricted, which would grant strictly more than the + // allowlist ever permitted. + t.Run("AllowlistFilteringToEmptyRejected", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + app := seedApp(t, sql.NullString{String: "openid profile email", Valid: true}) + resp := authorizeRequest(ctx, t, client, http.MethodPost, app.ID.String(), "") + defer resp.Body.Close() + + requireInvalidScope(t, resp) + }) + + // AC8: the GET handler rejects before the consent page renders, so the user + // is never asked to approve a request that cannot succeed. + t.Run("ConsentPageNotRenderedForInvalidScope", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + app := seedApp(t, sql.NullString{String: scopeInCatalog, Valid: true}) + + resp := authorizeRequest(ctx, t, client, http.MethodGet, app.ID.String(), scopeInCatalog+" template:read") + defer resp.Body.Close() + require.Equal(t, http.StatusBadRequest, resp.StatusCode) + require.NotContains(t, readBody(t, resp), `id="allow-form"`, + "the consent page must not render for a scope the app cannot be granted") + + // Positive control: the same handler still renders the consent page for + // a request the app can be granted, so the assertion above is about the + // scope and not about the request shape. + okResp := authorizeRequest(ctx, t, client, http.MethodGet, app.ID.String(), scopeInCatalog) + defer okResp.Body.Close() + require.Equal(t, http.StatusOK, okResp.StatusCode) + require.Contains(t, readBody(t, okResp), `id="allow-form"`) + }) +} + +// TestOAuth2AuthorizeDCRScopeCompatibility pins the compatibility break +// §4.2.2 accepts: dynamic client registration performs no catalog validation, +// so an app can register an allowlist this server cannot grant from. Both +// directions fail, and both fail loudly with invalid_scope rather than +// silently granting a scope dbauthz has no way to evaluate. +func TestOAuth2AuthorizeDCRScopeCompatibility(t *testing.T) { + t.Parallel() + + client := coderdtest.New(t, nil) + _ = coderdtest.CreateFirstUser(t, client) + oauth2providertest.EnableDCR(t, client) + + ctx := testutil.Context(t, testutil.WaitLong) + registration, err := client.PostOAuth2ClientRegistration(ctx, codersdk.OAuth2ClientRegistrationRequest{ + RedirectURIs: []string{"https://example.com/callback"}, + ClientName: testutil.GetRandomName(t), + Scope: "openid profile email", + }) + require.NoError(t, err, "registration itself is unchanged: no catalog check happens here") + + t.Run("RequestingRegisteredScopeRejected", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + resp := authorizeRequest(ctx, t, client, http.MethodPost, registration.ClientID, "openid") + defer resp.Body.Close() + + requireInvalidScope(t, resp) + }) + + t.Run("OmittingScopeRejected", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) + + resp := authorizeRequest(ctx, t, client, http.MethodPost, registration.ClientID, "") + defer resp.Body.Close() + + requireInvalidScope(t, resp) + }) +} + +// authorizeRequest issues an /oauth2/authorize request for the given app. +// Redirects are not followed, so a successful POST surfaces as a 302 whose +// Location carries the code. +func authorizeRequest(ctx context.Context, t *testing.T, client *codersdk.Client, method, clientID, scope string) *http.Response { + t.Helper() + + authURL, err := url.Parse(client.URL.String() + "/oauth2/authorize") + require.NoError(t, err) + + _, challenge := oauth2providertest.GeneratePKCE(t) + query := url.Values{} + query.Set("client_id", clientID) + query.Set("response_type", "code") + query.Set("state", uuid.NewString()) + query.Set("code_challenge", challenge) + query.Set("code_challenge_method", "S256") + if scope != "" { + query.Set("scope", scope) + } + authURL.RawQuery = query.Encode() + + req, err := http.NewRequestWithContext(ctx, method, authURL.String(), nil) + require.NoError(t, err) + req.Header.Set(codersdk.SessionTokenHeader, client.SessionToken()) + + httpClient := &http.Client{ + CheckRedirect: func(_ *http.Request, _ []*http.Request) error { + return http.ErrUseLastResponse + }, + } + resp, err := httpClient.Do(req) + require.NoError(t, err) + return resp +} + +// persistedCodeScope follows a successful authorization to the code it issued +// and returns the scope recorded on that row, which is what the token exchange +// will later read. +func persistedCodeScope(ctx context.Context, t *testing.T, db database.Store, resp *http.Response) string { + t.Helper() + + require.Equal(t, http.StatusFound, resp.StatusCode) + location, err := url.Parse(resp.Header.Get("Location")) + require.NoError(t, err) + + formatted := location.Query().Get("code") + require.NotEmpty(t, formatted, "authorization did not issue a code") + + parsed, err := oauth2provider.ParseFormattedSecret(formatted) + require.NoError(t, err) + + code, err := db.GetOAuth2ProviderAppCodeByPrefix(ctx, []byte(parsed.Prefix)) + require.NoError(t, err) + return code.Scope +} + +// requireInvalidScope asserts the RFC 6749 §4.1.2.1 rejection, and that the +// request produced no authorization code at all. +func requireInvalidScope(t *testing.T, resp *http.Response) { + t.Helper() + + require.Equal(t, http.StatusBadRequest, resp.StatusCode) + require.Empty(t, resp.Header.Get("Location"), "a rejected request must not redirect with a code") + + var errResp oauth2providertest.OAuth2Error + require.NoError(t, json.NewDecoder(resp.Body).Decode(&errResp)) + require.Equal(t, string(codersdk.OAuth2ErrorCodeInvalidScope), errResp.Error) + require.NotEmpty(t, errResp.ErrorDescription) +} + +func readBody(t *testing.T, resp *http.Response) string { + t.Helper() + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + return string(body) +} diff --git a/coderd/oauth2provider/validation_test.go b/coderd/oauth2provider/validation_test.go index 2bb442ab3c1..d7164eadec6 100644 --- a/coderd/oauth2provider/validation_test.go +++ b/coderd/oauth2provider/validation_test.go @@ -541,7 +541,18 @@ func TestOAuth2ClientNameValidation(t *testing.T) { } } -// TestOAuth2ClientScopeValidation tests scope parameter validation +// TestOAuth2ClientScopeValidation tests scope parameter validation at +// registration time, which accepts any syntactically valid scope string. +// +// Registration performs no scope catalog validation, so the values below are +// stored verbatim as the app's scope allowlist. Authorization is where the +// catalog is enforced: a name outside rbac.IsExternalScope cannot be granted, +// so an app registered with only such names can no longer complete an +// authorization in either direction. Requesting one is rejected with +// invalid_scope, and omitting scope entirely is rejected too, because the +// allowlist filters to nothing. TestOAuth2AuthorizeDCRScopeCompatibility +// covers both. Every non-empty scope below is in that position: none of read, +// write, openid, profile, email, admin, or custom:scope is in the catalog. func TestOAuth2ClientScopeValidation(t *testing.T) { t.Parallel() @@ -596,9 +607,11 @@ func TestOAuth2ClientScopeValidation(t *testing.T) { expectError: false, }, { - name: "InvalidAdmin", - scope: "admin", - expectError: false, // Admin scope should be allowed but validated during authorization + name: "InvalidAdmin", + scope: "admin", + // Registration accepts it; authorization rejects it with + // invalid_scope, since "admin" is not a grantable scope name. + expectError: false, }, { name: "ValidCustom",