From 20d2cd00ccb067b8be480f291b32702002e4c92f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Fr=C4=85k?= Date: Tue, 14 Jun 2022 17:20:11 +0200 Subject: [PATCH 1/2] Fix endpoints in README.md --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index e8770393..a2f51856 100644 --- a/README.md +++ b/README.md @@ -163,7 +163,7 @@ The following example uses the default `master` realm but the demo will also wor 1. Run `mvn clean package` in the repository root 2. Navigate to `./docker` 3. Execute `docker-compose up` -4. Open [http://localhost:8024/auth/admin/](http://localhost:8024/auth/admin/) in a browser +4. Open [http://localhost:8024/admin/](http://localhost:8024/admin/) in a browser 5. Log in with the credentials: * User: `admin` @@ -195,7 +195,7 @@ them automatically. ![Sign out from admin account](readme-images/sign-out-from-admin.png) -2. Go to the [http://localhost:8024/auth/realms/master/account](http://localhost:8024/auth/realms/master/account) URI. +2. Go to the [http://localhost:8024/realms/master/account](http://localhost:8024/realms/master/account) URI. Click the `Sign in` button to login as an example user: ![Welcome to Keycloak account](readme-images/welcome-to-keycloak-account.png) @@ -216,7 +216,7 @@ Setting `requiredActions`, `groups`, `attributes` or `roles` is completely optio legacy system for illustration purposes only. 4. The example user is successfully migrated. Log in again as admin - ([http://localhost:8024/auth/admin/](http://localhost:8024/auth/admin/)) and navigate to `Users` to verify the + ([http://localhost:8024/admin/](http://localhost:8024/admin/)) and navigate to `Users` to verify the results: ![Realm user list](readme-images/realm-user-list.png) From 11d1da99b986bde257f671b967048f75c0177244 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Fr=C4=85k?= Date: Tue, 14 Jun 2022 17:21:24 +0200 Subject: [PATCH 2/2] Prompt the user to provide a new password if the legacy one does not meet the password policy --- .../integration/migrating_users.spec.js | 83 +++++++++++-- pom.xml | 8 ++ .../providers/rest/LegacyProvider.java | 44 ++++++- .../providers/rest/LegacyProviderTest.java | 112 ++++++++++++++---- 4 files changed, 212 insertions(+), 35 deletions(-) diff --git a/docker/e2e/cypress/integration/migrating_users.spec.js b/docker/e2e/cypress/integration/migrating_users.spec.js index f40d8187..d531126c 100644 --- a/docker/e2e/cypress/integration/migrating_users.spec.js +++ b/docker/e2e/cypress/integration/migrating_users.spec.js @@ -127,7 +127,11 @@ describe('user migration plugin', () => { beforeEach(() => { deleteEmails(); - deleteTestUserIfExists(); + signInAsAdmin(); + deleteTestUserIfExists().then(() => { + deletePasswordPoliciesIfExist() + .then(() => signOutViaUI()); + }); }); function deleteEmails() { @@ -137,23 +141,59 @@ describe('user migration plugin', () => { } function deleteTestUserIfExists() { - signInAsAdmin(); + cy.log("Deleting test user..."); cy.visit('/admin/master/console/#/realms/master/users'); + getAllUsers(); + + return cy.get('td').contains(LEGACY_USER_EMAIL) + .should('have.length.gte', 0).then(userElement => { + if (!userElement.length) { + return; + } + + cy.intercept('DELETE', '/admin/realms/master/users/**').as("userDelete"); + cy.wrap(userElement).parent().contains('Delete').click(); + cy.get('.modal-dialog button').contains('Delete').click(); + cy.wait('@userDelete'); + cy.get('.alert').should('contain', "Success"); + }); + } + + function getAllUsers() { cy.intercept('GET', '/admin/realms/master/users*').as("userGet"); cy.get('#viewAllUsers').click(); cy.wait('@userGet'); + cy.wait(1000); + } - cy.get('body').then($body => { - if ($body.find('td:contains("' + LEGACY_USER_EMAIL + '")').length > 0) { - cy.contains(LEGACY_USER_EMAIL).parent().contains('Delete').click(); - cy.get('.modal-dialog button').contains('Delete').click(); - cy.get('.alert').should('contain', "Success"); + function deletePasswordPoliciesIfExist() { + goToPasswordPoliciesPage(); + return deleteEveryPasswordPolicyAndSave(); + } + + function deleteEveryPasswordPolicyAndSave() { + cy.log("Deleting password policies..."); + return cy.get('td[ng-click*="removePolicy"]') + .should('have.length.gte', 0).then(btn => { + if (!btn.length) { + return; } - signOutViaUI(); + cy.wrap(btn).click({multiple: true}); + + cy.intercept('GET', '/admin/realms/master').as("masterPut"); + cy.get('button').contains('Save').click(); + cy.wait('@masterPut'); }); } - it('should migrate users', () => { + function goToPasswordPoliciesPage() { + cy.intercept('GET', '/admin/realms/master').as("masterGet"); + cy.visit('/admin/master/console/#/realms/master/authentication/password-policy'); + cy.wait('@masterGet'); + cy.get("h1").should('contain', 'Authentication'); + } + + it('should migrate user', () => { signInAsLegacyUser(); updateAccountInformation(); assertIsLoggedInAsLegacyUser(); @@ -237,4 +277,29 @@ describe('user migration plugin', () => { triggerPasswordReset(); resetPasswordViaEmail() }); + + it('should migrate user when password breaks policy', () => { + signInAsAdmin(); + addSpecialCharactersPasswordPolicy(); + signOutViaUI(); + + signInAsLegacyUser(); + provideNewPassword(); + updateAccountInformation(); + assertIsLoggedInAsLegacyUser(); + }); + + function addSpecialCharactersPasswordPolicy() { + cy.visit('/admin/master/console/#/realms/master/authentication/password-policy'); + let policyDropdownSelector = 'select[ng-model="selectedPolicy"]'; + cy.get(policyDropdownSelector).select('Special Characters'); + cy.get('button').contains('Save').click(); + cy.get('.alert').should('contain', "Your changes have been saved to the realm"); + } + + function provideNewPassword() { + cy.get('#password-new').type("pa$$word"); + cy.get('#password-confirm').type("pa$$word"); + cy.get("input").contains("Submit").click(); + } }); \ No newline at end of file diff --git a/pom.xml b/pom.xml index acab3475..8a464782 100644 --- a/pom.xml +++ b/pom.xml @@ -46,6 +46,14 @@ provided + + + org.keycloak + keycloak-server-spi-private + ${keycloak.version} + provided + + org.jboss.logging diff --git a/src/main/java/com/danielfrak/code/keycloak/providers/rest/LegacyProvider.java b/src/main/java/com/danielfrak/code/keycloak/providers/rest/LegacyProvider.java index 89c57492..60bc4aac 100644 --- a/src/main/java/com/danielfrak/code/keycloak/providers/rest/LegacyProvider.java +++ b/src/main/java/com/danielfrak/code/keycloak/providers/rest/LegacyProvider.java @@ -12,6 +12,8 @@ import org.keycloak.models.RealmModel; import org.keycloak.models.UserModel; import org.keycloak.models.credential.PasswordCredentialModel; +import org.keycloak.policy.PasswordPolicyManagerProvider; +import org.keycloak.policy.PolicyError; import org.keycloak.storage.UserStorageProvider; import org.keycloak.storage.user.UserLookupProvider; @@ -69,12 +71,18 @@ public boolean isValid(RealmModel realmModel, UserModel userModel, CredentialInp } var userIdentifier = getUserIdentifier(userModel); - if (legacyUserService.isPasswordValid(userIdentifier, input.getChallengeResponse())) { + + if (!legacyUserService.isPasswordValid(userIdentifier, input.getChallengeResponse())) { + return false; + } + + if (passwordDoesNotBreakPolicy(realmModel, userModel, input.getChallengeResponse())) { session.userCredentialManager().updateCredential(realmModel, userModel, input); - return true; + } else { + addUpdatePasswordAction(userModel, userIdentifier); } - return false; + return true; } private String getUserIdentifier(UserModel userModel) { @@ -83,6 +91,29 @@ private String getUserIdentifier(UserModel userModel) { return useUserId ? userModel.getId() : userModel.getUsername(); } + private boolean passwordDoesNotBreakPolicy(RealmModel realmModel, UserModel userModel, String password) { + PasswordPolicyManagerProvider passwordPolicyManagerProvider = session.getProvider( + PasswordPolicyManagerProvider.class); + PolicyError error = passwordPolicyManagerProvider + .validate(realmModel, userModel, password); + + return error == null; + } + + private void addUpdatePasswordAction(UserModel userModel, String userIdentifier) { + if (updatePasswordActionMissing(userModel)) { + LOG.infof("Could not use legacy password for user %s due to password policy." + + " Adding UPDATE_PASSWORD action.", + userIdentifier); + userModel.addRequiredAction(UserModel.RequiredAction.UPDATE_PASSWORD); + } + } + + private boolean updatePasswordActionMissing(UserModel userModel) { + return userModel.getRequiredActionsStream() + .noneMatch(s -> s.contains(UserModel.RequiredAction.UPDATE_PASSWORD.name())); + } + @Override public boolean supportsCredentialType(String s) { return supportedCredentialTypes.contains(s); @@ -105,11 +136,16 @@ public void close() { @Override public boolean updateCredential(RealmModel realm, UserModel user, CredentialInput input) { + severFederationLink(user); + return false; + } + + private void severFederationLink(UserModel user) { + LOG.info("Severing federation link for " + user.getUsername()); String link = user.getFederationLink(); if (link != null && !link.isBlank()) { user.setFederationLink(null); } - return false; } @Override diff --git a/src/test/java/com/danielfrak/code/keycloak/providers/rest/LegacyProviderTest.java b/src/test/java/com/danielfrak/code/keycloak/providers/rest/LegacyProviderTest.java index efd39c3b..90c97f8e 100644 --- a/src/test/java/com/danielfrak/code/keycloak/providers/rest/LegacyProviderTest.java +++ b/src/test/java/com/danielfrak/code/keycloak/providers/rest/LegacyProviderTest.java @@ -15,20 +15,20 @@ import org.keycloak.models.UserCredentialManager; import org.keycloak.models.UserModel; import org.keycloak.models.credential.PasswordCredentialModel; +import org.keycloak.policy.PasswordPolicyManagerProvider; +import org.keycloak.policy.PolicyError; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; import java.util.List; import java.util.Optional; +import java.util.stream.Stream; +import static com.danielfrak.code.keycloak.providers.rest.ConfigurationProperties.USE_USER_ID_FOR_CREDENTIAL_VERIFICATION; import static java.util.Collections.emptySet; -import static com.danielfrak.code.keycloak.providers.rest.ConfigurationProperties.*; import static org.junit.jupiter.api.Assertions.*; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.never; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; @ExtendWith(MockitoExtension.class) class LegacyProviderTest { @@ -53,13 +53,19 @@ class LegacyProviderTest { @Mock private ComponentModel model; + @Mock + private PasswordPolicyManagerProvider passwordPolicyManagerProvider; + @BeforeEach void setUp() { legacyProvider = new LegacyProvider(session, legacyUserService, userModelFactory, model); + + lenient().when(session.getProvider(PasswordPolicyManagerProvider.class)) + .thenReturn(passwordPolicyManagerProvider); } @Test - void getsUserByUsername() { + void shouldGetUserByUsername() { final String username = "user"; final LegacyUser user = new LegacyUser(); when(legacyUserService.findByUsername(username)) @@ -73,7 +79,7 @@ void getsUserByUsername() { } @Test - void returnsNullIfUserNotFoundByUsername() { + void shouldReturnNullIfUserNotFoundByUsername() { final String username = "user"; when(legacyUserService.findByUsername(username)) .thenReturn(Optional.empty()); @@ -84,7 +90,7 @@ void returnsNullIfUserNotFoundByUsername() { } @Test - void getsUserByEmail() { + void shouldGetUserByEmail() { final String email = "email"; final LegacyUser user = new LegacyUser(); when(legacyUserService.findByEmail(email)) @@ -98,7 +104,7 @@ void getsUserByEmail() { } @Test - void returnsNullIfUserNotFoundByEmail() { + void shouldReturnNullIfUserNotFoundByEmail() { final String username = "user"; when(legacyUserService.findByEmail(username)) .thenReturn(Optional.empty()); @@ -109,7 +115,7 @@ void returnsNullIfUserNotFoundByEmail() { } @Test - void isValidReturnsFalseOnWrongCredentialType() { + void isValidShouldReturnFalseOnWrongCredentialType() { var input = mock(CredentialInput.class); when(input.getType()) .thenReturn(CredentialModel.KERBEROS); @@ -120,7 +126,7 @@ void isValidReturnsFalseOnWrongCredentialType() { } @Test - void isValidReturnsFalseWhenInvalidCredentialValue() { + void isValidShouldReturnFalseWhenInvalidCredentialValue() { var input = mock(CredentialInput.class); when(input.getType()) .thenReturn(PasswordCredentialModel.TYPE); @@ -145,7 +151,8 @@ void isValidReturnsFalseWhenInvalidCredentialValue() { } @Test - void isValidReturnsTrueWhenUserValidated() { + void isValidShouldReturnTrueAndUpdateCredentialsWhenUserValidated() { + var userCredentialManager = mock(UserCredentialManager.class); var input = mock(CredentialInput.class); when(input.getType()) .thenReturn(PasswordCredentialModel.TYPE); @@ -165,15 +172,17 @@ void isValidReturnsTrueWhenUserValidated() { .thenReturn(true); when(session.userCredentialManager()) - .thenReturn(mock(UserCredentialManager.class)); + .thenReturn(userCredentialManager); var result = legacyProvider.isValid(realmModel, userModel, input); assertTrue(result); + verify(userCredentialManager).updateCredential(realmModel, userModel, input); } @Test - void isValidReturnsTrueWhenUserValidatedWithUserId() { + void isValidShouldReturnTrueAndUpdateCredentialsWhenUserValidatedWithUserId() { + var userCredentialManager = mock(UserCredentialManager.class); var input = mock(CredentialInput.class); when(input.getType()) .thenReturn(PasswordCredentialModel.TYPE); @@ -193,15 +202,74 @@ void isValidReturnsTrueWhenUserValidatedWithUserId() { .thenReturn(true); when(session.userCredentialManager()) - .thenReturn(mock(UserCredentialManager.class)); + .thenReturn(userCredentialManager); var result = legacyProvider.isValid(realmModel, userModel, input); assertTrue(result); + verify(userCredentialManager).updateCredential(realmModel, userModel, input); + } + + @Test + void isValidShouldReturnTrueAndNotUpdateCredentialsAndRequirePasswordChangeWhenUserValidatedButPasswordBreaksPolicy() { + var input = mock(CredentialInput.class); + when(input.getType()) + .thenReturn(PasswordCredentialModel.TYPE); + + MultivaluedHashMap config = new MultivaluedHashMap<>(); + config.put(USE_USER_ID_FOR_CREDENTIAL_VERIFICATION, List.of("true")); + when(model.getConfig()).thenReturn(config); + + final String userId = "1234567890"; + final String password = "password"; + + when(userModel.getId()) + .thenReturn(userId); + when(input.getChallengeResponse()) + .thenReturn(password); + when(legacyUserService.isPasswordValid(userId, password)) + .thenReturn(true); + when(passwordPolicyManagerProvider.validate(realmModel, userModel, password)) + .thenReturn(new PolicyError("someError")); + + var result = legacyProvider.isValid(realmModel, userModel, input); + + assertTrue(result); + verify(session, never()).userCredentialManager(); + verify(userModel).addRequiredAction(UserModel.RequiredAction.UPDATE_PASSWORD); + } + + @Test + void isValidShouldNotAddRequirePasswordActionWhenOneAlreadyExists() { + var input = mock(CredentialInput.class); + when(input.getType()) + .thenReturn(PasswordCredentialModel.TYPE); + + MultivaluedHashMap config = new MultivaluedHashMap<>(); + config.put(USE_USER_ID_FOR_CREDENTIAL_VERIFICATION, List.of("true")); + when(model.getConfig()).thenReturn(config); + + final String userId = "1234567890"; + final String password = "password"; + + when(userModel.getId()) + .thenReturn(userId); + when(input.getChallengeResponse()) + .thenReturn(password); + when(legacyUserService.isPasswordValid(userId, password)) + .thenReturn(true); + when(passwordPolicyManagerProvider.validate(realmModel, userModel, password)) + .thenReturn(new PolicyError("someError")); + when(userModel.getRequiredActionsStream()) + .thenReturn(Stream.of(UserModel.RequiredAction.UPDATE_PASSWORD.name())); + + legacyProvider.isValid(realmModel, userModel, input); + + verify(userModel, never()).addRequiredAction(UserModel.RequiredAction.UPDATE_PASSWORD); } @Test - void supportsPasswordCredentialType() { + void shouldSupportPasswordCredentialType() { assertTrue(legacyProvider.supportsCredentialType(PasswordCredentialModel.TYPE)); } @@ -218,7 +286,7 @@ void getUserByIdShouldThrowException() { } @Test - void removeFederationLinkWhenCredentialUpdates() { + void shouldRemoveFederationLinkWhenCredentialUpdates() { var input = mock(CredentialInput.class); when(userModel.getFederationLink()) .thenReturn("someId"); @@ -230,7 +298,7 @@ void removeFederationLinkWhenCredentialUpdates() { } @Test - void doNotRemoveFederationLinkWhenBlankAndCredentialUpdates() { + void shouldNotRemoveFederationLinkWhenBlankAndCredentialUpdates() { var input = mock(CredentialInput.class); when(userModel.getFederationLink()) .thenReturn(" "); @@ -242,7 +310,7 @@ void doNotRemoveFederationLinkWhenBlankAndCredentialUpdates() { } @Test - void doNotRemoveFederationLinkWhenNullAndCredentialUpdates() { + void shouldNotRemoveFederationLinkWhenNullAndCredentialUpdates() { var input = mock(CredentialInput.class); when(userModel.getFederationLink()) .thenReturn(null); @@ -254,19 +322,19 @@ void doNotRemoveFederationLinkWhenNullAndCredentialUpdates() { } @Test - void disableCredentialTypeDoesNothing() { + void disableCredentialTypeShouldDoNothing() { legacyProvider.disableCredentialType(realmModel, userModel, "someType"); Mockito.verifyNoInteractions(session, legacyUserService, userModelFactory, realmModel, userModel); } @Test - void closeDoesNothing() { + void closeShouldDoNothing() { legacyProvider.close(); Mockito.verifyNoInteractions(session, legacyUserService, userModelFactory, realmModel, userModel); } @Test - void getDisableableCredentialTypesAlwaysReturnsEmptySet() { + void getDisableableCredentialTypesShouldAlwaysReturnEmptySet() { assertEquals(emptySet(), legacyProvider.getDisableableCredentialTypes(realmModel, userModel)); } } \ No newline at end of file