Merge pull request 'OZG-3961 OZG-4083 fix passwort NPE, create password only...

Merge pull request 'OZG-3961 OZG-4083 fix passwort NPE, create password only if unset' (#2) from OZG-3961-create-user-password into master

Reviewed-on: https://git.ozg-sh.de/mgm/ozgcloud-user-operator/pulls/2

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

@@ -24,16 +24,11 @@
 package de.ozgcloud.operator.keycloak.user;
 
 import java.util.Optional;
-import java.util.logging.Level;
 
-import org.apache.commons.lang3.RandomStringUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Component;
 
-import lombok.extern.java.Log;
-
-@Log
 @Component
 class KeycloakUserService {
 
@@ -47,14 +42,10 @@ class KeycloakUserService {
 	private KeycloakUserMapper userMapper;
 
 	public void createOrUpdateUser(OzgKeycloakUserSpec userSpec, String namespace) {
-		if (!userSecretService.exists(userSpec, namespace)) {
-			log.log(Level.INFO, "Update password...");
-			var userPassword = userSpec.getKeycloakUser().getPassword();
-			var password = StringUtils.isEmpty(userPassword) ? generatePassword() : userPassword;
-			userSpec.getKeycloakUser().setPassword(password);
 
-			log.log(Level.INFO, "Create secret for user: " + userSpec.getKeycloakUser().getUsername());
-			userSecretService.create(userSpec, namespace);
+		if (userHasNoPassword(userSpec, namespace)) {
+			var secret = userSecretService.getOrCreateClusterSecret(userSpec, namespace);
+			userSpec.getKeycloakUser().setPassword(userSecretService.getPasswordFromSecret(secret));
 		}
 
 		remoteService.getUserByName(userSpec.getKeycloakUser().getUsername(), namespace)
@@ -62,10 +53,8 @@ class KeycloakUserService {
 						() -> remoteService.createUser(userMapper.map(userSpec), namespace));
 	}
 
-	String generatePassword() {
-		var upperCaseCharacter = RandomStringUtils.randomAlphabetic(1).toUpperCase();
-		var randomString = RandomStringUtils.randomAlphanumeric(7);
-		return upperCaseCharacter + randomString;
+	boolean userHasNoPassword(OzgKeycloakUserSpec userSpec, String namespace) {
+		return StringUtils.isEmpty(userSpec.getKeycloakUser().getPassword());
 	}
 
 	public void deleteUser(OzgKeycloakUserSpec userSpec, String namespace) {

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

 package de.ozgcloud.operator.keycloak.user;
 
+import org.apache.commons.lang3.RandomStringUtils;
 import org.springframework.stereotype.Component;
 
 import de.ozgcloud.operator.keycloak.user.OzgKeycloakUserSpec.KeycloakUserSpecUser;
@@ -19,10 +20,16 @@ class UserSecretBuilder {
 				.withType(SECRET_TYPE)
 				.withMetadata(createMetaData(name, namespace))
 				.addToStringData(SECRET_NAME_FIELD, userSpec.getUsername())
-				.addToStringData(SECRET_PASSWORD_FIELD, userSpec.getPassword())
+				.addToStringData(SECRET_PASSWORD_FIELD, generatePassword())
 				.build();
 	}
 
+	String generatePassword() {
+		var upperCaseCharacter = RandomStringUtils.randomAlphabetic(1).toUpperCase();
+		var randomString = RandomStringUtils.randomAlphanumeric(7);
+		return upperCaseCharacter + randomString;
+	}
+
 	private ObjectMeta createMetaData(String name, String namespace) {
 		var metadata = new ObjectMeta();
 		metadata.setName(name);

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

+/*
+ * Copyright (C) 2022 Das Land Schleswig-Holstein vertreten durch den
+ * Ministerpräsidenten des Landes Schleswig-Holstein
+ * Staatskanzlei
+ * Abteilung Digitalisierung und zentrales IT-Management der Landesregierung
+ *
+ * Lizenziert unter der EUPL, Version 1.2 oder - sobald
+ * diese von der Europäischen Kommission genehmigt wurden -
+ * Folgeversionen der EUPL ("Lizenz");
+ * Sie dürfen dieses Werk ausschließlich gemäß
+ * dieser Lizenz nutzen.
+ * Eine Kopie der Lizenz finden Sie hier:
+ *
+ * https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12
+ *
+ * Sofern nicht durch anwendbare Rechtsvorschriften
+ * gefordert oder in schriftlicher Form vereinbart, wird
+ * die unter der Lizenz verbreitete Software "so wie sie
+ * ist", OHNE JEGLICHE GEWÄHRLEISTUNG ODER BEDINGUNGEN -
+ * ausdrücklich oder stillschweigend - verbreitet.
+ * Die sprachspezifischen Genehmigungen und Beschränkungen
+ * unter der Lizenz sind dem Lizenztext zu entnehmen.
+ */
+
+package de.ozgcloud.operator.keycloak.user;
+
+import java.io.IOException;
+
+import org.keycloak.common.util.Base64;
+import org.springframework.stereotype.Component;
+
+import io.fabric8.kubernetes.api.model.Secret;
+
+@Component
+class UserSecretReader {
+
+	public String getPasswortFromSecret(Secret secret) {
+		String encodedPassword = secret.getData().get(UserSecretBuilder.SECRET_PASSWORD_FIELD);
+		return decode(encodedPassword, secret);
+	}
+
+	private String decode(String encodedPassword, Secret secret) {
+		try {
+			return new String(Base64.decode(encodedPassword));
+		} catch (IOException e) {
+			throw new RuntimeException("Could not decode content from secret (base64) for secret " + secret.getFullResourceName());
+		}
+	}
+}

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

 package de.ozgcloud.operator.keycloak.user;
 
-import java.util.Objects;
+import java.util.Optional;
 
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.stereotype.Component;
@@ -17,19 +17,19 @@ class UserSecretService {
 	@Autowired
 	private UserSecretBuilder secretBuilder;
 	@Autowired
+	private UserSecretReader secretReader;
+	@Autowired
 	private KubernetesRemoteService kubernetesRemoteService;
 
-	public boolean exists(OzgKeycloakUserSpec userSpec, String namespace) {
-		return Objects.nonNull(getUserSecret(userSpec, namespace).get());
-	}
-
-	public void create(OzgKeycloakUserSpec userSpec, String namespace) {
+	public Secret create(OzgKeycloakUserSpec userSpec, String namespace) {
 		var secretName = userNameConverter.toSecretName(userSpec.getKeycloakUser());
 
 		var credentialsSecret = secretBuilder.build(secretName, userSpec.getKeycloakUser(), namespace);
 
 		var adapter = createResourceAdpater(getUserSecret(userSpec, namespace));
 		adapter.create(credentialsSecret);
+
+		return credentialsSecret;
 	}
 
 	ResourceAdapter<Secret> createResourceAdpater(Resource<Secret> secretResource) {
@@ -41,4 +41,13 @@ class UserSecretService {
 
 		return kubernetesRemoteService.getSecret(namespace, secretName);
 	}
+
+	public String getPasswordFromSecret(Secret secret) {
+		return secretReader.getPasswortFromSecret(secret);
+	}
+
+	public Secret getOrCreateClusterSecret(OzgKeycloakUserSpec userSpec, String namespace) {
+		return Optional.ofNullable(getUserSecret(userSpec, namespace)).map(Resource::get)
+				.orElseGet(() -> create(userSpec, namespace));
+	}
 }

src/test/java/de/ozgcloud/operator/keycloak/client/KeycloakLivelTest.java

@@ -67,6 +67,7 @@ class KeycloakLivelTest {
 //		remoteService.updateClientRole(remoteRole.get(), createClient().getClientId(), realm);
 	}
 
+	@SuppressWarnings("unused")
 	private RoleRepresentation createRoles() {
 		return mapper.mapRole(OzgKeycloakClientSpecTestFactory.ROLE1);
 	}

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

@@ -74,65 +74,57 @@ class KeycloakUserServiceTest {
 
 		private final OzgKeycloakUserSpec userSpec = OzgKeycloakUserSpecTestFactory.create();
 
-		@DisplayName("on missing secret")
+		@DisplayName("user has no password")
 		@Nested
-		class TestOnMissingSecret {
+		class TestOnUserHasNoPassword {
 
 			@BeforeEach
 			void mock() {
-				when(userSecretService.exists(any(), any())).thenReturn(false);
+				doReturn(true).when(service).userHasNoPassword(any(), eq(TEST_NAMESPACE));
 			}
 
 			@Test
-			void shouldCreateSecretIfNotExists() {
+			void shouldGetOrCreateClusterSecret() {
 				service.createOrUpdateUser(userSpec, TEST_NAMESPACE);
 
-				verify(userSecretService).create(userSpec, TEST_NAMESPACE);
+				verify(userSecretService).getOrCreateClusterSecret(userSpec, TEST_NAMESPACE);
 			}
 
 			@Test
-			void shouldUpdatePasswordIfNoExists() {
+			void shouldUpdateUserPassword() {
 				var userWithoutPassword = OzgKeycloakUserSpecTestFactory.createBuilder()
 						.keycloakUser(KeycloakUserSpecUserTestFactory.createBuiler().password(StringUtils.EMPTY).build()).build();
+				when(userSecretService.getPasswordFromSecret(any())).thenReturn(KeycloakUserSpecUserTestFactory.PASSWORD);
 
 				service.createOrUpdateUser(userWithoutPassword, TEST_NAMESPACE);
 
 				verify(userMapper).map(ozgKeycloakUserSpecCaptor.capture());
-				assertThat(ozgKeycloakUserSpecCaptor.getValue().getKeycloakUser().getPassword()).isNotEmpty();
+				assertThat(ozgKeycloakUserSpecCaptor.getValue().getKeycloakUser().getPassword()).isEqualTo(KeycloakUserSpecUserTestFactory.PASSWORD);
 			}
 		}
 
-		@DisplayName("generate password")
+		@DisplayName("on user has password")
 		@Nested
-		class TestGeneratePassword {
+		class TestOnUserHasPassword {
 
-			@Test
-			void shouldHaveSize() {
-				var password = service.generatePassword();
-
-				assertThat(password).hasSize(8);
-			}
-
-			@Test
-			void shouldHaveUpperCaseLetterAtFirst() {
-				var password = service.generatePassword();
-
-				assertThat(StringUtils.substring(password, 0, 1)).isUpperCase();
+			@BeforeEach
+			void mock() {
+				doReturn(false).when(service).userHasNoPassword(any(), any());
 			}
 
 			@Test
-			void shouldContainsAlphanumericOnly() {
-				var password = service.generatePassword();
+			void shouldNotReadSecretFromCluster() {
+				service.createOrUpdateUser(userSpec, TEST_NAMESPACE);
 
-				assertThat(password).isAlphanumeric();
+				verify(userSecretService, never()).create(userSpec, TEST_NAMESPACE);
 			}
 		}
 
 		@Test
-		void shouldVerifiySecretExists() {
+		void shouldCallUserHasNoPassword() {
 			service.createOrUpdateUser(userSpec, TEST_NAMESPACE);
 
-			verify(userSecretService).exists(userSpec, TEST_NAMESPACE);
+			verify(service).userHasNoPassword(userSpec, TEST_NAMESPACE);
 		}
 
 		@Test
@@ -175,6 +167,36 @@ class KeycloakUserServiceTest {
 		}
 	}
 
+	@DisplayName("Test user has no password")
+	@Nested
+	class TestUserHasNoPassword {
+
+		@Test
+		void testUserHasNoPasswordTrue() {
+			OzgKeycloakUserSpec user = OzgKeycloakUserSpecTestFactory.create();
+
+			boolean userHasNoPassword = service.userHasNoPassword(user, TEST_NAMESPACE);
+
+			assertThat(userHasNoPassword).isFalse();
+		}
+
+		@Test
+		void testUserHasNoPasswordFalse() {
+			OzgKeycloakUserSpec user = createUserWithoutPassword();
+
+			boolean userHasNoPassword = service.userHasNoPassword(user, TEST_NAMESPACE);
+
+			assertThat(userHasNoPassword).isTrue();
+		}
+
+		private OzgKeycloakUserSpec createUserWithoutPassword() {
+			return OzgKeycloakUserSpecTestFactory.createBuilder()
+					.keycloakUser(KeycloakUserSpecUserTestFactory.createBuiler()
+							.password(null).build())
+					.build();
+		}
+	}
+
 	@Nested
 	class TestDeleteUser {
 

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

+/*
+ * Copyright (C) 2022 Das Land Schleswig-Holstein vertreten durch den
+ * Ministerpräsidenten des Landes Schleswig-Holstein
+ * Staatskanzlei
+ * Abteilung Digitalisierung und zentrales IT-Management der Landesregierung
+ *
+ * Lizenziert unter der EUPL, Version 1.2 oder - sobald
+ * diese von der Europäischen Kommission genehmigt wurden -
+ * Folgeversionen der EUPL ("Lizenz");
+ * Sie dürfen dieses Werk ausschließlich gemäß
+ * dieser Lizenz nutzen.
+ * Eine Kopie der Lizenz finden Sie hier:
+ *
+ * https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12
+ *
+ * Sofern nicht durch anwendbare Rechtsvorschriften
+ * gefordert oder in schriftlicher Form vereinbart, wird
+ * die unter der Lizenz verbreitete Software "so wie sie
+ * ist", OHNE JEGLICHE GEWÄHRLEISTUNG ODER BEDINGUNGEN -
+ * ausdrücklich oder stillschweigend - verbreitet.
+ * Die sprachspezifischen Genehmigungen und Beschränkungen
+ * unter der Lizenz sind dem Lizenztext zu entnehmen.
+ */
+
+package de.ozgcloud.operator.keycloak.user;
+
+import java.util.UUID;
+
+import io.fabric8.kubernetes.api.model.Secret;
+import io.fabric8.kubernetes.api.model.SecretBuilder;
+
+public class SecretTestFactory {
+
+	public static final String PASSWORD = UUID.randomUUID().toString();
+
+	public static Secret create() {
+		return createBuilder().build();
+	}
+
+	private static SecretBuilder createBuilder() {
+		return new SecretBuilder();
+	}
+}

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

 package de.ozgcloud.operator.keycloak.user;
 
 import static org.assertj.core.api.Assertions.*;
+import static org.mockito.Mockito.*;
 
+import org.apache.commons.lang3.StringUtils;
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
+import org.mockito.Spy;
 
 import de.ozgcloud.operator.keycloak.user.OzgKeycloakUserSpec.KeycloakUserSpecUser;
 
@@ -13,7 +16,8 @@ public class UserSecretBuilderTest {
 	private final static String NAME = "dummyName";
 	private final static String NAMESPACE = "dummyNamespace";
 
-	private UserSecretBuilder builder = new UserSecretBuilder();
+	@Spy
+	private UserSecretBuilder builder;
 
 	@DisplayName("Build")
 	@Nested
@@ -35,12 +39,20 @@ public class UserSecretBuilderTest {
 			assertThat(secret.getStringData()).containsEntry(UserSecretBuilder.SECRET_NAME_FIELD, KeycloakUserSpecUserTestFactory.USERNAME);
 		}
 
+		@Test
+		void shouldCallGeneratePassword() {
+			builder.build(NAME, userSpec, NAMESPACE);
+
+			verify(builder).generatePassword();
+		}
+
 		@Test
 		void shouldHavePassword() {
+			doReturn(SecretTestFactory.PASSWORD).when(builder).generatePassword();
+
 			var secret = builder.build(NAME, userSpec, NAMESPACE);
 
-			assertThat(secret.getStringData()).containsEntry(UserSecretBuilder.SECRET_PASSWORD_FIELD,
-					KeycloakUserSpecUserTestFactory.PASSWORD);
+			assertThat(secret.getStringData()).containsEntry(UserSecretBuilder.SECRET_PASSWORD_FIELD, SecretTestFactory.PASSWORD);
 		}
 
 		@DisplayName("metadata")
@@ -61,5 +73,31 @@ public class UserSecretBuilderTest {
 				assertThat(secret.getMetadata().getNamespace()).isEqualTo(NAMESPACE);
 			}
 		}
+
+		@DisplayName("generate password")
+		@Nested
+		class TestGeneratePassword {
+
+			@Test
+			void shouldHaveSize() {
+				var password = builder.generatePassword();
+
+				assertThat(password).hasSize(8);
+			}
+
+			@Test
+			void shouldHaveUpperCaseLetterAtFirst() {
+				var password = builder.generatePassword();
+
+				assertThat(StringUtils.substring(password, 0, 1)).isUpperCase();
+			}
+
+			@Test
+			void shouldContainsAlphanumericOnly() {
+				var password = builder.generatePassword();
+
+				assertThat(password).isAlphanumeric();
+			}
+		}
 	}
 }

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

+/*
+ * Copyright (C) 2022 Das Land Schleswig-Holstein vertreten durch den
+ * Ministerpräsidenten des Landes Schleswig-Holstein
+ * Staatskanzlei
+ * Abteilung Digitalisierung und zentrales IT-Management der Landesregierung
+ *
+ * Lizenziert unter der EUPL, Version 1.2 oder - sobald
+ * diese von der Europäischen Kommission genehmigt wurden -
+ * Folgeversionen der EUPL ("Lizenz");
+ * Sie dürfen dieses Werk ausschließlich gemäß
+ * dieser Lizenz nutzen.
+ * Eine Kopie der Lizenz finden Sie hier:
+ *
+ * https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12
+ *
+ * Sofern nicht durch anwendbare Rechtsvorschriften
+ * gefordert oder in schriftlicher Form vereinbart, wird
+ * die unter der Lizenz verbreitete Software "so wie sie
+ * ist", OHNE JEGLICHE GEWÄHRLEISTUNG ODER BEDINGUNGEN -
+ * ausdrücklich oder stillschweigend - verbreitet.
+ * Die sprachspezifischen Genehmigungen und Beschränkungen
+ * unter der Lizenz sind dem Lizenztext zu entnehmen.
+ */
+
+package de.ozgcloud.operator.keycloak.user;
+
+import static org.assertj.core.api.Assertions.*;
+
+import java.util.Base64;
+import java.util.Map;
+
+import org.junit.jupiter.api.Test;
+import org.mockito.Spy;
+
+import io.fabric8.kubernetes.api.model.Secret;
+import io.fabric8.kubernetes.api.model.SecretBuilder;
+
+class UserSecretReaderTest {
+
+	@Spy
+	private UserSecretReader reader;
+
+	@Test
+	void shouldReturnPasssowrd() {
+		Secret secret = buildSecret();
+
+		String password = reader.getPasswortFromSecret(secret);
+
+		assertThat(password).isEqualTo(SecretTestFactory.PASSWORD);
+	}
+
+	private Secret buildSecret() {
+		return new SecretBuilder()
+				.addToData(Map.of(UserSecretBuilder.SECRET_PASSWORD_FIELD,
+						encodeStringBase64(SecretTestFactory.PASSWORD)))
+				.build();
+	}
+
+	private String encodeStringBase64(String string) {
+		return Base64.getEncoder().encodeToString(string.getBytes());
+	}
+}

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

@@ -28,52 +28,10 @@ class UserSecretServiceTest {
 	@Mock
 	private UserSecretBuilder secretBuilder;
 	@Mock
+	private UserSecretReader secretReader;
+	@Mock
 	private KubernetesRemoteService kubernetesRemoteService;
 
-	@DisplayName("Exists secret")
-	@Nested
-	class TestExistsSecret {
-
-		private final OzgKeycloakUserSpec userSpec = OzgKeycloakUserSpecTestFactory.create();
-
-		@Mock
-		private Resource<Secret> resourceMock;
-		@Mock
-		private Secret secret;
-
-		@BeforeEach
-		void mock() {
-			doReturn(resourceMock).when(userSecretService).getUserSecret(any(), any());
-		}
-
-		@Test
-		void shouldGetUserSecret() {
-			when(resourceMock.get()).thenReturn(secret);
-
-			userSecretService.exists(userSpec, NAMESPACE);
-
-			verify(userSecretService).getUserSecret(userSpec, NAMESPACE);
-		}
-
-		@Test
-		void shouldReturnTrueIfExists() {
-			when(resourceMock.get()).thenReturn(secret);
-
-			var exists = userSecretService.exists(userSpec, NAMESPACE);
-
-			assertThat(exists).isTrue();
-		}
-
-		@Test
-		void shouldReturnFalseIfNotExists() {
-			when(resourceMock.get()).thenReturn(null);
-
-			var exists = userSecretService.exists(userSpec, NAMESPACE);
-
-			assertThat(exists).isFalse();
-		}
-	}
-
 	@DisplayName("Create Secret")
 	@Nested
 	class TestCreateSecret {
@@ -128,6 +86,27 @@ class UserSecretServiceTest {
 
 			verify(resourceAdapter).create(secret);
 		}
+
+		@Test
+		void shouldReturnCreatedSecret() {
+			Secret createdSecret = userSecretService.create(userSpec, NAMESPACE);
+
+			assertThat(createdSecret).isNotNull();
+		}
+	}
+
+	@DisplayName("Get password from secret")
+	@Nested
+	class TestGetPasswordFromSecret {
+
+		@Test
+		void shouldCallSecretReader() {
+			Secret secret = SecretTestFactory.create();
+
+			userSecretService.getPasswordFromSecret(secret);
+
+			verify(secretReader).getPasswortFromSecret(secret);
+		}
 	}
 
 	@DisplayName("Get user secret")
@@ -157,4 +136,38 @@ class UserSecretServiceTest {
 			verify(kubernetesRemoteService).getSecret(NAMESPACE, CONVERTED_NAME);
 		}
 	}
+
+	@DisplayName("Get or create cluster secret")
+	@Nested
+	class TestGetOrCreateClusterSecret {
+
+		@Mock
+		private Resource<Secret> secretResource;
+
+		private Secret secret = SecretTestFactory.create();
+
+		@BeforeEach
+		void init() {
+			doReturn(secretResource).when(userSecretService).getUserSecret(any(OzgKeycloakUserSpec.class), eq(NAMESPACE));
+		}
+
+		@Test
+		void shouldReturnExistingSecret() {
+			when(secretResource.get()).thenReturn(secret);
+
+			Secret secretResponse = userSecretService.getOrCreateClusterSecret(OzgKeycloakUserSpecTestFactory.create(), NAMESPACE);
+
+			assertThat(secretResponse).isSameAs(secret);
+		}
+
+		@Test
+		void shouldReturnNewSecretIfNotExisting() {
+			when(secretResource.get()).thenReturn(null);
+			doReturn(secret).when(userSecretService).create(any(OzgKeycloakUserSpec.class), eq(NAMESPACE));
+
+			Secret secretResponse = userSecretService.getOrCreateClusterSecret(OzgKeycloakUserSpecTestFactory.create(), NAMESPACE);
+
+			assertThat(secretResponse).isSameAs(secret);
+		}
+	}
 }
\ No newline at end of file