From 30463ebcf819c0893cf64e316e002bfbc70fcdda Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Mon, 4 Sep 2017 16:38:16 +0200 Subject: [PATCH 1/2] Migrate ApprovalsUtil#checkApprovals() to PermissionBackend Change-Id: I6b10e93f06c833dfb7ac23c409e4ba22eab2f7f1 --- .../google/gerrit/server/ApprovalsUtil.java | 25 ++++++++++--------- .../gerrit/server/change/ChangeInserter.java | 4 +-- .../gerrit/server/git/receive/ReplaceOp.java | 5 ++-- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java index 7750729994..d858da5bb8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java @@ -31,8 +31,6 @@ import com.google.common.primitives.Shorts; import com.google.gerrit.common.Nullable; import com.google.gerrit.common.data.LabelType; 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.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.RestApiException; @@ -48,6 +46,7 @@ import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.notedb.ReviewerStateInternal; import com.google.gerrit.server.permissions.ChangePermission; +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; @@ -308,7 +307,7 @@ public class ApprovalsUtil { * @param update change update. * @param labelTypes label types for the containing project. * @param ps patch set being approved. - * @param changeCtl change control for user adding approvals. + * @param user user adding approvals. * @param approvals approvals to add. * @throws RestApiException * @throws OrmException @@ -318,10 +317,10 @@ public class ApprovalsUtil { ChangeUpdate update, LabelTypes labelTypes, PatchSet ps, - ChangeControl changeCtl, + CurrentUser user, Map approvals) - throws RestApiException, OrmException { - Account.Id accountId = changeCtl.getUser().getAccountId(); + throws RestApiException, OrmException, PermissionBackendException { + Account.Id accountId = user.getAccountId(); checkArgument( accountId.equals(ps.getUploader()), "expected user %s to match patch set uploader %s", @@ -330,12 +329,12 @@ public class ApprovalsUtil { if (approvals.isEmpty()) { return ImmutableList.of(); } - checkApprovals(approvals, changeCtl); + checkApprovals(approvals, permissionBackend.user(user).database(db).change(update.getNotes())); List cells = new ArrayList<>(approvals.size()); Date ts = update.getWhen(); for (Map.Entry vote : approvals.entrySet()) { LabelType lt = labelTypes.byLabel(vote.getKey()); - cells.add(newApproval(ps.getId(), changeCtl.getUser(), lt.getLabelId(), vote.getValue(), ts)); + cells.add(newApproval(ps.getId(), user, lt.getLabelId(), vote.getValue(), ts)); } for (PatchSetApproval psa : cells) { update.putApproval(psa.getLabel(), psa.getValue()); @@ -356,13 +355,15 @@ public class ApprovalsUtil { } } - private static void checkApprovals(Map approvals, ChangeControl changeCtl) - throws AuthException { + private static void checkApprovals( + Map approvals, PermissionBackend.ForChange forChange) + throws AuthException, PermissionBackendException { for (Map.Entry vote : approvals.entrySet()) { String name = vote.getKey(); Short value = vote.getValue(); - PermissionRange range = changeCtl.getRange(Permission.forLabel(name)); - if (range == null || !range.contains(value)) { + try { + forChange.check(new LabelPermission.WithValue(name, value)); + } catch (AuthException e) { throw new AuthException( String.format("applying label \"%s\": %d is restricted", name, value)); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java index 385baba1ab..e3e3e328b2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java @@ -378,7 +378,7 @@ public class ChangeInserter implements InsertChangeOp { @Override public boolean updateChange(ChangeContext ctx) - throws RestApiException, OrmException, IOException { + throws RestApiException, OrmException, IOException, PermissionBackendException { change = ctx.getChange(); // Use defensive copy created by ChangeControl. ReviewDb db = ctx.getDb(); ChangeControl ctl = ctx.getControl(); @@ -444,7 +444,7 @@ public class ChangeInserter implements InsertChangeOp { filterOnChangeVisibility(db, ctx.getNotes(), reviewersToAdd), Collections.emptySet()); approvalsUtil.addApprovalsForNewPatchSet( - db, update, labelTypes, patchSet, ctx.getControl(), approvals); + db, update, labelTypes, patchSet, ctx.getUser(), approvals); // Check if approvals are changing in with this update. If so, add current user to reviewers. // Note that this is done separately as addReviewers is filtering out the change owner as // reviewer which is needed in several other code paths. diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReplaceOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReplaceOp.java index 0187304a84..3399071b74 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReplaceOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/receive/ReplaceOp.java @@ -52,6 +52,7 @@ import com.google.gerrit.server.mail.MailUtil.MailRecipients; import com.google.gerrit.server.mail.send.ReplacePatchSetSender; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; +import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectControl; import com.google.gerrit.server.query.change.ChangeData; @@ -224,7 +225,7 @@ public class ReplaceOp implements BatchUpdateOp { @Override public boolean updateChange(ChangeContext ctx) - throws RestApiException, OrmException, IOException { + throws RestApiException, OrmException, IOException, PermissionBackendException { notes = ctx.getNotes(); Change change = notes.getChange(); if (change == null || change.getStatus().isClosed()) { @@ -306,7 +307,7 @@ public class ReplaceOp implements BatchUpdateOp { update, projectControl.getProjectState().getLabelTypes(), newPatchSet, - ctx.getControl(), + ctx.getUser(), approvals); approvalCopier.copyInReviewDb( ctx.getDb(), From 66d0b56c41968f01aa2996fa643335c121c88027 Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Thu, 7 Sep 2017 09:30:19 +0200 Subject: [PATCH 2/2] Make ChangeControl#getRange private and migrate caller The only caller of ChangeControl#getRange can be migrated to use PermissionBackend instead by splitting up the call into a call to determine if the user is allowed to vote on the label at all and a follow up call to squash the vote to a be within allowed bounds. Change-Id: Idf06a87540a402cfd3238511d38440ae8fff75f0 --- .../google/gerrit/server/ApprovalCopier.java | 3 +- .../gerrit/server/git/LabelNormalizer.java | 47 ++++++++++++------- .../server/git/strategy/SubmitStrategyOp.java | 5 +- .../gerrit/server/project/ChangeControl.java | 2 +- 4 files changed, 35 insertions(+), 22 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java index 6771616459..5e2f045734 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java @@ -30,6 +30,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.change.ChangeKindCache; import com.google.gerrit.server.git.LabelNormalizer; import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; +import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectState; @@ -204,7 +205,7 @@ public class ApprovalCopier { } } return labelNormalizer.normalize(ctl, byUser.values()).getNormalized(); - } catch (IOException e) { + } catch (IOException | PermissionBackendException e) { throw new OrmException(e); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java index a3f50932fd..2eeed242b5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java @@ -24,13 +24,15 @@ import com.google.common.collect.Lists; import com.google.gerrit.common.data.LabelType; import com.google.gerrit.common.data.LabelTypes; 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.reviewdb.client.Account; +import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSetApproval; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.notedb.ChangeNotes; +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.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -76,15 +78,18 @@ public class LabelNormalizer { private final Provider db; private final ChangeControl.GenericFactory changeFactory; private final IdentifiedUser.GenericFactory userFactory; + private final PermissionBackend permissionBackend; @Inject LabelNormalizer( Provider db, ChangeControl.GenericFactory changeFactory, - IdentifiedUser.GenericFactory userFactory) { + IdentifiedUser.GenericFactory userFactory, + PermissionBackend permissionBackend) { this.db = db; this.changeFactory = changeFactory; this.userFactory = userFactory; + this.permissionBackend = permissionBackend; } /** @@ -96,7 +101,7 @@ public class LabelNormalizer { * @throws OrmException */ public Result normalize(Change change, Collection approvals) - throws OrmException { + throws OrmException, PermissionBackendException { IdentifiedUser user = userFactory.create(change.getOwner()); return normalize(changeFactory.controlFor(db.get(), change, user), approvals); } @@ -108,7 +113,8 @@ public class LabelNormalizer { * for the user. Approvals for unknown labels are not included in the output, nor are * approvals where the user has no permissions for that label. */ - public Result normalize(ChangeControl ctl, Collection approvals) { + public Result normalize(ChangeControl ctl, Collection approvals) + throws PermissionBackendException { List unchanged = Lists.newArrayListWithCapacity(approvals.size()); List updated = Lists.newArrayListWithCapacity(approvals.size()); List deleted = Lists.newArrayListWithCapacity(approvals.size()); @@ -132,7 +138,7 @@ public class LabelNormalizer { } PatchSetApproval copy = copy(psa); applyTypeFloor(label, copy); - if (!applyRightFloor(ctl, label, copy)) { + if (!applyRightFloor(ctl.getNotes(), label, copy)) { deleted.add(psa); } else if (copy.getValue() != psa.getValue()) { updated.add(copy); @@ -147,19 +153,24 @@ public class LabelNormalizer { return new PatchSetApproval(src.getPatchSetId(), src); } - private PermissionRange getRange(ChangeControl ctl, LabelType lt, Account.Id id) { - String permission = Permission.forLabel(lt.getName()); - IdentifiedUser user = userFactory.create(id); - return ctl.forUser(user).getRange(permission); - } - - private boolean applyRightFloor(ChangeControl ctl, LabelType lt, PatchSetApproval a) { - PermissionRange range = getRange(ctl, lt, a.getAccountId()); - if (range.isEmpty()) { + private boolean applyRightFloor(ChangeNotes notes, LabelType lt, PatchSetApproval a) + throws PermissionBackendException { + PermissionBackend.ForChange forChange = + permissionBackend.user(userFactory.create(a.getAccountId())).database(db).change(notes); + // Check if the user is allowed to vote on the label at all + try { + forChange.check(new LabelPermission(lt.getName())); + } catch (AuthException e) { return false; } - a.setValue((short) range.squash(a.getValue())); - return true; + // Squash vote to nearest allowed value + try { + forChange.check(new LabelPermission.WithValue(lt.getName(), a.getValue())); + return true; + } catch (AuthException e) { + a.setValue(forChange.squashThenCheck(lt, a.getValue())); + return true; + } } private void applyTypeFloor(LabelType lt, PatchSetApproval a) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java index a3163c3309..561a0d3114 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategyOp.java @@ -45,6 +45,7 @@ import com.google.gerrit.server.git.MergeUtil; import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.git.SubmoduleException; import com.google.gerrit.server.notedb.ChangeUpdate; +import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.project.ProjectState; import com.google.gerrit.server.update.BatchUpdateOp; import com.google.gerrit.server.update.ChangeContext; @@ -330,7 +331,7 @@ abstract class SubmitStrategyOp implements BatchUpdateOp { } private void setApproval(ChangeContext ctx, IdentifiedUser user) - throws OrmException, IOException { + throws OrmException, IOException, PermissionBackendException { Change.Id id = ctx.getChange().getId(); List records = args.commitStatus.getSubmitRecords(id); PatchSet.Id oldPsId = toMerge.getPatchsetId(); @@ -352,7 +353,7 @@ abstract class SubmitStrategyOp implements BatchUpdateOp { } private LabelNormalizer.Result approve(ChangeContext ctx, ChangeUpdate update) - throws OrmException, IOException { + throws OrmException, IOException, PermissionBackendException { PatchSet.Id psId = update.getPatchSetId(); Map byKey = new HashMap<>(); for (PatchSetApproval psa : diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java index b9649ed021..4c386617a5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java @@ -275,7 +275,7 @@ public class ChangeControl { } /** The range of permitted values associated with a label permission. */ - public PermissionRange getRange(String permission) { + private PermissionRange getRange(String permission) { return getRefControl().getRange(permission, isOwner()); }