OZG-3961 OZG-4082 do add client roles on update user; tiny cleanup

src/main/java/de/ozgcloud/operator/keycloak/user/KeycloakUserPreconditionService.java

@@ -38,20 +38,20 @@ class KeycloakUserPreconditionService {
 	@Autowired
 	private KeycloakGenericRemoteService keycloakGenericRemoteService;
 
-	Optional<String> getReconcilePreconditionErrors(OzgKeycloakUser user) {
+	public Optional<String> getReconcilePreconditionErrors(OzgKeycloakUser user) {
 
-		String namespace = user.getMetadata().getNamespace();
+		var namespace = user.getMetadata().getNamespace();
 
 		if (!keycloakGenericRemoteService.realmExists(namespace)) {
-			return Optional.of("Realm " + namespace + " does not yet exist");
+			return Optional.of(String.format("Realm %s does not yet exist", namespace));
 		}
 
-		Optional<String> clientErorr = clientsExists(user, namespace);
+		var clientErorr = clientsExists(user, namespace);
 		if (clientErorr.isPresent()) {
 			return clientErorr;
 		}
 
-		Optional<String> groupError = groupsExists(user, namespace);
+		var groupError = groupsExists(user, namespace);
 		if (groupError.isPresent()) {
 			return groupError;
 		}
@@ -64,7 +64,7 @@ class KeycloakUserPreconditionService {
 				.map(KeycloakUserSpecClientRole::getClientId)
 				.map(clientId -> keycloakGenericRemoteService.getByClientId(clientId, realm))
 				.filter(Optional::isEmpty)
-				.map(clientId -> "Client " + clientId + " does not yet exist")
+				.map(clientId -> String.format("Client %s does not yet exist", clientId))
 				.findAny();
 	}
 
@@ -72,7 +72,7 @@ class KeycloakUserPreconditionService {
 		return user.getSpec().getKeycloakUser().getGroups().stream()
 				.map(KeycloakUserSpecUserGroup::getName)
 				.filter(groupName -> !keycloakGenericRemoteService.groupExists(groupName, realm))
-				.map(groupName -> "Group " + groupName + " for realm " + realm + " does not yet exist")
+				.map(groupName -> String.format("Group %s for realm %s does not exist yet", groupName, realm))
 				.findAny();
 	}
 }

src/main/java/de/ozgcloud/operator/keycloak/user/KeycloakUserReconciler.java

@@ -24,7 +24,6 @@
 package de.ozgcloud.operator.keycloak.user;
 
 import java.time.Duration;
-import java.util.Optional;
 import java.util.logging.Level;
 
 import org.springframework.beans.factory.annotation.Autowired;
@@ -40,9 +39,9 @@ import io.javaoperatorsdk.operator.api.reconciler.Reconciler;
 import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
 import lombok.extern.java.Log;
 
+@Log
 @ControllerConfiguration
 @Component
-@Log
 public class KeycloakUserReconciler implements Reconciler<OzgKeycloakUser>, Cleaner<OzgKeycloakUser> {
 
 	@Autowired
@@ -53,54 +52,67 @@ public class KeycloakUserReconciler implements Reconciler<OzgKeycloakUser>, Clea
 
 	@Override
 	public UpdateControl<OzgKeycloakUser> reconcile(OzgKeycloakUser resource, Context<OzgKeycloakUser> context) {
-
 		try {
-			log.info("Reconciling KeycloakUser " + resource.getMetadata().getName());
-			String namespace = resource.getMetadata().getNamespace();
+			log.info(String.format("Reconciling user %s...", resource.getMetadata().getName()));
 
-			Optional<String> preconditionError = preconditionService.getReconcilePreconditionErrors(resource);
+			var preconditionError = preconditionService.getReconcilePreconditionErrors(resource);
 			if (preconditionError.isPresent()) {
 				return buildStatusInProgress(resource, preconditionError.get());
 			}
 
-			keycloakUserService.createOrUpdateUser(resource.getSpec(), namespace);
+			keycloakUserService.createOrUpdateUser(resource.getSpec(), resource.getMetadata().getNamespace());
 
-			return buildStatusOk(resource);
+			resource.setStatus(buildOzgKeycloakUserOkStatus());
+			return UpdateControl.updateStatus(resource);
 
 		} catch (Exception e) {
-			log.log(Level.SEVERE,
-					"Could not reconcile user " + resource.getMetadata().getName() + " in namespace " + resource.getMetadata().getNamespace() + ": "
-							+ e.getMessage(),
-					e);
-			resource.setStatus(OzgKeycloakUserStatus.builder().status(OzgCustomResourceStatus.ERROR).message(e.getMessage()).build());
-			return UpdateControl.updateStatus(resource).rescheduleAfter(Duration.ofSeconds(Config.RECONCILER_RETRY_SECONDS));
+			log.log(Level.SEVERE, String.format("Could not reconcile user %s for namespace %s: %s", resource.getMetadata().getName(),
+					resource.getMetadata().getNamespace(), e.getMessage()), e);
+
+			resource.setStatus(buildOzgKeycloakUserErrorStatus(e.getMessage()));
+			return createRescheduleUpdateControl(resource);
 		}
 	}
 
-	private UpdateControl<OzgKeycloakUser> buildStatusOk(OzgKeycloakUser resource) {
-		resource.setStatus(OzgKeycloakUserStatus.builder().status(OzgCustomResourceStatus.OK).message(null).build());
-		return UpdateControl.updateStatus(resource);
+	private OzgKeycloakUserStatus buildOzgKeycloakUserOkStatus() {
+		return buildOzgKeycloakUserStatus(OzgCustomResourceStatus.OK, null);
+	}
+
+	private OzgKeycloakUserStatus buildOzgKeycloakUserErrorStatus(String message) {
+		return buildOzgKeycloakUserStatus(OzgCustomResourceStatus.ERROR, message);
 	}
 
 	private UpdateControl<OzgKeycloakUser> buildStatusInProgress(OzgKeycloakUser resource, String errorMessage) {
-		log.log(Level.INFO,
-				"Could not yet reconcile user " + resource.getMetadata().getName() + " in namespace " + resource.getMetadata().getNamespace() + ": "
-						+ errorMessage);
-		resource.setStatus(OzgKeycloakUserStatus.builder().status(OzgCustomResourceStatus.IN_PROGRESS).message(errorMessage).build());
-		return UpdateControl.updateStatus(resource).rescheduleAfter(Duration.ofSeconds(Config.RECONCILER_RETRY_SECONDS));
+		log.log(Level.WARNING, String.format("Could not reconcile user %s in namespace %s: %s", resource.getMetadata().getName(),
+				resource.getMetadata().getNamespace(), errorMessage));
+
+		resource.setStatus(buildOzgKeycloakUserInProgressStatus(errorMessage));
+		return createRescheduleUpdateControl(resource);
 	}
 
-	@Override
-	public DeleteControl cleanup(OzgKeycloakUser crd, Context<OzgKeycloakUser> context) {
+	private OzgKeycloakUserStatus buildOzgKeycloakUserInProgressStatus(String message) {
+		return buildOzgKeycloakUserStatus(OzgCustomResourceStatus.IN_PROGRESS, message);
+	}
+
+	private OzgKeycloakUserStatus buildOzgKeycloakUserStatus(OzgCustomResourceStatus status, String message) {
+		return OzgKeycloakUserStatus.builder().status(status).message(message).build();
+	}
+
+	private UpdateControl<OzgKeycloakUser> createRescheduleUpdateControl(OzgKeycloakUser userResource) {
+		return UpdateControl.updateStatus(userResource).rescheduleAfter(Duration.ofSeconds(Config.RECONCILER_RETRY_SECONDS));
+	}
 
+	@Override
+	public DeleteControl cleanup(OzgKeycloakUser userResource, Context<OzgKeycloakUser> context) {
 		try {
-			log.info("Deleting KeycloakUser " + crd.getMetadata().getName());
+			log.info(String.format("Deleting KeycloakUser %s", userResource.getMetadata().getName()));
 
-			keycloakUserService.deleteUser(crd.getSpec(), crd.getMetadata().getNamespace());
+			keycloakUserService.deleteUser(userResource.getSpec(), userResource.getMetadata().getNamespace());
 
 			return DeleteControl.defaultDelete();
 		} catch (Exception e) {
-			log.log(Level.SEVERE, "Could not delete user " + crd.getMetadata().getName() + " in namespace " + crd.getMetadata().getNamespace(), e);
+			log.log(Level.SEVERE, String.format("Could not delete user %s in namespace %s", userResource.getMetadata().getName(),
+					userResource.getMetadata().getNamespace()), e);
 			return DeleteControl.defaultDelete();
 		}
 	}

src/main/java/de/ozgcloud/operator/keycloak/user/KeycloakUserRemoteService.java

@@ -26,8 +26,6 @@ package de.ozgcloud.operator.keycloak.user;
 import java.util.Arrays;
 import java.util.Optional;
 
-import javax.ws.rs.core.Response;
-
 import org.keycloak.admin.client.CreatedResponseUtil;
 import org.keycloak.admin.client.Keycloak;
 import org.keycloak.admin.client.resource.RealmResource;
@@ -50,30 +48,46 @@ class KeycloakUserRemoteService {
 	@Autowired
 	private KeycloakGenericRemoteService keycloakGenericRemoteService;
 
-	void createUser(UserRepresentation user, String realm) {
-
-		RealmResource realmResource = keycloak.realm(realm);
+	public void createUser(UserRepresentation user, String namespace) {
+		var realmResource = getRealm(namespace);
 
-		Response response = realmResource.users().create(user);
+		var response = realmResource.users().create(user);
 		KeycloakResultParser.parseCreatedResponse(response);
 
-		String userId = CreatedResponseUtil.getCreatedId(response);
+		var userId = CreatedResponseUtil.getCreatedId(response);
+
+		addClientRoles(userId, realmResource, namespace, user);
+	}
+
+	public Optional<UserRepresentation> getUserByName(String userName, String namespace) {
+		return getRealm(namespace).users().search(userName, true).stream().findFirst();
+	}
+
+	public void deleteUser(String userId, String namespace) {
+		getRealm(namespace).users().delete(userId);
+	}
+
+	public void updateUser(UserRepresentation user, String namespace) {
+		var realmResource = getRealm(namespace);
 
-		addClientRoles(userId, realmResource, realm, user);
+		realmResource.users().get(user.getId()).update(user);
+
+		addClientRoles(user.getId(), realmResource, namespace, user);
 	}
 
-	void addClientRoles(String userId, RealmResource realmResource, String realm, UserRepresentation user) {
+	private RealmResource getRealm(String namespace) {
+		return keycloak.realm(namespace);
+	}
 
-		user.getClientRoles().keySet()
-				.forEach(clientId -> {
-					ClientRepresentation appClient = getRealmClient(realmResource, clientId);
+	void addClientRoles(String userId, RealmResource realmResource, String namespace, UserRepresentation user) {
+		user.getClientRoles().keySet().forEach(clientId -> {
+			var realmClient = getRealmClient(realmResource, clientId);
 			user.getClientRoles().get(clientId).stream()
-							.map(clientRoleName -> keycloakGenericRemoteService.getClientRole(clientRoleName, appClient.getId(), realm)
+					.map(clientRoleName -> keycloakGenericRemoteService.getClientRole(clientRoleName, realmClient.getId(), namespace)
 							.orElseThrow(() -> new KeycloakException(
-											"Role " + clientRoleName + " not found for client with clientId " + clientId + " in realm " + realm)))
-							.forEach(clientRole -> addClientRoleToUser(clientRole, realmResource, userId, appClient));
+									"Role " + clientRoleName + " not found for client with clientId " + clientId + " in realm " + namespace)))
+					.forEach(clientRole -> addClientRoleToUser(clientRole, realmResource, userId, realmClient));
 		});
-
 	}
 
 	ClientRepresentation getRealmClient(RealmResource realmResource, String clientId) {
@@ -85,16 +99,4 @@ class KeycloakUserRemoteService {
 	void addClientRoleToUser(RoleRepresentation clientRole, RealmResource realmResource, String userId, ClientRepresentation appClient) {
 		realmResource.users().get(userId).roles().clientLevel(appClient.getId()).add(Arrays.asList(clientRole));
 	}
-
-	Optional<UserRepresentation> getUserByName(String username, String realm) {
-		return keycloak.realm(realm).users().search(username, true).stream().findFirst();
-	}
-
-	void deleteUser(String userId, String realm) {
-		keycloak.realm(realm).users().delete(userId);
-	}
-
-	void updateUser(UserRepresentation user, String realm) {
-		keycloak.realm(realm).users().get(user.getId()).update(user);
-	}
 }
\ No newline at end of file

src/main/java/de/ozgcloud/operator/keycloak/user/KeycloakUserService.java

@@ -37,13 +37,13 @@ class KeycloakUserService {
 	@Autowired
 	private KeycloakUserMapper userMapper;
 
-	void createOrUpdateUser(OzgKeycloakUserSpec userSpec, String namespace) {
+	public void createOrUpdateUser(OzgKeycloakUserSpec userSpec, String namespace) {
 		remoteService.getUserByName(userSpec.getKeycloakUser().getUsername(), namespace)
 				.ifPresentOrElse(existingUser -> remoteService.updateUser(userMapper.update(existingUser, userSpec), namespace),
 						() -> remoteService.createUser(userMapper.map(userSpec), namespace));
 	}
 
-	void deleteUser(OzgKeycloakUserSpec userSpec, String namespace) {
+	public void deleteUser(OzgKeycloakUserSpec userSpec, String namespace) {
 		Optional.of(userSpec)
 				.map(userMapper::map)
 				.flatMap(keycloakUser -> remoteService.getUserByName(keycloakUser.getUsername(), namespace))

src/test/java/de/ozgcloud/operator/keycloak/user/KeycloakUserRemoteServiceTest.java

@@ -35,16 +35,14 @@ import java.util.Optional;
 import javax.ws.rs.core.Response;
 
 import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
 import org.keycloak.admin.client.Keycloak;
-import org.keycloak.admin.client.resource.ClientResource;
 import org.keycloak.admin.client.resource.ClientsResource;
 import org.keycloak.admin.client.resource.RealmResource;
 import org.keycloak.admin.client.resource.RoleMappingResource;
-import org.keycloak.admin.client.resource.RoleResource;
 import org.keycloak.admin.client.resource.RoleScopeResource;
-import org.keycloak.admin.client.resource.RolesResource;
 import org.keycloak.admin.client.resource.UserResource;
 import org.keycloak.admin.client.resource.UsersResource;
 import org.keycloak.representations.idm.ClientRepresentation;
@@ -84,16 +82,10 @@ class KeycloakUserRemoteServiceTest {
 	@Mock
 	private ClientsResource clientsResource;
 	@Mock
-	private ClientResource clientResource;
-	@Mock
 	private ClientRepresentation clientRepresentation;
 	@Mock
 	private RoleRepresentation roleRepresentation;
 	@Mock
-	private RolesResource rolesResource;
-	@Mock
-	private RoleResource roleResource;
-	@Mock
 	private RoleScopeResource roleScopeResource;
 	@Mock
 	private RoleMappingResource roleMappingResource;
@@ -113,8 +105,6 @@ class KeycloakUserRemoteServiceTest {
 
 		@Test
 		void shouldThrowOnResponseError() {
-
-
 			when(response.getStatusInfo()).thenReturn(Response.Status.INTERNAL_SERVER_ERROR);
 
 			assertThrows(KeycloakException.class, () -> userRemoteService.createUser(USER_REPRESENTATION, REALM));
@@ -209,6 +199,7 @@ class KeycloakUserRemoteServiceTest {
 		}
 	}
 
+	@DisplayName("Update user")
 	@Nested
 	class TestUpdateUser {
 
@@ -218,7 +209,9 @@ class KeycloakUserRemoteServiceTest {
 		void init() {
 			when(keycloak.realm(REALM)).thenReturn(realmResource);
 			when(realmResource.users()).thenReturn(usersResource);
-			when(usersResource.get(USERID)).thenReturn(userResource);
+			when(usersResource.get(any())).thenReturn(userResource);
+
+			doNothing().when(userRemoteService).addClientRoles(any(), any(), eq(REALM), any());
 		}
 
 		@Test
@@ -230,6 +223,13 @@ class KeycloakUserRemoteServiceTest {
 
 			verify(userResource).update(user);
 		}
+
+		@Test
+		void shouldCallAddClientRoles() {
+			userRemoteService.updateUser(USER_REPRESENTATION, REALM);
+
+			verify(userRemoteService).addClientRoles(isNull(), eq(realmResource), eq(REALM), eq(USER_REPRESENTATION));
+		}
 	}
 
 	@Nested