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
Support for multiple identity providers #29064
Conversation
3b1a3e5
to
f57fdd3
Compare
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.
Unreported flaky test detected, please review
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.actions.RequiredActionUpdateProfileTest#updateProfileKeycloak CI - Forms IT (chrome)
|
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.
Unreported flaky test detected, please review
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.adapter.servlet.BrokerLinkAndTokenExchangeTest#testExternalExchangeCreateNewUserUsingMappers
org.keycloak.testsuite.adapter.servlet.BrokerLinkAndTokenExchangeTest#testExternalExchange
org.keycloak.testsuite.adapter.servlet.BrokerLinkAndTokenExchangeTest#testAccountLinkNoTokenStore
org.keycloak.testsuite.admin.concurrency.ConcurrencyTest#createClient
org.keycloak.testsuite.client.ClientTypesTest#testUpdateClientWithClientType
org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testExchangeWithDynamicScopesEnabled
org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testClientExchange
org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testIntrospectTokenAfterImpersonation
org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testPublicClientNotAllowed
org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testExchangeUsingServiceAccount
org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testImpersonation
org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testImpersonationUsingPublicClient
|
model/jpa/src/main/java/org/keycloak/organization/jpa/JpaOrganizationProvider.java
Outdated
Show resolved
Hide resolved
model/jpa/src/main/java/org/keycloak/organization/jpa/JpaOrganizationProvider.java
Outdated
Show resolved
Hide resolved
server-spi-private/src/main/java/org/keycloak/organization/OrganizationProvider.java
Outdated
Show resolved
Hide resolved
...ain/java/org/keycloak/organization/admin/resource/OrganizationIdentityProvidersResource.java
Outdated
Show resolved
Hide resolved
...g/keycloak/organization/authentication/authenticators/browser/OrganizationAuthenticator.java
Outdated
Show resolved
Hide resolved
return false; | ||
} | ||
|
||
realm.removeIdentityProviderByAlias(identityProvider.getAlias()); |
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.
When we call this function do we want to delete the identity provider from the realm or only remove the association between the idp and the organization?
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.
For now, we remove it because we create it in the scope of the organization. However, we are going to change the Organization API to work similarly as users when linking brokers to an organization.
For that, we are going to link an existing broker instead therefore not removing it when removing from the organization, only the link as you are suggesting.
...ain/java/org/keycloak/organization/admin/resource/OrganizationIdentityProvidersResource.java
Outdated
Show resolved
Hide resolved
...g/keycloak/organization/authentication/authenticators/browser/OrganizationAuthenticator.java
Outdated
Show resolved
Hide resolved
...eycloak/organization/forms/login/freemarker/model/OrganizationAwareIdentityProviderBean.java
Show resolved
Hide resolved
...ain/java/org/keycloak/organization/admin/resource/OrganizationIdentityProvidersResource.java
Show resolved
Hide resolved
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.
Unreported flaky test detected, please review
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.client.ClientTypesTest#testUpdateClientWithClientType
org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testExchangeWithDynamicScopesEnabled
org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testClientExchange
org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testIntrospectTokenAfterImpersonation
org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testPublicClientNotAllowed
org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testExchangeUsingServiceAccount
org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testImpersonation
org.keycloak.testsuite.oauth.ClientTokenExchangeTest#testImpersonationUsingPublicClient
|
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.
Thank you @pedroigor for this PR. I didn't finished the whole review yet, especially authenticators and tests, but wanted to add some comments so you could look at them already.
I think it's unfortunate to manage identity providers in a way that we iterate over all realm IdP in various places, but I see it is the way how it was implemented in the past and there is not much we can do about it unless we re-do significant part of current implementation.
.../admin-client-jee/src/main/java/org/keycloak/admin/client/resource/OrganizationResource.java
Outdated
Show resolved
Hide resolved
List<IdentityProviderModel> brokers = organization.getIdentityProviders().toList(); | ||
|
||
if (identityProvider == null) { | ||
if (brokers.isEmpty()) { | ||
return false; | ||
} | ||
|
||
FederatedIdentityModel federatedIdentity = userProvider.getFederatedIdentity(realm, member, identityProvider.getAlias()); | ||
List<FederatedIdentityModel> federatedIdentities = userProvider.getFederatedIdentitiesStream(realm, member) | ||
.map(federatedIdentityModel -> realm.getIdentityProviderByAlias(federatedIdentityModel.getIdentityProvider())).filter(brokers::contains) | ||
.map(m -> userProvider.getFederatedIdentity(realm, member, m.getAlias())) | ||
.toList(); | ||
|
||
return federatedIdentity != null; | ||
return !federatedIdentities.isEmpty(); |
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.
Perhaps we could avoid collecting streams to collections here. Something like might do the trick,. wdyt?
return userProvider.getFederatedIdentitiesStream(realm, member)
.map(federatedIdentityModel -> realm.getIdentityProviderByAlias(federatedIdentityModel.getIdentityProvider()))
.filter(idp -> organization.getIdentityProviders().anyMatch(broker -> broker.getAlias().equals(idp.getAlias())))
.map(idp -> userProvider.getFederatedIdentity(realm, member, idp.getAlias()))
.anyMatch(Objects::nonNull);
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 would keep it as is. No need to resolve federated identities if there is no brokers associated with the orgs.
...ain/java/org/keycloak/organization/admin/resource/OrganizationIdentityProvidersResource.java
Outdated
Show resolved
Hide resolved
return response; | ||
} | ||
|
||
throw ErrorResponse.error("Identity provider not associated with the organization", Status.BAD_REQUEST); |
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 might be overlooking something, but shouldn't be there a different error message? If I understand it correctly in this point there was something wrong during removing IdP from organization.
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.
If not removed is because it wasn't associated. Looks correct to me.
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 is the problem with methods like removeIdentityProvider
that return a status boolean. As the caller of the method, you don't actually know what went wrong. Line 116 is assuming why the error happened. And for now it's correct. But later someone comes along to change that method and returns false
for some other reason. Then then assumption made in line 116 could be wrong.
I would suggest throwing an exception from inside the removeIdentityProvider
method instead of using a status boolean.
Also, at some point we need to localize error messages that could end up in the UI. But that may be a task for another day.
public Response delete() { | ||
Response response = super.delete(); | ||
|
||
if (organizationProvider.removeIdentityProvider(organization, broker)) { |
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.
organizationProvider.removeIdentityProvider(organization, broker)
should remove the IdP from realm ... shouldn't it be already removed by super.delete();
?
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.
Yeah, and we are removing both delete and update operations from this endpoint. Similarly, as we did for users. That also means only unlinking the broker from the org instead of removing it entirely.
For now, I would keep as is.
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 afraid I do not understand, my point was that the IdP is being removed twice, is that intentional?
services/src/main/java/org/keycloak/organization/admin/resource/OrganizationResource.java
Outdated
Show resolved
Hide resolved
@@ -87,7 +87,7 @@ public String getHelpText() { | |||
|
|||
@Override | |||
public boolean isUserSetupAllowed() { | |||
return false; | |||
return true; |
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.
Could you please elaborate a bit on this change?
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.
To avoid failing the flow when the authenticator is not "configured for" the current flow and user.
Yeah, fixing this is out of scope. |
Closes keycloak#28840 Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
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.
Approving based on @sguilhen's review. Thanks!
Closes #28840