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

fix(auth): Improve error management with sso + fix microsoft saml #9799

Merged
merged 13 commits into from
Jan 24, 2025

Conversation

AMoreaux
Copy link
Contributor

@AMoreaux AMoreaux commented Jan 22, 2025

Fix #9760 #9758

Replaced static error handling in multiple auth guards with a new dynamic error dispatch mechanism using GuardErrorManagerService. Added context-based error propagation, ensuring consistent error management across all guards, including OAuth and provider checks.
@AMoreaux AMoreaux self-assigned this Jan 22, 2025
Copy link

github-actions bot commented Jan 22, 2025

Fails
🚫

node failed.

Log

�[31mError: �[39m SyntaxError: Unexpected token C in JSON at position 0
    at JSON.parse (<anonymous>)
�[90m    at parseJSONFromBytes (node:internal/deps/undici/undici:5584:19)�[39m
�[90m    at successSteps (node:internal/deps/undici/undici:5555:27)�[39m
�[90m    at fullyReadBody (node:internal/deps/undici/undici:1665:9)�[39m
�[90m    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)�[39m
�[90m    at async specConsumeBody (node:internal/deps/undici/undici:5564:7)�[39m
danger-results://tmp/danger-results-b94e97ad.json

Generated by 🚫 dangerJS against 5d74917

Replaced `getRequestOriginByHeaders` with `getCallerByContext` for improved clarity and cohesion. Removed unused `OAUTH_INVALID_PAYLOAD` error code and associated metadata handling, simplifying AuthException logic. Adjusted SSO guards to derive subdomain from `origin_url` when available.
Updated the import path for `getCallerByContext` to reflect the correct module location. This ensures the unit tests reference the appropriate file and avoid potential failures due to incorrect imports.
Updated validation schemas to require non-empty strings for critical SSOIdentityProvider fields such as clientID, clientSecret, id, ssoURL, certificate, and issuer. This ensures stricter validation and
Simplified error handling by replacing `GuardManagerService` with `GuardRedirectService`, improving clarity and modularity. Removed the unused `getCallerByContext`
# Conflicts:
#	packages/twenty-server/src/engine/core-modules/auth/auth.module.ts
#	packages/twenty-server/src/engine/core-modules/billing/services/billing-portal.workspace-service.ts
Reorganized imports in `billing-portal.workspace-service.ts` to maintain alphabetical order and consistency. No functional changes were made.
Copy link
Member

@charlesBochet charlesBochet left a comment

Choose a reason for hiding this comment

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

Love it, thanks for addressing all the feedbacks

@charlesBochet charlesBochet merged commit 5783c41 into main Jan 24, 2025
47 checks passed
@charlesBochet charlesBochet deleted the fix/sso branch January 24, 2025 09:36
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.

Error message is unclear when SSO setup fails
2 participants