opendev/gerrit
Code Issues Proposed changes

Do not require 'Create Reference' for pushing annotated tag

Browse Source 
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 <ekempin@google.com>
This commit is contained in:
Edwin Kempin 2017-06-27 09:56:38 +02:00
parent e53404645c
commit 61e7176bf0
5 changed files with 133 additions and 96 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

162
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/PushTagIT.java → gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java
View File

@ -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);
}

12
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/BUILD
View File

@ -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",
],
)

23
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/PushAnnotatedTagIT.java Normal file
View File

@ -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;
}
}

23
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/PushLightweightTagIT.java Normal file
View File

@ -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;
}
}

9
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@ -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;
}