refacto(*): remove everything about default workspace #9157

AMoreaux · 2024-12-19T17:23:13Z

Summary

Remove defaultWorkspace in user
Remove all occurrence of defaultWorkspace and defaultWorkspaceId
Improve activate workspace flow
Improve security on social login
Add ImpersonateGuard
Allow to use impersonation with couple User/Workspace
Prevent unexpected reload on activate workspace
Scope login token with workspaceId

Replaced `Verify` and `CurrentUserOutput` dto usages with direct use of `AuthTokens` and `User` entities, respectively. Simplified related GraphQL queries, mutations, and services to reflect the updated data model. Enhanced code readability and reduced redundancy by removing unnecessary intermediate representations.

…hing-about-defaultWorkspaceId # Conflicts: # packages/twenty-server/test/integration/metadata/suites/object-metadata/rename-custom-object.integration-spec.ts

Removed `ActivateWorkspaceOutput` in favor of directly returning `Workspace` for cleaner API responses. Consolidated workspace fetching logic into `getWorkspaceByOriginOrDefaultWorkspace` to reduce duplication and improve maintainability. Updated related queries, mutations, and frontend logic to reflect these changes.

github-actions · 2024-12-20T13:30:19Z

TODOs/FIXMEs:

// TODO: factorize with UserProviderEffect: packages/twenty-front/src/modules/auth/hooks/useAuth.ts
~~// TODO: factorize with UserProviderEffect~~: packages/twenty-front/src/modules/auth/hooks/useAuth.ts
~~// TODO AMOREAUX: this logger is trigger twice and the second time the message is undefined for an unknown reason~~: packages/twenty-server/src/engine/core-modules/domain-manager/service/domain-manager.service.ts

Generated by 🚫 dangerJS against 4400f5e

Unifies impersonation functionality and feature flag management under a single admin panel component. Replaces the standalone impersonation component and integrates updated GraphQL types and APIs, improving handling of login tokens and workspace redirections.

greptile-apps

PR Summary

This PR implements a significant architectural change by removing the concept of default workspaces and improving workspace activation flows. The changes span across both frontend and backend codebases.

Key changes:

Removes defaultWorkspaceId from User entity and all related references throughout the codebase
Introduces new ImpersonateGuard with workspace-specific impersonation requiring both userId and workspaceId
Replaces defaultWorkspace with currentWorkspace in user-related components and services
Adds domain manager service with improved workspace lookup and validation
Simplifies workspace activation flow by removing automatic workspace assignment and login token generation

Potential issues:

Migration's down function adds NOT NULL constraint without handling existing data
Some impersonation security checks may be incomplete (workspace-level permissions)
Removal of OAuthService needs careful testing of authentication flows

_{72 file(s) reviewed, 29 comment(s)}
_{Edit PR Review Bot Settings | Greptile}

packages/twenty-front/src/modules/app/components/SettingsRoutes.tsx

greptile-apps · 2024-12-20T15:29:42Z

packages/twenty-front/src/modules/auth/hooks/useAuth.ts

+    if (!user) {
+      throw new Error('No current user result');
+    }


style: this error should include more context about why the current user result is missing to help with debugging

greptile-apps · 2024-12-20T15:29:43Z

packages/twenty-front/src/modules/auth/hooks/useAuth.ts

+      if (isMultiWorkspaceEnabled) {
+        return redirectToWorkspaceDomain(
+          signUpResult.data.signUp.workspace.subdomain,
+          AppPath.Verify,
+          {
+            loginToken: signUpResult.data.signUp.loginToken.token,
+          },
+        );
+      }


logic: no error handling for failed workspace domain redirects - should catch and handle redirect failures

greptile-apps · 2024-12-20T15:30:10Z

packages/twenty-front/src/modules/users/components/UserProviderEffect.tsx

+    if (isDefined(queryData.currentUser.currentWorkspace)) {
+      setCurrentWorkspace(queryData.currentUser.currentWorkspace);
+    }


style: consider handling the case where currentWorkspace is undefined - may need to select first available workspace or show workspace selector

greptile-apps · 2024-12-20T15:30:38Z

packages/twenty-front/src/pages/onboarding/CreateWorkspace.tsx

        if (isDefined(result.errors)) {
          throw result.errors ?? new Error('Unknown error');
        }


logic: Error handling logic is incorrect - result.errors check should come before loadCurrentUser() call to prevent executing it when there are errors

greptile-apps · 2024-12-20T15:37:38Z

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

+    await this.workspaceRepository.update(workspace.id, {
      activationStatus: WorkspaceActivationStatus.ONGOING_CREATION,
    });


style: Consider wrapping workspace status updates and initialization in a transaction to ensure atomicity

greptile-apps · 2024-12-20T15:38:00Z

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

-      workspace,
-      loginToken,
-    };
+    workspaceValidator.assertIsDefinedOrThrow(workspace);


style: should pass a WorkspaceException like on line 194 to provide better error handling

greptile-apps · 2024-12-20T15:38:11Z

packages/twenty-server/src/engine/guards/impersonate-guard.ts

+    const request = ctx.getContext().req;
+
+    return request.user.canImpersonate === true;


logic: Missing null check on request.user could cause runtime error if authentication fails

greptile-apps · 2024-12-20T15:38:12Z

