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

[NEW] Options for SAML auth for individual organizations needs #14275

Merged
merged 4 commits into from
Aug 21, 2019

Conversation

Deltachaos
Copy link
Contributor

Closes #6481

This PR fixes a problem with changed mail addresses for SAML logins by letting the admins to set if they want to identify the created Rocket.Chat user by ether "Username" or "E-Mail Address".

To increase the compatibility with various SAML providers, it also adds attribute mapping as many other SAML service provider already have implemented.

@CLAassistant
Copy link

CLAassistant commented Apr 28, 2019

CLA assistant check
All committers have signed the CLA.

ghost
ghost previously approved these changes May 16, 2019
@gerbsen
Copy link
Contributor

gerbsen commented May 24, 2019

hey there @ymybe, can we do something to help this MR get merged? I don't really understand why the last missing pipeline is on hold? Can we do something about this or is this cause the Rocket.Chat team needs to say "It's okay"? Cheers, Daniel

@Deltachaos Deltachaos changed the title [FIX] More options for SAML auth for individual organization needs. [FIX] More options for SAML auth for individual organizations needs. Jun 4, 2019
@anicoa
Copy link
Contributor

anicoa commented Jul 15, 2019

@rodrigok sorry for bother you, but you helped already with #12153 ;)

Would it be possible to get this pr merged? it enhances the saml authentication system and i think mapping of attributes is a huge improvement. thanks a lot for your effort!

@Deltachaos
Copy link
Contributor Author

@kukkjanos and @sampaiodiego You both have worked on the SAML component the recently. Do you have an opinion about this PR? What can we do to get this merged?

Copy link
Member

@sampaiodiego sampaiodiego left a comment

Choose a reason for hiding this comment

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

sorry the delay guys..

app/meteor-accounts-saml/server/saml_server.js Outdated Show resolved Hide resolved
@Deltachaos
Copy link
Contributor Author

@sampaiodiego have updated the PR

@Meirianecoelho

This comment has been minimized.

@Meirianecoelho

This comment has been minimized.

@Meirianecoelho

This comment has been minimized.

@Deltachaos
Copy link
Contributor Author

@sampaiodiego is there a chance to get this into 1.3.0 stable?

@sampaiodiego sampaiodiego changed the title [FIX] More options for SAML auth for individual organizations needs. [NEW] Options for SAML auth for individual organizations needs Aug 21, 2019
@sampaiodiego sampaiodiego merged commit f892671 into RocketChat:develop Aug 21, 2019
@Deltachaos
Copy link
Contributor Author

@sampaiodiego thanks for merging!

@sampaiodiego sampaiodiego mentioned this pull request Sep 12, 2019
@BarnumD
Copy link

BarnumD commented Sep 16, 2019

We have a staff id that never changes and I'm thinking about using that to identify the account in Rocket.Chat. But how? Would I map that field to 'username' in the saml configuration and select username as the immutable field name?
If I do that, Is the user's staff ID going to be visable in the rocket.chat interface.. For instance, when people try to mention someone using the @ symbol.
@Deltachaos

@Deltachaos
Copy link
Contributor Author

@BarnumD jep thats correct :/ i have not thought about this case. I guess the best would be to add another option to the SAML configuration where you can enter an arbitrary id field and then adding an option to the "SAML__username_normalize" list.

As a workaround you can probably also let your SAML provider return the field eppn. This overrides all the other methods for finding the user. I guess this property is not used anywhere else.

@BarnumD
Copy link

BarnumD commented Sep 25, 2019

Should I open a new issue since this one has been closed/merged?

@Deltachaos
Copy link
Contributor Author

@BarnumD i guess yes. But it is not really hard to implement. Feel free to open a new PR.

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.

SAML: changing realname and email address results in new user
9 participants