summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorPatrick Hiesel <hiesel@google.com>2020-11-02 14:30:54 +0100
committerPatrick Hiesel <hiesel@google.com>2020-11-11 11:20:43 +0100
commit5529e33e2dec5d1f5943d9b99cc530970af112ff (patch)
treea3188ab3aba45cd79e9562ca86d6ba3fbd248c16
parent77e925d048d3cf0b9c3d6d45924905e9cf2f7381 (diff)
Make PermissionBackend#ForRef authoritative
This change fixes a misconception that leads to data being accessible through Gerrit APIs that should be locked down. Gerrit had two components for determining if a Git ref is visible to a user: (Default)RefFilter and PermissionBackend#ForRef (ex RefControl). The former was always capable of providing correct results for all refs. The latter only had logic to decide if a Git ref is visible according to the Gerrit READ permissions. This includes all refs under refs/heads as well as any other ref that isn't a database ref or a Git tag. This component was unware of Git tags and database references. Hence, when asked for a database reference such as refs/changes/xx/yyyyxx/meta the logic would allow access if the user has READ permissions on any of the ref prefixes, such as the default "read refs/* Anonymous Users". That is problematic, because it bypasses documented behavior [1] where a user should only have access to a change if they can see the destination ref. The same goes for other database references. This change fixes the problem. It is intentionally kept to a minimally invasive code change so that it's easier to backport it. Add tests to assert the correct behavior. These tests would fail before this fix. We have included them in this change to be able to backport just a single commit. [1] https://gerrit-review.googlesource.com/Documentation/access-control.html Change-Id: Ice3a756cf573dd9b38e3f198ccc44899ccf65f75
-rw-r--r--java/com/google/gerrit/reviewdb/client/RefNames.java16
-rw-r--r--java/com/google/gerrit/server/permissions/ChangeControl.java41
-rw-r--r--java/com/google/gerrit/server/permissions/DefaultRefFilter.java145
-rw-r--r--java/com/google/gerrit/server/permissions/ProjectControl.java11
-rw-r--r--java/com/google/gerrit/server/permissions/RefControl.java84
-rw-r--r--java/com/google/gerrit/server/permissions/RefVisibilityControl.java180
-rw-r--r--javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java1
-rw-r--r--javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java6
-rw-r--r--javatests/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java59
-rw-r--r--javatests/com/google/gerrit/acceptance/rest/project/GetBranchIT.java470
-rw-r--r--javatests/com/google/gerrit/server/permissions/RefControlTest.java13
11 files changed, 890 insertions, 136 deletions
diff --git a/java/com/google/gerrit/reviewdb/client/RefNames.java b/java/com/google/gerrit/reviewdb/client/RefNames.java
index 1f119218c8..984e43e925 100644
--- a/java/com/google/gerrit/reviewdb/client/RefNames.java
+++ b/java/com/google/gerrit/reviewdb/client/RefNames.java
@@ -123,6 +123,11 @@ public class RefNames {
return shard(id.get(), r).append(META_SUFFIX).toString();
}
+ public static String patchSetRef(PatchSet.Id id) {
+ StringBuilder r = newStringBuilder().append(REFS_CHANGES);
+ return shard(id.changeId().get(), r).append('/').append(id.get()).toString();
+ }
+
public static String robotCommentsRef(Change.Id id) {
StringBuilder r = newStringBuilder().append(REFS_CHANGES);
return shard(id.get(), r).append(ROBOT_COMMENTS_SUFFIX).toString();
@@ -278,10 +283,16 @@ public class RefNames {
* Whether the ref is managed by Gerrit. Covers all Gerrit-internal refs like refs/cache-automerge
* and refs/meta as well as refs/changes. Does not cover user-created refs like branches or custom
* ref namespaces like refs/my-company.
+ *
+ * <p>Any ref for which this method evaluates to true will be served to users who have the {@code
+ * ACCESS_DATABASE} capability.
+ *
+ * <p><b>Caution</b>Any ref not in this list will be served if the user was granted a READ
+ * permission on it using Gerrit's permission model.
*/
public static boolean isGerritRef(String ref) {
return ref.startsWith(REFS_CHANGES)
- || ref.startsWith(REFS_META)
+ || ref.startsWith(REFS_EXTERNAL_IDS)
|| ref.startsWith(REFS_CACHE_AUTOMERGE)
|| ref.startsWith(REFS_DRAFT_COMMENTS)
|| ref.startsWith(REFS_DELETED_GROUPS)
@@ -289,7 +300,8 @@ public class RefNames {
|| ref.startsWith(REFS_GROUPS)
|| ref.startsWith(REFS_GROUPNAMES)
|| ref.startsWith(REFS_USERS)
- || ref.startsWith(REFS_STARRED_CHANGES);
+ || ref.startsWith(REFS_STARRED_CHANGES)
+ || ref.startsWith(REFS_REJECT_COMMITS);
}
static Integer parseShardedRefPart(String name) {
diff --git a/java/com/google/gerrit/server/permissions/ChangeControl.java b/java/com/google/gerrit/server/permissions/ChangeControl.java
index ee36200283..c3be7f2609 100644
--- a/java/com/google/gerrit/server/permissions/ChangeControl.java
+++ b/java/com/google/gerrit/server/permissions/ChangeControl.java
@@ -65,6 +65,8 @@ class ChangeControl {
private final RefControl refControl;
private final ChangeNotes notes;
+ private ChangeData cd;
+
private ChangeControl(
ChangeData.Factory changeDataFactory, RefControl refControl, ChangeNotes notes) {
this.changeDataFactory = changeDataFactory;
@@ -73,7 +75,8 @@ class ChangeControl {
}
ForChange asForChange(@Nullable ChangeData cd) {
- return new ForChangeImpl(cd);
+ setChangeData(cd);
+ return new ForChangeImpl();
}
private CurrentUser getUser() {
@@ -88,12 +91,27 @@ class ChangeControl {
return notes.getChange();
}
+ private ChangeData changeData() {
+ if (cd == null) {
+ cd = changeDataFactory.create(notes);
+ }
+ return cd;
+ }
+
+ ChangeControl setChangeData(@Nullable ChangeData cd) {
+ if (cd != null) {
+ this.cd = cd;
+ }
+ return this;
+ }
+
/** Can this user see this change? */
- private boolean isVisible(@Nullable ChangeData cd) {
- if (getChange().isPrivate() && !isPrivateVisible(cd)) {
+ boolean isVisible() {
+ if (getChange().isPrivate() && !isPrivateVisible(changeData())) {
return false;
}
- return refControl.isVisible();
+ // Does the user have READ permission on the destination?
+ return refControl.asForRef().testOrFalse(RefPermission.READ);
}
/** Can this user abandon this change? */
@@ -219,20 +237,11 @@ class ChangeControl {
}
private class ForChangeImpl extends ForChange {
- private ChangeData cd;
+
private Map<String, PermissionRange> labels;
private String resourcePath;
- ForChangeImpl(@Nullable ChangeData cd) {
- this.cd = cd;
- }
-
- private ChangeData changeData() {
- if (cd == null) {
- cd = changeDataFactory.create(notes);
- }
- return cd;
- }
+ private ForChangeImpl() {}
@Override
public String resourcePath() {
@@ -285,7 +294,7 @@ class ChangeControl {
try {
switch (perm) {
case READ:
- return isVisible(changeData());
+ return isVisible();
case ABANDON:
return canAbandon();
case DELETE:
diff --git a/java/com/google/gerrit/server/permissions/DefaultRefFilter.java b/java/com/google/gerrit/server/permissions/DefaultRefFilter.java
index 7fa712d5d4..2d1f6fc5fe 100644
--- a/java/com/google/gerrit/server/permissions/DefaultRefFilter.java
+++ b/java/com/google/gerrit/server/permissions/DefaultRefFilter.java
@@ -16,10 +16,8 @@ package com.google.gerrit.server.permissions;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.collect.ImmutableList.toImmutableList;
-import static com.google.gerrit.reviewdb.client.RefNames.REFS_CACHE_AUTOMERGE;
import static com.google.gerrit.reviewdb.client.RefNames.REFS_CONFIG;
import static com.google.gerrit.reviewdb.client.RefNames.REFS_USERS_SELF;
-import static java.util.Objects.requireNonNull;
import static java.util.stream.Collectors.toMap;
import com.google.auto.value.AutoValue;
@@ -35,20 +33,15 @@ import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.metrics.Counter0;
import com.google.gerrit.metrics.Description;
import com.google.gerrit.metrics.MetricMaker;
-import com.google.gerrit.reviewdb.client.Account;
-import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.account.GroupCache;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.git.SearchingChangeCacheImpl;
import com.google.gerrit.server.git.TagCache;
import com.google.gerrit.server.git.TagMatcher;
-import com.google.gerrit.server.group.InternalGroup;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeNotes.Factory.ChangeNotesResult;
import com.google.gerrit.server.permissions.PermissionBackend.RefFilterOptions;
@@ -78,8 +71,8 @@ class DefaultRefFilter {
private final TagCache tagCache;
private final ChangeNotes.Factory changeNotesFactory;
@Nullable private final SearchingChangeCacheImpl changeCache;
- private final GroupCache groupCache;
private final PermissionBackend permissionBackend;
+ private final RefVisibilityControl refVisibilityControl;
private final ProjectControl projectControl;
private final CurrentUser user;
private final ProjectState projectState;
@@ -95,16 +88,16 @@ class DefaultRefFilter {
TagCache tagCache,
ChangeNotes.Factory changeNotesFactory,
@Nullable SearchingChangeCacheImpl changeCache,
- GroupCache groupCache,
PermissionBackend permissionBackend,
+ RefVisibilityControl refVisibilityControl,
@GerritServerConfig Config config,
MetricMaker metricMaker,
@Assisted ProjectControl projectControl) {
this.tagCache = tagCache;
this.changeNotesFactory = changeNotesFactory;
this.changeCache = changeCache;
- this.groupCache = groupCache;
this.permissionBackend = permissionBackend;
+ this.refVisibilityControl = refVisibilityControl;
this.skipFullRefEvaluationIfAllRefsAreVisible =
config.getBoolean("auth", "skipFullRefEvaluationIfAllRefsAreVisible", true);
this.projectControl = projectControl;
@@ -194,108 +187,64 @@ class DefaultRefFilter {
}
fullFilterCount.increment();
- boolean viewMetadata;
- boolean isAdmin;
- Account.Id userId;
- IdentifiedUser identifiedUser;
- PermissionBackend.WithUser withUser = permissionBackend.user(user);
- if (user.isIdentifiedUser()) {
- viewMetadata = withUser.testOrFalse(GlobalPermission.ACCESS_DATABASE);
- isAdmin = withUser.testOrFalse(GlobalPermission.ADMINISTRATE_SERVER);
- identifiedUser = user.asIdentifiedUser();
- userId = identifiedUser.getAccountId();
- } else {
- viewMetadata = false;
- isAdmin = false;
- userId = null;
- identifiedUser = null;
- }
-
- Map<String, Ref> result = new HashMap<>();
+ boolean hasAccessDatabase =
+ permissionBackend
+ .user(projectControl.getUser())
+ .testOrFalse(GlobalPermission.ACCESS_DATABASE);
+ Map<String, Ref> resultRefs = Maps.newHashMapWithExpectedSize(refs.size());
List<Ref> deferredTags = new ArrayList<>();
for (Ref ref : refs.values()) {
- String name = ref.getName();
+ String refName = ref.getName();
Change.Id changeId;
- Account.Id accountId;
- AccountGroup.UUID accountGroupUuid;
- if (name.startsWith(REFS_CACHE_AUTOMERGE) || (opts.filterMeta() && isMetadata(name))) {
- continue;
- } else if (RefNames.isRefsEdit(name)) {
- // Edits are visible only to the owning user, if change is visible.
- if (viewMetadata || visibleEdit(repo, name)) {
- result.put(name, ref);
- }
- } else if ((changeId = Change.Id.fromRef(name)) != null) {
- // Change ref is visible only if the change is visible.
- if (viewMetadata || visible(repo, changeId)) {
- result.put(name, ref);
- }
- } else if ((accountId = Account.Id.fromRef(name)) != null) {
- // Account ref is visible only to the corresponding account.
- if (viewMetadata || (accountId.equals(userId) && canReadRef(name))) {
- result.put(name, ref);
- }
- } else if ((accountGroupUuid = AccountGroup.UUID.fromRef(name)) != null) {
- // Group ref is visible only to the corresponding owner group.
- InternalGroup group = groupCache.get(accountGroupUuid).orElse(null);
- if (viewMetadata
- || (group != null
- && isGroupOwner(group, identifiedUser, isAdmin)
- && canReadRef(name))) {
- result.put(name, ref);
- }
+ if (opts.filterMeta() && isMetadata(refName)) {
+ logger.atFinest().log("Filter out metadata ref %s", refName);
} else if (isTag(ref)) {
if (hasReadOnRefsStar) {
- // The user has READ on refs/*. This is the broadest permission one can assign. There is
- // no way to grant access to (specific) tags in Gerrit, so we have to assume that these
- // users can see all tags because there could be tags that aren't reachable by any visible
- // ref while the user can see all non-Gerrit refs. This matches Gerrit's historic
- // behavior.
+ // The user has READ on refs/* with no effective block permission. This is the broadest
+ // permission one can assign. There is no way to grant access to (specific) tags in
+ // Gerrit,
+ // so we have to assume that these users can see all tags because there could be tags that
+ // aren't reachable by any visible ref while the user can see all non-Gerrit refs. This
+ // matches Gerrit's historic behavior.
// This makes it so that these users could see commits that they can't see otherwise
// (e.g. a private change ref) if a tag was attached to it. Tags are meant to be used on
// the regular Git tree that users interact with, not on any of the Gerrit trees, so this
// is a negligible risk.
- result.put(name, ref);
+ logger.atFinest().log("Include tag ref %s because user has read on refs/*", refName);
+ resultRefs.put(refName, ref);
} else {
// If its a tag, consider it later.
if (ref.getObjectId() != null) {
+ logger.atFinest().log("Defer tag ref %s", refName);
deferredTags.add(ref);
+ } else {
+ logger.atFinest().log("Filter out tag ref %s that is not a tag", refName);
}
}
- } else if (name.startsWith(RefNames.REFS_SEQUENCES)) {
- // Sequences are internal database implementation details.
- if (viewMetadata) {
- result.put(name, ref);
- }
- } else if (projectState.isAllUsers()
- && (name.equals(RefNames.REFS_EXTERNAL_IDS) || name.equals(RefNames.REFS_GROUPNAMES))) {
- // The notes branches with the external IDs / group names must not be exposed to normal
- // users.
- if (viewMetadata) {
- result.put(name, ref);
- }
- } else if (canReadRef(ref.getLeaf().getName())) {
- // Use the leaf to lookup the control data. If the reference is
- // symbolic we want the control around the final target. If its
- // not symbolic then getLeaf() is a no-op returning ref itself.
- result.put(name, ref);
- } else if (isRefsUsersSelf(ref)) {
- // viewMetadata allows to see all account refs, hence refs/users/self should be included as
- // well
- if (viewMetadata) {
- result.put(name, ref);
+ } else if ((changeId = Change.Id.fromRef(refName)) != null) {
+ // This is a mere performance optimization. RefVisibilityControl could determine the
+ // visibility of these refs just fine. But instead, we use highly-optimized logic that
+ // looks only on the last 10k most recent changes using the change index and a cache.
+ if (hasAccessDatabase) {
+ resultRefs.put(refName, ref);
+ } else if (!visible(repo, changeId)) {
+ logger.atFinest().log("Filter out invisible change ref %s", refName);
+ } else if (RefNames.isRefsEdit(refName) && !visibleEdit(repo, refName)) {
+ logger.atFinest().log("Filter out invisible change edit ref %s", refName);
+ } else {
+ // Change is visible
+ resultRefs.put(refName, ref);
}
+ } else if (refVisibilityControl.isVisible(projectControl, ref.getLeaf().getName())) {
+ resultRefs.put(refName, ref);
}
}
- return new AutoValue_DefaultRefFilter_Result(result, deferredTags);
+ return new AutoValue_DefaultRefFilter_Result(resultRefs, deferredTags);
}
/**
* Returns all refs tag we regard as starting points for reachability computation for tags. In
- * general, these are all refs not managed by Gerrit excluding symbolic refs and tags.
- *
- * <p>We exclude symbolic refs because their target will be included and this will suffice for
- * computing reachability.
+ * general, these are all refs not managed by Gerrit.
*/
private static Map<String, Ref> getTaggableRefsMap(Repository repo)
throws PermissionBackendException {
@@ -305,7 +254,8 @@ class DefaultRefFilter {
r ->
!RefNames.isGerritRef(r.getName())
&& !r.getName().startsWith(RefNames.REFS_TAGS)
- && !r.isSymbolic())
+ && !r.isSymbolic()
+ && !REFS_CONFIG.equals(r.getName()))
.collect(toMap(Ref::getName, r -> r));
} catch (IOException e) {
throw new PermissionBackendException(e);
@@ -450,10 +400,6 @@ class DefaultRefFilter {
return ref.getLeaf().getName().startsWith(Constants.R_TAGS);
}
- private static boolean isRefsUsersSelf(Ref ref) {
- return ref.getName().startsWith(REFS_USERS_SELF);
- }
-
private boolean canReadRef(String ref) throws PermissionBackendException {
try {
permissionBackendForProject.ref(ref).check(RefPermission.READ);
@@ -474,15 +420,6 @@ class DefaultRefFilter {
return true;
}
- private boolean isGroupOwner(
- InternalGroup group, @Nullable IdentifiedUser user, boolean isAdmin) {
- requireNonNull(group);
-
- // Keep this logic in sync with GroupControl#isOwner().
- return isAdmin
- || (user != null && user.getEffectiveGroups().contains(group.getOwnerGroupUUID()));
- }
-
/**
* Returns true if the user can see the provided change ref. Uses NoteDb for evaluation, hence
* does not suffer from the limitations documented in {@link SearchingChangeCacheImpl}.
diff --git a/java/com/google/gerrit/server/permissions/ProjectControl.java b/java/com/google/gerrit/server/permissions/ProjectControl.java
index dfc33395f2..f2f7e8bed2 100644
--- a/java/com/google/gerrit/server/permissions/ProjectControl.java
+++ b/java/com/google/gerrit/server/permissions/ProjectControl.java
@@ -38,6 +38,7 @@ import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.GroupMembership;
import com.google.gerrit.server.config.GitReceivePackGroups;
import com.google.gerrit.server.config.GitUploadPackGroups;
+import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.PermissionBackend.ForChange;
@@ -68,6 +69,8 @@ class ProjectControl {
private final Set<AccountGroup.UUID> uploadGroups;
private final Set<AccountGroup.UUID> receiveGroups;
private final PermissionBackend permissionBackend;
+ private final RefVisibilityControl refVisibilityControl;
+ private final GitRepositoryManager repositoryManager;
private final CurrentUser user;
private final ProjectState state;
private final ChangeControl.Factory changeControlFactory;
@@ -85,6 +88,8 @@ class ProjectControl {
PermissionCollection.Factory permissionFilter,
ChangeControl.Factory changeControlFactory,
PermissionBackend permissionBackend,
+ RefVisibilityControl refVisibilityControl,
+ GitRepositoryManager repositoryManager,
DefaultRefFilter.Factory refFilterFactory,
@Assisted CurrentUser who,
@Assisted ProjectState ps) {
@@ -93,6 +98,8 @@ class ProjectControl {
this.receiveGroups = receiveGroups;
this.permissionFilter = permissionFilter;
this.permissionBackend = permissionBackend;
+ this.refVisibilityControl = refVisibilityControl;
+ this.repositoryManager = repositoryManager;
this.refFilterFactory = refFilterFactory;
user = who;
state = ps;
@@ -122,7 +129,7 @@ class ProjectControl {
RefControl ctl = refControls.get(refName);
if (ctl == null) {
PermissionCollection relevant = permissionFilter.filter(access(), refName, user);
- ctl = new RefControl(this, refName, relevant);
+ ctl = new RefControl(refVisibilityControl, this, repositoryManager, refName, relevant);
refControls.put(refName, ctl);
}
return ctl;
@@ -447,7 +454,7 @@ class ProjectControl {
return canPushToAtLeastOneRef();
case READ_CONFIG:
- return controlForRef(RefNames.REFS_CONFIG).isVisible();
+ return controlForRef(RefNames.REFS_CONFIG).hasReadPermissionOnRef(false);
case BAN_COMMIT:
case READ_REFLOG:
diff --git a/java/com/google/gerrit/server/permissions/RefControl.java b/java/com/google/gerrit/server/permissions/RefControl.java
index 9a2ecdd17e..650c8bab4e 100644
--- a/java/com/google/gerrit/server/permissions/RefControl.java
+++ b/java/com/google/gerrit/server/permissions/RefControl.java
@@ -16,6 +16,7 @@ package com.google.gerrit.server.permissions;
import static com.google.common.base.Preconditions.checkArgument;
+import com.google.common.collect.ImmutableList;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRange;
@@ -28,22 +29,30 @@ import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.logging.CallerFinder;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.PermissionBackend.ForChange;
import com.google.gerrit.server.permissions.PermissionBackend.ForRef;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.util.MagicBranch;
+import java.io.IOException;
import java.util.Collection;
import java.util.EnumSet;
import java.util.List;
import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.Repository;
/** Manages access control for Git references (aka branches, tags). */
class RefControl {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ private final RefVisibilityControl refVisibilityControl;
private final ProjectControl projectControl;
+ private final GitRepositoryManager repositoryManager;
private final String refName;
/** All permissions that apply to this reference. */
@@ -56,10 +65,17 @@ class RefControl {
private Boolean owner;
private Boolean canForgeAuthor;
private Boolean canForgeCommitter;
- private Boolean isVisible;
-
- RefControl(ProjectControl projectControl, String ref, PermissionCollection relevant) {
+ private Boolean hasReadPermissionOnRef;
+
+ RefControl(
+ RefVisibilityControl refVisibilityControl,
+ ProjectControl projectControl,
+ GitRepositoryManager repositoryManager,
+ String ref,
+ PermissionCollection relevant) {
+ this.refVisibilityControl = refVisibilityControl;
this.projectControl = projectControl;
+ this.repositoryManager = repositoryManager;
this.refName = ref;
this.relevant = relevant;
this.callerFinder =
@@ -92,12 +108,27 @@ class RefControl {
return owner;
}
- /** Can this user see this reference exists? */
- boolean isVisible() {
- if (isVisible == null) {
- isVisible = getUser().isInternalUser() || canPerform(Permission.READ);
+ /**
+ * Returns {@code true} if the user has permission to read the ref. This method evaluates {@link
+ * RefPermission#READ} only. Hence, it is not authoritative. For example, it does not tell if the
+ * user can see NoteDb refs such as {@code refs/meta/external-ids} which requires {@link
+ * GlobalPermission#ACCESS_DATABASE} and deny access in this case.
+ */
+ boolean hasReadPermissionOnRef(boolean allowNoteDbRefs) {
+ // Don't allow checking for NoteDb refs unless instructed otherwise.
+ if (!allowNoteDbRefs
+ && (refName.startsWith(Constants.R_TAGS) || RefNames.isGerritRef(refName))) {
+ logger.atWarning().atMostEvery(30, TimeUnit.SECONDS).log(
+ "%s: Can't determine visibility of %s in RefControl. Denying access. "
+ + "This case should have been handled before.",
+ projectControl.getProject().getName(), refName);
+ return false;
}
- return isVisible;
+
+ if (hasReadPermissionOnRef == null) {
+ hasReadPermissionOnRef = getUser().isInternalUser() || canPerform(Permission.READ);
+ }
+ return hasReadPermissionOnRef;
}
/** @return true if this user can add a new patch set to this ref */
@@ -572,7 +603,10 @@ class RefControl {
private boolean can(RefPermission perm) throws PermissionBackendException {
switch (perm) {
case READ:
- return isVisible();
+ if (refName.startsWith(Constants.R_TAGS)) {
+ return isTagVisible();
+ }
+ return refVisibilityControl.isVisible(projectControl, refName);
case CREATE:
// TODO This isn't an accurate test.
return canPerform(refPermissionName(perm));
@@ -622,6 +656,38 @@ class RefControl {
}
throw new PermissionBackendException(perm + " unsupported");
}
+
+ private boolean isTagVisible() throws PermissionBackendException {
+ if (projectControl.asForProject().test(ProjectPermission.READ)) {
+ // The user has READ on refs/* with no effective block permission. This is the broadest
+ // permission one can assign. There is no way to grant access to (specific) tags in Gerrit,
+ // so we have to assume that these users can see all tags because there could be tags that
+ // aren't reachable by any visible ref while the user can see all non-Gerrit refs. This
+ // matches Gerrit's historic behavior.
+ // This makes it so that these users could see commits that they can't see otherwise
+ // (e.g. a private change ref) if a tag was attached to it. Tags are meant to be used on
+ // the regular Git tree that users interact with, not on any of the Gerrit trees, so this
+ // is a negligible risk.
+ return true;
+ }
+
+ try (Repository repo =
+ repositoryManager.openRepository(projectControl.getProject().getNameKey())) {
+ // Tag visibility requires going through RefFilter because it entails loading all taggable
+ // refs and filtering them all by visibility.
+ Ref resolvedRef = repo.getRefDatabase().exactRef(refName);
+ if (resolvedRef == null) {
+ return false;
+ }
+ return projectControl.asForProject()
+ .filter(
+ ImmutableList.of(resolvedRef), repo, PermissionBackend.RefFilterOptions.defaults())
+ .values().stream()
+ .anyMatch(r -> refName.equals(r.getName()));
+ } catch (IOException e) {
+ throw new PermissionBackendException(e);
+ }
+ }
}
private static String refPermissionName(RefPermission refPermission) {
diff --git a/java/com/google/gerrit/server/permissions/RefVisibilityControl.java b/java/com/google/gerrit/server/permissions/RefVisibilityControl.java
new file mode 100644
index 0000000000..6db505138a
--- /dev/null
+++ b/java/com/google/gerrit/server/permissions/RefVisibilityControl.java
@@ -0,0 +1,180 @@
+// Copyright (C) 2020 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.server.permissions;
+
+import static com.google.common.base.Preconditions.checkState;
+
+import com.google.common.base.Throwables;
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.exceptions.NoSuchGroupException;
+import com.google.gerrit.exceptions.StorageException;
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.account.GroupControl;
+import com.google.gerrit.server.project.NoSuchChangeException;
+import com.google.gerrit.server.query.change.ChangeData;
+import javax.inject.Inject;
+import javax.inject.Singleton;
+import org.eclipse.jgit.lib.Constants;
+
+/**
+ * This class is a component that is internal to {@link DefaultPermissionBackend}. It can
+ * authoritatively tell if a ref is accessible by a user.
+ */
+@Singleton
+class RefVisibilityControl {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ private final PermissionBackend permissionBackend;
+ private final GroupControl.GenericFactory groupControlFactory;
+ private final ChangeData.Factory changeDataFactory;
+
+ @Inject
+ RefVisibilityControl(
+ PermissionBackend permissionBackend,
+ GroupControl.GenericFactory groupControlFactory,
+ ChangeData.Factory changeDataFactory) {
+ this.permissionBackend = permissionBackend;
+ this.groupControlFactory = groupControlFactory;
+ this.changeDataFactory = changeDataFactory;
+ }
+
+ /**
+ * Returns an authoritative answer if the ref is visible to the user. Does not have support for
+ * tags and will throw a {@link PermissionBackendException} if asked for tags visibility.
+ */
+ boolean isVisible(ProjectControl projectControl, String refName)
+ throws PermissionBackendException {
+ if (refName.startsWith(Constants.R_TAGS)) {
+ throw new PermissionBackendException(
+ "can't check tags through RefVisibilityControl. Use PermissionBackend#filter instead.");
+ }
+ if (!RefNames.isGerritRef(refName)) {
+ // This is not a special Gerrit ref and not a NoteDb ref. Likely, it's just a ref under
+ // refs/heads or another ref the user created. Apply the regular permissions with inheritance.
+ return projectControl.controlForRef(refName).hasReadPermissionOnRef(false);
+ }
+
+ if (refName.startsWith(RefNames.REFS_CACHE_AUTOMERGE)) {
+ // Internal cache state that is accessible to no one.
+ return false;
+ }
+
+ boolean hasAccessDatabase =
+ permissionBackend
+ .user(projectControl.getUser())
+ .testOrFalse(GlobalPermission.ACCESS_DATABASE);
+ if (hasAccessDatabase) {
+ return true;
+ }
+
+ // Change and change edit visibility
+ Change.Id changeId;
+ if ((changeId = Change.Id.fromRef(refName)) != null) {
+ // Change ref is visible only if the change is visible.
+ ChangeData cd;
+ try {
+ cd = changeDataFactory.create(projectControl.getProject().getNameKey(), changeId);
+ checkState(cd.change().getId().equals(changeId));
+ } catch (StorageException e) {
+ if (Throwables.getCausalChain(e).stream()
+ .anyMatch(e2 -> e2 instanceof NoSuchChangeException)) {
+ // The change was deleted or is otherwise not accessible anymore.
+ // If the caller can see all refs and is allowed to see private changes on refs/, allow
+ // access. This is an escape hatch for receivers of "ref deleted" events.
+ PermissionBackend.ForProject forProject = projectControl.asForProject();
+ return forProject.test(ProjectPermission.READ)
+ && forProject.ref("refs/").test(RefPermission.READ_PRIVATE_CHANGES);
+ }
+ throw new PermissionBackendException(e);
+ }
+ if (RefNames.isRefsEdit(refName)) {
+ // Edits are visible only to the owning user, if change is visible.
+ return visibleEdit(refName, projectControl, cd);
+ }
+ return projectControl.controlFor(cd.notes()).setChangeData(cd).isVisible();
+ }
+
+ // Account visibility
+ CurrentUser user = projectControl.getUser();
+ Account.Id currentUserAccountId = user.isIdentifiedUser() ? user.getAccountId() : null;
+ Account.Id accountId;
+ if ((accountId = Account.Id.fromRef(refName)) != null) {
+ // Account ref is visible only to the corresponding account.
+ if (accountId.equals(currentUserAccountId)
+ && projectControl.controlForRef(refName).hasReadPermissionOnRef(true)) {
+ return true;
+ }
+ return false;
+ }
+
+ // Group visibility
+ AccountGroup.UUID accountGroupUuid;
+ if ((accountGroupUuid = AccountGroup.UUID.fromRef(refName)) != null) {
+ // Group ref is visible only to the corresponding owner group.
+ try {
+ return projectControl.controlForRef(refName).hasReadPermissionOnRef(true)
+ && groupControlFactory.controlFor(user, accountGroupUuid).isOwner();
+ } catch (NoSuchGroupException e) {
+ // The group is broken, but the ref is still around. Pretend the ref is not visible.
+ logger.atWarning().withCause(e).log("Found group ref %s but group isn't parsable", refName);
+ return false;
+ }
+ }
+
+ // We are done checking all cases where we would allow access to Gerrit-managed refs. Deny
+ // access in case we got this far.
+ logger.atFine().log(
+ "Denying access to %s because user doesn't have access to this Gerrit ref", refName);
+ return false;
+ }
+
+ private boolean visibleEdit(String refName, ProjectControl projectControl, ChangeData cd)
+ throws PermissionBackendException {
+ Change.Id id = Change.Id.fromEditRefPart(refName);
+ if (id == null) {
+ throw new IllegalStateException("unable to parse change id from edit ref " + refName);
+ }
+
+ if (!projectControl.controlFor(cd.notes()).setChangeData(cd).isVisible()) {
+ // The user can't see the change so they can't see any edits.
+ return false;
+ }
+
+ if (projectControl.getUser().isIdentifiedUser()
+ && refName.startsWith(
+ RefNames.refsEditPrefix(projectControl.getUser().asIdentifiedUser().getAccountId()))) {
+ logger.atFinest().log("Own change edit ref is visible: %s", refName);
+ return true;
+ }
+
+ try {
+ // Default to READ_PRIVATE_CHANGES as there is no special permission for reading edits.
+ projectControl
+ .asForProject()
+ .ref(cd.change().getDest().branch())
+ .check(RefPermission.READ_PRIVATE_CHANGES);
+ logger.atFinest().log("Foreign change edit ref is visible: %s", refName);
+ return true;
+ } catch (AuthException e) {
+ logger.atFinest().log("Foreign change edit ref is not visible: %s", refName);
+ return false;
+ }
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index fe97688b07..f59238c532 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -1082,6 +1082,7 @@ public class ChangeIT extends AbstractDaemonTest {
com.google.gerrit.acceptance.TestAccount deleteAs)
throws Exception {
try {
+ allow(projectName, "refs/*", Permission.VIEW_PRIVATE_CHANGES, ANONYMOUS_USERS);
requestScopeOperations.setApiUser(owner.id());
ChangeInput in = new ChangeInput();
in.project = projectName.get();
diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
index 47ac7a9f9c..b86fb29795 100644
--- a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java
@@ -1036,6 +1036,8 @@ public class GroupsIT extends AbstractDaemonTest {
@Test
public void pushToDeletedGroupBranchIsRejectedForAllUsersRepo() throws Exception {
+ // refs/deleted-groups is only visible with ACCESS_DATABASE
+ allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE);
String groupRef =
RefNames.refsDeletedGroups(
new AccountGroup.UUID(gApi.groups().create(name("foo")).get().id));
@@ -1217,7 +1219,9 @@ public class GroupsIT extends AbstractDaemonTest {
@Test
public void cannotDeleteDeletedGroupBranch() throws Exception {
- String groupRef = RefNames.refsDeletedGroups(new AccountGroup.UUID(name("foo")));
+ // refs/deleted-groups is only visible with ACCESS_DATABASE
+ allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE);
+ String groupRef = RefNames.refsDeletedGroups(AccountGroup.uuid(name("foo")));
createBranch(allUsers, groupRef);
testCannotDeleteGroupBranch(RefNames.REFS_DELETED_GROUPS + "*", groupRef);
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java b/javatests/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java
index 9420304681..35812fd4a7 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java
+++ b/javatests/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java
@@ -28,6 +28,8 @@ import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.GitUtil;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.reviewdb.client.RefNames;
+import com.google.gerrit.testing.ConfigSuite;
+import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.transport.PushResult;
@@ -48,6 +50,13 @@ public abstract class AbstractPushTag extends AbstractDaemonTest {
}
}
+ @ConfigSuite.Config
+ public static Config skipFalse() {
+ Config config = new Config();
+ config.setBoolean("auth", null, "skipFullRefEvaluationIfAllRefsAreVisible", false);
+ return config;
+ }
+
private RevCommit initialHead;
private TagType tagType;
@@ -87,6 +96,20 @@ public abstract class AbstractPushTag extends AbstractDaemonTest {
}
@Test
+ public void createTagForExistingCommit_withoutGlobalReadPermissions() throws Exception {
+ removeReadAccessOnRefsStar();
+ grantReadAccessOnRefsHeadsStar();
+ createTagForExistingCommit();
+ }
+
+ @Test
+ public void createTagForNewCommit_withoutGlobalReadPermissions() throws Exception {
+ removeReadAccessOnRefsStar();
+ grantReadAccessOnRefsHeadsStar();
+ createTagForNewCommit();
+ }
+
+ @Test
public void fastForward() throws Exception {
allowTagCreation();
String tagName = pushTagForExistingCommit(Status.OK);
@@ -103,6 +126,15 @@ public abstract class AbstractPushTag extends AbstractDaemonTest {
fastForwardTagToExistingCommit(tagName, expectedStatus);
fastForwardTagToNewCommit(tagName, expectedStatus);
+ // Above we just fast-forwarded the tag to a new commit which is not part of any branch. By
+ // default this tag is not visible, as users can only see tags that point to commits that are
+ // part of visible branches, which is not the case for this tag. It's odd that we allow the user
+ // to create such a tag that is then not visible to the creator. Below we want to fast-forward
+ // this tag, but this is only possible if the tag is visible. To make it visible we must allow
+ // the user to read all tags, regardless of whether it points to a commit that is part of a
+ // visible branch.
+ allowReadingAllTag();
+
allowForcePushOnRefsTags();
fastForwardTagToExistingCommit(tagName, Status.OK);
fastForwardTagToNewCommit(tagName, Status.OK);
@@ -224,6 +256,33 @@ public abstract class AbstractPushTag extends AbstractDaemonTest {
assertThat(refUpdate.getStatus()).named(tagType.name()).isEqualTo(expectedStatus);
}
+ private void removeReadAccessOnRefsStar() throws Exception {
+ removePermission(allProjects, "refs/heads/*", Permission.READ);
+ removePermission(project, "refs/heads/*", Permission.READ);
+ }
+
+ private void grantReadAccessOnRefsHeadsStar() throws Exception {
+ grant(project, "refs/heads/*", Permission.READ, false, REGISTERED_USERS);
+ }
+
+ private void allowReadingAllTag() throws Exception {
+ // Tags are only visible if the commits to which they point are part of a visible branch.
+ // To make all tags visible, including tags that point to commits that are not part of a visible
+ // branch, either auth.skipFullRefEvaluationIfAllRefsAreVisible in gerrit.config needs to be
+ // true, or the user must have READ access for all refs in the repository.
+
+ if (cfg.getBoolean("auth", "skipFullRefEvaluationIfAllRefsAreVisible", true)) {
+ return;
+ }
+
+ // By default READ access in the All-Projects project is granted to registered users on refs/*,
+ // which makes all refs, except refs/meta/config, visible to them. refs/meta/config is not
+ // visible since by default READ access to it is exclusively granted to the project owners only.
+ // This means to make all refs, and thus all tags, visible, we must allow registered users to
+ // see the refs/meta/config branch.
+ allow(project, "refs/meta/config", Permission.READ, REGISTERED_USERS);
+ }
+
private void allowTagCreation() throws Exception {
grant(project, "refs/tags/*", tagType.createPermission, false, REGISTERED_USERS);
}
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/GetBranchIT.java b/javatests/com/google/gerrit/acceptance/rest/project/GetBranchIT.java
new file mode 100644
index 0000000000..56265c6893
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/rest/project/GetBranchIT.java
@@ -0,0 +1,470 @@
+// Copyright (C) 2020 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.server.group.SystemGroupBackend.ANONYMOUS_USERS;
+import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
+import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.acceptance.testsuite.group.GroupOperations;
+import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
+import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
+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.BranchInfo;
+import com.google.gerrit.extensions.api.projects.TagInfo;
+import com.google.gerrit.extensions.api.projects.TagInput;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
+import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.reviewdb.client.AccountGroup;
+import com.google.gerrit.reviewdb.client.Change;
+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.notedb.Sequences;
+import com.google.gerrit.testing.ConfigSuite;
+import com.google.inject.Inject;
+import org.eclipse.jgit.junit.TestRepository;
+import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.Repository;
+import org.junit.Test;
+
+public class GetBranchIT extends AbstractDaemonTest {
+ @Inject private GroupOperations groupOperations;
+ @Inject private ProjectOperations projectOperations;
+ @Inject private RequestScopeOperations requestScopeOperations;
+
+ @ConfigSuite.Config
+ public static Config skipFalse() {
+ Config config = new Config();
+ config.setBoolean("auth", null, "skipFullRefEvaluationIfAllRefsAreVisible", false);
+ return config;
+ }
+
+ @Test
+ public void cannotGetNonExistingBranch() {
+ assertBranchNotFound(project, RefNames.fullName("non-existing"));
+ }
+
+ @Test
+ public void getBranch() throws Exception {
+ requestScopeOperations.setApiUser(user.id());
+ assertBranchFound(project, RefNames.fullName("master"));
+ }
+
+ @Test
+ public void getBranchByShortName() throws Exception {
+ requestScopeOperations.setApiUser(user.id());
+ assertBranchFound(project, "master");
+ }
+
+ @Test
+ public void cannotGetNonVisibleBranch() throws Exception {
+ String branchName = "master";
+
+ // block read access to the branch
+ block(project, RefNames.fullName(branchName), Permission.READ, ANONYMOUS_USERS);
+
+ requestScopeOperations.setApiUser(user.id());
+ assertBranchNotFound(project, RefNames.fullName(branchName));
+ }
+
+ @Test
+ public void cannotGetNonVisibleBranchByShortName() throws Exception {
+ String branchName = "master";
+
+ // block read access to the branch
+ block(project, RefNames.fullName(branchName), Permission.READ, ANONYMOUS_USERS);
+
+ requestScopeOperations.setApiUser(user.id());
+ assertBranchNotFound(project, branchName);
+ }
+
+ @Test
+ public void getChangeRef() throws Exception {
+ // create a change
+ Change.Id changeId = createChange("refs/for/master").getPatchSetId().changeId();
+
+ // a user without the 'Access Database' capability can see the change ref
+ requestScopeOperations.setApiUser(user.id());
+ String changeRef = RefNames.patchSetRef(PatchSet.id(changeId, 1));
+ assertBranchFound(project, changeRef);
+ }
+
+ @Test
+ public void getChangeRefOfNonVisibleChange() throws Exception {
+ // create a change
+ String branchName = "master";
+ Change.Id changeId = createChange("refs/for/" + branchName).getPatchSetId().changeId();
+
+ // block read access to the branch
+ block(project, RefNames.fullName(branchName), Permission.READ, ANONYMOUS_USERS);
+
+ // a user without the 'Access Database' capability cannot see the change ref
+ requestScopeOperations.setApiUser(user.id());
+ String changeRef = RefNames.patchSetRef(PatchSet.id(changeId, 1));
+ assertBranchNotFound(project, changeRef);
+
+ // a user with the 'Access Database' capability can see the change ref
+ testGetRefWithAccessDatabase(project, changeRef);
+ }
+
+ @Test
+ public void getChangeEditRef() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ // create a change
+ Change.Id changeId = createChange("refs/for/master").getPatchSetId().changeId();
+
+ // create a change edit by 'user'
+ requestScopeOperations.setApiUser(user.id());
+ gApi.changes().id(changeId.get()).edit().create();
+
+ // every user can see their own change edit refs
+ String changeEditRef = RefNames.refsEdit(user.id(), changeId, PatchSet.id(changeId, 1));
+ assertBranchFound(project, changeEditRef);
+
+ // a user without the 'Access Database' capability cannot see the change edit ref of another
+ // user
+ requestScopeOperations.setApiUser(user2.id());
+ assertBranchNotFound(project, changeEditRef);
+
+ // a user with the 'Access Database' capability can see the change edit ref of another user
+ testGetRefWithAccessDatabase(project, changeEditRef);
+ }
+
+ @Test
+ public void cannotGetChangeEditRefOfNonVisibleChange() throws Exception {
+ // create a change
+ String branchName = "master";
+ Change.Id changeId = createChange("refs/for/" + branchName).getPatchSetId().changeId();
+
+ // create a change edit by 'user'
+ requestScopeOperations.setApiUser(user.id());
+ gApi.changes().id(changeId.get()).edit().create();
+
+ // make the change non-visible by blocking read access on the destination
+ block(project, RefNames.fullName(branchName), Permission.READ, ANONYMOUS_USERS);
+
+ // user cannot see their own change edit refs if the change is no longer visible
+ String changeEditRef = RefNames.refsEdit(user.id(), changeId, PatchSet.id(changeId, 1));
+ assertBranchNotFound(project, changeEditRef);
+
+ // a user with the 'Access Database' capability can see the change edit ref
+ testGetRefWithAccessDatabase(project, changeEditRef);
+ }
+
+ @Test
+ public void getChangeMetaRef() throws Exception {
+ // create a change
+ Change.Id changeId = createChange("refs/for/master").getPatchSetId().changeId();
+
+ // A user without the 'Access Database' capability can see the change meta ref.
+ // This may be surprising, as 'Access Database' guards access to meta refs and the change meta
+ // ref is a meta ref, however change meta refs have been always visible to all users that can
+ // see the change and some tools rely on seeing these refs, so we have to keep the current
+ // behaviour.
+ requestScopeOperations.setApiUser(user.id());
+ String changeMetaRef = RefNames.changeMetaRef(changeId);
+ assertBranchFound(project, changeMetaRef);
+ }
+
+ @Test
+ public void getRefsMetaConfig() throws Exception {
+ // a non-project owner cannot get the refs/meta/config branch
+ requestScopeOperations.setApiUser(user.id());
+ assertBranchNotFound(project, RefNames.REFS_CONFIG);
+
+ // a non-project owner cannot get the refs/meta/config branch even with the 'Access Database'
+ // capability
+ allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE);
+ try {
+ assertBranchNotFound(project, RefNames.REFS_CONFIG);
+ } finally {
+ removeGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE);
+ }
+
+ requestScopeOperations.setApiUser(user.id());
+
+ // a project owner can get the refs/meta/config branch
+ allow(project, "refs/*", Permission.OWNER, REGISTERED_USERS);
+ assertBranchFound(project, RefNames.REFS_CONFIG);
+ }
+
+ @Test
+ public void getUserRefOfOtherUser() throws Exception {
+ String userRef = RefNames.refsUsers(admin.id());
+
+ // a user without the 'Access Database' capability cannot see the user ref of another user
+ requestScopeOperations.setApiUser(user.id());
+ assertBranchNotFound(allUsers, userRef);
+
+ // a user with the 'Access Database' capability can see the user ref of another user
+ testGetRefWithAccessDatabase(allUsers, userRef);
+ }
+
+ @Test
+ public void getOwnUserRef() throws Exception {
+ // every user can see the own user ref
+ requestScopeOperations.setApiUser(user.id());
+ assertBranchFound(allUsers, RefNames.refsUsers(user.id()));
+
+ // TODO: every user can see the own user ref via the magic ref/users/self ref
+ // requestScopeOperations.setApiUser(user.id());
+ // assertBranchFound(allUsers, RefNames.REFS_USERS_SELF);
+ }
+
+ @Test
+ public void getExternalIdsRefs() throws Exception {
+ // a user without the 'Access Database' capability cannot see the refs/meta/external-ids ref
+ requestScopeOperations.setApiUser(user.id());
+ assertBranchNotFound(allUsers, RefNames.REFS_EXTERNAL_IDS);
+
+ // a user with the 'Access Database' capability can see the refs/meta/external-ids ref
+ testGetRefWithAccessDatabase(allUsers, RefNames.REFS_EXTERNAL_IDS);
+ }
+
+ @Test
+ public void getGroupRef() throws Exception {
+ // create a group
+ AccountGroup.UUID ownerGroupUuid =
+ groupOperations.newGroup().name("owner-group").addMember(admin.id()).create();
+ AccountGroup.UUID testGroupUuid =
+ groupOperations.newGroup().name("test-group").ownerGroupUuid(ownerGroupUuid).create();
+
+ // a non-group owner without the 'Access Database' capability cannot see the group ref
+ requestScopeOperations.setApiUser(user.id());
+ String groupRef = RefNames.refsGroups(testGroupUuid);
+ assertBranchNotFound(allUsers, groupRef);
+
+ // a non-group owner with the 'Access Database' capability can see the group ref
+ testGetRefWithAccessDatabase(allUsers, groupRef);
+
+ // a group owner can see the group ref if the group ref is visible
+ groupOperations.group(ownerGroupUuid).forUpdate().addMember(user.id()).update();
+ assertBranchFound(allUsers, groupRef);
+
+ // A group owner cannot see the group ref if the group ref is not visible.
+ // The READ access for refs/groups/* must be blocked on All-Projects rather than All-Users.
+ // This is because READ access for refs/groups/* on All-Users is by default granted to
+ // REGISTERED_USERS, and if an ALLOW rule and a BLOCK rule are on the same project and ref,
+ // the ALLOW rule takes precedence.
+ block(allProjects, "refs/groups/*", Permission.READ, ANONYMOUS_USERS);
+ assertBranchNotFound(allUsers, groupRef);
+ }
+
+ @Test
+ public void getGroupNamesRef() throws Exception {
+ // a user without the 'Access Database' capability cannot see the refs/meta/group-names ref
+ requestScopeOperations.setApiUser(user.id());
+ assertBranchNotFound(allUsers, RefNames.REFS_GROUPNAMES);
+
+ // a user with the 'Access Database' capability can see the refs/meta/group-names ref
+ testGetRefWithAccessDatabase(allUsers, RefNames.REFS_GROUPNAMES);
+ }
+
+ @Test
+ public void getDeletedGroupRef() throws Exception {
+ // Create a deleted group ref. We must create a directly in the repo, since group deletion is
+ // not supported yet.
+ String deletedGroupRef = RefNames.refsDeletedGroups(AccountGroup.uuid("deleted-group"));
+ try (TestRepository<Repository> testRepo =
+ new TestRepository<>(repoManager.openRepository(allUsers))) {
+ testRepo
+ .branch(deletedGroupRef)
+ .commit()
+ .message("Some Message")
+ .add("group.config", "content")
+ .create();
+ }
+
+ requestScopeOperations.setApiUser(user.id());
+ assertBranchNotFound(allUsers, deletedGroupRef);
+
+ // a user with the 'Access Database' capability can see the deleted group ref
+ testGetRefWithAccessDatabase(allUsers, deletedGroupRef);
+ }
+
+ @Test
+ public void getDraftCommentsRef() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ // create a change
+ String fileName = "a.txt";
+ Change change = createChange("A Change", fileName, "content").getChange().change();
+
+ // create a draft comment by the by 'user'
+ requestScopeOperations.setApiUser(user.id());
+ DraftInput draftInput = new DraftInput();
+ draftInput.path = fileName;
+ draftInput.line = 0;
+ draftInput.message = "Some Comment";
+ gApi.changes().id(change.getChangeId()).current().createDraft(draftInput);
+
+ // every user can see their own draft comments refs
+ // TODO: is this a bug?
+ String draftCommentsRef = RefNames.refsDraftComments(change.getId(), user.id());
+ assertBranchFound(allUsers, draftCommentsRef);
+
+ // a user without the 'Access Database' capability cannot see the draft comments ref of another
+ // user
+ requestScopeOperations.setApiUser(user2.id());
+ assertBranchNotFound(allUsers, draftCommentsRef);
+
+ // a user with the 'Access Database' capability can see the draft comments ref of another user
+ testGetRefWithAccessDatabase(allUsers, draftCommentsRef);
+ }
+
+ @Test
+ public void getStarredChangesRef() throws Exception {
+ TestAccount user2 = accountCreator.user2();
+
+ // create a change
+ Change change = createChange().getChange().change();
+
+ // let user star the change
+ requestScopeOperations.setApiUser(user.id());
+ gApi.accounts().self().starChange(Integer.toString(change.getChangeId()));
+
+ // every user can see their own starred changes refs
+ // TODO: is this a bug?
+ String starredChangesRef = RefNames.refsStarredChanges(change.getId(), user.id());
+ assertBranchFound(allUsers, starredChangesRef);
+
+ // a user without the 'Access Database' capability cannot see the starred changes ref of another
+ // user
+ requestScopeOperations.setApiUser(user2.id());
+ assertBranchNotFound(allUsers, starredChangesRef);
+
+ // a user with the 'Access Database' capability can see the starred changes ref of another user
+ testGetRefWithAccessDatabase(allUsers, starredChangesRef);
+ }
+
+ @Test
+ public void getTagRef() throws Exception {
+ // create a tag
+ TagInput input = new TagInput();
+ input.message = "My Tag";
+ input.revision = projectOperations.project(project).getHead("master").name();
+ TagInfo tagInfo = gApi.projects().name(project.get()).tag("my-tag").create(input).get();
+
+ // any user who can see the project, can see the tag
+ requestScopeOperations.setApiUser(user.id());
+ assertBranchFound(project, tagInfo.ref);
+ }
+
+ @Test
+ public void cannotGetTagRefThatPointsToNonVisibleBranch() throws Exception {
+ // create a tag
+ TagInput input = new TagInput();
+ input.message = "My Tag";
+ input.revision = projectOperations.project(project).getHead("master").name();
+ TagInfo tagInfo = gApi.projects().name(project.get()).tag("my-tag").create(input).get();
+
+ // block read access to the branch
+ block(allProjects, RefNames.fullName("master"), Permission.READ, ANONYMOUS_USERS);
+
+ // if the user cannot see the project, the tag is not visible
+ requestScopeOperations.setApiUser(user.id());
+ assertBranchNotFound(project, tagInfo.ref);
+ }
+
+ @Test
+ public void getSymbolicRef() throws Exception {
+ // 'HEAD' is visible since it points to 'master' that is visible
+ requestScopeOperations.setApiUser(user.id());
+ assertBranchFound(project, "HEAD");
+ }
+
+ @Test
+ public void cannotGetSymbolicRefThatPointsToNonVisibleBranch() throws Exception {
+ // block read access to the branch to which HEAD points by default
+ block(allProjects, RefNames.fullName("master"), Permission.READ, ANONYMOUS_USERS);
+
+ // since 'master' is not visible, 'HEAD' which points to 'master' is also not visible
+ requestScopeOperations.setApiUser(user.id());
+ assertBranchNotFound(project, "HEAD");
+ }
+
+ @Test
+ public void getAccountSequenceRef() throws Exception {
+ // a user without the 'Access Database' capability cannot see the refs/sequences/accounts ref
+ requestScopeOperations.setApiUser(user.id());
+ String accountSequenceRef = RefNames.REFS_SEQUENCES + Sequences.NAME_ACCOUNTS;
+ assertBranchNotFound(allUsers, accountSequenceRef);
+
+ // a user with the 'Access Database' capability can see the refs/sequences/accounts ref
+ testGetRefWithAccessDatabase(allUsers, accountSequenceRef);
+ }
+
+ @Test
+ public void getChangeSequenceRef() throws Exception {
+ // a user without the 'Access Database' capability cannot see the refs/sequences/changes ref
+ requestScopeOperations.setApiUser(user.id());
+ String changeSequenceRef = RefNames.REFS_SEQUENCES + Sequences.NAME_CHANGES;
+ assertBranchNotFound(allProjects, changeSequenceRef);
+
+ // a user with the 'Access Database' capability can see the refs/sequences/changes ref
+ testGetRefWithAccessDatabase(allProjects, changeSequenceRef);
+ }
+
+ @Test
+ public void getGroupSequenceRef() throws Exception {
+ // a user without the 'Access Database' capability cannot see the refs/sequences/groups ref
+ requestScopeOperations.setApiUser(user.id());
+ String groupSequenceRef = RefNames.REFS_SEQUENCES + Sequences.NAME_GROUPS;
+ assertBranchNotFound(allUsers, groupSequenceRef);
+
+ // a user with the 'Access Database' capability can see the refs/sequences/groups ref
+ testGetRefWithAccessDatabase(allUsers, groupSequenceRef);
+ }
+
+ @Test
+ public void getVersionMetaRef() throws Exception {
+ // TODO: a user without the 'Access Database' capability cannot see the refs/meta/version ref
+ // requestScopeOperations.setApiUser(user.id());
+ // assertBranchNotFound(allProjects, RefNames.REFS_VERSION);
+
+ // a user with the 'Access Database' capability can see the refs/meta/vaersion ref
+ testGetRefWithAccessDatabase(allProjects, RefNames.REFS_VERSION);
+ }
+
+ private void testGetRefWithAccessDatabase(Project.NameKey project, String ref) throws Exception {
+ allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE);
+ try {
+ requestScopeOperations.setApiUser(user.id());
+ assertBranchFound(project, ref);
+ } finally {
+ removeGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE);
+ }
+ }
+
+ private void assertBranchNotFound(Project.NameKey project, String ref) {
+ ResourceNotFoundException exception =
+ assertThrows(
+ ResourceNotFoundException.class,
+ () -> gApi.projects().name(project.get()).branch(ref).get());
+ assertThat(exception).hasMessageThat().isEqualTo("Not found: " + ref);
+ }
+
+ private void assertBranchFound(Project.NameKey project, String ref) throws RestApiException {
+ BranchInfo branchInfo = gApi.projects().name(project.get()).branch(ref).get();
+ assertThat(branchInfo.ref).isEqualTo(RefNames.fullName(ref));
+ }
+}
diff --git a/javatests/com/google/gerrit/server/permissions/RefControlTest.java b/javatests/com/google/gerrit/server/permissions/RefControlTest.java
index 58adc0c6c9..e529745462 100644
--- a/javatests/com/google/gerrit/server/permissions/RefControlTest.java
+++ b/javatests/com/google/gerrit/server/permissions/RefControlTest.java
@@ -116,11 +116,17 @@ public class RefControlTest extends GerritBaseTests {
}
private void assertCanRead(String ref, ProjectControl u) {
- assertThat(u.controlForRef(ref).isVisible()).named("can read " + ref).isTrue();
+ assertThat(u.controlForRef(ref).hasReadPermissionOnRef(true))
+ // This should be false but the test relies on inheritance into refs/tags
+ .named("can read " + ref)
+ .isTrue();
}
private void assertCannotRead(String ref, ProjectControl u) {
- assertThat(u.controlForRef(ref).isVisible()).named("cannot read " + ref).isFalse();
+ assertThat(u.controlForRef(ref).hasReadPermissionOnRef(true))
+ // This should be false but the test relies on inheritance into refs/tags
+ .named("cannot read " + ref)
+ .isFalse();
}
private void assertCanSubmit(String ref, ProjectControl u) {
@@ -200,6 +206,7 @@ public class RefControlTest extends GerritBaseTests {
@Inject private TransferConfig transferConfig;
@Inject private MetricMaker metricMaker;
@Inject private ProjectConfig.Factory projectConfigFactory;
+ @Inject private RefVisibilityControl refVisibilityControl;
@Before
public void setUp() throws Exception {
@@ -989,6 +996,8 @@ public class RefControlTest extends GerritBaseTests {
sectionSorter,
changeControlFactory,
permissionBackend,
+ refVisibilityControl,
+ repoManager,
refFilterFactory,
new MockUser(name, memberOf),
newProjectState(local));