packages/twenty-server/src/engine/guards/impersonate-guard.ts

+    const ctx = GqlExecutionContext.create(context);
+    const request = ctx.getContext().req;
+
+    return request.user.canImpersonate === true;


logic: Should also check workspace.allowImpersonation before allowing impersonation

greptile-apps · 2024-12-20T15:38:16Z

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

+  exceptionToThrowCustom: AuthException = new AuthException(
+    `${provider} auth is not enabled for this workspace`,
+    AuthExceptionCode.OAUTH_ACCESS_DENIED,
+  ),


style: Default exception message includes template literal that could expose internal details. Consider using a static message instead.

Renamed `SettingsAdminFeatureFlags` to `SettingsAdminContent` for clarity and updated associated routes. Enhanced impersonation checks by adding a condition to ensure the workspace allows impersonation. This improves code readability and adds stricter validation for impersonation actions.

Moved SettingsAdminContent to a more appropriate module directory for improved project structure and maintainability. Updated all import paths to reflect the new location.

The guard no longer checks for `workspace.allowImpersonation` as it is unnecessary. Impersonation access is now solely determined by the user's permissions. This simplifies the logic and avoids potential misconfigurations.

Added allowImpersonation property to workspace entities in both front-end and back-end. This enables tracking of whether impersonation is allowed for each workspace in user lookup operations.

Enhance login token generation by including workspace ID, ensuring tokens are tied to specific workspaces. Updated `verifyLoginToken` to validate workspace ID, adding stricter access control. Adjusted all relevant controllers and services to support this feature.

Added support for passing the workspace ID when generating login tokens. This enhances token specificity and aligns with multi-workspace authentication requirements. Adjusted controller and token service to handle the additional parameter.

Removed the defaultWorkspaceId column and its associated constraints from database migrations. Additionally, added an import in workspace.entity.ts to address circular dependency issues.

Added an explanatory comment detailing the specific circular dependency path to improve code readability and maintenance. This change does not affect functionality but aims to assist future developers in understanding the issue

AMoreaux added 3 commits December 18, 2024 18:57

WIP

06c29f5

WIP

ecb0e69

AMoreaux self-assigned this Dec 19, 2024

AMoreaux added 3 commits December 19, 2024 18:23

Merge remote-tracking branch 'origin/main' into refacto/remove-everyt…

549cd59

…hing-about-defaultWorkspaceId # Conflicts: # packages/twenty-server/test/integration/metadata/suites/object-metadata/rename-custom-object.integration-spec.ts

WIP

5b91dc2

AMoreaux requested review from charlesBochet and FelixMalfait December 20, 2024 15:18

AMoreaux marked this pull request as ready for review December 20, 2024 15:27

greptile-apps bot reviewed Dec 20, 2024

View reviewed changes

AMoreaux added 8 commits December 20, 2024 16:59

refactor(settings): relocate SettingsAdminContent component

4a7b973

Moved SettingsAdminContent to a more appropriate module directory for improved project structure and maintainability. Updated all import paths to reflect the new location.

fix(impersonate-guard): remove redundant workspace check

3b8e508

The guard no longer checks for `workspace.allowImpersonation` as it is unnecessary. Impersonation access is now solely determined by the user's permissions. This simplifies the logic and avoids potential misconfigurations.

feat(admin-panel): Add support for allowImpersonation flag

70fad0b

Added allowImpersonation property to workspace entities in both front-end and back-end. This enables tracking of whether impersonation is allowed for each workspace in user lookup operations.

refactor(database): remove defaultWorkspaceId and fix imports

bef5132

Removed the defaultWorkspaceId column and its associated constraints from database migrations. Additionally, added an import in workspace.entity.ts to address circular dependency issues.

chore(workspace): document circular dependency workaround

4400f5e

Added an explanatory comment detailing the specific circular dependency path to improve code readability and maintenance. This change does not affect functionality but aims to assist future developers in understanding the issue

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refacto(*): remove everything about default workspace #9157

refacto(*): remove everything about default workspace #9157

AMoreaux commented Dec 19, 2024 •

edited

Loading

github-actions bot commented Dec 20, 2024 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot Dec 20, 2024

greptile-apps bot Dec 20, 2024

greptile-apps bot Dec 20, 2024

greptile-apps bot Dec 20, 2024

greptile-apps bot Dec 20, 2024

greptile-apps bot Dec 20, 2024

greptile-apps bot Dec 20, 2024

greptile-apps bot Dec 20, 2024

greptile-apps bot Dec 20, 2024

		const request = ctx.getContext().req;

		return request.user.canImpersonate === true;

refacto(*): remove everything about default workspace #9157

Are you sure you want to change the base?

refacto(*): remove everything about default workspace #9157

Conversation

AMoreaux commented Dec 19, 2024 • edited Loading

Summary

github-actions bot commented Dec 20, 2024 • edited Loading

TODOs/FIXMEs:

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Dec 20, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 20, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 20, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 20, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 20, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 20, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 20, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 20, 2024

Choose a reason for hiding this comment

greptile-apps bot Dec 20, 2024

Choose a reason for hiding this comment

AMoreaux commented Dec 19, 2024 •

edited

Loading

github-actions bot commented Dec 20, 2024 •

edited

Loading