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

chore(auth): update SAML strategy configuration #9829

Merged
merged 1 commit into from
Jan 24, 2025

Conversation

AMoreaux
Copy link
Contributor

Added disableRequestedAuthnContext flag to SAML auth strategy to align with compatibility requirements. Adjustments ensure seamless integration with certain Identity Providers. No functional impact on existing flows.

Added `disableRequestedAuthnContext` flag to SAML auth strategy to align with compatibility requirements. Adjustments ensure seamless integration with certain Identity Providers. No functional impact on existing flows.
@AMoreaux AMoreaux self-assigned this Jan 24, 2025
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 updates the SAML authentication strategy configuration by adding a compatibility flag to improve integration with various Identity Providers.

  • Added disableRequestedAuthnContext: true in /packages/twenty-server/src/engine/core-modules/auth/strategies/saml.auth.strategy.ts to disable enforcement of specific authentication contexts
  • Reorganized security settings in SAML config for better code readability while maintaining existing security flags

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

Comment on lines 39 to +42
// TODO: Improve the feature by sign the response
wantAssertionsSigned: false,
wantAuthnResponseSigned: false,
disableRequestedAuthnContext: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: All signature verification is disabled and AuthnContext is now disabled. This reduces security. Consider enabling at least one form of signature verification.

@FelixMalfait FelixMalfait merged commit f23de2f into main Jan 24, 2025
32 checks passed
@FelixMalfait FelixMalfait deleted the chore/disable-authn-context-for-saml branch January 24, 2025 13:52
Copy link

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-0bbfbc49.json

Generated by 🚫 dangerJS against 326d421

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

Successfully merging this pull request may close these issues.

2 participants