refacto(auth): improve type + remove complexity #9949
Conversation
AMoreaux
commented
Jan 31, 2025
•
AMoreaux
commented
- Improve type
- Remove unnecessary code
- Fix the issue that prevents the usage of invitations when a user signs in with social media.
- Add Microsoft icon for sso list page
Introduce `forceSubdomainUrl` for SSO login flow to handle subdomain-specific redirections. Updated guards, controllers, and services to incorporate the new parameter while removing redundant dependencies like `EnvironmentService`. Also added utility hooks and modified UI components to support the feature seamlessly.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR refactors the authentication system to improve type safety and simplify the social media sign-in flow, particularly focusing on invitation handling and state management.
- Standardized invitation handling across auth strategies by moving from
req.paramstoreq.queryfor invite-related parameters in/packages/twenty-server/src/engine/core-modules/auth/strategies/*.strategy.ts - Added proper type definitions and interfaces for SAML, OIDC, Google, and Microsoft requests in respective strategy files
- Simplified state management by removing unnecessary conditional spreading of optional parameters in auth strategies
- Improved error handling with custom
AuthExceptionand proper state validation in SSO controllers - Consolidated invitation check logic to only depend on
currentWorkspaceandemailin auth controllers
9 file(s) reviewed, 6 comment(s)
Edit PR Review Bot Settings | Greptile
| payload: { email: string }, | ||
| identityProvider: WorkspaceSSOIdentityProvider, |
There was a problem hiding this comment.
logic: The payload type is too restrictive - OIDC and SAML responses typically include firstName and lastName which are used in formatUserDataPayload()
| options = { | ||
| ...options, | ||
| state: JSON.stringify({ | ||
| workspaceInviteHash: req.params.workspaceInviteHash, | ||
| workspaceInviteHash: req.query.workspaceInviteHash, | ||
| workspaceId: req.params.workspaceId, |
There was a problem hiding this comment.
logic: workspaceId is still being read from req.params while other parameters were moved to req.query. This inconsistency could cause issues with workspace ID retrieval.
| @@ -30,7 +45,7 @@ export class OIDCAuthStrategy extends PassportStrategy( | |||
| }); | |||
| } | |||
|
|
|||
| async authenticate(req: any, options: any) { | |||
| async authenticate(req: Request, options: any) { | |||
There was a problem hiding this comment.
style: options parameter should be properly typed instead of 'any'
| async validate( | ||
| req: Request, | ||
| tokenset: TokenSet, | ||
| done: (err: any, user?: OIDCRequest['user']) => void, |
There was a problem hiding this comment.
style: err parameter should be properly typed instead of 'any'
| if (!userinfo.email || !isEmail(userinfo.email)) { | ||
| return done(new Error('Invalid email')); | ||
| } |
There was a problem hiding this comment.
logic: error message should use AuthException with proper error code for consistency with extractState method
| if (!userinfo.email || !isEmail(userinfo.email)) { | |
| return done(new Error('Invalid email')); | |
| } | |
| if (!userinfo.email || !isEmail(userinfo.email)) { | |
| return done(new AuthException('Invalid email', AuthExceptionCode.INVALID_INPUT)); | |
| } |
| }; | ||
| } | ||
|
|
||
| throw new Error(); |
There was a problem hiding this comment.
style: throwing empty Error here is redundant since it's immediately caught and transformed
Updated the icon mapping logic to recognize Microsoft SSO URLs and return the appropriate icon. Also corrected the import and usage of the Microsoft icon asset to ensure consistency.
Log |
