Skip to content

Commit 3e18461

Browse files
BobbyHoclaude
BobbyHo
and
claude
committed
test(coderd/oauth2provider): cover the negotiated scope end to end
Assert the issued access token against the live API rather than only against the api_keys row: coder:workspaces.access reads a template and is refused the deletion. Drive every name the scope catalog offers through the conversion so a name added to the catalog but not the api_key_scope enum fails here instead of leaving a client holding an unredeemable code. Refresh a token row seeded the way migration 000569 leaves a pre-existing grant to confirm the backfilled coder:all still means unrestricted. Document the scope parameter, the registration-time allowlist, and the two limitations that remain: only Dynamic Client Registration can declare an allowlist, and a scope parameter on refresh is ignored. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 6178479 commit 3e18461

4 files changed

Lines changed: 152 additions & 8 deletions

File tree

‎coderd/oauth2provider/tokens.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,11 @@ var (
5555
// database: the request was well-formed and the deployment's data is not. This
5656
// returns errUnstorableScope instead, which Tokens() maps to invalid_scope.
5757
//
58-
// The empty case is defensive only. The column is NOT NULL with
59-
// CHECK (scope <> ”), and authorization always writes at least the
60-
// unrestricted sentinel, so a row cannot reach here empty. Treating it as
61-
// unrestricted rather than as an error would silently widen a grant, so it is
62-
// rejected too.
58+
// The empty case is defensive only. The column is NOT NULL with a CHECK
59+
// constraint rejecting the empty string, and authorization always writes at
60+
// least the unrestricted sentinel, so a row cannot reach here empty. Treating
61+
// it as unrestricted rather than as an error would silently widen a grant, so
62+
// it is rejected too.
6363
func scopeStringToAPIKeyScopes(scope string) (database.APIKeyScopes, error) {
6464
names := strings.Fields(scope)
6565
if len(names) == 0 {

‎coderd/oauth2provider/tokens_internal_test.go‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"github.com/stretchr/testify/require"
1010

1111
"github.com/coder/coder/v2/coderd/database"
12+
"github.com/coder/coder/v2/coderd/rbac"
1213
"github.com/coder/coder/v2/codersdk"
1314
)
1415

@@ -36,6 +37,23 @@ func TestScopeStringToAPIKeyScopes(t *testing.T) {
3637
}, scopes)
3738
})
3839

40+
// The scope catalog and the api_key_scope enum are maintained separately.
41+
// A name the catalog offers but the enum does not define is negotiable at
42+
// authorization and unmintable at exchange, so the client would hold a
43+
// code it can never redeem. Driving the whole catalog through the
44+
// conversion catches that drift when a name is added on one side only.
45+
t.Run("EveryCatalogNameMintable", func(t *testing.T) {
46+
t.Parallel()
47+
48+
names := rbac.ExternalScopeNames()
49+
require.NotEmpty(t, names)
50+
for _, name := range names {
51+
scopes, err := scopeStringToAPIKeyScopes(name)
52+
require.NoErrorf(t, err, "scope %q can be negotiated but not minted", name)
53+
require.Equal(t, database.APIKeyScopes{database.APIKeyScope(name)}, scopes)
54+
}
55+
})
56+
3957
t.Run("UnknownNameRejected", func(t *testing.T) {
4058
t.Parallel()
4159

‎coderd/oauth2provider/tokens_test.go‎

Lines changed: 105 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,72 @@ func TestOAuth2TokenExchangeScope(t *testing.T) {
9292
mintedKeyScopes(ctx, t, db, token.RefreshToken))
9393
})
9494

