diff --git a/clouddriver-core/src/main/java/com/netflix/spinnaker/clouddriver/security/AccountDefinitionService.java b/clouddriver-core/src/main/java/com/netflix/spinnaker/clouddriver/security/AccountDefinitionService.java index 5ac734ec3f5..a101f2fde66 100644 --- a/clouddriver-core/src/main/java/com/netflix/spinnaker/clouddriver/security/AccountDefinitionService.java +++ b/clouddriver-core/src/main/java/com/netflix/spinnaker/clouddriver/security/AccountDefinitionService.java @@ -20,6 +20,7 @@ import com.netflix.spinnaker.fiat.model.Authorization; import com.netflix.spinnaker.kork.annotations.Beta; import com.netflix.spinnaker.kork.annotations.NonnullByDefault; +import com.netflix.spinnaker.kork.annotations.VisibleForTesting; import com.netflix.spinnaker.kork.secrets.SecretException; import com.netflix.spinnaker.kork.secrets.user.UserSecretReference; import com.netflix.spinnaker.kork.web.exceptions.InvalidRequestException; @@ -126,14 +127,21 @@ private void validateAccountWritePermissions( if (policy.isAdmin(username)) { return; } - var accountName = definition.getName(); + Set userRoles = policy.getRoles(username); + validateAccountAuthorization(userRoles, definition, action); + validateUserSecretAuthorization(userRoles, definition, action); + } + + @VisibleForTesting + void validateAccountAuthorization( + Set userRoles, CredentialsDefinition definition, AccountAction action) { + String accountName = definition.getName(); var type = definition.getClass(); Set authorizedRoles = extractors.stream() .filter(extractor -> extractor.supportsType(type)) .flatMap(extractor -> extractor.getAuthorizedRoles(definition).stream()) .collect(Collectors.toSet()); - var userRoles = policy.getRoles(username); // if the account defines authorized roles and the user has no roles in common with these // authorized roles, then the user attempted to create an account they'd immediately be // locked out from which is a poor user experience @@ -144,11 +152,19 @@ private void validateAccountWritePermissions( "Cannot %s account without granting permissions for current user (name: %s)", action.name().toLowerCase(Locale.ROOT), accountName)); } + } + + @VisibleForTesting + void validateUserSecretAuthorization( + Set userRoles, CredentialsDefinition definition, AccountAction action) { + var type = definition.getClass(); Set secretReferences = new HashSet<>(); ReflectionUtils.doWithFields( type, - field -> - UserSecretReference.tryParse(field.get(definition)).ifPresent(secretReferences::add), + field -> { + field.setAccessible(true); + UserSecretReference.tryParse(field.get(definition)).ifPresent(secretReferences::add); + }, field -> field.getType() == String.class); // if the account uses any UserSecrets and the user has no roles in common with any of // the UserSecrets, then don't allow the user to save this account due to lack of authorization @@ -169,7 +185,8 @@ private void validateAccountWritePermissions( } } - private enum AccountAction { + @VisibleForTesting + enum AccountAction { CREATE, UPDATE, SAVE diff --git a/clouddriver-core/src/main/java/com/netflix/spinnaker/clouddriver/security/DefaultAccountSecurityPolicy.java b/clouddriver-core/src/main/java/com/netflix/spinnaker/clouddriver/security/DefaultAccountSecurityPolicy.java index 704c5a601df..302894d1d26 100644 --- a/clouddriver-core/src/main/java/com/netflix/spinnaker/clouddriver/security/DefaultAccountSecurityPolicy.java +++ b/clouddriver-core/src/main/java/com/netflix/spinnaker/clouddriver/security/DefaultAccountSecurityPolicy.java @@ -84,7 +84,6 @@ public boolean canModifyAccount(@Nonnull String username, @Nonnull String accoun } private static boolean isAccountManager(UserPermission.View permission) { - // TODO(jvz): replace with UserPermission.View::isAccountManager after fiat updated - return permission.isAdmin(); + return permission.isAccountManager(); } } diff --git a/clouddriver-core/src/test/java/com/netflix/spinnaker/clouddriver/security/AccountDefinitionServiceTest.java b/clouddriver-core/src/test/java/com/netflix/spinnaker/clouddriver/security/AccountDefinitionServiceTest.java new file mode 100644 index 00000000000..3bb5383405f --- /dev/null +++ b/clouddriver-core/src/test/java/com/netflix/spinnaker/clouddriver/security/AccountDefinitionServiceTest.java @@ -0,0 +1,133 @@ +/* + * Copyright 2024 Salesforce, Inc. + * + * 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 com.netflix.spinnaker.clouddriver.security; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; + +import com.netflix.spinnaker.credentials.definition.CredentialsDefinition; +import com.netflix.spinnaker.kork.secrets.user.UserSecret; +import com.netflix.spinnaker.kork.web.exceptions.InvalidRequestException; +import io.spinnaker.test.security.ValueAccount; +import java.util.List; +import java.util.Set; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.security.access.AccessDeniedException; + +public class AccountDefinitionServiceTest { + AccountDefinitionRepository repository; + AccountDefinitionSecretManager secretManager; + AccountCredentialsProvider accountCredentialsProvider; + AccountSecurityPolicy policy; + AuthorizedRolesExtractor extractor; + CredentialsDefinition definition = + ValueAccount.builder().name("name").value("secret://test?k=value").build(); + AccountDefinitionService accountDefinitionService; + Set authorizedRoles = Set.of("role1", "role2"); + + @BeforeEach + public void setup() { + repository = mock(AccountDefinitionRepository.class); + secretManager = mock(AccountDefinitionSecretManager.class); + accountCredentialsProvider = mock(AccountCredentialsProvider.class); + policy = mock(AccountSecurityPolicy.class); + extractor = mock(AuthorizedRolesExtractor.class); + List extractors = List.of(extractor); + accountDefinitionService = + new AccountDefinitionService( + repository, secretManager, accountCredentialsProvider, policy, extractors); + + doReturn(true).when(extractor).supportsType(definition.getClass()); + } + + @Test + public void testValidateAccountAuthorizationWithCommonRole() { + doReturn(authorizedRoles).when(extractor).getAuthorizedRoles(definition); + Set userRoles = Set.of("role1"); + assertDoesNotThrow( + () -> + accountDefinitionService.validateAccountAuthorization( + userRoles, definition, AccountDefinitionService.AccountAction.UPDATE)); + } + + @Test + public void testValidateAccountAuthorizationEmptyAuthorizedRoles() { + doReturn(Set.of()).when(extractor).getAuthorizedRoles(definition); + Set userRoles = Set.of("role1"); + assertDoesNotThrow( + () -> + accountDefinitionService.validateAccountAuthorization( + userRoles, definition, AccountDefinitionService.AccountAction.UPDATE)); + } + + @Test + public void testValidateAccountAuthorizationNoCommonRoles() { + doReturn(authorizedRoles).when(extractor).getAuthorizedRoles(definition); + Set userRoles = Set.of("oneRole", "anotherRole"); + assertThrows( + InvalidRequestException.class, + () -> + accountDefinitionService.validateAccountAuthorization( + userRoles, definition, AccountDefinitionService.AccountAction.UPDATE)); + } + + @Test + public void testValidateUserSecretAuthorizationWithCommonRole() { + UserSecret userSecret = mock(UserSecret.class); + doReturn(List.copyOf(authorizedRoles)).when(userSecret).getRoles(); + doReturn(userSecret).when(secretManager).getUserSecret(any()); + Set userRoles = Set.of("role1", "role3"); + + assertDoesNotThrow( + () -> + accountDefinitionService.validateUserSecretAuthorization( + userRoles, definition, AccountDefinitionService.AccountAction.UPDATE)); + } + + @Test + public void testValidateUserSecretAuthorizationEmptyAuthorizedRoles() { + UserSecret userSecret = mock(UserSecret.class); + doReturn(List.of()).when(userSecret).getRoles(); + doReturn(userSecret).when(secretManager).getUserSecret(any()); + Set userRoles = Set.of("role1", "role3"); + + assertThrows( + AccessDeniedException.class, + () -> + accountDefinitionService.validateUserSecretAuthorization( + userRoles, definition, AccountDefinitionService.AccountAction.UPDATE)); + } + + @Test + public void testValidateUserSecretAuthorizationNoCommonRoles() { + UserSecret userSecret = mock(UserSecret.class); + doReturn(List.copyOf(authorizedRoles)).when(userSecret).getRoles(); + doReturn(userSecret).when(secretManager).getUserSecret(any()); + Set userRoles = Set.of("role3"); + + assertThrows( + AccessDeniedException.class, + () -> + accountDefinitionService.validateUserSecretAuthorization( + userRoles, definition, AccountDefinitionService.AccountAction.UPDATE)); + } +} diff --git a/clouddriver-core/src/test/java/com/netflix/spinnaker/clouddriver/security/DefaultAccountSecurityPolicyTest.java b/clouddriver-core/src/test/java/com/netflix/spinnaker/clouddriver/security/DefaultAccountSecurityPolicyTest.java index 3ecc87ea482..5d72ccc7065 100644 --- a/clouddriver-core/src/test/java/com/netflix/spinnaker/clouddriver/security/DefaultAccountSecurityPolicyTest.java +++ b/clouddriver-core/src/test/java/com/netflix/spinnaker/clouddriver/security/DefaultAccountSecurityPolicyTest.java @@ -30,10 +30,12 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.CsvSource; import org.junit.jupiter.params.provider.ValueSource; public class DefaultAccountSecurityPolicyTest { private static final String username = "testUser"; + private static final String account = "testAccount"; FiatPermissionEvaluator fiatPermissionEvaluator = mock(FiatPermissionEvaluator.class); DefaultAccountSecurityPolicy policy; @@ -54,9 +56,8 @@ public void testIsAdmin(boolean isUserAdmin) { @ParameterizedTest @ValueSource(booleans = {true, false}) public void testIsAccountManager(boolean isAccountManager) { - // TODO: replace with UserPermission.View::setAccountManager after fiat updated when(fiatPermissionEvaluator.getPermission(username)) - .thenReturn(new UserPermission.View().setAdmin(isAccountManager)); + .thenReturn(new UserPermission.View().setAccountManager(isAccountManager)); assertEquals(isAccountManager, policy.isAccountManager(username)); } @@ -78,7 +79,6 @@ public void testGetRoles() { @ParameterizedTest @ValueSource(booleans = {true, false}) public void testCanUseAccount_NotAdmin(boolean hasPermission) { - String account = "testAccount"; when(fiatPermissionEvaluator.getPermission(username)) .thenReturn(new UserPermission.View().setAdmin(false)); when(fiatPermissionEvaluator.hasPermission(username, account, "account", Authorization.WRITE)) @@ -90,11 +90,22 @@ public void testCanUseAccount_NotAdmin(boolean hasPermission) { @ParameterizedTest @ValueSource(booleans = {true, false}) public void testCanModifyAccount(boolean isAdmin) { - /* TODO: when isAccountManager uses the account manager value, update to get value from - hasPermission mock. */ when(fiatPermissionEvaluator.getPermission(username)) .thenReturn(new UserPermission.View().setAdmin(isAdmin)); - assertEquals(isAdmin, policy.canModifyAccount(username, "testAccount")); + assertEquals(isAdmin, policy.canModifyAccount(username, account)); + } + + @ParameterizedTest + @CsvSource({"false,false", "false,true", "true,false", "true,true"}) + public void testCanModifyAccountAsAccountManager( + boolean isAccountManager, boolean hasWritePermission) { + when(fiatPermissionEvaluator.getPermission(username)) + .thenReturn(new UserPermission.View().setAdmin(false).setAccountManager(isAccountManager)); + when(fiatPermissionEvaluator.hasPermission(username, account, "account", Authorization.WRITE)) + .thenReturn(hasWritePermission); + + assertEquals( + isAccountManager && hasWritePermission, policy.canModifyAccount(username, account)); } }