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(providers): add BoxyHQ SAML Jackson provider #3782
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looking good, but I added some comments. Could you also provide a screenshot of a successful login for future reference?
Thanks @balazsorban44, I'll address the suggestions shortly. |
Thanks @balazsorban44, for very useful suggestions. I wasn't aware of a few things you mentioned there. |
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/nextauthjs/next-auth/EwMwyrpaeKHLXsvFLsmH9m2r443B [Deployment for ab81e1a canceled] |
@balazsorban44 Adjusted the code for the monorepo changes but unable to test due to some issues with running the dev app, will look into it soon. nextauthjs/docs#217 can be closed |
# Conflicts: # docs/providers.json
@balazsorban44 Thanks again for the review, I have made the necessary changes and also added an example for you to test. |
Checking this now 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Besides the minor concerns and formatting preferences, is there a way to use consistent naming in all places?
I am seeing BoxyHQ SAML Jackson and all its permutations in different places:
- BoxyHQ SAML Jackson
"boxyhq-saml"
"saml"
SAMLJacksonProfile
BoxyHQSAMLProvider
- SAML Jackson
We should try to stick to a similar name, derived from the same brand naming in all places like docs, TypeScript, function names, ids, etc.
Could we unify these? 🙏
clientId: "dummy", // The dummy here is necessary since we'll pass tenant and product custom attributes in the client code | ||
clientSecret: "dummy", // The dummy here is necessary since we'll pass tenant and product custom attributes in the client code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that I understand this correctly. These values are always dummy? 🤔
It might be my limited knowledge of SAML, but setting a hardcoded clientSecret
does not sound right in OAuth 2.0. What am I missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@balazsorban44 Please see here - #3782 (comment). User can opt for a proper clientSecret or the convenience of tenant and product.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also this configuration can be set during deployment to change the default dummy
value - https://boxyhq.com/docs/jackson/deploy/env-variables#client_secret_verifier
I have to add, the demo provider is wonderful, I wish most providers would have a public example like that 🙏 ✨ |
- env var default values moved to env.local.example - consistent naming and use of id
Thanks @balazsorban44 🙏, we built the Mock SAML tool specifically for this purpose (and to ease testing as well). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Many thanks @balazsorban44! |
https://github.com/boxyhq/jackson is an open-source SAML SSO service that implements SAML login as an OAuth 2.0 flow. Adding it as a provider here for the convenience of our users.
Reasoning 💡
This simplifies the usage for our customers and we hope other NextAuth users will find our SAML integration useful for their enterprise apps.
Checklist 🧢
[ ] TestsAffected issues 🎟
n/a