From 61e7176bf0defc2d39c48ab665854442df1e18c5 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Tue, 27 Jun 2017 09:56:38 +0200 Subject: [PATCH] Do not require 'Create Reference' for pushing annotated tag Change I80ad478522 added an unconditional check for the 'Create Reference' permission in ReceiveCommands#parseCreate(...). Checking this permission on push of annotated tags is wrong as pushing annotated tags (for commits that already exist in the Gerrit repository) should (only) require the 'Create Annotated Tag' permission. The correct permission checks for pushing new branches and tags are implemented in RefControl#canCreate(...) and this method is also invoked from ReceiveCommands#parseCreate(...). This means we can simply drop the check for the 'Create Reference' permission in ReceiveCommands#parseCreate(...) as RefControl#canCreate(...) will check for this permission too, if it's needed (for pushing new branches and lightweight tags). This problem was not discovered by the PushTagIT because tests for pushing lightweight and annotated tags were executed in a loop. For testing pushing of lightweight tags the 'Create Reference' permission was assigned and it was still there when pushing of annotated tags was tested. To fix this PushTagIT is split into 2 tests, PushAnnotatedTagIT and PushLightweightTagIT. Change-Id: I656febfb71e9fb7d1e805c1a63ca9c32405b57e6 Signed-off-by: Edwin Kempin --- .../{PushTagIT.java => AbstractPushTag.java} | 162 ++++++++---------- .../gerrit/acceptance/rest/project/BUILD | 12 ++ .../rest/project/PushAnnotatedTagIT.java | 23 +++ .../rest/project/PushLightweightTagIT.java | 23 +++ .../gerrit/server/git/ReceiveCommits.java | 9 +- 5 files changed, 133 insertions(+), 96 deletions(-) rename gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/{PushTagIT.java => AbstractPushTag.java} (51%) create mode 100644 gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/PushAnnotatedTagIT.java create mode 100644 gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/PushLightweightTagIT.java diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/PushTagIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java similarity index 51% rename from gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/PushTagIT.java rename to gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java index 691ea8ac6c..cea907d941 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/PushTagIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java @@ -19,8 +19,8 @@ import static com.google.gerrit.acceptance.GitUtil.createAnnotatedTag; import static com.google.gerrit.acceptance.GitUtil.deleteRef; import static com.google.gerrit.acceptance.GitUtil.pushHead; import static com.google.gerrit.acceptance.GitUtil.updateAnnotatedTag; -import static com.google.gerrit.acceptance.rest.project.PushTagIT.TagType.ANNOTATED; -import static com.google.gerrit.acceptance.rest.project.PushTagIT.TagType.LIGHTWEIGHT; +import static com.google.gerrit.acceptance.rest.project.AbstractPushTag.TagType.ANNOTATED; +import static com.google.gerrit.acceptance.rest.project.AbstractPushTag.TagType.LIGHTWEIGHT; import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; import com.google.common.base.MoreObjects; @@ -38,7 +38,7 @@ import org.junit.Before; import org.junit.Test; @NoHttpd -public class PushTagIT extends AbstractDaemonTest { +public abstract class AbstractPushTag extends AbstractDaemonTest { enum TagType { LIGHTWEIGHT(Permission.CREATE), ANNOTATED(Permission.CREATE_TAG); @@ -51,6 +51,7 @@ public class PushTagIT extends AbstractDaemonTest { } private RevCommit initialHead; + private TagType tagType; @Before public void setup() throws Exception { @@ -58,145 +59,131 @@ public class PushTagIT extends AbstractDaemonTest { testRepo = cloneProject(project, user); initialHead = getRemoteHead(); + tagType = getTagType(); } + protected abstract TagType getTagType(); + @Test public void createTagForExistingCommit() throws Exception { - for (TagType tagType : TagType.values()) { - pushTagForExistingCommit(tagType, Status.REJECTED_OTHER_REASON); + pushTagForExistingCommit(Status.REJECTED_OTHER_REASON); - allowTagCreation(tagType); - pushTagForExistingCommit(tagType, Status.OK); + allowTagCreation(); + pushTagForExistingCommit(Status.OK); - allowPushOnRefsTags(); - pushTagForExistingCommit(tagType, Status.OK); + allowPushOnRefsTags(); + pushTagForExistingCommit(Status.OK); - removePushFromRefsTags(); - } + removePushFromRefsTags(); } @Test public void createTagForNewCommit() throws Exception { - for (TagType tagType : TagType.values()) { - pushTagForNewCommit(tagType, Status.REJECTED_OTHER_REASON); + pushTagForNewCommit(Status.REJECTED_OTHER_REASON); - allowTagCreation(tagType); - pushTagForNewCommit(tagType, Status.REJECTED_OTHER_REASON); + allowTagCreation(); + pushTagForNewCommit(Status.REJECTED_OTHER_REASON); - allowPushOnRefsTags(); - pushTagForNewCommit(tagType, Status.OK); + allowPushOnRefsTags(); + pushTagForNewCommit(Status.OK); - removePushFromRefsTags(); - } + removePushFromRefsTags(); } @Test public void fastForward() throws Exception { - for (TagType tagType : TagType.values()) { - allowTagCreation(tagType); - String tagName = pushTagForExistingCommit(tagType, Status.OK); + allowTagCreation(); + String tagName = pushTagForExistingCommit(Status.OK); - fastForwardTagToExistingCommit(tagType, tagName, Status.REJECTED_OTHER_REASON); - fastForwardTagToNewCommit(tagType, tagName, Status.REJECTED_OTHER_REASON); + fastForwardTagToExistingCommit(tagName, Status.REJECTED_OTHER_REASON); + fastForwardTagToNewCommit(tagName, Status.REJECTED_OTHER_REASON); - allowTagDeletion(); - fastForwardTagToExistingCommit(tagType, tagName, Status.REJECTED_OTHER_REASON); - fastForwardTagToNewCommit(tagType, tagName, Status.REJECTED_OTHER_REASON); + allowTagDeletion(); + fastForwardTagToExistingCommit(tagName, Status.REJECTED_OTHER_REASON); + fastForwardTagToNewCommit(tagName, Status.REJECTED_OTHER_REASON); - allowPushOnRefsTags(); - Status expectedStatus = tagType == ANNOTATED ? Status.REJECTED_OTHER_REASON : Status.OK; - fastForwardTagToExistingCommit(tagType, tagName, expectedStatus); - fastForwardTagToNewCommit(tagType, tagName, expectedStatus); + allowPushOnRefsTags(); + Status expectedStatus = tagType == ANNOTATED ? Status.REJECTED_OTHER_REASON : Status.OK; + fastForwardTagToExistingCommit(tagName, expectedStatus); + fastForwardTagToNewCommit(tagName, expectedStatus); - allowForcePushOnRefsTags(); - fastForwardTagToExistingCommit(tagType, tagName, Status.OK); - fastForwardTagToNewCommit(tagType, tagName, Status.OK); + allowForcePushOnRefsTags(); + fastForwardTagToExistingCommit(tagName, Status.OK); + fastForwardTagToNewCommit(tagName, Status.OK); - removePushFromRefsTags(); - } + removePushFromRefsTags(); } @Test public void forceUpdate() throws Exception { - for (TagType tagType : TagType.values()) { - allowTagCreation(tagType); - String tagName = pushTagForExistingCommit(tagType, Status.OK); + allowTagCreation(); + String tagName = pushTagForExistingCommit(Status.OK); - forceUpdateTagToExistingCommit(tagType, tagName, Status.REJECTED_OTHER_REASON); - forceUpdateTagToNewCommit(tagType, tagName, Status.REJECTED_OTHER_REASON); + forceUpdateTagToExistingCommit(tagName, Status.REJECTED_OTHER_REASON); + forceUpdateTagToNewCommit(tagName, Status.REJECTED_OTHER_REASON); - allowPushOnRefsTags(); - forceUpdateTagToExistingCommit(tagType, tagName, Status.REJECTED_OTHER_REASON); - forceUpdateTagToNewCommit(tagType, tagName, Status.REJECTED_OTHER_REASON); + allowPushOnRefsTags(); + forceUpdateTagToExistingCommit(tagName, Status.REJECTED_OTHER_REASON); + forceUpdateTagToNewCommit(tagName, Status.REJECTED_OTHER_REASON); - allowTagDeletion(); - forceUpdateTagToExistingCommit(tagType, tagName, Status.REJECTED_OTHER_REASON); - forceUpdateTagToNewCommit(tagType, tagName, Status.REJECTED_OTHER_REASON); + allowTagDeletion(); + forceUpdateTagToExistingCommit(tagName, Status.REJECTED_OTHER_REASON); + forceUpdateTagToNewCommit(tagName, Status.REJECTED_OTHER_REASON); - allowForcePushOnRefsTags(); - forceUpdateTagToExistingCommit(tagType, tagName, Status.OK); - forceUpdateTagToNewCommit(tagType, tagName, Status.OK); + allowForcePushOnRefsTags(); + forceUpdateTagToExistingCommit(tagName, Status.OK); + forceUpdateTagToNewCommit(tagName, Status.OK); - removePushFromRefsTags(); - } + removePushFromRefsTags(); } @Test public void delete() throws Exception { - for (TagType tagType : TagType.values()) { - allowTagCreation(tagType); - String tagName = pushTagForExistingCommit(tagType, Status.OK); + allowTagCreation(); + String tagName = pushTagForExistingCommit(Status.OK); - pushTagDeletion(tagType, tagName, Status.REJECTED_OTHER_REASON); + pushTagDeletion(tagName, Status.REJECTED_OTHER_REASON); - allowPushOnRefsTags(); - pushTagDeletion(tagType, tagName, Status.REJECTED_OTHER_REASON); - } + allowPushOnRefsTags(); + pushTagDeletion(tagName, Status.REJECTED_OTHER_REASON); allowForcePushOnRefsTags(); - for (TagType tagType : TagType.values()) { - String tagName = pushTagForExistingCommit(tagType, Status.OK); - pushTagDeletion(tagType, tagName, Status.OK); - } + tagName = pushTagForExistingCommit(Status.OK); + pushTagDeletion(tagName, Status.OK); removePushFromRefsTags(); allowTagDeletion(); - for (TagType tagType : TagType.values()) { - String tagName = pushTagForExistingCommit(tagType, Status.OK); - pushTagDeletion(tagType, tagName, Status.OK); - } + tagName = pushTagForExistingCommit(Status.OK); + pushTagDeletion(tagName, Status.OK); } - private String pushTagForExistingCommit(TagType tagType, Status expectedStatus) throws Exception { - return pushTag(tagType, null, false, false, expectedStatus); + private String pushTagForExistingCommit(Status expectedStatus) throws Exception { + return pushTag(null, false, false, expectedStatus); } - private String pushTagForNewCommit(TagType tagType, Status expectedStatus) throws Exception { - return pushTag(tagType, null, true, false, expectedStatus); + private String pushTagForNewCommit(Status expectedStatus) throws Exception { + return pushTag(null, true, false, expectedStatus); } - private void fastForwardTagToExistingCommit( - TagType tagType, String tagName, Status expectedStatus) throws Exception { - pushTag(tagType, tagName, false, false, expectedStatus); - } - - private void fastForwardTagToNewCommit(TagType tagType, String tagName, Status expectedStatus) + private void fastForwardTagToExistingCommit(String tagName, Status expectedStatus) throws Exception { - pushTag(tagType, tagName, true, false, expectedStatus); + pushTag(tagName, false, false, expectedStatus); } - private void forceUpdateTagToExistingCommit( - TagType tagType, String tagName, Status expectedStatus) throws Exception { - pushTag(tagType, tagName, false, true, expectedStatus); + private void fastForwardTagToNewCommit(String tagName, Status expectedStatus) throws Exception { + pushTag(tagName, true, false, expectedStatus); } - private void forceUpdateTagToNewCommit(TagType tagType, String tagName, Status expectedStatus) + private void forceUpdateTagToExistingCommit(String tagName, Status expectedStatus) throws Exception { - pushTag(tagType, tagName, true, true, expectedStatus); + pushTag(tagName, false, true, expectedStatus); } - private String pushTag( - TagType tagType, String tagName, boolean newCommit, boolean force, Status expectedStatus) + private void forceUpdateTagToNewCommit(String tagName, Status expectedStatus) throws Exception { + pushTag(tagName, true, true, expectedStatus); + } + + private String pushTag(String tagName, boolean newCommit, boolean force, Status expectedStatus) throws Exception { if (force) { testRepo.reset(initialHead); @@ -234,15 +221,14 @@ public class PushTagIT extends AbstractDaemonTest { return tagName; } - private void pushTagDeletion(TagType tagType, String tagName, Status expectedStatus) - throws Exception { + private void pushTagDeletion(String tagName, Status expectedStatus) throws Exception { String tagRef = tagRef(tagName); PushResult r = deleteRef(testRepo, tagRef); RemoteRefUpdate refUpdate = r.getRemoteUpdate(tagRef); assertThat(refUpdate.getStatus()).named(tagType.name()).isEqualTo(expectedStatus); } - private void allowTagCreation(TagType tagType) throws Exception { + private void allowTagCreation() throws Exception { grant(project, "refs/tags/*", tagType.createPermission, false, REGISTERED_USERS); } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/BUILD b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/BUILD index 3266be8081..fbe5d8067b 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/BUILD +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/BUILD @@ -6,6 +6,7 @@ acceptance_tests( labels = ["rest"], deps = [ ":project", + ":push_tag_util", ":refassert", ], ) @@ -35,3 +36,14 @@ java_library( "//lib:truth", ], ) + +java_library( + name = "push_tag_util", + testonly = 1, + srcs = [ + "AbstractPushTag.java", + ], + deps = [ + "//gerrit-acceptance-tests:lib", + ], +) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/PushAnnotatedTagIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/PushAnnotatedTagIT.java new file mode 100644 index 0000000000..24c8ed0f85 --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/PushAnnotatedTagIT.java @@ -0,0 +1,23 @@ +// Copyright (C) 2017 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance.rest.project; + +public class PushAnnotatedTagIT extends AbstractPushTag { + + @Override + protected TagType getTagType() { + return TagType.ANNOTATED; + } +} diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/PushLightweightTagIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/PushLightweightTagIT.java new file mode 100644 index 0000000000..20d83a0cd2 --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/PushLightweightTagIT.java @@ -0,0 +1,23 @@ +// Copyright (C) 2017 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance.rest.project; + +public class PushLightweightTagIT extends AbstractPushTag { + + @Override + protected TagType getTagType() { + return TagType.LIGHTWEIGHT; + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index 86daf8e5d9..15a0935ded 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -1070,14 +1070,7 @@ public class ReceiveCommits { } RefControl ctl = projectControl.controlForRef(cmd.getRefName()); - boolean ok; - try { - permissions.ref(cmd.getRefName()).check(RefPermission.CREATE); - ok = true; - } catch (AuthException err) { - ok = false; - } - if (ok && ctl.canCreate(rp.getRepository(), obj)) { + if (ctl.canCreate(rp.getRepository(), obj)) { if (!validRefOperation(cmd)) { return; }