-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Move SAML config into separate page and add multi-SAML #32469
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
Conversation
| 6. After uploading the IdP metadata, return to the **Login Methods** page and turn SAML `on` by default. | ||
|
|
||
| **Note**: To configure SAML for a multi-org, see [Managing Multiple-Organization Accounts][18]. | ||
|
|
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.
New content starts below
Preview links (active after the
|
buraizu
left a comment
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.
Looks good overall! I left a few suggestions that:
- Update some links
- Make note format consistent
- Make minor style updates
The suggestions that add a hyphen to single sign on are based on the Microsoft style guide. Both styles are used in the docs currently, but the hyphenated version seems to be more common. Let me know if there's a reason to keep this un-hyphenated.
| * [Auth0][10] | ||
| * [Google][12] | ||
| * [Microsoft Entra ID][11] | ||
| * [NoPassword][13] |
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.
| * [NoPassword][13] | |
| * [LastPass][13] |
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.
This seems to have been deprecated:
NoPassword was acquired by LogMeIn, the creator of LastPass. The NoPassword Datadog SAML integration is no longer available. Follow the LastPass integration instructions above.
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.
That's a good point. Would you recommend removing the mention from this page entirely?
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.
I'm guessing this can be removed entirely, and maybe also this page as well.
However, I'll defer to the AAA team, who may have more context on this.
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.
@wangadong one more question for you if you don't mind! Do we still want to mention NoPassword/LastPass in this doc?
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.
This should be a question for our PM @tomnof or customer service people who is closer to our customer integration. But since NoPassword is no longer available, so we can remove it. But I guess LastPass still provides integration as a SAML IdP, and it's a question about whether we want to update the doc to show how to integrate with LastPass?
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.
Since NoPassword doesn't exist anymore -- let's remove.
Since LastPass do exist -- let's keep.
from what I can tell, we currently don't have any saml customers with LastPass, but lets keep anyway
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.
I'm going to leave LastPass in for this PR but make a ticket for myself to keep investigating, because it looks like the LastPass/Datadog page referenced doesn't exist anymore on their side. So we might want to remove the mention entirely in the future (but it doesn't have to block this PR merging)
Co-authored-by: Bryce Eadie <bryce.eadie@datadoghq.com>
Co-authored-by: Bryce Eadie <bryce.eadie@datadoghq.com>
buraizu
left a comment
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 for following up on those suggestions! I've added a couple of additional suggestions to add detail about the required permissions, but overall LGTM and approving.
|
|
||
| ### Role mapping with multiple SAML providers | ||
|
|
||
| If you use SAML role mapping and want to use the same role mappings in any additional providers you add, make sure the attributes in the new IdP(s) match what is defined in your mappings. If you add a new IdP, make sure to either use the same attribute names as your existing IdP, or add new mappings that align with the new IdP's attributes to ensure roles are assigned correctly when users log in with different IdPs. |
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.
should we add a link to this page:
https://docs-staging.datadoghq.com/eva/DOCS-11869/account_management/saml/mapping/#map-saml-attributes-to-datadog-roles
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.
Yes! Good idea. Added
| * [Auth0][10] | ||
| * [Google][12] | ||
| * [Microsoft Entra ID][11] | ||
| * [NoPassword][13] |
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.
Since NoPassword doesn't exist anymore -- let's remove.
Since LastPass do exist -- let's keep.
from what I can tell, we currently don't have any saml customers with LastPass, but lets keep anyway
Co-authored-by: Bryce Eadie <bryce.eadie@datadoghq.com>
| * [LastPass][13] | ||
| * [Okta][14] | ||
| * [SafeNet][15] | ||
|
|
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.
something potentially worth noting - although right now the UI is different for multi saml & non multi saml customers
We are going to unify the experience under the same (new) UI which we created for multi saml
The only difference is that non-enterprise customers won't have the ability to add additional SAML
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.
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.
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.
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.
nope! when you upload your first saml, we turn it on for you :)
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.
Perfect, I'll remove that step!
| ### Role mapping with multiple SAML providers | ||
|
|
||
| If you use [SAML role mapping][19] and want to use the same role mappings in any additional providers you add, make sure the attributes in the new IdP(s) match what is defined in your mappings. If you add a new IdP, make sure to either use the same attribute names as your existing IdP, or add new mappings that align with the new IdP's attributes to ensure roles are assigned correctly when users log in with different IdPs. | ||
|
|
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.
the same also applies to team mappings :)
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.
Sweet, I can change this section to say "role or team mappings" throughout
* Move SAML config into separate page and add multi-SAML * Fix menu * Fix menu but for real * Apply suggestions from code review Co-authored-by: Bryce Eadie <bryce.eadie@datadoghq.com> * Add considerations + restyle notes * Rewording * Incorporate considerations into procedure * Update link and remove a sentence * Apply suggestions from code review Co-authored-by: Bryce Eadie <bryce.eadie@datadoghq.com> * Apply suggestions from code review Co-authored-by: Bryce Eadie <bryce.eadie@datadoghq.com> * NoPassword->LastPass, and link to role mapping doc * Swap screenshot and rewrite single SAML config procedure * Remove unnecessary step and add team mapping info --------- Co-authored-by: Bryce Eadie <bryce.eadie@datadoghq.com>

What does this PR do? What is the motivation?
You can now enable multiple SAML providers per organization (available to enterprise customers only). This PR moves the SAML configuration information out of the top-level SAML doc and into a standalone configuration guide, and adds a new section "Configuring multiple SAML providers."
Merge instructions
Merge readiness: