Skip to content

Social: Microsoft - Enable single-tenant app registrations#11207

Closed
Exordian wants to merge 1 commit into
keycloak:mainfrom
cloudflightio:feature/je/social-microsoft-fix-single-tenant-app
Closed

Social: Microsoft - Enable single-tenant app registrations#11207
Exordian wants to merge 1 commit into
keycloak:mainfrom
cloudflightio:feature/je/social-microsoft-fix-single-tenant-app

Conversation

@Exordian

@Exordian Exordian commented Apr 9, 2022 •
edited
Loading

Copy link
Copy Markdown

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

  1. 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
  2. resolves the token-exchange feature for Microsoft federated users

Related, if you think it's worth to discuss, we should open a seperate issue:
I'd argue that it would also be nice to customize the keycloak id of federated users on the generic oidc provider. Microsoft offers "oid" next to "sub" in their access tokens to get the object id of an Azure user.

[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

@Exordian

Exordian commented Apr 9, 2022

Copy link
Copy Markdown
Author

Error message on single-tenant app registrations when used with the Microsoft Social provider:

AADSTS50194: Application 'XXX'(YYY) is not configured as a multi-tenant application. Usage of the /common endpoint is not supported for such applications created after '10/15/2018'. Use a tenant-specific endpoint or configure the application to be multi-tenant.

@Exordian

Exordian commented May 3, 2022

Copy link
Copy Markdown
Author

cc @sguilhen ; i hope it's not rude to notify you and ask for a review. Thanks in advance!

I'd argue that the change is quite minimal and you have been the latest RedHatter who worked on the microsoft federation plugin. We'd be very glad if we could ditch our custom plugin again and use the upstream code again for the MS federation plugin

@sguilhen

sguilhen commented May 13, 2022

Copy link
Copy Markdown
Contributor

@Exordian no problem at all - I didn't reply earlier because I was on PTO until today. I'll try to take a look early next week.

@Exordian

Exordian commented Jun 9, 2022

Copy link
Copy Markdown
Author

@sguilhen ping ;)

sguilhen
sguilhen reviewed Jun 10, 2022
View reviewed changes
Comment thread services/src/main/java/org/keycloak/social/microsoft/MicrosoftIdentityProviderConfig.java
@@ -0,0 +1,25 @@
package org.keycloak.social.microsoft;

@sguilhen sguilhen Jun 10, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add the copyright header to this new file

sguilhen
sguilhen reviewed Jun 10, 2022
View reviewed changes

@sguilhen sguilhen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Exordian Thanks for the PR! Overall it looks good to me (just missing copyright header), but since it touches the admin console as well I'm asking the UI team to check this too.

@sguilhen sguilhen requested review from pedroigor and ssilvert June 10, 2022 15:01
@sguilhen

sguilhen commented Jun 10, 2022

Copy link
Copy Markdown
Contributor

@ssilvert can you review the admin console changes here?

@ssilvert

ssilvert commented Jun 13, 2022

Copy link
Copy Markdown
Contributor

@sguilhen @Exordian I don't mind accepting these small changes, but we are trying to get away from changing the old admin console.

Furthermore, to accept these changes, I need to see equivalent changes in the new console at keycloak/keycloak-admin-ui.

pedroigor
pedroigor reviewed Jun 14, 2022
View reviewed changes

@pedroigor pedroigor left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, looking at the broker changes.

We also need to run social tests for this one.

@pedroigor

pedroigor commented Jun 14, 2022

Copy link
Copy Markdown
Contributor

@Exordian Could you please create an issue and link it here by changing your commit to reference it? Please, see other PRs to check how the commit message should look like.

@pedroigor

pedroigor commented Jun 14, 2022

Copy link
Copy Markdown
Contributor

Started keycloak-build-pipeline/757/.

@pedroigor

pedroigor commented Jun 14, 2022

Copy link
Copy Markdown
Contributor

Running build #759.

@Exordian

Exordian commented Jul 17, 2022

Copy link
Copy Markdown
Author

thanks for the review. i hope i find some minutes next week in order to test this change with the new admin UI and create an issue with a similar text as mentioned above.
the copyright headers felt quite inconsistent in keycloak, hence i avoided them. I can also add one, i don't mind too much

pedroigor
pedroigor reviewed Aug 26, 2022
View reviewed changes

@pedroigor pedroigor left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Exordian If you can:

  • Update commit message to link the corresponding issuer
  • Add copyright headers as suggested by @sguilhen

Then we are good to go.

@magicrobotmonkey

magicrobotmonkey commented Feb 16, 2023

Copy link
Copy Markdown

@Exordian can I help move this forward?

@Tasssadar Tasssadar mentioned this pull request May 31, 2023
Closed
@Tasssadar Tasssadar mentioned this pull request May 31, 2023
Merged
@jonkoops

jonkoops commented Jun 5, 2023

Copy link
Copy Markdown
Contributor

Superseded by #20699

@jonkoops jonkoops closed this Jun 5, 2023
ssilvert pushed a commit to Tasssadar/keycloak that referenced this pull request Oct 10, 2023
@Tasssadar @ssilvert
Add support for single-tenant mode to Microsoft Identity Provider
9f4e535 
Fixes keycloak#20695
Closes keycloak#11207
ssilvert pushed a commit that referenced this pull request Oct 10, 2023
@Tasssadar
Add support for single-tenant mode to Microsoft Identity Provider (#2…
8871983 
…0699)

* Add support for single-tenant mode to Microsoft Identity Provider

Fixes #20695
Closes #11207

* Add SocialLoginTest for Microsoft single-tenant variant
@pedroigor pedroigor mentioned this pull request Apr 19, 2024
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pedroigor pedroigor pedroigor left review comments

@ssilvert ssilvert Awaiting requested review from ssilvert

+1 more reviewer

@sguilhen sguilhen sguilhen left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@pedroigor pedroigor

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@Exordian @sguilhen @ssilvert @pedroigor @magicrobotmonkey @jonkoops @jenglisch-clf