b/273390621 Add configuration option for max number of reviewers (#54)

* List default limit of 10 reviewers for an activation request * Add configuration option ACTIVATION_REQUEST_MAX_REVIEWERS * Validate number of peers in RoleActivationService * Check number of reviewers in API resource to ensure that a proper error is shown to users
GoogleCloudPlatform · Mar 14, 2023 · d235671 · d235671
1 parent 96fca59
commit d235671
Show file tree

Hide file tree

Showing 6 changed files with 189 additions and 43 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
@@ -284,6 +284,8 @@ public ActivationRequest createActivationRequestForPeer(
 
     Preconditions.checkArgument(ProjectId.isProjectFullResourceName(roleBinding.fullResourceName));
     Preconditions.checkArgument(!reviewers.isEmpty(), "At least one reviewer must be provided");
+    Preconditions.checkArgument(reviewers.size() <= this.options.maxNumberOfReviewersPerActivationRequest,
+      "The number of reviewers must not exceed " + this.options.maxNumberOfReviewersPerActivationRequest);
     Preconditions.checkArgument(!reviewers.contains(callerAndBeneficiary), "The beneficiary cannot be a reviewer");
 
     //
@@ -474,14 +476,17 @@ public static class Options {
     public final Duration activationDuration;
     public final String justificationHint;
     public final Pattern justificationPattern;
+    public final int maxNumberOfReviewersPerActivationRequest;
 
     public Options(
       String justificationHint,
       Pattern justificationPattern,
-      Duration activationDuration) {
+      Duration activationDuration,
+      int maxNumberOfReviewersPerActivationRequest) {
       this.activationDuration = activationDuration;
       this.justificationHint = justificationHint;
       this.justificationPattern = justificationPattern;
+      this.maxNumberOfReviewersPerActivationRequest = maxNumberOfReviewersPerActivationRequest;
     }
   }
 }
diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/ApiResource.java b/sources/src/main/java/com/google/solutions/jitaccess/web/ApiResource.java
@@ -369,6 +369,8 @@ public ActivationStatusResponse requestActivation(
     assert this.activationTokenService != null;
     assert this.notificationService != null;
 
+    var maxReviewers = this.roleActivationService.getOptions().maxNumberOfReviewersPerActivationRequest;
+
     Preconditions.checkArgument(
       projectIdString != null && !projectIdString.trim().isEmpty(),
       "A projectId is required");
@@ -377,8 +379,11 @@ public ActivationStatusResponse requestActivation(
       request.role != null && !request.role.isEmpty(),
       "A role is required");
     Preconditions.checkArgument(
-      request.peers != null && request.peers.size() > 0 && request.peers.size() <= 10,
+      request.peers != null && request.peers.size() > 0,
       "At least one peer is required");
+    Preconditions.checkArgument(
+      request.peers.size() <= maxReviewers,
+      "The number of reviewers must not exceed " + maxReviewers);
     Preconditions.checkArgument(
       request.justification != null && request.justification.length() > 0 && request.justification.length() < 100,
       "A justification must be provided");

diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/RuntimeConfiguration.java b/sources/src/main/java/com/google/solutions/jitaccess/web/RuntimeConfiguration.java
@@ -59,6 +59,9 @@ public RuntimeConfiguration(Function<String, String> readSetting) {
     this.justificationHint = new StringSetting(
       List.of("JUSTIFICATION_HINT"),
       "Bug or case number");
+    this.maxNumberOfReviewersPerActivationRequest = new IntSetting(
+      List.of("ACTIVATION_REQUEST_MAX_REVIEWERS"),
+      10);
 
     //
     // Backend service id
@@ -164,6 +167,11 @@ public RuntimeConfiguration(Function<String, String> readSetting) {
    */
   public final StringSetting backendServiceId;
 
+  /**
+   * Maximum number of reviewers foa an activation request.
+   */
+  public final IntSetting maxNumberOfReviewersPerActivationRequest;
+
   public boolean isSmtpConfigured() {
     var requiredSettings = List.of(smtpHost, smtpPort, smtpSenderName, smtpSenderAddress);
     return requiredSettings.stream().allMatch(s -> s.isValid());

diff --git a/sources/src/main/java/com/google/solutions/jitaccess/web/RuntimeEnvironment.java b/sources/src/main/java/com/google/solutions/jitaccess/web/RuntimeEnvironment.java
@@ -39,7 +39,6 @@
 import com.google.solutions.jitaccess.core.services.NotificationService;
 import com.google.solutions.jitaccess.core.services.RoleActivationService;
 import com.google.solutions.jitaccess.core.services.RoleDiscoveryService;
-import com.google.solutions.jitaccess.web.RuntimeConfiguration.StringSetting;
 
 import javax.enterprise.context.ApplicationScoped;
 import javax.enterprise.inject.Produces;
@@ -294,7 +293,8 @@ public RoleActivationService.Options getRoleActivationServiceOptions() {
     return new RoleActivationService.Options(
       this.configuration.justificationHint.getValue(),
       Pattern.compile(this.configuration.justificationPattern.getValue()),
-      this.configuration.activationTimeout.getValue());
+      this.configuration.activationTimeout.getValue(),
+      this.configuration.maxNumberOfReviewersPerActivationRequest.getValue());
   }
 
   @Produces

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
@@ -22,7 +22,6 @@
 package com.google.solutions.jitaccess.core.services;
 
 import com.google.solutions.jitaccess.core.AccessDeniedException;
-import com.google.solutions.jitaccess.core.AlreadyExistsException;
 import com.google.solutions.jitaccess.core.adapters.ResourceManagerAdapter;
 import com.google.solutions.jitaccess.core.data.ProjectId;
 import com.google.solutions.jitaccess.core.data.ProjectRole;
@@ -48,7 +47,8 @@ public class TestRoleActivationService {
   private static final ProjectId SAMPLE_PROJECT_ID = new ProjectId("project-1");
   private static final String SAMPLE_PROJECT_RESOURCE_1 = "//cloudresourcemanager.googleapis.com/projects/project-1";
   private static final String SAMPLE_ROLE = "roles/resourcemanager.projectIamAdmin";
-  private static final Pattern JUSTIFICATION_PATTERN = Pattern.compile(".*");
+  private static final Pattern DEFAULT_JUSTIFICATION_PATTERN = Pattern.compile(".*");
+  private static final int DEFAULT_MAX_NUMBER_OF_REVIEWERS = 10;
 
   // ---------------------------------------------------------------------
   // activateProjectRoleForSelf.
@@ -64,8 +64,9 @@ public void whenResourceIsNotAProject_ThenActivateProjectRoleForSelfThrowsExcept
       resourceAdapter,
       new RoleActivationService.Options(
         "hint",
-        JUSTIFICATION_PATTERN,
-        Duration.ofMinutes(1)));
+        DEFAULT_JUSTIFICATION_PATTERN,
+        Duration.ofMinutes(1),
+        DEFAULT_MAX_NUMBER_OF_REVIEWERS));
 
     assertThrows(IllegalArgumentException.class,
       () -> service.activateProjectRoleForSelf(
@@ -97,8 +98,9 @@ public void whenCallerLacksRoleBinding_ThenActivateProjectRoleForSelfThrowsExcep
       resourceAdapter,
       new RoleActivationService.Options(
         "hint",
-        JUSTIFICATION_PATTERN,
-        Duration.ofMinutes(1)));
+        DEFAULT_JUSTIFICATION_PATTERN,
+        Duration.ofMinutes(1),
+        DEFAULT_MAX_NUMBER_OF_REVIEWERS));
 
     assertThrows(AccessDeniedException.class,
       () -> service.activateProjectRoleForSelf(
@@ -117,7 +119,8 @@ public void whenJustificationDoesNotMatch_ThenActivateProjectRoleForSelfThrowsEx
       new RoleActivationService.Options(
         "hint",
         Pattern.compile("^\\d+$"),
-        Duration.ofMinutes(1)));
+        Duration.ofMinutes(1),
+        DEFAULT_MAX_NUMBER_OF_REVIEWERS));
 
     assertThrows(AccessDeniedException.class,
       () -> service.activateProjectRoleForSelf(
@@ -149,8 +152,9 @@ public void whenCallerIsJitEligible_ThenActivateProjectRoleForSelfAddsBinding()
       resourceAdapter,
       new RoleActivationService.Options(
         "hint",
-        JUSTIFICATION_PATTERN,
-        Duration.ofMinutes(1)));
+        DEFAULT_JUSTIFICATION_PATTERN,
+        Duration.ofMinutes(1),
+        DEFAULT_MAX_NUMBER_OF_REVIEWERS));
 
     var roleBinding = new RoleBinding(
       SAMPLE_PROJECT_RESOURCE_1,
@@ -188,8 +192,9 @@ public void whenCallerSameAsBeneficiary_ThenActivateProjectRoleForPeerThrowsExce
       Mockito.mock(ResourceManagerAdapter.class),
       new RoleActivationService.Options(
         "hint",
-        JUSTIFICATION_PATTERN,
-        Duration.ofMinutes(1)));
+        DEFAULT_JUSTIFICATION_PATTERN,
+        Duration.ofMinutes(1),
+        DEFAULT_MAX_NUMBER_OF_REVIEWERS));
 
     var request = RoleActivationService.ActivationRequest.createForTestingOnly(
       RoleActivationService.ActivationId.newId(RoleActivationService.ActivationType.MPA),
@@ -213,8 +218,9 @@ public void whenCallerNotListedAsReviewer_ThenActivateProjectRoleForPeerThrowsEx
       Mockito.mock(ResourceManagerAdapter.class),
       new RoleActivationService.Options(
         "hint",
-        JUSTIFICATION_PATTERN,
-        Duration.ofMinutes(1)));
+        DEFAULT_JUSTIFICATION_PATTERN,
+        Duration.ofMinutes(1),
+        DEFAULT_MAX_NUMBER_OF_REVIEWERS));
 
     var request = RoleActivationService.ActivationRequest.createForTestingOnly(
       RoleActivationService.ActivationId.newId(RoleActivationService.ActivationType.MPA),
@@ -263,8 +269,9 @@ public void whenRoleNotMpaEligibleForCaller_ThenActivateProjectRoleForPeerThrows
       Mockito.mock(ResourceManagerAdapter.class),
       new RoleActivationService.Options(
         "hint",
-        JUSTIFICATION_PATTERN,
-        Duration.ofMinutes(1)));
+        DEFAULT_JUSTIFICATION_PATTERN,
+        Duration.ofMinutes(1),
+        DEFAULT_MAX_NUMBER_OF_REVIEWERS));
 
     assertThrows(AccessDeniedException.class,
       () -> service.activateProjectRoleForPeer(caller, request));
