diff --git a/coderd/oauth2_metadata_validation_test.go b/coderd/oauth2_metadata_validation_test.go index 01b2143f5a..3bce27a8af 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 7e03c1ab86..d25b7d878b 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 4f2d3fc993..2654af1fd8 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 61e037a8a4..e6b3a35370 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 2bb442ab3c..d7164eadec 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",