Skip to content

Commit

Permalink
feat(account): Implement the isAccountManager flag (#6141)
Browse files Browse the repository at this point in the history
* test(account): Define when an account manager can modify accounts

* feat(account): Implement isAccountManager

* test(account): Test validateAccountWritePermissions

* fix(account): Allow reflection to read private fields

This fixes an IllegalStateException when reflection tries to read
the private fields of a class to check if any fields are UserSecretReferences.
The error looks like: "Not allowed to access field 'name':
java.lang.IllegalAccessException: class com.netflix.spinnaker.clouddriver.security.AccountDefinitionService
cannot access a member of class
com.netflix.spinnaker.clouddriver.kubernetes.config.KubernetesAccountProperties$ManagedAccount
with modifiers \"private\""

---------

Co-authored-by: Daniel Zheng <d.zheng@salesforce.com>
  • Loading branch information
dzhengg and Daniel Zheng authored Feb 2, 2024
1 parent 1a787e3 commit 5da2a2d
Show file tree
Hide file tree
Showing 4 changed files with 173 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -126,14 +127,21 @@ private void validateAccountWritePermissions(
if (policy.isAdmin(username)) {
return;
}
var accountName = definition.getName();
Set<String> userRoles = policy.getRoles(username);
validateAccountAuthorization(userRoles, definition, action);
validateUserSecretAuthorization(userRoles, definition, action);
}

@VisibleForTesting
void validateAccountAuthorization(
Set<String> userRoles, CredentialsDefinition definition, AccountAction action) {
String accountName = definition.getName();
var type = definition.getClass();
Set<String> 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
Expand All @@ -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<String> userRoles, CredentialsDefinition definition, AccountAction action) {
var type = definition.getClass();
Set<UserSecretReference> 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
Expand All @@ -169,7 +185,8 @@ private void validateAccountWritePermissions(
}
}

private enum AccountAction {
@VisibleForTesting
enum AccountAction {
CREATE,
UPDATE,
SAVE
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
Original file line number Diff line number Diff line change
@@ -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<String> 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<AuthorizedRolesExtractor> 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<String> userRoles = Set.of("role1");
assertDoesNotThrow(
() ->
accountDefinitionService.validateAccountAuthorization(
userRoles, definition, AccountDefinitionService.AccountAction.UPDATE));
}

@Test
public void testValidateAccountAuthorizationEmptyAuthorizedRoles() {
doReturn(Set.of()).when(extractor).getAuthorizedRoles(definition);
Set<String> userRoles = Set.of("role1");
assertDoesNotThrow(
() ->
accountDefinitionService.validateAccountAuthorization(
userRoles, definition, AccountDefinitionService.AccountAction.UPDATE));
}

@Test
public void testValidateAccountAuthorizationNoCommonRoles() {
doReturn(authorizedRoles).when(extractor).getAuthorizedRoles(definition);
Set<String> 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<String> 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<String> 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<String> userRoles = Set.of("role3");

assertThrows(
AccessDeniedException.class,
() ->
accountDefinitionService.validateUserSecretAuthorization(
userRoles, definition, AccountDefinitionService.AccountAction.UPDATE));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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));
}
Expand All @@ -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))
Expand All @@ -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));
}
}

0 comments on commit 5da2a2d

Please sign in to comment.