b/275265215 Permit approval when role activated (#70)

Fix an issue where approval failed if the reviewer previously activated the same role
GoogleCloudPlatform · Apr 17, 2023 · d6eab1e · d6eab1e
1 parent 86dd282
commit d6eab1e
Show file tree

Hide file tree

Showing 4 changed files with 292 additions and 61 deletions.
diff --git a/...ces/src/main/java/com/google/solutions/jitaccess/core/services/RoleActivationService.java b/...ces/src/main/java/com/google/solutions/jitaccess/core/services/RoleActivationService.java
@@ -84,9 +84,19 @@ private void checkUserCanActivateProjectRole(
     RoleBinding roleBinding,
     ActivationType activationType
   ) throws AccessException, IOException {
+    //
+    // Check if the given role is among the roles that the
+    // user is eligible to JIT-/MPA-activate.
+    //
+    // NB. It doesn't matter whether the user has already
+    // activated the role.
+    //
     if (this.roleDiscoveryService.listEligibleProjectRoles(
         user,
-        ProjectId.fromFullResourceName(roleBinding.fullResourceName))
+        ProjectId.fromFullResourceName(roleBinding.fullResourceName),
+        EnumSet.of(
+          ProjectRole.Status.ELIGIBLE_FOR_JIT,
+          ProjectRole.Status.ELIGIBLE_FOR_MPA))
       .getItems()
       .stream()
       .filter(pr -> pr.roleBinding.equals(roleBinding))
@@ -219,18 +229,10 @@ public Activation activateProjectRoleForPeer(
       request.roleBinding,
       ActivationType.MPA);
 
-    try {
-      checkUserCanActivateProjectRole(
-        request.beneficiary,
-        request.roleBinding,
-        ActivationType.MPA);
-    }
-    catch (AccessDeniedException e) {
-      throw new AccessDeniedException(
-        String.format(
-          "The request has been approved already, or the user %s is no longer allowed to activate the role",
-          request.beneficiary));
-    }
+    checkUserCanActivateProjectRole(
+      request.beneficiary,
+      request.roleBinding,
+      ActivationType.MPA);
 
     //
     // Add time-bound IAM binding for the beneficiary.

diff --git a/sources/src/main/java/com/google/solutions/jitaccess/core/services/RoleDiscoveryService.java b/sources/src/main/java/com/google/solutions/jitaccess/core/services/RoleDiscoveryService.java
@@ -152,6 +152,23 @@ public Set<ProjectId> listAvailableProjects(
   public Result<ProjectRole> listEligibleProjectRoles(
     UserId user,
     ProjectId projectId
+  ) throws AccessException, IOException {
+    return listEligibleProjectRoles(
+      user,
+      projectId,
+      EnumSet.of(
+        ProjectRole.Status.ACTIVATED,
+        ProjectRole.Status.ELIGIBLE_FOR_JIT,
+        ProjectRole.Status.ELIGIBLE_FOR_MPA));
+  }
+
+  /**
+   * List eligible role bindings for the given user.
+   */
+  public Result<ProjectRole> listEligibleProjectRoles(
+    UserId user,
+    ProjectId projectId,
+    EnumSet<ProjectRole.Status> statusesToInclude
   ) throws AccessException, IOException {
     Preconditions.checkNotNull(user, "user");
     Preconditions.checkNotNull(projectId, "projectId");
@@ -182,39 +199,57 @@ public Result<ProjectRole> listEligibleProjectRoles(
     // the condition evaluates to true (indicating it's still
     // valid).
     //
-    var activatedRoles = findRoleBindings(
-      analysisResult,
-      condition -> JitConstraints.isActivated(condition),
-      evalResult -> "TRUE".equalsIgnoreCase(evalResult))
-      .stream()
-      .map(binding -> new ProjectRole(binding, ProjectRole.Status.ACTIVATED))
-      .collect(Collectors.toSet());
+    Set<ProjectRole> activatedRoles;
+    if (statusesToInclude.contains(ProjectRole.Status.ACTIVATED)) {
+      activatedRoles = findRoleBindings(
+        analysisResult,
+        condition -> JitConstraints.isActivated(condition),
+        evalResult -> "TRUE".equalsIgnoreCase(evalResult))
+        .stream()
+        .map(binding -> new ProjectRole(binding, ProjectRole.Status.ACTIVATED))
+        .collect(Collectors.toSet());
+    }
+    else {
+      activatedRoles = Set.of();
+    }
 
     //
     // Find all JIT-eligible role bindings. The bindings are
     // conditional and have a special condition that serves
     // as marker.
     //
-    var jitEligibleRoles = findRoleBindings(
-      analysisResult,
-      condition -> JitConstraints.isJitAccessConstraint(condition),
-      evalResult -> "CONDITIONAL".equalsIgnoreCase(evalResult))
-      .stream()
-      .map(binding -> new ProjectRole(binding, ProjectRole.Status.ELIGIBLE_FOR_JIT))
-      .collect(Collectors.toSet());
+    Set<ProjectRole> jitEligibleRoles;
+    if (statusesToInclude.contains(ProjectRole.Status.ELIGIBLE_FOR_JIT)) {
+      jitEligibleRoles = findRoleBindings(
+        analysisResult,
+        condition -> JitConstraints.isJitAccessConstraint(condition),
+        evalResult -> "CONDITIONAL".equalsIgnoreCase(evalResult))
+        .stream()
+        .map(binding -> new ProjectRole(binding, ProjectRole.Status.ELIGIBLE_FOR_JIT))
+        .collect(Collectors.toSet());
+    }
+    else {
+      jitEligibleRoles = Set.of();
+    }
 
     //
     // Find all MPA-eligible role bindings. The bindings are
     // conditional and have a special condition that serves
     // as marker.
     //
-    var mpaEligibleRoles = findRoleBindings(
-      analysisResult,
-      condition -> JitConstraints.isMultiPartyApprovalConstraint(condition),
-      evalResult -> "CONDITIONAL".equalsIgnoreCase(evalResult))
-      .stream()
-      .map(binding -> new ProjectRole(binding, ProjectRole.Status.ELIGIBLE_FOR_MPA))
-      .collect(Collectors.toSet());
+    Set<ProjectRole> mpaEligibleRoles;
+    if (statusesToInclude.contains(ProjectRole.Status.ELIGIBLE_FOR_MPA)) {
+      mpaEligibleRoles = findRoleBindings(
+        analysisResult,
+        condition -> JitConstraints.isMultiPartyApprovalConstraint(condition),
+        evalResult -> "CONDITIONAL".equalsIgnoreCase(evalResult))
+        .stream()
+        .map(binding -> new ProjectRole(binding, ProjectRole.Status.ELIGIBLE_FOR_MPA))
+        .collect(Collectors.toSet());
+    }
+    else {
+      mpaEligibleRoles = Set.of();
+    }
 
     //
     // Determine effective set of eligible roles. If a role is both JIT- and
@@ -252,7 +287,7 @@ public Result<ProjectRole> listEligibleProjectRoles(
   }
 
   /**
-   * List users that can approve the activation of an eligible role binding
+   * List users that can approve the activation of an eligible role binding.
    */
   public Set<UserId> listEligibleUsersForProjectRole(
     UserId callerUserId,

diff --git a/...src/test/java/com/google/solutions/jitaccess/core/services/TestRoleActivationService.java b/...src/test/java/com/google/solutions/jitaccess/core/services/TestRoleActivationService.java
@@ -88,7 +88,7 @@ public void whenCallerLacksRoleBinding_ThenActivateProjectRoleForSelfThrowsExcep
 
     var caller = SAMPLE_USER;
 
-    when(discoveryService.listEligibleProjectRoles(eq(caller), eq(SAMPLE_PROJECT_ID)))
+    when(discoveryService.listEligibleProjectRoles(eq(caller), eq(SAMPLE_PROJECT_ID), any()))
       .thenReturn(new Result<ProjectRole>(
         List.of(new ProjectRole(
           new RoleBinding(
@@ -168,7 +168,7 @@ public void whenCallerIsJitEligible_ThenActivateProjectRoleForSelfAddsBinding()
 
     var caller = SAMPLE_USER;
 
-    when(discoveryService.listEligibleProjectRoles(eq(caller), eq(SAMPLE_PROJECT_ID)))
+    when(discoveryService.listEligibleProjectRoles(eq(caller), eq(SAMPLE_PROJECT_ID), any()))
       .thenReturn(new Result<ProjectRole>(
         List.of(new ProjectRole(
           new RoleBinding(
@@ -281,7 +281,7 @@ public void whenRoleNotMpaEligibleForCaller_ThenActivateProjectRoleForPeerThrows
       SAMPLE_ROLE);
 
     var discoveryService = Mockito.mock(RoleDiscoveryService.class);
-    when(discoveryService.listEligibleProjectRoles(eq(caller), eq(SAMPLE_PROJECT_ID)))
+    when(discoveryService.listEligibleProjectRoles(eq(caller), eq(SAMPLE_PROJECT_ID), any()))
       .thenReturn(new Result<ProjectRole>(
         List.of(new ProjectRole(
           roleBinding,
@@ -322,7 +322,7 @@ public void whenRoleIsJitEligibleForCaller_ThenActivateProjectRoleForPeerThrowsE
       SAMPLE_ROLE);
 
     var discoveryService = Mockito.mock(RoleDiscoveryService.class);
-    when(discoveryService.listEligibleProjectRoles(eq(caller), eq(SAMPLE_PROJECT_ID)))
+    when(discoveryService.listEligibleProjectRoles(eq(caller), eq(SAMPLE_PROJECT_ID), any()))
       .thenReturn(new Result<ProjectRole>(
         List.of(new ProjectRole(
           roleBinding,
@@ -363,13 +363,13 @@ public void whenRoleNotEligibleForPeer_ThenActivateProjectRoleForPeerThrowsExcep
       SAMPLE_ROLE);
 
     var discoveryService = Mockito.mock(RoleDiscoveryService.class);
-    when(discoveryService.listEligibleProjectRoles(eq(caller), eq(SAMPLE_PROJECT_ID)))
+    when(discoveryService.listEligibleProjectRoles(eq(caller), eq(SAMPLE_PROJECT_ID), any()))
       .thenReturn(new Result<ProjectRole>(
         List.of(new ProjectRole(
           roleBinding,
           ProjectRole.Status.ELIGIBLE_FOR_MPA)),
         List.of()));
-    when(discoveryService.listEligibleProjectRoles(eq(peer), eq(SAMPLE_PROJECT_ID)))
+    when(discoveryService.listEligibleProjectRoles(eq(peer), eq(SAMPLE_PROJECT_ID), any()))
       .thenReturn(new Result<ProjectRole>(
         List.of(),
         List.of()));
@@ -409,13 +409,23 @@ public void whenPeerAndCallerEligible_ThenActivateProjectRoleAddsBinding() throw
 
     var resourceAdapter = Mockito.mock(ResourceManagerAdapter.class);
     var discoveryService = Mockito.mock(RoleDiscoveryService.class);
-    when(discoveryService.listEligibleProjectRoles(eq(caller), eq(SAMPLE_PROJECT_ID)))
+    when(discoveryService.listEligibleProjectRoles(
+        eq(caller),
+        eq(SAMPLE_PROJECT_ID),
+        eq(EnumSet.of(
+          ProjectRole.Status.ELIGIBLE_FOR_JIT,
+          ProjectRole.Status.ELIGIBLE_FOR_MPA))))
       .thenReturn(new Result<ProjectRole>(
         List.of(new ProjectRole(
           roleBinding,
           ProjectRole.Status.ELIGIBLE_FOR_MPA)),
         List.of()));
-    when(discoveryService.listEligibleProjectRoles(eq(peer), eq(SAMPLE_PROJECT_ID)))
+    when(discoveryService.listEligibleProjectRoles(
+        eq(peer),
+        eq(SAMPLE_PROJECT_ID),
+        eq(EnumSet.of(
+          ProjectRole.Status.ELIGIBLE_FOR_JIT,
+          ProjectRole.Status.ELIGIBLE_FOR_MPA))))
       .thenReturn(new Result<ProjectRole>(
         List.of(new ProjectRole(
           roleBinding,
@@ -475,13 +485,13 @@ public void whenRoleAlreadyActivated_ThenActivateProjectRoleAddsBinding() throws
 
     var resourceAdapter = Mockito.mock(ResourceManagerAdapter.class);
     var discoveryService = Mockito.mock(RoleDiscoveryService.class);
-    when(discoveryService.listEligibleProjectRoles(eq(caller), eq(SAMPLE_PROJECT_ID)))
+    when(discoveryService.listEligibleProjectRoles(eq(caller), eq(SAMPLE_PROJECT_ID), any()))
       .thenReturn(new Result<ProjectRole>(
         List.of(new ProjectRole(
           roleBinding,
           ProjectRole.Status.ELIGIBLE_FOR_MPA)),
         List.of()));
-    when(discoveryService.listEligibleProjectRoles(eq(peer), eq(SAMPLE_PROJECT_ID)))
+    when(discoveryService.listEligibleProjectRoles(eq(peer), eq(SAMPLE_PROJECT_ID), any()))
       .thenReturn(new Result<ProjectRole>(
         List.of(new ProjectRole(
           roleBinding,
@@ -649,7 +659,7 @@ public void whenRoleNotMpaEligibleForCaller_ThenCreateActivationRequestForPeerTh
       SAMPLE_ROLE);
 
     var discoveryService = Mockito.mock(RoleDiscoveryService.class);
-    when(discoveryService.listEligibleProjectRoles(eq(caller), eq(SAMPLE_PROJECT_ID)))
+    when(discoveryService.listEligibleProjectRoles(eq(caller), eq(SAMPLE_PROJECT_ID), any()))
       .thenReturn(new Result<ProjectRole>(
         List.of(new ProjectRole(
           roleBinding,
@@ -684,7 +694,7 @@ public void whenRoleIsJitEligibleForCaller_ThenCreateActivationRequestForPeerThr
       SAMPLE_ROLE);
 
     var discoveryService = Mockito.mock(RoleDiscoveryService.class);
-    when(discoveryService.listEligibleProjectRoles(eq(caller), eq(SAMPLE_PROJECT_ID)))
+    when(discoveryService.listEligibleProjectRoles(eq(caller), eq(SAMPLE_PROJECT_ID), any()))
       .thenReturn(new Result<ProjectRole>(
         List.of(new ProjectRole(
           roleBinding,
@@ -719,7 +729,7 @@ public void whenCallerEligible_ThenCreateActivationRequestForPeerSucceeds() thro
       SAMPLE_ROLE);
 
     var discoveryService = Mockito.mock(RoleDiscoveryService.class);
-    when(discoveryService.listEligibleProjectRoles(eq(caller), eq(SAMPLE_PROJECT_ID)))
+    when(discoveryService.listEligibleProjectRoles(eq(caller), eq(SAMPLE_PROJECT_ID), any()))
       .thenReturn(new Result<ProjectRole>(
         List.of(new ProjectRole(
           roleBinding,