Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(auth): clean up and enhance sign-in-up logic #8653

Open
wants to merge 9 commits into
base: feat/allow-to-select-auth-methods-by-workspace
Choose a base branch
from

Conversation

AMoreaux
Copy link
Contributor

The Pull Request introduces new authentication provider columns to the Workspace entity and updates various services and modules to accommodate these changes. It also includes the creation of a new WorkspaceException class.

  • Adds new columns to the Workspace entity for managing authentication providers.
  • Modifies several services (AuthService, WorkspaceService, SignInUpService, etc.) to utilize the new columns.
  • Introduces WorkspaceException for better error handling in workspace-related operations.
  • Updates tests and modules to accommodate the new columns and exception.
  • Includes code format and refactoring for improved readability and maintainability.

Reorganized the sign-in-up code for better readability and maintainability. Introduced validation for workspace invitations and moved some utility functions to dedicated methods. Added a migration for new auth provider columns in the workspace entity.
Removed unused `EnvironmentService` and redundant `code` property in exceptions for clarity. Simplified email validation in `workspace-invitation.service.ts`. Refactored tests to remove unnecessary `targetWorkspaceSubdomain` property.
Removed redundant services from auth and core modules to streamline the code. Added debug log statements in sign-in-up service and skipped certain test cases temporarily for evaluation.
Eliminate debug logging for workspace invite tokens and validation. This cleanup improves readability and reduces console clutter during runtime.
…ninup

# Conflicts:
#	packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.ts
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Summary

This PR enhances workspace authentication by adding configurable auth provider settings and improving the sign-in/up flow with better separation of concerns.

  • Added new columns isGoogleAuthEnabled, isPasswordAuthEnabled, isMicrosoftAuthEnabled to workspace.entity.ts for granular auth control
  • Introduced getAuthProvidersByWorkspaceId in workspace.service.ts to expose available auth methods including SSO configurations
  • Moved workspace invitation validation to dedicated WorkspaceInvitationService with separate methods for public/personal invites
  • Added saveDefaultWorkspace to UserService for handling workspace switching during auth flows
  • Created new WorkspaceException class with specific error codes for better error handling

14 file(s) reviewed, 8 comment(s)
Edit PR Review Bot Settings | Greptile

@AMoreaux AMoreaux changed the base branch from main to feat/allow-to-select-auth-methods-by-workspace November 22, 2024 09:32
…facto/improve-signinup

# Conflicts:
#	packages/twenty-server/src/engine/core-modules/auth/dto/available-workspaces.output.ts
#	packages/twenty-server/src/engine/core-modules/auth/services/auth.service.ts
#	packages/twenty-server/src/engine/core-modules/auth/services/sign-in-up.service.ts
#	packages/twenty-server/src/engine/core-modules/user/services/user.service.ts
#	packages/twenty-server/src/engine/core-modules/workspace/services/workspace.service.ts
Removed the redundant `code` property from WorkspaceInvitationException. Adjusted the import order in WorkspaceResolver to comply with coding standards.
Replace user object with user ID in saveDefaultWorkspace method call to fix incorrect parameter type. This ensures that the function operates as expected and prevents potential errors in default workspace assignment.
where: {
email: challengeInput.email,
},
relations: ['workspaces'],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double check

@@ -130,20 +135,29 @@ export class AuthService {
});
}

async verify(email: string): Promise<Verify> {
private async findOneWithWorkspacesByEmail(email: string) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove this function

});
let user = await this.findOneWithWorkspacesByEmail(email);

if (user && workspaceId && user.defaultWorkspaceId !== workspaceId) {
Copy link
Member

@charlesBochet charlesBochet Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const defaultWorkspaceId = await this.userRepository.findOne({
select: ['defaultWorkspaceId']
      where: {
        email,
      },
    });

if (user.defaultWorkspaceId !== workspaceId) {
await this.userService.saveDefaultWorkspace(user.id, workspaceId);
}

const user = await this.userRepository.findOne({
      where: {
        email,
      },
      relations: ['defaultWorkspace', 'workspaces', 'workspaces.workspace'],
    });

workspaceInviteHash,
email,
})
: null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (workspacePersonalInviteToken || workspaceInviteHash) {
const invitationValidation =
await this.workspaceInvitationService.validateInvitation({
workspacePersonalInviteToken: workspacePersonalInviteToken,
workspaceInviteHash,
email,
})

if (
invitationValidation?.isValid === true &&
invitationValidation.workspace
) {
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure it's better

.getOne();
}

// UTILS METHODS
castAppTokenToWorkspaceInvitation(appToken: AppToken) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be extracted to an .util.ts

});

if (invitationByType.type === 'PUBLIC_INVITATION') {
return await this.validatePublicInvitation(invitationByType);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

workspaceInviteHash

}

if (invitationByType.type === 'PERSONAL_INVITATION') {
return await this.validatePersonalInvitation(invitationByType, email);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

workspacePersonalInviteToken

workspaceInviteHash?: string;
email: string;
}) {
const invitationByType = this.getInvitationByType({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the value of invitationByType actually in this code

await this.workspaceRepository.update(user.defaultWorkspaceId, {
displayName: data.displayName,
activationStatus: WorkspaceActivationStatus.ACTIVE,
});

return existingWorkspace;
return await this.workspaceRepository.findOneBy({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double check

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants