Skip to content

Commit 055d631

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 9dfcc8a commit 055d631

34 files changed

+568
-409
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: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5341,7 +5341,18 @@ func (s *MethodTestSuite) TestOAuth2ProviderApps() {
53415341
})
53425342
}))
53435343
s.Run("InsertOAuth2ProviderApp", s.Subtest(func(db database.Store, check *expects) {
5344-
check.Args(database.InsertOAuth2ProviderAppParams{}).Asserts(rbac.ResourceOauth2App, policy.ActionCreate)
5344+
check.Args(database.InsertOAuth2ProviderAppParams{
5345+
ID: uuid.New(),
5346+
Name: fmt.Sprintf("test-app-%d", time.Now().UnixNano()),
5347+
CreatedAt: dbtestutil.NowInDefaultTimezone(),
5348+
UpdatedAt: dbtestutil.NowInDefaultTimezone(),
5349+
RedirectUris: []string{"http://localhost"},
5350+
ClientType: sql.NullString{String: "confidential", Valid: true},
5351+
DynamicallyRegistered: sql.NullBool{Bool: false, Valid: true},
5352+
GrantTypes: []string{"authorization_code", "refresh_token"},
5353+
ResponseTypes: []string{"code"},
5354+
TokenEndpointAuthMethod: sql.NullString{String: "client_secret_basic", Valid: true},
5355+
}).Asserts(rbac.ResourceOauth2App, policy.ActionCreate)
53455356
}))
53465357
s.Run("UpdateOAuth2ProviderAppByID", s.Subtest(func(db database.Store, check *expects) {
53475358
dbtestutil.DisableForeignKeysAndTriggers(s.T(), db)
@@ -5352,7 +5363,6 @@ func (s *MethodTestSuite) TestOAuth2ProviderApps() {
53525363
ID: app.ID,
53535364
Name: app.Name,
53545365
Icon: app.Icon,
5355-
CallbackURL: app.CallbackURL,
53565366
RedirectUris: app.RedirectUris,
53575367
ClientType: app.ClientType,
53585368
DynamicallyRegistered: app.DynamicallyRegistered,
@@ -5394,7 +5404,6 @@ func (s *MethodTestSuite) TestOAuth2ProviderApps() {
53945404
ID: app.ID,
53955405
Name: app.Name,
53965406
Icon: app.Icon,
5397-
CallbackURL: app.CallbackURL,
53985407
RedirectUris: app.RedirectUris,
53995408
ClientType: app.ClientType,
54005409
ClientSecretExpiresAt: app.ClientSecretExpiresAt,

‎coderd/database/dbgen/dbgen.go

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