VisibleRefFilter: Hide refs for draft comments and stars of other users

When NoteDb is enabled draft comments and starred changes are stored in
special refs inside the All-Users repository. These refs should only be
visible to the owning user (and users which can view all metadata), but
at the moment anyone who has read permissions to All-Users can see all
draft comments and starred changes refs.

To filter out user branches VisibleRefFilter already filters out refs
that contain an account ID, if the account ID does not match the account
ID of the current user. Parsing an account ID from a ref is implemented
in Account.fromRef(String), but so far this method is only considering
user branches. Extend this method so that it can also parse account IDs
from draft comments and starred changes refs. In draft comments and
starred changes refs the account ID follows a sharded change ID, e.g.
refs/draft-comments/73/67473/1011123. Generic logic to parse an ID that
follows a sharded ID is added to RefNames.

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

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

@@ -762,9 +762,14 @@ public abstract class AbstractDaemonTest {
   }
 
   protected void allow(String ref, String permission, AccountGroup.UUID id) throws Exception {
-    ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
+    allow(project, ref, permission, id);
+  }
+
+  protected void allow(Project.NameKey p, String ref, String permission, AccountGroup.UUID id)
+      throws Exception {
+    ProjectConfig cfg = projectCache.checkedGet(p).getConfig();
     Util.allow(cfg, permission, id, ref);
-    saveProjectConfig(project, cfg);
+    saveProjectConfig(p, cfg);
   }
 
   protected void allowGlobalCapabilities(AccountGroup.UUID id, String... capabilityNames)

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

@@ -24,15 +24,19 @@ import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.AcceptanceTestRequestScope;
 import com.google.gerrit.acceptance.NoHttpd;
 import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.TestAccount;
 import com.google.gerrit.common.data.AccessSection;
 import com.google.gerrit.common.data.GlobalCapability;
 import com.google.gerrit.common.data.Permission;
+import com.google.gerrit.extensions.api.changes.DraftInput;
 import com.google.gerrit.extensions.api.projects.BranchInput;
 import com.google.gerrit.reviewdb.client.AccountGroup;
 import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.Patch;
 import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.config.AnonymousCowardName;
 import com.google.gerrit.server.git.ProjectConfig;
 import com.google.gerrit.server.git.ReceiveCommitsAdvertiseRefsHook;
@@ -63,6 +67,7 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
   @Inject private VisibleRefFilter.Factory refFilterFactory;
   @Inject private ChangeNoteUtil noteUtil;
   @Inject @AnonymousCowardName private String anonymousCowardName;
+  @Inject private AllUsersName allUsersName;
 
   private AccountGroup.UUID admins;
 
@@ -529,6 +534,53 @@ public class RefAdvertisementIT extends AbstractDaemonTest {
     }
   }
 
