Test permitted labels with PermissionBackend

Use PermissionBackend and its test methods to determine which labels
a reviewer might be able to assign during a review.

Change-Id: I7fe6ba1f45de0d4f04575cdba1ae541304ffd7a2
This commit is contained in:
Shawn Pearce
2017-02-19 12:59:16 -08:00
committed by David Pursehouse
parent 9d58a38670
commit 557a89fb52
10 changed files with 141 additions and 59 deletions

View File

@@ -38,6 +38,7 @@ import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.group.SystemGroupBackend; import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectControl; import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.project.RefControl; import com.google.gerrit.server.project.RefControl;
@@ -174,7 +175,11 @@ public class ReviewProjectAccess extends ProjectAccessHandler<Change.Id> {
AddReviewerInput input = new AddReviewerInput(); AddReviewerInput input = new AddReviewerInput();
input.reviewer = projectOwners; input.reviewer = projectOwners;
reviewersProvider.get().apply(rsrc, input); reviewersProvider.get().apply(rsrc, input);
} catch (IOException | OrmException | RestApiException | UpdateException e) { } catch (IOException
| OrmException
| RestApiException
| UpdateException
| PermissionBackendException e) {
// one of the owner groups is not visible to the user and this it why it // one of the owner groups is not visible to the user and this it why it
// can't be added as reviewer // can't be added as reviewer
} }
@@ -193,7 +198,11 @@ public class ReviewProjectAccess extends ProjectAccessHandler<Change.Id> {
AddReviewerInput input = new AddReviewerInput(); AddReviewerInput input = new AddReviewerInput();
input.reviewer = r.getGroup().getUUID().get(); input.reviewer = r.getGroup().getUUID().get();
reviewersProvider.get().apply(rsrc, input); reviewersProvider.get().apply(rsrc, input);
} catch (IOException | OrmException | RestApiException | UpdateException e) { } catch (IOException
| OrmException
| RestApiException
| UpdateException
| PermissionBackendException e) {
// ignore // ignore
} }
} }

View File

