opendev/gerrit
Code Issues Proposed changes

Obsolete PermissionBackend#indexedChange

Browse Source 
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
This commit is contained in:
Patrick Hiesel
2020-03-06 16:38:30 +01:00
parent e96815ac36
commit 707c75541a
15 changed files with 109 additions and 149 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
java/com/google/gerrit/server/change/ChangeJson.java
View File

@@ -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()));
}
}

16
java/com/google/gerrit/server/change/LabelsJson.java
View File

@@ -131,7 +131,7 @@ public class LabelsJson {
Map<String, Short> labels = null;
Set<LabelPermission.WithValue> can =
permissionBackendForChange(filterApprovalsBy, cd).testLabels(toCheck.values());
permissionBackend.absentUser(filterApprovalsBy).change(cd).testLabels(toCheck.values());
SetMultimap<String, String> 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<String, VotingRangeInfo> pvr = getPermittedVotingRanges(permittedLabels(accountId, cd));
for (Map.Entry<String, LabelWithStatus> 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<SubmitRecord> submitRecords(ChangeData cd) {
return cd.submitRecords(ChangeJson.SUBMIT_RULE_OPTIONS_LENIENT);
}

21
java/com/google/gerrit/server/change/RevisionJson.java
View File

@@ -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<ListChangesOption> 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;
}

2
java/com/google/gerrit/server/git/SearchingChangeCacheImpl.java
View File

@@ -165,7 +165,7 @@ public class SearchingChangeCacheImpl implements GitReferenceUpdatedListener {
List<CachedChange> 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);
}

50
java/com/google/gerrit/server/permissions/ChangeControl.java
View File

@@ -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<String, PermissionRange> 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

1
java/com/google/gerrit/server/permissions/DefaultPermissionBackendModule.java
View File

@@ -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);
}
}
}

3
java/com/google/gerrit/server/permissions/DefaultRefFilter.java
View File

@@ -448,12 +448,11 @@ class DefaultRefFilter {
try {
Map<Change.Id, BranchNameKey> 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.

5
java/com/google/gerrit/server/permissions/FailedPermissionBackend.java
View File

@@ -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);

24
java/com/google/gerrit/server/permissions/PermissionBackend.java
View File

@@ -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;

17
java/com/google/gerrit/server/permissions/ProjectControl.java
View File

@@ -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<SectionMatcher> allSections;
private Map<String, RefControl> refControls;
@@ -83,17 +83,17 @@ class ProjectControl {
@GitUploadPackGroups Set<AccountGroup.UUID> uploadGroups,
@GitReceivePackGroups Set<AccountGroup.UUID> 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;

19
java/com/google/gerrit/server/permissions/RefControl.java
View File

@@ -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

8
java/com/google/gerrit/server/query/change/ChangeData.java
View File

@@ -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) {

3
java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java
View File

@@ -67,7 +67,6 @@ public class ChangeIsVisibleToPredicate extends IsVisibleToPredicate<ChangeData>
return false;
}
ChangeNotes notes = notesFactory.createFromIndexedChange(change);
Optional<ProjectState> 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<ChangeData>
? 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) {

70
javatests/com/google/gerrit/acceptance/server/permissions/PermissionBackendIT.java Normal file
View File

@@ -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();
}
}

2
javatests/com/google/gerrit/acceptance/ssh/SetReviewersIT.java
View File

@@ -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<Account.Id> reviewers = change.getChange().getReviewers().all();
ImmutableSet<Account.Id> reviewers = change.getChange().reviewers().all();
if (add) {
assertThat(reviewers).contains(user.id());
} else {