diff --git a/clouddriver-core/src/test/groovy/com/netflix/spinnaker/clouddriver/deploy/DescriptionAuthorizerServiceSpec.groovy b/clouddriver-core/src/test/groovy/com/netflix/spinnaker/clouddriver/deploy/DescriptionAuthorizerServiceSpec.groovy deleted file mode 100644 index 225f2c70e8c..00000000000 --- a/clouddriver-core/src/test/groovy/com/netflix/spinnaker/clouddriver/deploy/DescriptionAuthorizerServiceSpec.groovy +++ /dev/null @@ -1,153 +0,0 @@ -/* - * Copyright 2016 Google, 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.deploy - -import com.netflix.spectator.api.NoopRegistry -import com.netflix.spinnaker.clouddriver.security.AccountDefinitionSecretManager -import com.netflix.spinnaker.clouddriver.security.DefaultAccountSecurityPolicy -import com.netflix.spinnaker.clouddriver.security.config.SecurityConfig -import com.netflix.spinnaker.clouddriver.security.resources.AccountNameable -import com.netflix.spinnaker.clouddriver.security.resources.ApplicationNameable -import com.netflix.spinnaker.clouddriver.security.resources.ResourcesNameable -import com.netflix.spinnaker.fiat.model.resources.ResourceType -import com.netflix.spinnaker.fiat.shared.FiatPermissionEvaluator -import com.netflix.spinnaker.kork.secrets.user.UserSecretManager -import org.springframework.security.authentication.TestingAuthenticationToken -import org.springframework.security.core.context.SecurityContextHolder -import spock.lang.Specification -import spock.lang.Subject -import spock.lang.Unroll - -class DescriptionAuthorizerServiceSpec extends Specification { - def registry = new NoopRegistry() - def evaluator = Mock(FiatPermissionEvaluator) - def userSecretManager = Mock(UserSecretManager) - def opsSecurityConfigProps - - @Subject - DescriptionAuthorizerService service - - def setup() { - opsSecurityConfigProps = new SecurityConfig.OperationsSecurityConfigurationProperties() - service = new DescriptionAuthorizerService(registry, Optional.of(evaluator), opsSecurityConfigProps, new AccountDefinitionSecretManager(userSecretManager, new DefaultAccountSecurityPolicy(evaluator))) - } - - def "should authorize passed description"() { - given: - def auth = new TestingAuthenticationToken(null, null) - - def ctx = SecurityContextHolder.createEmptyContext() - ctx.setAuthentication(auth) - SecurityContextHolder.setContext(ctx) - - def description = new TestDescription( - "testAccount", ["testApplication", null], ["testResource1", "testResource2", null] - ) - - def errors = new DescriptionValidationErrors(description) - - when: - service.authorize(description, errors) - - then: - 3 * evaluator.hasPermission(*_) >> false - 1 * evaluator.storeWholePermission() - errors.allErrors.size() == 4 - } - - @Unroll - def "should skip authentication for image tagging description if allowUnauthenticatedImageTaggingInAccounts contains the account"() { - given: - def description = new TestImageTaggingDescription("testAccount") - - def errors = new DescriptionValidationErrors(description) - - opsSecurityConfigProps.allowUnauthenticatedImageTaggingInAccounts = allowUnauthenticatedImageTaggingInAccounts - - when: - service.authorize(description, errors) - - then: - 0 * evaluator.hasPermission(*_) >> false - 0 * evaluator.storeWholePermission() - errors.allErrors.size() == expectedNumberOfErrors - - where: - allowUnauthenticatedImageTaggingInAccounts || expectedNumberOfErrors - ["testAccount"] || 0 - ["anotherAccount"] || 1 - [] || 1 - } - - @Unroll - def "should only authz specified resource type"() { - given: - def auth = new TestingAuthenticationToken(null, null) - - def ctx = SecurityContextHolder.createEmptyContext() - ctx.setAuthentication(auth) - SecurityContextHolder.setContext(ctx) - - def description = new TestDescription( - "testAccount", ["testApplication", null], ["testResource1", "testResource2", null] - ) - - def errors = new DescriptionValidationErrors(description) - - when: - service.authorize(description, errors, List.of(resourceType)) - - then: - expectedNumberOfAuthChecks * evaluator.hasPermission(*_) >> false - errors.allErrors.size() == expectedNumberOfErrors - - where: - resourceType || expectedNumberOfAuthChecks | expectedNumberOfErrors - ResourceType.APPLICATION || 3 | 3 - ResourceType.ACCOUNT || 0 | 1 - } - - class TestDescription implements AccountNameable, ApplicationNameable, ResourcesNameable { - String account - Collection applications - List names - - TestDescription(String account, Collection applications, List names) { - this.account = account - this.applications = applications - this.names = names - } - } - - class TestImageTaggingDescription implements AccountNameable { - String account - - TestImageTaggingDescription(String account) { - this.account = account - } - - @Override - boolean requiresApplicationRestriction() { - return false - } - - @Override - boolean requiresAuthorization(SecurityConfig.OperationsSecurityConfigurationProperties opsSecurityConfigProps) { - return !opsSecurityConfigProps.allowUnauthenticatedImageTaggingInAccounts.contains(account) - } - } -} diff --git a/clouddriver-core/src/test/java/com/netflix/spinnaker/clouddriver/deploy/DescriptionAuthorizerServiceTest.java b/clouddriver-core/src/test/java/com/netflix/spinnaker/clouddriver/deploy/DescriptionAuthorizerServiceTest.java new file mode 100644 index 00000000000..3c5e3029611 --- /dev/null +++ b/clouddriver-core/src/test/java/com/netflix/spinnaker/clouddriver/deploy/DescriptionAuthorizerServiceTest.java @@ -0,0 +1,239 @@ +/* + * Copyright 2023 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.deploy; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import com.netflix.spectator.api.DefaultRegistry; +import com.netflix.spinnaker.clouddriver.security.AccountDefinitionSecretManager; +import com.netflix.spinnaker.clouddriver.security.config.SecurityConfig; +import com.netflix.spinnaker.clouddriver.security.resources.AccountNameable; +import com.netflix.spinnaker.clouddriver.security.resources.ApplicationNameable; +import com.netflix.spinnaker.clouddriver.security.resources.ResourcesNameable; +import com.netflix.spinnaker.fiat.model.resources.ResourceType; +import com.netflix.spinnaker.fiat.shared.FiatPermissionEvaluator; +import java.util.Arrays; +import java.util.Collection; +import java.util.List; +import java.util.Optional; +import java.util.stream.Stream; +import lombok.Getter; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.CsvSource; +import org.junit.jupiter.params.provider.MethodSource; +import org.junit.jupiter.params.provider.ValueSource; +import org.springframework.security.authentication.TestingAuthenticationToken; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.context.SecurityContextHolder; + +public class DescriptionAuthorizerServiceTest { + + private final DefaultRegistry registry = new DefaultRegistry(); + private final FiatPermissionEvaluator evaluator = mock(FiatPermissionEvaluator.class); + private final AccountDefinitionSecretManager secretManager = + mock(AccountDefinitionSecretManager.class); + private SecurityConfig.OperationsSecurityConfigurationProperties opsSecurityConfigProps; + private DescriptionAuthorizerService service; + private final String username = "testUser"; + + @BeforeEach + public void setup() { + opsSecurityConfigProps = new SecurityConfig.OperationsSecurityConfigurationProperties(); + service = + new DescriptionAuthorizerService( + registry, Optional.of(evaluator), opsSecurityConfigProps, secretManager); + TestingAuthenticationToken auth = new TestingAuthenticationToken(username, null); + SecurityContextHolder.getContext().setAuthentication(auth); + } + + @AfterEach + public void resetRegistry() { + registry.reset(); + } + + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void shouldAuthorizePassedDescription(boolean hasPermission) { + TestDescription description = + new TestDescription( + "testAccount", + Arrays.asList("testApplication", null), + Arrays.asList("testResource1", "testResource2", null)); + + DescriptionValidationErrors errors = new DescriptionValidationErrors(description); + + when(secretManager.canAccessAccountWithSecrets(username, "testAccount")) + .thenReturn(hasPermission); + when(evaluator.hasPermission(any(Authentication.class), anyString(), anyString(), anyString())) + .thenReturn(hasPermission); + + service.authorize(description, errors); + + assertEquals(hasPermission ? 0 : 4, errors.getAllErrors().size()); + verify(secretManager).canAccessAccountWithSecrets(username, "testAccount"); + verify(evaluator, times(3)) + .hasPermission(any(Authentication.class), anyString(), anyString(), anyString()); + verify(evaluator, times(1)).storeWholePermission(); + + verifySuccessMetric(hasPermission, "TestDescription"); + } + + private static Stream provideSkipAuthenticationForImageTaggingArgs() { + return Stream.of( + Arguments.of(List.of("testAccount"), 0), + Arguments.of(List.of("anotherAccount"), 1), + Arguments.of(List.of(), 1)); + } + + @ParameterizedTest + @MethodSource("provideSkipAuthenticationForImageTaggingArgs") + public void shouldSkipAuthenticationForImageTaggingDescription( + List allowUnauthenticatedImageTaggingInAccounts, int expectedNumberOfErrors) { + TestImageTaggingDescription description = new TestImageTaggingDescription("testAccount"); + DescriptionValidationErrors errors = new DescriptionValidationErrors(description); + + opsSecurityConfigProps.setAllowUnauthenticatedImageTaggingInAccounts( + allowUnauthenticatedImageTaggingInAccounts); + + service.authorize(description, errors); + + assertEquals(errors.getAllErrors().size(), expectedNumberOfErrors); + if (!allowUnauthenticatedImageTaggingInAccounts.isEmpty() + && allowUnauthenticatedImageTaggingInAccounts.get(0).equals("testAccount")) { + verify(secretManager, never()).canAccessAccountWithSecrets(username, "testAccount"); + } else { + verify(secretManager).canAccessAccountWithSecrets(username, "testAccount"); + } + verify(evaluator, never()) + .hasPermission(any(Authentication.class), anyString(), anyString(), anyString()); + verify(evaluator, never()).storeWholePermission(); + + verifySuccessMetric(expectedNumberOfErrors == 0, "TestImageTaggingDescription"); + + assertEquals( + expectedNumberOfErrors > 0 ? 0 : 1, + registry + .counter("authorization.skipped", "descriptionClass", "TestImageTaggingDescription") + .count()); + } + + @ParameterizedTest + @CsvSource({"APPLICATION", "ACCOUNT"}) + public void shouldOnlyAuthzSpecifiedResourceType(ResourceType resourceType) { + TestDescription description = + new TestDescription( + "testAccount", + Arrays.asList("testApplication", null), + Arrays.asList("testResource1", "testResource2", null)); + + DescriptionValidationErrors errors = new DescriptionValidationErrors(description); + + service.authorize(description, errors, List.of(resourceType)); + + if (resourceType.equals(ResourceType.APPLICATION)) { + verify(evaluator, times(3)).hasPermission(any(Authentication.class), any(), any(), any()); + assertEquals(3, errors.getAllErrors().size()); + } else { + verify(secretManager).canAccessAccountWithSecrets(username, "testAccount"); + assertEquals(1, errors.getAllErrors().size()); + } + + verifySuccessMetric(false, "TestDescription"); + } + + @Test + public void shouldAddMetricWithApplicationRestrictionAndNoAccount() { + TestDescription description = new TestDescription("testAccount", List.of(), List.of()); + DescriptionValidationErrors errors = new DescriptionValidationErrors(description); + + service.authorize(description, errors); + + assertEquals(errors.getAllErrors().size(), 1); + assertEquals( + 1, + registry + .counter( + "authorization.missingApplication", + "descriptionClass", + "TestDescription", + "hasValidationErrors", + "true") + .count()); + verifySuccessMetric(false, "TestDescription"); + } + + private void verifySuccessMetric(boolean success, String descriptionClass) { + assertEquals( + 1, + registry + .counter( + "authorization", + "descriptionClass", + descriptionClass, + "success", + String.valueOf(success)) + .count()); + } + + @Getter + public static class TestDescription + implements AccountNameable, ApplicationNameable, ResourcesNameable { + String account; + Collection applications; + List names; + + public TestDescription(String account, Collection applications, List names) { + this.account = account; + this.applications = applications; + this.names = names; + } + } + + @Getter + public static class TestImageTaggingDescription implements AccountNameable { + String account; + + public TestImageTaggingDescription(String account) { + this.account = account; + } + + @Override + public boolean requiresApplicationRestriction() { + return false; + } + + @Override + public boolean requiresAuthorization( + SecurityConfig.OperationsSecurityConfigurationProperties opsSecurityConfigProps) { + return !opsSecurityConfigProps + .getAllowUnauthenticatedImageTaggingInAccounts() + .contains(account); + } + } +} 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 new file mode 100644 index 00000000000..3ecc87ea482 --- /dev/null +++ b/clouddriver-core/src/test/java/com/netflix/spinnaker/clouddriver/security/DefaultAccountSecurityPolicyTest.java @@ -0,0 +1,100 @@ +/* + * Copyright 2023 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.assertEquals; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import com.netflix.spinnaker.fiat.model.Authorization; +import com.netflix.spinnaker.fiat.model.UserPermission; +import com.netflix.spinnaker.fiat.model.resources.Role; +import com.netflix.spinnaker.fiat.shared.FiatPermissionEvaluator; +import java.util.Set; +import java.util.stream.Collectors; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +public class DefaultAccountSecurityPolicyTest { + private static final String username = "testUser"; + FiatPermissionEvaluator fiatPermissionEvaluator = mock(FiatPermissionEvaluator.class); + DefaultAccountSecurityPolicy policy; + + @BeforeEach + void setup() { + policy = new DefaultAccountSecurityPolicy(fiatPermissionEvaluator); + } + + @ParameterizedTest + @ValueSource(booleans = {true, false}) + public void testIsAdmin(boolean isUserAdmin) { + when(fiatPermissionEvaluator.getPermission(username)) + .thenReturn(new UserPermission.View().setAdmin(isUserAdmin)); + + assertEquals(isUserAdmin, policy.isAdmin(username)); + } + + @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)); + + assertEquals(isAccountManager, policy.isAccountManager(username)); + } + + @Test + public void testGetRoles() { + Set roles = Set.of("role1", "role2", "role3"); + when(fiatPermissionEvaluator.getPermission(username)) + .thenReturn( + new UserPermission.View() + .setRoles( + roles.stream() + .map(role -> new Role.View().setName(role).setSource(Role.Source.LDAP)) + .collect(Collectors.toSet()))); + + assertEquals(roles, policy.getRoles(username)); + } + + @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)) + .thenReturn(hasPermission); + + assertEquals(hasPermission, policy.canUseAccount(username, account)); + } + + @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")); + } +}