mirror of
https://github.com/coder/coder.git
synced 2026-10-16 14:48:56 +08:00
feat: negotiate and persist authorization scope (#28178)
**TL;DR** Middle of the stack, split out of #28045, that makes an OAuth2 app's scope allowlist actually restrict what a client can be granted. The authorize endpoint has always parsed `scope` and then thrown it away. | PR | What it does | |---|---| | #28007 | Schema: `codes.scope` and `tokens.scope`, so a negotiated scope has somewhere to live. Every code is still stamped unrestricted. | | #28167 | `ScopesCover` compares an allowlist against a request by permission coverage rather than by name, and fails closed on any shape it does not model. Added with no production callers. | | **#28178 (this)** | The authorize endpoint checks the request against the scope catalog, grants it only if the app's filtered allowlist covers it, and persists the result on the code. Both GET and POST negotiate, so a doomed request fails before the consent page renders. | | #28179 | The consent page states what was negotiated, and `invalid_scope` is delivered to the client's own callback per RFC 6749 §4.1.2.1 instead of a Coder error page. | - Each earlier PR is inert alone: the columns carry a placeholder, the coverage rules have no caller. - Visible on upgrade, unlike the rest of the stack. A scope name outside the catalog, OIDC vocabulary included, now gets `invalid_scope`, on admin-created apps as well as DCR ones. - Tokens are unaffected. This changes which authorization requests succeed, not what a token can do. **Where in the flow** - Authorize endpoint only, both legs: GET before the consent page renders, POST before the code is minted. The token exchange, refresh, and revocation are untouched. - The result is persisted on the authorization code. The exchange copies it onto the token row, but the API key it mints still carries no scope, so nothing is enforced at request time yet. - Registration and discovery are unchanged. The catalog checked against is the `scopes_supported` both discovery documents already advertise. **What it satisfies** - OAuth 2.1 §1.4.1 — the server "MAY fully or partially ignore the scope requested by the client, based on the authorization server policy". The app's allowlist is that policy; until now the server ignored scope in every direction. - OAuth 2.1 §1.4.1 — on an omitted scope it "MUST either process the request using a pre-defined default value or fail the request". The filtered allowlist is that default; an allowlist filtering to nothing takes the second branch. Same rule as RFC 6749 §3.3. - OAuth 2.1 §1.4.1 — scope values "are defined by the authorization server", and OIDC is named there as an extension that defines its own. Hence `invalid_scope` for openid/profile/email rather than silent acceptance. - OAuth 2.1 §4.1.2.1 — `invalid_scope` is "invalid, unknown, or malformed", which now covers an uncatalogued name and an over-broad one alike. Delivery is interim here (static page on GET, error body on POST); §4.1.2.1 wants it redirected to the client's callback, which is #28179. - OAuth 2.1 §4.1.1 — `scope` stays OPTIONAL. Omitting it selects a default, never an error. - RFC 7591 §2 — a registered `scope` is the list "the client can use when requesting access tokens", a ceiling rather than a menu. Enforced here for the first time; a client that registered an unsupportable one repairs it via RFC 7592. - RFC 8414 §2 — `scopes_supported` is that catalog, advertised since before this stack, so a conformant client had no grounds to send a name outside it. - Not yet: §1.4.1 also requires `scope` in the token response when the grant differs from the request. Tokens are untouched here, so that lands with enforcement. --- Split out of #28045 ([PLAT-479](https://linear.app/codercom/issue/PLAT-479)). #28167 has merged, so this diff is against main and reviews on its own. The authorize endpoint parsed scope and threw it away, so an app's allowlist never restricted anything and a client asking for too much was never told no. Phase 1 added the columns that carry a negotiated scope from a code to the token, but nothing wrote one. **What changes** - Requested names must be in the external scope catalog. The app's stored allowlist is filtered through that same catalog, which only ever narrows. - The allowlist bounds authority, not spelling. A request is granted when every permission it grants is also granted by the allowlist, named or not, so an app allowed coder:workspaces.access can approve a client asking only for workspace:ssh. - Omitting scope grants the filtered allowlist (RFC 6749 §3.3). - Both handlers negotiate, so a request that cannot succeed fails before the consent page renders rather than after the user clicks Allow. - Two paths produce an empty result and stay distinct. No allowlist and no request keeps the previous unrestricted grant, written as an explicit sentinel because the column is NOT NULL with a non-empty CHECK. An allowlist filtering to nothing is rejected, since falling back would grant more than it ever permitted. **Accepted compatibility break, and it is wider than DCR.** The catalog check runs before the allowlist is consulted, so it covers admin-created apps too. - Admin-created apps store no allowlist and had scope discarded outright. A client sending OIDC vocabulary (openid, profile, email) now gets invalid_scope on upgrade. - DCR apps registered with openid, read or admin also hold allowlists this server cannot grant from, and fail in both directions. - scopes_supported has always been advertised on both discovery documents, predating this stack, so a conformant client had no grounds to send an uncatalogued name. RFC 6749 §4.1.2.1 makes invalid_scope the answer for one that does. - Grandfathering would seed the enforcement path with values it cannot evaluate: a visible error now, a silent hole later. - The rejection names the registered scopes and the remedy. Pinned in TestOAuth2AuthorizeDCRScopeCompatibility. **Interim error delivery, replaced in PR 3.** Rejections go out however each handler already reports errors: a static error page on GET, an OAuth2 error body on POST. #28179 delivers them to the client's own callback (RFC 6749 §4.1.2.1) and rewrites the requireInvalidScope helper. About 25 lines here are interim on purpose and flagged as such in that helper. **Not changed.** Issued tokens are still unrestricted. The exchange copies the negotiated scope onto the token record, but the API key it mints carries no scope. This changes which authorization requests succeed, not what a token can do. Stack: #28167 (merged), this PR, then #28179 for the consent page and the invalid_scope redirect. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Steven Masley <Emyrk@users.noreply.github.com>
This commit is contained in:
Bobby Ho
co-authored by
Claude Opus 5
Steven Masley
Claude Opus 5
Steven Masley
parent
3d3b36ff89
commit
57cbca585f
Generated
+2
-2
@@ -16525,7 +16525,7 @@ const docTemplate = `{
|
||||
},
|
||||
{
|
||||
"type": "string",
|
||||
"description": "Token scopes (currently ignored)",
|
||||
"description": "Space-separated scopes to request. Each must be supported by this deployment, and the app's allowlist, when it has one, must cover the permissions requested rather than name each scope. Defaults to that allowlist, or to coder:all for an app with no allowlist",
|
||||
"name": "scope",
|
||||
"in": "query"
|
||||
}
|
||||
@@ -16581,7 +16581,7 @@ const docTemplate = `{
|
||||
},
|
||||
{
|
||||
"type": "string",
|
||||
"description": "Token scopes (currently ignored)",
|
||||
"description": "Space-separated scopes to request. Each must be supported by this deployment, and the app's allowlist, when it has one, must cover the permissions requested rather than name each scope. Defaults to that allowlist, or to coder:all for an app with no allowlist",
|
||||
"name": "scope",
|
||||
"in": "query"
|
||||
}
|
||||
|
||||
Generated
+2
-2
@@ -14688,7 +14688,7 @@
|
||||
},
|
||||
{
|
||||
"type": "string",
|
||||
"description": "Token scopes (currently ignored)",
|
||||
"description": "Space-separated scopes to request. Each must be supported by this deployment, and the app's allowlist, when it has one, must cover the permissions requested rather than name each scope. Defaults to that allowlist, or to coder:all for an app with no allowlist",
|
||||
"name": "scope",
|
||||
"in": "query"
|
||||
}
|
||||
@@ -14739,7 +14739,7 @@
|
||||
},
|
||||
{
|
||||
"type": "string",
|
||||
"description": "Token scopes (currently ignored)",
|
||||
"description": "Space-separated scopes to request. Each must be supported by this deployment, and the app's allowlist, when it has one, must cover the permissions requested rather than name each scope. Defaults to that allowlist, or to coder:all for an app with no allowlist",
|
||||
"name": "scope",
|
||||
"in": "query"
|
||||
}
|
||||
|
||||
+4
-4
@@ -120,11 +120,11 @@ func (api *API) deleteOAuth2ProviderAppSecret() http.HandlerFunc {
|
||||
// @Param state query string true "A random unguessable string"
|
||||
// @Param response_type query codersdk.OAuth2ProviderResponseType true "Response type"
|
||||
// @Param redirect_uri query string false "Redirect here after authorization"
|
||||
// @Param scope query string false "Token scopes (currently ignored)"
|
||||
// @Param scope query string false "Space-separated scopes to request. Each must be supported by this deployment, and the app's allowlist, when it has one, must cover the permissions requested rather than name each scope. Defaults to that allowlist, or to coder:all for an app with no allowlist"
|
||||
// @Success 200 "Returns HTML authorization page"
|
||||
// @Router /oauth2/authorize [get]
|
||||
func (api *API) getOAuth2ProviderAppAuthorize() http.HandlerFunc {
|
||||
return oauth2provider.ShowAuthorizePage(api.AccessURL)
|
||||
return oauth2provider.ShowAuthorizePage(api.AccessURL, api.Logger)
|
||||
}
|
||||
|
||||
// @Summary OAuth2 authorization request (POST - process authorization).
|
||||
@@ -135,11 +135,11 @@ func (api *API) getOAuth2ProviderAppAuthorize() http.HandlerFunc {
|
||||
// @Param state query string true "A random unguessable string"
|
||||
// @Param response_type query codersdk.OAuth2ProviderResponseType true "Response type"
|
||||
// @Param redirect_uri query string false "Redirect here after authorization"
|
||||
// @Param scope query string false "Token scopes (currently ignored)"
|
||||
// @Param scope query string false "Space-separated scopes to request. Each must be supported by this deployment, and the app's allowlist, when it has one, must cover the permissions requested rather than name each scope. Defaults to that allowlist, or to coder:all for an app with no allowlist"
|
||||
// @Success 302 "Returns redirect with authorization code"
|
||||
// @Router /oauth2/authorize [post]
|
||||
func (api *API) postOAuth2ProviderAppAuthorize() http.HandlerFunc {
|
||||
return oauth2provider.ProcessAuthorize(api.Database)
|
||||
return oauth2provider.ProcessAuthorize(api.Database, api.Logger)
|
||||
}
|
||||
|
||||
// @Summary OAuth2 token exchange.
|
||||
|
||||
@@ -541,7 +541,9 @@ func TestOAuth2ClientNameValidation(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestOAuth2ClientScopeValidation tests scope parameter validation
|
||||
// Registration stores the scope verbatim, so every value below is accepted.
|
||||
// The catalog is enforced at authorization: see
|
||||
// TestOAuth2AuthorizeDCRScopeCompatibility in coderd/oauth2provider.
|
||||
func TestOAuth2ClientScopeValidation(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
@@ -598,7 +600,7 @@ func TestOAuth2ClientScopeValidation(t *testing.T) {
|
||||
{
|
||||
name: "InvalidAdmin",
|
||||
scope: "admin",
|
||||
expectError: false, // Admin scope should be allowed but validated during authorization
|
||||
expectError: false, // Rejected at authorization, not registration.
|
||||
},
|
||||
{
|
||||
name: "ValidCustom",
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
package oauth2provider
|
||||
|
||||
import (
|
||||
"context"
|
||||
"crypto/sha256"
|
||||
"database/sql"
|
||||
"encoding/hex"
|
||||
@@ -15,14 +16,168 @@ import (
|
||||
"github.com/justinas/nosurf"
|
||||
"golang.org/x/xerrors"
|
||||
|
||||
"cdr.dev/slog/v3"
|
||||
"github.com/coder/coder/v2/coderd/database"
|
||||
"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/coderd/util/slice"
|
||||
"github.com/coder/coder/v2/codersdk"
|
||||
"github.com/coder/coder/v2/site"
|
||||
)
|
||||
|
||||
// Rejection reasons from negotiateScope.
|
||||
var (
|
||||
// errUnknownScope covers a requested name outside the external scope
|
||||
// catalog, whether unrecognized entirely or recognized but internal-only.
|
||||
errUnknownScope = xerrors.New("unknown or unsupported scope")
|
||||
// errNoGrantableScope covers an allowlist whose every entry falls outside
|
||||
// the catalog.
|
||||
errNoGrantableScope = xerrors.New("none of the scopes registered for this app are supported by this deployment; change the app's registered scopes to supported ones")
|
||||
// errScopeNotAllowed checks whether the scope's expanded permissions are
|
||||
// covered by the allowlist. For example, "coder:workspaces.create" expands
|
||||
// to several workspace permissions.
|
||||
errScopeNotAllowed = xerrors.New("scope requests permissions beyond this app's allowed scopes")
|
||||
// errCoverageUndecidable covers a comparison that failed outright. The
|
||||
// underlying error names RBAC internals, so it is logged rather than
|
||||
// rendered into error_description.
|
||||
errCoverageUndecidable = xerrors.New("scope coverage against this app's allowed scopes could not be determined")
|
||||
)
|
||||
|
||||
// canonicalScopes rewrites each name to the spelling the api_key_scope enum
|
||||
// stores and drops repeats, preserving the order of first appearance. It
|
||||
// neither validates nor filters: callers check rbac.IsExternalScope separately.
|
||||
//
|
||||
// Canonicalization matters because rbac.IsExternalScope accepts the aliases
|
||||
// `all` and `application_connect`, which are not enum members, so persisting a
|
||||
// validated name verbatim can write a value the column's vocabulary does not
|
||||
// contain.
|
||||
func canonicalScopes(names []string) []string {
|
||||
canonical := make([]string, 0, len(names))
|
||||
for _, name := range names {
|
||||
canonical = append(canonical, string(rbac.CanonicalScopeName(rbac.ScopeName(name))))
|
||||
}
|
||||
return slice.Unique(canonical)
|
||||
}
|
||||
|
||||
// noScopeAllowlist reports whether an app has no scope allowlist configured.
|
||||
// NULL and "" are one state: admin-created apps store sql.NullString{}
|
||||
// (apps.go), while DCR-registered apps store Valid: true carrying a
|
||||
// possibly-empty req.Scope (registration.go).
|
||||
//
|
||||
// A whitespace-only allowlist is deliberately not this state. It is a
|
||||
// configured value that grants nothing, so it falls through to
|
||||
// negotiateScope's filtered-to-empty rejection instead of the unrestricted
|
||||
// fallback.
|
||||
func noScopeAllowlist(appScope sql.NullString) bool {
|
||||
return !appScope.Valid || appScope.String == ""
|
||||
}
|
||||
|
||||
// negotiateScope decides the scope the authorization code will carry. Every
|
||||
// requested name must be in the external scope catalog, and the request must
|
||||
// be covered by the app's configured allowlist. A rejection is an RFC 6749
|
||||
// §4.1.2.1 invalid_scope.
|
||||
//
|
||||
// What each branch returns:
|
||||
//
|
||||
// allowlist request result
|
||||
// absent absent ApiKeyScopeCoderAll, the pre-enforcement grant
|
||||
// absent present the request, which is narrower than unrestricted
|
||||
// present absent the allowlist, catalog-filtered (RFC 6749 §3.3 default)
|
||||
// present present the request, once shown to be within the allowlist
|
||||
//
|
||||
// An allowlist whose every entry falls outside the catalog is rejected rather
|
||||
// than read as absent, since falling back there would grant strictly more than
|
||||
// the allowlist ever permitted.
|
||||
//
|
||||
// The result is written directly to a NOT NULL column whose CHECK also rejects
|
||||
// the empty string, so it is never empty alongside a nil error, and its names
|
||||
// are canonical api_key_scope spellings carrying no duplicates.
|
||||
func negotiateScope(ctx context.Context, logger slog.Logger, app database.OAuth2ProviderApp, requested []string) (string, error) {
|
||||
// Canonicalized before the catalog check so that check, the coverage
|
||||
// comparison, and the persisted value all read one vocabulary. Rewriting
|
||||
// ahead of validation loses nothing: CanonicalScopeName only touches the
|
||||
// `all` and `application_connect` aliases, and the catalog holds both
|
||||
// spellings of each.
|
||||
granted := canonicalScopes(requested)
|
||||
|
||||
// The catalog is a curation, not a validity check: RBAC can expand
|
||||
// internal-only names such as debug_info:read, and the api_key_scope enum
|
||||
// would store them. Only catalog names are client-requestable, whether or
|
||||
// not the app has an allowlist to check them against.
|
||||
for _, s := range granted {
|
||||
if !rbac.IsExternalScope(rbac.ScopeName(s)) {
|
||||
return "", xerrors.Errorf("%q: %w", s, errUnknownScope)
|
||||
}
|
||||
}
|
||||
|
||||
if noScopeAllowlist(app.Scope) {
|
||||
if len(granted) == 0 {
|
||||
// Unrestricted, the same grant this app got before scope
|
||||
// enforcement existed, stated explicitly because an empty string
|
||||
// would violate the column's CHECK.
|
||||
return string(database.ApiKeyScopeCoderAll), nil
|
||||
}
|
||||
return strings.Join(granted, " "), nil
|
||||
}
|
||||
|
||||
// The allowlist was stored at registration time and may name a scope since
|
||||
// removed from the catalog, or never in it. Filtering only ever narrows
|
||||
// what is granted.
|
||||
//
|
||||
// Canonicalized in the same pass so both sides expand: rbac.ExpandScope
|
||||
// knows `coder:all` and not the `all` alias that IsExternalScope accepts.
|
||||
allowed := strings.Fields(app.Scope.String)
|
||||
filtered := make([]rbac.ScopeName, 0, len(allowed))
|
||||
for _, a := range allowed {
|
||||
if name := rbac.ScopeName(a); rbac.IsExternalScope(name) {
|
||||
filtered = append(filtered, rbac.CanonicalScopeName(name))
|
||||
}
|
||||
}
|
||||
filtered = slice.Unique(filtered)
|
||||
if len(filtered) == 0 {
|
||||
// Falling through to the no-allowlist branch would grant strictly more
|
||||
// than this allowlist ever permitted.
|
||||
//
|
||||
// The message names the stored value verbatim rather than rejoining
|
||||
// the filter's input, which would render a whitespace-only allowlist
|
||||
// as "" for the one configuration that most needs naming.
|
||||
return "", xerrors.Errorf("%q: %w", app.Scope.String, errNoGrantableScope)
|
||||
}
|
||||
|
||||
if len(granted) == 0 {
|
||||
names := make([]string, 0, len(filtered))
|
||||
for _, name := range filtered {
|
||||
names = append(names, string(name))
|
||||
}
|
||||
return strings.Join(names, " "), nil // RFC 6749 §3.3 default
|
||||
}
|
||||
|
||||
// The allowlist is a ceiling on authority, not a menu of spellings, so the
|
||||
// check is permission coverage rather than name membership: an app allowed
|
||||
// `coder:workspaces.access` can approve a client asking only for
|
||||
// `workspace:read`, which that composite already grants.
|
||||
for _, s := range granted {
|
||||
covered, err := rbac.ScopesCover(filtered, rbac.ScopeName(s))
|
||||
if err != nil {
|
||||
// Refuse rather than grant on an incomplete comparison. The
|
||||
// underlying error names RBAC internals the client can do nothing
|
||||
// with, so it goes to the log alongside the app that provoked it.
|
||||
logger.Warn(ctx, "oauth2 scope coverage could not be determined",
|
||||
slog.Error(err),
|
||||
slog.F("app_id", app.ID.String()),
|
||||
slog.F("app_scope", app.Scope.String),
|
||||
slog.F("requested_scope", s))
|
||||
return "", xerrors.Errorf("%q: %w", s, errCoverageUndecidable)
|
||||
}
|
||||
if !covered {
|
||||
return "", xerrors.Errorf("%q: %w", s, errScopeNotAllowed)
|
||||
}
|
||||
}
|
||||
return strings.Join(granted, " "), nil
|
||||
}
|
||||
|
||||
type authorizeParams struct {
|
||||
clientID string
|
||||
redirectURL *url.URL
|
||||
@@ -96,7 +251,7 @@ func extractAuthorizeParams(r *http.Request, callbackURL *url.URL) (authorizePar
|
||||
}
|
||||
|
||||
// ShowAuthorizePage handles GET /oauth2/authorize requests to display the HTML authorization page.
|
||||
func ShowAuthorizePage(accessURL *url.URL) http.HandlerFunc {
|
||||
func ShowAuthorizePage(accessURL *url.URL, logger slog.Logger) http.HandlerFunc {
|
||||
return func(rw http.ResponseWriter, r *http.Request) {
|
||||
app := httpmw.OAuth2ProviderApp(r)
|
||||
ua := httpmw.UserAuthorization(r.Context())
|
||||
@@ -156,6 +311,26 @@ func ShowAuthorizePage(accessURL *url.URL) http.HandlerFunc {
|
||||
return
|
||||
}
|
||||
|
||||
// Negotiate here as well as on POST, so a request that cannot succeed
|
||||
// fails before the consent page renders rather than after the user
|
||||
// clicks Allow. The consent form posts back to this URL, so both
|
||||
// handlers see the same query string and reach the same decision.
|
||||
if _, err := negotiateScope(r.Context(), logger, app, params.scope); err != nil {
|
||||
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
|
||||
Status: http.StatusBadRequest,
|
||||
HideStatus: false,
|
||||
Title: "Invalid Scope",
|
||||
Description: 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")
|
||||
@@ -197,7 +372,7 @@ func ShowAuthorizePage(accessURL *url.URL) http.HandlerFunc {
|
||||
|
||||
// ProcessAuthorize handles POST /oauth2/authorize requests to process the user's authorization decision
|
||||
// and generate an authorization code.
|
||||
func ProcessAuthorize(db database.Store) http.HandlerFunc {
|
||||
func ProcessAuthorize(db database.Store, logger slog.Logger) http.HandlerFunc {
|
||||
return func(rw http.ResponseWriter, r *http.Request) {
|
||||
ctx := r.Context()
|
||||
apiKey := httpmw.APIKey(r)
|
||||
@@ -234,7 +409,13 @@ func ProcessAuthorize(db database.Store) http.HandlerFunc {
|
||||
return
|
||||
}
|
||||
|
||||
// TODO: Ignoring scope for now, but should look into implementing.
|
||||
grantedScope, err := negotiateScope(ctx, logger, app, params.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")
|
||||
@@ -271,11 +452,11 @@ 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: string(database.ApiKeyScopeCoderAll),
|
||||
// The negotiated scope, not the requested one. The exchange
|
||||
// copies it onto the token row but does not yet put it on the
|
||||
// API key it mints, so this records what was agreed, not yet
|
||||
// what is enforced.
|
||||
Scope: grantedScope,
|
||||
})
|
||||
if err != nil {
|
||||
return xerrors.Errorf("insert oauth2 authorization code: %w", err)
|
||||
|
||||
@@ -2,13 +2,269 @@ package oauth2provider
|
||||
|
||||
import (
|
||||
"crypto/sha256"
|
||||
"database/sql"
|
||||
"encoding/hex"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/google/uuid"
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
|
||||
"cdr.dev/slog/v3/sloggers/slogtest"
|
||||
"github.com/coder/coder/v2/coderd/database"
|
||||
"github.com/coder/coder/v2/coderd/rbac"
|
||||
)
|
||||
|
||||
func TestNegotiateScope(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
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 error
|
||||
wantErrText string
|
||||
}{
|
||||
{
|
||||
name: "UnknownRequestedScopeRejected",
|
||||
requested: []string{"not_a_real_scope"},
|
||||
appScope: sql.NullString{String: inCatalog, Valid: true},
|
||||
wantErr: errUnknownScope,
|
||||
},
|
||||
{
|
||||
name: "UnknownRequestedScopeRejectedWithoutAllowlist",
|
||||
requested: []string{"not_a_real_scope"},
|
||||
appScope: noAllowlist,
|
||||
wantErr: errUnknownScope,
|
||||
},
|
||||
{
|
||||
// RBAC expands debug_info:read; only the catalog keeps it internal.
|
||||
name: "InternalOnlyScopeRejected",
|
||||
requested: []string{"debug_info:read"},
|
||||
appScope: noAllowlist,
|
||||
wantErr: errUnknownScope,
|
||||
},
|
||||
{
|
||||
name: "NoAllowlistOmittedRequestIsUnrestricted",
|
||||
requested: nil,
|
||||
appScope: noAllowlist,
|
||||
want: string(database.ApiKeyScopeCoderAll),
|
||||
},
|
||||
{
|
||||
name: "EmptyAllowlistBehavesAsNoAllowlist",
|
||||
requested: nil,
|
||||
appScope: emptyAllowlist,
|
||||
want: string(database.ApiKeyScopeCoderAll),
|
||||
},
|
||||
{
|
||||
name: "NoAllowlistExplicitRequestPassesThrough",
|
||||
requested: []string{inCatalog},
|
||||
appScope: noAllowlist,
|
||||
want: inCatalog,
|
||||
},
|
||||
{
|
||||
name: "EmptyAllowlistExplicitRequestPassesThrough",
|
||||
requested: []string{inCatalog},
|
||||
appScope: emptyAllowlist,
|
||||
want: inCatalog,
|
||||
},
|
||||
{
|
||||
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:update"},
|
||||
appScope: sql.NullString{String: inCatalog, Valid: true},
|
||||
wantErr: errScopeNotAllowed,
|
||||
},
|
||||
{
|
||||
name: "LowLevelScopeCoveredByCompositeAllowlistAccepted",
|
||||
requested: []string{"workspace:ssh"},
|
||||
appScope: sql.NullString{String: inCatalog, Valid: true},
|
||||
want: "workspace:ssh",
|
||||
},
|
||||
{
|
||||
name: "PartiallyCoveredRequestRejectedWhole",
|
||||
requested: []string{"workspace:ssh", "workspace:delete"},
|
||||
appScope: sql.NullString{String: inCatalog, Valid: true},
|
||||
wantErr: errScopeNotAllowed,
|
||||
},
|
||||
{
|
||||
name: "WildcardActionNotCoveredByCompositeAllowlist",
|
||||
requested: []string{"workspace:*"},
|
||||
appScope: sql.NullString{String: inCatalog, Valid: true},
|
||||
wantErr: errScopeNotAllowed,
|
||||
},
|
||||
{
|
||||
name: "AllAllowlistCoversAnyScope",
|
||||
requested: []string{"user_secret:delete"},
|
||||
appScope: sql.NullString{String: string(database.ApiKeyScopeCoderAll), Valid: true},
|
||||
want: "user_secret:delete",
|
||||
},
|
||||
{
|
||||
name: "CoverageSpansMultipleAllowlistEntries",
|
||||
requested: []string{"file:create", "workspace:ssh"},
|
||||
appScope: sql.NullString{String: inCatalog + " " + alsoInCatalog, Valid: true},
|
||||
want: "file:create workspace:ssh",
|
||||
},
|
||||
{
|
||||
name: "StaleAllowlistEntryDroppedNotGranted",
|
||||
requested: nil,
|
||||
appScope: sql.NullString{String: inCatalog + " " + notInCatalog, Valid: true},
|
||||
want: inCatalog,
|
||||
},
|
||||
{
|
||||
name: "StaleAllowlistEntryNotRequestableExplicitly",
|
||||
requested: []string{notInCatalog},
|
||||
appScope: sql.NullString{String: inCatalog + " " + notInCatalog, Valid: true},
|
||||
wantErr: errUnknownScope,
|
||||
},
|
||||
{
|
||||
name: "AllowlistFilteringToEmptyRejected",
|
||||
requested: nil,
|
||||
appScope: sql.NullString{String: "openid profile email", Valid: true},
|
||||
wantErr: errNoGrantableScope,
|
||||
},
|
||||
{
|
||||
name: "RegisteredNonCatalogScopeRejected",
|
||||
requested: []string{neverInCatalog},
|
||||
appScope: sql.NullString{String: neverInCatalog, Valid: true},
|
||||
wantErr: errUnknownScope,
|
||||
},
|
||||
{
|
||||
name: "WhitespaceOnlyAllowlistRejected",
|
||||
requested: nil,
|
||||
appScope: sql.NullString{String: " ", Valid: true},
|
||||
wantErr: errNoGrantableScope,
|
||||
wantErrText: `" "`,
|
||||
},
|
||||
{
|
||||
name: "LegacyAllAliasCanonicalized",
|
||||
requested: []string{"all"},
|
||||
appScope: noAllowlist,
|
||||
want: "coder:all",
|
||||
},
|
||||
{
|
||||
name: "LegacyApplicationConnectAliasCanonicalized",
|
||||
requested: []string{"application_connect"},
|
||||
appScope: noAllowlist,
|
||||
want: "coder:application_connect",
|
||||
},
|
||||
{
|
||||
name: "LegacyAliasInAllowlistCoversCanonicalRequest",
|
||||
requested: []string{"coder:all"},
|
||||
appScope: sql.NullString{String: "all", Valid: true},
|
||||
want: "coder:all",
|
||||
},
|
||||
{
|
||||
name: "CanonicalAllowlistCoversLegacyAliasRequest",
|
||||
requested: []string{"all"},
|
||||
appScope: sql.NullString{String: "coder:all", Valid: true},
|
||||
want: "coder:all",
|
||||
},
|
||||
{
|
||||
name: "DuplicateRequestedScopesDeduplicated",
|
||||
requested: []string{inCatalog, inCatalog},
|
||||
appScope: noAllowlist,
|
||||
want: inCatalog,
|
||||
},
|
||||
{
|
||||
name: "DuplicateAllowlistEntriesDeduplicated",
|
||||
requested: nil,
|
||||
appScope: sql.NullString{String: inCatalog + " " + inCatalog, Valid: true},
|
||||
want: inCatalog,
|
||||
},
|
||||
{
|
||||
name: "AliasAndCanonicalAllowlistEntriesCollapse",
|
||||
requested: nil,
|
||||
appScope: sql.NullString{String: "all coder:all", Valid: true},
|
||||
want: "coder:all",
|
||||
},
|
||||
}
|
||||
|
||||
for _, test := range tests {
|
||||
t.Run(test.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
app := database.OAuth2ProviderApp{ID: uuid.New(), Scope: test.appScope}
|
||||
got, err := negotiateScope(t.Context(), slogtest.Make(t, nil), app, test.requested)
|
||||
if test.wantErr != nil {
|
||||
require.ErrorIs(t, err, test.wantErr)
|
||||
assert.Empty(t, got, "a rejected request must not return a persistable scope")
|
||||
assert.Equal(t, 1, strings.Count(err.Error(), test.wantErr.Error()),
|
||||
"the rejection reason must appear once, not doubled by the wrap")
|
||||
if test.wantErrText != "" {
|
||||
assert.Contains(t, err.Error(), test.wantErrText,
|
||||
"the rejection must name the value the app owner has to change")
|
||||
}
|
||||
return
|
||||
}
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, test.want, got)
|
||||
assert.NotEmpty(t, got, "the code scope column is NOT NULL with CHECK (scope <> '')")
|
||||
requirePersistableScope(t, got)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// requirePersistableScope asserts every name can be stored as an api_key_scope
|
||||
// and expanded by RBAC. Passing the external catalog implies neither.
|
||||
func requirePersistableScope(t *testing.T, scope string) {
|
||||
t.Helper()
|
||||
|
||||
for _, name := range strings.Fields(scope) {
|
||||
require.Contains(t, database.AllAPIKeyScopeValues(), database.APIKeyScope(name),
|
||||
"scope %q is not an api_key_scope member, so the column cannot store it", name)
|
||||
|
||||
_, err := rbac.ExpandScope(rbac.ScopeName(name))
|
||||
require.NoError(t, err, "scope %q cannot be expanded by RBAC, so it cannot be enforced", name)
|
||||
}
|
||||
}
|
||||
|
||||
// Rejection reasons for the package's black-box tests, which cannot reach the
|
||||
// sentinels.
|
||||
var (
|
||||
ReasonUnknownScope = errUnknownScope.Error()
|
||||
ReasonNoGrantableScope = errNoGrantableScope.Error()
|
||||
ReasonScopeNotAllowed = errScopeNotAllowed.Error()
|
||||
)
|
||||
|
||||
func TestNoScopeAllowlist(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
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()
|
||||
|
||||
|
||||
@@ -1,15 +1,28 @@
|
||||
package oauth2provider_test
|
||||
|
||||
import (
|
||||
"context"
|
||||
"database/sql"
|
||||
"html"
|
||||
htmltemplate "html/template"
|
||||
"io"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"net/url"
|
||||
"testing"
|
||||
|
||||
"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 +47,296 @@ func TestOAuthConsentFormIncludesCSRFToken(t *testing.T) {
|
||||
assert.Contains(t, body, `id="allow-form"`)
|
||||
assert.Contains(t, body, `id="cancel-link"`)
|
||||
}
|
||||
|
||||
const (
|
||||
scopeInCatalog = "coder:workspaces.access"
|
||||
scopeAlsoInCatalog = "coder:templates.build"
|
||||
scopeOutOfCatalog = "some_removed_scope"
|
||||
// In the catalog, but scopeInCatalog grants template:read, never
|
||||
// template:update.
|
||||
scopeOutOfAllowlist = "template:update"
|
||||
)
|
||||
|
||||
const (
|
||||
appCallbackURL = "https://example.com/callback"
|
||||
authorizeState = "test-authorize-state"
|
||||
)
|
||||
|
||||
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)
|
||||
|
||||
// Only one code exists per app/user pair, so each sub-test needs its own app.
|
||||
seedApp := func(t *testing.T, appScope sql.NullString) database.OAuth2ProviderApp {
|
||||
t.Helper()
|
||||
return dbgen.OAuth2ProviderApp(t, db, database.OAuth2ProviderApp{
|
||||
Name: testutil.GetRandomName(t),
|
||||
CallbackURL: appCallbackURL,
|
||||
Scope: appScope,
|
||||
})
|
||||
}
|
||||
|
||||
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+" "+scopeOutOfAllowlist)
|
||||
defer resp.Body.Close()
|
||||
|
||||
requireInvalidScope(t, resp, reasonScopeNotAllowed)
|
||||
})
|
||||
|
||||
t.Run("UnlistedScopeCoveredByAllowlistGranted", 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(), "workspace:ssh")
|
||||
defer resp.Body.Close()
|
||||
|
||||
require.Equal(t, "workspace:ssh", persistedCodeScope(ctx, t, db, resp))
|
||||
})
|
||||
|
||||
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, reasonUnknownScope)
|
||||
})
|
||||
|
||||
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("LegacyAliasPersistedCanonically", 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(), "all")
|
||||
defer resp.Body.Close()
|
||||
|
||||
require.Equal(t, string(database.ApiKeyScopeCoderAll), persistedCodeScope(ctx, t, db, resp))
|
||||
})
|
||||
|
||||
t.Run("DuplicateRequestedScopePersistedOnce", 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+" "+scopeInCatalog)
|
||||
defer resp.Body.Close()
|
||||
|
||||
require.Equal(t, scopeInCatalog, persistedCodeScope(ctx, t, db, resp))
|
||||
})
|
||||
|
||||
// NULL comes from admin-created apps, '' from DCR apps that sent no scope.
|
||||
t.Run("NullAndEmptyAllowlistGrantUnrestricted", 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, string(database.ApiKeyScopeCoderAll), nullScope)
|
||||
require.Equal(t, nullScope, emptyScope)
|
||||
})
|
||||
|
||||
t.Run("StaleAllowlistEntryDroppedNotGranted", 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))
|
||||
})
|
||||
|
||||
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, reasonNoGrantableScope)
|
||||
})
|
||||
}
|
||||
|
||||
// Registration performs no catalog validation, so an app can register an
|
||||
// allowlist this server cannot grant from. Authorization then rejects it rather
|
||||
// than granting a scope dbauthz cannot 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{appCallbackURL},
|
||||
ClientName: testutil.GetRandomName(t),
|
||||
Scope: "openid profile email",
|
||||
})
|
||||
require.NoError(t, err, "registration performs no catalog check")
|
||||
|
||||
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, reasonUnknownScope)
|
||||
})
|
||||
|
||||
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, reasonNoGrantableScope)
|
||||
})
|
||||
|
||||
t.Run("RejectionNamesTheRegisteredScopes", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
|
||||
resp := authorizeRequest(ctx, t, client, http.MethodGet, registration.ClientID, "")
|
||||
defer resp.Body.Close()
|
||||
body := requireInvalidScope(t, resp, reasonNoGrantableScope)
|
||||
|
||||
require.Contains(t, body, "openid profile email",
|
||||
"the rejection must name the registered scopes the owner has to change")
|
||||
})
|
||||
}
|
||||
|
||||
// authorizeQuery builds a well-formed /oauth2/authorize query. Callers varying
|
||||
// another parameter mutate the result before sending it.
|
||||
func authorizeQuery(t *testing.T, clientID, scope string) url.Values {
|
||||
t.Helper()
|
||||
|
||||
_, challenge := oauth2providertest.GeneratePKCE(t)
|
||||
query := url.Values{}
|
||||
query.Set("client_id", clientID)
|
||||
query.Set("response_type", "code")
|
||||
query.Set("state", authorizeState)
|
||||
query.Set("code_challenge", challenge)
|
||||
query.Set("code_challenge_method", "S256")
|
||||
if scope != "" {
|
||||
query.Set("scope", scope)
|
||||
}
|
||||
return query
|
||||
}
|
||||
|
||||
// authorizeRequest issues an /oauth2/authorize request without following
|
||||
// redirects, so a successful POST surfaces as a 302 carrying the code.
|
||||
func authorizeRequest(ctx context.Context, t *testing.T, client *codersdk.Client, method, clientID, scope string) *http.Response {
|
||||
t.Helper()
|
||||
|
||||
return sendAuthorizeRequest(ctx, t, client, method, authorizeQuery(t, clientID, scope))
|
||||
}
|
||||
|
||||
func sendAuthorizeRequest(ctx context.Context, t *testing.T, client *codersdk.Client, method string, query url.Values) *http.Response {
|
||||
t.Helper()
|
||||
|
||||
authURL, err := url.Parse(client.URL.String() + "/oauth2/authorize")
|
||||
require.NoError(t, err)
|
||||
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 returns the scope stored on the issued code, which is what
|
||||
// the token exchange later reads.
|
||||
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
|
||||
}
|
||||
|
||||
// The rejection reasons from authorize.go, each unique to one branch. The wire
|
||||
// carries only the rendered description, so these stand in for errors.Is.
|
||||
var (
|
||||
reasonUnknownScope = oauth2provider.ReasonUnknownScope
|
||||
reasonNoGrantableScope = oauth2provider.ReasonNoGrantableScope
|
||||
reasonScopeNotAllowed = oauth2provider.ReasonScopeNotAllowed
|
||||
)
|
||||
|
||||
// requireInvalidScope asserts the request was refused by the named branch
|
||||
// rather than issued a code. GET answers with an error page and POST with an
|
||||
// OAuth2 error body, so the returned body is unescaped for either.
|
||||
func requireInvalidScope(t *testing.T, resp *http.Response, wantReason string) string {
|
||||
t.Helper()
|
||||
|
||||
require.Equal(t, http.StatusBadRequest, resp.StatusCode)
|
||||
require.Empty(t, resp.Header.Get("Location"),
|
||||
"a rejected request must not be redirected, least of all with a code")
|
||||
|
||||
body := html.UnescapeString(readBody(t, resp))
|
||||
require.Contains(t, body, wantReason,
|
||||
"the rejection must come from the branch this case covers")
|
||||
return body
|
||||
}
|
||||
|
||||
func readBody(t *testing.T, resp *http.Response) string {
|
||||
t.Helper()
|
||||
|
||||
body, err := io.ReadAll(resp.Body)
|
||||
require.NoError(t, err)
|
||||
return string(body)
|
||||
}
|
||||
|
||||
@@ -541,7 +541,9 @@ func TestOAuth2ClientNameValidation(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestOAuth2ClientScopeValidation tests scope parameter validation
|
||||
// Registration stores the scope verbatim, so every value below is accepted.
|
||||
// The catalog is enforced at authorization: see
|
||||
// TestOAuth2AuthorizeDCRScopeCompatibility.
|
||||
func TestOAuth2ClientScopeValidation(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
@@ -598,7 +600,7 @@ func TestOAuth2ClientScopeValidation(t *testing.T) {
|
||||
{
|
||||
name: "InvalidAdmin",
|
||||
scope: "admin",
|
||||
expectError: false, // Admin scope should be allowed but validated during authorization
|
||||
expectError: false, // Rejected at authorization, not registration.
|
||||
},
|
||||
{
|
||||
name: "ValidCustom",
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
package rbac
|
||||
|
||||
import (
|
||||
"reflect"
|
||||
"testing"
|
||||
|
||||
"github.com/stretchr/testify/require"
|
||||
@@ -165,3 +166,56 @@ func TestScopesCoverGuards(t *testing.T) {
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestCoverageModelFields fails when one of the types coverage reads grows a
|
||||
// field, so someone decides whether checkCoverable has to account for it.
|
||||
func TestCoverageModelFields(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
typ reflect.Type
|
||||
fields []string
|
||||
}{
|
||||
{
|
||||
// permissionCovered compares ResourceType and Action; Negate is
|
||||
// why checkCoverable refuses a scope carrying one.
|
||||
name: "Permission",
|
||||
typ: reflect.TypeOf(Permission{}),
|
||||
fields: []string{"Negate", "ResourceType", "Action"},
|
||||
},
|
||||
{
|
||||
// Scope embeds Role, so Role is where Site, User, and ByOrgID
|
||||
// reach the guards.
|
||||
name: "Role",
|
||||
typ: reflect.TypeOf(Role{}),
|
||||
fields: []string{"Identifier", "DisplayName", "Site", "User", "ByOrgID", "cachedRegoValue"},
|
||||
},
|
||||
{
|
||||
name: "Scope",
|
||||
typ: reflect.TypeOf(Scope{}),
|
||||
fields: []string{"Role", "AllowIDList"},
|
||||
},
|
||||
{
|
||||
// allowListContainsAll reads both fields to decide whether the
|
||||
// allow list leaves the Site permissions unconditional.
|
||||
name: "AllowListElement",
|
||||
typ: reflect.TypeOf(AllowListElement{}),
|
||||
fields: []string{"ID", "Type"},
|
||||
},
|
||||
}
|
||||
|
||||
for _, test := range tests {
|
||||
t.Run(test.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
got := make([]string, 0, test.typ.NumField())
|
||||
for i := range test.typ.NumField() {
|
||||
got = append(got, test.typ.Field(i).Name)
|
||||
}
|
||||
|
||||
require.ElementsMatchf(t, test.fields, got,
|
||||
"%s changed shape: decide whether the new field carries authority coverage must model, extend checkCoverable if it does, then update this list", test.name)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
Generated
+14
-14
@@ -4885,13 +4885,13 @@ curl -X GET http://coder-server:8080/oauth2/authorize?client_id=string&state=str
|
||||
|
||||
### Parameters
|
||||
|
||||
| Name | In | Type | Required | Description |
|
||||
|-----------------|-------|--------|----------|-----------------------------------|
|
||||
| `client_id` | query | string | true | Client ID |
|
||||
| `state` | query | string | true | A random unguessable string |
|
||||
| `response_type` | query | string | true | Response type |
|
||||
| `redirect_uri` | query | string | false | Redirect here after authorization |
|
||||
| `scope` | query | string | false | Token scopes (currently ignored) |
|
||||
| Name | In | Type | Required | Description |
|
||||
|-----------------|-------|--------|----------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
|
||||
| `client_id` | query | string | true | Client ID |
|
||||
| `state` | query | string | true | A random unguessable string |
|
||||
| `response_type` | query | string | true | Response type |
|
||||
| `redirect_uri` | query | string | false | Redirect here after authorization |
|
||||
| `scope` | query | string | false | Space-separated scopes to request. Each must be supported by this deployment, and the app's allowlist, when it has one, must cover the permissions requested rather than name each scope. Defaults to that allowlist, or to coder:all for an app with no allowlist |
|
||||
|
||||
#### Enumerated Values
|
||||
|
||||
@@ -4921,13 +4921,13 @@ curl -X POST http://coder-server:8080/oauth2/authorize?client_id=string&state=st
|
||||
|
||||
### Parameters
|
||||
|
||||
| Name | In | Type | Required | Description |
|
||||
|-----------------|-------|--------|----------|-----------------------------------|
|
||||
| `client_id` | query | string | true | Client ID |
|
||||
| `state` | query | string | true | A random unguessable string |
|
||||
| `response_type` | query | string | true | Response type |
|
||||
| `redirect_uri` | query | string | false | Redirect here after authorization |
|
||||
| `scope` | query | string | false | Token scopes (currently ignored) |
|
||||
| Name | In | Type | Required | Description |
|
||||
|-----------------|-------|--------|----------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
|
||||
| `client_id` | query | string | true | Client ID |
|
||||
| `state` | query | string | true | A random unguessable string |
|
||||
| `response_type` | query | string | true | Response type |
|
||||
| `redirect_uri` | query | string | false | Redirect here after authorization |
|
||||
| `scope` | query | string | false | Space-separated scopes to request. Each must be supported by this deployment, and the app's allowlist, when it has one, must cover the permissions requested rather than name each scope. Defaults to that allowlist, or to coder:all for an app with no allowlist |
|
||||
|
||||
#### Enumerated Values
|
||||
|
||||
|
||||
Reference in New Issue
Block a user