From 653d09f39a8c9e7b6b428ea715f6c62d3cb52f7c Mon Sep 17 00:00:00 2001 From: Theresa Henze Date: Wed, 19 Jul 2023 17:15:53 +0200 Subject: [PATCH] trigger REMOVE_TOTP event on removal of an OTP credential Closes #15403 Signed-off-by: Theresa Henze --- .../java/org/keycloak/events/Details.java | 1 + .../account/AccountCredentialResource.java | 16 +++- .../resources/account/AccountRestService.java | 2 +- .../account/AccountRestServiceTest.java | 76 ++++++++++++++++++- 4 files changed, 91 insertions(+), 4 deletions(-) diff --git a/server-spi-private/src/main/java/org/keycloak/events/Details.java b/server-spi-private/src/main/java/org/keycloak/events/Details.java index db2db25c4e..857487b80f 100755 --- a/server-spi-private/src/main/java/org/keycloak/events/Details.java +++ b/server-spi-private/src/main/java/org/keycloak/events/Details.java @@ -87,6 +87,7 @@ public interface Details { String CREDENTIAL_TYPE = "credential_type"; String SELECTED_CREDENTIAL_ID = "selected_credential_id"; String AUTHENTICATION_ERROR_DETAIL = "authentication_error_detail"; + String CREDENTIAL_USER_LABEL = "credential_user_label"; String NOT_BEFORE = "not_before"; String NUM_FAILURES = "num_failures"; diff --git a/services/src/main/java/org/keycloak/services/resources/account/AccountCredentialResource.java b/services/src/main/java/org/keycloak/services/resources/account/AccountCredentialResource.java index 878687730b..5dc87e281c 100644 --- a/services/src/main/java/org/keycloak/services/resources/account/AccountCredentialResource.java +++ b/services/src/main/java/org/keycloak/services/resources/account/AccountCredentialResource.java @@ -11,12 +11,16 @@ import org.keycloak.credential.CredentialProvider; import org.keycloak.credential.CredentialProviderFactory; import org.keycloak.credential.CredentialTypeMetadata; import org.keycloak.credential.CredentialTypeMetadataContext; +import org.keycloak.events.Details; +import org.keycloak.events.EventBuilder; +import org.keycloak.events.EventType; import org.keycloak.models.AccountRoles; import org.keycloak.models.AuthenticationExecutionModel; import org.keycloak.models.AuthenticationFlowModel; import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; import org.keycloak.models.UserModel; +import org.keycloak.models.credential.OTPCredentialModel; import org.keycloak.models.utils.ModelToRepresentation; import org.keycloak.representations.account.CredentialMetadataRepresentation; import org.keycloak.representations.idm.CredentialRepresentation; @@ -61,11 +65,13 @@ public class AccountCredentialResource { private final UserModel user; private final RealmModel realm; private Auth auth; + private final EventBuilder event; - public AccountCredentialResource(KeycloakSession session, UserModel user, Auth auth) { + public AccountCredentialResource(KeycloakSession session, UserModel user, Auth auth, EventBuilder event) { this.session = session; this.user = user; this.auth = auth; + this.event = event; realm = session.getContext().getRealm(); } @@ -297,11 +303,17 @@ public class AccountCredentialResource { user.credentialManager().disableCredentialType(credentialType); return; } - throw new NotFoundException("Credential not found"); } checkIfCanBeRemoved(credential.getType()); user.credentialManager().removeStoredCredentialById(credentialId); + + if (OTPCredentialModel.TYPE.equals(credential.getType())) { + event.event(EventType.REMOVE_TOTP) + .detail(Details.SELECTED_CREDENTIAL_ID, credentialId) + .detail(Details.CREDENTIAL_USER_LABEL, credential.getUserLabel()); + event.success(); + } } diff --git a/services/src/main/java/org/keycloak/services/resources/account/AccountRestService.java b/services/src/main/java/org/keycloak/services/resources/account/AccountRestService.java index eb7e3a0d5c..a49a57fcb7 100755 --- a/services/src/main/java/org/keycloak/services/resources/account/AccountRestService.java +++ b/services/src/main/java/org/keycloak/services/resources/account/AccountRestService.java @@ -214,7 +214,7 @@ public class AccountRestService { @Path("/credentials") public AccountCredentialResource credentials() { checkAccountApiEnabled(); - return new AccountCredentialResource(session, user, auth); + return new AccountCredentialResource(session, user, auth, event); } @Path("/resources") diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountRestServiceTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountRestServiceTest.java index 2d5c531476..d5ba7b1ad6 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountRestServiceTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/account/AccountRestServiceTest.java @@ -44,6 +44,7 @@ import org.keycloak.models.credential.OTPCredentialModel; import org.keycloak.models.credential.PasswordCredentialModel; import org.keycloak.models.credential.WebAuthnCredentialModel; import org.keycloak.models.utils.DefaultAuthenticationFlows; +import org.keycloak.models.utils.ModelToRepresentation; import org.keycloak.representations.account.ClientRepresentation; import org.keycloak.representations.account.ConsentRepresentation; import org.keycloak.representations.account.ConsentScopeRepresentation; @@ -789,6 +790,79 @@ public class AccountRestServiceTest extends AbstractRestServiceTest { Assert.assertTrue(ObjectUtil.isEqualOrBothNull(otpCredential.getUserLabel(), otpCredentialLoaded.getUserLabel())); } + @Test + public void testRemoveCredentialWithNonOtpCredentialTriggeringNoEvent() throws IOException { + + List credentials = getCredentials(); + + UserResource user = ApiUtil.findUserByUsernameId(testRealm(), "test-user@localhost"); + assertEquals(1, user.credentials().size()); + + // Add non-OTP credential to the user through admin REST API + CredentialRepresentation nonOtpCredential = ModelToRepresentation.toRepresentation( + WebAuthnCredentialModel.create(WebAuthnCredentialModel.TYPE_TWOFACTOR, "foo", "foo", "foo", "foo", "foo", 2L, "foo")); + org.keycloak.representations.idm.UserRepresentation userRep = UserBuilder.edit(user.toRepresentation()) + .secret(nonOtpCredential) + .build(); + user.update(userRep); + + credentials = getCredentials(); + Assert.assertEquals(2, credentials.size()); + Assert.assertTrue(credentials.get(1).isRemoveable()); + + // Remove credential + CredentialRepresentation credential = user.credentials().stream() + .filter(credentialRep -> WebAuthnCredentialModel.TYPE_TWOFACTOR.equals(credentialRep.getType())) + .findFirst() + .get(); + Assert.assertNotNull(credential); + user.removeCredential(credential.getId()); + + events.poll(); + events.assertEmpty(); + } + + @Test + public void testRemoveCredentialWithOtpCredentialTriggeringEvent() throws IOException { + + List credentials = getCredentials(); + + UserResource user = ApiUtil.findUserByUsernameId(testRealm(), "test-user@localhost"); + assertEquals(1, user.credentials().size()); + + // Add OTP credential to the user through admin REST API + org.keycloak.representations.idm.UserRepresentation userRep = UserBuilder.edit(user.toRepresentation()) + .totpSecret("totpSecret") + .build(); + userRep.getCredentials().get(0).setUserLabel("totpCredentialUserLabel"); + user.update(userRep); + + credentials = getCredentials(); + Assert.assertEquals(2, credentials.size()); + Assert.assertTrue(credentials.get(1).isRemoveable()); + + // Remove credential + CredentialRepresentation otpCredential = user.credentials().stream() + .filter(credentialRep -> OTPCredentialModel.TYPE.equals(credentialRep.getType())) + .findFirst() + .get(); + SimpleHttp.Response response = SimpleHttp + .doDelete(getAccountUrl("credentials/" + otpCredential.getId()), httpClient) + .acceptJson() + .auth(tokenUtil.getToken()) + .asResponse(); + assertEquals(204, response.getStatus()); + + events.poll(); + events.expect(EventType.REMOVE_TOTP) + .client("account") + .user(user.toRepresentation().getId()) + .detail(Details.SELECTED_CREDENTIAL_ID, otpCredential.getId()) + .detail(Details.CREDENTIAL_USER_LABEL, "totpCredentialUserLabel") + .assertEvent(); + events.assertEmpty(); + } + // Send REST request to get all credential containers and credentials of current user private List getCredentials() throws IOException { return SimpleHttp.doGet(getAccountUrl("credentials"), httpClient) @@ -1688,7 +1762,7 @@ public class AccountRestServiceTest extends AbstractRestServiceTest { testRealm().update(realmRep); } } - + @EnableFeature(Profile.Feature.UPDATE_EMAIL) public void testEmailWhenUpdateEmailEnabled() throws Exception { reconnectAdminClient();