95+
// The scopes on the key only mean something once the authorizer reads them,
96+
// so this drives the issued access token against the real API: one action
97+
// the negotiated scope covers, one it does not. coder:workspaces.access
98+
// carries template:read but not template:delete, and the user behind the
99+
// grant owns the deployment, so the role permits both calls and the scope
100+
// is the only thing standing between the client and the deletion.
101+
t.Run("IssuedTokenBoundsTheAPI", func(t *testing.T) {
102+
t.Parallel()
103+
ctx := testutil.Context(t, testutil.WaitLong)
104+
105+
tpl := dbgen.Template(t, db, database.Template{
106+
OrganizationID: owner.OrganizationID,
107+
CreatedBy: owner.UserID,
108+
})
109+
110+
app := seedAppWithSecret(t, db, sql.NullString{String: scopeInCatalog, Valid: true})
111+
code, verifier := authorizeCode(ctx, t, client, app.ID.String(), "")
112+
token := exchangeCode(ctx, t, client, app, code, verifier)
113+
114+
asApp := codersdk.New(client.URL)
115+
asApp.SetSessionToken(token.AccessToken)
116+
117+
got, err := asApp.Template(ctx, tpl.ID)
118+
require.NoError(t, err, "template:read is within the negotiated scope")
119+
require.Equal(t, tpl.ID, got.ID)
120+
121+
err = asApp.DeleteTemplate(ctx, tpl.ID)
122+
var sdkErr *codersdk.Error
123+
require.ErrorAs(t, err, &sdkErr)
124+
require.Equal(t, http.StatusForbidden, sdkErr.StatusCode())
125+
})
126+
127+
// A grant that predates the scope columns carries what migration 000569
128+
// backfilled onto it: coder:all, which records an unrestricted grant rather
129+
// than an absent one. Refreshing one has to keep working and has to keep
130+
// meaning unrestricted, so the row is seeded the way the migration leaves
131+
// it instead of being written by an exchange this server just ran.
132+
t.Run("BackfilledScopeRefreshesUnrestricted", func(t *testing.T) {
133+
t.Parallel()
134+
ctx := testutil.Context(t, testutil.WaitLong)
135+
136+
tpl := dbgen.Template(t, db, database.Template{
137+
OrganizationID: owner.OrganizationID,
138+
CreatedBy: owner.UserID,
139+
})
140+
141+
app := seedAppWithSecret(t, db, sql.NullString{})
142+
refreshToken := seedRefreshToken(ctx, t, db, app, owner.UserID, string(database.ApiKeyScopeCoderAll))
143+
144+
form := url.Values{}
145+
form.Set("grant_type", "refresh_token")
146+
form.Set("refresh_token", refreshToken)
147+
form.Set("client_id", app.ID.String())
148+
form.Set("client_secret", app.ClientSecret)
149+
status, body := postTokenRequest(ctx, t, client, form)
150+
refreshed := requireTokenResponse(t, status, body)
151+
152+
require.Equal(t, database.APIKeyScopes{database.ApiKeyScopeCoderAll},
153+
mintedKeyScopes(ctx, t, db, refreshed.RefreshToken))
154+
155+
asApp := codersdk.New(client.URL)
156+
asApp.SetSessionToken(refreshed.AccessToken)
157+
require.NoError(t, asApp.DeleteTemplate(ctx, tpl.ID),
158+
"an unrestricted grant must still reach what it reached before")
159+
})
160+
95161
// A scope the api_key_scope enum does not define cannot become a key. The
96162
// authorize endpoint cannot produce such a row, so the code is seeded
97163
// directly: the case covers a row written before the name was removed, or
@@ -125,6 +191,7 @@ func TestOAuth2TokenExchangeScope(t *testing.T) {
125191
type appWithSecret struct {
126192
database.OAuth2ProviderApp
127193
ClientSecret string
194+
SecretID uuid.UUID
128195
}
129196

130197
func seedAppWithSecret(t *testing.T, db database.Store, allowlist sql.NullString) appWithSecret {
@@ -138,13 +205,49 @@ func seedAppWithSecret(t *testing.T, db database.Store, allowlist sql.NullString
138205

139206
secret, err := oauth2provider.GenerateSecret()
140207
require.NoError(t, err)
141-
dbgen.OAuth2ProviderAppSecret(t, db, database.OAuth2ProviderAppSecret{
208+
dbSecret := dbgen.OAuth2ProviderAppSecret(t, db, database.OAuth2ProviderAppSecret{
142209
AppID: app.ID,
143210
SecretPrefix: []byte(secret.Prefix),
144211
HashedSecret: secret.Hashed,
145212
})
146213

147-
return appWithSecret{OAuth2ProviderApp: app, ClientSecret: secret.Formatted}
214+
return appWithSecret{
215+
OAuth2ProviderApp: app,
216+
ClientSecret: secret.Formatted,
217+
SecretID: dbSecret.ID,
218+
}
219+
}
220+
221+
// seedRefreshToken writes a refresh token row for an existing grant and returns
222+
// the secret that redeems it, so a refresh can be exercised without the
223+
// exchange that would otherwise have written the row. dbgen is not used because
224+
// it derives expires_at from created_at, which would leave the row expired and
225+
// fail the refresh before it reaches the scope.
226+
func seedRefreshToken(ctx context.Context, t *testing.T, db database.Store, app appWithSecret, userID uuid.UUID, scope string) string {
227+
t.Helper()
228+
229+
key, _ := dbgen.APIKey(t, db, database.APIKey{
230+
UserID: userID,
231+
LoginType: database.LoginTypeOAuth2ProviderApp,
232+
})
233+
234+
secret, err := oauth2provider.GenerateSecret()
235+
require.NoError(t, err)
236+
237+
_, err = db.InsertOAuth2ProviderAppToken(dbauthz.AsSystemRestricted(ctx), database.InsertOAuth2ProviderAppTokenParams{
238+
ID: uuid.New(),
239+
CreatedAt: dbtime.Now(),
240+
ExpiresAt: dbtime.Now().Add(time.Hour),
241+
HashPrefix: []byte(secret.Prefix),
242+
RefreshHash: secret.Hashed,
243+
AppID: app.ID,
244+
AppSecretID: uuid.NullUUID{UUID: app.SecretID, Valid: true},
245+
APIKeyID: key.ID,
246+
UserID: userID,
247+
Scope: scope,
248+
})
249+
require.NoError(t, err)
250+
return secret.Formatted
148251
}
149252

150253
// authorizeCode runs a full authorization and returns the issued code with the

‎docs/admin/integrations/oauth2-provider.md‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -220,6 +220,28 @@ confidential clients must include PKCE parameters:
220220
"$CODER_URL/oauth2/tokens"
221221
```
222222

223+
## Scopes
224+
225+
An access token is bounded by the scope negotiated when the user authorized it, on top of that user's own permissions. A token can never do more than its user can.
226+
227+
Scope names come from the same vocabulary as [API key scopes](../users/sessions-tokens.md#api-key-scopes): individual `resource:action` names such as `workspace:ssh`, and `coder:` composites such as `coder:workspaces.access` that stand for a set of them. `coder:all` records an unrestricted grant.
228+
229+
A client asks for a scope with the `scope` parameter on the authorization request, space separated:
230+
231+
```txt
232+
https://coder.example.com/oauth2/authorize?
233+
client_id=your-client-id&
234+
response_type=code&
235+
scope=coder:workspaces.access&
236+
code_challenge=$CODE_CHALLENGE&
237+
code_challenge_method=S256&
238+
redirect_uri=https://yourapp.example.com/callback
239+
```
240+
241+
An application registered through [Dynamic Client Registration](#dynamic-client-registration) can declare a `scope` field, which acts as an allowlist. The client may then request anything that allowlist covers, and is granted the whole allowlist if it requests nothing. Applications created through the web UI or the management API declare no allowlist, so any requested scope is honored and a request that names no scope is granted `coder:all`.
242+
243+
The consent page states the scope being granted before the user approves it, and refreshing a token keeps the scope originally granted.
244+
223245
## Discovery Endpoints
224246

225247
Coder provides OAuth2 discovery endpoints for programmatic integration:
@@ -367,7 +389,8 @@ Public clients (`token_endpoint_auth_method: none`) additionally cannot register
367389

368390
As an experimental feature, the current implementation has limitations:
369391

370-
- No scope system - all tokens have full API access
392+
- A scope allowlist can only be declared at [Dynamic Client Registration](#dynamic-client-registration); applications created through the web UI or the management API cannot restrict which scopes a client may request
393+
- A `scope` parameter on a refresh request is ignored, and the refreshed token keeps the scope originally granted
371394
- No client credentials grant support
372395
- Implicit grant (`response_type=token`) is not supported; OAuth 2.1
373396
deprecated this flow due to token leakage risks, and requests return

0 commit comments

Comments
 (0)