Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): Add account manager roles config #928

Merged
merged 4 commits into from
Apr 1, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ public class UserPermission {
private Set<BuildService> buildServices = new LinkedHashSet<>();
private Set<Resource> extensionResources = new LinkedHashSet<>();
private boolean admin = false;
private boolean accountManager;

/**
* Custom setter to normalize the input. lombok.accessors.chain is enabled, so setters must return
Expand Down Expand Up @@ -118,6 +119,7 @@ public static class View extends Viewable.BaseView {
Set<BuildService.View> buildServices;
HashMap<ResourceType, Set<Authorizable>> extensionResources;
boolean admin;
boolean accountManager;
boolean legacyFallback = false;
boolean allowAccessToUnknownApplications = false;

Expand Down Expand Up @@ -149,6 +151,7 @@ public View(UserPermission permission) {
}

this.admin = permission.isAdmin();
this.accountManager = permission.isAccountManager();
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package com.netflix.spinnaker.fiat.config;

import java.util.List;
import lombok.Data;
import org.springframework.boot.context.properties.ConfigurationProperties;
import org.springframework.context.annotation.Configuration;

/** Configures Fiat aspects of the account management API. */
@Configuration
@ConfigurationProperties("fiat.account.manager")
@Data
public class AccountManagerConfig {

/** List of roles allowed to programmatically manage accounts. */
private List<String> roles = List.of();
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.fasterxml.jackson.databind.ObjectMapper;
import com.google.common.collect.ImmutableList;
import com.netflix.spinnaker.fiat.config.AccountManagerConfig;
import com.netflix.spinnaker.fiat.config.FiatAdminConfig;
import com.netflix.spinnaker.fiat.config.UnrestrictedResourceConfig;
import com.netflix.spinnaker.fiat.model.UserPermission;
Expand All @@ -29,6 +30,7 @@
import com.netflix.spinnaker.fiat.roles.UserRolesProvider;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
Expand All @@ -51,6 +53,7 @@ public class DefaultPermissionsResolver implements PermissionsResolver {
private final ResourceProvider<ServiceAccount> serviceAccountProvider;
private final ImmutableList<ResourceProvider<? extends Resource>> resourceProviders;
private final FiatAdminConfig fiatAdminConfig;
private final AccountManagerConfig accountManagerConfig;
private final ObjectMapper mapper;

@Autowired
Expand All @@ -59,11 +62,13 @@ public DefaultPermissionsResolver(
ResourceProvider<ServiceAccount> serviceAccountProvider,
List<ResourceProvider<? extends Resource>> resourceProviders,
FiatAdminConfig fiatAdminConfig,
AccountManagerConfig accountManagerConfig,
@Qualifier("objectMapper") ObjectMapper mapper) {
this.userRolesProvider = userRolesProvider;
this.serviceAccountProvider = serviceAccountProvider;
this.resourceProviders = ImmutableList.copyOf(resourceProviders);
this.fiatAdminConfig = fiatAdminConfig;
this.accountManagerConfig = accountManagerConfig;
this.mapper = mapper;
}

Expand Down Expand Up @@ -103,15 +108,26 @@ public void clearCache() {
}
}

private boolean resolveAdminRole(Set<Role> roles) {
List<String> adminRoles = fiatAdminConfig.getAdmin().getRoles();
return roles.stream().map(Role::getName).anyMatch(adminRoles::contains);
private boolean hasAdminRole(Set<Role> roles) {
Set<String> adminRoles = Set.copyOf(fiatAdminConfig.getAdmin().getRoles());
Set<String> userRoles = roles.stream().map(Role::getName).collect(Collectors.toSet());
return !Collections.disjoint(adminRoles, userRoles);
}

private boolean hasAccountManagerRole(Set<Role> roles) {
Set<String> accountManagerRoles = Set.copyOf(accountManagerConfig.getRoles());
Set<String> userRoles = roles.stream().map(Role::getName).collect(Collectors.toSet());
return !Collections.disjoint(accountManagerRoles, userRoles);
}

@SuppressWarnings("unchecked")
private UserPermission getUserPermission(String userId, Set<Role> roles) {
UserPermission permission =
new UserPermission().setId(userId).setRoles(roles).setAdmin(resolveAdminRole(roles));
new UserPermission()
.setId(userId)
.setRoles(roles)
.setAdmin(hasAdminRole(roles))
.setAccountManager(hasAccountManagerRole(roles));

for (ResourceProvider provider : resourceProviders) {
try {
Expand Down Expand Up @@ -198,8 +214,8 @@ private Map<String, UserPermission> resolveResources(
return new UserPermission()
.setId(userId)
.setRoles(userRoles)
.setAdmin(resolveAdminRole(userRoles))
.addResources(getResources(userId, userRoles, resolveAdminRole(userRoles)));
.setAdmin(hasAdminRole(userRoles))
.addResources(getResources(userId, userRoles, hasAdminRole(userRoles)));
})
.collect(Collectors.toMap(UserPermission::getId, Function.identity()));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.netflix.spinnaker.fiat.permissions

import com.fasterxml.jackson.databind.ObjectMapper
import com.netflix.spinnaker.fiat.config.AccountManagerConfig
import com.netflix.spinnaker.fiat.config.FiatAdminConfig
import com.netflix.spinnaker.fiat.config.FiatRoleConfig
import com.netflix.spinnaker.fiat.model.Authorization
Expand Down Expand Up @@ -105,7 +106,7 @@ class DefaultPermissionsResolverSpec extends Specification {
def "should resolve the anonymous user permission, when enabled"() {
setup:
@Subject DefaultPermissionsResolver resolver = new DefaultPermissionsResolver(
userRolesProvider, serviceAccountProvider, resourceProviders, new FiatAdminConfig(), new ObjectMapper())
userRolesProvider, serviceAccountProvider, resourceProviders, new FiatAdminConfig(), new AccountManagerConfig(), new ObjectMapper())

when:
def result = resolver.resolveUnrestrictedUser()
Expand All @@ -124,7 +125,7 @@ class DefaultPermissionsResolverSpec extends Specification {
def testUserId = "testUserId"
UserRolesProvider userRolesProvider = Mock(UserRolesProvider)
@Subject DefaultPermissionsResolver resolver = new DefaultPermissionsResolver(
userRolesProvider, serviceAccountProvider, resourceProviders, new FiatAdminConfig(), new ObjectMapper())
userRolesProvider, serviceAccountProvider, resourceProviders, new FiatAdminConfig(), new AccountManagerConfig(), new ObjectMapper())

def role1 = new Role("group1")
def role2 = new Role("gRoUP2") // to test case insensitivity.
Expand Down Expand Up @@ -182,7 +183,7 @@ class DefaultPermissionsResolverSpec extends Specification {
UserRolesProvider userRolesProvider = Mock(UserRolesProvider)

@Subject DefaultPermissionsResolver resolver = new DefaultPermissionsResolver(
userRolesProvider, serviceAccountProvider, resourceProviders, fiatAdminConfig, new ObjectMapper())
userRolesProvider, serviceAccountProvider, resourceProviders, fiatAdminConfig, new AccountManagerConfig(), new ObjectMapper())

def role1 = new Role("delivery-team")
def testUser = new ExternalUser().setId(testUserId).setExternalRoles([role1])
Expand All @@ -204,7 +205,7 @@ class DefaultPermissionsResolverSpec extends Specification {
setup:
UserRolesProvider userRolesProvider = Mock(UserRolesProvider)
@Subject DefaultPermissionsResolver resolver = new DefaultPermissionsResolver(
userRolesProvider, serviceAccountProvider, resourceProviders, new FiatAdminConfig(), new ObjectMapper())
userRolesProvider, serviceAccountProvider, resourceProviders, new FiatAdminConfig(), new AccountManagerConfig(), new ObjectMapper())

def role1 = new Role("group1")
def role2 = new Role("group2")
Expand Down Expand Up @@ -258,7 +259,7 @@ class DefaultPermissionsResolverSpec extends Specification {
setup:
UserRolesProvider userRolesProvider = Mock(UserRolesProvider)
@Subject DefaultPermissionsResolver resolver = new DefaultPermissionsResolver(
userRolesProvider, serviceAccountProvider, resourceProviders, new FiatAdminConfig(), new ObjectMapper())
userRolesProvider, serviceAccountProvider, resourceProviders, new FiatAdminConfig(), new AccountManagerConfig(), new ObjectMapper())

def role1 = new Role(group1SvcAcct.memberOf[0])
def svc1 = new ExternalUser().setId(group1SvcAcct.name).setExternalRoles([role1])
Expand Down Expand Up @@ -293,4 +294,24 @@ class DefaultPermissionsResolverSpec extends Specification {
result.remove("user1") == expectedUser1
result.isEmpty()
}

def "should resolve account manager permissions"() {
setup:
def accountManagerConfig = new AccountManagerConfig()
accountManagerConfig.roles = ['sre']

def testUsername = 'ron'
def userRolesProvider = Mock(UserRolesProvider)
@Subject def resolver = new DefaultPermissionsResolver(userRolesProvider, serviceAccountProvider, resourceProviders, new FiatAdminConfig(), accountManagerConfig, new ObjectMapper())
def role = new Role('sre')
def user = new ExternalUser().setId(testUsername).setExternalRoles([role])

when:
def result = resolver.resolveAndMerge(user)

then:
1 * userRolesProvider.loadRoles({ u -> u.id == testUsername }) >> [role]
def expected = new UserPermission().setId(testUsername).setRoles(Set.of(role)).setAccountManager(true)
result == expected
}
}