Skip to content

Commit bed62ad

Browse files
ThomasK33ThomasK33
committed
feat: replace callback_url with redirect_uris for OAuth2 RFC 6749 compliance
Change-Id: I4823e475777ebdf75e3a80e47ff6bef1a556cd55 Signed-off-by: Thomas Kosiewski <tk@coder.com>
1 parent 9393060 commit bed62ad

34 files changed

+590
-408
lines changed

‎coderd/apidoc/docs.go

Lines changed: 25 additions & 14 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/apidoc/swagger.json

Lines changed: 23 additions & 12 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/db2sdk/db2sdk.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -355,10 +355,10 @@ func TemplateVersionParameterOptionFromPreview(option *previewtypes.ParameterOpt
355355

356356
func OAuth2ProviderApp(accessURL *url.URL, dbApp database.OAuth2ProviderApp) codersdk.OAuth2ProviderApp {
357357
return codersdk.OAuth2ProviderApp{
358-
ID: dbApp.ID,
359-
Name: dbApp.Name,
360-
CallbackURL: dbApp.CallbackURL,
361-
Icon: dbApp.Icon,
358+
ID: dbApp.ID,
359+
Name: dbApp.Name,
360+
RedirectURIs: dbApp.RedirectUris,
361+
Icon: dbApp.Icon,
362362
Endpoints: codersdk.OAuth2AppEndpoints{
363363
Authorization: accessURL.ResolveReference(&url.URL{
364364
Path: "/oauth2/authorize",

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5336,7 +5336,33 @@ func (s *MethodTestSuite) TestOAuth2ProviderApps() {
53365336
})
53375337
}))
53385338
s.Run("InsertOAuth2ProviderApp", s.Subtest(func(db database.Store, check *expects) {
5339-
check.Args(database.InsertOAuth2ProviderAppParams{}).Asserts(rbac.ResourceOauth2App, policy.ActionCreate)
5339+
check.Args(database.InsertOAuth2ProviderAppParams{
5340+
ID: uuid.New(),
5341+
Name: fmt.Sprintf("test-app-%d", time.Now().UnixNano()),
5342+
CreatedAt: dbtestutil.NowInDefaultTimezone(),
5343+
UpdatedAt: dbtestutil.NowInDefaultTimezone(),
5344+
Icon: "",
5345+
RedirectUris: []string{"http://localhost"},
5346+
ClientType: sql.NullString{String: "confidential", Valid: true},
5347+
DynamicallyRegistered: sql.NullBool{Bool: false, Valid: true},
5348+
ClientIDIssuedAt: sql.NullTime{},
5349+
ClientSecretExpiresAt: sql.NullTime{},
5350+
GrantTypes: []string{"authorization_code", "refresh_token"},
5351+
ResponseTypes: []string{"code"},
5352+
TokenEndpointAuthMethod: sql.NullString{String: "client_secret_basic", Valid: true},
5353+
Scope: sql.NullString{},
5354+
Contacts: []string{},
5355+
ClientUri: sql.NullString{},
5356+
LogoUri: sql.NullString{},
5357+
TosUri: sql.NullString{},
5358+
PolicyUri: sql.NullString{},
5359+
JwksUri: sql.NullString{},
5360+
Jwks: pqtype.NullRawMessage{},
5361+
SoftwareID: sql.NullString{},
5362+
SoftwareVersion: sql.NullString{},
5363+
RegistrationAccessToken: sql.NullString{},
5364+
RegistrationClientUri: sql.NullString{},
5365+
}).Asserts(rbac.ResourceOauth2App, policy.ActionCreate)
53405366
}))
53415367
s.Run("UpdateOAuth2ProviderAppByID", s.Subtest(func(db database.Store, check *expects) {
53425368
dbtestutil.DisableForeignKeysAndTriggers(s.T(), db)
@@ -5347,7 +5373,6 @@ func (s *MethodTestSuite) TestOAuth2ProviderApps() {
53475373
ID: app.ID,
53485374
Name: app.Name,
53495375
Icon: app.Icon,
5350-
CallbackURL: app.CallbackURL,
53515376
RedirectUris: app.RedirectUris,
53525377
ClientType: app.ClientType,
53535378
DynamicallyRegistered: app.DynamicallyRegistered,
@@ -5389,7 +5414,6 @@ func (s *MethodTestSuite) TestOAuth2ProviderApps() {
53895414
ID: app.ID,
53905415
Name: app.Name,
53915416
Icon: app.Icon,
5392-
CallbackURL: app.CallbackURL,
53935417
RedirectUris: app.RedirectUris,
53945418
ClientType: app.ClientType,
53955419
ClientSecretExpiresAt: app.ClientSecretExpiresAt,

‎coderd/database/dbgen/dbgen.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1203,8 +1203,7 @@ func OAuth2ProviderApp(t testing.TB, db database.Store, seed database.OAuth2Prov
12031203
CreatedAt: takeFirst(seed.CreatedAt, dbtime.Now()),
12041204
UpdatedAt: takeFirst(seed.UpdatedAt, dbtime.Now()),
12051205
Icon: takeFirst(seed.Icon, ""),
1206-
CallbackURL: takeFirst(seed.CallbackURL, "http://localhost"),
1207-
RedirectUris: takeFirstSlice(seed.RedirectUris, []string{}),
1206+
RedirectUris: takeFirstSlice(seed.RedirectUris, []string{"http://localhost"}),
12081207
ClientType: takeFirst(seed.ClientType, sql.NullString{String: "confidential", Valid: true}),
12091208
DynamicallyRegistered: takeFirst(seed.DynamicallyRegistered, sql.NullBool{Bool: false, Valid: true}),
12101209
ClientIDIssuedAt: takeFirst(seed.ClientIDIssuedAt, sql.NullTime{}),

‎coderd/database/dump.sql

Lines changed: 4 additions & 4 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/migrations/000351_remove_oauth2_callback_url.down.sql

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
-- Reverse migration: restore callback_url column from redirect_uris
2+
3+
-- Add back the callback_url column
4+
ALTER TABLE oauth2_provider_apps
5+
ADD COLUMN callback_url text;
6+
7+
-- Populate callback_url from the first redirect_uri
8+
UPDATE oauth2_provider_apps
9+
SET callback_url = redirect_uris[1]
10+
WHERE redirect_uris IS NOT NULL AND array_length(redirect_uris, 1) > 0;
11+
12+
-- Remove NOT NULL and CHECK constraints from redirect_uris (restore original state)
13+
ALTER TABLE oauth2_provider_apps
14+
DROP CONSTRAINT IF EXISTS oauth2_provider_apps_redirect_uris_nonempty;
15+
ALTER TABLE oauth2_provider_apps
16+
ALTER COLUMN redirect_uris DROP NOT NULL;
17+
18+
COMMENT ON COLUMN oauth2_provider_apps.callback_url IS 'Legacy callback URL field (replaced by redirect_uris array)';

‎coderd/database/migrations/000351_remove_oauth2_callback_url.up.sql

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
-- Migrate from callback_url to redirect_uris as source of truth for OAuth2 apps
2+
-- RFC 6749 compliance: use array of redirect URIs instead of single callback URL
3+
4+
-- Populate redirect_uris from callback_url where redirect_uris is empty or NULL.
5+
-- NULLIF is used to treat empty strings in callback_url as NULL.
6+
-- If callback_url is NULL or empty, this will result in redirect_uris
7+
-- being an array with a single NULL element. This is preferable to an empty
8+
-- array as it will pass a CHECK for array length > 0, enforcing that all
9+
-- apps have at least one URI entry, even if it's null.
10+
UPDATE oauth2_provider_apps
11+
SET redirect_uris = ARRAY[NULLIF(callback_url, '')]
12+
WHERE (redirect_uris IS NULL OR cardinality(redirect_uris) = 0);
13+
14+
-- Add NOT NULL constraint to redirect_uris
15+
ALTER TABLE oauth2_provider_apps
16+
ALTER COLUMN redirect_uris SET NOT NULL;
17+
18+
-- Add CHECK constraint to ensure redirect_uris is not empty.
19+
-- This prevents empty arrays, which could have been created by the old logic,
20+
-- and ensures data integrity going forward.
21+
ALTER TABLE oauth2_provider_apps
22+
ADD CONSTRAINT redirect_uris_not_empty CHECK (cardinality(redirect_uris) > 0);
23+
24+
-- Drop the callback_url column entirely
25+
ALTER TABLE oauth2_provider_apps
26+
DROP COLUMN callback_url;
27+
28+
COMMENT ON COLUMN oauth2_provider_apps.redirect_uris IS 'RFC 6749 compliant list of valid redirect URIs for the application';

‎coderd/database/models.go

Lines changed: 6 additions & 7 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)