8000 fix: remove unique constraint on OAuth2 provider app names (#18669) · coder/coder@60b08f0 · GitHub
[go: up one dir, main page]
Skip to content

Commit 60b08f0

Browse files
ThomasK33ThomasK33
authored
fix: remove unique constraint on OAuth2 provider app names (#18669)
# Remove unique constraint on OAuth2 provider app names This PR removes the unique constraint on the `name` field in the `oauth2_provider_apps` table to comply with RFC 7591, which only requires unique client IDs, not unique client names. Changes include: - Removing the unique constraint from the database schema - Adding migration files for both up and down migrations - Removing the name uniqueness check in the in-memory database implementation - Updating the unique constraint constants Change-Id: Iae7a1a06546fbc8de541a52e291f8a4510d57e8a Signed-off-by: Thomas Kosiewski <tk@coder.com>
1 parent 90a875d commit 60b08f0

File tree

6 files changed

+66
-27
lines changed

6 files changed

+66
-27
lines changed

‎coderd/database/dbmem/dbmem.go

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8983,12 +8983,6 @@ func (q *FakeQuerier) InsertOAuth2ProviderApp(_ context.Context, arg database.In
89838983
q.mutex.Lock()
89848984
defer q.mutex.Unlock()
89858985

8986-
for _, app := range q.oauth2ProviderApps {
8987-
if app.Name == arg.Name {
8988-
return database.OAuth2ProviderApp{}, errUniqueConstraint
8989-
}
8990-
}
8991-
89928986
//nolint:gosimple // Go wants database.OAuth2ProviderApp(arg), but we cannot be sure the structs will remain identical.
89938987
app := database.OAuth2ProviderApp{
89948988
ID: arg.ID,

‎coderd/database/dump.sql

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

‎coderd/database/migrations/000348_remove_oauth2_app_name_unique_constraint.down.sql

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
-- Restore unique constraint on oauth2_provider_apps.name for rollback
2+
-- Note: This rollback may fail if duplicate names exist in the database
3+
ALTER TABLE oauth2_provider_apps ADD CONSTRAINT oauth2_provider_apps_name_key UNIQUE (name);

‎coderd/database/migrations/000348_remove_oauth2_app_name_unique_constraint.up.sql

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
-- Remove unique constraint on oauth2_provider_apps.name to comply with RFC 7591
2+
-- RFC 7591 does not require unique client names, only unique client IDs
3+
ALTER TABLE oauth2_provider_apps DROP CONSTRAINT oauth2_provider_apps_name_key;

‎coderd/database/unique_constraint.go

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

‎coderd/oauth2_test.go

Lines changed: 60 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -64,13 +64,6 @@ func TestOAuth2ProviderApps(t *testing.T) {
6464
CallbackURL: "http://localhost:3000",
6565
},
6666
},
67-
{
68-
name: "NameTaken",
69-
req: codersdk.PostOAuth2ProviderAppRequest{
70-
Name: "taken",
71-
CallbackURL: "http://localhost:3000",
72-
},
73-
},
7467
{
7568
name: "URLMissing",
7669
req: codersdk.PostOAuth2ProviderAppRequest{
@@ -135,17 +128,8 @@ func TestOAuth2ProviderApps(t *testing.T) {
135128
},
136129
}
137130

138-
// Generate an application for testing name conflicts.
139-
req := codersdk.PostOAuth2ProviderAppRequest{
140-
Name: "taken",
141-
CallbackURL: "http://coder.com",
142-
}
143-
//nolint:gocritic // OAauth2 app management requires owner permission.
144-
_, err := client.PostOAuth2ProviderApp(ctx, req)
145-
require.NoError(t, err)
146-
147131
// Generate an application for testing PUTs.
148-
req = codersdk.PostOAuth2ProviderAppRequest{
132+
req := codersdk.PostOAuth2ProviderAppRequest{
149133
Name: fmt.Sprintf("quark-%d", time.Now().UnixNano()%1000000),
150134
CallbackURL: "http://coder.com",
151135
}
@@ -271,6 +255,65 @@ func TestOAuth2ProviderApps(t *testing.T) {
271255
require.NoError(t, err)
272256
require.Len(t, apps, 0)
273257
})
258+
259+
t.Run("DuplicateNames", func(t *testing.T) {
260+
t.Parallel()
261+
client := coderdtest.New(t, nil)
262+
_ = coderdtest.CreateFirstUser(t, client)
263+
ctx := testutil.Context(t, testutil.WaitLong)
264+
265+
// Create multiple OAuth2 apps with the same name to verify RFC 7591 compliance
266+
// RFC 7591 allows multiple apps to have the same name
267+
appName := fmt.Sprintf("duplicate-name-%d", time.Now().UnixNano()%1000000)
268+
269+
// Create first app
270+
//nolint:gocritic // OAuth2 app management requires owner permission.
271+
app1, err := client.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{
272+
Name: appName,
273+
CallbackURL: "http://localhost:3001",
274+
})
275+
require.NoError(t, err)
276+
require.Equal(t, appName, app1.Name)
277+
278+
// Create second app with the same name
279+
//nolint:gocritic // OAuth2 app management requires owner permission.
280+
app2, err := client.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{
281+
Name: appName,
282+
CallbackURL: "http://localhost:3002",
283+
})
284+
require.NoError(t, err)
285+
require.Equal(t, appName, app2.Name)
286+
287+
// Create third app with the same name
288+
//nolint:gocritic // OAuth2 app management requires owner permission.
289+
app3, err := client.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{
290+
Name: appName,
291+
CallbackURL: "http://localhost:3003",
292+
})
293+
require.NoError(t, err)
294+
require.Equal(t, appName, app3.Name)
295+
296+
// Verify all apps have different IDs but same name
297+
require.NotEqual(t, app1.ID, app2.ID)
298+
require.NotEqual(t, app1.ID, app3.ID)
299+
require.NotEqual(t, app2.ID, app3.ID)
300+
require.Equal(t, app1.Name, app2.Name)
301+
require.Equal(t, app1.Name, app3.Name)
302+
303+
// Verify all apps can be retrieved and have the same name
304+
//nolint:gocritic // OAuth2 app management requires owner permission.
305+
apps, err := client.OAuth2ProviderApps(ctx, codersdk.OAuth2ProviderAppFilter{})
306+
require.NoError(t, err)
307+
308+
// Count apps with our duplicate name
309+
duplicateNameCount := 0
310+
for _, app := range apps {
311+
if app.Name == appName {
312+
duplicateNameCount++
313+
}
314+
}
315+
require.Equal(t, 3, duplicateNameCount, "Should have exactly 3 apps with the duplicate name")
316+
})
274317
}
275318

276319
func TestOAuth2ProviderAppSecrets(t *testing.T) {

0 commit comments

Comments
 (0)
0