From 442689b8c48891c40261b51c9ab511f0a7c19513 Mon Sep 17 00:00:00 2001
From: Edwin Kempin <ekempin@google.com>
Date: Thu, 28 Jun 2018 08:43:26 +0200
Subject: [PATCH] Don't fail with ISE on creating an access change with an
 invalid ref

If an invalid ref is provided in the user input we should rather fail
with '400 Bad Request' and provide an error message to the user.

Error in PUT /projects/All-Projects/access:review
com.google.gerrit.common.errors.InvalidNameException: Invalid Name: refs/heads/stable_*
  at com.google.gerrit.server.project.RefPattern.validate(RefPattern.java:93)
  at com.google.gerrit.server.restapi.project.SetAccessUtil.validateChanges(SetAccessUtil.java:152)
  at com.google.gerrit.server.restapi.project.CreateAccessChange.apply(CreateAccessChange.java:124)
  at com.google.gerrit.server.restapi.project.CreateAccessChange.apply(CreateAccessChange.java:60)
  at com.google.gerrit.httpd.restapi.RestApiServlet.service(RestApiServlet.java:418)
  at javax.servlet.http.HttpServlet.service(HttpServlet.java:717)
  ...

Change-Id: I40053f0736f3f43281059da40f27d550eec68626
Signed-off-by: Edwin Kempin <ekempin@google.com>
---
 .../acceptance/rest/project/AccessIT.java     | 28 +++++++++++++++++++
 .../server/project/CreateAccessChange.java    |  3 ++
 2 files changed, 31 insertions(+)

diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/AccessIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/AccessIT.java
index f67012af76..a0c8275fd0 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/AccessIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/AccessIT.java
@@ -524,6 +524,34 @@ public class AccessIT extends AbstractDaemonTest {
     assertThat(cfg.getString(access, refsFor, unknownPermission)).isEqualTo(registeredUsers);
   }
 
+  @Test
+  public void addAccessSectionForInvalidRef() throws Exception {
+    ProjectAccessInput accessInput = newProjectAccessInput();
+    AccessSectionInfo accessSectionInfo = createDefaultAccessSectionInfo();
+
+    // 'refs/heads/stable_*' is invalid, correct would be '^refs/heads/stable_.*'
+    String invalidRef = Constants.R_HEADS + "stable_*";
+    accessInput.add.put(invalidRef, accessSectionInfo);
+
+    exception.expect(BadRequestException.class);
+    exception.expectMessage("Invalid Name: " + invalidRef);
+    pApi.access(accessInput);
+  }
+
+  @Test
+  public void createAccessChangeWithAccessSectionForInvalidRef() throws Exception {
+    ProjectAccessInput accessInput = newProjectAccessInput();
+    AccessSectionInfo accessSectionInfo = createDefaultAccessSectionInfo();
+
+    // 'refs/heads/stable_*' is invalid, correct would be '^refs/heads/stable_.*'
+    String invalidRef = Constants.R_HEADS + "stable_*";
+    accessInput.add.put(invalidRef, accessSectionInfo);
+
+    exception.expect(BadRequestException.class);
+    exception.expectMessage("Invalid Name: " + invalidRef);
+    pApi.accessChange(accessInput);
+  }
+
   private ProjectAccessInput newProjectAccessInput() {
     ProjectAccessInput p = new ProjectAccessInput();
     p.add = new HashMap<>();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateAccessChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateAccessChange.java
index 326d395d75..459a413f19 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateAccessChange.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/CreateAccessChange.java
@@ -22,6 +22,7 @@ import com.google.gerrit.common.errors.PermissionDeniedException;
 import com.google.gerrit.extensions.api.access.ProjectAccessInput;
 import com.google.gerrit.extensions.common.ChangeInfo;
 import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.BadRequestException;
 import com.google.gerrit.extensions.restapi.Response;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.extensions.restapi.RestModifyView;
@@ -145,6 +146,8 @@ public class CreateAccessChange implements RestModifyView<ProjectResource, Proje
         bu.execute();
         return Response.created(jsonFactory.noOptions().format(ins.getChange()));
       }
+    } catch (InvalidNameException e) {
+      throw new BadRequestException(e.toString());
     }
   }