VisibleRefFilter: Check visibility of refs/users/ branches
The user branch of a user was always advertised to that user even if
READ permissions had been denied or blocked.
Doing a visibility check for the user branches means that by default
they are now no longer visible to the owning users, but the default
will be changed by a follow-up change. The next change implements a
parameter variable for ref patterns that can be expanded to the
sharded account ID. This new parameter variable will then be used to
assign the default permissions for the user branches.
Leave an exception for change edit refs since the inline edit feature
depends on the change edit refs being always visible to the owning
user.
Change-Id: If836518de4e4d6b084b675b050bb992fec5fb1e6
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
index f681fd5..5d5c827 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/AbstractDaemonTest.java
@@ -620,9 +620,14 @@
protected void deny(String permission, AccountGroup.UUID id, String ref)
throws Exception {
- ProjectConfig cfg = projectCache.checkedGet(project).getConfig();
+ deny(project, permission, id, ref);
+ }
+
+ protected void deny(Project.NameKey p, String permission,
+ AccountGroup.UUID id, String ref) throws Exception {
+ ProjectConfig cfg = projectCache.checkedGet(p).getConfig();
Util.deny(cfg, permission, id, ref);
- saveProjectConfig(project, cfg);
+ saveProjectConfig(p, cfg);
}
protected PermissionRule block(String permission, AccountGroup.UUID id, String ref)
@@ -652,14 +657,22 @@
protected void grant(String permission, Project.NameKey project, String ref,
boolean force) throws RepositoryNotFoundException, IOException,
- ConfigInvalidException {
+ ConfigInvalidException {
+ AccountGroup adminGroup =
+ groupCache.get(new AccountGroup.NameKey("Administrators"));
+ grant(permission, project, ref, force, adminGroup.getGroupUUID());
+ }
+
+ protected void grant(String permission, Project.NameKey project, String ref,
+ boolean force, AccountGroup.UUID groupUUID)
+ throws RepositoryNotFoundException, IOException,
+ ConfigInvalidException {
try (MetaDataUpdate md = metaDataUpdateFactory.create(project)) {
md.setMessage(String.format("Grant %s on %s", permission, ref));
ProjectConfig config = ProjectConfig.read(md);
AccessSection s = config.getAccessSection(ref, true);
Permission p = s.getPermission(permission, true);
- AccountGroup adminGroup = groupCache.get(new AccountGroup.NameKey("Administrators"));
- PermissionRule rule = new PermissionRule(config.resolve(adminGroup));
+ PermissionRule rule = Util.newRule(config, groupUUID);
rule.setForce(force);
p.add(rule);
config.commit(md);
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index 2d3f98a..adbc9ce 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -26,6 +26,8 @@
import static com.google.gerrit.gpg.testutil.TestKeys.validKeyWithoutExpiration;
import static com.google.gerrit.server.StarredChangesUtil.DEFAULT_LABEL;
import static com.google.gerrit.server.StarredChangesUtil.IGNORE_LABEL;
+import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.common.base.Function;
@@ -68,6 +70,7 @@
import org.bouncycastle.bcpg.ArmoredOutputStream;
import org.bouncycastle.openpgp.PGPPublicKey;
import org.bouncycastle.openpgp.PGPPublicKeyRing;
+import org.eclipse.jgit.api.errors.TransportException;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.Config;
@@ -76,6 +79,7 @@
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.transport.PushCertificateIdent;
import org.junit.After;
+import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
@@ -366,21 +370,50 @@
public void fetchUserBranch() throws Exception {
// change something in the user preferences to ensure that the user branch
// is created
+ setApiUser(user);
GeneralPreferencesInfo input = new GeneralPreferencesInfo();
input.changesPerPage =
GeneralPreferencesInfo.defaults().changesPerPage + 10;
gApi.accounts().self().setPreferences(input);
- TestRepository<InMemoryRepository> allUsersRepo = cloneProject(allUsers);
- fetch(allUsersRepo, RefNames.refsUsers(admin.id) + ":userRef");
+ TestRepository<InMemoryRepository> allUsersRepo =
+ cloneProject(allUsers, user);
+ String userRefName = RefNames.refsUsers(user.id);
+
+ // deny READ permission that is inherited from All-Projects
+ deny(allUsers, Permission.READ, ANONYMOUS_USERS, RefNames.REFS + "*");
+
+ // fetching user branch without READ permission fails
+ try {
+ fetch(allUsersRepo, userRefName + ":userRef");
+ Assert.fail(
+ "user branch is visible although no READ permission is granted");
+ } catch (TransportException e) {
+ // expected because no READ granted on user branch
+ }
+
+ // allow each user to read its own user branch
+ grant(Permission.READ, allUsers, RefNames.REFS_USERS + "*", false,
+ REGISTERED_USERS);
+
+ // fetch user branch using refs/users/YY/XXXXXXX
+ fetch(allUsersRepo, userRefName + ":userRef");
Ref userRef = allUsersRepo.getRepository().exactRef("userRef");
assertThat(userRef).isNotNull();
+ // fetch user branch using refs/users/self
fetch(allUsersRepo, RefNames.REFS_USERS_SELF + ":userSelfRef");
Ref userSelfRef =
allUsersRepo.getRepository().getRefDatabase().exactRef("userSelfRef");
assertThat(userSelfRef).isNotNull();
assertThat(userSelfRef.getObjectId()).isEqualTo(userRef.getObjectId());
+
+ // fetching user branch of another user fails
+ String otherUserRefName = RefNames.refsUsers(admin.id);
+ exception.expect(TransportException.class);
+ exception.expectMessage(
+ "Remote does not have " + otherUserRefName + " available for fetch.");
+ fetch(allUsersRepo, otherUserRefName + ":otherUserRef");
}
@Test
diff --git a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java
index cd682d9..ec4ddb2 100644
--- a/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java
+++ b/gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/RefNames.java
@@ -177,6 +177,18 @@
.toString();
}
+ public static boolean isRefsEdit(String ref) {
+ return ref.startsWith(REFS_USERS) && ref.contains(EDIT_PREFIX);
+ }
+
+ public static boolean isRefsEditOf(String ref, Account.Id accountId) {
+ String prefix = new StringBuilder(refsUsers(accountId))
+ .append('/')
+ .append(EDIT_PREFIX)
+ .toString();
+ return ref.startsWith(prefix);
+ }
+
static Integer parseShardedRefPart(String name) {
if (name == null) {
return null;
diff --git a/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java b/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java
index 946b76c..4e3b659 100644
--- a/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java
+++ b/gerrit-reviewdb/src/test/java/com/google/gerrit/reviewdb/client/RefNamesTest.java
@@ -69,6 +69,42 @@
}
@Test
+ public void isRefsEdit() throws Exception {
+ assertThat(RefNames.isRefsEdit("refs/users/23/1011123/edit-67473/42"))
+ .isTrue();
+
+ // user ref, but no edit ref
+ assertThat(RefNames.isRefsEdit("refs/users/23/1011123")).isFalse();
+
+ // other ref
+ assertThat(RefNames.isRefsEdit("refs/heads/master")).isFalse();
+ }
+
+ @Test
+ public void isRefsEditOf() throws Exception {
+ assertThat(
+ RefNames.isRefsEditOf("refs/users/23/1011123/edit-67473/42", accountId))
+ .isTrue();
+
+ // other user
+ assertThat(
+ RefNames.isRefsEditOf("refs/users/20/1078620/edit-67473/42", accountId))
+ .isFalse();
+
+ // user ref, but no edit ref
+ assertThat(RefNames.isRefsEditOf("refs/users/23/1011123", accountId))
+ .isFalse();
+
+ // bad user shard
+ assertThat(
+ RefNames.isRefsEditOf("refs/users/77/1011123/edit-67473/42", accountId))
+ .isFalse();
+
+ // other ref
+ assertThat(RefNames.isRefsEditOf("refs/heads/master", accountId)).isFalse();
+ }
+
+ @Test
public void testParseShardedRefsPart() throws Exception {
assertThat(parseShardedRefPart("01/1")).isEqualTo(1);
assertThat(parseShardedRefPart("01/1-drafts")).isEqualTo(1);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
index a53c49b..01ee6b2 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
@@ -110,18 +110,16 @@
Account.Id accountId;
if (ref.getName().startsWith(RefNames.REFS_CACHE_AUTOMERGE)) {
continue;
- } else if ((accountId =
- Account.Id.fromRef(ref.getLeaf().getName())) != null) {
- // Reference related to an account is visible only for the current
- // account.
+ } else if (showMetadata
+ && (RefNames.isRefsEditOf(ref.getLeaf().getName(), currAccountId)
+ || (RefNames.isRefsEdit(ref.getLeaf().getName())
+ && canViewMetadata))) {
+ // Change edit reference related is visible to the account that owns the
+ // change edit.
//
- // TODO(dborowitz): If a ref matches an account and a change, verify
- // both (to exclude e.g. edits on changes that the user has lost access
- // to).
- if (showMetadata
- && (canViewMetadata || accountId.equals(currAccountId))) {
- result.put(ref.getName(), ref);
- }
+ // TODO(dborowitz): Verify if change is visible (to exclude edits on
+ // changes that the user has lost access to).
+ result.put(ref.getName(), ref);
} else if ((changeId = Change.Id.fromRef(ref.getName())) != null) {
// Reference related to a change is visible if the change is visible.
@@ -143,7 +141,18 @@
// symbolic we want the control around the final target. If its
// not symbolic then getLeaf() is a no-op returning ref itself.
//
- result.put(ref.getName(), ref);
+
+ if ((accountId =
+ Account.Id.fromRef(ref.getLeaf().getName())) != null) {
+ // Reference related to an account is visible only for the current
+ // account.
+ if (showMetadata
+ && (canViewMetadata || accountId.equals(currAccountId))) {
+ result.put(ref.getName(), ref);
+ }
+ } else {
+ result.put(ref.getName(), ref);
+ }
}
}
