From 707c75541a4f4a57bb90734e2051983feb2703a4 Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Fri, 6 Mar 2020 16:38:30 +0100 Subject: [PATCH 1/2] Obsolete PermissionBackend#indexedChange When refactoring Gerrit's code to move all callers to use PermissionBackend for all permission checks, we introduced indexedChange as a way to contruct a PermissionBackend from a change that we recovered from the index (i.e. has no ChangeNotes). This commit refactors the logic in PermissionBackend and as an end result, we obsolete #indexedChange. If a change was contructed from the index, callers have ChangeData and can just pass this into PermissionBackend. Callers who got the entity from the primary storage can pass ChangeNotes instead. This is an effort to make the DETAILED_LABELS ListChangesOption not require lazyload as well as a general refactoring of the code. Change-Id: I3dfd8f91dbaf54bb472b6c922d798365fe539674 --- .../gerrit/server/change/ChangeJson.java | 17 ++--- .../gerrit/server/change/LabelsJson.java | 16 +---- .../gerrit/server/change/RevisionJson.java | 21 +----- .../server/git/SearchingChangeCacheImpl.java | 2 +- .../server/permissions/ChangeControl.java | 50 +++---------- .../DefaultPermissionBackendModule.java | 1 - .../server/permissions/DefaultRefFilter.java | 3 +- .../permissions/FailedPermissionBackend.java | 5 -- .../server/permissions/PermissionBackend.java | 24 ------- .../server/permissions/ProjectControl.java | 17 ++--- .../gerrit/server/permissions/RefControl.java | 19 ++--- .../server/query/change/ChangeData.java | 8 +-- .../change/ChangeIsVisibleToPredicate.java | 3 +- .../permissions/PermissionBackendIT.java | 70 +++++++++++++++++++ .../gerrit/acceptance/ssh/SetReviewersIT.java | 2 +- 15 files changed, 109 insertions(+), 149 deletions(-) create mode 100644 javatests/com/google/gerrit/acceptance/server/permissions/PermissionBackendIT.java diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java index b464104f82..e2beef63a4 100644 --- a/java/com/google/gerrit/server/change/ChangeJson.java +++ b/java/com/google/gerrit/server/change/ChangeJson.java @@ -722,7 +722,10 @@ public class ChangeJson { // testRemoveReviewer check for a specific reviewer in the loop saving potentially many // permission checks. boolean canRemoveAnyReviewer = - permissionBackendForChange(userProvider.get(), cd).test(ChangePermission.REMOVE_REVIEWER); + permissionBackend + .user(userProvider.get()) + .change(cd) + .test(ChangePermission.REMOVE_REVIEWER); for (LabelInfo label : labels) { if (label.all == null) { continue; @@ -817,16 +820,4 @@ public class ChangeJson { } return map; } - - /** - * @return {@link com.google.gerrit.server.permissions.PermissionBackend.ForChange} constructed - * from either an index-backed or a database-backed {@link ChangeData} depending on {@code - * lazyload}. - */ - private PermissionBackend.ForChange permissionBackendForChange(CurrentUser user, ChangeData cd) { - PermissionBackend.WithUser withUser = permissionBackend.user(user); - return lazyLoad - ? withUser.change(cd) - : withUser.indexedChange(cd, notesFactory.createFromIndexedChange(cd.change())); - } } diff --git a/java/com/google/gerrit/server/change/LabelsJson.java b/java/com/google/gerrit/server/change/LabelsJson.java index c6f49696ca..2db17d6700 100644 --- a/java/com/google/gerrit/server/change/LabelsJson.java +++ b/java/com/google/gerrit/server/change/LabelsJson.java @@ -131,7 +131,7 @@ public class LabelsJson { Map labels = null; Set can = - permissionBackendForChange(filterApprovalsBy, cd).testLabels(toCheck.values()); + permissionBackend.absentUser(filterApprovalsBy).change(cd).testLabels(toCheck.values()); SetMultimap permitted = LinkedHashMultimap.create(); for (SubmitRecord rec : submitRecords(cd)) { if (rec.labels == null) { @@ -452,7 +452,7 @@ public class LabelsJson { LabelTypes labelTypes = cd.getLabelTypes(); for (Account.Id accountId : allUsers) { - PermissionBackend.ForChange perm = permissionBackendForChange(accountId, cd); + PermissionBackend.ForChange perm = permissionBackend.absentUser(accountId).change(cd); Map pvr = getPermittedVotingRanges(permittedLabels(accountId, cd)); for (Map.Entry e : labels.entrySet()) { LabelType lt = labelTypes.byLabel(e.getKey()); @@ -492,18 +492,6 @@ public class LabelsJson { } } - /** - * @return {@link com.google.gerrit.server.permissions.PermissionBackend.ForChange} constructed - * from either an index-backed or a database-backed {@link ChangeData} depending on {@code - * lazyload}. - */ - private PermissionBackend.ForChange permissionBackendForChange(Account.Id user, ChangeData cd) { - PermissionBackend.WithUser withUser = permissionBackend.absentUser(user); - return lazyLoad - ? withUser.change(cd) - : withUser.indexedChange(cd, notesFactory.createFromIndexedChange(cd.change())); - } - private List submitRecords(ChangeData cd) { return cd.submitRecords(ChangeJson.SUBMIT_RULE_OPTIONS_LENIENT); } diff --git a/java/com/google/gerrit/server/change/RevisionJson.java b/java/com/google/gerrit/server/change/RevisionJson.java index 001a532116..788a7ad64f 100644 --- a/java/com/google/gerrit/server/change/RevisionJson.java +++ b/java/com/google/gerrit/server/change/RevisionJson.java @@ -60,7 +60,6 @@ import com.google.gerrit.server.account.AccountLoader; import com.google.gerrit.server.account.GpgApiAdapter; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.MergeUtil; -import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.permissions.ChangePermission; import com.google.gerrit.server.permissions.PermissionBackend; @@ -107,8 +106,6 @@ public class RevisionJson { private final AnonymousUser anonymous; private final GitRepositoryManager repoManager; private final PermissionBackend permissionBackend; - private final ChangeNotes.Factory notesFactory; - private final boolean lazyLoad; @Inject RevisionJson( @@ -128,7 +125,6 @@ public class RevisionJson { ChangeKindCache changeKindCache, GitRepositoryManager repoManager, PermissionBackend permissionBackend, - ChangeNotes.Factory notesFactory, @Assisted Iterable options) { this.userProvider = userProvider; this.anonymous = anonymous; @@ -145,10 +141,8 @@ public class RevisionJson { this.changeResourceFactory = changeResourceFactory; this.changeKindCache = changeKindCache; this.permissionBackend = permissionBackend; - this.notesFactory = notesFactory; this.repoManager = repoManager; this.options = ImmutableSet.copyOf(options); - this.lazyLoad = containsAnyOf(this.options, ChangeJson.REQUIRE_LAZY_LOAD); } /** @@ -346,22 +340,9 @@ public class RevisionJson { return options.contains(option); } - /** - * @return {@link com.google.gerrit.server.permissions.PermissionBackend.ForChange} constructed - * from either an index-backed or a database-backed {@link ChangeData} depending on {@code - * lazyload}. - */ - private PermissionBackend.ForChange permissionBackendForChange( - PermissionBackend.WithUser withUser, ChangeData cd) { - return lazyLoad - ? withUser.change(cd) - : withUser.indexedChange(cd, notesFactory.createFromIndexedChange(cd.change())); - } - private boolean isWorldReadable(ChangeData cd) throws PermissionBackendException { try { - permissionBackendForChange(permissionBackend.user(anonymous), cd) - .check(ChangePermission.READ); + permissionBackend.user(anonymous).change(cd).check(ChangePermission.READ); } catch (AuthException ae) { return false; } diff --git a/java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java b/java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java index 196fc61f10..fed6541402 100644 --- a/java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java +++ b/java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java @@ -165,7 +165,7 @@ public class SearchingChangeCacheImpl implements GitReferenceUpdatedListener { List result = new ArrayList<>(cds.size()); for (ChangeData cd : cds) { result.add( - new AutoValue_SearchingChangeCacheImpl_CachedChange(cd.change(), cd.getReviewers())); + new AutoValue_SearchingChangeCacheImpl_CachedChange(cd.change(), cd.reviewers())); } return Collections.unmodifiableList(result); } diff --git a/java/com/google/gerrit/server/permissions/ChangeControl.java b/java/com/google/gerrit/server/permissions/ChangeControl.java index 253f50cba8..f56c3c04b3 100644 --- a/java/com/google/gerrit/server/permissions/ChangeControl.java +++ b/java/com/google/gerrit/server/permissions/ChangeControl.java @@ -19,21 +19,16 @@ import static com.google.gerrit.server.permissions.LabelPermission.ForUser.ON_BE import com.google.common.collect.Maps; import com.google.common.collect.Sets; -import com.google.gerrit.common.Nullable; import com.google.gerrit.common.data.Permission; import com.google.gerrit.common.data.PermissionRange; import com.google.gerrit.entities.Account; import com.google.gerrit.entities.Change; -import com.google.gerrit.entities.Project; import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.extensions.conditions.BooleanCondition; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.server.CurrentUser; -import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.permissions.PermissionBackend.ForChange; import com.google.gerrit.server.query.change.ChangeData; -import com.google.inject.Inject; -import com.google.inject.Singleton; import java.util.Collection; import java.util.EnumSet; import java.util.Map; @@ -41,39 +36,16 @@ import java.util.Set; /** Access control management for a user accessing a single change. */ class ChangeControl { - @Singleton - static class Factory { - private final ChangeData.Factory changeDataFactory; - private final ChangeNotes.Factory notesFactory; - - @Inject - Factory(ChangeData.Factory changeDataFactory, ChangeNotes.Factory notesFactory) { - this.changeDataFactory = changeDataFactory; - this.notesFactory = notesFactory; - } - - ChangeControl create(RefControl refControl, Project.NameKey project, Change.Id changeId) { - return create(refControl, notesFactory.create(project, changeId)); - } - - ChangeControl create(RefControl refControl, ChangeNotes notes) { - return new ChangeControl(changeDataFactory, refControl, notes); - } - } - - private final ChangeData.Factory changeDataFactory; private final RefControl refControl; - private final ChangeNotes notes; + private final ChangeData changeData; - private ChangeControl( - ChangeData.Factory changeDataFactory, RefControl refControl, ChangeNotes notes) { - this.changeDataFactory = changeDataFactory; + ChangeControl(RefControl refControl, ChangeData changeData) { this.refControl = refControl; - this.notes = notes; + this.changeData = changeData; } - ForChange asForChange(@Nullable ChangeData cd) { - return new ForChangeImpl(cd); + ForChange asForChange() { + return new ForChangeImpl(); } private CurrentUser getUser() { @@ -85,7 +57,7 @@ class ChangeControl { } private Change getChange() { - return notes.getChange(); + return changeData.change(); } /** Can this user see this change? */ @@ -218,19 +190,13 @@ class ChangeControl { } private class ForChangeImpl extends ForChange { - private ChangeData cd; private Map labels; private String resourcePath; - ForChangeImpl(@Nullable ChangeData cd) { - this.cd = cd; - } + private ForChangeImpl() {} private ChangeData changeData() { - if (cd == null) { - cd = changeDataFactory.create(notes); - } - return cd; + return changeData; } @Override diff --git a/java/com/google/gerrit/server/permissions/DefaultPermissionBackendModule.java b/java/com/google/gerrit/server/permissions/DefaultPermissionBackendModule.java index f3a3c784ab..3f84dffc21 100644 --- a/java/com/google/gerrit/server/permissions/DefaultPermissionBackendModule.java +++ b/java/com/google/gerrit/server/permissions/DefaultPermissionBackendModule.java @@ -31,7 +31,6 @@ public class DefaultPermissionBackendModule extends AbstractModule { // TODO(hiesel) Hide ProjectControl, RefControl, ChangeControl related bindings. factory(ProjectControl.Factory.class); factory(DefaultRefFilter.Factory.class); - bind(ChangeControl.Factory.class); } } } diff --git a/java/com/google/gerrit/server/permissions/DefaultRefFilter.java b/java/com/google/gerrit/server/permissions/DefaultRefFilter.java index 90d9d88c19..4e1a30c06a 100644 --- a/java/com/google/gerrit/server/permissions/DefaultRefFilter.java +++ b/java/com/google/gerrit/server/permissions/DefaultRefFilter.java @@ -448,12 +448,11 @@ class DefaultRefFilter { try { Map visibleChanges = new HashMap<>(); for (ChangeData cd : changeCache.getChangeData(project)) { - ChangeNotes notes = changeNotesFactory.createFromIndexedChange(cd.change()); if (!projectState.statePermitsRead()) { continue; } try { - permissionBackendForProject.indexedChange(cd, notes).check(ChangePermission.READ); + permissionBackendForProject.change(cd).check(ChangePermission.READ); visibleChanges.put(cd.getId(), cd.change().getDest()); } catch (AuthException e) { // Do nothing. diff --git a/java/com/google/gerrit/server/permissions/FailedPermissionBackend.java b/java/com/google/gerrit/server/permissions/FailedPermissionBackend.java index 2344781420..749ca6b027 100644 --- a/java/com/google/gerrit/server/permissions/FailedPermissionBackend.java +++ b/java/com/google/gerrit/server/permissions/FailedPermissionBackend.java @@ -172,11 +172,6 @@ public class FailedPermissionBackend { return new FailedChange(message, cause); } - @Override - public ForChange indexedChange(ChangeData cd, ChangeNotes notes) { - return new FailedChange(message, cause); - } - @Override public void check(RefPermission perm) throws PermissionBackendException { throw new PermissionBackendException(message, cause); diff --git a/java/com/google/gerrit/server/permissions/PermissionBackend.java b/java/com/google/gerrit/server/permissions/PermissionBackend.java index 653c3b5f5f..23145baf0a 100644 --- a/java/com/google/gerrit/server/permissions/PermissionBackend.java +++ b/java/com/google/gerrit/server/permissions/PermissionBackend.java @@ -173,15 +173,6 @@ public abstract class PermissionBackend { return ref(notes.getChange().getDest()).change(notes); } - /** - * Returns an instance scoped for the change loaded from index, and its destination ref and - * project. This method should only be used when database access is harmful and potentially - * stale data from the index is acceptable. - */ - public ForChange indexedChange(ChangeData cd, ChangeNotes notes) { - return ref(notes.getChange().getDest()).indexedChange(cd, notes); - } - /** Verify scoped user can {@code perm}, throwing if denied. */ public abstract void check(GlobalOrPluginPermission perm) throws AuthException, PermissionBackendException; @@ -289,15 +280,6 @@ public abstract class PermissionBackend { return ref(notes.getChange().getDest().branch()).change(notes); } - /** - * Returns an instance scoped for the change loaded from index, and its destination ref and - * project. This method should only be used when database access is harmful and potentially - * stale data from the index is acceptable. - */ - public ForChange indexedChange(ChangeData cd, ChangeNotes notes) { - return ref(notes.getChange().getDest().branch()).indexedChange(cd, notes); - } - /** Verify scoped user can {@code perm}, throwing if denied. */ public abstract void check(CoreOrPluginProjectPermission perm) throws AuthException, PermissionBackendException; @@ -386,12 +368,6 @@ public abstract class PermissionBackend { /** Returns an instance scoped to change. */ public abstract ForChange change(ChangeNotes notes); - /** - * @return instance scoped to change loaded from index. This method should only be used when - * database access is harmful and potentially stale data from the index is acceptable. - */ - public abstract ForChange indexedChange(ChangeData cd, ChangeNotes notes); - /** Verify scoped user can {@code perm}, throwing if denied. */ public abstract void check(RefPermission perm) throws AuthException, PermissionBackendException; diff --git a/java/com/google/gerrit/server/permissions/ProjectControl.java b/java/com/google/gerrit/server/permissions/ProjectControl.java index 145e0b61bc..e6d66ee4b2 100644 --- a/java/com/google/gerrit/server/permissions/ProjectControl.java +++ b/java/com/google/gerrit/server/permissions/ProjectControl.java @@ -70,9 +70,9 @@ class ProjectControl { private final PermissionBackend permissionBackend; private final CurrentUser user; private final ProjectState state; - private final ChangeControl.Factory changeControlFactory; private final PermissionCollection.Factory permissionFilter; private final DefaultRefFilter.Factory refFilterFactory; + private final ChangeData.Factory changeDataFactory; private List allSections; private Map refControls; @@ -83,17 +83,17 @@ class ProjectControl { @GitUploadPackGroups Set uploadGroups, @GitReceivePackGroups Set receiveGroups, PermissionCollection.Factory permissionFilter, - ChangeControl.Factory changeControlFactory, PermissionBackend permissionBackend, DefaultRefFilter.Factory refFilterFactory, + ChangeData.Factory changeDataFactory, @Assisted CurrentUser who, @Assisted ProjectState ps) { - this.changeControlFactory = changeControlFactory; this.uploadGroups = uploadGroups; this.receiveGroups = receiveGroups; this.permissionFilter = permissionFilter; this.permissionBackend = permissionBackend; this.refFilterFactory = refFilterFactory; + this.changeDataFactory = changeDataFactory; user = who; state = ps; } @@ -102,13 +102,8 @@ class ProjectControl { return new ForProjectImpl(); } - ChangeControl controlFor(Change change) { - return changeControlFactory.create( - controlForRef(change.getDest()), change.getProject(), change.getId()); - } - - ChangeControl controlFor(ChangeNotes notes) { - return changeControlFactory.create(controlForRef(notes.getChange().getDest()), notes); + ChangeControl controlFor(ChangeData cd) { + return new ChangeControl(controlForRef(cd.change().getDest()), cd); } RefControl controlForRef(BranchNameKey ref) { @@ -122,7 +117,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(changeDataFactory, this, refName, relevant); refControls.put(refName, ctl); } return ctl; diff --git a/java/com/google/gerrit/server/permissions/RefControl.java b/java/com/google/gerrit/server/permissions/RefControl.java index 2c633ba458..395b8cf5ba 100644 --- a/java/com/google/gerrit/server/permissions/RefControl.java +++ b/java/com/google/gerrit/server/permissions/RefControl.java @@ -43,6 +43,7 @@ import java.util.Set; class RefControl { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + private final ChangeData.Factory changeDataFactory; private final ProjectControl projectControl; private final String refName; @@ -58,7 +59,12 @@ class RefControl { private Boolean canForgeCommitter; private Boolean isVisible; - RefControl(ProjectControl projectControl, String ref, PermissionCollection relevant) { + RefControl( + ChangeData.Factory changeDataFactory, + ProjectControl projectControl, + String ref, + PermissionCollection relevant) { + this.changeDataFactory = changeDataFactory; this.projectControl = projectControl; this.refName = ref; this.relevant = relevant; @@ -440,7 +446,7 @@ class RefControl { @Override public ForChange change(ChangeData cd) { try { - return getProjectControl().controlFor(cd.notes()).asForChange(cd); + return getProjectControl().controlFor(cd).asForChange(); } catch (StorageException e) { return FailedPermissionBackend.change("unavailable", e); } @@ -455,12 +461,9 @@ class RefControl { "expected change in project %s, not %s", project, change.getProject()); - return getProjectControl().controlFor(notes).asForChange(null); - } - - @Override - public ForChange indexedChange(ChangeData cd, ChangeNotes notes) { - return getProjectControl().controlFor(notes).asForChange(cd); + // Having ChangeNotes means it's OK to load values from NoteDb if needed. + // ChangeData.Factory will allow lazyLoading + return getProjectControl().controlFor(changeDataFactory.create(notes)).asForChange(); } @Override diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java index 6e55bf2552..ae23821060 100644 --- a/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/java/com/google/gerrit/server/query/change/ChangeData.java @@ -674,7 +674,9 @@ public class ChangeData { public ReviewerSet reviewers() { if (reviewers == null) { if (!lazyLoad) { - return ReviewerSet.empty(); + // We are not allowed to load values from NoteDb. Reviewers were not populated with values + // from the index. However, we need these values for permission checks. + throw new IllegalStateException("reviewers not populated"); } reviewers = approvalsUtil.getReviewers(notes(), approvals().values()); } @@ -685,10 +687,6 @@ public class ChangeData { this.reviewers = reviewers; } - public ReviewerSet getReviewers() { - return reviewers; - } - public ReviewerByEmailSet reviewersByEmail() { if (reviewersByEmail == null) { if (!lazyLoad) { diff --git a/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java b/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java index 2e5c33b0fa..ec8d1210da 100644 --- a/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java +++ b/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java @@ -67,7 +67,6 @@ public class ChangeIsVisibleToPredicate extends IsVisibleToPredicate return false; } - ChangeNotes notes = notesFactory.createFromIndexedChange(change); Optional projectState = projectCache.get(cd.project()); if (!projectState.isPresent()) { logger.atFine().log("Filter out change %s of non-existing project %s", cd, cd.project()); @@ -83,7 +82,7 @@ public class ChangeIsVisibleToPredicate extends IsVisibleToPredicate ? permissionBackend.absentUser(user.getAccountId()) : permissionBackend.user(anonymousUserProvider.get()); try { - withUser.indexedChange(cd, notes).check(ChangePermission.READ); + withUser.change(cd).check(ChangePermission.READ); } catch (PermissionBackendException e) { Throwable cause = e.getCause(); if (cause instanceof RepositoryNotFoundException) { diff --git a/javatests/com/google/gerrit/acceptance/server/permissions/PermissionBackendIT.java b/javatests/com/google/gerrit/acceptance/server/permissions/PermissionBackendIT.java new file mode 100644 index 0000000000..2aab159c67 --- /dev/null +++ b/javatests/com/google/gerrit/acceptance/server/permissions/PermissionBackendIT.java @@ -0,0 +1,70 @@ +// 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.server.permissions; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.common.collect.Iterables; +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.entities.Change; +import com.google.gerrit.server.notedb.ChangeNotes; +import com.google.gerrit.server.permissions.ChangePermission; +import com.google.gerrit.server.permissions.PermissionBackend; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.inject.Inject; +import org.junit.Test; + +/** Asserts behavior on {@link PermissionBackend} using a fully-started Gerrit. */ +public class PermissionBackendIT extends AbstractDaemonTest { + @Inject PermissionBackend pb; + @Inject ChangeNotes.Factory changeNotesFactory; + + @Test + public void changeDataFromIndex_canCheckReviewerState() throws Exception { + Change.Id changeId = createChange().getChange().getId(); + gApi.changes().id(changeId.get()).setPrivate(true); + gApi.changes().id(changeId.get()).addReviewer(user.email()); + + ChangeData changeData = + Iterables.getOnlyElement(queryProvider.get().byLegacyChangeId(changeId)); + boolean reviewerCanSee = + pb.absentUser(user.id()).change(changeData).test(ChangePermission.READ); + assertThat(reviewerCanSee).isTrue(); + } + + @Test + public void changeDataFromNoteDb_canCheckReviewerState() throws Exception { + Change.Id changeId = createChange().getChange().getId(); + gApi.changes().id(changeId.get()).setPrivate(true); + gApi.changes().id(changeId.get()).addReviewer(user.email()); + + ChangeNotes notes = changeNotesFactory.create(project, changeId); + ChangeData changeData = changeDataFactory.create(notes); + boolean reviewerCanSee = + pb.absentUser(user.id()).change(changeData).test(ChangePermission.READ); + assertThat(reviewerCanSee).isTrue(); + } + + @Test + public void changeNotes_canCheckReviewerState() throws Exception { + Change.Id changeId = createChange().getChange().getId(); + gApi.changes().id(changeId.get()).setPrivate(true); + gApi.changes().id(changeId.get()).addReviewer(user.email()); + + ChangeNotes notes = changeNotesFactory.create(project, changeId); + boolean reviewerCanSee = pb.absentUser(user.id()).change(notes).test(ChangePermission.READ); + assertThat(reviewerCanSee).isTrue(); + } +} diff --git a/javatests/com/google/gerrit/acceptance/ssh/SetReviewersIT.java b/javatests/com/google/gerrit/acceptance/ssh/SetReviewersIT.java index 5a31bfda2a..58c251712b 100644 --- a/javatests/com/google/gerrit/acceptance/ssh/SetReviewersIT.java +++ b/javatests/com/google/gerrit/acceptance/ssh/SetReviewersIT.java @@ -65,7 +65,7 @@ public class SetReviewersIT extends AbstractDaemonTest { session.exec( String.format("gerrit set-reviewers -%s %s %s", add ? "a" : "r", user.email(), id)); session.assertSuccess(); - ImmutableSet reviewers = change.getChange().getReviewers().all(); + ImmutableSet reviewers = change.getChange().reviewers().all(); if (add) { assertThat(reviewers).contains(user.id()); } else { From 43fee50c1221617fd824499c1f4c51ecaa3a819f Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Fri, 17 Apr 2020 11:15:57 +0200 Subject: [PATCH 2/2] Unmark DETAILED_LABELS as requiring lazy load Prework has made it so that we can do permission checks without requiring ChangeNotes. RemoveReviewerControl does also not require ChangeNotes when #test is used. We can therefore mark DETAILED_LABELS as not requiring lazy load. Change-Id: Ib468eb4bf775ee111e3b5fc032defdfbe1346c92 --- java/com/google/gerrit/server/change/ChangeJson.java | 1 - 1 file changed, 1 deletion(-) diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java index e2beef63a4..97e79600f4 100644 --- a/java/com/google/gerrit/server/change/ChangeJson.java +++ b/java/com/google/gerrit/server/change/ChangeJson.java @@ -140,7 +140,6 @@ public class ChangeJson { COMMIT_FOOTERS, CURRENT_ACTIONS, CURRENT_COMMIT, - DETAILED_LABELS, // may need to load ChangeNotes to check remove reviewer permissions MESSAGES); @Singleton