From a7c8aa1dd3b38b2d3975e334e24ae37bb3d2a9d2 Mon Sep 17 00:00:00 2001 From: rmartinc Date: Thu, 10 Mar 2022 13:21:39 +0100 Subject: [PATCH] [#10616] Incorrect username logged for federated accounts (#10662) Closes #10616 --- .../resources/IdentityBrokerService.java | 3 + .../broker/KcOidcBrokerEventTest.java | 209 ++++++++++++++++++ 2 files changed, 212 insertions(+) create mode 100644 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerEventTest.java diff --git a/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java b/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java index 27e81cefde..c5ed900117 100755 --- a/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java +++ b/services/src/main/java/org/keycloak/services/resources/IdentityBrokerService.java @@ -25,6 +25,7 @@ import org.keycloak.authentication.AuthenticationProcessor; import org.keycloak.authentication.authenticators.broker.AbstractIdpAuthenticator; import org.keycloak.authentication.authenticators.broker.util.PostBrokerLoginConstants; import org.keycloak.authentication.authenticators.broker.util.SerializedBrokeredIdentityContext; +import org.keycloak.authentication.authenticators.browser.AbstractUsernameFormAuthenticator; import org.keycloak.broker.provider.AuthenticationRequest; import org.keycloak.broker.provider.BrokeredIdentityContext; import org.keycloak.broker.provider.IdentityBrokerException; @@ -546,6 +547,7 @@ public class IdentityBrokerService implements IdentityProvider.AuthenticationCal if (federatedUser == null) { logger.debugf("Federated user not found for provider '%s' and broker username '%s'", providerId, context.getUsername()); + authenticationSession.setAuthNote(AbstractUsernameFormAuthenticator.ATTEMPTED_USERNAME, context.getUsername()); String username = context.getModelUsername(); if (username == null) { @@ -592,6 +594,7 @@ public class IdentityBrokerService implements IdentityProvider.AuthenticationCal return Response.status(302).location(redirect).build(); } else { + authenticationSession.setAuthNote(AbstractUsernameFormAuthenticator.ATTEMPTED_USERNAME, federatedUser.getUsername()); Response response = validateUser(authenticationSession, federatedUser, realmModel); if (response != null) { return response; diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerEventTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerEventTest.java new file mode 100644 index 0000000000..a9c4ec2c61 --- /dev/null +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/broker/KcOidcBrokerEventTest.java @@ -0,0 +1,209 @@ +/* + * Copyright 2022 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.keycloak.testsuite.broker; + +import java.util.List; +import org.hamcrest.Matchers; +import org.junit.Assert; +import org.junit.Rule; +import org.junit.Test; +import org.keycloak.admin.client.resource.RealmResource; +import org.keycloak.events.Details; +import org.keycloak.events.EventType; +import org.keycloak.representations.idm.UserRepresentation; +import org.keycloak.testsuite.AssertEvents; + +/** + * Simple test to check the events after a broker login using OIDC. It also + * tests that the event username is not wrong after a form login error + * (Issue #10616). + * + * @author rmartinc + */ +public final class KcOidcBrokerEventTest extends AbstractBrokerTest { + + @Rule + public AssertEvents events = new AssertEvents(this); + + @Override + protected BrokerConfiguration getBrokerConfiguration() { + return KcOidcBrokerConfiguration.INSTANCE; + } + + private void checkLoginEvents(RealmResource providerRealm, RealmResource consumerRealm, String providerUserId, String consumerUserId) { + events.expect(EventType.LOGIN) + .realm(providerRealm.toRepresentation().getId()) + .user(providerUserId) + .client(bc.getIDPClientIdInProviderRealm()) + .session(Matchers.any(String.class)) + .detail(Details.USERNAME, bc.getUserLogin()) + .assertEvent(); + + events.expect(EventType.CODE_TO_TOKEN) + .session(Matchers.any(String.class)) + .realm(providerRealm.toRepresentation().getId()) + .user(providerUserId) + .client(bc.getIDPClientIdInProviderRealm()) + .assertEvent(); + + events.expect(EventType.USER_INFO_REQUEST) + .session(Matchers.any(String.class)) + .realm(providerRealm.toRepresentation().getId()) + .user(providerUserId) + .client(bc.getIDPClientIdInProviderRealm()) + .assertEvent(); + + events.expect(EventType.LOGIN) + .realm(consumerRealm.toRepresentation().getId()) + .client("account") + .user(consumerUserId == null? Matchers.any(String.class) : Matchers.is(consumerUserId)) + .session(Matchers.any(String.class)) + .detail(Details.USERNAME, bc.getUserLogin()) + .detail(Details.IDENTITY_PROVIDER_USERNAME, bc.getUserLogin()) + .detail(Details.IDENTITY_PROVIDER, bc.getIDPAlias()) + .assertEvent(); + + events.clear(); + } + + private void doALoginError(RealmResource consumerRealm) { + events.clear(); + + // navigate to the account url of the consumer realm + driver.navigate().to(getAccountUrl(BrokerTestTools.getConsumerRoot(), bc.consumerRealmName())); + + // Do a wrong login with a user that does not exist + loginPage.login("wrong-user", "wrong-password"); + + events.expect(EventType.LOGIN_ERROR) + .realm(consumerRealm.toRepresentation().getId()) + .user((String) null) + .client("account") + .session((String) null) + .detail(Details.USERNAME, "wrong-user") + .error("user_not_found") + .assertEvent(); + + events.clear(); + } + + @Override + protected void loginUser() { + RealmResource providerRealm = adminClient.realm(bc.providerRealmName()); + RealmResource consumerRealm = adminClient.realm(bc.consumerRealmName()); + UserRepresentation providerUser = providerRealm.users().search(bc.getUserLogin()).iterator().next(); + events.clear(); + + super.loginUser(); + + checkLoginEvents(providerRealm, consumerRealm, providerUser.getId(), null); + } + + private void loginUserAfterError() { + RealmResource providerRealm = adminClient.realm(bc.providerRealmName()); + RealmResource consumerRealm = adminClient.realm(bc.consumerRealmName()); + UserRepresentation providerUser = providerRealm.users().search(bc.getUserLogin()).iterator().next(); + + doALoginError(consumerRealm); + + logInWithBroker(bc); + + BrokerTestTools.waitForPage(driver, "update account information", false); + updateAccountInformationPage.assertCurrent(); + Assert.assertTrue("We must be on correct realm right now", + driver.getCurrentUrl().contains("/auth/realms/" + bc.consumerRealmName() + "/")); + + log.debug("Updating info on updateAccount page"); + updateAccountInformationPage.updateAccountInformation(bc.getUserLogin(), bc.getUserEmail(), "Firstname", "Lastname"); + + List users = consumerRealm.users().search(bc.getUserLogin()); + Assert.assertEquals("There must be one user", 1, users.size()); + UserRepresentation consumerUser = users.iterator().next(); + Assert.assertEquals(bc.getUserEmail(), consumerUser.getEmail()); + + checkLoginEvents(providerRealm, consumerRealm, providerUser.getId(), consumerUser.getId()); + } + + @Override + protected void testSingleLogout() { + RealmResource providerRealm = adminClient.realm(bc.providerRealmName()); + UserRepresentation providerUser = providerRealm.users().search(bc.getUserLogin()).iterator().next(); + events.clear(); + + super.testSingleLogout(); + + events.expect(EventType.LOGOUT) + .realm(providerRealm.toRepresentation().getId()) + .user(providerUser.getId()) + .client((String) null) + .session(Matchers.any(String.class)) + .assertEvent(); + + events.clear(); + } + + @Test + @Override + public void loginWithExistingUser() { + // first login to execute the first login flow and create/link the user + testLogInAsUserInIDP(); + + RealmResource providerRealm = adminClient.realm(bc.providerRealmName()); + RealmResource consumerRealm = adminClient.realm(bc.consumerRealmName()); + UserRepresentation providerUser = providerRealm.users().search(bc.getUserLogin()).iterator().next(); + UserRepresentation consumerUser = consumerRealm.users().search(bc.getUserLogin()).iterator().next(); + Integer userCount = adminClient.realm(bc.consumerRealmName()).users().count(); + + // now do the second login + driver.navigate().to(getAccountUrl(BrokerTestTools.getConsumerRoot(), bc.consumerRealmName())); + logInWithBroker(bc); + + Assert.assertEquals(accountPage.buildUri().toASCIIString().replace("master", "consumer") + "/", driver.getCurrentUrl()); + Assert.assertEquals(userCount, adminClient.realm(bc.consumerRealmName()).users().count()); + + checkLoginEvents(providerRealm, consumerRealm, providerUser.getId(), consumerUser.getId()); + } + + @Test + public void testLogInAsUserInIDPAfterError() { + loginUserAfterError(); + testSingleLogout(); + } + + @Test + public void loginWithExistingUserAfterError() { + // first login to execute the first login flow and create/link the user + testLogInAsUserInIDP(); + + RealmResource providerRealm = adminClient.realm(bc.providerRealmName()); + RealmResource consumerRealm = adminClient.realm(bc.consumerRealmName()); + UserRepresentation providerUser = providerRealm.users().search(bc.getUserLogin()).iterator().next(); + UserRepresentation consumerUser = consumerRealm.users().search(bc.getUserLogin()).iterator().next(); + Integer userCount = adminClient.realm(bc.consumerRealmName()).users().count(); + + doALoginError(consumerRealm); + + // now perform the login via the broker + logInWithBroker(bc); + + Assert.assertEquals(accountPage.buildUri().toASCIIString().replace("master", "consumer") + "/", driver.getCurrentUrl()); + Assert.assertEquals(userCount, adminClient.realm(bc.consumerRealmName()).users().count()); + + checkLoginEvents(providerRealm, consumerRealm, providerUser.getId(), consumerUser.getId()); + } +}