From 8cb88cabee2a8ecbbbcb0a9acf62b4a177e85f47 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A9lix=20Malfait?= Date: Wed, 27 May 2026 10:54:02 +0200 Subject: [PATCH] fix(role): rebind API keys + agents before deleting their role (#20935) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Customer-reported bug A customer hit this when using the AI chat: ```json { "message": "API key 760d4822-da40-4b3f-9031-40563d7ed6c9 has no role assigned", "extensions": { "code": "INTERNAL_SERVER_ERROR", "userFriendlyMessage": "This API key has no role assigned." } } ``` Their integration authenticates via API key. Somewhere along the way, the role bound to that API key was deleted, leaving the API key authenticated but role-less. Any request that hits a permission check (`getRoleIdForApiKeyId`) blows up. ## Root cause In `RoleService.deleteManyRoles`, the pre-deletion cleanup (`assignDefaultRoleToMembersWithRoleToDelete`) only rebinds **user workspaces** to the workspace default role. API keys and agents pointing at the role are ignored. Because `RoleTargetEntity.role` declares `onDelete: 'CASCADE'`, the FK then drops the role_target rows for those API keys / agents — but the API keys themselves stay in `api_key`, now orphaned in `apiKeyRoleMap`. A previous read-side workaround ([2767ddac44](https://github.com/twentyhq/twenty/commit/2767ddac44) — make the `role` ResolveField nullable) handled the API-key-details page, but did not address the write paths (`getRoleIdForApiKeyId`). ## Fix - Rename `assignDefaultRoleToMembersWithRoleToDelete` → `rebindTargetsOfRoleToDeleteToDefaultRole` and extend it to rebind API keys (via `ApiKeyRoleService.assignRoleToApiKey`) and agents (via `AiAgentRoleService.assignRoleToAgent`) in the same step, before the role is deleted. - If the workspace default role doesn't satisfy `canBeAssignedToApiKeys` / `canBeAssignedToAgents`, the inner `assignRoleTo*` validation throws. We catch that and rethrow as a `PermissionsException` with a role-deletion-context message and two new codes — `ROLE_CANNOT_BE_ASSIGNED_TO_API_KEYS` / `ROLE_CANNOT_BE_ASSIGNED_TO_AGENTS` — so the admin sees a clear "reassign these first" prompt rather than a confusing inner error. ## Scope / non-goals - **Already-orphaned API keys are not auto-healed.** The customer still needs to reassign a role to their existing orphan API key via the UI (Settings > API Keys > [the key] > role). A separate cleanup command for existing orphans is a follow-up. - I did not investigate *why* the customer's session was authenticated via API key in the AI chat — that may be their integration setup. Worth confirming with them separately. ## Test plan - [ ] Workspace with default role `Admin` (which has `canBeAssignedToApiKeys: true`): create an API key with a custom role, delete the custom role → API key is rebound to Admin, requests keep working. - [ ] Workspace with default role `Member` (default, has `canBeAssignedToApiKeys: false`): create an API key with a custom role, delete the custom role → role deletion fails with the new `ROLE_CANNOT_BE_ASSIGNED_TO_API_KEYS` error explaining the admin must reassign first. API key + custom role are both unchanged. - [ ] Same two scenarios for agents (`canBeAssignedToAgents`). - [ ] Existing user-workspace rebind behavior is unchanged. - [ ] Role deletion with no dependent API keys / agents still works. --- ...ngsDeleteRoleConfirmationModalSubtitle.tsx | 47 ++++++++- .../permissions/permissions.exception.ts | 6 ++ ...sion-graphql-api-exception-handler.util.ts | 2 + ...-api-exception-code-to-http-status.util.ts | 2 + .../metadata-modules/role/role.service.ts | 96 ++++++++++++++++++- ...cessful-agent-creation.integration-spec.ts | 10 +- 6 files changed, 158 insertions(+), 5 deletions(-) diff --git a/packages/twenty-front/src/modules/settings/roles/role-settings/components/SettingsRoleSettingsDeleteRoleConfirmationModalSubtitle.tsx b/packages/twenty-front/src/modules/settings/roles/role-settings/components/SettingsRoleSettingsDeleteRoleConfirmationModalSubtitle.tsx index ffa1b37fc0..2b07bd5fa4 100644 --- a/packages/twenty-front/src/modules/settings/roles/role-settings/components/SettingsRoleSettingsDeleteRoleConfirmationModalSubtitle.tsx +++ b/packages/twenty-front/src/modules/settings/roles/role-settings/components/SettingsRoleSettingsDeleteRoleConfirmationModalSubtitle.tsx @@ -1,6 +1,6 @@ import { settingsDraftRoleFamilyState } from '@/settings/roles/states/settingsDraftRoleFamilyState'; import { useAtomFamilyStateValue } from '@/ui/utilities/state/jotai/hooks/useAtomFamilyStateValue'; -import { t } from '@lingui/core/macro'; +import { plural, t } from '@lingui/core/macro'; type SettingsRoleSettingsDeleteRoleConfirmationModalSubtitleProps = { roleId: string; @@ -14,8 +14,51 @@ export const SettingsRoleSettingsDeleteRoleConfirmationModalSubtitle = ({ roleId, ); const roleName = settingsDraftRole.label; + const memberCount = settingsDraftRole.workspaceMembers.length; + const apiKeyCount = settingsDraftRole.apiKeys.length; + const agentCount = settingsDraftRole.agents.length; + + const segments: string[] = []; + + if (memberCount > 0) { + segments.push( + plural(memberCount, { + one: `${memberCount} member`, + other: `${memberCount} members`, + }), + ); + } + + if (apiKeyCount > 0) { + segments.push( + plural(apiKeyCount, { + one: `${apiKeyCount} API key`, + other: `${apiKeyCount} API keys`, + }), + ); + } + + if (agentCount > 0) { + segments.push( + plural(agentCount, { + one: `${agentCount} agent`, + other: `${agentCount} agents`, + }), + ); + } + + if (segments.length === 0) { + return ( + <>{t`Confirm deletion of ${roleName} role? This cannot be undone.`} + ); + } + + const reassignSubject = + segments.length === 1 + ? segments[0] + : `${segments.slice(0, -1).join(', ')} ${t`and`} ${segments.at(-1)}`; return ( - <>{t`Confirm deletion of ${roleName} role? This cannot be undone. All members will be reassigned to the default role.`} + <>{t`Confirm deletion of ${roleName} role? This cannot be undone. ${reassignSubject} will be reassigned to the default role.`} ); }; diff --git a/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.exception.ts b/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.exception.ts index ec1a3c3693..6f4fd477a1 100644 --- a/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.exception.ts +++ b/packages/twenty-server/src/engine/metadata-modules/permissions/permissions.exception.ts @@ -48,6 +48,8 @@ export enum PermissionsExceptionCode { COMPOSITE_TYPE_NOT_FOUND = 'COMPOSITE_TYPE_NOT_FOUND', ROLE_MUST_HAVE_AT_LEAST_ONE_TARGET = 'ROLE_MUST_HAVE_AT_LEAST_ONE_TARGET', ROLE_CANNOT_BE_ASSIGNED_TO_USERS = 'ROLE_CANNOT_BE_ASSIGNED_TO_USERS', + ROLE_CANNOT_BE_ASSIGNED_TO_API_KEYS = 'ROLE_CANNOT_BE_ASSIGNED_TO_API_KEYS', + ROLE_CANNOT_BE_ASSIGNED_TO_AGENTS = 'ROLE_CANNOT_BE_ASSIGNED_TO_AGENTS', APPLICATION_ROLE_NOT_FOUND = 'APPLICATION_ROLE_NOT_FOUND', ROLE_BELONGS_TO_ANOTHER_APPLICATION = 'ROLE_BELONGS_TO_ANOTHER_APPLICATION', } @@ -142,6 +144,10 @@ const getPermissionsExceptionUserFriendlyMessage = ( return msg`Role must have at least one target.`; case PermissionsExceptionCode.ROLE_CANNOT_BE_ASSIGNED_TO_USERS: return msg`This role cannot be assigned to users.`; + case PermissionsExceptionCode.ROLE_CANNOT_BE_ASSIGNED_TO_API_KEYS: + return msg`This role cannot be assigned to API keys.`; + case PermissionsExceptionCode.ROLE_CANNOT_BE_ASSIGNED_TO_AGENTS: + return msg`This role cannot be assigned to agents.`; case PermissionsExceptionCode.APPLICATION_ROLE_NOT_FOUND: return msg`No role assigned to the application.`; case PermissionsExceptionCode.ROLE_BELONGS_TO_ANOTHER_APPLICATION: diff --git a/packages/twenty-server/src/engine/metadata-modules/permissions/utils/permission-graphql-api-exception-handler.util.ts b/packages/twenty-server/src/engine/metadata-modules/permissions/utils/permission-graphql-api-exception-handler.util.ts index e03148fbe7..a53069f69a 100644 --- a/packages/twenty-server/src/engine/metadata-modules/permissions/utils/permission-graphql-api-exception-handler.util.ts +++ b/packages/twenty-server/src/engine/metadata-modules/permissions/utils/permission-graphql-api-exception-handler.util.ts @@ -44,6 +44,8 @@ export const permissionGraphqlApiExceptionHandler = ( case PermissionsExceptionCode.EMPTY_FIELD_PERMISSION_NOT_ALLOWED: case PermissionsExceptionCode.ROLE_MUST_HAVE_AT_LEAST_ONE_TARGET: case PermissionsExceptionCode.ROLE_CANNOT_BE_ASSIGNED_TO_USERS: + case PermissionsExceptionCode.ROLE_CANNOT_BE_ASSIGNED_TO_API_KEYS: + case PermissionsExceptionCode.ROLE_CANNOT_BE_ASSIGNED_TO_AGENTS: throw new UserInputError(error); case PermissionsExceptionCode.ROLE_NOT_FOUND: case PermissionsExceptionCode.OBJECT_METADATA_NOT_FOUND: diff --git a/packages/twenty-server/src/engine/metadata-modules/permissions/utils/permission-rest-api-exception-code-to-http-status.util.ts b/packages/twenty-server/src/engine/metadata-modules/permissions/utils/permission-rest-api-exception-code-to-http-status.util.ts index 8f21a3a0a5..b58728a73d 100644 --- a/packages/twenty-server/src/engine/metadata-modules/permissions/utils/permission-rest-api-exception-code-to-http-status.util.ts +++ b/packages/twenty-server/src/engine/metadata-modules/permissions/utils/permission-rest-api-exception-code-to-http-status.util.ts @@ -26,6 +26,8 @@ export const permissionRestApiExceptionCodeToHttpStatus = ( case PermissionsExceptionCode.EMPTY_FIELD_PERMISSION_NOT_ALLOWED: case PermissionsExceptionCode.ROLE_MUST_HAVE_AT_LEAST_ONE_TARGET: case PermissionsExceptionCode.ROLE_CANNOT_BE_ASSIGNED_TO_USERS: + case PermissionsExceptionCode.ROLE_CANNOT_BE_ASSIGNED_TO_API_KEYS: + case PermissionsExceptionCode.ROLE_CANNOT_BE_ASSIGNED_TO_AGENTS: return 400; case PermissionsExceptionCode.ROLE_NOT_FOUND: case PermissionsExceptionCode.OBJECT_METADATA_NOT_FOUND: diff --git a/packages/twenty-server/src/engine/metadata-modules/role/role.service.ts b/packages/twenty-server/src/engine/metadata-modules/role/role.service.ts index 53f045ab21..e86a2481d0 100644 --- a/packages/twenty-server/src/engine/metadata-modules/role/role.service.ts +++ b/packages/twenty-server/src/engine/metadata-modules/role/role.service.ts @@ -5,9 +5,19 @@ import { msg } from '@lingui/core/macro'; import { isDefined } from 'twenty-shared/utils'; import { Repository } from 'typeorm'; +import { + ApiKeyException, + ApiKeyExceptionCode, +} from 'src/engine/core-modules/api-key/exceptions/api-key.exception'; +import { ApiKeyRoleService } from 'src/engine/core-modules/api-key/services/api-key-role.service'; import { ApplicationService } from 'src/engine/core-modules/application/application.service'; import { type FlatApplication } from 'src/engine/core-modules/application/types/flat-application.type'; import { WorkspaceEntity } from 'src/engine/core-modules/workspace/workspace.entity'; +import { AiAgentRoleService } from 'src/engine/metadata-modules/ai/ai-agent-role/ai-agent-role.service'; +import { + AiException, + AiExceptionCode, +} from 'src/engine/metadata-modules/ai/ai.exception'; import { WorkspaceManyOrAllFlatEntityMapsCacheService } from 'src/engine/metadata-modules/flat-entity/services/workspace-many-or-all-flat-entity-maps-cache.service'; import { findFlatEntityByIdInFlatEntityMapsOrThrow } from 'src/engine/metadata-modules/flat-entity/utils/find-flat-entity-by-id-in-flat-entity-maps-or-throw.util'; import { findFlatEntityByUniversalIdentifier } from 'src/engine/metadata-modules/flat-entity/utils/find-flat-entity-by-universal-identifier.util'; @@ -40,6 +50,8 @@ export class RoleService { private readonly roleRepository: Repository, private readonly userRoleService: UserRoleService, private readonly applicationService: ApplicationService, + private readonly apiKeyRoleService: ApiKeyRoleService, + private readonly aiAgentRoleService: AiAgentRoleService, ) {} public async getWorkspaceRoles(workspaceId: string): Promise { @@ -316,8 +328,9 @@ export class RoleService { ); } - await this.assignDefaultRoleToMembersWithRoleToDelete({ + await this.rebindTargetsOfRoleToDeleteToDefaultRole({ roleId, + roleLabel: flatRoleToDelete.label, workspaceId, defaultRoleId, }); @@ -407,12 +420,14 @@ export class RoleService { } // TODO: Move to migration side effect / To address for rollback of role deletion - private async assignDefaultRoleToMembersWithRoleToDelete({ + private async rebindTargetsOfRoleToDeleteToDefaultRole({ roleId, + roleLabel, workspaceId, defaultRoleId, }: { roleId: string; + roleLabel: string; workspaceId: string; defaultRoleId: string; }): Promise { @@ -427,5 +442,82 @@ export class RoleService { roleId: defaultRoleId, workspaceId, }); + + const apiKeysToRebind = + await this.apiKeyRoleService.getApiKeysAssignedToRole( + roleId, + workspaceId, + ); + + for (const apiKey of apiKeysToRebind) { + try { + await this.apiKeyRoleService.assignRoleToApiKey({ + apiKeyId: apiKey.id, + roleId: defaultRoleId, + workspaceId, + }); + } catch (error) { + if ( + error instanceof ApiKeyException && + error.code === ApiKeyExceptionCode.ROLE_CANNOT_BE_ASSIGNED_TO_API_KEYS + ) { + throw this.toRoleDeleteRebindException({ + roleLabel, + targetKind: 'apiKey', + }); + } + throw error; + } + } + + const agentsToRebind = + await this.aiAgentRoleService.getAgentsAssignedToRole( + roleId, + workspaceId, + ); + + for (const agent of agentsToRebind) { + try { + await this.aiAgentRoleService.assignRoleToAgent({ + agentId: agent.id, + roleId: defaultRoleId, + workspaceId, + }); + } catch (error) { + if ( + error instanceof AiException && + error.code === AiExceptionCode.ROLE_CANNOT_BE_ASSIGNED_TO_AGENTS + ) { + throw this.toRoleDeleteRebindException({ + roleLabel, + targetKind: 'agent', + }); + } + throw error; + } + } + } + + private toRoleDeleteRebindException({ + roleLabel, + targetKind, + }: { + roleLabel: string; + targetKind: 'apiKey' | 'agent'; + }): Error { + const targetLabel = targetKind === 'apiKey' ? 'API key' : 'agent'; + + return new PermissionsException( + `Cannot delete role "${roleLabel}": the workspace default role cannot be assigned to ${targetLabel}s.`, + targetKind === 'apiKey' + ? PermissionsExceptionCode.ROLE_CANNOT_BE_ASSIGNED_TO_API_KEYS + : PermissionsExceptionCode.ROLE_CANNOT_BE_ASSIGNED_TO_AGENTS, + { + userFriendlyMessage: + targetKind === 'apiKey' + ? msg`Cannot delete this role: it is still assigned to one or more API keys, and the workspace default role cannot be assigned to API keys. Please reassign these API keys to another role first.` + : msg`Cannot delete this role: it is still assigned to one or more agents, and the workspace default role cannot be assigned to agents. Please reassign these agents to another role first.`, + }, + ); } } diff --git a/packages/twenty-server/test/integration/metadata/suites/agent/successful-agent-creation.integration-spec.ts b/packages/twenty-server/test/integration/metadata/suites/agent/successful-agent-creation.integration-spec.ts index 2487634bd2..16e4e234d2 100644 --- a/packages/twenty-server/test/integration/metadata/suites/agent/successful-agent-creation.integration-spec.ts +++ b/packages/twenty-server/test/integration/metadata/suites/agent/successful-agent-creation.integration-spec.ts @@ -208,7 +208,15 @@ describe('Agent creation should succeed', () => { isCustom: true, }); - // Clean up the role + // Delete the agent first so its role_target is removed; otherwise + // deleting the role would refuse to orphan the agent (the workspace + // default role isn't agent-assignable). + await deleteOneAgent({ + expectToFail: false, + input: { id: createdAgentId }, + }); + createdAgentId = ''; + await deleteOneRole({ expectToFail: false, input: { idToDelete: createdRoleId },