opendev/gerrit
Code Issues Proposed changes

VisibleRefFilter: Check visibility of refs/users/ branches

Browse Source 
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>
This commit is contained in:
Edwin Kempin
2016-05-04 18:13:33 +02:00
committed by David Pursehouse
parent 88fcd6bcfa
commit adb1ed4c77
5 changed files with 122 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -620,9 +620,14 @@ public abstract class AbstractDaemonTest {
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 @@ public abstract class AbstractDaemonTest {
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);

37
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/accounts/AccountIT.java
View File

@@ -26,6 +26,8 @@ import static com.google.gerrit.gpg.testutil.TestKeys.validKeyWithSecondUserId;
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 com.google.inject.Provider;
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.RefUpdate;
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 class AccountIT extends AbstractDaemonTest {
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

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

@@ -177,6 +177,18 @@ public class RefNames {
.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;

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

@@ -68,6 +68,42 @@ public class RefNamesTest {
.isEqualTo("refs/users/23/1011123/edit-67473/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);

33
gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
View File

@@ -110,18 +110,16 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook {
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 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook {
// 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);
}
}
}