Remove PermissionBackend.*#user()

This commit removes the user() method from all PermissionBackend
subclasses. This method just encourages callers to pass around a
PermissionBackend when they really want to pass around a CurrentUser
and/or ChangeNotes.

Passing around heavy objects that represent a subsystem for the benefit
of getting smaller objects from it is an anti-pattern. We had this issue
at a larger scale with {Ref,Project,Change}Control that were often just
passed around to have a user and/or entity information. Removing the
user() methods should prevent that.

This refactoring also uncovered some wrongdoing that we already had in
the code as a direct result from passing PermissionBackends around (see
previous commit).

Removing user() comes at the (small) const that every PermissionBackend
has to implement #testCond by itsself, which seems fair and like a
better pattern in general.

Change-Id: Ibae1fc25e51228dca2acaf0faec76beee539da01
This commit is contained in:
Patrick Hiesel
2018-07-09 13:24:37 +02:00
parent 51b5df33af
commit b682dae920
11 changed files with 122 additions and 127 deletions

View File

@@ -658,10 +658,9 @@ public class ChangeJson {
// list permitted labels, since users can't vote on those patch sets.
if (user.isIdentifiedUser()
&& (!limitToPsId.isPresent() || limitToPsId.get().equals(in.currentPatchSetId()))) {
PermissionBackend.ForChange perm = permissionBackendForChange(user, cd);
out.permittedLabels =
cd.change().getStatus() != Change.Status.ABANDONED
? permittedLabels(perm, cd)
? permittedLabels(user.getAccountId(), cd)
: ImmutableMap.of();
}
@@ -889,7 +888,7 @@ public class ChangeJson {
LabelTypes labelTypes = cd.getLabelTypes();
for (Account.Id accountId : allUsers) {
PermissionBackend.ForChange perm = permissionBackendForChange(accountId, cd);
Map<String, VotingRangeInfo> pvr = getPermittedVotingRanges(permittedLabels(perm, cd));
Map<String, VotingRangeInfo> pvr = getPermittedVotingRanges(permittedLabels(accountId, cd));
for (Map.Entry<String, LabelWithStatus> e : labels.entrySet()) {
LabelType lt = labelTypes.byLabel(e.getKey());
if (lt == null) {
@@ -1030,8 +1029,7 @@ public class ChangeJson {
Map<String, ApprovalInfo> byLabel = Maps.newHashMapWithExpectedSize(labels.size());
Map<String, VotingRangeInfo> pvr = Collections.emptyMap();
if (detailed) {
PermissionBackend.ForChange perm = permissionBackendForChange(accountId, cd);
pvr = getPermittedVotingRanges(permittedLabels(perm, cd));
pvr = getPermittedVotingRanges(permittedLabels(accountId, cd));
for (Map.Entry<String, LabelWithStatus> entry : labels.entrySet()) {
ApprovalInfo ai = approvalInfo(accountId, 0, null, null, null);
byLabel.put(entry.getKey(), ai);
@@ -1106,8 +1104,7 @@ public class ChangeJson {
}
private Map<String, Collection<String>> permittedLabels(
PermissionBackend.ForChange perm, ChangeData cd)
throws OrmException, PermissionBackendException {
Account.Id filterApprovalsBy, ChangeData cd) throws OrmException, PermissionBackendException {
boolean isMerged = cd.change().getStatus() == Change.Status.MERGED;
LabelTypes labelTypes = cd.getLabelTypes();
Map<String, LabelType> toCheck = new HashMap<>();
@@ -1123,7 +1120,8 @@ public class ChangeJson {
}
Map<String, Short> labels = null;
Set<LabelPermission.WithValue> can = perm.testLabels(toCheck.values());
Set<LabelPermission.WithValue> can =
permissionBackendForChange(filterApprovalsBy, cd).testLabels(toCheck.values());
SetMultimap<String, String> permitted = LinkedHashMultimap.create();
for (SubmitRecord rec : submitRecords(cd)) {
if (rec.labels == null) {
@@ -1139,7 +1137,7 @@ public class ChangeJson {
boolean ok = can.contains(new LabelPermission.WithValue(type, v));
if (isMerged) {
if (labels == null) {
labels = currentLabels(perm, cd);
labels = currentLabels(filterApprovalsBy, cd);
}
short prev = labels.getOrDefault(type.getName(), (short) 0);
ok &= v.getValue() >= prev;
@@ -1163,16 +1161,15 @@ public class ChangeJson {
return permitted.asMap();
}
private Map<String, Short> currentLabels(PermissionBackend.ForChange perm, ChangeData cd)
private Map<String, Short> currentLabels(Account.Id accountId, ChangeData cd)
throws OrmException {
IdentifiedUser user = perm.user().asIdentifiedUser();
Map<String, Short> result = new HashMap<>();
for (PatchSetApproval psa :
approvalsUtil.byPatchSetUser(
db.get(),
lazyLoad ? cd.notes() : notesFactory.createFromIndexedChange(cd.change()),
cd.change().currentPatchSetId(),
user.getAccountId(),
accountId,
null,
null)) {
result.put(psa.getLabel(), psa.getValue());

View File

@@ -23,6 +23,7 @@ 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.extensions.conditions.BooleanCondition;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
@@ -262,14 +263,9 @@ class ChangeControl {
return cd;
}
@Override
public CurrentUser user() {
return getUser();
}
@Override
public ForChange user(CurrentUser user) {
return user().equals(user) ? this : forUser(user).asForChange(cd, db);
return getUser().equals(user) ? this : forUser(user).asForChange(cd, db);
}
@Override
@@ -308,6 +304,11 @@ class ChangeControl {
return ok;
}
@Override
public BooleanCondition testCond(ChangePermissionOrLabel perm) {
return new PermissionBackendCondition.ForChange(this, perm, getUser());
}
private boolean can(ChangePermissionOrLabel perm) throws PermissionBackendException {
if (perm instanceof ChangePermission) {
return can((ChangePermission) perm);

View File

@@ -23,6 +23,7 @@ import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.common.data.PermissionRule.Action;
import com.google.gerrit.extensions.api.access.GlobalOrPluginPermission;
import com.google.gerrit.extensions.api.access.PluginPermission;
import com.google.gerrit.extensions.conditions.BooleanCondition;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
@@ -32,6 +33,7 @@ import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PeerDaemonUser;
import com.google.gerrit.server.account.CapabilityCollection;
import com.google.gerrit.server.cache.PerThreadCache;
import com.google.gerrit.server.permissions.PermissionBackendCondition.WithUser;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.inject.Inject;
@@ -97,11 +99,6 @@ public class DefaultPermissionBackend extends PermissionBackend {
this.user = checkNotNull(user, "user");
}
@Override
public CurrentUser user() {
return user;
}
@Override
public ForProject project(Project.NameKey project) {
try {
@@ -138,6 +135,11 @@ public class DefaultPermissionBackend extends PermissionBackend {
return ok;
}
@Override
public BooleanCondition testCond(GlobalOrPluginPermission perm) {
return new PermissionBackendCondition.WithUser(this, perm, user);
}
private boolean can(GlobalOrPluginPermission perm) throws PermissionBackendException {
if (perm instanceof GlobalPermission) {
return can((GlobalPermission) perm);

View File

@@ -15,6 +15,7 @@
package com.google.gerrit.server.permissions;
import com.google.gerrit.extensions.api.access.GlobalOrPluginPermission;
import com.google.gerrit.extensions.conditions.BooleanCondition;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -83,11 +84,6 @@ public class FailedPermissionBackend {
this.cause = cause;
}
@Override
public CurrentUser user() {
throw new UnsupportedOperationException("FailedPermissionBackend is not scoped to user");
}
@Override
public ForProject project(Project.NameKey project) {
return new FailedProject(message, cause);
@@ -103,6 +99,12 @@ public class FailedPermissionBackend {
throws PermissionBackendException {
throw new PermissionBackendException(message, cause);
}
@Override
public BooleanCondition testCond(GlobalOrPluginPermission perm) {
throw new UnsupportedOperationException(
"FailedPermissionBackend does not support conditions");
}
}
private static class FailedProject extends ForProject {
@@ -119,11 +121,6 @@ public class FailedPermissionBackend {
return this;
}
@Override
public CurrentUser user() {
throw new UnsupportedOperationException("FailedPermissionBackend is not scoped to user");
}
@Override
public ForProject user(CurrentUser user) {
return this;
@@ -156,6 +153,12 @@ public class FailedPermissionBackend {
throw new PermissionBackendException(message, cause);
}
@Override
public BooleanCondition testCond(ProjectPermission perm) {
throw new UnsupportedOperationException(
"FailedPermissionBackend does not support conditions");
}
@Override
public Map<String, Ref> filter(Map<String, Ref> refs, Repository repo, RefFilterOptions opts)
throws PermissionBackendException {
@@ -177,11 +180,6 @@ public class FailedPermissionBackend {
return this;
}
@Override
public CurrentUser user() {
throw new UnsupportedOperationException("FailedPermissionBackend is not scoped to user");
}
@Override
public ForRef user(CurrentUser user) {
return this;
@@ -223,6 +221,12 @@ public class FailedPermissionBackend {
throws PermissionBackendException {
throw new PermissionBackendException(message, cause);
}
@Override
public BooleanCondition testCond(RefPermission perm) {
throw new UnsupportedOperationException(
"FailedPermissionBackend does not support conditions");
}
}
private static class FailedChange extends ForChange {
@@ -267,8 +271,9 @@ public class FailedPermissionBackend {
}
@Override
public CurrentUser user() {
throw new UnsupportedOperationException("FailedPermissionBackend is not scoped to user");
public BooleanCondition testCond(ChangePermissionOrLabel perm) {
throw new UnsupportedOperationException(
"FailedPermissionBackend does not support conditions");
}
}
}

View File

@@ -173,9 +173,6 @@ public abstract class PermissionBackend {
/** PermissionBackend scoped to a specific user. */
public abstract static class WithUser extends AcceptsReviewDb<WithUser> {
/** Returns the user this instance is scoped to. */
public abstract CurrentUser user();
/** Returns an instance scoped for the specified project. */
public abstract ForProject project(Project.NameKey project);
@@ -257,9 +254,7 @@ public abstract class PermissionBackend {
}
}
public BooleanCondition testCond(GlobalOrPluginPermission perm) {
return new PermissionBackendCondition.WithUser(this, perm);
}
public abstract BooleanCondition testCond(GlobalOrPluginPermission perm);
/**
* Filter a set of projects using {@code check(perm)}.
@@ -296,9 +291,6 @@ public abstract class PermissionBackend {
/** PermissionBackend scoped to a user and project. */
public abstract static class ForProject extends AcceptsReviewDb<ForProject> {
/** Returns the user this instance is scoped to. */
public abstract CurrentUser user();
/** Returns the fully qualified resource path that this instance is scoped to. */
public abstract String resourcePath();
@@ -355,9 +347,7 @@ public abstract class PermissionBackend {
}
}
public BooleanCondition testCond(ProjectPermission perm) {
return new PermissionBackendCondition.ForProject(this, perm);
}
public abstract BooleanCondition testCond(ProjectPermission perm);
/**
* Filter a map of references by visibility.
@@ -407,9 +397,6 @@ public abstract class PermissionBackend {
/** PermissionBackend scoped to a user, project and reference. */
public abstract static class ForRef extends AcceptsReviewDb<ForRef> {
/** Returns the user this instance is scoped to. */
public abstract CurrentUser user();
/** Returns a fully qualified resource path that this instance is scoped to. */
public abstract String resourcePath();
@@ -461,16 +448,11 @@ public abstract class PermissionBackend {
}
}
public BooleanCondition testCond(RefPermission perm) {
return new PermissionBackendCondition.ForRef(this, perm);
}
public abstract BooleanCondition testCond(RefPermission perm);
}
/** PermissionBackend scoped to a user, project, reference and change. */
public abstract static class ForChange extends AcceptsReviewDb<ForChange> {
/** Returns the user this instance is scoped to. */
public abstract CurrentUser user();
/** Returns the fully qualified resource path that this instance is scoped to. */
public abstract String resourcePath();
@@ -511,9 +493,7 @@ public abstract class PermissionBackend {
}
}
public BooleanCondition testCond(ChangePermissionOrLabel perm) {
return new PermissionBackendCondition.ForChange(this, perm);
}
public abstract BooleanCondition testCond(ChangePermissionOrLabel perm);
/**
* Test which values of a label the user may be able to set.

View File

@@ -56,10 +56,13 @@ public abstract class PermissionBackendCondition
public static class WithUser extends PermissionBackendCondition {
private final PermissionBackend.WithUser impl;
private final GlobalOrPluginPermission perm;
private final CurrentUser user;
WithUser(PermissionBackend.WithUser impl, GlobalOrPluginPermission perm) {
public WithUser(
PermissionBackend.WithUser impl, GlobalOrPluginPermission perm, CurrentUser user) {
this.impl = impl;
this.perm = perm;
this.user = user;
}
public PermissionBackend.WithUser withUser() {
@@ -82,7 +85,7 @@ public abstract class PermissionBackendCondition
@Override
public int hashCode() {
return Objects.hash(perm, hashForUser(impl.user()));
return Objects.hash(perm, hashForUser(user));
}
@Override
@@ -91,17 +94,19 @@ public abstract class PermissionBackendCondition
return false;
}
WithUser other = (WithUser) obj;
return Objects.equals(perm, other.perm) && usersAreEqual(impl.user(), other.impl.user());
return Objects.equals(perm, other.perm) && usersAreEqual(user, other.user);
}
}
public static class ForProject extends PermissionBackendCondition {
private final PermissionBackend.ForProject impl;
private final ProjectPermission perm;
private final CurrentUser user;
ForProject(PermissionBackend.ForProject impl, ProjectPermission perm) {
public ForProject(PermissionBackend.ForProject impl, ProjectPermission perm, CurrentUser user) {
this.impl = impl;
this.perm = perm;
this.user = user;
}
public PermissionBackend.ForProject project() {
@@ -124,7 +129,7 @@ public abstract class PermissionBackendCondition
@Override
public int hashCode() {
return Objects.hash(perm, impl.resourcePath(), hashForUser(impl.user()));
return Objects.hash(perm, impl.resourcePath(), hashForUser(user));
}
@Override
@@ -135,17 +140,19 @@ public abstract class PermissionBackendCondition
ForProject other = (ForProject) obj;
return Objects.equals(perm, other.perm)
&& Objects.equals(impl.resourcePath(), other.impl.resourcePath())
&& usersAreEqual(impl.user(), other.impl.user());
&& usersAreEqual(user, other.user);
}
}
public static class ForRef extends PermissionBackendCondition {
private final PermissionBackend.ForRef impl;
private final RefPermission perm;
private final CurrentUser user;
ForRef(PermissionBackend.ForRef impl, RefPermission perm) {
public ForRef(PermissionBackend.ForRef impl, RefPermission perm, CurrentUser user) {
this.impl = impl;
this.perm = perm;
this.user = user;
}
public PermissionBackend.ForRef ref() {
@@ -168,7 +175,7 @@ public abstract class PermissionBackendCondition
@Override
public int hashCode() {
return Objects.hash(perm, impl.resourcePath(), hashForUser(impl.user()));
return Objects.hash(perm, impl.resourcePath(), hashForUser(user));
}
@Override
@@ -179,17 +186,20 @@ public abstract class PermissionBackendCondition
ForRef other = (ForRef) obj;
return Objects.equals(perm, other.perm)
&& Objects.equals(impl.resourcePath(), other.impl.resourcePath())
&& usersAreEqual(impl.user(), other.impl.user());
&& usersAreEqual(user, other.user);
}
}
public static class ForChange extends PermissionBackendCondition {
private final PermissionBackend.ForChange impl;
private final ChangePermissionOrLabel perm;
private final CurrentUser user;
ForChange(PermissionBackend.ForChange impl, ChangePermissionOrLabel perm) {
public ForChange(
PermissionBackend.ForChange impl, ChangePermissionOrLabel perm, CurrentUser user) {
this.impl = impl;
this.perm = perm;
this.user = user;
}
public PermissionBackend.ForChange change() {
@@ -212,7 +222,7 @@ public abstract class PermissionBackendCondition
@Override
public int hashCode() {
return Objects.hash(perm, impl.resourcePath(), hashForUser(impl.user()));
return Objects.hash(perm, impl.resourcePath(), hashForUser(user));
}
@Override
@@ -223,7 +233,7 @@ public abstract class PermissionBackendCondition
ForChange other = (ForChange) obj;
return Objects.equals(perm, other.perm)
&& Objects.equals(impl.resourcePath(), other.impl.resourcePath())
&& usersAreEqual(impl.user(), other.impl.user());
&& usersAreEqual(user, other.user);
}
}

View File

@@ -19,6 +19,7 @@ import static com.google.common.base.Preconditions.checkArgument;
import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.extensions.conditions.BooleanCondition;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
@@ -322,11 +323,6 @@ class ProjectControl {
private DefaultRefFilter refFilter;
private String resourcePath;
@Override
public CurrentUser user() {
return getUser();
}
@Override
public ForProject user(CurrentUser user) {
return forUser(user).asForProject().database(db);
@@ -394,6 +390,11 @@ class ProjectControl {
return ok;
}
@Override
public BooleanCondition testCond(ProjectPermission perm) {
return new PermissionBackendCondition.ForProject(this, perm, getUser());
}
@Override
public Map<String, Ref> filter(Map<String, Ref> refs, Repository repo, RefFilterOptions opts)
throws PermissionBackendException {

View File

@@ -20,6 +20,7 @@ import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRange;
import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.common.data.PermissionRule.Action;
import com.google.gerrit.extensions.conditions.BooleanCondition;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
@@ -407,11 +408,6 @@ class RefControl {
private class ForRefImpl extends ForRef {
private String resourcePath;
@Override
public CurrentUser user() {
return getUser();
}
@Override
public ForRef user(CurrentUser user) {
return forUser(user).asForRef().database(db);
@@ -480,6 +476,11 @@ class RefControl {
return ok;
}
@Override
public BooleanCondition testCond(RefPermission perm) {
return new PermissionBackendCondition.ForRef(this, perm, getUser());
}
private boolean can(RefPermission perm) throws PermissionBackendException {
switch (perm) {
case READ:

View File

@@ -452,17 +452,13 @@ public class PostReviewers
}
ChangeData cd = changeDataFactory.create(dbProvider.get(), notes);
PermissionBackend.ForChange perm =
permissionBackend.user(caller).database(dbProvider).change(cd);
// Generate result details and fill AccountLoader. This occurs outside
// the Op because the accounts are in a different table.
PostReviewersOp.Result opResult = op.getResult();
if (migration.readChanges() && state == CC) {
result.ccs = Lists.newArrayListWithCapacity(opResult.addedCCs().size());
for (Account.Id accountId : opResult.addedCCs()) {
result.ccs.add(
json.format(new ReviewerInfo(accountId.get()), perm.absentUser(accountId), cd));
result.ccs.add(json.format(new ReviewerInfo(accountId.get()), accountId, cd));
}
accountLoaderFactory.create(true).fill(result.ccs);
for (Address a : reviewersByEmail) {
@@ -475,7 +471,7 @@ public class PostReviewers
result.reviewers.add(
json.format(
new ReviewerInfo(psa.getAccountId().get()),
perm.absentUser(psa.getAccountId()),
psa.getAccountId(),
cd,
ImmutableList.of(psa)));
}

View File

@@ -80,10 +80,7 @@ public class ReviewerJson {
ReviewerInfo info =
format(
new ReviewerInfo(rsrc.getReviewerUser().getAccountId().get()),
permissionBackend
.absentUser(rsrc.getReviewerUser().getAccountId())
.database(db)
.change(cd),
rsrc.getReviewerUser().getAccountId(),
cd);
loader.put(info);
infos.add(info);
@@ -97,22 +94,19 @@ public class ReviewerJson {
return format(ImmutableList.<ReviewerResource>of(rsrc));
}
public ReviewerInfo format(ReviewerInfo out, PermissionBackend.ForChange perm, ChangeData cd)
public ReviewerInfo format(ReviewerInfo out, Account.Id reviewer, ChangeData cd)
throws OrmException, PermissionBackendException {
PatchSet.Id psId = cd.change().currentPatchSetId();
return format(
out,
perm,
reviewer,
cd,
approvalsUtil.byPatchSetUser(
db.get(), cd.notes(), psId, new Account.Id(out._accountId), null, null));
}
public ReviewerInfo format(
ReviewerInfo out,
PermissionBackend.ForChange perm,
ChangeData cd,
Iterable<PatchSetApproval> approvals)
ReviewerInfo out, Account.Id reviewer, ChangeData cd, Iterable<PatchSetApproval> approvals)
throws OrmException, PermissionBackendException {
LabelTypes labelTypes = cd.getLabelTypes();
@@ -128,6 +122,9 @@ public class ReviewerJson {
// do not exist in the DB.
PatchSet ps = cd.currentPatchSet();
if (ps != null) {
PermissionBackend.ForChange perm =
permissionBackend.absentUser(reviewer).database(db).change(cd);
for (SubmitRecord rec : submitRuleEvaluator.evaluate(cd)) {
if (rec.labels == null) {
continue;

View File

@@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.gerrit.extensions.conditions.BooleanCondition;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.server.CurrentUser;
@@ -42,31 +43,6 @@ public class UiActionsTest {
private static class FakeForProject extends ForProject {
private boolean allowValueQueries = true;
@Override
public CurrentUser user() {
return new CurrentUser() {
@Override
public GroupMembership getEffectiveGroups() {
throw new UnsupportedOperationException("not implemented");
}
@Override
public Object getCacheKey() {
return new Object();
}
@Override
public boolean isIdentifiedUser() {
return true;
}
@Override
public Account.Id getAccountId() {
return new Account.Id(1);
}
};
}
@Override
public String resourcePath() {
return "/projects/test-project";
@@ -99,6 +75,11 @@ public class UiActionsTest {
return ImmutableSet.of(ProjectPermission.READ);
}
@Override
public BooleanCondition testCond(ProjectPermission perm) {
return new PermissionBackendCondition.ForProject(this, perm, fakeUser());
}
@Override
public Map<String, Ref> filter(Map<String, Ref> refs, Repository repo, RefFilterOptions opts)
throws PermissionBackendException {
@@ -108,6 +89,30 @@ public class UiActionsTest {
private void disallowValueQueries() {
allowValueQueries = false;
}
private static CurrentUser fakeUser() {
return new CurrentUser() {
@Override
public GroupMembership getEffectiveGroups() {
throw new UnsupportedOperationException("not implemented");
}
@Override
public Object getCacheKey() {
return new Object();
}
@Override
public boolean isIdentifiedUser() {
return true;
}
@Override
public Account.Id getAccountId() {
return new Account.Id(1);
}
};
}
}
@Test