Merge pull request #1001 from spotify/guard-check-role

Guard checkRole
spotify · Aug 5, 2022 · f9cf238 · f9cf238
2 parents 7149885 + 095fe2e
commit f9cf238
Show file tree

Hide file tree

Showing 2 changed files with 38 additions and 2 deletions.
diff --git a/styx-service-common/src/main/java/com/spotify/styx/api/ServiceAccountUsageAuthorizer.java b/styx-service-common/src/main/java/com/spotify/styx/api/ServiceAccountUsageAuthorizer.java
@@ -224,8 +224,7 @@ public ServiceAccountUsageAuthorizationResult checkServiceAccountUsageAuthorizat
       final Supplier<String> projectIdSupplier = Suppliers.memoize(() -> serviceAccountProjectId(serviceAccount));
 
       return checkIsPrincipalBlacklisted(principalEmail)
-          .or(() -> checkRole(serviceAccount, principalEmail, projectIdSupplier))
-          .or(() -> checkIsPrincipalAdmin(principalEmail))
+          .or(() -> checkRoleOrIsPrincipalAdmin(serviceAccount, principalEmail, projectIdSupplier))
           .orElseGet(() -> deny(serviceAccount, principalEmail, projectIdSupplier));
     }
 
@@ -238,6 +237,31 @@ private Optional<ServiceAccountUsageAuthorizationResult> checkIsPrincipalBlackli
               .build());
     }
 
+    private Optional<ServiceAccountUsageAuthorizationResult> checkRoleOrIsPrincipalAdmin(String serviceAccount,
+                                                                                         String principalEmail,
+                                                                                         Supplier<String> projectIdSupplier) {
+
+      Optional<ServiceAccountUsageAuthorizationResult> result = Optional.empty();
+      RuntimeException checkRoleException = null;
+
+      try {
+        result = checkRole(serviceAccount, principalEmail, projectIdSupplier);
+      } catch (RuntimeException e) {
+        checkRoleException = e;
+      }
+
+      if (result.isPresent()) {
+        return result;
+      }
+
+      result = checkIsPrincipalAdmin(principalEmail);
+
+      if (result.isEmpty() && checkRoleException != null) {
+        throw checkRoleException;
+      }
+      return result;
+    }
+
     private Optional<ServiceAccountUsageAuthorizationResult> checkIsPrincipalAdmin(
         String principalEmail) {
       return memberStatus(principalEmail, administrators)

diff --git a/...-service-common/src/test/java/com/spotify/styx/api/ServiceAccountUsageAuthorizerTest.java b/...-service-common/src/test/java/com/spotify/styx/api/ServiceAccountUsageAuthorizerTest.java
@@ -357,6 +357,18 @@ public void shouldAuthorizeIfPrincipalIsAdminViaGroup(String serviceAccount) thr
     assertCachedSuccess(() -> sut.authorizeServiceAccountUsage(WORKFLOW_ID, serviceAccount, idToken));
   }
 
+  @Parameters({SERVICE_ACCOUNT, MANAGED_SERVICE_ACCOUNT})
+  @Test
+  public void shouldAuthorizeIfPrincipalIsAdminViaGroupEvenCheckRoleFails(String serviceAccount) throws IOException {
+    final Throwable cause = googleJsonResponseException(418);
+    var errorRequest = mock(Directory.Members.HasMember.class);
+    doThrow(cause).when(errorRequest).execute();
+    doReturn(errorRequest).when(members).hasMember(PROJECT_ADMINS_GROUP_EMAIL, PRINCIPAL_EMAIL);
+    doReturn(isMember).when(members).hasMember(STYX_ADMINS_GROUP_EMAIL, PRINCIPAL_EMAIL);
+    assertCachedSuccess(() -> sut.authorizeServiceAccountUsage(WORKFLOW_ID, serviceAccount, idToken));
+  }
+
+
   @Parameters({SERVICE_ACCOUNT, MANAGED_SERVICE_ACCOUNT})
   @Test
   public void shouldAuthorizeIfPrincipalHasUserRoleOnProjectViaGroup(String serviceAccount) throws IOException {