+  @Test
+  public void advertisedReferencesOmitDraftCommentRefsOfOtherUsers() throws Exception {
+    assume().that(notesMigration.commitChangeWrites()).isTrue();
+
+    allow(project, "refs/*", Permission.READ, REGISTERED_USERS);
+    allow(allUsersName, "refs/*", Permission.READ, REGISTERED_USERS);
+
+    setApiUser(user);
+    DraftInput draftInput = new DraftInput();
+    draftInput.line = 1;
+    draftInput.message = "nit: trailing whitespace";
+    draftInput.path = Patch.COMMIT_MSG;
+    gApi.changes().id(c3.getId().get()).current().createDraft(draftInput);
+    String draftCommentRef = RefNames.refsDraftComments(c3.getId(), user.id);
+
+    // user can see the draft comment ref of the own draft comment
+    assertThat(lsRemote(allUsersName, user)).contains(draftCommentRef);
+
+    // user2 can't see the draft comment ref of user's draft comment
+    assertThat(lsRemote(allUsersName, accountCreator.user2())).doesNotContain(draftCommentRef);
+  }
+
+  @Test
+  public void advertisedReferencesOmitStarredChangesRefsOfOtherUsers() throws Exception {
+    assume().that(notesMigration.commitChangeWrites()).isTrue();
+
+    allow(project, "refs/*", Permission.READ, REGISTERED_USERS);
+    allow(allUsersName, "refs/*", Permission.READ, REGISTERED_USERS);
+
+    setApiUser(user);
+    gApi.accounts().self().starChange(c3.getId().toString());
+    String starredChangesRef = RefNames.refsStarredChanges(c3.getId(), user.id);
+
+    // user can see the starred changes ref of the own star
+    assertThat(lsRemote(allUsersName, user)).contains(starredChangesRef);
+
+    // user2 can't see the starred changes ref of admin's star
+    assertThat(lsRemote(allUsersName, accountCreator.user2())).doesNotContain(starredChangesRef);
+  }
+
+  private List<String> lsRemote(Project.NameKey p, TestAccount a) throws Exception {
+    TestRepository<?> testRepository = cloneProject(p, a);
+    try (Git git = testRepository.git()) {
+      return git.lsRemote().call().stream().map(Ref::getName).collect(toList());
+    }
+  }
+
   /**
    * Assert that refs seen by a non-admin user match expected.
    *

gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/Account.java

@@ -14,6 +14,8 @@
 
 package com.google.gerrit.reviewdb.client;
 
+import static com.google.gerrit.reviewdb.client.RefNames.REFS_DRAFT_COMMENTS;
+import static com.google.gerrit.reviewdb.client.RefNames.REFS_STARRED_CHANGES;
 import static com.google.gerrit.reviewdb.client.RefNames.REFS_USERS;
 
 import com.google.gerrit.extensions.client.DiffPreferencesInfo;
@@ -105,6 +107,10 @@ public final class Account {
       }
       if (name.startsWith(REFS_USERS)) {
         return fromRefPart(name.substring(REFS_USERS.length()));
+      } else if (name.startsWith(REFS_DRAFT_COMMENTS)) {
+        return parseAfterShardedRefPart(name.substring(REFS_DRAFT_COMMENTS.length()));
+      } else if (name.startsWith(REFS_STARRED_CHANGES)) {
+        return parseAfterShardedRefPart(name.substring(REFS_STARRED_CHANGES.length()));
       }
       return null;
     }
@@ -120,6 +126,11 @@ public final class Account {
       return id != null ? new Account.Id(id) : null;
     }
 
+    public static Id parseAfterShardedRefPart(String name) {
+      Integer id = RefNames.parseAfterShardedRefPart(name);
+      return id != null ? new Account.Id(id) : null;
+    }
+
     /**
      * Parse an Account.Id out of the last part of a ref name.
      *

gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java

@@ -37,9 +37,6 @@ public class RefNames {
   /** Note tree listing external IDs */
   public static final String REFS_EXTERNAL_IDS = "refs/meta/external-ids";
 
-  /** Preference settings for a user {@code refs/users} */
-  public static final String REFS_USERS = "refs/users/";
-
   /** Magic user branch in All-Users {@code refs/users/self} */
   public static final String REFS_USERS_SELF = "refs/users/self";
 
