From e10fceb23c25c3b43f92191bfb4ddbd9cc5a85c0 Mon Sep 17 00:00:00 2001 From: George K Date: Mon, 5 Jan 2026 07:43:08 -0800 Subject: [PATCH] fix(coderd/database): allow same custom role name for different orgs (#21312) Previously the `idx_custom_roles_name_lower` index prevented that. A check constraint was also added to ensure the `organization_id` column cannot be set to the all-zero UUID. --- coderd/database/check_constraint.go | 1 + coderd/database/dump.sql | 5 ++-- ..._same_role_name_in_different_orgs.down.sql | 6 ++++ ...ow_same_role_name_in_different_orgs.up.sql | 28 +++++++++++++++++++ ...ow_same_role_name_in_different_orgs.up.sql | 10 +++++++ coderd/database/unique_constraint.go | 2 +- 6 files changed, 49 insertions(+), 3 deletions(-) create mode 100644 coderd/database/migrations/000404_allow_same_role_name_in_different_orgs.down.sql create mode 100644 coderd/database/migrations/000404_allow_same_role_name_in_different_orgs.up.sql create mode 100644 coderd/database/migrations/testdata/fixtures/000403_pre_allow_same_role_name_in_different_orgs.up.sql diff --git a/coderd/database/check_constraint.go b/coderd/database/check_constraint.go index c8752b207d..ae22af7815 100644 --- a/coderd/database/check_constraint.go +++ b/coderd/database/check_constraint.go @@ -7,6 +7,7 @@ type CheckConstraint string // CheckConstraint enums. const ( CheckAPIKeysAllowListNotEmpty CheckConstraint = "api_keys_allow_list_not_empty" // api_keys + CheckOrganizationIDNotZero CheckConstraint = "organization_id_not_zero" // custom_roles CheckOneTimePasscodeSet CheckConstraint = "one_time_passcode_set" // users CheckUsersUsernameMinLength CheckConstraint = "users_username_min_length" // users CheckMaxProvisionerLogsLength CheckConstraint = "max_provisioner_logs_length" // provisioner_jobs diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 790659669f..c7255565df 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -1202,7 +1202,8 @@ CREATE TABLE custom_roles ( created_at timestamp with time zone DEFAULT CURRENT_TIMESTAMP NOT NULL, updated_at timestamp with time zone DEFAULT CURRENT_TIMESTAMP NOT NULL, organization_id uuid, - id uuid DEFAULT gen_random_uuid() NOT NULL + id uuid DEFAULT gen_random_uuid() NOT NULL, + CONSTRAINT organization_id_not_zero CHECK ((organization_id <> '00000000-0000-0000-0000-000000000000'::uuid)) ); COMMENT ON TABLE custom_roles IS 'Custom roles allow dynamic roles expanded at runtime'; @@ -3331,7 +3332,7 @@ CREATE INDEX idx_connection_logs_workspace_owner_id ON connection_logs USING btr CREATE INDEX idx_custom_roles_id ON custom_roles USING btree (id); -CREATE UNIQUE INDEX idx_custom_roles_name_lower ON custom_roles USING btree (lower(name)); +CREATE UNIQUE INDEX idx_custom_roles_name_lower_organization_id ON custom_roles USING btree (lower(name), COALESCE(organization_id, '00000000-0000-0000-0000-000000000000'::uuid)); CREATE INDEX idx_inbox_notifications_user_id_read_at ON inbox_notifications USING btree (user_id, read_at); diff --git a/coderd/database/migrations/000404_allow_same_role_name_in_different_orgs.down.sql b/coderd/database/migrations/000404_allow_same_role_name_in_different_orgs.down.sql new file mode 100644 index 0000000000..0e1584255a --- /dev/null +++ b/coderd/database/migrations/000404_allow_same_role_name_in_different_orgs.down.sql @@ -0,0 +1,6 @@ +-- Restore the original unique constraint (name only, no organization_id). +DROP INDEX IF EXISTS idx_custom_roles_name_lower_organization_id; + +ALTER TABLE custom_roles DROP CONSTRAINT IF EXISTS organization_id_not_zero; + +CREATE UNIQUE INDEX idx_custom_roles_name_lower ON custom_roles USING btree (LOWER(name)); diff --git a/coderd/database/migrations/000404_allow_same_role_name_in_different_orgs.up.sql b/coderd/database/migrations/000404_allow_same_role_name_in_different_orgs.up.sql new file mode 100644 index 0000000000..59f777adc5 --- /dev/null +++ b/coderd/database/migrations/000404_allow_same_role_name_in_different_orgs.up.sql @@ -0,0 +1,28 @@ +-- Fix the unique index in `custom_roles` to allow the same role name +-- in different organizations. The original index only covered name, +-- but names don't have to be unique across different organizations. +-- +-- Note: after fixing it, we end up with an almost-replica of the +-- existing `custom_roles_unique_key` constraint. That's unfortunate, +-- but since we can't define a constraint on an expression (e.g. lower()), +-- we'll have to keep both of them. +DROP INDEX IF EXISTS idx_custom_roles_name_lower; + +-- Use `COALESCE` to handle `NULL` organization_id. Site-wide custom +-- roles are currently not used, but that can change in the future and +-- this will become necessary. And there are no performance implications. +-- +-- Note: Using `NULLS NOT DISTINCT` instead of `COALESCE` here would +-- limit us to PG15+. + +-- Paranoia check. +UPDATE custom_roles SET organization_id = NULL WHERE organization_id = '00000000-0000-0000-0000-000000000000'; + +ALTER TABLE custom_roles + ADD CONSTRAINT organization_id_not_zero + CHECK (organization_id <> '00000000-0000-0000-0000-000000000000'::uuid); + +CREATE UNIQUE INDEX idx_custom_roles_name_lower_organization_id ON custom_roles USING btree ( + LOWER(name), + COALESCE(organization_id, '00000000-0000-0000-0000-000000000000'::uuid) +); diff --git a/coderd/database/migrations/testdata/fixtures/000403_pre_allow_same_role_name_in_different_orgs.up.sql b/coderd/database/migrations/testdata/fixtures/000403_pre_allow_same_role_name_in_different_orgs.up.sql new file mode 100644 index 0000000000..3eb29727c1 --- /dev/null +++ b/coderd/database/migrations/testdata/fixtures/000403_pre_allow_same_role_name_in_different_orgs.up.sql @@ -0,0 +1,10 @@ +-- Fixture for migration 000404_allow_same_role_name_in_different_orgs. +-- Inserts a custom role with an all-zero organization_id to ensure the +-- migration correctly normalizes such values. +INSERT INTO custom_roles (name, display_name, organization_id) +VALUES ( + 'custom-role-zero-org-id', + 'Custom Role (Zero Org ID)', + '00000000-0000-0000-0000-000000000000'::uuid +) +ON CONFLICT DO NOTHING; diff --git a/coderd/database/unique_constraint.go b/coderd/database/unique_constraint.go index b804d9a730..94fe9cc239 100644 --- a/coderd/database/unique_constraint.go +++ b/coderd/database/unique_constraint.go @@ -112,7 +112,7 @@ const ( UniqueWorkspacesPkey UniqueConstraint = "workspaces_pkey" // ALTER TABLE ONLY workspaces ADD CONSTRAINT workspaces_pkey PRIMARY KEY (id); UniqueIndexAPIKeyName UniqueConstraint = "idx_api_key_name" // CREATE UNIQUE INDEX idx_api_key_name ON api_keys USING btree (user_id, token_name) WHERE (login_type = 'token'::login_type); UniqueIndexConnectionLogsConnectionIDWorkspaceIDAgentName UniqueConstraint = "idx_connection_logs_connection_id_workspace_id_agent_name" // CREATE UNIQUE INDEX idx_connection_logs_connection_id_workspace_id_agent_name ON connection_logs USING btree (connection_id, workspace_id, agent_name); - UniqueIndexCustomRolesNameLower UniqueConstraint = "idx_custom_roles_name_lower" // CREATE UNIQUE INDEX idx_custom_roles_name_lower ON custom_roles USING btree (lower(name)); + UniqueIndexCustomRolesNameLowerOrganizationID UniqueConstraint = "idx_custom_roles_name_lower_organization_id" // CREATE UNIQUE INDEX idx_custom_roles_name_lower_organization_id ON custom_roles USING btree (lower(name), COALESCE(organization_id, '00000000-0000-0000-0000-000000000000'::uuid)); UniqueIndexOrganizationNameLower UniqueConstraint = "idx_organization_name_lower" // CREATE UNIQUE INDEX idx_organization_name_lower ON organizations USING btree (lower(name)) WHERE (deleted = false); UniqueIndexProvisionerDaemonsOrgNameOwnerKey UniqueConstraint = "idx_provisioner_daemons_org_name_owner_key" // CREATE UNIQUE INDEX idx_provisioner_daemons_org_name_owner_key ON provisioner_daemons USING btree (organization_id, name, lower(COALESCE((tags ->> 'owner'::text), ''::text))); UniqueIndexTemplateVersionPresetsDefault UniqueConstraint = "idx_template_version_presets_default" // CREATE UNIQUE INDEX idx_template_version_presets_default ON template_version_presets USING btree (template_version_id) WHERE (is_default = true);