mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
OAuth2 tokens issued by Coder ignore scope entirely. The authorize endpoint parses the `scope` parameter and then discards it, and both grant paths mint API keys with full API access regardless of what the client requested or what the app's allowlist permits. There is also nowhere to put a negotiated scope: nothing carries one from the authorize step to the token it produces. Schema and query groundwork for that pipeline. No behavior change on its own. - Migration `000569` adds a `scope` column to `oauth2_provider_app_codes` and `oauth2_provider_app_tokens`, so a negotiated scope can travel from a code to the token it is exchanged for, and from a token to its refreshed successor. - Existing rows are backfilled to `coder:all`, then both columns become NOT NULL with a non-empty CHECK. Every OAuth2 key is unrestricted in fact today, so the backfill only writes that down, and a caller that omits the column now fails instead of silently issuing full access. - Adds `DeleteOAuth2ProviderAppCodeByIDReturningRow` and `DeleteAPIKeyByIDReturningRow`, which return `sql.ErrNoRows` when the row is already gone. Postgres serializes concurrent deletes on the row lock, so exactly one caller gets a row back, which is what will let the grant paths enforce single use of a code or refresh token without a read-then-write race. - No callers yet. The existing blind deletes and all of their call sites are untouched, and codes and tokens record `coder:all` until a later phase negotiates a real value. Phase 1 of [PLAT-470](https://linear.app/codercom/issue/PLAT-470), tracked as [PLAT-478](https://linear.app/codercom/issue/PLAT-478/phase-1-schema-and-queries). Scope validation at authorize, applying the negotiated scope in the code grant, and refresh narrowing follow as separate PRs. Verified locally: `make gen` and `make lint` clean, the migrations suite passes both up and down, and dbauthz's `TestMethodTestSuite` passes. <details> <summary>End-to-end scope enforcement flow (green marks what this PR touches)</summary> ```mermaid flowchart TD subgraph authorize["/oauth2/authorize"] AZ1["ShowAuthorizePage (GET)<br/>renders consent page"] AZ2["ProcessAuthorize (POST)<br/>scope parsed, then discarded"] Q1["InsertOAuth2ProviderAppCode<br/>gains a Scope param"] AZ1 --> AZ2 --> Q1 end Q1 --> CODES[("oauth2_provider_app_codes<br/>new column: scope text NOT NULL")] subgraph codegrant["POST /oauth2/token, grant_type=authorization_code"] G1["authorizationCodeGrant"] Q2["GetOAuth2ProviderAppCodeByPrefix<br/>now returns Scope"] Q4["DeleteOAuth2ProviderAppCodeByIDReturningRow<br/>added, no caller yet"] G2["apikey.Generate + UserRBACSubject<br/>hardcoded to full access"] G1 --> Q2 --> G2 G1 -.-> Q4 end CODES --> G1 G2 --> Q3 Q3["InsertOAuth2ProviderAppToken<br/>gains a Scope param"] Q3 --> TOKENS[("oauth2_provider_app_tokens<br/>new column: scope text NOT NULL")] subgraph refresh["POST /oauth2/token, grant_type=refresh_token"] G3["refreshTokenGrant"] Q5["GetOAuth2ProviderAppTokenByPrefix<br/>now returns Scope"] Q6["DeleteAPIKeyByIDReturningRow<br/>added, no caller yet"] G3 --> Q5 G3 -.-> Q6 end TOKENS --> G3 Q5 --> Q3 subgraph enforce["Every authenticated API request"] E1["httpmw ExtractAPIKey"] --> E2["APIKey.ScopeSet()"] --> E3["UserRBACSubject"] --> E4["dbauthz authorize"] end TOKENS --> E1 classDef changed fill:#c8e6c9,stroke:#2e7d32,stroke-width:2px,color:#1b3c1e classDef dormant fill:#c8e6c9,stroke:#2e7d32,stroke-width:2px,stroke-dasharray:5 3,color:#1b3c1e class Q1,Q2,Q3,Q5,CODES,TOKENS changed class Q4,Q6 dormant ``` Solid green is added or changed here. Dashed green exists but has no caller yet. Everything else is unchanged, including the enforcement engine at the bottom, which already reads a key's scopes correctly and only needs real data fed into it. </details> <details> <summary>Suggested reading order</summary> Most of the diff is generated. `dump.sql`, `models.go`, `querier.go`, `queries.sql.go`, `check_constraint.go`, and the dbmock and dbmetrics packages all come from `make gen`. 1. `migrations/000569_oauth2_scope_columns.{up,down}.sql`: additive column, backfill, NOT NULL, CHECK, and a `COMMENT ON COLUMN` on each. 2. `queries/oauth2.sql` and `queries/apikeys.sql`: `scope` added to both insert column lists, plus the two new returning-row deletes alongside the untouched originals. The `Get...ByPrefix` selects needed no edit, since they are `SELECT *`. 3. `dbauthz/dbauthz.go`: hand-written wrappers for the two new queries, each fetching by ID, authorizing delete against the fetched object, then delegating. The generic `deleteQ` helper does not fit, since it requires the delete to return only `error`. 4. `oauth2provider/authorize.go` and `oauth2provider/tokens.go`: the only production changes, all behavior-neutral. 5. `dbgen/dbgen.go` and `dbauthz/dbauthz_test.go`: seed threading, plus a case per new query. `MethodTestSuite` fails with "Method never called" for anything untested. Neither type needs to become auditable, which `make lint` confirms by not erroring on `enterprise/audit/table.go`. </details> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
316 lines
11 KiB
Go
316 lines
11 KiB
Go
package oauth2provider
|
|
|
|
import (
|
|
"crypto/sha256"
|
|
"database/sql"
|
|
"encoding/hex"
|
|
"errors"
|
|
htmltemplate "html/template"
|
|
"net/http"
|
|
"net/url"
|
|
"strings"
|
|
"time"
|
|
|
|
"github.com/google/uuid"
|
|
"github.com/justinas/nosurf"
|
|
"golang.org/x/xerrors"
|
|
|
|
"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/codersdk"
|
|
"github.com/coder/coder/v2/site"
|
|
)
|
|
|
|
type authorizeParams struct {
|
|
clientID string
|
|
redirectURL *url.URL
|
|
redirectURIProvided bool
|
|
responseType codersdk.OAuth2ProviderResponseType
|
|
scope []string
|
|
state string
|
|
resource string // RFC 8707 resource indicator
|
|
codeChallenge string // PKCE code challenge
|
|
codeChallengeMethod string // PKCE challenge method
|
|
}
|
|
|
|
func extractAuthorizeParams(r *http.Request, callbackURL *url.URL) (authorizeParams, []codersdk.ValidationError, error) {
|
|
p := httpapi.NewQueryParamParser()
|
|
vals := r.URL.Query()
|
|
|
|
// response_type and client_id are always required.
|
|
p.RequiredNotEmpty("response_type", "client_id")
|
|
|
|
params := authorizeParams{
|
|
clientID: p.String(vals, "", "client_id"),
|
|
redirectURL: p.RedirectURL(vals, callbackURL, "redirect_uri"),
|
|
redirectURIProvided: vals.Get("redirect_uri") != "",
|
|
responseType: httpapi.ParseCustom(p, vals, "", "response_type", httpapi.ParseEnum[codersdk.OAuth2ProviderResponseType]),
|
|
scope: strings.Fields(strings.TrimSpace(p.String(vals, "", "scope"))),
|
|
state: p.String(vals, "", "state"),
|
|
resource: p.String(vals, "", "resource"),
|
|
codeChallenge: p.String(vals, "", "code_challenge"),
|
|
codeChallengeMethod: p.String(vals, "", "code_challenge_method"),
|
|
}
|
|
|
|
// PKCE is required for authorization code flow requests. Reject a
|
|
// malformed code_challenge here (RFC 7636 §4.4.1) rather than storing it
|
|
// verbatim and failing later at token exchange, where the error would
|
|
// point at the code_verifier instead of the parameter that was actually
|
|
// invalid.
|
|
if params.responseType == codersdk.OAuth2ProviderResponseTypeCode {
|
|
switch {
|
|
case params.codeChallenge == "":
|
|
p.Errors = append(p.Errors, codersdk.ValidationError{
|
|
Field: "code_challenge",
|
|
Detail: `Query param "code_challenge" is required and cannot be empty`,
|
|
})
|
|
case !ValidPKCEFormat(params.codeChallenge):
|
|
p.Errors = append(p.Errors, codersdk.ValidationError{
|
|
Field: "code_challenge",
|
|
Detail: "must be 43 to 128 characters from the unreserved character set [A-Za-z0-9-._~]",
|
|
})
|
|
}
|
|
}
|
|
|
|
// Validate resource indicator syntax (RFC 8707): must be absolute URI without fragment
|
|
if err := validateResourceParameter(params.resource); err != nil {
|
|
p.Errors = append(p.Errors, codersdk.ValidationError{
|
|
Field: "resource",
|
|
Detail: "must be an absolute URI without fragment",
|
|
})
|
|
}
|
|
|
|
p.ErrorExcessParams(vals)
|
|
if len(p.Errors) > 0 {
|
|
// Create a readable error message with validation details
|
|
var errorDetails []string
|
|
for _, err := range p.Errors {
|
|
errorDetails = append(errorDetails, err.Error())
|
|
}
|
|
errorMsg := "Invalid query params: " + strings.Join(errorDetails, ", ")
|
|
return authorizeParams{}, p.Errors, xerrors.Errorf(errorMsg)
|
|
}
|
|
return params, nil, nil
|
|
}
|
|
|
|
// ShowAuthorizePage handles GET /oauth2/authorize requests to display the HTML authorization page.
|
|
func ShowAuthorizePage(accessURL *url.URL) http.HandlerFunc {
|
|
return func(rw http.ResponseWriter, r *http.Request) {
|
|
app := httpmw.OAuth2ProviderApp(r)
|
|
ua := httpmw.UserAuthorization(r.Context())
|
|
|
|
callbackURL, err := url.Parse(app.CallbackURL)
|
|
if err != nil {
|
|
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
|
|
Status: http.StatusInternalServerError,
|
|
HideStatus: false,
|
|
Title: "Internal Server Error",
|
|
Description: err.Error(),
|
|
Actions: []site.Action{
|
|
{
|
|
URL: accessURL.String(),
|
|
Text: "Back to site",
|
|
},
|
|
},
|
|
})
|
|
return
|
|
}
|
|
|
|
params, validationErrs, err := extractAuthorizeParams(r, callbackURL)
|
|
if err != nil {
|
|
errStr := make([]string, len(validationErrs))
|
|
for i, err := range validationErrs {
|
|
errStr[i] = err.Detail
|
|
}
|
|
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
|
|
Status: http.StatusBadRequest,
|
|
HideStatus: false,
|
|
Title: "Invalid Query Parameters",
|
|
Description: "One or more query parameters are missing or invalid.",
|
|
Warnings: errStr,
|
|
Actions: []site.Action{
|
|
{
|
|
URL: accessURL.String(),
|
|
Text: "Back to site",
|
|
},
|
|
},
|
|
})
|
|
return
|
|
}
|
|
|
|
if params.responseType != codersdk.OAuth2ProviderResponseTypeCode {
|
|
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
|
|
Status: http.StatusBadRequest,
|
|
HideStatus: false,
|
|
Title: "Unsupported Response Type",
|
|
Description: "Only response_type=code is supported.",
|
|
Actions: []site.Action{
|
|
{
|
|
URL: accessURL.String(),
|
|
Text: "Back to site",
|
|
},
|
|
},
|
|
})
|
|
return
|
|
}
|
|
|
|
cancel := params.redirectURL
|
|
cancelQuery := params.redirectURL.Query()
|
|
cancelQuery.Add("error", "access_denied")
|
|
cancelQuery.Add("error_description", "The resource owner or authorization server denied the request")
|
|
if params.state != "" {
|
|
cancelQuery.Add("state", params.state)
|
|
}
|
|
cancel.RawQuery = cancelQuery.Encode()
|
|
|
|
cancelURI := cancel.String()
|
|
if err := codersdk.ValidateRedirectURIScheme(cancel); err != nil {
|
|
site.RenderStaticErrorPage(rw, r, site.ErrorPageData{
|
|
Status: http.StatusBadRequest,
|
|
HideStatus: false,
|
|
Title: "Invalid Callback URL",
|
|
Description: "The application's registered callback URL has an invalid scheme.",
|
|
Actions: []site.Action{
|
|
{
|
|
URL: accessURL.String(),
|
|
Text: "Back to site",
|
|
},
|
|
},
|
|
})
|
|
return
|
|
}
|
|
|
|
site.RenderOAuthAllowPage(rw, r, site.RenderOAuthAllowData{
|
|
AppIcon: app.Icon,
|
|
AppName: app.Name,
|
|
// #nosec G203 -- The scheme is validated by
|
|
// codersdk.ValidateRedirectURIScheme above.
|
|
CancelURI: htmltemplate.URL(cancelURI),
|
|
DashboardURL: accessURL.String(),
|
|
CSRFToken: nosurf.Token(r),
|
|
Username: ua.FriendlyName,
|
|
})
|
|
}
|
|
}
|
|
|
|
// 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 {
|
|
return func(rw http.ResponseWriter, r *http.Request) {
|
|
ctx := r.Context()
|
|
apiKey := httpmw.APIKey(r)
|
|
app := httpmw.OAuth2ProviderApp(r)
|
|
|
|
callbackURL, err := url.Parse(app.CallbackURL)
|
|
if err != nil {
|
|
httpapi.WriteOAuth2Error(r.Context(), rw, http.StatusInternalServerError, codersdk.OAuth2ErrorCodeServerError, "Failed to validate query parameters")
|
|
return
|
|
}
|
|
|
|
params, _, err := extractAuthorizeParams(r, callbackURL)
|
|
if err != nil {
|
|
httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeInvalidRequest, err.Error())
|
|
return
|
|
}
|
|
|
|
// OAuth 2.1 removes the implicit grant. Only
|
|
// authorization code flow is supported.
|
|
if params.responseType != codersdk.OAuth2ProviderResponseTypeCode {
|
|
httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest,
|
|
codersdk.OAuth2ErrorCodeUnsupportedResponseType,
|
|
"Only response_type=code is supported")
|
|
return
|
|
}
|
|
|
|
// code_challenge is required (enforced by RequiredNotEmpty above),
|
|
// but default the method to S256 if omitted.
|
|
if params.codeChallengeMethod == "" {
|
|
params.codeChallengeMethod = string(codersdk.OAuth2PKCECodeChallengeMethodS256)
|
|
}
|
|
if err := codersdk.ValidatePKCECodeChallengeMethod(params.codeChallengeMethod); err != nil {
|
|
httpapi.WriteOAuth2Error(ctx, rw, http.StatusBadRequest, codersdk.OAuth2ErrorCodeInvalidRequest, err.Error())
|
|
return
|
|
}
|
|
|
|
// TODO: Ignoring scope for now, but should look into implementing.
|
|
code, err := GenerateSecret()
|
|
if err != nil {
|
|
httpapi.WriteOAuth2Error(r.Context(), rw, http.StatusInternalServerError, codersdk.OAuth2ErrorCodeServerError, "Failed to generate OAuth2 app authorization code")
|
|
return
|
|
}
|
|
err = db.InTx(func(tx database.Store) error {
|
|
// Delete any previous codes.
|
|
err = tx.DeleteOAuth2ProviderAppCodesByAppAndUserID(ctx, database.DeleteOAuth2ProviderAppCodesByAppAndUserIDParams{
|
|
AppID: app.ID,
|
|
UserID: apiKey.UserID,
|
|
})
|
|
if err != nil && !errors.Is(err, sql.ErrNoRows) {
|
|
return xerrors.Errorf("delete oauth2 app codes: %w", err)
|
|
}
|
|
|
|
// Insert the new code.
|
|
_, err = tx.InsertOAuth2ProviderAppCode(ctx, database.InsertOAuth2ProviderAppCodeParams{
|
|
ID: uuid.New(),
|
|
CreatedAt: dbtime.Now(),
|
|
// TODO: Configurable expiration? Ten minutes matches GitHub.
|
|
// This timeout is only for the code that will be exchanged for the
|
|
// access token, not the access token itself. It does not need to be
|
|
// long-lived because normally it will be exchanged immediately after it
|
|
// is received. If the application does wait before exchanging the
|
|
// token (for example suppose they ask the user to confirm and the user
|
|
// has left) then they can just retry immediately and get a new code.
|
|
ExpiresAt: dbtime.Now().Add(time.Duration(10) * time.Minute),
|
|
SecretPrefix: []byte(code.Prefix),
|
|
HashedSecret: code.Hashed,
|
|
AppID: app.ID,
|
|
UserID: apiKey.UserID,
|
|
ResourceUri: sql.NullString{String: params.resource, Valid: params.resource != ""},
|
|
CodeChallenge: sql.NullString{String: params.codeChallenge, Valid: params.codeChallenge != ""},
|
|
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),
|
|
})
|
|
if err != nil {
|
|
return xerrors.Errorf("insert oauth2 authorization code: %w", err)
|
|
}
|
|
|
|
return nil
|
|
}, nil)
|
|
if err != nil {
|
|
httpapi.WriteOAuth2Error(ctx, rw, http.StatusInternalServerError, codersdk.OAuth2ErrorCodeServerError, "Failed to generate OAuth2 authorization code")
|
|
return
|
|
}
|
|
|
|
newQuery := params.redirectURL.Query()
|
|
newQuery.Add("code", code.Formatted)
|
|
if params.state != "" {
|
|
newQuery.Add("state", params.state)
|
|
}
|
|
params.redirectURL.RawQuery = newQuery.Encode()
|
|
|
|
// (ThomasK33): Use a 302 redirect as some (external) OAuth 2 apps and browsers
|
|
// do not work with the 307.
|
|
http.Redirect(rw, r, params.redirectURL.String(), http.StatusFound)
|
|
}
|
|
}
|
|
|
|
// hashOAuth2State returns a SHA-256 hash of the OAuth2 state parameter. If
|
|
// the state is empty, it returns a null string.
|
|
func hashOAuth2State(state string) sql.NullString {
|
|
if state == "" {
|
|
return sql.NullString{}
|
|
}
|
|
hash := sha256.Sum256([]byte(state))
|
|
return sql.NullString{
|
|
String: hex.EncodeToString(hash[:]),
|
|
Valid: true,
|
|
}
|
|
}
|