From bccf27e26091761144cf1f32363f8aa42dd3b21f Mon Sep 17 00:00:00 2001 From: Changcheng Xiao Date: Mon, 28 Jan 2019 14:22:32 +0100 Subject: [PATCH] CreateChange: move permission checks to its own method This commit groups those permission checks to a dedicated method. It also renames "rsrc" to "projectResource" to make it more readable. Change-Id: I7b9ed0f9aa3d4af5e3fb1a9d0b9de96455d605a8 --- .../server/restapi/change/CreateChange.java | 52 ++++++++++--------- .../acceptance/api/change/ChangeIT.java | 4 +- .../rest/change/CreateChangeIT.java | 3 +- 3 files changed, 32 insertions(+), 27 deletions(-) diff --git a/java/com/google/gerrit/server/restapi/change/CreateChange.java b/java/com/google/gerrit/server/restapi/change/CreateChange.java index 5f801b7ac4..91699cdc91 100644 --- a/java/com/google/gerrit/server/restapi/change/CreateChange.java +++ b/java/com/google/gerrit/server/restapi/change/CreateChange.java @@ -164,24 +164,14 @@ public class CreateChange UpdateException, PermissionBackendException, ConfigInvalidException { checkAndSanitizeChangeInput(input); - ProjectResource rsrc = projectsCollection.parse(input.project); + ProjectResource projectResource = projectsCollection.parse(input.project); + ProjectState projectState = projectResource.getProjectState(); + projectState.checkStatePermitsWrite(); - contributorAgreements.check(rsrc.getNameKey(), rsrc.getUser()); + Project.NameKey project = projectResource.getNameKey(); + contributorAgreements.check(project, user.get()); - Project.NameKey project = rsrc.getNameKey(); - String refName = RefNames.fullName(input.branch); - try { - permissionBackend.currentUser().project(project).ref(refName).check(RefPermission.READ); - } catch (AuthException e) { - throw new ResourceNotFoundException(String.format("ref %s not found", refName)); - } - - permissionBackend - .currentUser() - .project(project) - .ref(refName) - .check(RefPermission.CREATE_CHANGE); - rsrc.getProjectState().checkStatePermitsWrite(); + checkRequiredPermissions(project, input.branch); try (Repository git = gitManager.openRepository(project); ObjectInserter oi = git.newObjectInserter(); @@ -189,7 +179,7 @@ public class CreateChange RevWalk rw = new RevWalk(reader)) { ObjectId parentCommit; List groups; - Ref destRef = git.getRefDatabase().exactRef(refName); + Ref destRef = git.getRefDatabase().exactRef(input.branch); if (input.baseChange != null) { List notes = changeFinder.find(input.baseChange); if (notes.size() != 1) { @@ -215,14 +205,14 @@ public class CreateChange RevCommit destRefRevCommit = rw.parseCommit(destRef.getObjectId()); if (!rw.isMergedInto(parentRevCommit, destRefRevCommit)) { throw new BadRequestException( - String.format("Commit %s doesn't exist on ref %s", input.baseCommit, refName)); + String.format("Commit %s doesn't exist on ref %s", input.baseCommit, input.branch)); } groups = Collections.emptyList(); } else { if (destRef != null) { if (Boolean.TRUE.equals(input.newBranch)) { throw new ResourceConflictException( - String.format("Branch %s already exists.", refName)); + String.format("Branch %s already exists.", input.branch)); } parentCommit = destRef.getObjectId(); } else { @@ -244,7 +234,7 @@ public class CreateChange boolean isWorkInProgress = input.workInProgress == null - ? rsrc.getProjectState().is(BooleanProjectConfig.WORK_IN_PROGRESS_BY_DEFAULT) + ? projectState.is(BooleanProjectConfig.WORK_IN_PROGRESS_BY_DEFAULT) || MoreObjects.firstNonNull(info.workInProgressByDefault, false) : input.workInProgress; @@ -274,16 +264,14 @@ public class CreateChange || submitType.equals(SubmitType.MERGE_IF_NECESSARY))) { throw new BadRequestException("Submit type: " + submitType + " is not supported"); } - c = - newMergeCommit( - git, oi, rw, rsrc.getProjectState(), mergeTip, input.merge, author, commitMessage); + c = newMergeCommit(git, oi, rw, projectState, mergeTip, input.merge, author, commitMessage); } else { // create an empty commit c = newCommit(oi, rw, author, mergeTip, commitMessage); } Change.Id changeId = new Change.Id(seq.nextChangeId()); - ChangeInserter ins = changeInserterFactory.create(changeId, c, refName); + ChangeInserter ins = changeInserterFactory.create(changeId, c, input.branch); ins.setMessage(String.format("Uploaded patch set %s.", ins.getPatchSetId().get())); ins.setTopic(input.topic); ins.setPrivate(input.isPrivate); @@ -321,6 +309,7 @@ public class CreateChange if (Strings.isNullOrEmpty(input.branch)) { throw new BadRequestException("branch must be non-empty"); } + input.branch = RefNames.fullName(input.branch); String subject = Strings.nullToEmpty(input.subject); subject = subject.replaceAll("(?m)^#.*$\n?", "").trim(); @@ -352,6 +341,21 @@ public class CreateChange input.isPrivate = isPrivate; } + private void checkRequiredPermissions(Project.NameKey project, String refName) + throws ResourceNotFoundException, AuthException, PermissionBackendException { + try { + permissionBackend.currentUser().project(project).ref(refName).check(RefPermission.READ); + } catch (AuthException e) { + throw new ResourceNotFoundException(String.format("ref %s not found", refName)); + } + + permissionBackend + .currentUser() + .project(project) + .ref(refName) + .check(RefPermission.CREATE_CHANGE); + } + private static RevCommit newCommit( ObjectInserter oi, RevWalk rw, diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java index 04e23ae068..1c335c6da3 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -2451,7 +2451,7 @@ public class ChangeIT extends AbstractDaemonTest { in.project = project.get(); ChangeInfo info = gApi.changes().create(in).get(); assertThat(info.project).isEqualTo(in.project); - assertThat(info.branch).isEqualTo(in.branch); + assertThat(RefNames.fullName(info.branch)).isEqualTo(RefNames.fullName(in.branch)); assertThat(info.subject).isEqualTo(in.subject); assertThat(Iterables.getOnlyElement(info.messages).message).isEqualTo("Uploaded patch set 1."); } @@ -2899,7 +2899,7 @@ public class ChangeIT extends AbstractDaemonTest { in.newBranch = true; ChangeInfo info = gApi.changes().create(in).get(); assertThat(info.project).isEqualTo(in.project); - assertThat(info.branch).isEqualTo(in.branch); + assertThat(RefNames.fullName(info.branch)).isEqualTo(RefNames.fullName(in.branch)); assertThat(info.subject).isEqualTo(in.subject); assertThat(Iterables.getOnlyElement(info.messages).message).isEqualTo("Uploaded patch set 1."); } diff --git a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java index bfb306bfeb..bad8843afb 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java @@ -48,6 +48,7 @@ import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.reviewdb.client.Branch; import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.submit.ChangeAlreadyMergedException; import com.google.gerrit.testing.FakeEmailSender.Message; import com.google.gerrit.testing.TestTimeUtil; @@ -483,7 +484,7 @@ public class CreateChangeIT extends AbstractDaemonTest { private ChangeInfo assertCreateSucceeds(ChangeInput in) throws Exception { ChangeInfo out = gApi.changes().create(in).get(); assertThat(out.project).isEqualTo(in.project); - assertThat(out.branch).isEqualTo(in.branch); + assertThat(RefNames.fullName(out.branch)).isEqualTo(RefNames.fullName(in.branch)); assertThat(out.subject).isEqualTo(in.subject.split("\n")[0]); assertThat(out.topic).isEqualTo(in.topic); assertThat(out.status).isEqualTo(in.status);