@@ -49,12 +46,6 @@ public class RefNames {
   /** Configurations of project-specific dashboards (canned search queries). */
   public static final String REFS_DASHBOARDS = "refs/meta/dashboards/";
 
-  /** Draft inline comments of a user on a change */
-  public static final String REFS_DRAFT_COMMENTS = "refs/draft-comments/";
-
-  /** A change starred by a user */
-  public static final String REFS_STARRED_CHANGES = "refs/starred-changes/";
-
   /** Sequence counters in NoteDb. */
   public static final String REFS_SEQUENCES = "refs/sequences/";
 
@@ -76,6 +67,27 @@ public class RefNames {
 
   public static final String EDIT_PREFIX = "edit-";
 
+  /*
+   * The following refs contain an account ID and should be visible only to that account.
+   *
+   * Parsing the account ID from the ref is implemented in Account.Id#fromRef(String). This ensures
+   * that VisibleRefFilter hides those refs from other users.
+   *
+   * This applies to:
+   * - User branches (e.g. 'refs/users/23/1011123')
+   * - Draft comment refs (e.g. 'refs/draft-comments/73/67473/1011123')
+   * - Starred changes refs (e.g. 'refs/starred-changes/73/67473/1011123')
+   */
+
+  /** Preference settings for a user {@code refs/users} */
+  public static final String REFS_USERS = "refs/users/";
+
+  /** Draft inline comments of a user on a change */
+  public static final String REFS_DRAFT_COMMENTS = "refs/draft-comments/";
+
+  /** A change starred by a user */
+  public static final String REFS_STARRED_CHANGES = "refs/starred-changes/";
+
   public static String fullName(String ref) {
     return (ref.startsWith(REFS) || ref.equals(HEAD)) ? ref : REFS_HEADS + ref;
   }
@@ -249,6 +261,88 @@ public class RefNames {
     return id;
   }
 
+  /**
+   * Skips a sharded ref part at the beginning of the name.
+   *
+   * <p>E.g.: "01/1" -> "", "01/1/" -> "/", "01/1/2" -> "/2", "01/1-edit" -> "-edit"
+   *
+   * @param name ref part name
+   * @return the rest of the name, {@code null} if the ref name part doesn't start with a valid
+   *     sharded ID
+   */
+  static String skipShardedRefPart(String name) {
+    if (name == null) {
+      return null;
+    }
+
+    String[] parts = name.split("/");
+    int n = parts.length;
+    if (n < 2) {
+      return null;
+    }
+
+    // Last 2 digits.
+    int le;
+    for (le = 0; le < parts[0].length(); le++) {
+      if (!Character.isDigit(parts[0].charAt(le))) {
+        return null;
+      }
+    }
+    if (le != 2) {
+      return null;
+    }
+
+    // Full ID.
+    int ie;
+    for (ie = 0; ie < parts[1].length(); ie++) {
+      if (!Character.isDigit(parts[1].charAt(ie))) {
+        if (ie == 0) {
+          return null;
+        }
+        break;
+      }
+    }
+
+    int shard = Integer.parseInt(parts[0]);
+    int id = Integer.parseInt(parts[1].substring(0, ie));
+
+    if (id % 100 != shard) {
+      return null;
+    }
+
+    return name.substring(2 + 1 + ie); // 2 for the length of the shard, 1 for the '/'
+  }
+
+  /**
+   * Parses an ID that follows a sharded ref part at the beginning of the name.
+   *
+   * <p>E.g.: "01/1/2" -> 2, "01/1/2/4" -> 2, ""01/1/2-edit" -> 2
+   *
+   * @param name ref part name
+   * @return ID that follows the sharded ref part at the beginning of the name, {@code null} if the
+   *     ref name part doesn't start with a valid sharded ID or if no valid ID follows the sharded
+   *     ref part
+   */
+  static Integer parseAfterShardedRefPart(String name) {
+    String rest = skipShardedRefPart(name);
+    if (rest == null || !rest.startsWith("/")) {
+      return null;
+    }
+
+    rest = rest.substring(1);
+
+    int ie;
+    for (ie = 0; ie < rest.length(); ie++) {
+      if (!Character.isDigit(rest.charAt(ie))) {
+        break;
+      }
+    }
+    if (ie == 0) {
+      return null;
+    }
+    return Integer.parseInt(rest.substring(0, ie));
+  }
+
   static Integer parseRefSuffix(String name) {
     if (name == null) {
       return null;

gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/AccountTest.java

@@ -43,6 +43,42 @@ public class AccountTest {
     assertThat(fromRef("refs/users/1/1")).isNull();
   }
 
+  @Test
+  public void parseDraftCommentsRefName() {
+    assertThat(fromRef("refs/draft-comments/35/135/1")).isEqualTo(id(1));
+    assertThat(fromRef("refs/draft-comments/35/135/1-foo/2")).isEqualTo(id(1));
+    assertThat(fromRef("refs/draft-comments/35/135/1/foo/2")).isEqualTo(id(1));
+
+    // Invalid characters.
+    assertThat(fromRef("refs/draft-comments/35a/135/1")).isNull();
+    assertThat(fromRef("refs/draft-comments/35/135a/1")).isNull();
+    assertThat(fromRef("refs/draft-comments/35/135/a1")).isNull();
+
+    // Mismatched shard.
+    assertThat(fromRef("refs/draft-comments/02/135/1")).isNull();
+
+    // Shard too short.
+    assertThat(fromRef("refs/draft-comments/2/2/1")).isNull();
+  }
+
+  @Test
+  public void parseStarredChangesRefName() {
+    assertThat(fromRef("refs/starred-changes/35/135/1")).isEqualTo(id(1));
+    assertThat(fromRef("refs/starred-changes/35/135/1-foo/2")).isEqualTo(id(1));
+    assertThat(fromRef("refs/starred-changes/35/135/1/foo/2")).isEqualTo(id(1));
+
+    // Invalid characters.
+    assertThat(fromRef("refs/starred-changes/35a/135/1")).isNull();
+    assertThat(fromRef("refs/starred-changes/35/135a/1")).isNull();
+    assertThat(fromRef("refs/starred-changes/35/135/a1")).isNull();
+
+    // Mismatched shard.
+    assertThat(fromRef("refs/starred-changes/02/135/1")).isNull();
+
+    // Shard too short.
+    assertThat(fromRef("refs/starred-changes/2/2/1")).isNull();
+  }
+
   @Test
   public void parseRefNameParts() {
     assertThat(fromRefPart("01/1")).isEqualTo(id(1));

gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java

@@ -15,8 +15,10 @@
 package com.google.gerrit.reviewdb.client;
 
 import static com.google.common.truth.Truth.assertThat;
+import static com.google.gerrit.reviewdb.client.RefNames.parseAfterShardedRefPart;
 import static com.google.gerrit.reviewdb.client.RefNames.parseRefSuffix;
 import static com.google.gerrit.reviewdb.client.RefNames.parseShardedRefPart;
+import static com.google.gerrit.reviewdb.client.RefNames.skipShardedRefPart;
 
 import org.junit.Test;
 
@@ -90,7 +92,7 @@ public class RefNamesTest {
   }
 
   @Test
-  public void testparseShardedRefsPart() throws Exception {
+  public void parseShardedRefsPart() throws Exception {
     assertThat(parseShardedRefPart("01/1")).isEqualTo(1);
     assertThat(parseShardedRefPart("01/1-drafts")).isEqualTo(1);
     assertThat(parseShardedRefPart("01/1-drafts/2")).isEqualTo(1);
@@ -112,6 +114,58 @@ public class RefNamesTest {
     assertThat(parseShardedRefPart("1/1")).isNull();
   }
 
+  @Test
+  public void skipShardedRefsPart() throws Exception {
+    assertThat(skipShardedRefPart("01/1")).isEqualTo("");
+    assertThat(skipShardedRefPart("01/1/")).isEqualTo("/");
+    assertThat(skipShardedRefPart("01/1/2")).isEqualTo("/2");
+    assertThat(skipShardedRefPart("01/1-edit")).isEqualTo("-edit");
+
+    assertThat(skipShardedRefPart(null)).isNull();
+    assertThat(skipShardedRefPart("")).isNull();
+
+    // Prefix not stripped.
+    assertThat(skipShardedRefPart("refs/draft-comments/01/1/2")).isNull();
+
+    // Invalid characters.
+    assertThat(skipShardedRefPart("01a/1/2")).isNull();
+    assertThat(skipShardedRefPart("01a/a1/2")).isNull();
+
+    // Mismatched shard.
+    assertThat(skipShardedRefPart("01/23/2")).isNull();
+
+    // Shard too short.
+    assertThat(skipShardedRefPart("1/1")).isNull();
+  }
+
+  @Test
+  public void parseAfterShardedRefsPart() throws Exception {
+    assertThat(parseAfterShardedRefPart("01/1/2")).isEqualTo(2);
+    assertThat(parseAfterShardedRefPart("01/1/2/4")).isEqualTo(2);
+    assertThat(parseAfterShardedRefPart("01/1/2-edit")).isEqualTo(2);
+
+    assertThat(parseAfterShardedRefPart(null)).isNull();
+    assertThat(parseAfterShardedRefPart("")).isNull();
+
+    // No ID after sharded ref part
+    assertThat(parseAfterShardedRefPart("01/1")).isNull();
+    assertThat(parseAfterShardedRefPart("01/1/")).isNull();
+    assertThat(parseAfterShardedRefPart("01/1/a")).isNull();
+
+    // Prefix not stripped.
+    assertThat(parseAfterShardedRefPart("refs/draft-comments/01/1/2")).isNull();
+
+    // Invalid characters.
+    assertThat(parseAfterShardedRefPart("01a/1/2")).isNull();
+    assertThat(parseAfterShardedRefPart("01a/a1/2")).isNull();
+
+    // Mismatched shard.
+    assertThat(parseAfterShardedRefPart("01/23/2")).isNull();
+
+    // Shard too short.
+    assertThat(parseAfterShardedRefPart("1/1")).isNull();
+  }
+
   @Test
   public void testParseRefSuffix() throws Exception {
     assertThat(parseRefSuffix("1/2/34")).isEqualTo(34);