Make permissions for tag creation consistent

For each tag type you need a special permission for the tag creation:
- Lightweight tags require 'Create Reference'
- Annontated tags require 'Push Annotated Tags'
- Signed tags require 'Push Signed Tags'

When creating a tag by push there are 2 cases:
1. The commit to which the tag points already exists in Gerrit
   (it is reachable from any branch/tag that is readable by the
   calling user)
2. The commit to which the tag points is new
   (it is not reachable from any branch/tag that is readable by the
   calling user)

So far the permissions that were required to push a tag on a new
commit were inconsistent:
- For lightweight tags we required 'Push' in addition to 'Create
  Reference'
- For annotated/signed tags 'Push Annotated Tags'/'Push Signed Tags'
  were sufficient.

Due to this it was not possible to allow pushing of annotated/signed
tags for existing commits, but not for new commits.

Change the behaviour for annotated/signed tags so that it's consistent
with the behaviour for lightweight tags and require 'Push' in addition
to 'Push Annotated Tags'/'Push Signed Tags', if the tag points to a
new commit.

We may consider renaming 'Push Annotated Tags'/'Push Signed Tags' to
'Create Annotated Tags'/'Create Signed Tags' later.

Add tests for the tag creation by push that cover lightweight and
annotated tags on existing and new commits. Tests for signed tags may
be added later.

Change-Id: I1094a2be4871e16239b6a6daefc537ffc77af3bf
Signed-off-by: Edwin Kempin <ekempin@google.com>

Documentation/access-control.txt

@@ -466,7 +466,9 @@ as well as bypass review for new commits on that branch.
 
 To push lightweight (non-annotated) tags, grant
 `Create Reference` for reference name `+refs/tags/*+`, as lightweight
-tags are implemented just like branches in Git.
+tags are implemented just like branches in Git. To push a lightweight
+tag on a new commit (commit not reachable from any branch/tag) grant
+`Push` permission on `+refs/tags/*+` too.
 
 For example, to grant the possibility to create new branches under the
 namespace `foo`, you have to grant this permission on
@@ -682,6 +684,9 @@ To delete or overwrite an existing tag, grant `Push` with the force
 option enabled for reference name `+refs/tags/*+`, as deleting a tag
 requires the same permission as deleting a branch.
 
+To push an annotated tag on a new commit (commit not reachable from any
+branch/tag) grant `Push` permission on `+refs/tags/*+` too.
+
 
 [[category_push_signed]]
 === Push Signed Tag

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java

@@ -807,6 +807,19 @@ public abstract class AbstractDaemonTest {
     }
   }
 
+  protected void removePermission(String permission, Project.NameKey project,
+      String ref) throws IOException, ConfigInvalidException {
+    try (MetaDataUpdate md = metaDataUpdateFactory.create(project)) {
+      md.setMessage(String.format("Remove %s on %s", permission, ref));
+      ProjectConfig config = ProjectConfig.read(md);
+      AccessSection s = config.getAccessSection(ref, true);
+      Permission p = s.getPermission(permission, true);
+      p.getRules().clear();
+      config.commit(md);
+      projectCache.evict(config.getProject());
+    }
+  }
+
   protected void blockRead(String ref) throws Exception {
     block(Permission.READ, REGISTERED_USERS, ref);
   }

gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/GitUtil.java

@@ -28,12 +28,15 @@ import com.jcraft.jsch.Session;
 
 import org.eclipse.jgit.api.FetchCommand;
 import org.eclipse.jgit.api.PushCommand;
+import org.eclipse.jgit.api.TagCommand;
 import org.eclipse.jgit.api.errors.GitAPIException;
 import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
 import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.transport.FetchResult;
@@ -137,6 +140,16 @@ public class GitUtil {
     return cloneProject(project, sshSession.getUrl() + "/" + project.get());
   }
 
