diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index e31b065430..d106e6a585 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -8983,12 +8983,6 @@ func (q *FakeQuerier) InsertOAuth2ProviderApp(_ context.Context, arg database.In q.mutex.Lock() defer q.mutex.Unlock() - for _, app := range q.oauth2ProviderApps { - if app.Name == arg.Name { - return database.OAuth2ProviderApp{}, errUniqueConstraint - } - } - //nolint:gosimple // Go wants database.OAuth2ProviderApp(arg), but we cannot be sure the structs will remain identical. app := database.OAuth2ProviderApp{ ID: arg.ID, diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 0cd3e0d4da..54f984294f 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -2494,9 +2494,6 @@ ALTER TABLE ONLY oauth2_provider_app_tokens ALTER TABLE ONLY oauth2_provider_app_tokens ADD CONSTRAINT oauth2_provider_app_tokens_pkey PRIMARY KEY (id); -ALTER TABLE ONLY oauth2_provider_apps - ADD CONSTRAINT oauth2_provider_apps_name_key UNIQUE (name); - ALTER TABLE ONLY oauth2_provider_apps ADD CONSTRAINT oauth2_provider_apps_pkey PRIMARY KEY (id); diff --git a/coderd/database/migrations/000348_remove_oauth2_app_name_unique_constraint.down.sql b/coderd/database/migrations/000348_remove_oauth2_app_name_unique_constraint.down.sql new file mode 100644 index 0000000000..eb9f3403a2 --- /dev/null +++ b/coderd/database/migrations/000348_remove_oauth2_app_name_unique_constraint.down.sql @@ -0,0 +1,3 @@ +-- Restore unique constraint on oauth2_provider_apps.name for rollback +-- Note: This rollback may fail if duplicate names exist in the database +ALTER TABLE oauth2_provider_apps ADD CONSTRAINT oauth2_provider_apps_name_key UNIQUE (name); \ No newline at end of file diff --git a/coderd/database/migrations/000348_remove_oauth2_app_name_unique_constraint.up.sql b/coderd/database/migrations/000348_remove_oauth2_app_name_unique_constraint.up.sql new file mode 100644 index 0000000000..f58fe95948 --- /dev/null +++ b/coderd/database/migrations/000348_remove_oauth2_app_name_unique_constraint.up.sql @@ -0,0 +1,3 @@ +-- Remove unique constraint on oauth2_provider_apps.name to comply with RFC 7591 +-- RFC 7591 does not require unique client names, only unique client IDs +ALTER TABLE oauth2_provider_apps DROP CONSTRAINT oauth2_provider_apps_name_key; \ No newline at end of file diff --git a/coderd/database/unique_constraint.go b/coderd/database/unique_constraint.go index 8377c630a6..b3af136997 100644 --- a/coderd/database/unique_constraint.go +++ b/coderd/database/unique_constraint.go @@ -36,7 +36,6 @@ const ( UniqueOauth2ProviderAppSecretsSecretPrefixKey UniqueConstraint = "oauth2_provider_app_secrets_secret_prefix_key" // ALTER TABLE ONLY oauth2_provider_app_secrets ADD CONSTRAINT oauth2_provider_app_secrets_secret_prefix_key UNIQUE (secret_prefix); UniqueOauth2ProviderAppTokensHashPrefixKey UniqueConstraint = "oauth2_provider_app_tokens_hash_prefix_key" // ALTER TABLE ONLY oauth2_provider_app_tokens ADD CONSTRAINT oauth2_provider_app_tokens_hash_prefix_key UNIQUE (hash_prefix); UniqueOauth2ProviderAppTokensPkey UniqueConstraint = "oauth2_provider_app_tokens_pkey" // ALTER TABLE ONLY oauth2_provider_app_tokens ADD CONSTRAINT oauth2_provider_app_tokens_pkey PRIMARY KEY (id); - UniqueOauth2ProviderAppsNameKey UniqueConstraint = "oauth2_provider_apps_name_key" // ALTER TABLE ONLY oauth2_provider_apps ADD CONSTRAINT oauth2_provider_apps_name_key UNIQUE (name); UniqueOauth2ProviderAppsPkey UniqueConstraint = "oauth2_provider_apps_pkey" // ALTER TABLE ONLY oauth2_provider_apps ADD CONSTRAINT oauth2_provider_apps_pkey PRIMARY KEY (id); UniqueOrganizationMembersPkey UniqueConstraint = "organization_members_pkey" // ALTER TABLE ONLY organization_members ADD CONSTRAINT organization_members_pkey PRIMARY KEY (organization_id, user_id); UniqueOrganizationsPkey UniqueConstraint = "organizations_pkey" // ALTER TABLE ONLY organizations ADD CONSTRAINT organizations_pkey PRIMARY KEY (id); diff --git a/coderd/oauth2_test.go b/coderd/oauth2_test.go index f485c2f0c7..3b3caeaa39 100644 --- a/coderd/oauth2_test.go +++ b/coderd/oauth2_test.go @@ -64,13 +64,6 @@ func TestOAuth2ProviderApps(t *testing.T) { CallbackURL: "http://localhost:3000", }, }, - { - name: "NameTaken", - req: codersdk.PostOAuth2ProviderAppRequest{ - Name: "taken", - CallbackURL: "http://localhost:3000", - }, - }, { name: "URLMissing", req: codersdk.PostOAuth2ProviderAppRequest{ @@ -135,17 +128,8 @@ func TestOAuth2ProviderApps(t *testing.T) { }, } - // Generate an application for testing name conflicts. - req := codersdk.PostOAuth2ProviderAppRequest{ - Name: "taken", - CallbackURL: "http://coder.com", - } - //nolint:gocritic // OAauth2 app management requires owner permission. - _, err := client.PostOAuth2ProviderApp(ctx, req) - require.NoError(t, err) - // Generate an application for testing PUTs. - req = codersdk.PostOAuth2ProviderAppRequest{ + req := codersdk.PostOAuth2ProviderAppRequest{ Name: fmt.Sprintf("quark-%d", time.Now().UnixNano()%1000000), CallbackURL: "http://coder.com", } @@ -271,6 +255,65 @@ func TestOAuth2ProviderApps(t *testing.T) { require.NoError(t, err) require.Len(t, apps, 0) }) + + t.Run("DuplicateNames", func(t *testing.T) { + t.Parallel() + client := coderdtest.New(t, nil) + _ = coderdtest.CreateFirstUser(t, client) + ctx := testutil.Context(t, testutil.WaitLong) + + // Create multiple OAuth2 apps with the same name to verify RFC 7591 compliance + // RFC 7591 allows multiple apps to have the same name + appName := fmt.Sprintf("duplicate-name-%d", time.Now().UnixNano()%1000000) + + // Create first app + //nolint:gocritic // OAuth2 app management requires owner permission. + app1, err := client.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{ + Name: appName, + CallbackURL: "http://localhost:3001", + }) + require.NoError(t, err) + require.Equal(t, appName, app1.Name) + + // Create second app with the same name + //nolint:gocritic // OAuth2 app management requires owner permission. + app2, err := client.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{ + Name: appName, + CallbackURL: "http://localhost:3002", + }) + require.NoError(t, err) + require.Equal(t, appName, app2.Name) + + // Create third app with the same name + //nolint:gocritic // OAuth2 app management requires owner permission. + app3, err := client.PostOAuth2ProviderApp(ctx, codersdk.PostOAuth2ProviderAppRequest{ + Name: appName, + CallbackURL: "http://localhost:3003", + }) + require.NoError(t, err) + require.Equal(t, appName, app3.Name) + + // Verify all apps have different IDs but same name + require.NotEqual(t, app1.ID, app2.ID) + require.NotEqual(t, app1.ID, app3.ID) + require.NotEqual(t, app2.ID, app3.ID) + require.Equal(t, app1.Name, app2.Name) + require.Equal(t, app1.Name, app3.Name) + + // Verify all apps can be retrieved and have the same name + //nolint:gocritic // OAuth2 app management requires owner permission. + apps, err := client.OAuth2ProviderApps(ctx, codersdk.OAuth2ProviderAppFilter{}) + require.NoError(t, err) + + // Count apps with our duplicate name + duplicateNameCount := 0 + for _, app := range apps { + if app.Name == appName { + duplicateNameCount++ + } + } + require.Equal(t, 3, duplicateNameCount, "Should have exactly 3 apps with the duplicate name") + }) } func TestOAuth2ProviderAppSecrets(t *testing.T) {