From 3c462799233fb90e90f2f995f88c965c0332654f Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Mon, 2 Nov 2020 14:30:54 +0100 Subject: 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 --- .../google/gerrit/reviewdb/client/RefNames.java | 30 ++ .../gerrit/server/permissions/ChangeControl.java | 33 +- .../server/permissions/DefaultRefFilter.java | 174 ++++---- .../gerrit/server/permissions/ProjectControl.java | 11 +- .../gerrit/server/permissions/RefControl.java | 89 +++- .../server/permissions/RefVisibilityControl.java | 248 +++++++++++ .../gerrit/acceptance/api/change/ChangeIT.java | 1 + .../gerrit/acceptance/api/group/GroupsIT.java | 4 + .../acceptance/rest/project/AbstractPushTag.java | 59 +++ .../acceptance/rest/project/GetBranchIT.java | 454 +++++++++++++++++++++ .../gerrit/server/permissions/RefControlTest.java | 13 +- tools/BUILD | 2 +- 12 files changed, 996 insertions(+), 122 deletions(-) create mode 100644 java/com/google/gerrit/server/permissions/RefVisibilityControl.java create mode 100644 javatests/com/google/gerrit/acceptance/rest/project/GetBranchIT.java diff --git a/java/com/google/gerrit/reviewdb/client/RefNames.java b/java/com/google/gerrit/reviewdb/client/RefNames.java index fd2fb56b0e..994a299ef1 100644 --- a/java/com/google/gerrit/reviewdb/client/RefNames.java +++ b/java/com/google/gerrit/reviewdb/client/RefNames.java @@ -118,6 +118,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(); @@ -263,6 +268,31 @@ public class RefNames { return REFS_CONFIG.equals(ref); } + /** + * 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. + * + *

Any ref for which this method evaluates to true will be served to users who have the {@code + * ACCESS_DATABASE} capability. + * + *

CautionAny 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_EXTERNAL_IDS) + || ref.startsWith(REFS_CACHE_AUTOMERGE) + || ref.startsWith(REFS_DRAFT_COMMENTS) + || ref.startsWith(REFS_DELETED_GROUPS) + || ref.startsWith(REFS_SEQUENCES) + || ref.startsWith(REFS_GROUPS) + || ref.startsWith(REFS_GROUPNAMES) + || ref.startsWith(REFS_USERS) + || ref.startsWith(REFS_STARRED_CHANGES) + || ref.startsWith(REFS_REJECT_COMMITS); + } + static Integer parseShardedRefPart(String name) { if (name == null) { return null; diff --git a/java/com/google/gerrit/server/permissions/ChangeControl.java b/java/com/google/gerrit/server/permissions/ChangeControl.java index f4e659eb7a..4d88f241b3 100644 --- a/java/com/google/gerrit/server/permissions/ChangeControl.java +++ b/java/com/google/gerrit/server/permissions/ChangeControl.java @@ -70,6 +70,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; @@ -78,7 +80,8 @@ class ChangeControl { } ForChange asForChange(@Nullable ChangeData cd, @Nullable Provider db) { - return new ForChangeImpl(cd, db); + setChangeData(cd); + return new ForChangeImpl(db); } private CurrentUser getUser() { @@ -93,12 +96,27 @@ class ChangeControl { return notes.getChange(); } + private ChangeData changeData(ReviewDb db) { + if (cd == null) { + cd = changeDataFactory.create(db, 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(ReviewDb db, @Nullable ChangeData cd) throws OrmException { + boolean isVisible(ReviewDb db) throws OrmException { if (getChange().isPrivate() && !isPrivateVisible(db, cd)) { 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? */ @@ -215,13 +233,12 @@ class ChangeControl { || getUser().isInternalUser(); } - private class ForChangeImpl extends ForChange { - private ChangeData cd; + class ForChangeImpl extends ForChange { + private Map labels; private String resourcePath; - ForChangeImpl(@Nullable ChangeData cd, @Nullable Provider db) { - this.cd = cd; + ForChangeImpl(@Nullable Provider db) { this.db = db; } @@ -295,7 +312,7 @@ class ChangeControl { try { switch (perm) { case READ: - return isVisible(db(), changeData()); + return isVisible(db()); 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 55d7c6c62f..2a0e41c848 100644 --- a/java/com/google/gerrit/server/permissions/DefaultRefFilter.java +++ b/java/com/google/gerrit/server/permissions/DefaultRefFilter.java @@ -15,11 +15,10 @@ package com.google.gerrit.server.permissions; 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_CHANGES; 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.common.collect.ImmutableList; import com.google.common.collect.ImmutableSet; @@ -30,21 +29,16 @@ 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.reviewdb.server.ReviewDb; 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; @@ -77,8 +71,8 @@ class DefaultRefFilter { private final ChangeNotes.Factory changeNotesFactory; @Nullable private final SearchingChangeCacheImpl changeCache; private final Provider db; - 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,8 +89,8 @@ class DefaultRefFilter { ChangeNotes.Factory changeNotesFactory, @Nullable SearchingChangeCacheImpl changeCache, Provider db, - GroupCache groupCache, PermissionBackend permissionBackend, + RefVisibilityControl refVisibilityControl, @GerritServerConfig Config config, MetricMaker metricMaker, @Assisted ProjectControl projectControl) { @@ -104,8 +98,8 @@ class DefaultRefFilter { this.changeNotesFactory = changeNotesFactory; this.changeCache = changeCache; this.db = db; - this.groupCache = groupCache; this.permissionBackend = permissionBackend; + this.refVisibilityControl = refVisibilityControl; this.skipFullRefEvaluationIfAllRefsAreVisible = config.getBoolean("auth", "skipFullRefEvaluationIfAllRefsAreVisible", true); this.projectControl = projectControl; @@ -145,92 +139,66 @@ 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 result = new HashMap<>(); + boolean hasAccessDatabase = + permissionBackend + .user(projectControl.getUser()) + .testOrFalse(GlobalPermission.ACCESS_DATABASE); + Map resultRefs = Maps.newHashMapWithExpectedSize(refs.size()); List deferredTags = new ArrayList<>(); + boolean hasReadOnRefsStar = + checkProjectPermission(permissionBackendForProject, ProjectPermission.READ); 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 its a tag, consider it later. - if (ref.getObjectId() != null) { - deferredTags.add(ref); - } - } else if (name.startsWith(RefNames.REFS_SEQUENCES)) { - // Sequences are internal database implementation details. - if (viewMetadata) { - result.put(name, ref); + if (hasReadOnRefsStar) { + // 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. + 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 (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); } } // If we have tags that were deferred, we need to do a revision walk // to identify what tags we can actually reach, and what we cannot. // - if (!deferredTags.isEmpty() && (!result.isEmpty() || opts.filterTagsSeparately())) { + if (!deferredTags.isEmpty() && (!resultRefs.isEmpty() || opts.filterTagsSeparately())) { TagMatcher tags = tagCache .get(projectState.getNameKey()) @@ -239,19 +207,38 @@ class DefaultRefFilter { repo, opts.filterTagsSeparately() ? filter( - repo.getAllRefs(), + getTaggableRefsMap(repo), repo, opts.toBuilder().setFilterTagsSeparately(false).build()) .values() - : result.values()); + : resultRefs.values()); for (Ref tag : deferredTags) { if (tags.isReachable(tag)) { - result.put(tag.getName(), tag); + resultRefs.put(tag.getName(), tag); } } } + return resultRefs; + } - return result; + /** + * Returns all refs tag we regard as starting points for reachability computation for tags. In + * general, these are all refs not managed by Gerrit. + */ + private static Map getTaggableRefsMap(Repository repo) + throws PermissionBackendException { + try { + return repo.getRefDatabase().getRefs().stream() + .filter( + r -> + !RefNames.isGerritRef(r.getName()) + && !r.getName().startsWith(RefNames.REFS_TAGS) + && !r.isSymbolic() + && !REFS_CONFIG.equals(r.getName())) + .collect(toMap(Ref::getName, r -> r)); + } catch (IOException e) { + throw new PermissionBackendException(e); + } } private Map fastHideRefsMetaConfig(Map refs) @@ -391,10 +378,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); @@ -414,13 +397,4 @@ 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())); - } } diff --git a/java/com/google/gerrit/server/permissions/ProjectControl.java b/java/com/google/gerrit/server/permissions/ProjectControl.java index 0094979732..c3d0719283 100644 --- a/java/com/google/gerrit/server/permissions/ProjectControl.java +++ b/java/com/google/gerrit/server/permissions/ProjectControl.java @@ -35,6 +35,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; @@ -67,6 +68,8 @@ class ProjectControl { private final Set uploadGroups; private final Set 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; @@ -84,6 +87,8 @@ class ProjectControl { PermissionCollection.Factory permissionFilter, ChangeControl.Factory changeControlFactory, PermissionBackend permissionBackend, + RefVisibilityControl refVisibilityControl, + GitRepositoryManager repositoryManager, DefaultRefFilter.Factory refFilterFactory, @Assisted CurrentUser who, @Assisted ProjectState ps) { @@ -92,6 +97,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; @@ -121,7 +128,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; @@ -434,7 +441,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 74b04a32bf..5d6910a68d 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.ImmutableMap; import com.google.common.flogger.FluentLogger; import com.google.gerrit.common.data.Permission; import com.google.gerrit.common.data.PermissionRange; @@ -27,6 +28,7 @@ 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; @@ -35,16 +37,23 @@ import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.util.MagicBranch; import com.google.gwtorm.server.OrmException; import com.google.inject.util.Providers; +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. */ @@ -57,10 +66,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 = @@ -93,12 +109,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 */ @@ -577,7 +608,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)); @@ -627,6 +661,43 @@ 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( + ImmutableMap.of(resolvedRef.getName(), resolvedRef), + repo, + PermissionBackend.RefFilterOptions.builder() + .setFilterMeta(true) + .setFilterTagsSeparately(true) + .build()) + .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..7af94dc78a --- /dev/null +++ b/java/com/google/gerrit/server/permissions/RefVisibilityControl.java @@ -0,0 +1,248 @@ +// 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.Optional; +import com.google.common.base.Throwables; +import com.google.common.flogger.FluentLogger; +import com.google.gerrit.common.Nullable; +import com.google.gerrit.common.errors.NoSuchGroupException; +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.reviewdb.server.ReviewDb; +import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.account.GroupControl; +import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.project.NoSuchChangeException; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.gerrit.server.util.ManualRequestContext; +import com.google.gerrit.server.util.OneOffRequestContext; +import com.google.gwtorm.server.OrmException; +import javax.inject.Inject; +import javax.inject.Provider; +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 Provider dbProvider; + private final PermissionBackend permissionBackend; + private final GroupControl.GenericFactory groupControlFactory; + private final ChangeData.Factory changeDataFactory; + private final OneOffRequestContext oneOffRequestContext; + + @Inject + RefVisibilityControl( + Provider dbProvider, + PermissionBackend permissionBackend, + GroupControl.GenericFactory groupControlFactory, + ChangeData.Factory changeDataFactory, + OneOffRequestContext oneOffRequestContext) { + this.dbProvider = dbProvider; + this.permissionBackend = permissionBackend; + this.groupControlFactory = groupControlFactory; + this.changeDataFactory = changeDataFactory; + this.oneOffRequestContext = oneOffRequestContext; + } + + /** + * 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. + try (CloseableOneTimeReviewDb ignored = new CloseableOneTimeReviewDb()) { + ChangeData cd; + try { + cd = + changeDataFactory.create( + dbProvider.get(), projectControl.getProject().getNameKey(), changeId); + checkState(cd.change().getId().equals(changeId)); + } catch (OrmException 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 isVisible(projectControl.controlFor(getNotes(cd)).setChangeData(cd)); + } + } + + // 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 (!isVisible(projectControl.controlFor(getNotes(cd)).setChangeData(cd))) { + // 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().get()) + .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; + } catch (OrmException e) { + throw new PermissionBackendException(e); + } + } + + private ChangeNotes getNotes(ChangeData cd) throws PermissionBackendException { + try { + return cd.notes(); + } catch (OrmException e) { + throw new PermissionBackendException(e); + } + } + + private boolean isVisible(ChangeControl changeControl) throws PermissionBackendException { + try { + Optional db = getReviewDb(); + return changeControl.isVisible(dbProvider.get()); + } catch (OrmException e) { + throw new PermissionBackendException(e); + } + } + + private Optional getReviewDb() { + try { + return Optional.of(dbProvider.get()); + } catch (Exception e) { + return Optional.absent(); + } + } + + /** Helper to establish a database connection. */ + private class CloseableOneTimeReviewDb implements AutoCloseable { + @Nullable private final ManualRequestContext ctx; + + CloseableOneTimeReviewDb() throws PermissionBackendException { + if (!getReviewDb().isPresent()) { + try { + ctx = oneOffRequestContext.open(); + } catch (OrmException e) { + throw new PermissionBackendException(e); + } + } else { + ctx = null; + } + } + + @Override + public void close() { + if (ctx != null) { + ctx.close(); + } + } + } +} diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java index e6dfa1d865..99d14d5a83 100644 --- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java @@ -1117,6 +1117,7 @@ public class ChangeIT extends AbstractDaemonTest { throws Exception { try { setApiUser(owner); + allow(projectName, "refs/*", Permission.VIEW_PRIVATE_CHANGES, ANONYMOUS_USERS); ChangeInput in = new ChangeInput(); in.project = projectName.get(); in.branch = "refs/heads/master"; diff --git a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java index e2d390e6fd..c6da810896 100644 --- a/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java +++ b/javatests/com/google/gerrit/acceptance/api/group/GroupsIT.java @@ -1018,6 +1018,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)); @@ -1196,6 +1198,8 @@ public class GroupsIT extends AbstractDaemonTest { @Test public void cannotDeleteDeletedGroupBranch() throws Exception { + // refs/deleted-groups is only visible with ACCESS_DATABASE + allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); String groupRef = RefNames.refsDeletedGroups(new 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 12468c3734..f095ae106b 100644 --- a/javatests/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java +++ b/javatests/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java @@ -31,6 +31,8 @@ import com.google.gerrit.common.data.Permission; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.project.testing.Util; +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; @@ -51,6 +53,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; @@ -89,6 +98,20 @@ public abstract class AbstractPushTag extends AbstractDaemonTest { removePushFromRefsTags(); } + @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(); @@ -106,6 +129,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); @@ -227,6 +259,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..c6e2484416 --- /dev/null +++ b/javatests/com/google/gerrit/acceptance/rest/project/GetBranchIT.java @@ -0,0 +1,454 @@ +// 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.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.Sequences; +import com.google.gerrit.testing.ConfigSuite; +import com.google.gerrit.testing.NoteDbMode; +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; + + @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 { + setApiUser(user); + assertBranchFound(project, RefNames.fullName("master")); + } + + @Test + public void getBranchByShortName() throws Exception { + setApiUser(user); + 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); + + setApiUser(user); + 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); + + setApiUser(user); + 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 + setApiUser(user); + String changeRef = RefNames.patchSetRef(new 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 + setApiUser(user); + String changeRef = RefNames.patchSetRef(new 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' + setApiUser(user); + gApi.changes().id(changeId.get()).edit().create(); + + // every user can see their own change edit refs + String changeEditRef = RefNames.refsEdit(user.id, changeId, new PatchSet.Id(changeId, 1)); + assertBranchFound(project, changeEditRef); + + // a user without the 'Access Database' capability cannot see the change edit ref of another + // user + setApiUser(user2); + 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' + setApiUser(user); + 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, new 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. + setApiUser(user); + String changeMetaRef = RefNames.changeMetaRef(changeId); + if (NoteDbMode.get().ordinal() >= NoteDbMode.WRITE.ordinal()) { + // Branch is there when we write to NoteDb + assertBranchFound(project, changeMetaRef); + } + } + + @Test + public void getRefsMetaConfig() throws Exception { + // a non-project owner cannot get the refs/meta/config branch + setApiUser(user); + 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); + } + + setApiUser(user); + + // 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 + setApiUser(user); + 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 + setApiUser(user); + assertBranchFound(allUsers, RefNames.refsUsers(user.id)); + + // TODO: every user can see the own user ref via the magic ref/users/self ref + // setApiUser(user); + // 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 + setApiUser(user); + 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 + setApiUser(user); + 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 + setApiUser(user); + 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(new AccountGroup.UUID("deleted-group")); + TestRepository testRepo = + new TestRepository<>(repoManager.openRepository(allUsers)); + testRepo + .branch(deletedGroupRef) + .commit() + .message("Some Message") + .add("group.config", "content") + .create(); + + setApiUser(user); + 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' + setApiUser(user); + 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); + if (NoteDbMode.get().ordinal() >= NoteDbMode.WRITE.ordinal()) { + // Branch is there when we write to NoteDb + assertBranchFound(allUsers, draftCommentsRef); + } + + // a user without the 'Access Database' capability cannot see the draft comments ref of another + // user + setApiUser(user2); + assertBranchNotFound(allUsers, draftCommentsRef); + + // a user with the 'Access Database' capability can see the draft comments ref of another user + if (NoteDbMode.get().ordinal() >= NoteDbMode.WRITE.ordinal()) { + // Branch is there when we write to NoteDb + 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 + setApiUser(user); + 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 + setApiUser(user2); + 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 = gApi.projects().name(project.get()).head(); + TagInfo tagInfo = gApi.projects().name(project.get()).tag("my-tag").create(input).get(); + + // any user who can see the project, can see the tag + setApiUser(user); + assertBranchFound(project, tagInfo.ref); + } + + @Test + public void cannotGetTagRefThatPointsToNonVisibleBranch() throws Exception { + // create a tag + TagInput input = new TagInput(); + input.message = "My Tag"; + input.revision = gApi.projects().name(project.get()).head(); + 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 + setApiUser(user); + assertBranchNotFound(project, tagInfo.ref); + } + + @Test + public void getSymbolicRef() throws Exception { + // 'HEAD' is visible since it points to 'master' that is visible + setApiUser(user); + 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 + setApiUser(user); + assertBranchNotFound(project, "HEAD"); + } + + @Test + public void getAccountSequenceRef() throws Exception { + // a user without the 'Access Database' capability cannot see the refs/sequences/accounts ref + setApiUser(user); + 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 getGroupSequenceRef() throws Exception { + // a user without the 'Access Database' capability cannot see the refs/sequences/groups ref + setApiUser(user); + 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); + } + + private void testGetRefWithAccessDatabase(Project.NameKey project, String ref) throws Exception { + allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); + try { + setApiUser(user); + 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 0d19984427..bf8ea86f02 100644 --- a/javatests/com/google/gerrit/server/permissions/RefControlTest.java +++ b/javatests/com/google/gerrit/server/permissions/RefControlTest.java @@ -121,11 +121,17 @@ public class RefControlTest { } 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) { @@ -206,6 +212,7 @@ public class RefControlTest { @Inject private DefaultRefFilter.Factory refFilterFactory; @Inject private TransferConfig transferConfig; @Inject private MetricMaker metricMaker; + @Inject private RefVisibilityControl refVisibilityControl; @Before public void setUp() throws Exception { @@ -1012,6 +1019,8 @@ public class RefControlTest { sectionSorter, changeControlFactory, permissionBackend, + refVisibilityControl, + repoManager, refFilterFactory, new MockUser(name, memberOf), newProjectState(local)); diff --git a/tools/BUILD b/tools/BUILD index 9a53c8b6c6..c246c57a4f 100644 --- a/tools/BUILD +++ b/tools/BUILD @@ -39,7 +39,7 @@ java_package_configuration( "-Xep:ElementsCountedInLoop:WARN", "-Xep:EqualsHashCode:WARN", "-Xep:EqualsIncompatibleType:WARN", - "-Xep:ExpectedExceptionChecker:ERROR", + "-Xep:ExpectedExceptionChecker:WARN", "-Xep:Finally:WARN", "-Xep:FloatingPointLiteralPrecision:WARN", "-Xep:FragmentInjection:WARN", -- cgit v1.2.3 From 3c9320d98ee978ab812e1d2b5a475e9ff46d3bea Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Wed, 11 Nov 2020 23:08:49 +0000 Subject: Fix tests for stable-2.15 branch Add the 'manual' tag to wct test_suite templates, so it is excluded from bazel test //... (cherry picked from commit ae42cd00bdfa8a34e75c563b62f0151a561cc82b) Change-Id: Idc62df90e90e6000fa0792799a3997580fc6b011 --- polygerrit-ui/app/BUILD | 1 + 1 file changed, 1 insertion(+) diff --git a/polygerrit-ui/app/BUILD b/polygerrit-ui/app/BUILD index 8012c20897..39533d6b92 100644 --- a/polygerrit-ui/app/BUILD +++ b/polygerrit-ui/app/BUILD @@ -146,6 +146,7 @@ DIRECTORIES = [ tags = [ # Should not run sandboxed. "local", + "manual", "template", ], ) for directory in DIRECTORIES] -- cgit v1.2.3 From 1346eab23259f8dc4adec9cb098e2f818c9cf79d Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Wed, 11 Nov 2020 21:11:54 +0000 Subject: Validate Gerrit changes on stable-2.15 with Jenkins Change-Id: I35c47ba60c08e8d5d1f767672b5e83b7d29fea1b --- Jenkinsfile | 1 + 1 file changed, 1 insertion(+) create mode 100644 Jenkinsfile diff --git a/Jenkinsfile b/Jenkinsfile new file mode 100644 index 0000000000..565dd45586 --- /dev/null +++ b/Jenkinsfile @@ -0,0 +1 @@ +gerritPipeline() -- cgit v1.2.3 From a9313bf170644a2b2fb698787ce64c9230f26445 Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Mon, 2 Nov 2020 14:30:54 +0100 Subject: 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 --- .../acceptance/rest/project/AbstractPushTag.java | 23 ++ .../acceptance/rest/project/GetBranchIT.java | 384 +++++++++++++++++++++ .../google/gerrit/reviewdb/client/RefNames.java | 21 ++ .../google/gerrit/server/git/VisibleRefFilter.java | 89 +++-- .../server/permissions/RefVisibilityControl.java | 215 ++++++++++++ .../gerrit/server/project/ChangeControl.java | 37 +- .../gerrit/server/project/ProjectControl.java | 25 +- .../google/gerrit/server/project/RefControl.java | 90 ++++- .../gerrit/server/project/RefControlTest.java | 15 +- tools/BUILD | 2 +- 10 files changed, 818 insertions(+), 83 deletions(-) create mode 100644 gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/GetBranchIT.java create mode 100644 gerrit-server/src/main/java/com/google/gerrit/server/permissions/RefVisibilityControl.java diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java index cea907d941..101a9affe4 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/AbstractPushTag.java @@ -157,6 +157,29 @@ public abstract class AbstractPushTag extends AbstractDaemonTest { pushTagDeletion(tagName, Status.OK); } + @Test + public void createTagForExistingCommit_withoutGlobalReadPermissions() throws Exception { + removeReadAccessOnRefsStar(); + grantReadAccessOnRefsHeadsStar(); + createTagForExistingCommit(); + } + + @Test + public void createTagForNewCommit_withoutGlobalReadPermissions() throws Exception { + removeReadAccessOnRefsStar(); + grantReadAccessOnRefsHeadsStar(); + createTagForNewCommit(); + } + + 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 String pushTagForExistingCommit(Status expectedStatus) throws Exception { return pushTag(null, false, false, expectedStatus); } diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/GetBranchIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/GetBranchIT.java new file mode 100644 index 0000000000..bd93a44150 --- /dev/null +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/project/GetBranchIT.java @@ -0,0 +1,384 @@ +// 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.testutil.GerritJUnit.assertThrows; + +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.TestAccount; +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.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.config.AllUsersName; +import com.google.gerrit.testutil.NoteDbMode; +import com.google.inject.Inject; +import org.junit.Test; + +public class GetBranchIT extends AbstractDaemonTest { + @Inject protected AllUsersName allUsers; + + @Test + public void cannotGetNonExistingBranch() { + assertBranchNotFound(project, RefNames.fullName("non-existing")); + } + + @Test + public void getBranch() throws Exception { + setApiUser(user); + assertBranchFound(project, RefNames.fullName("master")); + } + + @Test + public void getBranchByShortName() throws Exception { + setApiUser(user); + 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); + + setApiUser(user); + 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); + + setApiUser(user); + 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 + setApiUser(user); + String changeRef = RefNames.patchSetRef(new 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 + setApiUser(user); + String changeRef = RefNames.patchSetRef(new 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' + setApiUser(user); + gApi.changes().id(changeId.get()).edit().create(); + + // every user can see their own change edit refs + String changeEditRef = RefNames.refsEdit(user.id, changeId, new PatchSet.Id(changeId, 1)); + assertBranchFound(project, changeEditRef); + + // a user without the 'Access Database' capability cannot see the change edit ref of another + // user + setApiUser(user2); + 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' + setApiUser(user); + 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, new 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. + setApiUser(user); + String changeMetaRef = RefNames.changeMetaRef(changeId); + if (NoteDbMode.get().ordinal() >= NoteDbMode.WRITE.ordinal()) { + // Branch is there when we write to NoteDb + assertBranchFound(project, changeMetaRef); + } + } + + @Test + public void getRefsMetaConfig() throws Exception { + // a non-project owner cannot get the refs/meta/config branch + setApiUser(user); + 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); + } + + setApiUser(user); + + // 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 + setApiUser(user); + 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 + setApiUser(user); + assertBranchFound(allUsers, RefNames.refsUsers(user.id)); + + // TODO: every user can see the own user ref via the magic ref/users/self ref + // setApiUser(user); + // 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 + setApiUser(user); + 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 { + // Groups were not yet in NoteDb in this release. + } + + @Test + public void getGroupNamesRef() throws Exception { + // Groups were not yet in NoteDb in this release. + } + + @Test + public void getDeletedGroupRef() throws Exception { + // Groups were not yet in NoteDb in this release. + } + + @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' + setApiUser(user); + 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); + if (NoteDbMode.get().ordinal() >= NoteDbMode.WRITE.ordinal()) { + // Branch is there when we write to NoteDb + assertBranchFound(allUsers, draftCommentsRef); + } + + // a user without the 'Access Database' capability cannot see the draft comments ref of another + // user + setApiUser(user2); + assertBranchNotFound(allUsers, draftCommentsRef); + + // a user with the 'Access Database' capability can see the draft comments ref of another user + if (NoteDbMode.get().ordinal() >= NoteDbMode.WRITE.ordinal()) { + // Branch is there when we write to NoteDb + 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 + setApiUser(user); + 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 + setApiUser(user2); + 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 = gApi.projects().name(project.get()).head(); + TagInfo tagInfo = gApi.projects().name(project.get()).tag("my-tag").create(input).get(); + + // any user who can see the project, can see the tag + setApiUser(user); + assertBranchFound(project, tagInfo.ref); + } + + @Test + public void cannotGetTagRefThatPointsToNonVisibleBranch() throws Exception { + // create a tag + TagInput input = new TagInput(); + input.message = "My Tag"; + input.revision = gApi.projects().name(project.get()).head(); + TagInfo tagInfo = gApi.projects().name(project.get()).tag("my-tag").create(input).get(); + + // block read access to the branch + block(project, RefNames.fullName("master"), Permission.READ, ANONYMOUS_USERS); + + // if the user cannot see the project, the tag is not visible + setApiUser(user); + assertBranchNotFound(project, tagInfo.ref); + } + + @Test + public void getSymbolicRef() throws Exception { + // 'HEAD' is visible since it points to 'master' that is visible + setApiUser(user); + assertBranchFound(project, "HEAD"); + } + + @Test + public void cannotGetSymbolicRefThatPointsToNonVisibleBranch() throws Exception { + // block read access to the branch to which HEAD points by default + block(project, RefNames.fullName("master"), Permission.READ, ANONYMOUS_USERS); + + // since 'master' is not visible, 'HEAD' which points to 'master' is also not visible + setApiUser(user); + assertBranchNotFound(project, "HEAD"); + } + + @Test + public void getAccountSequenceRef() throws Exception { + // Sequences were not yet in NoteDb in this release. + } + + @Test + public void getGroupSequenceRef() throws Exception { + // Sequences were not yet in NoteDb in this release. + } + + private void testGetRefWithAccessDatabase(Project.NameKey project, String ref) throws Exception { + allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); + try { + setApiUser(user); + 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/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 89de9dcd64..16896aab96 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 @@ -101,11 +101,32 @@ public class RefNames { return ref; } + /** + * 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. + */ + public static boolean isGerritRef(String ref) { + return ref.startsWith(REFS_CHANGES) + || ref.startsWith(REFS_EXTERNAL_IDS) + || ref.startsWith(REFS_CACHE_AUTOMERGE) + || ref.startsWith(REFS_DRAFT_COMMENTS) + || ref.startsWith(REFS_SEQUENCES) + || ref.startsWith(REFS_USERS) + || ref.startsWith(REFS_STARRED_CHANGES) + || ref.startsWith(REFS_REJECT_COMMITS); + } + public static String changeMetaRef(Change.Id id) { StringBuilder r = newStringBuilder().append(REFS_CHANGES); 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(); 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 c16c195d3a..9b368beeae 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 @@ -14,7 +14,6 @@ package com.google.gerrit.server.git; -import static com.google.gerrit.reviewdb.client.RefNames.REFS_CACHE_AUTOMERGE; import static com.google.gerrit.reviewdb.client.RefNames.REFS_CHANGES; import static com.google.gerrit.reviewdb.client.RefNames.REFS_CONFIG; import static com.google.gerrit.reviewdb.client.RefNames.REFS_USERS_SELF; @@ -38,6 +37,7 @@ import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.permissions.ProjectPermission; import com.google.gerrit.server.permissions.RefPermission; +import com.google.gerrit.server.permissions.RefVisibilityControl; import com.google.gerrit.server.project.ProjectControl; import com.google.gerrit.server.project.ProjectState; import com.google.gerrit.server.query.change.ChangeData; @@ -80,6 +80,7 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook { private final PermissionBackend.ForProject perm; private final ProjectState projectState; private final Repository git; + private final RefVisibilityControl refVisibilityControl; private ProjectControl projectCtl; private boolean showMetadata = true; private String userEditPrefix; @@ -93,6 +94,7 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook { Provider db, Provider user, PermissionBackend permissionBackend, + RefVisibilityControl refVisibilityControl, @Assisted ProjectState projectState, @Assisted Repository git) { this.tagCache = tagCache; @@ -105,6 +107,7 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook { permissionBackend.user(user).database(db).project(projectState.getProject().getNameKey()); this.projectState = projectState; this.git = git; + this.refVisibilityControl = refVisibilityControl; } /** Show change references. Default is {@code true}. */ @@ -128,68 +131,54 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook { } } - Account.Id userId; - boolean viewMetadata; + boolean hasAccessDatabase; if (user.get().isIdentifiedUser()) { - viewMetadata = withUser.testOrFalse(GlobalPermission.ACCESS_DATABASE); + hasAccessDatabase = withUser.testOrFalse(GlobalPermission.ACCESS_DATABASE); IdentifiedUser u = user.get().asIdentifiedUser(); - userId = u.getAccountId(); + Account.Id userId = u.getAccountId(); userEditPrefix = RefNames.refsEditPrefix(userId); } else { - userId = null; - viewMetadata = false; + hasAccessDatabase = false; } - Map result = new HashMap<>(); + Map resultRefs = new HashMap<>(); List deferredTags = new ArrayList<>(); projectCtl = projectState.controlFor(user.get()); for (Ref ref : refs.values()) { - String name = ref.getName(); + String refName = ref.getName(); Change.Id changeId; - Account.Id accountId; - if (name.startsWith(REFS_CACHE_AUTOMERGE) || (!showMetadata && isMetadata(name))) { - continue; - } else if (RefNames.isRefsEdit(name)) { - // Edits are visible only to the owning user, if change is visible. - if (viewMetadata || visibleEdit(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(changeId)) { - result.put(name, ref); - } - } else if ((accountId = Account.Id.fromRef(name)) != null) { - // Account ref is visible only to corresponding account. - if (viewMetadata || (accountId.equals(userId) && canReadRef(name))) { - result.put(name, ref); - } + if (!showMetadata && isMetadata(refName)) { + log.debug("Filter out metadata ref %s", refName); } else if (isTag(ref)) { // If its a tag, consider it later. if (ref.getObjectId() != null) { + log.debug("Defer tag ref %s", refName); deferredTags.add(ref); + } else { + log.debug("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 ((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(changeId)) { + log.debug("Filter out invisible change ref %s", refName); + } else if (RefNames.isRefsEdit(refName) && !visibleEdit(refName)) { + log.debug("Filter out invisible change edit ref %s", refName); + } else { + // Change is visible + resultRefs.put(refName, ref); } - } else if (projectState.isAllUsers() && name.equals(RefNames.REFS_EXTERNAL_IDS)) { - // The notes branch with the external IDs of all users 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 { + try { + if (refVisibilityControl.isVisible(projectCtl, ref.getLeaf().getName())) { + resultRefs.put(refName, ref); + } + } catch (PermissionBackendException e) { + log.warn("could not evaluate ref permission", e); } } } @@ -197,22 +186,22 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook { // If we have tags that were deferred, we need to do a revision walk // to identify what tags we can actually reach, and what we cannot. // - if (!deferredTags.isEmpty() && (!result.isEmpty() || filterTagsSeparately)) { + if (!deferredTags.isEmpty() && (!resultRefs.isEmpty() || filterTagsSeparately)) { TagMatcher tags = tagCache .get(projectState.getNameKey()) .matcher( tagCache, git, - filterTagsSeparately ? filter(git.getAllRefs()).values() : result.values()); + filterTagsSeparately ? filter(git.getAllRefs()).values() : resultRefs.values()); for (Ref tag : deferredTags) { if (tags.isReachable(tag)) { - result.put(tag.getName(), tag); + resultRefs.put(tag.getName(), tag); } } } - return result; + return resultRefs; } private Map fastHideRefsMetaConfig(Map refs) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/RefVisibilityControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/RefVisibilityControl.java new file mode 100644 index 0000000000..d75bc7dbc1 --- /dev/null +++ b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/RefVisibilityControl.java @@ -0,0 +1,215 @@ +// 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.Optional; +import com.google.common.base.Throwables; +import com.google.gerrit.common.Nullable; +import com.google.gerrit.reviewdb.client.Account; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.RefNames; +import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.project.ChangeControl; +import com.google.gerrit.server.project.NoSuchChangeException; +import com.google.gerrit.server.project.ProjectControl; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.gerrit.server.util.ManualRequestContext; +import com.google.gerrit.server.util.OneOffRequestContext; +import com.google.gwtorm.server.OrmException; +import javax.inject.Inject; +import javax.inject.Provider; +import javax.inject.Singleton; +import org.eclipse.jgit.lib.Constants; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * This class is a component that is internal to {@link DefaultPermissionBackend}. It can + * authoritatively tell if a ref is accessible by a user. + */ +@Singleton +public class RefVisibilityControl { + private static final Logger logger = LoggerFactory.getLogger(RefVisibilityControl.class); + + private final Provider dbProvider; + private final OneOffRequestContext oneOffRequestContext; + private final PermissionBackend permissionBackend; + private final ChangeData.Factory changeDataFactory; + + @Inject + RefVisibilityControl( + Provider dbProvider, + OneOffRequestContext oneOffRequestContext, + PermissionBackend permissionBackend, + ChangeData.Factory changeDataFactory) { + this.dbProvider = dbProvider; + this.oneOffRequestContext = oneOffRequestContext; + this.permissionBackend = permissionBackend; + 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. + */ + public 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. + try (CloseableOneTimeReviewDb ignored = new CloseableOneTimeReviewDb()) { + ChangeData cd; + try { + cd = + changeDataFactory.create( + dbProvider.get(), projectControl.getProject().getNameKey(), changeId); + checkState(cd.change().getId().equals(changeId)); + } catch (OrmException 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); + } + 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 isVisible(projectControl.controlFor(getNotes(cd)).setChangeData(cd)); + } + } + + // 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; + } + + // We are done checking all cases where we would allow access to Gerrit-managed refs. Deny + // access in case we got this far. + logger.debug( + "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 (!isVisible(projectControl.controlFor(getNotes(cd)).setChangeData(cd))) { + // 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.debug("Own change edit ref is visible: %s", refName); + return true; + } + + return false; + } + + private ChangeNotes getNotes(ChangeData cd) throws PermissionBackendException { + try { + return cd.notes(); + } catch (OrmException e) { + throw new PermissionBackendException(e); + } + } + + private boolean isVisible(ChangeControl changeControl) throws PermissionBackendException { + try { + return changeControl.isVisible(dbProvider.get()); + } catch (OrmException e) { + throw new PermissionBackendException(e); + } + } + + private Optional getReviewDb() { + try { + return Optional.of(dbProvider.get()); + } catch (Exception e) { + return Optional.absent(); + } + } + + /** Helper to establish a database connection. */ + private class CloseableOneTimeReviewDb implements AutoCloseable { + @Nullable private final ManualRequestContext ctx; + + CloseableOneTimeReviewDb() throws PermissionBackendException { + if (!getReviewDb().isPresent()) { + try { + ctx = oneOffRequestContext.open(); + } catch (OrmException e) { + throw new PermissionBackendException(e); + } + } else { + ctx = null; + } + } + + @Override + public void close() { + if (ctx != null) { + ctx.close(); + } + } + } +} diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java index 1582d43958..a605a7d240 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java @@ -50,7 +50,7 @@ import java.util.Map; import java.util.Set; /** Access control management for a user accessing a single change. */ -class ChangeControl { +public class ChangeControl { @Singleton static class Factory { private final ChangeData.Factory changeDataFactory; @@ -87,6 +87,8 @@ class ChangeControl { private final ChangeNotes notes; private final PatchSetUtil patchSetUtil; + private ChangeData cd; + ChangeControl( ChangeData.Factory changeDataFactory, ApprovalsUtil approvalsUtil, @@ -128,17 +130,20 @@ class ChangeControl { return notes; } - /** Can this user see this change? */ - private boolean isVisible(ReviewDb db, @Nullable ChangeData cd) throws OrmException { - if (getChange().isPrivate() && !isPrivateVisible(db, cd)) { - return false; + public ChangeControl setChangeData(@Nullable ChangeData cd) { + if (cd != null) { + this.cd = cd; } - return isRefVisible(); + return this; } - /** Can the user see this change? Does not account for draft status */ - private boolean isRefVisible() { - return getRefControl().isVisible(); + /** Can this user see this change? */ + public boolean isVisible(ReviewDb db) throws OrmException { + if (getChange().isPrivate() && !isPrivateVisible(db, changeData(db))) { + return false; + } + // Does the user have READ permission on the destination? + return refControl.asForRef().testOrFalse(RefPermission.READ); } /** Can this user abandon this change? */ @@ -237,7 +242,7 @@ class ChangeControl { /** Is this user a reviewer for the change? */ private boolean isReviewer(ReviewDb db, @Nullable ChangeData cd) throws OrmException { if (getUser().isIdentifiedUser()) { - Collection results = changeData(db, cd).reviewers().all(); + Collection results = setChangeData(cd).changeData(db).reviewers().all(); return results.contains(getUser().getAccountId()); } return false; @@ -282,8 +287,8 @@ class ChangeControl { || getProjectControl().isAdmin(); } - private ChangeData changeData(ReviewDb db, @Nullable ChangeData cd) { - return cd != null ? cd : changeDataFactory.create(db, getNotes()); + private ChangeData changeData(ReviewDb db) { + return this.cd != null ? cd : changeDataFactory.create(db, getNotes()); } private boolean isPrivateVisible(ReviewDb db, ChangeData cd) throws OrmException { @@ -294,15 +299,13 @@ class ChangeControl { } ForChange asForChange(@Nullable ChangeData cd, @Nullable Provider db) { - return new ForChangeImpl(cd, db); + return new ForChangeImpl(db); } private class ForChangeImpl extends ForChange { - private ChangeData cd; private Map labels; - ForChangeImpl(@Nullable ChangeData cd, @Nullable Provider db) { - this.cd = cd; + ForChangeImpl(@Nullable Provider db) { this.db = db; } @@ -370,7 +373,7 @@ class ChangeControl { try { switch (perm) { case READ: - return isVisible(db(), changeData()); + return isVisible(db()); case ABANDON: return canAbandon(db()); case DELETE: diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java index 76e9e9816f..a0b1f678b8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java @@ -41,6 +41,8 @@ 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.git.VisibleRefFilter; import com.google.gerrit.server.group.SystemGroupBackend; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.permissions.FailedPermissionBackend; @@ -51,6 +53,7 @@ import com.google.gerrit.server.permissions.PermissionBackend.ForProject; import com.google.gerrit.server.permissions.PermissionBackend.ForRef; import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.permissions.ProjectPermission; +import com.google.gerrit.server.permissions.RefVisibilityControl; import com.google.gerrit.server.query.change.ChangeData; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -134,6 +137,9 @@ public class ProjectControl { private final CommitsCollection commits; private final ChangeControl.Factory changeControlFactory; private final PermissionCollection.Factory permissionFilter; + private final RefVisibilityControl refVisibilityControl; + private final VisibleRefFilter.Factory visibleRefFilterFactory; + private final GitRepositoryManager gitRepositoryManager; private List allSections; private Map refControls; @@ -147,6 +153,9 @@ public class ProjectControl { CommitsCollection commits, ChangeControl.Factory changeControlFactory, PermissionBackend permissionBackend, + RefVisibilityControl refVisibilityControl, + GitRepositoryManager gitRepositoryManager, + VisibleRefFilter.Factory visibleRefFilterFactory, @Assisted CurrentUser who, @Assisted ProjectState ps) { this.changeControlFactory = changeControlFactory; @@ -155,6 +164,9 @@ public class ProjectControl { this.permissionFilter = permissionFilter; this.commits = commits; this.perm = permissionBackend.user(who); + this.refVisibilityControl = refVisibilityControl; + this.gitRepositoryManager = gitRepositoryManager; + this.visibleRefFilterFactory = visibleRefFilterFactory; user = who; state = ps; } @@ -186,7 +198,14 @@ public 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( + visibleRefFilterFactory, + refVisibilityControl, + this, + gitRepositoryManager, + refName, + relevant); refControls.put(refName, ctl); } return ctl; @@ -428,11 +447,11 @@ public class ProjectControl { } } - ForProject asForProject() { + public ForProject asForProject() { return new ForProjectImpl(); } - private class ForProjectImpl extends ForProject { + public class ForProjectImpl extends ForProject { @Override public ForProject user(CurrentUser user) { return forUser(user).asForProject().database(db); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java index cde2c8098e..2ba5e0f6d2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java @@ -16,6 +16,7 @@ package com.google.gerrit.server.project; import static com.google.common.base.Preconditions.checkArgument; +import com.google.common.collect.ImmutableMap; import com.google.gerrit.common.data.Permission; import com.google.gerrit.common.data.PermissionRange; import com.google.gerrit.common.data.PermissionRule; @@ -24,15 +25,20 @@ 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.git.VisibleRefFilter; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.permissions.FailedPermissionBackend; import com.google.gerrit.server.permissions.PermissionBackend.ForChange; import com.google.gerrit.server.permissions.PermissionBackend.ForRef; import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.gerrit.server.permissions.ProjectPermission; import com.google.gerrit.server.permissions.RefPermission; +import com.google.gerrit.server.permissions.RefVisibilityControl; import com.google.gerrit.server.query.change.ChangeData; import com.google.gwtorm.server.OrmException; import com.google.inject.util.Providers; +import java.io.IOException; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -42,10 +48,16 @@ import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Set; +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). */ public class RefControl { + private final VisibleRefFilter.Factory visibleRefFilterFactory; + private final RefVisibilityControl refVisibilityControl; private final ProjectControl projectControl; + private final GitRepositoryManager repositoryManager; private final String refName; /** All permissions that apply to this reference. */ @@ -57,10 +69,19 @@ public 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( + VisibleRefFilter.Factory visibleRefFilterFactory, + RefVisibilityControl refVisibilityControl, + ProjectControl projectControl, + GitRepositoryManager repositoryManager, + String ref, + PermissionCollection relevant) { + this.visibleRefFilterFactory = visibleRefFilterFactory; + this.refVisibilityControl = refVisibilityControl; this.projectControl = projectControl; + this.repositoryManager = repositoryManager; this.refName = ref; this.relevant = relevant; this.effective = new HashMap<>(); @@ -83,7 +104,13 @@ public class RefControl { if (relevant.isUserSpecific()) { return newCtl.controlForRef(getRefName()); } - return new RefControl(newCtl, getRefName(), relevant); + return new RefControl( + visibleRefFilterFactory, + refVisibilityControl, + newCtl, + repositoryManager, + getRefName(), + relevant); } /** Is this user a ref owner? */ @@ -99,14 +126,24 @@ public class RefControl { return owner; } - /** Can this user see this reference exists? */ - boolean isVisible() { - if (isVisible == null) { - isVisible = + /** + * 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. + */ + public boolean hasReadPermissionOnRef(boolean allowNoteDbRefs) { + // Don't allow checking for NoteDb refs unless instructed otherwise. + if (!allowNoteDbRefs + && (refName.startsWith(Constants.R_TAGS) || RefNames.isGerritRef(refName))) { + return false; + } + if (hasReadPermissionOnRef == null) { + hasReadPermissionOnRef = (getUser().isInternalUser() || canPerform(Permission.READ)) && isProjectStatePermittingRead(); } - return isVisible; + return hasReadPermissionOnRef; } /** Can this user see other users change edits? */ @@ -552,7 +589,10 @@ public 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(perm.permissionName().get()); @@ -588,4 +628,34 @@ public 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 visibleRefFilterFactory.create(projectControl.getProjectState(), repo) + .filter(ImmutableMap.of(resolvedRef.getName(), resolvedRef), true).values().stream() + .anyMatch(r -> refName.equals(r.getName())); + } catch (IOException e) { + throw new PermissionBackendException(e); + } + } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java index 1fc95c18bc..dff5af07a9 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java @@ -55,12 +55,15 @@ import com.google.gerrit.server.config.AllProjectsNameProvider; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.AllUsersNameProvider; import com.google.gerrit.server.config.SitePaths; +import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.git.TransferConfig; +import com.google.gerrit.server.git.VisibleRefFilter; import com.google.gerrit.server.index.SingleVersionModule.SingleVersionListener; import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.ProjectPermission; import com.google.gerrit.server.permissions.RefPermission; +import com.google.gerrit.server.permissions.RefVisibilityControl; import com.google.gerrit.server.schema.SchemaCreator; import com.google.gerrit.server.util.RequestContext; import com.google.gerrit.server.util.ThreadLocalRequestContext; @@ -118,11 +121,13 @@ public class RefControlTest { } private void assertCanRead(String ref, ProjectControl u) { - assertThat(u.controlForRef(ref).isVisible()).named("can read " + ref).isTrue(); + assertThat(u.controlForRef(ref).hasReadPermissionOnRef(true)).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)) + .named("cannot read " + ref) + .isFalse(); } private void assertCanSubmit(String ref, ProjectControl u) { @@ -209,6 +214,9 @@ public class RefControlTest { @Inject private InMemoryDatabase schemaFactory; @Inject private ThreadLocalRequestContext requestContext; @Inject private TransferConfig transferConfig; + @Inject private RefVisibilityControl refVisibilityControl; + @Inject private GitRepositoryManager gitRepositoryManager; + @Inject private VisibleRefFilter.Factory visibleRefFilterFactory; @Before public void setUp() throws Exception { @@ -886,6 +894,9 @@ public class RefControlTest { null, // commitsCollection changeControlFactory, permissionBackend, + refVisibilityControl, + gitRepositoryManager, + visibleRefFilterFactory, new MockUser(name, memberOf), newProjectState(local)); } diff --git a/tools/BUILD b/tools/BUILD index 646414037a..094cb0149f 100644 --- a/tools/BUILD +++ b/tools/BUILD @@ -48,7 +48,7 @@ java_package_configuration( "-Xep:ElementsCountedInLoop:WARN", "-Xep:EqualsHashCode:WARN", "-Xep:EqualsIncompatibleType:WARN", - "-Xep:ExpectedExceptionChecker:ERROR", + "-Xep:ExpectedExceptionChecker:WARN", "-Xep:Finally:WARN", "-Xep:FloatingPointLiteralPrecision:WARN", "-Xep:FragmentInjection:WARN", -- cgit v1.2.3 From 094f09c8f15fb16d427e09ff2dff839be22e6fba Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Thu, 12 Nov 2020 00:55:38 +0000 Subject: Set version to 2.16.24 Change-Id: If3ea98f0db8ef6b102ce3775e19a64739b883f8e --- tools/maven/gerrit-acceptance-framework_pom.xml | 2 +- tools/maven/gerrit-extension-api_pom.xml | 2 +- tools/maven/gerrit-plugin-api_pom.xml | 2 +- tools/maven/gerrit-plugin-gwtui_pom.xml | 2 +- tools/maven/gerrit-war_pom.xml | 2 +- version.bzl | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tools/maven/gerrit-acceptance-framework_pom.xml b/tools/maven/gerrit-acceptance-framework_pom.xml index d3c5f35813..b3703af61a 100644 --- a/tools/maven/gerrit-acceptance-framework_pom.xml +++ b/tools/maven/gerrit-acceptance-framework_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-acceptance-framework - 2.16.24-SNAPSHOT + 2.16.24 jar Gerrit Code Review - Acceptance Test Framework Framework for Gerrit's acceptance tests diff --git a/tools/maven/gerrit-extension-api_pom.xml b/tools/maven/gerrit-extension-api_pom.xml index 7ab1abfc0a..d330b12753 100644 --- a/tools/maven/gerrit-extension-api_pom.xml +++ b/tools/maven/gerrit-extension-api_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-extension-api - 2.16.24-SNAPSHOT + 2.16.24 jar Gerrit Code Review - Extension API API for Gerrit Extensions diff --git a/tools/maven/gerrit-plugin-api_pom.xml b/tools/maven/gerrit-plugin-api_pom.xml index e783e366a6..6168e2cdfa 100644 --- a/tools/maven/gerrit-plugin-api_pom.xml +++ b/tools/maven/gerrit-plugin-api_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-api - 2.16.24-SNAPSHOT + 2.16.24 jar Gerrit Code Review - Plugin API API for Gerrit Plugins diff --git a/tools/maven/gerrit-plugin-gwtui_pom.xml b/tools/maven/gerrit-plugin-gwtui_pom.xml index 61adfae4b5..911988ce83 100644 --- a/tools/maven/gerrit-plugin-gwtui_pom.xml +++ b/tools/maven/gerrit-plugin-gwtui_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-gwtui - 2.16.24-SNAPSHOT + 2.16.24 jar Gerrit Code Review - Plugin GWT UI Common Classes for Gerrit GWT UI Plugins diff --git a/tools/maven/gerrit-war_pom.xml b/tools/maven/gerrit-war_pom.xml index ff8f63f824..0689441403 100644 --- a/tools/maven/gerrit-war_pom.xml +++ b/tools/maven/gerrit-war_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-war - 2.16.24-SNAPSHOT + 2.16.24 war Gerrit Code Review - WAR Gerrit WAR diff --git a/version.bzl b/version.bzl index 63e650ea46..b4bb71e0f7 100644 --- a/version.bzl +++ b/version.bzl @@ -2,4 +2,4 @@ # Used by :api_install and :api_deploy targets # when talking to the destination repository. # -GERRIT_VERSION = "2.16.24-SNAPSHOT" +GERRIT_VERSION = "2.16.24" -- cgit v1.2.3 From ab96c734407ca60c2e70ee7208f6977be82e2bc6 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Thu, 12 Nov 2020 07:16:47 +0000 Subject: Set version to 2.16.25-SNAPSHOT Change-Id: Icc90a7b68e2764cbdb677c7a7f2261c7cf015e7c --- tools/maven/gerrit-acceptance-framework_pom.xml | 2 +- tools/maven/gerrit-extension-api_pom.xml | 2 +- tools/maven/gerrit-plugin-api_pom.xml | 2 +- tools/maven/gerrit-plugin-gwtui_pom.xml | 2 +- tools/maven/gerrit-war_pom.xml | 2 +- version.bzl | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tools/maven/gerrit-acceptance-framework_pom.xml b/tools/maven/gerrit-acceptance-framework_pom.xml index b3703af61a..36102189cc 100644 --- a/tools/maven/gerrit-acceptance-framework_pom.xml +++ b/tools/maven/gerrit-acceptance-framework_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-acceptance-framework - 2.16.24 + 2.16.25-SNAPSHOT jar Gerrit Code Review - Acceptance Test Framework Framework for Gerrit's acceptance tests diff --git a/tools/maven/gerrit-extension-api_pom.xml b/tools/maven/gerrit-extension-api_pom.xml index d330b12753..55102fd2d8 100644 --- a/tools/maven/gerrit-extension-api_pom.xml +++ b/tools/maven/gerrit-extension-api_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-extension-api - 2.16.24 + 2.16.25-SNAPSHOT jar Gerrit Code Review - Extension API API for Gerrit Extensions diff --git a/tools/maven/gerrit-plugin-api_pom.xml b/tools/maven/gerrit-plugin-api_pom.xml index 6168e2cdfa..87be647b5e 100644 --- a/tools/maven/gerrit-plugin-api_pom.xml +++ b/tools/maven/gerrit-plugin-api_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-api - 2.16.24 + 2.16.25-SNAPSHOT jar Gerrit Code Review - Plugin API API for Gerrit Plugins diff --git a/tools/maven/gerrit-plugin-gwtui_pom.xml b/tools/maven/gerrit-plugin-gwtui_pom.xml index 911988ce83..d5a8b17491 100644 --- a/tools/maven/gerrit-plugin-gwtui_pom.xml +++ b/tools/maven/gerrit-plugin-gwtui_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-gwtui - 2.16.24 + 2.16.25-SNAPSHOT jar Gerrit Code Review - Plugin GWT UI Common Classes for Gerrit GWT UI Plugins diff --git a/tools/maven/gerrit-war_pom.xml b/tools/maven/gerrit-war_pom.xml index 0689441403..50f1700cb5 100644 --- a/tools/maven/gerrit-war_pom.xml +++ b/tools/maven/gerrit-war_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-war - 2.16.24 + 2.16.25-SNAPSHOT war Gerrit Code Review - WAR Gerrit WAR diff --git a/version.bzl b/version.bzl index b4bb71e0f7..abc703d680 100644 --- a/version.bzl +++ b/version.bzl @@ -2,4 +2,4 @@ # Used by :api_install and :api_deploy targets # when talking to the destination repository. # -GERRIT_VERSION = "2.16.24" +GERRIT_VERSION = "2.16.25-SNAPSHOT" -- cgit v1.2.3 From 269ad8a2031106a3d7448eac0e4588be573c3731 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 23 Oct 2020 00:52:21 +0100 Subject: Remove generation for c.g.gwtexpui.* JavaDoc The JavaDoc for com.google.gwtexpui.* cannot be generated because the source files are not accessible anymore. Failing to generate the JavaDocs caused the Gerrit build to fail with 'No source files for package com.google.gwtexpui...'. Change-Id: Ie36e650962636813d8f9f615e495a980b7280420 --- gerrit-plugin-gwtui/BUILD | 4 ---- 1 file changed, 4 deletions(-) diff --git a/gerrit-plugin-gwtui/BUILD b/gerrit-plugin-gwtui/BUILD index 86f24bbc8f..1f75912f90 100644 --- a/gerrit-plugin-gwtui/BUILD +++ b/gerrit-plugin-gwtui/BUILD @@ -74,10 +74,6 @@ java_doc( ], pkgs = [ "com.google.gerrit.plugin", - "com.google.gwtexpui.clippy", - "com.google.gwtexpui.globalkey", - "com.google.gwtexpui.safehtml", - "com.google.gwtexpui.user", ], title = "Gerrit Review GWT Extension API Documentation", ) -- cgit v1.2.3 From e918bfd391a9743fdaee62e8a63c5f4373d34dc1 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Thu, 12 Nov 2020 15:41:52 +0000 Subject: Fetch JGit documentation from the archive site Change-Id: I8e78f5064fda7c2ff73134f6ac3d681c6be2e7d1 --- lib/jgit/jgit.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/jgit/jgit.bzl b/lib/jgit/jgit.bzl index 141b293b66..74e63cbd13 100644 --- a/lib/jgit/jgit.bzl +++ b/lib/jgit/jgit.bzl @@ -4,7 +4,7 @@ _JGIT_VERS = "4.11.9.201909030838-r" _DOC_VERS = _JGIT_VERS # Set to _JGIT_VERS unless using a snapshot -JGIT_DOC_URL = "https://download.eclipse.org/jgit/site/" + _DOC_VERS + "/apidocs" +JGIT_DOC_URL = "https://archive.eclipse.org/jgit/site/" + _DOC_VERS + "/apidocs" _JGIT_REPO = MAVEN_CENTRAL # Leave here even if set to MAVEN_CENTRAL. -- cgit v1.2.3 From 83ac8f020dcf6d4b77a19d595afe326c3b6a4c62 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Thu, 12 Nov 2020 15:27:48 +0000 Subject: Set version to 2.15.20 Change-Id: I83a8ece5ace5da608b3377461c572399b70962d0 --- gerrit-acceptance-framework/pom.xml | 2 +- gerrit-extension-api/pom.xml | 2 +- gerrit-plugin-api/pom.xml | 2 +- gerrit-plugin-gwtui/pom.xml | 2 +- gerrit-war/pom.xml | 2 +- version.bzl | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/gerrit-acceptance-framework/pom.xml b/gerrit-acceptance-framework/pom.xml index 82199d88e4..47cd746b2e 100644 --- a/gerrit-acceptance-framework/pom.xml +++ b/gerrit-acceptance-framework/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-acceptance-framework - 2.15.20-SNAPSHOT + 2.15.20 jar Gerrit Code Review - Acceptance Test Framework Framework for Gerrit's acceptance tests diff --git a/gerrit-extension-api/pom.xml b/gerrit-extension-api/pom.xml index fefe81d734..200bf12641 100644 --- a/gerrit-extension-api/pom.xml +++ b/gerrit-extension-api/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-extension-api - 2.15.20-SNAPSHOT + 2.15.20 jar Gerrit Code Review - Extension API API for Gerrit Extensions diff --git a/gerrit-plugin-api/pom.xml b/gerrit-plugin-api/pom.xml index 9c24d1163f..a1789a0c11 100644 --- a/gerrit-plugin-api/pom.xml +++ b/gerrit-plugin-api/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-api - 2.15.20-SNAPSHOT + 2.15.20 jar Gerrit Code Review - Plugin API API for Gerrit Plugins diff --git a/gerrit-plugin-gwtui/pom.xml b/gerrit-plugin-gwtui/pom.xml index ccca22a5de..b5d5b423de 100644 --- a/gerrit-plugin-gwtui/pom.xml +++ b/gerrit-plugin-gwtui/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-gwtui - 2.15.20-SNAPSHOT + 2.15.20 jar Gerrit Code Review - Plugin GWT UI Common Classes for Gerrit GWT UI Plugins diff --git a/gerrit-war/pom.xml b/gerrit-war/pom.xml index 568dddd158..f122916519 100644 --- a/gerrit-war/pom.xml +++ b/gerrit-war/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-war - 2.15.20-SNAPSHOT + 2.15.20 war Gerrit Code Review - WAR Gerrit WAR diff --git a/version.bzl b/version.bzl index 4b922458ff..b826272075 100644 --- a/version.bzl +++ b/version.bzl @@ -2,4 +2,4 @@ # Used by :api_install and :api_deploy targets # when talking to the destination repository. # -GERRIT_VERSION = "2.15.20-SNAPSHOT" +GERRIT_VERSION = "2.15.20" -- cgit v1.2.3 From e65ce8f8f738bceab5a2f3c827e576d6f446201c Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Thu, 12 Nov 2020 20:23:58 +0000 Subject: Set version to 2.15.21-SNAPSHOT Change-Id: I3f5c762fda9d47da21685ca12b0f6c80032a3be2 --- gerrit-acceptance-framework/pom.xml | 2 +- gerrit-extension-api/pom.xml | 2 +- gerrit-plugin-api/pom.xml | 2 +- gerrit-plugin-gwtui/pom.xml | 2 +- gerrit-war/pom.xml | 2 +- version.bzl | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/gerrit-acceptance-framework/pom.xml b/gerrit-acceptance-framework/pom.xml index 47cd746b2e..fefdc639d9 100644 --- a/gerrit-acceptance-framework/pom.xml +++ b/gerrit-acceptance-framework/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-acceptance-framework - 2.15.20 + 2.15.21-SNAPSHOT jar Gerrit Code Review - Acceptance Test Framework Framework for Gerrit's acceptance tests diff --git a/gerrit-extension-api/pom.xml b/gerrit-extension-api/pom.xml index 200bf12641..18cda24dc0 100644 --- a/gerrit-extension-api/pom.xml +++ b/gerrit-extension-api/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-extension-api - 2.15.20 + 2.15.21-SNAPSHOT jar Gerrit Code Review - Extension API API for Gerrit Extensions diff --git a/gerrit-plugin-api/pom.xml b/gerrit-plugin-api/pom.xml index a1789a0c11..cca83ced1d 100644 --- a/gerrit-plugin-api/pom.xml +++ b/gerrit-plugin-api/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-api - 2.15.20 + 2.15.21-SNAPSHOT jar Gerrit Code Review - Plugin API API for Gerrit Plugins diff --git a/gerrit-plugin-gwtui/pom.xml b/gerrit-plugin-gwtui/pom.xml index b5d5b423de..fb77806805 100644 --- a/gerrit-plugin-gwtui/pom.xml +++ b/gerrit-plugin-gwtui/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-gwtui - 2.15.20 + 2.15.21-SNAPSHOT jar Gerrit Code Review - Plugin GWT UI Common Classes for Gerrit GWT UI Plugins diff --git a/gerrit-war/pom.xml b/gerrit-war/pom.xml index f122916519..a1ae546e4c 100644 --- a/gerrit-war/pom.xml +++ b/gerrit-war/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-war - 2.15.20 + 2.15.21-SNAPSHOT war Gerrit Code Review - WAR Gerrit WAR diff --git a/version.bzl b/version.bzl index b826272075..658e58e6ba 100644 --- a/version.bzl +++ b/version.bzl @@ -2,4 +2,4 @@ # Used by :api_install and :api_deploy targets # when talking to the destination repository. # -GERRIT_VERSION = "2.15.20" +GERRIT_VERSION = "2.15.21-SNAPSHOT" -- cgit v1.2.3 From 1be1d6ff45f18c978fd21e5c7d437d0a1351d7d8 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 13 Nov 2020 18:44:29 +0000 Subject: Workaround Gitiles bug on All-Users visibility Gitiles has special FilteredRepository wrapper that allows to carefully hide refs based on the project's ACLs. There is however an optimisation that skips the filtering in case a user has READ permissions on every ACLs patterns. When the target repository is All-Users, the optimisation turns into a security issue because it allows seeing everything that belongs to everyone: - draft comments - PII of all users - external ids - draft edits Block Gitiles or any other part of Gerrit to abuse of this power when the target repository is All-Users, where nobody can be authorised to skip the ACLs evaluation. Cover the additional special case of the All-Users project access with two explicit positive and negative tests, so that the security check is covered. Bug: Issue 13621 Change-Id: Ia6ea1a9fd5473adff534204aea7d8f25324a45b7 (cherry picked from commit 45071d6977932bca5a1427c8abad24710fed2e33) --- .../gerrit/server/project/ProjectControl.java | 8 ++++- .../gerrit/server/project/RefControlTest.java | 38 +++++++++++++++++++++- 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java index a0b1f678b8..2dd960fe4d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java @@ -39,6 +39,7 @@ import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.account.GroupMembership; +import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.GitReceivePackGroups; import com.google.gerrit.server.config.GitUploadPackGroups; import com.google.gerrit.server.git.GitRepositoryManager; @@ -140,6 +141,7 @@ public class ProjectControl { private final RefVisibilityControl refVisibilityControl; private final VisibleRefFilter.Factory visibleRefFilterFactory; private final GitRepositoryManager gitRepositoryManager; + private final AllUsersName allUsersName; private List allSections; private Map refControls; @@ -156,6 +158,7 @@ public class ProjectControl { RefVisibilityControl refVisibilityControl, GitRepositoryManager gitRepositoryManager, VisibleRefFilter.Factory visibleRefFilterFactory, + AllUsersName allUsersName, @Assisted CurrentUser who, @Assisted ProjectState ps) { this.changeControlFactory = changeControlFactory; @@ -167,6 +170,7 @@ public class ProjectControl { this.refVisibilityControl = refVisibilityControl; this.gitRepositoryManager = gitRepositoryManager; this.visibleRefFilterFactory = visibleRefFilterFactory; + this.allUsersName = allUsersName; user = who; state = ps; } @@ -262,7 +266,9 @@ public class ProjectControl { } private boolean allRefsAreVisible(Set ignore) { - return user.isInternalUser() || canPerformOnAllRefs(Permission.READ, ignore); + return user.isInternalUser() + || (!getProject().getNameKey().equals(allUsersName) + && canPerformOnAllRefs(Permission.READ, ignore)); } /** Returns whether the project is hidden. */ diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java index dff5af07a9..364013fcba 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java @@ -61,6 +61,7 @@ import com.google.gerrit.server.git.TransferConfig; import com.google.gerrit.server.git.VisibleRefFilter; import com.google.gerrit.server.index.SingleVersionModule.SingleVersionListener; import com.google.gerrit.server.permissions.PermissionBackend; +import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.permissions.ProjectPermission; import com.google.gerrit.server.permissions.RefPermission; import com.google.gerrit.server.permissions.RefVisibilityControl; @@ -110,6 +111,16 @@ public class RefControlTest { assertThat(u.controlForRef(ref).isOwner()).named("NOT OWN " + ref).isFalse(); } + private void assertAllRefsAreVisible(ProjectControl u) throws PermissionBackendException { + assertThat(u.asForProject().test(ProjectPermission.READ)).named("all refs visible").isTrue(); + } + + private void assertAllRefsAreNotVisible(ProjectControl u) throws PermissionBackendException { + assertThat(u.asForProject().test(ProjectPermission.READ)) + .named("all refs NOT visible") + .isFalse(); + } + private void assertCanAccess(ProjectControl u) { boolean access = u.asForProject().testOrFalse(ProjectPermission.ACCESS); assertThat(access).named("can access").isTrue(); @@ -199,6 +210,7 @@ public class RefControlTest { private final Map all = new HashMap<>(); private Project.NameKey localKey = new Project.NameKey("local"); private ProjectConfig local; + private ProjectConfig allUsers; private Project.NameKey parentKey = new Project.NameKey("parent"); private ProjectConfig parent; private InMemoryRepositoryManager repoManager; @@ -230,7 +242,7 @@ public class RefControlTest { @Override public ProjectState getAllUsers() { - return null; + return get(allUsersName); } @Override @@ -290,6 +302,11 @@ public class RefControlTest { LabelType cr = Util.codeReview(); allProjects.getLabelSections().put(cr.getName(), cr); add(allProjects); + + Repository allUsersRepo = repoManager.createRepository(allUsersName); + allUsers = new ProjectConfig(new Project.NameKey(allUsersName.get())); + allUsers.load(allUsersRepo); + add(allUsers); } catch (IOException | ConfigInvalidException e) { throw new RuntimeException(e); } @@ -363,6 +380,24 @@ public class RefControlTest { assertAdminsAreOwnersAndDevsAreNot(); } + @Test + public void allRefsAreVisibleForRegularProject() throws Exception { + allow(local, READ, DEVS, "refs/*"); + allow(local, READ, DEVS, "refs/groups/*"); + allow(local, READ, DEVS, "refs/users/default"); + + assertAllRefsAreVisible(user(local, DEVS)); + } + + @Test + public void allRefsAreNotVisibleForAllUsers() throws Exception { + allow(allUsers, READ, DEVS, "refs/*"); + allow(allUsers, READ, DEVS, "refs/groups/*"); + allow(allUsers, READ, DEVS, "refs/users/default"); + + assertAllRefsAreNotVisible(user(allUsers, DEVS)); + } + @Test public void branchDelegation1() { allow(local, OWNER, ADMIN, "refs/*"); @@ -897,6 +932,7 @@ public class RefControlTest { refVisibilityControl, gitRepositoryManager, visibleRefFilterFactory, + allUsersName, new MockUser(name, memberOf), newProjectState(local)); } -- cgit v1.2.3 From 3dc150c8ecf7bb31948e1f8bc3b7c3776a3857a6 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 13 Nov 2020 00:12:38 +0000 Subject: Workaround Gitiles bug on All-Users visibility Gitiles has special FilteredRepository wrapper that allows to carefully hide refs based on the project's ACLs. There is however an optimisation that skips the filtering in case a user has READ permissions on every ACLs patterns. When the target repository is All-Users, the optimisation turns into a security issue because it allows seeing everything that belongs to everyone: - draft comments - PII of all users - external ids - draft edits Block Gitiles or any other part of Gerrit to abuse of this power when the target repository is All-Users, where nobody can be authorised to skip the ACLs evaluation. Cover the additional special case of the All-Users project access with two explicit positive and negative tests, so that the security check is covered. Bug: Issue 13621 Change-Id: Ia6ea1a9fd5473adff534204aea7d8f25324a45b7 (cherry picked from commit 45071d6977932bca5a1427c8abad24710fed2e33) --- .../gerrit/server/permissions/ProjectControl.java | 8 ++++- .../gerrit/server/permissions/RefControlTest.java | 39 ++++++++++++++++++++-- 2 files changed, 43 insertions(+), 4 deletions(-) diff --git a/java/com/google/gerrit/server/permissions/ProjectControl.java b/java/com/google/gerrit/server/permissions/ProjectControl.java index c3d0719283..85e7fb16b4 100644 --- a/java/com/google/gerrit/server/permissions/ProjectControl.java +++ b/java/com/google/gerrit/server/permissions/ProjectControl.java @@ -33,6 +33,7 @@ import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.account.GroupMembership; +import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.GitReceivePackGroups; import com.google.gerrit.server.config.GitUploadPackGroups; import com.google.gerrit.server.git.GitRepositoryManager; @@ -75,6 +76,7 @@ class ProjectControl { private final ChangeControl.Factory changeControlFactory; private final PermissionCollection.Factory permissionFilter; private final DefaultRefFilter.Factory refFilterFactory; + private final AllUsersName allUsersName; private List allSections; private Map refControls; @@ -90,6 +92,7 @@ class ProjectControl { RefVisibilityControl refVisibilityControl, GitRepositoryManager repositoryManager, DefaultRefFilter.Factory refFilterFactory, + AllUsersName allUsersName, @Assisted CurrentUser who, @Assisted ProjectState ps) { this.changeControlFactory = changeControlFactory; @@ -100,6 +103,7 @@ class ProjectControl { this.refVisibilityControl = refVisibilityControl; this.repositoryManager = repositoryManager; this.refFilterFactory = refFilterFactory; + this.allUsersName = allUsersName; user = who; state = ps; } @@ -175,7 +179,9 @@ class ProjectControl { } boolean allRefsAreVisible(Set ignore) { - return user.isInternalUser() || canPerformOnAllRefs(Permission.READ, ignore); + return user.isInternalUser() + || (!getProject().getNameKey().equals(allUsersName) + && canPerformOnAllRefs(Permission.READ, ignore)); } /** Can the user run upload pack? */ diff --git a/javatests/com/google/gerrit/server/permissions/RefControlTest.java b/javatests/com/google/gerrit/server/permissions/RefControlTest.java index bf8ea86f02..0f0f1c3ec1 100644 --- a/javatests/com/google/gerrit/server/permissions/RefControlTest.java +++ b/javatests/com/google/gerrit/server/permissions/RefControlTest.java @@ -106,6 +106,14 @@ public class RefControlTest { assertThat(u.isOwner()).named("not owner").isFalse(); } + private void assertAllRefsAreVisible(ProjectControl u) { + assertThat(u.allRefsAreVisible(Collections.emptySet())).named("all refs visible").isTrue(); + } + + private void assertAllRefsAreNotVisible(ProjectControl u) { + assertThat(u.allRefsAreVisible(Collections.emptySet())).named("all refs NOT visible").isFalse(); + } + private void assertNotOwner(String ref, ProjectControl u) { assertThat(u.controlForRef(ref).isOwner()).named("NOT OWN " + ref).isFalse(); } @@ -195,6 +203,7 @@ public class RefControlTest { private final Map all = new HashMap<>(); private Project.NameKey localKey = new Project.NameKey("local"); private ProjectConfig local; + private ProjectConfig allUsers; private Project.NameKey parentKey = new Project.NameKey("parent"); private ProjectConfig parent; private InMemoryRepositoryManager repoManager; @@ -226,7 +235,7 @@ public class RefControlTest { @Override public ProjectState getAllUsers() { - return null; + return get(allUsersName); } @Override @@ -280,12 +289,17 @@ public class RefControlTest { injector.injectMembers(this); try { - Repository repo = repoManager.createRepository(allProjectsName); + Repository allProjectsRepo = repoManager.createRepository(allProjectsName); ProjectConfig allProjects = new ProjectConfig(new Project.NameKey(allProjectsName.get())); - allProjects.load(repo); + allProjects.load(allProjectsRepo); LabelType cr = Util.codeReview(); allProjects.getLabelSections().put(cr.getName(), cr); add(allProjects); + + Repository allUsersRepo = repoManager.createRepository(allUsersName); + allUsers = new ProjectConfig(new Project.NameKey(allUsersName.get())); + allUsers.load(allUsersRepo); + add(allUsers); } catch (IOException | ConfigInvalidException e) { throw new RuntimeException(e); } @@ -359,6 +373,24 @@ public class RefControlTest { assertAdminsAreOwnersAndDevsAreNot(); } + @Test + public void allRefsAreVisibleForRegularProject() throws Exception { + allow(local, READ, DEVS, "refs/*"); + allow(local, READ, DEVS, "refs/groups/*"); + allow(local, READ, DEVS, "refs/users/default"); + + assertAllRefsAreVisible(user(local, DEVS)); + } + + @Test + public void allRefsAreNotVisibleForAllUsers() throws Exception { + allow(allUsers, READ, DEVS, "refs/*"); + allow(allUsers, READ, DEVS, "refs/groups/*"); + allow(allUsers, READ, DEVS, "refs/users/default"); + + assertAllRefsAreNotVisible(user(allUsers, DEVS)); + } + @Test public void branchDelegation1() throws Exception { allow(local, OWNER, ADMIN, "refs/*"); @@ -1022,6 +1054,7 @@ public class RefControlTest { refVisibilityControl, repoManager, refFilterFactory, + allUsersName, new MockUser(name, memberOf), newProjectState(local)); } -- cgit v1.2.3 From 1786100b0ed413270c2d2102afd2892b3e294de1 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Mon, 16 Nov 2020 13:57:23 +0000 Subject: Set version to 2.16.25 Change-Id: I67be710b6fda2069e798964ec81ad9add637bab5 --- tools/maven/gerrit-acceptance-framework_pom.xml | 2 +- tools/maven/gerrit-extension-api_pom.xml | 2 +- tools/maven/gerrit-plugin-api_pom.xml | 2 +- tools/maven/gerrit-plugin-gwtui_pom.xml | 2 +- tools/maven/gerrit-war_pom.xml | 2 +- version.bzl | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tools/maven/gerrit-acceptance-framework_pom.xml b/tools/maven/gerrit-acceptance-framework_pom.xml index 36102189cc..5cdf55359a 100644 --- a/tools/maven/gerrit-acceptance-framework_pom.xml +++ b/tools/maven/gerrit-acceptance-framework_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-acceptance-framework - 2.16.25-SNAPSHOT + 2.16.25 jar Gerrit Code Review - Acceptance Test Framework Framework for Gerrit's acceptance tests diff --git a/tools/maven/gerrit-extension-api_pom.xml b/tools/maven/gerrit-extension-api_pom.xml index 55102fd2d8..09bfb19c3e 100644 --- a/tools/maven/gerrit-extension-api_pom.xml +++ b/tools/maven/gerrit-extension-api_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-extension-api - 2.16.25-SNAPSHOT + 2.16.25 jar Gerrit Code Review - Extension API API for Gerrit Extensions diff --git a/tools/maven/gerrit-plugin-api_pom.xml b/tools/maven/gerrit-plugin-api_pom.xml index 87be647b5e..dbc2edeea2 100644 --- a/tools/maven/gerrit-plugin-api_pom.xml +++ b/tools/maven/gerrit-plugin-api_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-api - 2.16.25-SNAPSHOT + 2.16.25 jar Gerrit Code Review - Plugin API API for Gerrit Plugins diff --git a/tools/maven/gerrit-plugin-gwtui_pom.xml b/tools/maven/gerrit-plugin-gwtui_pom.xml index d5a8b17491..05e4a446ad 100644 --- a/tools/maven/gerrit-plugin-gwtui_pom.xml +++ b/tools/maven/gerrit-plugin-gwtui_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-gwtui - 2.16.25-SNAPSHOT + 2.16.25 jar Gerrit Code Review - Plugin GWT UI Common Classes for Gerrit GWT UI Plugins diff --git a/tools/maven/gerrit-war_pom.xml b/tools/maven/gerrit-war_pom.xml index 50f1700cb5..a716671cf0 100644 --- a/tools/maven/gerrit-war_pom.xml +++ b/tools/maven/gerrit-war_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-war - 2.16.25-SNAPSHOT + 2.16.25 war Gerrit Code Review - WAR Gerrit WAR diff --git a/version.bzl b/version.bzl index abc703d680..496b48bcb8 100644 --- a/version.bzl +++ b/version.bzl @@ -2,4 +2,4 @@ # Used by :api_install and :api_deploy targets # when talking to the destination repository. # -GERRIT_VERSION = "2.16.25-SNAPSHOT" +GERRIT_VERSION = "2.16.25" -- cgit v1.2.3 From db8c3be3ff84dd93c38083c6018e5bbd10a2a2a1 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Mon, 16 Nov 2020 14:19:53 +0000 Subject: Set version to 2.16.26-SNAPSHOT Change-Id: Icc689699eff3eb06a6b10e8221feab87e38b11e0 --- tools/maven/gerrit-acceptance-framework_pom.xml | 2 +- tools/maven/gerrit-extension-api_pom.xml | 2 +- tools/maven/gerrit-plugin-api_pom.xml | 2 +- tools/maven/gerrit-plugin-gwtui_pom.xml | 2 +- tools/maven/gerrit-war_pom.xml | 2 +- version.bzl | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tools/maven/gerrit-acceptance-framework_pom.xml b/tools/maven/gerrit-acceptance-framework_pom.xml index 5cdf55359a..8c0d0d1d44 100644 --- a/tools/maven/gerrit-acceptance-framework_pom.xml +++ b/tools/maven/gerrit-acceptance-framework_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-acceptance-framework - 2.16.25 + 2.16.26-SNAPSHOT jar Gerrit Code Review - Acceptance Test Framework Framework for Gerrit's acceptance tests diff --git a/tools/maven/gerrit-extension-api_pom.xml b/tools/maven/gerrit-extension-api_pom.xml index 09bfb19c3e..9c82d72b42 100644 --- a/tools/maven/gerrit-extension-api_pom.xml +++ b/tools/maven/gerrit-extension-api_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-extension-api - 2.16.25 + 2.16.26-SNAPSHOT jar Gerrit Code Review - Extension API API for Gerrit Extensions diff --git a/tools/maven/gerrit-plugin-api_pom.xml b/tools/maven/gerrit-plugin-api_pom.xml index dbc2edeea2..1343db2a98 100644 --- a/tools/maven/gerrit-plugin-api_pom.xml +++ b/tools/maven/gerrit-plugin-api_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-api - 2.16.25 + 2.16.26-SNAPSHOT jar Gerrit Code Review - Plugin API API for Gerrit Plugins diff --git a/tools/maven/gerrit-plugin-gwtui_pom.xml b/tools/maven/gerrit-plugin-gwtui_pom.xml index 05e4a446ad..a86de7260f 100644 --- a/tools/maven/gerrit-plugin-gwtui_pom.xml +++ b/tools/maven/gerrit-plugin-gwtui_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-gwtui - 2.16.25 + 2.16.26-SNAPSHOT jar Gerrit Code Review - Plugin GWT UI Common Classes for Gerrit GWT UI Plugins diff --git a/tools/maven/gerrit-war_pom.xml b/tools/maven/gerrit-war_pom.xml index a716671cf0..333dd05bf0 100644 --- a/tools/maven/gerrit-war_pom.xml +++ b/tools/maven/gerrit-war_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-war - 2.16.25 + 2.16.26-SNAPSHOT war Gerrit Code Review - WAR Gerrit WAR diff --git a/version.bzl b/version.bzl index 496b48bcb8..3a35896dbd 100644 --- a/version.bzl +++ b/version.bzl @@ -2,4 +2,4 @@ # Used by :api_install and :api_deploy targets # when talking to the destination repository. # -GERRIT_VERSION = "2.16.25" +GERRIT_VERSION = "2.16.26-SNAPSHOT" -- cgit v1.2.3 From 60f10748a8cc893cf437f41ac3b1c4aada13cc13 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Mon, 16 Nov 2020 14:51:47 +0000 Subject: Set version to 2.15.21 Change-Id: I3e3eb891d717169f912a20e7de948cea1f47fab3 --- gerrit-acceptance-framework/pom.xml | 2 +- gerrit-extension-api/pom.xml | 2 +- gerrit-plugin-api/pom.xml | 2 +- gerrit-plugin-gwtui/pom.xml | 2 +- gerrit-war/pom.xml | 2 +- version.bzl | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/gerrit-acceptance-framework/pom.xml b/gerrit-acceptance-framework/pom.xml index fefdc639d9..76154ae791 100644 --- a/gerrit-acceptance-framework/pom.xml +++ b/gerrit-acceptance-framework/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-acceptance-framework - 2.15.21-SNAPSHOT + 2.15.21 jar Gerrit Code Review - Acceptance Test Framework Framework for Gerrit's acceptance tests diff --git a/gerrit-extension-api/pom.xml b/gerrit-extension-api/pom.xml index 18cda24dc0..0eda0b17ab 100644 --- a/gerrit-extension-api/pom.xml +++ b/gerrit-extension-api/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-extension-api - 2.15.21-SNAPSHOT + 2.15.21 jar Gerrit Code Review - Extension API API for Gerrit Extensions diff --git a/gerrit-plugin-api/pom.xml b/gerrit-plugin-api/pom.xml index cca83ced1d..65a922d9c5 100644 --- a/gerrit-plugin-api/pom.xml +++ b/gerrit-plugin-api/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-api - 2.15.21-SNAPSHOT + 2.15.21 jar Gerrit Code Review - Plugin API API for Gerrit Plugins diff --git a/gerrit-plugin-gwtui/pom.xml b/gerrit-plugin-gwtui/pom.xml index fb77806805..db5162bb51 100644 --- a/gerrit-plugin-gwtui/pom.xml +++ b/gerrit-plugin-gwtui/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-gwtui - 2.15.21-SNAPSHOT + 2.15.21 jar Gerrit Code Review - Plugin GWT UI Common Classes for Gerrit GWT UI Plugins diff --git a/gerrit-war/pom.xml b/gerrit-war/pom.xml index a1ae546e4c..17bdfbbbf2 100644 --- a/gerrit-war/pom.xml +++ b/gerrit-war/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-war - 2.15.21-SNAPSHOT + 2.15.21 war Gerrit Code Review - WAR Gerrit WAR diff --git a/version.bzl b/version.bzl index 658e58e6ba..9d7a33f420 100644 --- a/version.bzl +++ b/version.bzl @@ -2,4 +2,4 @@ # Used by :api_install and :api_deploy targets # when talking to the destination repository. # -GERRIT_VERSION = "2.15.21-SNAPSHOT" +GERRIT_VERSION = "2.15.21" -- cgit v1.2.3 From b3b98a35c4a60a81d6fdb0144902ecb2763af63c Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Mon, 16 Nov 2020 15:13:08 +0000 Subject: Set version to 2.15.22-SNAPSHOT Change-Id: I1ed863213d9946b77ae558d52094731db10ff721 --- gerrit-acceptance-framework/pom.xml | 2 +- gerrit-extension-api/pom.xml | 2 +- gerrit-plugin-api/pom.xml | 2 +- gerrit-plugin-gwtui/pom.xml | 2 +- gerrit-war/pom.xml | 2 +- version.bzl | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/gerrit-acceptance-framework/pom.xml b/gerrit-acceptance-framework/pom.xml index 76154ae791..34dd6a3a1f 100644 --- a/gerrit-acceptance-framework/pom.xml +++ b/gerrit-acceptance-framework/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-acceptance-framework - 2.15.21 + 2.15.22-SNAPSHOT jar Gerrit Code Review - Acceptance Test Framework Framework for Gerrit's acceptance tests diff --git a/gerrit-extension-api/pom.xml b/gerrit-extension-api/pom.xml index 0eda0b17ab..dabc892678 100644 --- a/gerrit-extension-api/pom.xml +++ b/gerrit-extension-api/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-extension-api - 2.15.21 + 2.15.22-SNAPSHOT jar Gerrit Code Review - Extension API API for Gerrit Extensions diff --git a/gerrit-plugin-api/pom.xml b/gerrit-plugin-api/pom.xml index 65a922d9c5..701c788de4 100644 --- a/gerrit-plugin-api/pom.xml +++ b/gerrit-plugin-api/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-api - 2.15.21 + 2.15.22-SNAPSHOT jar Gerrit Code Review - Plugin API API for Gerrit Plugins diff --git a/gerrit-plugin-gwtui/pom.xml b/gerrit-plugin-gwtui/pom.xml index db5162bb51..addc365519 100644 --- a/gerrit-plugin-gwtui/pom.xml +++ b/gerrit-plugin-gwtui/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-gwtui - 2.15.21 + 2.15.22-SNAPSHOT jar Gerrit Code Review - Plugin GWT UI Common Classes for Gerrit GWT UI Plugins diff --git a/gerrit-war/pom.xml b/gerrit-war/pom.xml index 17bdfbbbf2..7fcdb2d774 100644 --- a/gerrit-war/pom.xml +++ b/gerrit-war/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-war - 2.15.21 + 2.15.22-SNAPSHOT war Gerrit Code Review - WAR Gerrit WAR diff --git a/version.bzl b/version.bzl index 9d7a33f420..b5879e4b80 100644 --- a/version.bzl +++ b/version.bzl @@ -2,4 +2,4 @@ # Used by :api_install and :api_deploy targets # when talking to the destination repository. # -GERRIT_VERSION = "2.15.21" +GERRIT_VERSION = "2.15.22-SNAPSHOT" -- cgit v1.2.3 From c8d81c4ab6784d9ccd9bceb5189d89c0a892eef2 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Wed, 11 Nov 2020 21:11:54 +0000 Subject: Validate Gerrit changes on stable-2.15 with Jenkins Change-Id: I35c47ba60c08e8d5d1f767672b5e83b7d29fea1b (cherry picked from commit 1346eab23259f8dc4adec9cb098e2f818c9cf79d) --- Jenkinsfile | 1 + 1 file changed, 1 insertion(+) create mode 100644 Jenkinsfile diff --git a/Jenkinsfile b/Jenkinsfile new file mode 100644 index 0000000000..565dd45586 --- /dev/null +++ b/Jenkinsfile @@ -0,0 +1 @@ +gerritPipeline() -- cgit v1.2.3 From 10fd930aed1f457202c220a70194de83e1942971 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 13 Nov 2020 18:44:29 +0000 Subject: Workaround Gitiles bug on All-Users visibility Gitiles has special FilteredRepository wrapper that allows to carefully hide refs based on the project's ACLs. There is however an optimisation that skips the filtering in case a user has READ permissions on every ACLs patterns. When the target repository is All-Users, the optimisation turns into a security issue because it allows seeing everything that belongs to everyone: - draft comments - PII of all users - external ids - draft edits Block Gitiles or any other part of Gerrit to abuse of this power when the target repository is All-Users, where nobody can be authorised to skip the ACLs evaluation. Cover the additional special case of the All-Users project access with two explicit positive and negative tests, so that the security check is covered. Bug: Issue 13621 Change-Id: Ia6ea1a9fd5473adff534204aea7d8f25324a45b7 (cherry picked from commit 45071d6977932bca5a1427c8abad24710fed2e33) (cherry picked from commit 1be1d6ff45f18c978fd21e5c7d437d0a1351d7d8) --- .../gerrit/server/project/ProjectControl.java | 8 ++++- .../gerrit/server/project/RefControlTest.java | 35 +++++++++++++++++++++- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java index fefc84d73d..1b035b95b8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java @@ -39,6 +39,7 @@ import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.GroupMembership; import com.google.gerrit.server.change.IncludedInResolver; +import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.CanonicalWebUrl; import com.google.gerrit.server.config.GitReceivePackGroups; import com.google.gerrit.server.config.GitUploadPackGroups; @@ -171,6 +172,7 @@ public class ProjectControl { @Nullable private final SearchingChangeCacheImpl changeCache; private final Provider queryProvider; private final Metrics metrics; + private final AllUsersName allUsersName; private List allSections; private List localSections; @@ -190,6 +192,7 @@ public class ProjectControl { Provider queryProvider, @Nullable SearchingChangeCacheImpl changeCache, @CanonicalWebUrl @Nullable String canonicalWebUrl, + AllUsersName allUsersName, @Assisted CurrentUser who, @Assisted ProjectState ps, Metrics metrics) { @@ -204,6 +207,7 @@ public class ProjectControl { this.canonicalWebUrl = canonicalWebUrl; this.queryProvider = queryProvider; this.metrics = metrics; + this.allUsersName = allUsersName; user = who; state = ps; } @@ -318,7 +322,9 @@ public class ProjectControl { } public boolean allRefsAreVisible(Set ignore) { - return user.isInternalUser() || canPerformOnAllRefs(Permission.READ, ignore); + return user.isInternalUser() + || (!getProject().getNameKey().equals(allUsersName) + && canPerformOnAllRefs(Permission.READ, ignore)); } /** Is this user a project owner? Ownership does not imply {@link #isVisible()} */ diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java index 4f2284cb40..0c3d4c28e9 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java @@ -114,6 +114,14 @@ public class RefControlTest { assertThat(u.isVisible()).named("can read").isTrue(); } + private void assertAllRefsAreVisible(ProjectControl u) { + assertThat(u.allRefsAreVisible()).named("all refs visible").isTrue(); + } + + private void assertAllRefsAreNotVisible(ProjectControl u) { + assertThat(u.allRefsAreVisible()).named("all refs NOT visible").isFalse(); + } + private void assertCannotRead(ProjectControl u) { assertThat(u.isVisible()).named("cannot read").isFalse(); } @@ -189,6 +197,7 @@ public class RefControlTest { private final Map all = new HashMap<>(); private Project.NameKey localKey = new Project.NameKey("local"); private ProjectConfig local; + private ProjectConfig allUsers; private Project.NameKey parentKey = new Project.NameKey("parent"); private ProjectConfig parent; private InMemoryRepositoryManager repoManager; @@ -219,7 +228,7 @@ public class RefControlTest { @Override public ProjectState getAllUsers() { - return null; + return get(allUsersName); } @Override @@ -273,6 +282,11 @@ public class RefControlTest { LabelType cr = Util.codeReview(); allProjects.getLabelSections().put(cr.getName(), cr); add(allProjects); + + Repository allUsersRepo = repoManager.createRepository(allUsersName); + allUsers = new ProjectConfig(new Project.NameKey(allUsersName.get())); + allUsers.load(allUsersRepo); + add(allUsers); } catch (IOException | ConfigInvalidException e) { throw new RuntimeException(e); } @@ -346,6 +360,24 @@ public class RefControlTest { assertAdminsAreOwnersAndDevsAreNot(); } + @Test + public void allRefsAreVisibleForRegularProject() throws Exception { + allow(local, READ, DEVS, "refs/*"); + allow(local, READ, DEVS, "refs/groups/*"); + allow(local, READ, DEVS, "refs/users/default"); + + assertAllRefsAreVisible(user(local, DEVS)); + } + + @Test + public void allRefsAreNotVisibleForAllUsers() throws Exception { + allow(allUsers, READ, DEVS, "refs/*"); + allow(allUsers, READ, DEVS, "refs/groups/*"); + allow(allUsers, READ, DEVS, "refs/users/default"); + + assertAllRefsAreNotVisible(user(allUsers, DEVS)); + } + @Test public void branchDelegation1() { allow(local, OWNER, ADMIN, "refs/*"); @@ -908,6 +940,7 @@ public class RefControlTest { queryProvider, null, canonicalWebUrl, + allUsersName, new MockUser(name, memberOf), newProjectState(local), metrics); -- cgit v1.2.3 From a57f872df00f0f2d5f6484a11473de0d2a7297bc Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Wed, 18 Nov 2020 21:48:06 +0000 Subject: Set version to 2.14.22 Change-Id: Id3c767d04411ac7551e7016a37136a77e4ae8118 --- gerrit-acceptance-framework/pom.xml | 2 +- gerrit-extension-api/pom.xml | 2 +- gerrit-plugin-api/pom.xml | 2 +- gerrit-plugin-gwtui/pom.xml | 2 +- gerrit-war/pom.xml | 2 +- version.bzl | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/gerrit-acceptance-framework/pom.xml b/gerrit-acceptance-framework/pom.xml index 9e5a073e58..a286dca67d 100644 --- a/gerrit-acceptance-framework/pom.xml +++ b/gerrit-acceptance-framework/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-acceptance-framework - 2.14.22-SNAPSHOT + 2.14.22 jar Gerrit Code Review - Acceptance Test Framework Framework for Gerrit's acceptance tests diff --git a/gerrit-extension-api/pom.xml b/gerrit-extension-api/pom.xml index eb41098678..3b58b41af7 100644 --- a/gerrit-extension-api/pom.xml +++ b/gerrit-extension-api/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-extension-api - 2.14.22-SNAPSHOT + 2.14.22 jar Gerrit Code Review - Extension API API for Gerrit Extensions diff --git a/gerrit-plugin-api/pom.xml b/gerrit-plugin-api/pom.xml index b1f16a7fac..84a0e531c4 100644 --- a/gerrit-plugin-api/pom.xml +++ b/gerrit-plugin-api/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-api - 2.14.22-SNAPSHOT + 2.14.22 jar Gerrit Code Review - Plugin API API for Gerrit Plugins diff --git a/gerrit-plugin-gwtui/pom.xml b/gerrit-plugin-gwtui/pom.xml index 544741e219..ac92ed2a25 100644 --- a/gerrit-plugin-gwtui/pom.xml +++ b/gerrit-plugin-gwtui/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-gwtui - 2.14.22-SNAPSHOT + 2.14.22 jar Gerrit Code Review - Plugin GWT UI Common Classes for Gerrit GWT UI Plugins diff --git a/gerrit-war/pom.xml b/gerrit-war/pom.xml index 98f9012ecb..42caa6fb6a 100644 --- a/gerrit-war/pom.xml +++ b/gerrit-war/pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-war - 2.14.22-SNAPSHOT + 2.14.22 war Gerrit Code Review - WAR Gerrit WAR diff --git a/version.bzl b/version.bzl index 530373867a..1326191b1d 100644 --- a/version.bzl +++ b/version.bzl @@ -2,4 +2,4 @@ # Used by :api_install and :api_deploy targets # when talking to the destination repository. # -GERRIT_VERSION = "2.14.22-SNAPSHOT" +GERRIT_VERSION = "2.14.22" -- cgit v1.2.3 From eeebe41ee8fd3d22befc0c399c3f8bd119993b04 Mon Sep 17 00:00:00 2001 From: Antoine Musso Date: Thu, 12 Nov 2020 23:14:46 +0100 Subject: Disk cache metrics require cache.enableDiskStatMetrics After setting up the metrics-reporter-prometheus, I have missed entries for caches/disk_cached and caches/disk_hit_ratio. Turns out they are disabled by default via cache.enableDiskStatMetrics. The feature flag comes from I41ee2d9a368c312b7b2729d17d6c19bee0d90922 which has been backported to all stable branches. Add to the metrics documentation a reference to enableDiskStatMetrics setting. Change-Id: I3620e0cb68b992f094a1b8d7b0016fc834a8e7e6 --- Documentation/metrics.txt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/Documentation/metrics.txt b/Documentation/metrics.txt index cb3eb2179d..1666b2fd0b 100644 --- a/Documentation/metrics.txt +++ b/Documentation/metrics.txt @@ -61,6 +61,11 @@ objects needing finalization. * `caches/disk_cached`: Disk entries used by persistent cache. * `caches/disk_hit_ratio`: Disk hit ratio for persistent cache. +Cache disk metrics are expensive to compute on larger installations and are not +computed by default. They can be enabled via the +link:config.gerrit.html#cache.enableDiskStatMetrics[`cache.enableDiskStatMetrics`] +setting. + === HTTP ==== Jetty -- cgit v1.2.3