+  public static Ref createAnnotatedTag(TestRepository<?> testRepo, String name,
+      PersonIdent tagger) throws GitAPIException {
+    TagCommand cmd = testRepo.git().tag()
+        .setName(name)
+        .setAnnotated(true)
+        .setMessage(name)
+        .setTagger(tagger);
+    return cmd.call();
+  }
+
   public static void fetch(TestRepository<?> testRepo, String spec)
       throws GitAPIException {
     FetchCommand fetch = testRepo.git().fetch();
@@ -144,6 +157,11 @@ public class GitUtil {
     fetch.call();
   }
 
+  public static PushResult pushHead(TestRepository<?> testRepo, String ref)
+      throws GitAPIException {
+    return pushHead(testRepo, ref, false);
+  }
+
   public static PushResult pushHead(TestRepository<?> testRepo, String ref,
       boolean pushTags) throws GitAPIException {
     return pushHead(testRepo, ref, pushTags, false);
@@ -182,6 +200,20 @@ public class GitUtil {
     assertThat(rru.getMessage()).isEqualTo(expectedMessage);
   }
 
+  public static PushResult pushTag(TestRepository<?> testRepo, String tag)
+      throws GitAPIException {
+    return pushTag(testRepo, tag, false);
+  }
+
+  private static PushResult pushTag(TestRepository<?> testRepo, String tag,
+      boolean force) throws GitAPIException {
+    PushCommand pushCmd = testRepo.git().push();
+    pushCmd.setForce(force);
+    pushCmd.setRefSpecs(new RefSpec("refs/tags/" + tag + ":refs/tags/" + tag));
+    Iterable<PushResult> r = pushCmd.call();
+    return Iterables.getOnlyElement(r);
+  }
+
   public static Optional<String> getChangeId(TestRepository<?> tr, ObjectId id)
       throws IOException {
     RevCommit c = tr.getRevWalk().parseCommit(id);

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/SubmitOnPushIT.java

@@ -77,6 +77,7 @@ public class SubmitOnPushIT extends AbstractDaemonTest {
   @Test
   public void submitOnPushWithAnnotatedTag() throws Exception {
     grant(Permission.SUBMIT, project, "refs/for/refs/heads/master");
+    grant(Permission.PUSH, project, "refs/tags/*");
     PushOneCommit.AnnotatedTag tag =
         new PushOneCommit.AnnotatedTag("v1.0", "annotation", admin.getIdent());
     PushOneCommit push = pushFactory.create(db, admin.getIdent(), testRepo);

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/PushTagIT.java

@@ -0,0 +1,143 @@
+// Copyright (C) 2016 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;
+
+import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.acceptance.GitUtil.createAnnotatedTag;
+import static com.google.gerrit.acceptance.GitUtil.pushHead;
+import static com.google.gerrit.acceptance.rest.project.PushTagIT.TagType.LIGHTWEIGHT;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.GitUtil;
+import com.google.gerrit.acceptance.NoHttpd;
+import com.google.gerrit.common.data.Permission;
+
+import org.eclipse.jgit.lib.PersonIdent;
+import org.eclipse.jgit.transport.PushResult;
+import org.eclipse.jgit.transport.RemoteRefUpdate;
+import org.eclipse.jgit.transport.RemoteRefUpdate.Status;
+import org.junit.Before;
+import org.junit.Test;
+
+@NoHttpd
+public class PushTagIT extends AbstractDaemonTest {
+  enum TagType {
+    LIGHTWEIGHT(Permission.CREATE),
+    ANNOTATED(Permission.PUSH_TAG);
+
+    final String createPermission;
+
+    TagType(String createPermission) {
+      this.createPermission = createPermission;
+    }
+  }
+
+  @Before
+  public void setup() throws Exception {
+    // clone with user to avoid inherited tag permissions of admin user
+    testRepo = cloneProject(project, user);
+  }
+
+  @Test
+  public void createTagForExistingCommit() throws Exception {
+    for (TagType tagType : TagType.values()) {
+      pushTagForExistingCommit(tagType, Status.REJECTED_OTHER_REASON);
+
+      allowTagCreation(tagType);
+      pushTagForExistingCommit(tagType, Status.OK);
+
+      allowPushOnRefsTags();
+      pushTagForExistingCommit(tagType, Status.OK);
+
+      removePushFromRefsTags();
+    }
+  }
+
+  @Test
+  public void createTagForNewCommit() throws Exception {
+    for (TagType tagType : TagType.values()) {
+      pushTagForNewCommit(tagType, Status.REJECTED_OTHER_REASON);
+
+      allowTagCreation(tagType);
+      pushTagForNewCommit(tagType, Status.REJECTED_OTHER_REASON);
+
+      allowPushOnRefsTags();
+      pushTagForNewCommit(tagType, Status.OK);
+
+      removePushFromRefsTags();
+    }
+  }
+
+  private void pushTagForExistingCommit(TagType tagType,
+      Status expectedStatus) throws Exception {
+    pushTag(tagType, false, expectedStatus);
+  }
+
+  private void pushTagForNewCommit(TagType tagType,
+      Status expectedStatus) throws Exception {
+    pushTag(tagType, true, expectedStatus);
+  }
+
+  private void pushTag(TagType tagType, boolean newCommit,
+      Status expectedStatus) throws Exception {
+    commit(user.getIdent(), "subject");
+
+    String tagName = "v1" + "_" + System.nanoTime();
+    switch (tagType) {
+      case LIGHTWEIGHT:
+        break;
+      case ANNOTATED:
+        createAnnotatedTag(testRepo, tagName, user.getIdent());
+        break;
+      default:
+        throw new IllegalStateException("unexpected tag type: " + tagType);
+    }
+
+    if (!newCommit) {
+      grant(Permission.SUBMIT, project, "refs/for/refs/heads/master", false,
+          REGISTERED_USERS);
+      pushHead(testRepo, "refs/for/master%submit");
+    }
+
+    PushResult r = tagType == LIGHTWEIGHT
+        ? pushHead(testRepo, "refs/tags/" + tagName)
+        : GitUtil.pushTag(testRepo, tagName);
+    RemoteRefUpdate refUpdate = r.getRemoteUpdate("refs/tags/" + tagName);
+    assertThat(refUpdate.getStatus())
+        .named(tagType.name())
+        .isEqualTo(expectedStatus);
+  }
+
+  private void allowTagCreation(TagType tagType) throws Exception {
+    grant(tagType.createPermission, project, "refs/tags/*", false,
+        REGISTERED_USERS);
+  }
+
+  private void allowPushOnRefsTags() throws Exception {
+    grant(Permission.PUSH, project, "refs/tags/*", false, REGISTERED_USERS);
+  }
+
+  private void removePushFromRefsTags() throws Exception {
+    removePermission(Permission.PUSH, project, "refs/tags/*");
+  }
+
+  private void commit(PersonIdent ident, String subject) throws Exception {
+    commitBuilder()
+        .ident(ident)
+        .message(subject)
+        .create();
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java

@@ -276,18 +276,8 @@ public class RefControl {
       } else if (!canPerform(Permission.CREATE)) {
         // No create permissions.
         return false;
-      } else if (canUpdate()) {
-        // If the user has push permissions, they can create the ref regardless
-        // of whether they are pushing any new objects along with the create.
-        return true;
-      } else if (isMergedIntoBranchOrTag(db, repo, (RevCommit) object)) {
-        // If the user has no push permissions, check whether the object is
-        // merged into a branch or tag readable by this user. If so, they are
-        // not effectively "pushing" more objects, so they can create the ref
-        // even if they don't have push permission.
-        return true;
       }
-      return false;
+      return canCreateCommit(db, repo, (RevCommit) object, admin, owner);
     } else if (object instanceof RevTag) {
       final RevTag tag = (RevTag) object;
       try (RevWalk rw = new RevWalk(repo)) {
@@ -312,6 +302,17 @@ public class RefControl {
         }
       }
 
+      RevObject tagObject = tag.getObject();
+      if (tagObject instanceof RevCommit) {
+        if (!canCreateCommit(db, repo, (RevCommit) tagObject, admin, owner)) {
+          return false;
+        }
+      } else {
+        if (!canCreate(db, repo, tagObject)) {
+          return false;
+        }
+      }
+
       // If the tag has a PGP signature, allow a lower level of permission
       // than if it doesn't have a PGP signature.
       //
@@ -324,6 +325,25 @@ public class RefControl {
     }
   }
 
+  private boolean canCreateCommit(ReviewDb db, Repository repo,
+      RevCommit commit, boolean admin, boolean owner) {
+    if (admin || (owner && !isBlocked(Permission.CREATE))) {
+      // Admin or project owner; bypass visibility check.
+      return true;
+    } else if (canUpdate()) {
+      // If the user has push permissions, they can create the ref regardless
+      // of whether they are pushing any new objects along with the create.
+      return true;
+    } else if (isMergedIntoBranchOrTag(db, repo, commit)) {
+      // If the user has no push permissions, check whether the object is
+      // merged into a branch or tag readable by this user. If so, they are
+      // not effectively "pushing" more objects, so they can create the ref
+      // even if they don't have push permission.
+      return true;
+    }
+    return false;
+  }
+
   private boolean isMergedIntoBranchOrTag(ReviewDb db, Repository repo,
       RevCommit commit) {
     try (RevWalk rw = new RevWalk(repo)) {