[fix] [permissions] Fix role assignment at sign-up (#11045)

In [a previous PR](https://github.com/twentyhq/twenty/pull/11023) I
fixed the issue where users where re-assigned the default role when
signin up using SSO.
The "fix" actually introduced another error, calling
`assignRoleToUserWorkspace` only when a user is signing up to twenty,
forgetting about the case where an existing twenty user signs up to a
different workspace. They should still be assigned the role in that
case.

To fix this and improve clarity, I moved assignRoleToUserWorkspace to
addUserToWorkspace and renamed addUserToWorkspace to
addUserToWorkspaceIfUserNotInWorkspace since this is at each sign in but
does nothing if user is already in the workspace.

I think ideally we should refactor this part to improve readability and
understandability, maybe with different flows for each case: signIn and
signUp, to twenty or to a workspace

packages/twenty-server/src/engine/core-modules/auth/services/auth.service.spec.ts

@@ -9,8 +9,8 @@ import {
   AuthException,
   AuthExceptionCode,
 } from 'src/engine/core-modules/auth/auth.exception';
-import { SignInUpService } from 'src/engine/core-modules/auth/services/sign-in-up.service';
 import { AuthSsoService } from 'src/engine/core-modules/auth/services/auth-sso.service';
+import { SignInUpService } from 'src/engine/core-modules/auth/services/sign-in-up.service';
 import { AccessTokenService } from 'src/engine/core-modules/auth/token/services/access-token.service';
 import { RefreshTokenService } from 'src/engine/core-modules/auth/token/services/refresh-token.service';
 import { ExistingUserOrNewUser } from 'src/engine/core-modules/auth/types/signInUp.type';
@@ -101,7 +101,8 @@ describe('AuthService', () => {
           useValue: {
             checkUserWorkspaceExists:
               userWorkspaceServiceCheckUserWorkspaceExistsMock,
-            addUserToWorkspace: userWorkspaceAddUserToWorkspaceMock,
+            addUserToWorkspaceIfUserNotInWorkspace:
+              userWorkspaceAddUserToWorkspaceMock,
           },
         },
         {

packages/twenty-server/src/engine/core-modules/auth/services/auth.service.ts

@@ -37,8 +37,8 @@ import {
   UserNotExists,
 } from 'src/engine/core-modules/auth/dto/user-exists.entity';
 import { WorkspaceInviteHashValid } from 'src/engine/core-modules/auth/dto/workspace-invite-hash-valid.entity';
-import { SignInUpService } from 'src/engine/core-modules/auth/services/sign-in-up.service';
 import { AuthSsoService } from 'src/engine/core-modules/auth/services/auth-sso.service';
+import { SignInUpService } from 'src/engine/core-modules/auth/services/sign-in-up.service';
 import { AccessTokenService } from 'src/engine/core-modules/auth/token/services/access-token.service';
 import { RefreshTokenService } from 'src/engine/core-modules/auth/token/services/refresh-token.service';
 import {
@@ -106,7 +106,10 @@ export class AuthService {
         workspacePersonalInviteToken: invitation.value,
         email: user.email,
       });
-      await this.userWorkspaceService.addUserToWorkspace(user, workspace);
+      await this.userWorkspaceService.addUserToWorkspaceIfUserNotInWorkspace(
+        user,
+        workspace,
+      );
 
       return;
     }

packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.spec.ts

@@ -44,8 +44,6 @@ describe('SignInUpService', () => {
   let userWorkspaceService: UserWorkspaceService;
   let environmentService: EnvironmentService;
   let domainManagerService: DomainManagerService;
-  let userRoleService: UserRoleService;
-  let featureFlagService: FeatureFlagService;
 
   beforeEach(async () => {
     const module: TestingModule = await Test.createTestingModule({
@@ -83,7 +81,7 @@ describe('SignInUpService', () => {
         {
           provide: UserWorkspaceService,
           useValue: {
-            addUserToWorkspace: jest.fn(),
+            addUserToWorkspaceIfUserNotInWorkspace: jest.fn(),
             checkUserWorkspaceExists: jest.fn(),
             create: jest.fn(),
           },
@@ -148,8 +146,6 @@ describe('SignInUpService', () => {
     environmentService = module.get<EnvironmentService>(EnvironmentService);
     domainManagerService =
       module.get<DomainManagerService>(DomainManagerService);
-    userRoleService = module.get<UserRoleService>(UserRoleService);
-    featureFlagService = module.get<FeatureFlagService>(FeatureFlagService);
   });
 
   it('should handle signInUp with valid personal invitation', async () => {
@@ -179,10 +175,9 @@ describe('SignInUpService', () => {
       .spyOn(workspaceInvitationService, 'invalidateWorkspaceInvitation')
       .mockResolvedValue(undefined);
 
-    jest.spyOn(userWorkspaceService, 'addUserToWorkspace').mockResolvedValue({
+    jest
-      user: {} as User,
+      .spyOn(userWorkspaceService, 'addUserToWorkspaceIfUserNotInWorkspace')
-      userWorkspace: {} as UserWorkspace,
+      .mockResolvedValue(undefined);
-    });
 
     const result = await service.signInUp(params);
 
@@ -200,6 +195,9 @@ describe('SignInUpService', () => {
       (params.workspace as Workspace).id,
       'test@example.com',
     );
+    expect(
+      userWorkspaceService.addUserToWorkspaceIfUserNotInWorkspace,
+    ).toHaveBeenCalled();
   });
 
   it('should handle signInUp on existing workspace without invitation', async () => {
@@ -217,16 +215,17 @@ describe('SignInUpService', () => {
       },
     };
 
-    jest.spyOn(userWorkspaceService, 'addUserToWorkspace').mockResolvedValue({
+    jest
-      user: {} as User,
+      .spyOn(userWorkspaceService, 'addUserToWorkspaceIfUserNotInWorkspace')
-      userWorkspace: {} as UserWorkspace,
+      .mockResolvedValue(undefined);
-    });
 
     const result = await service.signInUp(params);
 
     expect(result.workspace).toEqual(params.workspace);
     expect(result.user).toBeDefined();
-    expect(userWorkspaceService.addUserToWorkspace).toHaveBeenCalled();
+    expect(
+      userWorkspaceService.addUserToWorkspaceIfUserNotInWorkspace,
+    ).toHaveBeenCalled();
   });
 
   it('should handle signUp on new workspace for a new user', async () => {
@@ -294,10 +293,9 @@ describe('SignInUpService', () => {
     };
 
     jest.spyOn(environmentService, 'get').mockReturnValue(false);
-    jest.spyOn(userWorkspaceService, 'addUserToWorkspace').mockResolvedValue({
+    jest
-      user: {} as User,
+      .spyOn(userWorkspaceService, 'addUserToWorkspaceIfUserNotInWorkspace')
-      userWorkspace: {} as UserWorkspace,
+      .mockResolvedValue(undefined);
-    });
     jest
       .spyOn(userWorkspaceService, 'checkUserWorkspaceExists')
       .mockResolvedValue({} as UserWorkspace);
@@ -306,7 +304,9 @@ describe('SignInUpService', () => {
 
     expect(result.workspace).toEqual(params.workspace);
     expect(result.user).toBeDefined();
-    expect(userWorkspaceService.addUserToWorkspace).toHaveBeenCalled();
+    expect(
+      userWorkspaceService.addUserToWorkspaceIfUserNotInWorkspace,
+    ).toHaveBeenCalled();
   });
 
   it('should throw - handle signUp on workspace in pending state', async () => {
@@ -365,81 +365,4 @@ describe('SignInUpService', () => {
     expect(WorkspaceRepository.create).toHaveBeenCalled();
     expect(WorkspaceRepository.save).toHaveBeenCalled();
   });
-
-  it('should not assign default role when permissions are enabled and user exists', async () => {
-    const params: SignInUpBaseParams &
-      ExistingUserOrPartialUserWithPicture &
-      AuthProviderWithPasswordType = {
-      workspace: {
-        id: 'workspaceId',
-        defaultRoleId: 'defaultRoleId',
-        activationStatus: WorkspaceActivationStatus.ACTIVE,
-      } as Workspace,
-      authParams: { provider: 'password', password: 'validPassword' },
-      userData: {
-        type: 'existingUser',
-        existingUser: { email: 'test@example.com' } as User,
-      },
-    };
-
-    const mockUserWorkspace = { id: 'userWorkspaceId' };
-
-    jest.spyOn(featureFlagService, 'isFeatureEnabled').mockResolvedValue(true);
-    jest.spyOn(userWorkspaceService, 'addUserToWorkspace').mockResolvedValue({
-      user: {} as User,
-      userWorkspace: mockUserWorkspace as UserWorkspace,
-    });
-
-    await service.signInUp(params);
-
-    expect(params.workspace).toBeDefined();
-    expect(userRoleService.assignRoleToUserWorkspace).not.toHaveBeenCalled();
-  });
-
-  it('should assign default role when permissions are enabled and user does not exist', async () => {
-    const params: SignInUpBaseParams &
-      ExistingUserOrPartialUserWithPicture &
-      AuthProviderWithPasswordType = {
-      workspace: {
-        id: 'workspaceId',
-        defaultRoleId: 'defaultRoleId',
-        activationStatus: WorkspaceActivationStatus.ACTIVE,
-      } as Workspace,
-      authParams: { provider: 'password', password: 'validPassword' },
-      userData: {
-        type: 'newUserWithPicture',
-        newUserWithPicture: {
-          email: 'newuser@example.com',
-          picture: 'pictureUrl',
-        },
-      },
-    };
-
-    const mockUserWorkspace = { id: 'userWorkspaceId' };
-
-    jest.spyOn(featureFlagService, 'isFeatureEnabled').mockResolvedValue(true);
-
-    jest.spyOn(fileUploadService, 'uploadImage').mockResolvedValue({
-      id: '',
-      mimeType: '',
-      paths: ['path/to/image'],
-    });
-    jest.spyOn(UserRepository, 'create').mockReturnValue({} as User);
-    jest
-      .spyOn(UserRepository, 'save')
-      .mockResolvedValue({ id: 'newUserId' } as User);
-    jest.spyOn(userWorkspaceService, 'addUserToWorkspace').mockResolvedValue({
-      user: {} as User,
-      userWorkspace: mockUserWorkspace as UserWorkspace,
-    });
-
-    await service.signInUp(params);
-
-    expect(params.workspace).toBeDefined();
-    expect(userRoleService.assignRoleToUserWorkspace).toHaveBeenCalledWith({
-      workspaceId: params.workspace!.id,
-      userWorkspaceId: mockUserWorkspace.id,
-      roleId: params.workspace!.defaultRoleId,
-    });
-  });
 });

packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.ts

@@ -226,37 +226,45 @@ export class SignInUpService {
   ) {
     await this.throwIfWorkspaceIsNotReadyForSignInUp(params.workspace, params);
 
-    const currentUser =
+    const isNewUser = params.userData.type === 'newUserWithPicture';
-      params.userData.type === 'newUserWithPicture'
-        ? await this.saveNewUser(
-            params.userData.newUserWithPicture,
-            params.workspace.id,
-            { canAccessFullAdminPanel: false, canImpersonate: false },
-          )
-        : params.userData.existingUser;
 
-    const { user: updatedUser, userWorkspace } =
+    if (isNewUser) {
-      await this.userWorkspaceService.addUserToWorkspace(
+      const userData = params.userData as {
-        currentUser,
+        type: 'newUserWithPicture';
+        newUserWithPicture: PartialUserWithPicture;
+      };
+
+      const user = await this.saveNewUser(
+        userData.newUserWithPicture,
+        params.workspace.id,
+        {
+          canAccessFullAdminPanel: false,
+          canImpersonate: false,
+        },
+      );
+
+      await this.activateOnboardingForUser(user, params.workspace);
+
+      await this.userWorkspaceService.addUserToWorkspaceIfUserNotInWorkspace(
+        user,
         params.workspace,
       );
 
-    const user = Object.assign(currentUser, updatedUser);
+      return user;
-
-    const isSignUp = params.userData.type === 'newUserWithPicture';
-
-    if (isSignUp) {
-      await this.activateOnboardingForUser(user, params.workspace);
-
-      if (params.workspace.defaultRoleId) {
-        await this.userRoleService.assignRoleToUserWorkspace({
-          workspaceId: params.workspace.id,
-          userWorkspaceId: userWorkspace.id,
-          roleId: params.workspace.defaultRoleId,
-        });
-      }
     }
 
+    const userData = params.userData as {
+      type: 'existingUser';
+      existingUser: User;
+    };
+
+    const user = userData.existingUser;
+
+    await this.userWorkspaceService.addUserToWorkspaceIfUserNotInWorkspace(
+      user,
+      params.workspace,
+    );
+
     return user;
   }
 

packages/twenty-server/src/engine/core-modules/user-workspace/user-workspace.module.ts

@@ -4,6 +4,7 @@ import { NestjsQueryGraphQLModule } from '@ptc-org/nestjs-query-graphql';
 import { NestjsQueryTypeOrmModule } from '@ptc-org/nestjs-query-typeorm';
 
 import { TypeORMModule } from 'src/database/typeorm/typeorm.module';
+import { DomainManagerModule } from 'src/engine/core-modules/domain-manager/domain-manager.module';
 import { TwoFactorMethod } from 'src/engine/core-modules/two-factor-method/two-factor-method.entity';
 import { UserWorkspace } from 'src/engine/core-modules/user-workspace/user-workspace.entity';
 import { UserWorkspaceResolver } from 'src/engine/core-modules/user-workspace/user-workspace.resolver';
@@ -13,9 +14,9 @@ import { WorkspaceInvitationModule } from 'src/engine/core-modules/workspace-inv
 import { Workspace } from 'src/engine/core-modules/workspace/workspace.entity';
 import { DataSourceModule } from 'src/engine/metadata-modules/data-source/data-source.module';
 import { ObjectMetadataEntity } from 'src/engine/metadata-modules/object-metadata/object-metadata.entity';
+import { UserRoleModule } from 'src/engine/metadata-modules/user-role/user-role.module';
 import { TwentyORMModule } from 'src/engine/twenty-orm/twenty-orm.module';
 import { WorkspaceDataSourceModule } from 'src/engine/workspace-datasource/workspace-datasource.module';
-import { DomainManagerModule } from 'src/engine/core-modules/domain-manager/domain-manager.module';
 
 @Module({
   imports: [
@@ -32,6 +33,7 @@ import { DomainManagerModule } from 'src/engine/core-modules/domain-manager/doma
         WorkspaceInvitationModule,
         DomainManagerModule,
         TwentyORMModule,
+        UserRoleModule,
       ],
       services: [UserWorkspaceService],
     }),

packages/twenty-server/src/engine/core-modules/user-workspace/user-workspace.service.ts

@@ -21,6 +21,12 @@ import { WorkspaceInvitationService } from 'src/engine/core-modules/workspace-in
 import { Workspace } from 'src/engine/core-modules/workspace/workspace.entity';
 import { DataSourceService } from 'src/engine/metadata-modules/data-source/data-source.service';
 import { ObjectMetadataEntity } from 'src/engine/metadata-modules/object-metadata/object-metadata.entity';
+import {
+  PermissionsException,
+  PermissionsExceptionCode,
+  PermissionsExceptionMessage,
+} from 'src/engine/metadata-modules/permissions/permissions.exception';
+import { UserRoleService } from 'src/engine/metadata-modules/user-role/user-role.service';
 import { TwentyORMGlobalManager } from 'src/engine/twenty-orm/twenty-orm-global.manager';
 import { WorkspaceEventEmitter } from 'src/engine/workspace-event-emitter/workspace-event-emitter';
 import { WorkspaceMemberWorkspaceEntity } from 'src/modules/workspace-member/standard-objects/workspace-member.workspace-entity';
@@ -30,6 +36,8 @@ export class UserWorkspaceService extends TypeOrmQueryService<UserWorkspace> {
   constructor(
     @InjectRepository(UserWorkspace, 'core')
     private readonly userWorkspaceRepository: Repository<UserWorkspace>,
+    @InjectRepository(Workspace, 'core')
+    private readonly workspaceRepository: Repository<Workspace>,
     @InjectRepository(User, 'core')
     private readonly userRepository: Repository<User>,
     @InjectRepository(ObjectMetadataEntity, 'metadata')
@@ -40,6 +48,7 @@ export class UserWorkspaceService extends TypeOrmQueryService<UserWorkspace> {
     private readonly workspaceEventEmitter: WorkspaceEventEmitter,
     private readonly domainManagerService: DomainManagerService,
     private readonly twentyORMGlobalManager: TwentyORMGlobalManager,
+    private readonly userRoleService: UserRoleService,
   ) {
     super(userWorkspaceRepository);
   }
@@ -112,7 +121,10 @@ export class UserWorkspaceService extends TypeOrmQueryService<UserWorkspace> {
     });
   }
 
-  async addUserToWorkspace(user: User, workspace: Workspace) {
+  async addUserToWorkspaceIfUserNotInWorkspace(
+    user: User,
+    workspace: Workspace,
+  ) {
     let userWorkspace = await this.checkUserWorkspaceExists(
       user.id,
       workspace.id,
@@ -122,17 +134,27 @@ export class UserWorkspaceService extends TypeOrmQueryService<UserWorkspace> {
       userWorkspace = await this.create(user.id, workspace.id);
 
       await this.createWorkspaceMember(workspace.id, user);
+
+      const defaultRoleId = workspace.defaultRoleId;
+
+      if (!isDefined(defaultRoleId)) {
+        throw new PermissionsException(
+          PermissionsExceptionMessage.DEFAULT_ROLE_NOT_FOUND,
+          PermissionsExceptionCode.DEFAULT_ROLE_NOT_FOUND,
+        );
+      }
+
+      await this.userRoleService.assignRoleToUserWorkspace({
+        workspaceId: workspace.id,
+        userWorkspaceId: userWorkspace.id,
+        roleId: defaultRoleId,
+      });
+
+      await this.workspaceInvitationService.invalidateWorkspaceInvitation(
+        workspace.id,
+        user.email,
+      );
     }
-
-    await this.workspaceInvitationService.invalidateWorkspaceInvitation(
-      workspace.id,
-      user.email,
-    );
-
-    return {
-      user,
-      userWorkspace,
-    };
   }
 
   public async getUserCount(workspaceId: string): Promise<number | undefined> {

packages/twenty-server/src/engine/metadata-modules/permissions/permissions.exception.ts

@@ -24,6 +24,7 @@ export enum PermissionsExceptionCode {
   INVALID_ARG = 'INVALID_ARG',
   PERMISSIONS_V2_NOT_ENABLED = 'PERMISSIONS_V2_NOT_ENABLED',
   ROLE_LABEL_ALREADY_EXISTS = 'ROLE_LABEL_ALREADY_EXISTS',
+  DEFAULT_ROLE_NOT_FOUND = 'DEFAULT_ROLE_NOT_FOUND',
 }
 
 export enum PermissionsExceptionMessage {
@@ -43,4 +44,5 @@ export enum PermissionsExceptionMessage {
   NO_ROLE_FOUND_FOR_USER_WORKSPACE = 'No role found for userWorkspace',
   PERMISSIONS_V2_NOT_ENABLED = 'Permissions V2 is not enabled',
   ROLE_LABEL_ALREADY_EXISTS = 'A role with this label already exists',
+  DEFAULT_ROLE_NOT_FOUND = 'Default role not found',
 }

packages/twenty-server/src/engine/metadata-modules/permissions/utils/permission-graphql-api-exception-handler.util.ts

@@ -25,6 +25,7 @@ export const permissionGraphqlApiExceptionHandler = (
     case PermissionsExceptionCode.ROLE_NOT_FOUND:
     case PermissionsExceptionCode.USER_WORKSPACE_NOT_FOUND:
       throw new NotFoundError(error.message);
+    case PermissionsExceptionCode.DEFAULT_ROLE_NOT_FOUND:
     default:
       throw new InternalServerError(error.message);
   }