@@ -291,8 +298,9 @@ public void whenRoleIsJitEligibleForCaller_ThenActivateProjectRoleForPeerThrowsE
       Mockito.mock(ResourceManagerAdapter.class),
       new RoleActivationService.Options(
         "hint",
-        JUSTIFICATION_PATTERN,
-        Duration.ofMinutes(1)));
+        DEFAULT_JUSTIFICATION_PATTERN,
+        Duration.ofMinutes(1),
+        DEFAULT_MAX_NUMBER_OF_REVIEWERS));
 
     var request = RoleActivationService.ActivationRequest.createForTestingOnly(
       RoleActivationService.ActivationId.newId(RoleActivationService.ActivationType.MPA),
@@ -334,8 +342,9 @@ public void whenRoleNotEligibleForPeer_ThenActivateProjectRoleForPeerThrowsExcep
       Mockito.mock(ResourceManagerAdapter.class),
       new RoleActivationService.Options(
         "hint",
-        JUSTIFICATION_PATTERN,
-        Duration.ofMinutes(1)));
+        DEFAULT_JUSTIFICATION_PATTERN,
+        Duration.ofMinutes(1),
+        DEFAULT_MAX_NUMBER_OF_REVIEWERS));
 
     var request = RoleActivationService.ActivationRequest.createForTestingOnly(
       RoleActivationService.ActivationId.newId(RoleActivationService.ActivationType.MPA),
@@ -380,8 +389,9 @@ public void whenPeerAndCallerEligible_ThenActivateProjectRoleAddsBinding() throw
       resourceAdapter,
       new RoleActivationService.Options(
         "hint",
-        JUSTIFICATION_PATTERN,
-        Duration.ofMinutes(1)));
+        DEFAULT_JUSTIFICATION_PATTERN,
+        Duration.ofMinutes(1),
+        DEFAULT_MAX_NUMBER_OF_REVIEWERS));
 
     var issuedAt = 1000L;
     var request = RoleActivationService.ActivationRequest.createForTestingOnly(
@@ -444,8 +454,9 @@ public void whenRoleAlreadyActivated_ThenActivateProjectRoleAddsBinding() throws
       resourceAdapter,
       new RoleActivationService.Options(
         "hint",
-        JUSTIFICATION_PATTERN,
-        Duration.ofMinutes(1)));
+        DEFAULT_JUSTIFICATION_PATTERN,
+        Duration.ofMinutes(1),
+        DEFAULT_MAX_NUMBER_OF_REVIEWERS));
 
     var issuedAt = 1000L;
     var request = RoleActivationService.ActivationRequest.createForTestingOnly(
@@ -474,15 +485,37 @@ public void whenRoleAlreadyActivated_ThenActivateProjectRoleAddsBinding() throws
   // createActivationRequestForPeer.
   // ---------------------------------------------------------------------
 
+  @Test
+  public void whenNumberOfReviewersExceedLimit_ThenCreateActivationRequestForPeerThrowsException() {
+    var service = new RoleActivationService(
+      Mockito.mock(RoleDiscoveryService.class),
+      Mockito.mock(ResourceManagerAdapter.class),
+      new RoleActivationService.Options(
+        "hint",
+        DEFAULT_JUSTIFICATION_PATTERN,
+        Duration.ofMinutes(1),
+        2));
+
+    assertThrows(IllegalArgumentException.class,
+      () -> service.createActivationRequestForPeer(
+        SAMPLE_USER,
+        Set.of(SAMPLE_USER, SAMPLE_USER_2, SAMPLE_USER_3),
+        new RoleBinding(
+          SAMPLE_PROJECT_RESOURCE_1,
+          SAMPLE_ROLE),
+        "justification"));
+  }
+
   @Test
   public void whenReviewerIncludesBeneficiary_ThenCreateActivationRequestForPeerThrowsException() {
     var service = new RoleActivationService(
       Mockito.mock(RoleDiscoveryService.class),
       Mockito.mock(ResourceManagerAdapter.class),
       new RoleActivationService.Options(
         "hint",
-        JUSTIFICATION_PATTERN,
-        Duration.ofMinutes(1)));
+        DEFAULT_JUSTIFICATION_PATTERN,
+        Duration.ofMinutes(1),
+        DEFAULT_MAX_NUMBER_OF_REVIEWERS));
 
     assertThrows(IllegalArgumentException.class,
       () -> service.createActivationRequestForPeer(
@@ -502,7 +535,8 @@ public void whenJustificationDoesNotMatch_ThenCreateActivationRequestForPeerThro
       new RoleActivationService.Options(
         "hint",
         Pattern.compile("^\\d+$"),
-        Duration.ofMinutes(1)));
+        Duration.ofMinutes(1),
+        DEFAULT_MAX_NUMBER_OF_REVIEWERS));
 
     assertThrows(AccessDeniedException.class,
       () -> service.createActivationRequestForPeer(
@@ -535,8 +569,9 @@ public void whenRoleNotMpaEligibleForCaller_ThenCreateActivationRequestForPeerTh
       Mockito.mock(ResourceManagerAdapter.class),
       new RoleActivationService.Options(
         "hint",
-        JUSTIFICATION_PATTERN,
-        Duration.ofMinutes(1)));
+        DEFAULT_JUSTIFICATION_PATTERN,
+        Duration.ofMinutes(1),
+        DEFAULT_MAX_NUMBER_OF_REVIEWERS));
 
     assertThrows(AccessDeniedException.class,
       () -> service.createActivationRequestForPeer(
@@ -567,8 +602,9 @@ public void whenRoleIsJitEligibleForCaller_ThenCreateActivationRequestForPeerThr
       Mockito.mock(ResourceManagerAdapter.class),
       new RoleActivationService.Options(
         "hint",
-        JUSTIFICATION_PATTERN,
-        Duration.ofMinutes(1)));
+        DEFAULT_JUSTIFICATION_PATTERN,
+        Duration.ofMinutes(1),
+        DEFAULT_MAX_NUMBER_OF_REVIEWERS));
 
     assertThrows(AccessDeniedException.class,
       () -> service.createActivationRequestForPeer(
@@ -599,8 +635,9 @@ public void whenCallerEligible_ThenCreateActivationRequestForPeerSucceeds() thro
       Mockito.mock(ResourceManagerAdapter.class),
       new RoleActivationService.Options(
         "hint",
-        JUSTIFICATION_PATTERN,
-        Duration.ofMinutes(1)));
+        DEFAULT_JUSTIFICATION_PATTERN,
+        Duration.ofMinutes(1),
+        DEFAULT_MAX_NUMBER_OF_REVIEWERS));
 
     var request = service.createActivationRequestForPeer(
         caller,