diff --git a/gate-core/gate-core.gradle b/gate-core/gate-core.gradle index 7ad8a916ad..235f536637 100644 --- a/gate-core/gate-core.gradle +++ b/gate-core/gate-core.gradle @@ -33,6 +33,7 @@ dependencies { implementation "io.spinnaker.fiat:fiat-core:$fiatVersion" implementation "io.spinnaker.kork:kork-core" + implementation "io.spinnaker.kork:kork-retrofit" implementation "io.spinnaker.kork:kork-web" implementation "io.spinnaker.kork:kork-security" implementation "com.netflix.spectator:spectator-api" diff --git a/gate-core/src/main/java/com/netflix/spinnaker/gate/retrofit/UpstreamBadRequest.java b/gate-core/src/main/java/com/netflix/spinnaker/gate/retrofit/UpstreamBadRequest.java index e0b38769b0..2695c89963 100644 --- a/gate-core/src/main/java/com/netflix/spinnaker/gate/retrofit/UpstreamBadRequest.java +++ b/gate-core/src/main/java/com/netflix/spinnaker/gate/retrofit/UpstreamBadRequest.java @@ -20,6 +20,8 @@ import static retrofit.RetrofitError.Kind.HTTP; import com.netflix.spinnaker.kork.exceptions.SpinnakerException; +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException; +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException; import java.util.Collection; import retrofit.RetrofitError; @@ -36,6 +38,14 @@ private UpstreamBadRequest(RetrofitError cause) { error = cause.getBody(); } + private UpstreamBadRequest(SpinnakerHttpException cause) { + super(cause.getMessage(), cause); + this.setRetryable(false); + status = cause.getResponseCode(); + url = cause.getUrl(); + error = cause.getResponseBody(); + } + public int getStatus() { return status; } @@ -66,4 +76,13 @@ public static RuntimeException classifyError( return error; } } + + public static RuntimeException classifyError(SpinnakerServerException error) { + if (error instanceof SpinnakerHttpException + && ((SpinnakerHttpException) error).getResponseCode() < INTERNAL_SERVER_ERROR.value()) { + return new UpstreamBadRequest((SpinnakerHttpException) error); + } else { + return error; + } + } } diff --git a/gate-core/src/main/java/com/netflix/spinnaker/gate/services/PermissionService.java b/gate-core/src/main/java/com/netflix/spinnaker/gate/services/PermissionService.java index 91c32e9f65..a6e79b485c 100644 --- a/gate-core/src/main/java/com/netflix/spinnaker/gate/services/PermissionService.java +++ b/gate-core/src/main/java/com/netflix/spinnaker/gate/services/PermissionService.java @@ -29,6 +29,9 @@ import com.netflix.spinnaker.kork.core.RetrySupport; import com.netflix.spinnaker.kork.exceptions.SpinnakerException; import com.netflix.spinnaker.kork.exceptions.SystemException; +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException; +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException; +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException; import com.netflix.spinnaker.security.AuthenticatedRequest; import com.netflix.spinnaker.security.User; import java.time.Duration; @@ -83,7 +86,7 @@ public void login(final String userId) { permissionEvaluator.invalidatePermission(userId); return null; }); - } catch (RetrofitError e) { + } catch (SpinnakerServerException e) { throw UpstreamBadRequest.classifyError(e); } } @@ -98,7 +101,7 @@ public void loginWithRoles(final String userId, final Collection roles) permissionEvaluator.invalidatePermission(userId); return null; }); - } catch (RetrofitError e) { + } catch (SpinnakerServerException e) { throw UpstreamBadRequest.classifyError(e); } } @@ -109,7 +112,7 @@ public void logout(String userId) { try { getFiatServiceForLogin().logoutUser(userId); permissionEvaluator.invalidatePermission(userId); - } catch (RetrofitError e) { + } catch (SpinnakerServerException e) { throw UpstreamBadRequest.classifyError(e); } } @@ -119,7 +122,7 @@ public void sync() { if (fiatStatus.isEnabled()) { try { getFiatServiceForLogin().sync(List.of()); - } catch (RetrofitError e) { + } catch (SpinnakerServerException e) { throw UpstreamBadRequest.classifyError(e); } } @@ -133,7 +136,7 @@ public Set getRoles(String userId) { var permission = permissionEvaluator.getPermission(userId); var roles = permission != null ? permission.getRoles() : null; return roles != null ? roles : Set.of(); - } catch (RetrofitError e) { + } catch (SpinnakerServerException e) { throw UpstreamBadRequest.classifyError(e); } } @@ -141,14 +144,19 @@ public Set getRoles(String userId) { List lookupServiceAccounts(String userId) { try { return extendedFiatService.getUserServiceAccounts(userId); - } catch (RetrofitError re) { - var response = re.getResponse(); - if (response != null && response.getStatus() == HttpStatus.NOT_FOUND.value()) { + } catch (SpinnakerNetworkException e) { + throw new SystemException("getUserServiceAccounts failed", e).setRetryable(true); + } catch (SpinnakerHttpException e) { + if (e.getResponseCode() == 404) { return List.of(); } - boolean shouldRetry = - response == null || HttpStatus.valueOf(response.getStatus()).is5xxServerError(); - throw new SystemException("getUserServiceAccounts failed", re).setRetryable(shouldRetry); + if (HttpStatus.valueOf(e.getResponseCode()).is5xxServerError()) { + throw new SystemException("getUserServiceAccounts failed", e).setRetryable(true); + } + throw UpstreamBadRequest.classifyError(e); + } catch (SpinnakerServerException e) { + e.setRetryable(false); + throw UpstreamBadRequest.classifyError(e); } } @@ -217,8 +225,8 @@ public List getServiceAccounts(@SpinnakerUser User user) { return permission.getServiceAccounts().stream() .map(ServiceAccount.View::getName) .collect(Collectors.toList()); - } catch (RetrofitError re) { - throw UpstreamBadRequest.classifyError(re); + } catch (SpinnakerServerException e) { + throw UpstreamBadRequest.classifyError(e); } } diff --git a/gate-core/src/test/groovy/com/netflix/spinnaker/gate/services/PermissionServiceSpec.groovy b/gate-core/src/test/groovy/com/netflix/spinnaker/gate/services/PermissionServiceSpec.groovy deleted file mode 100644 index bc93465fc5..0000000000 --- a/gate-core/src/test/groovy/com/netflix/spinnaker/gate/services/PermissionServiceSpec.groovy +++ /dev/null @@ -1,168 +0,0 @@ -/* - * Copyright 2020 Netflix, 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.gate.services - -import com.netflix.spinnaker.fiat.model.Authorization -import com.netflix.spinnaker.fiat.model.UserPermission -import com.netflix.spinnaker.fiat.model.resources.Application -import com.netflix.spinnaker.fiat.model.resources.Permissions -import com.netflix.spinnaker.fiat.model.resources.Role -import com.netflix.spinnaker.fiat.shared.FiatPermissionEvaluator -import com.netflix.spinnaker.fiat.shared.FiatStatus -import com.netflix.spinnaker.gate.services.internal.ExtendedFiatService -import com.netflix.spinnaker.kork.exceptions.SpinnakerException -import com.netflix.spinnaker.security.User -import retrofit.RetrofitError -import retrofit.client.Response -import retrofit.converter.ConversionException -import spock.lang.Specification -import spock.lang.Unroll - -class PermissionServiceSpec extends Specification { - - def "lookupServiceAccount returns empty on 404"() { - given: - def extendedFiatService = Stub(ExtendedFiatService) { - getUserServiceAccounts(user) >> { throw theFailure } - } - def subject = new PermissionService(null, extendedFiatService, null, null, null) - - when: - def result = subject.lookupServiceAccounts(user) - - then: - noExceptionThrown() - result == [] - - where: - user = 'foo@bar.com' - - theFailure << [httpRetrofitError(404), conversionError(404)] - - } - - @Unroll - def "lookupServiceAccount failures are marked retryable"() { - given: - def extendedFiatService = Stub(ExtendedFiatService) { - getUserServiceAccounts(user) >> { throw theFailure } - } - def subject = new PermissionService(null, extendedFiatService, null, null, null) - - when: - subject.lookupServiceAccounts(user) - - then: - def exception = thrown(SpinnakerException) - exception.retryable == expectedRetryable - - where: - user = 'foo@bar.com' - - theFailure || expectedRetryable - httpRetrofitError(400) || false - conversionError(400) || false - conversionError(200) || false - networkError() || true - httpRetrofitError(500) || true - conversionError(500) || true - unexpectedError() || true - } - - private RetrofitError conversionError(int code) { - RetrofitError.conversionError( - 'http://foo', - new Response('http://foo', code, 'you are bad', [], null), - null, - null, - new ConversionException('boom')) - } - - private RetrofitError networkError() { - RetrofitError.networkError('http://foo', new IOException()) - } - - private RetrofitError unexpectedError() { - RetrofitError.unexpectedError('http://foo', new Throwable()) - } - - private RetrofitError httpRetrofitError(int code) { - RetrofitError.httpError( - 'http://foo', - new Response('http://foo', code, 'you are bad', [], null), - null, - null) - } - - - @Unroll - def "getServiceAccountsForApplication when #desc looks up all serviceAccounts (#expectLookup) and falls back to getServiceAccounts (#expectFallback)"() { - given: - def cfgProps = new ServiceAccountFilterConfigProps(enabled, matchAuths ? auths : []) - def user = username == null ? null : Stub(User) { - getUsername() >> username - } - def fiatStatus = Stub(FiatStatus) { - isEnabled() >> fiatEnabled - } - def permissionEvaluator = Mock(FiatPermissionEvaluator) - def extendedFiatService = Mock(ExtendedFiatService) - def result = hasResult ? lookupResult : [] - - def subject = new PermissionService(null, extendedFiatService, cfgProps, permissionEvaluator, fiatStatus) - - when: - subject.getServiceAccountsForApplication(user, application) - - then: - (expectLookup ? 1 : 0) * extendedFiatService.getUserServiceAccounts(username) >> result - (expectFallback ? 1 : 0) * permissionEvaluator.getPermission(username) >> userPermissionView - 0 * _ - - where: - enabled | matchAuths | username | application | fiatEnabled | hasResult || expectLookup || expectFallback || desc - true | true | 'foo@bar.com' | 'myapp' | true | true || true || false || "successfully performs filtered lookup" - false | true | 'foo@bar.com' | 'myapp' | true | true || false || true || "filtering disabled" - true | false | 'foo@bar.com' | 'myapp' | true | true || false || true || "no authorizations to filter on" - true | true | null | 'myapp' | true | true || false || false || "no username supplied" - true | true | 'foo@bar.com' | null | true | true || false || true || "no application supplied" - true | true | 'foo@bar.com' | 'myapp' | false | true || false || false || "fiat disabled" - true | true | 'foo@bar.com' | 'myapp' | true | false || true || true || "no service accounts match" - - auths = [Authorization.WRITE] - lookupResult = [sa("foo-service-account", application, auths)] - userPermission = new UserPermission(id: 'foo@bar.com') - userPermissionView = userPermission.getView() - } - - private UserPermission.View sa(String name, String application, Collection auths) { - UserPermission userPermission = new UserPermission(); - userPermission.setId(name) - userPermission.setRoles([new Role(name: name, source: Role.Source.EXTERNAL)] as Set) - - Application app = new Application() - app.setName(application) - Permissions.Builder pb = new Permissions.Builder() - for (auth in auths) { - pb.add(auth, name) - } - app.setPermissions(pb.build()) - - userPermission.setApplications([app] as Set) - return userPermission.getView() - } -} diff --git a/gate-core/src/test/java/com/netflix/spinnaker/gate/services/PermissionServiceTest.java b/gate-core/src/test/java/com/netflix/spinnaker/gate/services/PermissionServiceTest.java new file mode 100644 index 0000000000..e31791720c --- /dev/null +++ b/gate-core/src/test/java/com/netflix/spinnaker/gate/services/PermissionServiceTest.java @@ -0,0 +1,330 @@ +/* + * Copyright 2024 OpsMx, 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.gate.services; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.fail; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; +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.Application; +import com.netflix.spinnaker.fiat.model.resources.Permissions; +import com.netflix.spinnaker.fiat.model.resources.Role; +import com.netflix.spinnaker.fiat.shared.FiatPermissionEvaluator; +import com.netflix.spinnaker.fiat.shared.FiatStatus; +import com.netflix.spinnaker.gate.services.internal.ExtendedFiatService; +import com.netflix.spinnaker.kork.exceptions.SpinnakerException; +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerConversionException; +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerHttpException; +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerNetworkException; +import com.netflix.spinnaker.kork.retrofit.exceptions.SpinnakerServerException; +import com.netflix.spinnaker.security.User; +import java.io.IOException; +import java.util.Collection; +import java.util.Collections; +import java.util.List; +import java.util.stream.Stream; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; +import retrofit.RetrofitError; +import retrofit.client.Response; +import retrofit.converter.ConversionException; + +public class PermissionServiceTest { + + @Test + public void lookupServiceAccountReturnsEmptyOn404() { + ExtendedFiatService extendedFiatService = mock(ExtendedFiatService.class); + String user = "foo@bar.com"; + + when(extendedFiatService.getUserServiceAccounts(user)).thenThrow(httpError(404)); + + PermissionService subject = new PermissionService(null, extendedFiatService, null, null, null); + + try { + var result = subject.lookupServiceAccounts(user); + assertNotNull(result); + assertTrue(result.isEmpty()); + } catch (Exception e) { + fail("No exception should be thrown"); + } + } + + private static Stream testCasesForRetryable() { + return Stream.of( + new TestCase(httpError(400), false), + new TestCase(conversionError(400), false), + new TestCase(conversionError(200), false), + new TestCase(networkError(), true), + new TestCase(httpError(500), true), + new TestCase(conversionError(500), false), + new TestCase(unexpectedError(), false)); + } + + @ParameterizedTest + @MethodSource("testCasesForRetryable") + public void lookupServiceAccountFailuresAreMarkedRetryable(TestCase testCase) { + ExtendedFiatService extendedFiatService = mock(ExtendedFiatService.class); + String user = "foo@bar.com"; + + when(extendedFiatService.getUserServiceAccounts(user)).thenThrow(testCase.theFailure); + + PermissionService subject = new PermissionService(null, extendedFiatService, null, null, null); + + SpinnakerException exception = + assertThrows(SpinnakerException.class, () -> subject.lookupServiceAccounts(user)); + + assertEquals(testCase.expectedRetryable, exception.getRetryable()); + } + + private static Stream testCases() { + return Stream.of( + new TestParams( + true, + "foo@bar.com", + "myapp", + true, + true, + true, + false, + false, + List.of(Authorization.WRITE), + List.of("foo-service-account"), + "successfully performs filtered lookup"), + new TestParams( + false, + "foo@bar.com", + "myapp", + true, + true, + false, + true, + true, + List.of(Authorization.WRITE), + List.of("foo-service-account"), + "filtering disabled"), + new TestParams( + true, + "foo@bar.com", + "myapp", + true, + true, + false, + true, + true, + List.of(), + List.of("foo-service-account"), + "no authorizations to filter on"), + new TestParams( + true, + null, + "myapp", + true, + true, + false, + false, + false, + List.of(Authorization.WRITE), + List.of("foo-service-account"), + "no username supplied"), + new TestParams( + true, + "foo@bar.com", + null, + true, + true, + false, + true, + true, + List.of(Authorization.WRITE), + List.of("foo-service-account"), + "no application supplied"), + new TestParams( + true, + "foo@bar.com", + "myapp", + false, + true, + false, + false, + false, + List.of(Authorization.WRITE), + List.of("foo-service-account"), + "fiat disabled"), + new TestParams( + true, + "foo@bar.com", + "myapp", + true, + false, + true, + true, + true, + List.of(Authorization.WRITE), + Collections.emptyList(), + "no service accounts match")); + } + + @ParameterizedTest + @MethodSource("testCases") + public void testGetServiceAccountsForApplication(TestParams params) { + + FiatStatus fiatStatus = mock(FiatStatus.class); + FiatPermissionEvaluator permissionEvaluator = mock(FiatPermissionEvaluator.class); + ExtendedFiatService extendedFiatService = mock(ExtendedFiatService.class); + + ServiceAccountFilterConfigProps cfgProps = + new ServiceAccountFilterConfigProps(params.enabled, params.auths ); + User user = params.username == null ? null : mock(User.class); + if (user != null) { + when(user.getUsername()).thenReturn(params.username); + } + when(fiatStatus.isEnabled()).thenReturn(params.fiatEnabled); + if (!params.lookupResultEmpty) { + when(extendedFiatService.getUserServiceAccounts(params.username)) + .thenReturn(List.of(sa("foo-service-account", params.application, params.auths))); + } + + PermissionService permissionService = + new PermissionService(null, extendedFiatService, cfgProps, permissionEvaluator, fiatStatus); + + // When: Call the method under test + permissionService.getServiceAccountsForApplication(user, params.application); + + // Then: Verify the expected interactions + if (params.expectLookup) { + verify(extendedFiatService, times(1)).getUserServiceAccounts(params.username); + } else { + verify(extendedFiatService, times(0)).getUserServiceAccounts(params.username); + } + + if (params.expectFallback) { + verify(permissionEvaluator, times(1)).getPermission(params.username); + } else { + verify(permissionEvaluator, times(0)).getPermission(params.username); + } + + verifyNoMoreInteractions(extendedFiatService, permissionEvaluator); + } + + private UserPermission.View sa(String name, String application, Collection auths) { + UserPermission userPermission = new UserPermission(); + userPermission.setId(name); + + Role role = new Role(name).setSource(Role.Source.EXTERNAL); + userPermission.setRoles(Collections.singleton(role)); + + Application app = new Application(); + app.setName(application); + + Permissions.Builder pb = new Permissions.Builder(); + for (Authorization auth : auths) { + pb.add(auth, name); + } + + app.setPermissions(pb.build()); + userPermission.setApplications(Collections.singleton(app)); + + return userPermission.getView(); + } + + private static Throwable conversionError(int code) { + RetrofitError re = + RetrofitError.conversionError( + "http://foo", + new Response("http://foo", code, "you are bad", List.of(), null), + null, + null, + new ConversionException("boom")); + return new SpinnakerConversionException(re); + } + + private static SpinnakerNetworkException networkError() { + return new SpinnakerNetworkException( + RetrofitError.networkError("http://foo", new IOException())); + } + + private static Throwable unexpectedError() { + return new SpinnakerServerException( + RetrofitError.unexpectedError("http://foo", new Throwable())); + } + + private static Throwable httpError(int status) { + RetrofitError re = + RetrofitError.httpError( + "http://foo", new Response("http://foo", status, "", List.of(), null), null, null); + return new SpinnakerHttpException(re); + } + + private static class TestCase { + final Throwable theFailure; + final boolean expectedRetryable; + + TestCase(Throwable theFailure, boolean expectedRetryable) { + this.theFailure = theFailure; + this.expectedRetryable = expectedRetryable; + } + } + + private static class TestParams { + final boolean enabled; + final String username; + final String application; + final boolean fiatEnabled; + final boolean hasResult; + final boolean expectLookup; + final boolean expectFallback; + final boolean lookupResultEmpty; + final List auths; + final List lookupResult; + final String desc; + + TestParams( + boolean enabled, + String username, + String application, + boolean fiatEnabled, + boolean hasResult, + boolean expectLookup, + boolean expectFallback, + boolean lookupResultEmpty, + List auths, + List lookupResult, + String desc) { + this.enabled = enabled; + this.username = username; + this.application = application; + this.fiatEnabled = fiatEnabled; + this.hasResult = hasResult; + this.expectLookup = expectLookup; + this.expectFallback = expectFallback; + this.lookupResultEmpty = lookupResultEmpty; + this.auths = auths; + this.lookupResult = lookupResult; + this.desc = desc; + } + } +}