Add support for single-tenant mode to Microsoft Identity Provider#20699
Conversation
e18fbe6 to
815e5ce
Compare
815e5ce to
25e620a
Compare
There was a problem hiding this comment.
If possible we prefer to use test id's to retrieve elements during test, for more information see the Cypress Testing Library documentation.
There was a problem hiding this comment.
Hi, not sure what you mean here - this file is pretty much copypasted from ProviderGoogleGeneralSettings verbatim, and all the Provider tests use this method to get the input element.
There was a problem hiding this comment.
You can search the project for findByTestId to get an idea of what needs to be done. Older code still uses id as we haven't updated it to match the new conventions everywhere.
There was a problem hiding this comment.
Thanks, I updated it to findByTestId. I also see the tests are failing, but it does not seem to be related to this PR.
|
@ssilvert can I get your input on the Java code for this one? |
25e620a to
bbd5f8a
Compare
ssilvert
left a comment
There was a problem hiding this comment.
Java code looks fine. Technically, the copyright should be updated to 2023. But I'm not going to hold this up for that.
5788461 to
91eacd0
Compare
1 flaky test on run #9331 ↗︎Details:
|
|||||||||||||||||||||
| Test | Artifacts | |
|---|---|---|
| Realm settings general tab tests > Test all general tab switches |
Test Replay
Output
Screenshots
|
|
Review all test suite changes for PR #20699 ↗︎
|
@pedroigor since you reviewed the original PR, can I ask you to review this one as well? |
|
Thanks for taking over! |
Head branch was pushed to by a user without write access
976e7bc to
55af7ab
Compare
|
Rebased, I'm not sure I fixed the |
16e7429 to
8d8a2b5
Compare
|
Hello, anything I can do to help push this along? |
8d8a2b5 to
7dcb648
Compare
|
@pedroigor @ssilvert could y'all give this another look? |
jonkoops
left a comment
There was a problem hiding this comment.
Changes to the client-side tests look good to me.
7dcb648 to
305ec23
Compare
|
Just wanting to +1 this merge. It's currently holding us back from using KeyCloak with Microsoft for the reasons explained. KeyCloak will be our enterprise management interface, and we don't really want it to be multi-tenant capable if possible. |
305ec23 to
cbb8023
Compare
cbb8023 to
3a1aa9e
Compare
This is pretty much #11207, but updated to main. Description copied from #11207:
This change allows the usage of "single-tenant" App Registrations on Azure AD.
Microsoft distinguishes between single-tenant and multi-tenant app registrations.
Usually, enterprise companies would rather like to use so called "single-tenant" app registrations instead of multi-tenant app registrations [1], as they're limited to users in their tenant.
Since 2018, Microsoft disallowed the use of the global "/common/" endpoints for single-tenant applications [2]. The usual answer on the internet has been "use the generic oidc provider". While the generic OIDC provider works fine most of the cases, it doesn't work properly when using the "exchange-token" feature of keycloak.
The generic OIDC connector maps user identities to the OIDC subject field "sub" [3], which differs on the same account for every client on Microsoft [4] (OIDC spec refers to the 'pairwise' mode here [5]). However, the microsoft social connector stores the object id of the account in keycloak for federated users [6].
Therefore, while using the token-exchange feature from external to internal tokens, the different "sub" fields per client for the same account leads to mapping problems [7]. The token-exchange provider fails to find the respective user and tries to create a new user with the same mail address, which fails.
This patch allows to use the Microsoft social provider with single-tenant apps, which makes it easier for admins to configure microsoft federation, if the only need a single-tenant to federate, as they have to configure way less properties compared to the generic OIDC adapter resolves the token-exchange feature for Microsoft federated users
[1] https://docs.microsoft.com/en-us/azure/active-directory/develop/single-and-multi-tenant-apps
[2] https://keycloak.discourse.group/t/how-to-configure-microsoft-identity-provider-with-single-tenant/11741 https://lists.jboss.org/pipermail/keycloak-user/2019-February/017254.html
[3] https://github.com/keycloak/keycloak/blob/main/services/src/main/java/org/keycloak/broker/oidc/OIDCIdentityProvider.java#L651
[4] https://stackoverflow.com/questions/52876679/the-sub-claim-value-is-different-between-access-and-id-tokens
[5] https://openid.net/specs/openid-connect-core-1_0.html#SubjectIDTypes
[6] https://github.com/keycloak/keycloak/blob/main/services/src/main/java/org/keycloak/social/microsoft/MicrosoftIdentityProvider.java#L80
[7] https://github.com/keycloak/keycloak/blob/main/services/src/main/java/org/keycloak/protocol/oidc/DefaultTokenExchangeProvider.java#L485
Fixes #20695
Closes #11207