Skip to content

Commit bdc94d5

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 aca2f6a commit bdc94d5

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
@@ -356,10 +356,10 @@ func TemplateVersionParameterOptionFromPreview(option *previewtypes.ParameterOpt
356356

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

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5307,7 +5307,18 @@ func (s *MethodTestSuite) TestOAuth2ProviderApps() {
53075307
})
53085308
}))
53095309
s.Run("InsertOAuth2ProviderApp", s.Subtest(func(db database.Store, check *expects) {
5310-
check.Args(database.InsertOAuth2ProviderAppParams{}).Asserts(rbac.ResourceOauth2App, policy.ActionCreate)
5310+
check.Args(database.InsertOAuth2ProviderAppParams{
5311+
ID: uuid.New(),
5312+
Name: fmt.Sprintf("test-app-%d", time.Now().UnixNano()),
5313+
CreatedAt: dbtestutil.NowInDefaultTimezone(),
5314+
UpdatedAt: dbtestutil.NowInDefaultTimezone(),
5315+
RedirectUris: []string{"http://localhost"},
5316+
ClientType: sql.NullString{String: "confidential", Valid: true},
5317+
DynamicallyRegistered: sql.NullBool{Bool: false, Valid: true},
5318+
GrantTypes: []string{"authorization_code", "refresh_token"},
5319+
ResponseTypes: []string{"code"},
5320+
TokenEndpointAuthMethod: sql.NullString{String: "client_secret_basic", Valid: true},
5321+
}).Asserts(rbac.ResourceOauth2App, policy.ActionCreate)
53115322
}))
53125323
s.Run("UpdateOAuth2ProviderAppByID", s.Subtest(func(db database.Store, check *expects) {
53135324
dbtestutil.DisableForeignKeysAndTriggers(s.T(), db)
@@ -5318,7 +5329,6 @@ func (s *MethodTestSuite) TestOAuth2ProviderApps() {
53185329
ID: app.ID,
53195330
Name: app.Name,
53205331
Icon: app.Icon,
5321-
CallbackURL: app.CallbackURL,
53225332
RedirectUris: app.RedirectUris,
53235333
ClientType: app.ClientType,
53245334
DynamicallyRegistered: app.DynamicallyRegistered,
@@ -5360,7 +5370,6 @@ func (s *MethodTestSuite) TestOAuth2ProviderApps() {
53605370
ID: app.ID,
53615371
Name: app.Name,
53625372
Icon: app.Icon,
5363-
CallbackURL: app.CallbackURL,
53645373
RedirectUris: app.RedirectUris,
53655374
ClientType: app.ClientType,
53665375
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/000357_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/000357_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)