@@ -416,7 +416,7 @@ class ChangeApiImpl implements ChangeApi {
public void addReviewer(AddReviewerInput in) throws RestApiException { public void addReviewer(AddReviewerInput in) throws RestApiException {
try { try {
postReviewers.apply(change, in); postReviewers.apply(change, in);
} catch (OrmException | IOException | UpdateException e) { } catch (OrmException | IOException | UpdateException | PermissionBackendException e) {
throw new RestApiException("Cannot add change reviewer", e); throw new RestApiException("Cannot add change reviewer", e);
} }
} }

View File

@@ -59,8 +59,6 @@ import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.LabelType; import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.common.data.LabelValue; import com.google.gerrit.common.data.LabelValue;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRange;
import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.common.data.SubmitTypeRecord; import com.google.gerrit.common.data.SubmitTypeRecord;
import com.google.gerrit.extensions.api.changes.FixInput; import com.google.gerrit.extensions.api.changes.FixInput;
@@ -104,7 +102,6 @@ import com.google.gerrit.server.account.AccountLoader;
import com.google.gerrit.server.api.accounts.AccountInfoComparator; import com.google.gerrit.server.api.accounts.AccountInfoComparator;
import com.google.gerrit.server.api.accounts.GpgApiAdapter; import com.google.gerrit.server.api.accounts.GpgApiAdapter;
import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.LabelNormalizer;
import com.google.gerrit.server.git.MergeUtil; import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.index.change.ChangeField; import com.google.gerrit.server.index.change.ChangeField;
import com.google.gerrit.server.index.change.ChangeIndexCollection; import com.google.gerrit.server.index.change.ChangeIndexCollection;
@@ -112,6 +109,9 @@ import com.google.gerrit.server.mail.Address;
import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ReviewerStateInternal; import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.permissions.LabelPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.SubmitRuleOptions; import com.google.gerrit.server.project.SubmitRuleOptions;
@@ -186,9 +186,9 @@ public class ChangeJson {
} }
private final Provider<ReviewDb> db; private final Provider<ReviewDb> db;
private final LabelNormalizer labelNormalizer;
private final Provider<CurrentUser> userProvider; private final Provider<CurrentUser> userProvider;
private final AnonymousUser anonymous; private final AnonymousUser anonymous;
private final PermissionBackend permissionBackend;
private final GitRepositoryManager repoManager; private final GitRepositoryManager repoManager;
private final ProjectCache projectCache; private final ProjectCache projectCache;
private final MergeUtil.Factory mergeUtilFactory; private final MergeUtil.Factory mergeUtilFactory;
@@ -217,9 +217,9 @@ public class ChangeJson {
@Inject @Inject
ChangeJson( ChangeJson(
Provider<ReviewDb> db, Provider<ReviewDb> db,
LabelNormalizer ln,
Provider<CurrentUser> user, Provider<CurrentUser> user,
AnonymousUser au, AnonymousUser au,
PermissionBackend permissionBackend,
GitRepositoryManager repoManager, GitRepositoryManager repoManager,
ProjectCache projectCache, ProjectCache projectCache,
MergeUtil.Factory mergeUtilFactory, MergeUtil.Factory mergeUtilFactory,
@@ -241,10 +241,10 @@ public class ChangeJson {
ApprovalsUtil approvalsUtil, ApprovalsUtil approvalsUtil,
@Assisted Iterable<ListChangesOption> options) { @Assisted Iterable<ListChangesOption> options) {
this.db = db; this.db = db;
this.labelNormalizer = ln;
this.userProvider = user; this.userProvider = user;
this.anonymous = au; this.anonymous = au;
this.changeDataFactory = cdf; this.changeDataFactory = cdf;
this.permissionBackend = permissionBackend;
this.repoManager = repoManager; this.repoManager = repoManager;
this.userFactory = uf; this.userFactory = uf;
this.projectCache = projectCache; this.projectCache = projectCache;
@@ -316,6 +316,7 @@ public class ChangeJson {
| GpgException | GpgException
| OrmException | OrmException
| IOException | IOException
| PermissionBackendException
| RuntimeException e) { | RuntimeException e) {
if (!has(CHECK)) { if (!has(CHECK)) {
Throwables.throwIfInstanceOf(e, OrmException.class); Throwables.throwIfInstanceOf(e, OrmException.class);
@@ -393,6 +394,7 @@ public class ChangeJson {
| GpgException | GpgException
| OrmException | OrmException
| IOException | IOException
| PermissionBackendException
| RuntimeException e) { | RuntimeException e) {
if (has(CHECK)) { if (has(CHECK)) {
i = checkOnly(cd); i = checkOnly(cd);
@@ -450,7 +452,8 @@ public class ChangeJson {
} }
private ChangeInfo toChangeInfo(ChangeData cd, Optional<PatchSet.Id> limitToPsId) private ChangeInfo toChangeInfo(ChangeData cd, Optional<PatchSet.Id> limitToPsId)
throws PatchListNotAvailableException, GpgException, OrmException, IOException { throws PatchListNotAvailableException, GpgException, OrmException, IOException,
PermissionBackendException {
ChangeInfo out = new ChangeInfo(); ChangeInfo out = new ChangeInfo();
CurrentUser user = userProvider.get(); CurrentUser user = userProvider.get();
ChangeControl ctl = cd.changeControl().forUser(user); ChangeControl ctl = cd.changeControl().forUser(user);
@@ -466,6 +469,7 @@ public class ChangeJson {
} }
} }
PermissionBackend.ForChange perm = permissionBackend.user(user).database(db).change(cd);
Change in = cd.change(); Change in = cd.change();
out.project = in.getProject().get(); out.project = in.getProject().get();
out.branch = in.getDest().getShortName(); out.branch = in.getDest().getShortName();
@@ -514,7 +518,7 @@ public class ChangeJson {
out.reviewed = cd.reviewedBy().contains(accountId) ? true : null; out.reviewed = cd.reviewedBy().contains(accountId) ? true : null;
} }
out.labels = labelsFor(ctl, cd, has(LABELS), has(DETAILED_LABELS)); out.labels = labelsFor(perm, ctl, cd, has(LABELS), has(DETAILED_LABELS));
out.submitted = getSubmittedOn(cd); out.submitted = getSubmittedOn(cd);
if (out.labels != null && has(DETAILED_LABELS)) { if (out.labels != null && has(DETAILED_LABELS)) {
@@ -523,7 +527,7 @@ public class ChangeJson {
if (!limitToPsId.isPresent() || limitToPsId.get().equals(in.currentPatchSetId())) { if (!limitToPsId.isPresent() || limitToPsId.get().equals(in.currentPatchSetId())) {
out.permittedLabels = out.permittedLabels =
cd.change().getStatus() != Change.Status.ABANDONED cd.change().getStatus() != Change.Status.ABANDONED
? permittedLabels(ctl, cd) ? permittedLabels(perm, ctl, cd)
: ImmutableMap.of(); : ImmutableMap.of();
} }
@@ -603,7 +607,12 @@ public class ChangeJson {
} }
private Map<String, LabelInfo> labelsFor( private Map<String, LabelInfo> labelsFor(
ChangeControl ctl, ChangeData cd, boolean standard, boolean detailed) throws OrmException { PermissionBackend.ForChange perm,
ChangeControl ctl,
ChangeData cd,
boolean standard,
boolean detailed)
throws OrmException, PermissionBackendException {
if (!standard && !detailed) { if (!standard && !detailed) {
return null; return null;
} }
@@ -615,17 +624,22 @@ public class ChangeJson {
LabelTypes labelTypes = ctl.getLabelTypes(); LabelTypes labelTypes = ctl.getLabelTypes();
Map<String, LabelWithStatus> withStatus = Map<String, LabelWithStatus> withStatus =
cd.change().getStatus().isOpen() cd.change().getStatus().isOpen()
? labelsForOpenChange(ctl, cd, labelTypes, standard, detailed) ? labelsForOpenChange(perm, ctl, cd, labelTypes, standard, detailed)
: labelsForClosedChange(ctl, cd, labelTypes, standard, detailed); : labelsForClosedChange(perm, ctl, cd, labelTypes, standard, detailed);
return ImmutableMap.copyOf(Maps.transformValues(withStatus, LabelWithStatus::label)); return ImmutableMap.copyOf(Maps.transformValues(withStatus, LabelWithStatus::label));
} }
private Map<String, LabelWithStatus> labelsForOpenChange( private Map<String, LabelWithStatus> labelsForOpenChange(
ChangeControl ctl, ChangeData cd, LabelTypes labelTypes, boolean standard, boolean detailed) PermissionBackend.ForChange perm,
throws OrmException { ChangeControl ctl,
ChangeData cd,
LabelTypes labelTypes,
boolean standard,
boolean detailed)
throws OrmException, PermissionBackendException {
Map<String, LabelWithStatus> labels = initLabels(cd, labelTypes, standard); Map<String, LabelWithStatus> labels = initLabels(cd, labelTypes, standard);
if (detailed) { if (detailed) {
setAllApprovals(ctl, cd, labels); setAllApprovals(perm, ctl, cd, labels);
} }
for (Map.Entry<String, LabelWithStatus> e : labels.entrySet()) { for (Map.Entry<String, LabelWithStatus> e : labels.entrySet()) {
LabelType type = labelTypes.byLabel(e.getKey()); LabelType type = labelTypes.byLabel(e.getKey());
@@ -712,8 +726,11 @@ public class ChangeJson {
} }
private void setAllApprovals( private void setAllApprovals(
ChangeControl baseCtrl, ChangeData cd, Map<String, LabelWithStatus> labels) PermissionBackend.ForChange basePerm,
throws OrmException { ChangeControl baseCtrl,
ChangeData cd,
Map<String, LabelWithStatus> labels)
throws OrmException, PermissionBackendException {
Change.Status status = cd.change().getStatus(); Change.Status status = cd.change().getStatus();
checkState(status.isOpen(), "should not call setAllApprovals on %s change", status); checkState(status.isOpen(), "should not call setAllApprovals on %s change", status);
@@ -732,12 +749,14 @@ public class ChangeJson {
current.put(psa.getAccountId(), psa.getLabel(), psa); current.put(psa.getAccountId(), psa.getLabel(), psa);
} }
LabelTypes labelTypes = baseCtrl.getLabelTypes();
for (Account.Id accountId : allUsers) { for (Account.Id accountId : allUsers) {
IdentifiedUser user = userFactory.create(accountId); IdentifiedUser user = userFactory.create(accountId);
PermissionBackend.ForChange perm = basePerm.user(user);
ChangeControl ctl = baseCtrl.forUser(user); ChangeControl ctl = baseCtrl.forUser(user);
Map<String, VotingRangeInfo> pvr = getPermittedVotingRanges(permittedLabels(ctl, cd)); Map<String, VotingRangeInfo> pvr = getPermittedVotingRanges(permittedLabels(perm, ctl, cd));
for (Map.Entry<String, LabelWithStatus> e : labels.entrySet()) { for (Map.Entry<String, LabelWithStatus> e : labels.entrySet()) {
LabelType lt = ctl.getLabelTypes().byLabel(e.getKey()); LabelType lt = labelTypes.byLabel(e.getKey());
if (lt == null) { if (lt == null) {
// Ignore submit record for undefined label; likely the submit rule // Ignore submit record for undefined label; likely the submit rule
// author didn't intend for the label to show up in the table. // author didn't intend for the label to show up in the table.
@@ -754,7 +773,7 @@ public class ChangeJson {
// This may be a dummy approval that was inserted when the reviewer // This may be a dummy approval that was inserted when the reviewer
// was added. Explicitly check whether the user can vote on this // was added. Explicitly check whether the user can vote on this
// label. // label.
value = labelNormalizer.canVote(ctl, lt, accountId) ? 0 : null; value = perm.test(new LabelPermission(lt)) ? 0 : null;
} }
tag = psa.getTag(); tag = psa.getTag();
date = psa.getGranted(); date = psa.getGranted();
@@ -765,7 +784,7 @@ public class ChangeJson {
// Either the user cannot vote on this label, or they were added as a // Either the user cannot vote on this label, or they were added as a
// reviewer but have not responded yet. Explicitly check whether the // reviewer but have not responded yet. Explicitly check whether the
// user can vote on this label. // user can vote on this label.
value = labelNormalizer.canVote(ctl, lt, accountId) ? 0 : null; value = perm.test(new LabelPermission(lt)) ? 0 : null;
} }
addApproval( addApproval(
e.getValue().label(), approvalInfo(accountId, value, permittedVotingRange, tag, date)); e.getValue().label(), approvalInfo(accountId, value, permittedVotingRange, tag, date));
@@ -813,12 +832,13 @@ public class ChangeJson {
} }
private Map<String, LabelWithStatus> labelsForClosedChange( private Map<String, LabelWithStatus> labelsForClosedChange(
PermissionBackend.ForChange basePerm,
ChangeControl baseCtrl, ChangeControl baseCtrl,
ChangeData cd, ChangeData cd,
LabelTypes labelTypes, LabelTypes labelTypes,
boolean standard, boolean standard,
boolean detailed) boolean detailed)
throws OrmException { throws OrmException, PermissionBackendException {
Set<Account.Id> allUsers = new HashSet<>(); Set<Account.Id> allUsers = new HashSet<>();
if (detailed) { if (detailed) {
// Users expect to see all reviewers on closed changes, even if they // Users expect to see all reviewers on closed changes, even if they
@@ -886,8 +906,10 @@ public class ChangeJson {
Map<String, ApprovalInfo> byLabel = Maps.newHashMapWithExpectedSize(labels.size()); Map<String, ApprovalInfo> byLabel = Maps.newHashMapWithExpectedSize(labels.size());
Map<String, VotingRangeInfo> pvr = Collections.emptyMap(); Map<String, VotingRangeInfo> pvr = Collections.emptyMap();
if (detailed) { if (detailed) {
ChangeControl ctl = baseCtrl.forUser(userFactory.create(accountId)); IdentifiedUser user = userFactory.create(accountId);
pvr = getPermittedVotingRanges(permittedLabels(ctl, cd)); PermissionBackend.ForChange perm = basePerm.user(user);
ChangeControl ctl = baseCtrl.forUser(user);
pvr = getPermittedVotingRanges(permittedLabels(perm, ctl, cd));
for (Map.Entry<String, LabelWithStatus> entry : labels.entrySet()) { for (Map.Entry<String, LabelWithStatus> entry : labels.entrySet()) {
ApprovalInfo ai = approvalInfo(accountId, 0, null, null, null); ApprovalInfo ai = approvalInfo(accountId, 0, null, null, null);
byLabel.put(entry.getKey(), ai); byLabel.put(entry.getKey(), ai);
@@ -961,15 +983,29 @@ public class ChangeJson {
} }
} }
private Map<String, Collection<String>> permittedLabels(ChangeControl ctl, ChangeData cd) private Map<String, Collection<String>> permittedLabels(
throws OrmException { PermissionBackend.ForChange perm, ChangeControl ctl, ChangeData cd)
throws OrmException, PermissionBackendException {
if (ctl == null || !ctl.getUser().isIdentifiedUser()) { if (ctl == null || !ctl.getUser().isIdentifiedUser()) {
return null; return null;
} }
Map<String, Short> labels = null; boolean isMerged = cd.change().getStatus() == Change.Status.MERGED;
boolean isMerged = ctl.getChange().getStatus() == Change.Status.MERGED;
LabelTypes labelTypes = ctl.getLabelTypes(); LabelTypes labelTypes = ctl.getLabelTypes();
Map<String, LabelType> toCheck = new HashMap<>();
for (SubmitRecord rec : submitRecords(cd)) {
if (rec.labels != null) {
for (SubmitRecord.Label r : rec.labels) {
LabelType type = labelTypes.byLabel(r.label);
if (type != null && (!isMerged || type.allowPostSubmit())) {
toCheck.put(type.getName(), type);
}
}
}
}
Map<String, Short> labels = null;
Set<LabelPermission.WithValue> can = perm.testLabels(toCheck.values());
SetMultimap<String, String> permitted = LinkedHashMultimap.create(); SetMultimap<String, String> permitted = LinkedHashMultimap.create();
for (SubmitRecord rec : submitRecords(cd)) { for (SubmitRecord rec : submitRecords(cd)) {
if (rec.labels == null) { if (rec.labels == null) {
@@ -980,9 +1016,9 @@ public class ChangeJson {
if (type == null || (isMerged && !type.allowPostSubmit())) { if (type == null || (isMerged && !type.allowPostSubmit())) {
continue; continue;
} }
PermissionRange range = ctl.getRange(Permission.forLabel(r.label));
for (LabelValue v : type.getValues()) { for (LabelValue v : type.getValues()) {
boolean ok = range.contains(v.getValue()); boolean ok = can.contains(new LabelPermission.WithValue(type, v));
if (isMerged) { if (isMerged) {
if (labels == null) { if (labels == null) {
labels = currentLabels(ctl); labels = currentLabels(ctl);
@@ -996,6 +1032,7 @@ public class ChangeJson {
} }
} }
} }
List<String> toClear = Lists.newArrayListWithCapacity(permitted.keySet().size()); List<String> toClear = Lists.newArrayListWithCapacity(permitted.keySet().size());
for (Map.Entry<String, Collection<String>> e : permitted.asMap().entrySet()) { for (Map.Entry<String, Collection<String>> e : permitted.asMap().entrySet()) {
if (isOnlyZero(e.getValue())) { if (isOnlyZero(e.getValue())) {

View File

@@ -16,6 +16,7 @@ package com.google.gerrit.server.change;
import com.google.gerrit.extensions.api.changes.ReviewerInfo; import com.google.gerrit.extensions.api.changes.ReviewerInfo;
import com.google.gerrit.extensions.restapi.RestReadView; import com.google.gerrit.extensions.restapi.RestReadView;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject; import com.google.inject.Inject;
import com.google.inject.Singleton; import com.google.inject.Singleton;
@@ -31,7 +32,8 @@ public class GetReviewer implements RestReadView<ReviewerResource> {
} }
@Override @Override
public List<ReviewerInfo> apply(ReviewerResource rsrc) throws OrmException { public List<ReviewerInfo> apply(ReviewerResource rsrc)
throws OrmException, PermissionBackendException {
return json.format(rsrc); return json.format(rsrc);
} }
} }

View File

@@ -20,6 +20,7 @@ import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil; import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.mail.Address; import com.google.gerrit.server.mail.Address;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject; import com.google.inject.Inject;
import com.google.inject.Provider; import com.google.inject.Provider;
@@ -48,7 +49,8 @@ class ListReviewers implements RestReadView<ChangeResource> {
} }
@Override @Override
public List<ReviewerInfo> apply(ChangeResource rsrc) throws OrmException { public List<ReviewerInfo> apply(ChangeResource rsrc)
throws OrmException, PermissionBackendException {
Map<String, ReviewerResource> reviewers = new LinkedHashMap<>(); Map<String, ReviewerResource> reviewers = new LinkedHashMap<>();
ReviewDb db = dbProvider.get(); ReviewDb db = dbProvider.get();
for (Account.Id accountId : approvalsUtil.getReviewers(db, rsrc.getNotes()).all()) { for (Account.Id accountId : approvalsUtil.getReviewers(db, rsrc.getNotes()).all()) {

View File

@@ -21,6 +21,7 @@ import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil; import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.mail.Address; import com.google.gerrit.server.mail.Address;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject; import com.google.inject.Inject;
import com.google.inject.Provider; import com.google.inject.Provider;
@@ -50,7 +51,7 @@ class ListRevisionReviewers implements RestReadView<RevisionResource> {
@Override @Override
public List<ReviewerInfo> apply(RevisionResource rsrc) public List<ReviewerInfo> apply(RevisionResource rsrc)
throws OrmException, MethodNotAllowedException { throws OrmException, MethodNotAllowedException, PermissionBackendException {
if (!rsrc.isCurrent()) { if (!rsrc.isCurrent()) {
throw new MethodNotAllowedException("Cannot list reviewers on non-current patch set"); throw new MethodNotAllowedException("Cannot list reviewers on non-current patch set");
} }

View File

@@ -54,6 +54,8 @@ import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.mail.Address; import com.google.gerrit.server.mail.Address;
import com.google.gerrit.server.mail.send.OutgoingEmailValidator; import com.google.gerrit.server.mail.send.OutgoingEmailValidator;
import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectCache;
@@ -79,6 +81,7 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
private final AccountsCollection accounts; private final AccountsCollection accounts;
private final ReviewerResource.Factory reviewerFactory; private final ReviewerResource.Factory reviewerFactory;
private final PermissionBackend permissionBackend;
private final GroupsCollection groupsCollection; private final GroupsCollection groupsCollection;
private final GroupMembers.Factory groupMembersFactory; private final GroupMembers.Factory groupMembersFactory;
@@ -98,6 +101,7 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
PostReviewers( PostReviewers(
AccountsCollection accounts, AccountsCollection accounts,
ReviewerResource.Factory reviewerFactory, ReviewerResource.Factory reviewerFactory,
PermissionBackend permissionBackend,
GroupsCollection groupsCollection, GroupsCollection groupsCollection,
GroupMembers.Factory groupMembersFactory, GroupMembers.Factory groupMembersFactory,
AccountLoader.Factory accountLoaderFactory, AccountLoader.Factory accountLoaderFactory,
@@ -113,6 +117,7 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
PostReviewersOp.Factory postReviewersOpFactory) { PostReviewersOp.Factory postReviewersOpFactory) {
this.accounts = accounts; this.accounts = accounts;
this.reviewerFactory = reviewerFactory; this.reviewerFactory = reviewerFactory;
this.permissionBackend = permissionBackend;
this.groupsCollection = groupsCollection; this.groupsCollection = groupsCollection;
this.groupMembersFactory = groupMembersFactory; this.groupMembersFactory = groupMembersFactory;
this.accountLoaderFactory = accountLoaderFactory; this.accountLoaderFactory = accountLoaderFactory;
@@ -130,7 +135,8 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
@Override @Override
public AddReviewerResult apply(ChangeResource rsrc, AddReviewerInput input) public AddReviewerResult apply(ChangeResource rsrc, AddReviewerInput input)
throws IOException, OrmException, RestApiException, UpdateException { throws IOException, OrmException, RestApiException, UpdateException,
PermissionBackendException {
if (input.reviewer == null) { if (input.reviewer == null) {
throw new BadRequestException("missing reviewer field"); throw new BadRequestException("missing reviewer field");
} }
@@ -398,14 +404,17 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
rsrc, this.reviewers, this.reviewersByEmail, state, notify, accountsToNotify); rsrc, this.reviewers, this.reviewersByEmail, state, notify, accountsToNotify);
} }
void gatherResults() throws OrmException { void gatherResults() throws OrmException, PermissionBackendException {
// Generate result details and fill AccountLoader. This occurs outside // Generate result details and fill AccountLoader. This occurs outside
// the Op because the accounts are in a different table. // the Op because the accounts are in a different table.
PostReviewersOp.Result opResult = op.getResult(); PostReviewersOp.Result opResult = op.getResult();
if (migration.readChanges() && state == CC) { if (migration.readChanges() && state == CC) {
result.ccs = Lists.newArrayListWithCapacity(opResult.addedCCs().size()); result.ccs = Lists.newArrayListWithCapacity(opResult.addedCCs().size());
for (Account.Id accountId : opResult.addedCCs()) { for (Account.Id accountId : opResult.addedCCs()) {
result.ccs.add(json.format(new ReviewerInfo(accountId.get()), reviewers.get(accountId))); ChangeControl ctl = reviewers.get(accountId);
PermissionBackend.ForChange perm =
permissionBackend.user(ctl.getUser()).database(dbProvider).change(ctl.getNotes());
result.ccs.add(json.format(new ReviewerInfo(accountId.get()), perm, ctl));
} }
accountLoaderFactory.create(true).fill(result.ccs); accountLoaderFactory.create(true).fill(result.ccs);
for (Address a : reviewersByEmail) { for (Address a : reviewersByEmail) {
@@ -415,11 +424,12 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
result.reviewers = Lists.newArrayListWithCapacity(opResult.addedReviewers().size()); result.reviewers = Lists.newArrayListWithCapacity(opResult.addedReviewers().size());
for (PatchSetApproval psa : opResult.addedReviewers()) { for (PatchSetApproval psa : opResult.addedReviewers()) {
// New reviewers have value 0, don't bother normalizing. // New reviewers have value 0, don't bother normalizing.
ChangeControl ctl = reviewers.get(psa.getAccountId());
PermissionBackend.ForChange perm =
permissionBackend.user(ctl.getUser()).database(dbProvider).change(ctl.getNotes());
result.reviewers.add( result.reviewers.add(
json.format( json.format(
new ReviewerInfo(psa.getAccountId().get()), new ReviewerInfo(psa.getAccountId().get()), perm, ctl, ImmutableList.of(psa)));
reviewers.get(psa.getAccountId()),
ImmutableList.of(psa)));
} }
accountLoaderFactory.create(true).fill(result.reviewers); accountLoaderFactory.create(true).fill(result.reviewers);
for (Address a : reviewersByEmail) { for (Address a : reviewersByEmail) {

View File

@@ -20,7 +20,6 @@ import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists; import com.google.common.collect.Lists;
import com.google.gerrit.common.data.LabelType; import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRange; import com.google.gerrit.common.data.PermissionRange;
import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.extensions.api.changes.ReviewerInfo; import com.google.gerrit.extensions.api.changes.ReviewerInfo;
@@ -30,6 +29,9 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil; import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.account.AccountLoader; import com.google.gerrit.server.account.AccountLoader;
import com.google.gerrit.server.permissions.LabelPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.SubmitRuleEvaluator; import com.google.gerrit.server.project.SubmitRuleEvaluator;
import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.ChangeData;
@@ -44,6 +46,7 @@ import java.util.TreeMap;
@Singleton @Singleton
public class ReviewerJson { public class ReviewerJson {
private final Provider<ReviewDb> db; private final Provider<ReviewDb> db;
private final PermissionBackend permissionBackend;
private final ChangeData.Factory changeDataFactory; private final ChangeData.Factory changeDataFactory;
private final ApprovalsUtil approvalsUtil; private final ApprovalsUtil approvalsUtil;
private final AccountLoader.Factory accountLoaderFactory; private final AccountLoader.Factory accountLoaderFactory;
@@ -51,22 +54,29 @@ public class ReviewerJson {
@Inject @Inject
ReviewerJson( ReviewerJson(
Provider<ReviewDb> db, Provider<ReviewDb> db,
PermissionBackend permissionBackend,
ChangeData.Factory changeDataFactory, ChangeData.Factory changeDataFactory,
ApprovalsUtil approvalsUtil, ApprovalsUtil approvalsUtil,
AccountLoader.Factory accountLoaderFactory) { AccountLoader.Factory accountLoaderFactory) {
this.db = db; this.db = db;
this.permissionBackend = permissionBackend;
this.changeDataFactory = changeDataFactory; this.changeDataFactory = changeDataFactory;
this.approvalsUtil = approvalsUtil; this.approvalsUtil = approvalsUtil;
this.accountLoaderFactory = accountLoaderFactory; this.accountLoaderFactory = accountLoaderFactory;
} }
public List<ReviewerInfo> format(Collection<ReviewerResource> rsrcs) throws OrmException { public List<ReviewerInfo> format(Collection<ReviewerResource> rsrcs)
throws OrmException, PermissionBackendException {
List<ReviewerInfo> infos = Lists.newArrayListWithCapacity(rsrcs.size()); List<ReviewerInfo> infos = Lists.newArrayListWithCapacity(rsrcs.size());
AccountLoader loader = accountLoaderFactory.create(true); AccountLoader loader = accountLoaderFactory.create(true);
for (ReviewerResource rsrc : rsrcs) { for (ReviewerResource rsrc : rsrcs) {
ReviewerInfo info = ReviewerInfo info =
format( format(
new ReviewerInfo(rsrc.getReviewerUser().getAccountId().get()), new ReviewerInfo(rsrc.getReviewerUser().getAccountId().get()),
permissionBackend
.user(rsrc.getReviewerUser())
.database(db)
.change(rsrc.getChangeResource().getNotes()),
rsrc.getReviewerControl()); rsrc.getReviewerControl());
loader.put(info); loader.put(info);
infos.add(info); infos.add(info);
@@ -75,21 +85,27 @@ public class ReviewerJson {
return infos; return infos;
} }
public List<ReviewerInfo> format(ReviewerResource rsrc) throws OrmException { public List<ReviewerInfo> format(ReviewerResource rsrc)
throws OrmException, PermissionBackendException {
return format(ImmutableList.<ReviewerResource>of(rsrc)); return format(ImmutableList.<ReviewerResource>of(rsrc));
} }
public ReviewerInfo format(ReviewerInfo out, ChangeControl ctl) throws OrmException { public ReviewerInfo format(ReviewerInfo out, PermissionBackend.ForChange perm, ChangeControl ctl)
throws OrmException, PermissionBackendException {
PatchSet.Id psId = ctl.getChange().currentPatchSetId(); PatchSet.Id psId = ctl.getChange().currentPatchSetId();
return format( return format(
out, out,
perm,
ctl, ctl,
approvalsUtil.byPatchSetUser(db.get(), ctl, psId, new Account.Id(out._accountId))); approvalsUtil.byPatchSetUser(db.get(), ctl, psId, new Account.Id(out._accountId)));
} }
public ReviewerInfo format( public ReviewerInfo format(
ReviewerInfo out, ChangeControl ctl, Iterable<PatchSetApproval> approvals) ReviewerInfo out,
throws OrmException { PermissionBackend.ForChange perm,
ChangeControl ctl,
Iterable<PatchSetApproval> approvals)
throws OrmException, PermissionBackendException {
LabelTypes labelTypes = ctl.getLabelTypes(); LabelTypes labelTypes = ctl.getLabelTypes();
// Don't use Maps.newTreeMap(Comparator) due to OpenJDK bug 100167. // Don't use Maps.newTreeMap(Comparator) due to OpenJDK bug 100167.
@@ -117,8 +133,10 @@ public class ReviewerJson {
} }
for (SubmitRecord.Label label : rec.labels) { for (SubmitRecord.Label label : rec.labels) {
String name = label.label; String name = label.label;
LabelType type = labelTypes.byLabel(name);
if (!out.approvals.containsKey(name) if (!out.approvals.containsKey(name)
&& !ctl.getRange(Permission.forLabel(name)).isEmpty()) { && type != null
&& perm.test(new LabelPermission(type))) {
out.approvals.put(name, formatValue((short) 0)); out.approvals.put(name, formatValue((short) 0));
} }
} }

View File

@@ -142,16 +142,6 @@ public class LabelNormalizer {
return Result.create(unchanged, updated, deleted); return Result.create(unchanged, updated, deleted);
} }
/**
* @param ctl change control (for any user).
* @param lt label type.
* @param id account ID.
* @return whether the given account ID has any permissions to vote on this label for this change.
*/
public boolean canVote(ChangeControl ctl, LabelType lt, Account.Id id) {
return !getRange(ctl, lt, id).isEmpty();
}
private PatchSetApproval copy(PatchSetApproval src) { private PatchSetApproval copy(PatchSetApproval src) {
return new PatchSetApproval(src.getPatchSetId(), src); return new PatchSetApproval(src.getPatchSetId(), src);
} }

View File

@@ -221,6 +221,19 @@ public abstract class PermissionBackend {
return test(valuesOf(checkNotNull(label, "LabelType"))); return test(valuesOf(checkNotNull(label, "LabelType")));
} }
/**
* Test which values of a group of labels the user may be able to set.
*
* @param types definition of the labels to test values of.
* @return set containing values the user may be able to use; may be empty if none.
* @throws PermissionBackendException if failure consulting backend configuration.
*/
public Set<LabelPermission.WithValue> testLabels(Collection<LabelType> types)
throws PermissionBackendException {
checkNotNull(types, "LabelType");
return test(types.stream().flatMap((t) -> valuesOf(t).stream()).collect(toSet()));
}
private static Set<LabelPermission.WithValue> valuesOf(LabelType label) { private static Set<LabelPermission.WithValue> valuesOf(LabelType label) {
return label return label
.getValues() .getValues()