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

feat(sso): add support for identityProviderId in SAML flow #9411

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

AMoreaux
Copy link
Contributor

@AMoreaux AMoreaux commented Jan 6, 2025

Updated SAML callback URLs and relevant logic to include identityProviderId, ensuring better handling of multiple identity providers. Refactored client and server-side code to streamline form interactions and validation within the SSO module.

Fix #9323 #9325

Updated SAML callback URLs and relevant logic to include identityProviderId, ensuring better handling of multiple identity providers. Refactored client and server-side code to streamline form interactions and validation within the SSO module.
@AMoreaux AMoreaux requested a review from FelixMalfait January 6, 2025 16:41
@AMoreaux AMoreaux self-assigned this Jan 6, 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 enhances SSO identity provider handling, particularly focusing on SAML flow improvements and direct provider access support.

  • Added identityProviderId to SAML callback URLs and metadata generation in sso-auth.controller.ts to support direct SSO access from providers
  • Fixed form validation in SettingsSSOSAMLForm.tsx by triggering validation after file upload to resolve greyed-out Save button
  • Optimized form state management in SettingsSSOIdentitiesProvidersForm.tsx using useMemo and watch instead of getValues
  • Updated URL path construction in sso.service.ts to include provider ID for consistent multi-provider support
  • Streamlined form submission in SettingsSecuritySSOIdentifyProvider.tsx using lodash.pick for targeted field selection

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

Comment on lines +38 to +45
await createSSOIdentityProvider(
SSOIdentitiesProvidersParamsSchema.parse(
pick(
formConfig.getValues(),
Object.keys(sSOIdentityProviderDefaultValues[type]()),
),
),
);
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: This validation approach could allow invalid fields to be submitted if they were populated before switching SSO types. Consider clearing irrelevant fields when type changes.

@FelixMalfait FelixMalfait merged commit 00e7147 into main Jan 7, 2025
23 checks passed
@FelixMalfait FelixMalfait deleted the fix/sso branch January 7, 2025 09:30
Copy link

github-actions bot commented Jan 7, 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-a6b531fd.json

Generated by 🚫 dangerJS against 977d730

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 small issues while creating sso provider
2 participants