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

[IMPROVE] SAML login process refactoring #12891

Merged
merged 157 commits into from
May 7, 2019
Merged

[IMPROVE] SAML login process refactoring #12891

merged 157 commits into from
May 7, 2019

Conversation

kukkjanos
Copy link
Contributor

  • the eppn is primary identifier match parameter (if not exist then email is a secondary)
  • update eppn to user profile
  • support the eduPersonPrincipalName (eppn) usually required saml parameter
  • support the displayName optional saml parameter
  • support two new saml settings
    • overwrite user fullname if need (use idp attribute [cn,username or displayName])
    • overwrite user mail address if need (use idp attribute)

FIX NEW DIRECTORY

Closes #ISSUE_NUMBER

kukkjanos and others added 16 commits September 13, 2018 13:31
- the eppn is primary identifier match parameter (if not exist then email is a secondary)
- update eppn to user profile
- support the eduPersonPrincipalName (eppn) usually required saml parameter
- support the displayName optional saml parameter
- support two new saml settings
  - overwrite user fullname if need (use idp attribute [cn,username or displayName])
  - overwrite user mail address if need (use idp attribute)
- the eppn is primary identifier match parameter (if not exist then email is a secondary)
- update eppn to user profile
- support the eduPersonPrincipalName (eppn) usually required saml parameter
- support the displayName optional saml parameter
- support two new saml settings
  - overwrite user fullname if need (use idp attribute [cn,username or displayName])
  - overwrite user mail address if need (use idp attribute)
@CLAassistant
Copy link

CLAassistant commented Dec 8, 2018

CLA assistant check
All committers have signed the CLA.

@misi
Copy link
Contributor

misi commented Dec 17, 2018

+1

@rukverc
Copy link

rukverc commented Dec 17, 2018

This is a very useful feature, strongly recommended.

@EmiTrif
Copy link

EmiTrif commented Dec 17, 2018

+1

1 similar comment
@northway
Copy link

+1

rukverc
rukverc previously approved these changes Dec 17, 2018
Copy link

@rukverc rukverc left a comment

Choose a reason for hiding this comment

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

Contains necessary changes, seems legit code.

@kukkjanos kukkjanos dismissed stale reviews from Hudell and Toushe via ee6afa0 April 29, 2019 13:11
Copy link

@Toushe Toushe left a comment

Choose a reason for hiding this comment

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

Good job!

@Hudell Hudell added this to the 1.1.0 milestone Apr 29, 2019
@sampaiodiego sampaiodiego changed the title [NEW] SAML login process refactoring [IMPROVE] SAML login process refactoring May 7, 2019
@sampaiodiego sampaiodiego merged commit 34e58da into RocketChat:develop May 7, 2019
@rodrigok rodrigok added this to PR 1.1.0 in 1.1.0 Review May 15, 2019
@rodrigok rodrigok moved this from PR Backlog to Done in 1.1.0 Review May 15, 2019
@sampaiodiego sampaiodiego mentioned this pull request May 28, 2019
@rAcHekLoS
Copy link

a) This change destroy SAML with Microsoft ADFS. The login works, but the user maps always to the User Rocket.Cat.
b) generally a problem is that you cannot configure the authnContext. To enable Kerberos or NTLM login, the app.js code must be modified.

May 29 16:43:40 x06-rocketchat rocketchat[17255]: { actionName: 'authorize',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: serviceName: 'adfs',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: credentialToken: 'id-x93vGuoAPjFWxtEtr' }
May 29 16:43:40 x06-rocketchat rocketchat[17255]: [ { provider: 'adfs',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: entryPoint: 'https://adfs.edag.de/adfs/ls/',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: idpSLORedirectURL: 'https://adfs.edag.de/adfs/ls/',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: issuer: 'https://rocketchat.edag.de',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: cert: '',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: privateCert: false,
May 29 16:43:40 x06-rocketchat rocketchat[17255]: privateKey: false,
May 29 16:43:40 x06-rocketchat rocketchat[17255]: callbackUrl: 'https://rocketchat.edag.de/_saml/validate/adfs',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: id: 'wvWk7Zjf1YnlDjEdn',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: protocol: 'https://',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: path: '/saml/consume',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: identifierFormat: 'urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: authnContext: 'urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport' } ]
May 29 16:43:40 x06-rocketchat rocketchat[17255]: adfs
May 29 16:43:40 x06-rocketchat rocketchat[17255]: { actionName: 'validate',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: serviceName: 'adfs',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: credentialToken: undefined }
May 29 16:43:40 x06-rocketchat rocketchat[17255]: [ { provider: 'adfs',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: entryPoint: 'https://adfs.edag.de/adfs/ls/',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: idpSLORedirectURL: 'https://adfs.edag.de/adfs/ls/',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: issuer: 'https://rocketchat.edag.de',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: cert: '',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: privateCert: false,
May 29 16:43:40 x06-rocketchat rocketchat[17255]: privateKey: false,
May 29 16:43:40 x06-rocketchat rocketchat[17255]: callbackUrl: 'https://rocketchat.edag.de/_saml/validate/adfs',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: id: 'id-x93vGuoAPjFWxtEtr',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: protocol: 'https://',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: path: '/saml/consume',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: identifierFormat: 'urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress',
May 29 16:43:40 x06-rocketchat rocketchat[17255]: authnContext: 'urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport' } ]
May 29 16:43:40 x06-rocketchat rocketchat[17255]: adfs
May 29 16:43:40 x06-rocketchat rocketchat[17255]: RESULT :{"profile":{"inResponseToId":"id-x93vGuoAPjFWxtEtr","issuer":"http://adfs.edag.de/adfs/services/trust","nameID":"xxxxx@xxxx.com","nameIDFormat":"urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress","sessionIndex":"_e4c81ddb-ddc0-49e4-bd51-36b501895c66","email":"xxxxx@xxxx.com","http://schemas-xmlsoap-org/ws/2005/05/identity/claims/name":"Test, Test","http://schemas-xmlsoap-org/ws/2005/05/identity/claims/emailaddress":"xxxxx@xxxx.com"}}

@Hudell
Copy link
Contributor

Hudell commented May 29, 2019

Does it need a different authnContext value to work? What value would that be?

@rAcHekLoS
Copy link

request += '<samlp:RequestedAuthnContext xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" Comparison="exact">' + '<saml:AuthnContextClassRef xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion">urn:federation:authentication:windows</saml:AuthnContextClassRef></samlp:RequestedAuthnContext>\n' + '</samlp:AuthnRequest>';

@isometry
Copy link

a) This change destroy SAML with Microsoft ADFS. The login works, but the user maps always to the User Rocket.Cat.

This change also breaks SAML with OneLogin IdP; same symptoms as reported here, but OneLogin does support the hard-coded authnContext of urn:oasis:names:tc:SAML:2.0:ac:classes:PasswordProtectedTransport

@Hudell
Copy link
Contributor

Hudell commented May 29, 2019

You mean login working but with the wrong user? That was fixed by #14686 and included on the 1.1.1 hotfix release.

@isometry
Copy link

You mean login working but with the wrong user? That was fixed by #14686 and included on the 1.1.1 hotfix release.

Confirmed fixed. Thanks!

@rAcHekLoS
Copy link

The problem #14686 is fixed, THX.

For AuthContext, I would suggest that you make an array to specify multiple AuthContext. This way you can support multiple variants.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
1.1.0 Review
  
Done
Development

Successfully merging this pull request may close these issues.

None yet