feat(auth): centralize SSO error handling logic #9832
Conversation
Introduce `SsoErrorRedirectService` to handle SSO error redirection and exception capturing across the authentication controllers. Refactor Microsoft, Google, and SSO authentication controllers to utilize this service, replacing the previous direct calls to `DomainManagerService`. Added unit tests for the new service to ensure robust error handling.
![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 introduces a new SsoErrorRedirectService to centralize error handling across SSO authentication flows, improving code organization and maintainability.
- Added
SsoErrorRedirectServicein/auth/services/sso-error-redirect.service.tsto handle error redirection and exception capturing - Refactored Microsoft, Google, and SSO auth controllers to use the new service, removing direct
DomainManagerServicedependencies - Added comprehensive test coverage in
sso-error-redirect.spec.tsfor various error scenarios - Improved error handling consistency by capturing and redirecting all error types uniformly
- Simplified OIDC issuer discovery logic in
sso.service.tswith better error handling patterns
8 file(s) reviewed, 11 comment(s)
Edit PR Review Bot Settings | Greptile
| } catch (err) { | ||
| if (err instanceof AuthException) { | ||
| return res.redirect( | ||
| this.domainManagerService.computeRedirectErrorUrl( | ||
| err.message, | ||
| currentWorkspace?.subdomain ?? | ||
| this.environmentService.get('DEFAULT_SUBDOMAIN'), | ||
| ), | ||
| ); | ||
| } | ||
| throw new AuthException(err, AuthExceptionCode.INTERNAL_SERVER_ERROR); | ||
| return res.redirect( | ||
| this.ssoErrorService.getRedirectErrorUrlAndCaptureExceptions( | ||
| err, | ||
| currentWorkspace ?? { | ||
| subdomain: this.environmentService.get('DEFAULT_SUBDOMAIN'), | ||
| }, | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
logic: error handling now captures all types of errors, not just AuthException, which could expose internal server errors to users if not properly sanitized in SsoErrorRedirectService
| err: any, | ||
| workspace: { id?: string; subdomain: string }, |
There was a problem hiding this comment.
style: err parameter should be typed more specifically than 'any'. Consider creating a union type of expected error types (AuthException | Error | unknown)
| !(err instanceof AuthException) || | ||
| ('code' in err && err.code === 'INTERNAL_SERVER_ERROR') | ||
| ) { |
There was a problem hiding this comment.
logic: this condition will capture exceptions for non-AuthExceptions OR AuthExceptions with INTERNAL_SERVER_ERROR code. The logic may need to be reversed to capture ONLY non-AuthExceptions and INTERNAL_SERVER_ERROR AuthExceptions
| const email = '[email protected]'; | ||
| const mockWorkspace = { id: 'workspace-id-456' } as Workspace; | ||
|
|
||
| jest.spyOn(environmentService, 'get').mockReturnValue(true); |
There was a problem hiding this comment.
logic: Missing test for environmentService.get() returning false - should test both enabled and disabled multi-workspace scenarios
| } catch (err) { | ||
| this.exceptionHandlerService.captureExceptions([err]); | ||
| } |
There was a problem hiding this comment.
logic: catching and capturing errors from the exception handler itself could create an infinite loop if the exception handler throws
| private getErrorMessage(err: any) { | ||
| try { | ||
| return err.message && err.message.length !== 0 | ||
| ? err.message | ||
| : 'Unknown error'; | ||
| } catch (err) { | ||
| return 'Unknown error'; | ||
| } | ||
| } |
There was a problem hiding this comment.
style: try-catch block here is unnecessary since err.message access won't throw - the ternary is sufficient
|
|
||
| import { SsoErrorRedirectService } from './sso-error-redirect.service'; | ||
|
|
||
| describe('SsoErrorService', () => { |
There was a problem hiding this comment.
syntax: test suite name 'SsoErrorService' doesn't match the actual service name 'SsoErrorRedirectService'
packages/twenty-server/src/engine/core-modules/auth/services/sso-error-redirect.spec.ts
Outdated
Show resolved
Hide resolved
packages/twenty-server/src/engine/core-modules/sso/services/sso.service.ts
Outdated
Show resolved
Hide resolved
| if (err instanceof SSOException) { | ||
| return err; | ||
| } |
There was a problem hiding this comment.
logic: Returning the error directly instead of throwing it creates inconsistent error handling. Consider throwing the error to maintain consistency with other methods
Integrate ExceptionHandlerService into SSOService and refine error capturing in SsoErrorRedirectService. Streamlined error handling logic by consolidating exception capture methods and improving maintainability.
…vice Removed SsoErrorRedirectService and its related tests, consolidating its functionality into GuardRedirectService. Updated all references across the codebase to use GuardRedirectService for error handling and redirection in guards and controllers. This change improves modularity and simplifies the error handling logic.
Introduce
SsoErrorRedirectServiceto handle SSO error redirection and exception capturing across the authentication controllers. Refactor Microsoft, Google, and SSO authentication controllers to utilize this service, replacing the previous direct calls toDomainManagerService. Added unit tests for the new service to ensure robust error handling.