From 1e964ead8bf8a320fdd7b09801b6bee4246ab9aa Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Fri, 28 Apr 2017 17:29:03 +0200 Subject: [PATCH 1/4] Push option bypass-review disables validation Add RefPermission BYPASS_REVIEW which can be selectively requested with a push option, for example: git push -o bypass-review origin master When set this option skips validating the new commits if the user has BYPASS_REVIEW permission in the PermissionBackend. For the default backend this is a set of permissions that must all be present in order to use this option. Gerrit still ignores the bypass-review option on refs/meta/config (as it must validate the configuration parses), and on the refs/for/ magic branch, where it needs to update code reviews. Change-Id: I80ad4785226de7113f0bf6812c7709ba2c6ec73c --- .../gerrit/server/git/ReceiveCommits.java | 36 ++++++++++++------- .../server/permissions/RefPermission.java | 1 + .../gerrit/server/project/RefControl.java | 7 ++++ 3 files changed, 32 insertions(+), 12 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index b7c017c158..acfecf6ff3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -188,6 +188,7 @@ import org.slf4j.LoggerFactory; /** Receives change upload using the Git receive-pack protocol. */ public class ReceiveCommits { private static final Logger log = LoggerFactory.getLogger(ReceiveCommits.class); + private static final String BYPASS_REVIEW = "bypass-review"; public static final Pattern NEW_PATCHSET = Pattern.compile("^" + REFS_CHANGES + "(?:[0-9][0-9]/)?([1-9][0-9]*)(?:/new)?$"); @@ -1051,7 +1052,7 @@ public class ReceiveCommits { } } - private void parseCreate(ReceiveCommand cmd) { + private void parseCreate(ReceiveCommand cmd) throws PermissionBackendException { RevObject obj; try { obj = rp.getRevWalk().parseAny(cmd.getNewId()); @@ -1069,7 +1070,14 @@ public class ReceiveCommits { } RefControl ctl = projectControl.controlForRef(cmd.getRefName()); - if (ctl.canCreate(db, rp.getRepository(), obj)) { + boolean ok; + try { + permissions.ref(cmd.getRefName()).check(RefPermission.CREATE); + ok = true; + } catch (AuthException err) { + ok = false; + } + if (ok && ctl.canCreate(db, rp.getRepository(), obj)) { if (!validRefOperation(cmd)) { return; } @@ -2704,17 +2712,21 @@ public class ReceiveCommits { return true; } - private void validateNewCommits(RefControl ctl, ReceiveCommand cmd) { - if (ctl.canForgeAuthor() - && ctl.canForgeCommitter() - && ctl.canForgeGerritServerIdentity() - && ctl.canUploadMerges() - && !projectControl.getProjectState().isUseSignedOffBy() - && Iterables.isEmpty(rejectCommits) - && !RefNames.REFS_CONFIG.equals(ctl.getRefName()) + private void validateNewCommits(RefControl ctl, ReceiveCommand cmd) + throws PermissionBackendException { + if (!RefNames.REFS_CONFIG.equals(cmd.getRefName()) && !(MagicBranch.isMagicBranch(cmd.getRefName()) - || NEW_PATCHSET.matcher(cmd.getRefName()).matches())) { - logDebug("Short-circuiting new commit validation"); + || NEW_PATCHSET.matcher(cmd.getRefName()).matches()) + && pushOptions.containsKey(BYPASS_REVIEW)) { + try { + permissions.ref(cmd.getRefName()).check(RefPermission.BYPASS_REVIEW); + if (!Iterables.isEmpty(rejectCommits)) { + throw new AuthException("reject-commits prevents " + BYPASS_REVIEW); + } + logDebug("Short-circuiting new commit validation"); + } catch (AuthException denied) { + reject(cmd, denied.getMessage()); + } return; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/RefPermission.java b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/RefPermission.java index 37744b034f..127603fc12 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/RefPermission.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/RefPermission.java @@ -28,6 +28,7 @@ public enum RefPermission { FORGE_AUTHOR(Permission.FORGE_AUTHOR), FORGE_COMMITTER(Permission.FORGE_COMMITTER), FORGE_SERVER(Permission.FORGE_SERVER), + BYPASS_REVIEW, CREATE_CHANGE; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java index bf53ab19c9..6771ad6da4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java @@ -738,6 +738,13 @@ public class RefControl { return canForgeGerritServerIdentity(); case CREATE_CHANGE: return canUpload(); + + case BYPASS_REVIEW: + return canForgeAuthor() + && canForgeCommitter() + && canForgeGerritServerIdentity() + && canUploadMerges() + && !projectControl.getProjectState().isUseSignedOffBy(); } throw new PermissionBackendException(perm + " unsupported"); } From e2923f43abd81faa9f5dc54e159fe50aa912224c Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Fri, 28 Apr 2017 18:12:18 +0200 Subject: [PATCH 2/4] Convert forge author, committer, server to PermissionBackend Change-Id: I081392237957d3dc4afc703ca2256e27954d3774 --- .../gerrit/server/change/ChangeInserter.java | 8 +- .../server/change/PatchSetInserter.java | 5 +- .../gerrit/server/git/ReceiveCommits.java | 22 +- .../validators/CommitValidationException.java | 5 + .../git/validators/CommitValidators.java | 195 +++++++++++------- .../gerrit/server/project/RefControl.java | 6 +- 6 files changed, 151 insertions(+), 90 deletions(-) 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 80d644f0fb..6a692d7e94 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 @@ -53,6 +53,7 @@ import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.patch.PatchSetInfoFactory; +import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.ProjectControl; @@ -90,6 +91,7 @@ public class ChangeInserter implements InsertChangeOp { private static final Logger log = LoggerFactory.getLogger(ChangeInserter.class); + private final PermissionBackend permissionBackend; private final ProjectControl.GenericFactory projectControlFactory; private final IdentifiedUser.GenericFactory userFactory; private final ChangeControl.GenericFactory changeControlFactory; @@ -138,6 +140,7 @@ public class ChangeInserter implements InsertChangeOp { @Inject ChangeInserter( + PermissionBackend permissionBackend, ProjectControl.GenericFactory projectControlFactory, IdentifiedUser.GenericFactory userFactory, ChangeControl.GenericFactory changeControlFactory, @@ -154,6 +157,7 @@ public class ChangeInserter implements InsertChangeOp { @Assisted Change.Id changeId, @Assisted ObjectId commitId, @Assisted String refName) { + this.permissionBackend = permissionBackend; this.projectControlFactory = projectControlFactory; this.userFactory = userFactory; this.changeControlFactory = changeControlFactory; @@ -543,6 +547,8 @@ public class ChangeInserter implements InsertChangeOp { return; } + PermissionBackend.ForRef perm = + permissionBackend.user(ctx.getUser()).project(ctx.getProject()).ref(refName); try { RefControl refControl = projectControlFactory.controlFor(ctx.getProject(), ctx.getUser()).controlForRef(refName); @@ -555,7 +561,7 @@ public class ChangeInserter implements InsertChangeOp { commitId, ctx.getIdentifiedUser())) { commitValidatorsFactory - .forGerritCommits(refControl, new NoSshInfo(), ctx.getRevWalk()) + .forGerritCommits(perm, refControl, new NoSshInfo(), ctx.getRevWalk()) .validate(event); } } catch (CommitValidationException e) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java index 581f2bae35..a5fe978945 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java @@ -320,6 +320,9 @@ public class PatchSetInserter implements BatchUpdateOp { return; } + PermissionBackend.ForRef perm = + permissionBackend.user(ctx.getUser()).ref(origCtl.getChange().getDest()); + String refName = getPatchSetId().toRefName(); try (CommitReceivedEvent event = new CommitReceivedEvent( @@ -333,7 +336,7 @@ public class PatchSetInserter implements BatchUpdateOp { commitId, ctx.getIdentifiedUser())) { commitValidatorsFactory - .forGerritCommits(origCtl.getRefControl(), new NoSshInfo(), ctx.getRevWalk()) + .forGerritCommits(perm, origCtl.getRefControl(), new NoSshInfo(), ctx.getRevWalk()) .validate(event); } catch (CommitValidationException e) { throw new ResourceConflictException(e.getFullMessage()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index acfecf6ff3..3ed3a1887d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -112,7 +112,6 @@ import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.permissions.ProjectPermission; import com.google.gerrit.server.permissions.RefPermission; -import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectControl; @@ -1210,6 +1209,7 @@ public class ReceiveCommits { private final boolean defaultPublishComments; Branch.NameKey dest; RefControl ctl; + PermissionBackend.ForRef perm; Set reviewer = Sets.newLinkedHashSet(); Set cc = Sets.newLinkedHashSet(); Map labels = new HashMap<>(); @@ -1496,6 +1496,7 @@ public class ReceiveCommits { magicBranch.dest = new Branch.NameKey(project.getNameKey(), ref); magicBranch.ctl = projectControl.controlForRef(ref); + magicBranch.perm = permissions.ref(ref); if (projectControl.getProject().getState() != com.google.gerrit.extensions.client.ProjectState.ACTIVE) { reject(cmd, "project is read only"); @@ -1837,7 +1838,7 @@ public class ReceiveCommits { logDebug("Creating new change for {} even though it is already tracked", name); } - if (!validCommit(rp.getRevWalk(), magicBranch.ctl, magicBranch.cmd, c)) { + if (!validCommit(rp.getRevWalk(), magicBranch.perm, magicBranch.ctl, magicBranch.cmd, c)) { // Not a change the user can propose? Abort as early as possible. newChanges = Collections.emptyList(); logDebug("Aborting early due to invalid commit"); @@ -2415,8 +2416,9 @@ public class ReceiveCommits { } } - ChangeControl changeCtl = projectControl.controlFor(notes); - if (!validCommit(rp.getRevWalk(), changeCtl.getRefControl(), inputCommand, newCommit)) { + PermissionBackend.ForRef perm = permissions.ref(change.getDest().get()); + RefControl refctl = projectControl.controlForRef(change.getDest()); + if (!validCommit(rp.getRevWalk(), perm, refctl, inputCommand, newCommit)) { return false; } rp.getRevWalk().parseBody(priorCommit); @@ -2714,12 +2716,13 @@ public class ReceiveCommits { private void validateNewCommits(RefControl ctl, ReceiveCommand cmd) throws PermissionBackendException { + PermissionBackend.ForRef perm = permissions.ref(ctl.getRefName()); if (!RefNames.REFS_CONFIG.equals(cmd.getRefName()) && !(MagicBranch.isMagicBranch(cmd.getRefName()) || NEW_PATCHSET.matcher(cmd.getRefName()).matches()) && pushOptions.containsKey(BYPASS_REVIEW)) { try { - permissions.ref(cmd.getRefName()).check(RefPermission.BYPASS_REVIEW); + perm.check(RefPermission.BYPASS_REVIEW); if (!Iterables.isEmpty(rejectCommits)) { throw new AuthException("reject-commits prevents " + BYPASS_REVIEW); } @@ -2747,7 +2750,7 @@ public class ReceiveCommits { i++; if (existing.keySet().contains(c)) { continue; - } else if (!validCommit(walk, ctl, cmd, c)) { + } else if (!validCommit(walk, perm, ctl, cmd, c)) { break; } @@ -2773,7 +2776,8 @@ public class ReceiveCommits { } } - private boolean validCommit(RevWalk rw, RefControl ctl, ReceiveCommand cmd, ObjectId id) + private boolean validCommit( + RevWalk rw, PermissionBackend.ForRef perm, RefControl ctl, ReceiveCommand cmd, ObjectId id) throws IOException { if (validCommits.contains(id)) { @@ -2791,8 +2795,8 @@ public class ReceiveCommits { && magicBranch.merged; CommitValidators validators = isMerged - ? commitValidatorsFactory.forMergedCommits(ctl) - : commitValidatorsFactory.forReceiveCommits(ctl, sshInfo, repo, rw); + ? commitValidatorsFactory.forMergedCommits(perm, ctl) + : commitValidatorsFactory.forReceiveCommits(perm, ctl, sshInfo, repo, rw); messages.addAll(validators.validate(receiveEvent)); } catch (CommitValidationException e) { logDebug("Commit validation failed on {}", c.name()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidationException.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidationException.java index 07f3b218a7..bffe382d89 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidationException.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidationException.java @@ -22,6 +22,11 @@ public class CommitValidationException extends ValidationException { private static final long serialVersionUID = 1L; private final ImmutableList messages; + public CommitValidationException(String reason, CommitValidationMessage message) { + super(reason); + this.messages = ImmutableList.of(message); + } + public CommitValidationException(String reason, List messages) { super(reason); this.messages = ImmutableList.copyOf(messages); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java index c086f1cf7b..c2b5947155 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/CommitValidators.java @@ -26,6 +26,7 @@ import com.google.gerrit.common.Nullable; import com.google.gerrit.common.PageLinks; import com.google.gerrit.extensions.api.config.ConsistencyCheckInfo.ConsistencyProblemInfo; import com.google.gerrit.extensions.registration.DynamicSet; +import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.GerritPersonIdent; @@ -39,7 +40,11 @@ import com.google.gerrit.server.events.CommitReceivedEvent; import com.google.gerrit.server.git.BanCommit; import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.git.ValidationError; +import com.google.gerrit.server.permissions.PermissionBackend; +import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.gerrit.server.permissions.RefPermission; import com.google.gerrit.server.project.ProjectControl; +import com.google.gerrit.server.project.ProjectState; import com.google.gerrit.server.project.RefControl; import com.google.gerrit.server.ssh.SshInfo; import com.google.gerrit.server.util.MagicBranch; @@ -96,38 +101,45 @@ public class CommitValidators { } public CommitValidators forReceiveCommits( - RefControl refControl, SshInfo sshInfo, Repository repo, RevWalk rw) throws IOException { + PermissionBackend.ForRef perm, + RefControl refctl, + SshInfo sshInfo, + Repository repo, + RevWalk rw) + throws IOException { NoteMap rejectCommits = BanCommit.loadRejectCommitsMap(repo, rw); + IdentifiedUser user = refctl.getUser().asIdentifiedUser(); return new CommitValidators( ImmutableList.of( - new UploadMergesPermissionValidator(refControl), - new AmendedGerritMergeCommitValidationListener(refControl, gerritIdent), - new AuthorUploaderValidator(refControl, canonicalWebUrl), - new CommitterUploaderValidator(refControl, canonicalWebUrl), - new SignedOffByValidator(refControl), - new ChangeIdValidator( - refControl, canonicalWebUrl, installCommitMsgHookCommand, sshInfo), - new ConfigValidator(refControl, rw, allUsers), + new UploadMergesPermissionValidator(refctl), + new AmendedGerritMergeCommitValidationListener(perm, gerritIdent), + new AuthorUploaderValidator(user, perm, canonicalWebUrl), + new CommitterUploaderValidator(user, perm, canonicalWebUrl), + new SignedOffByValidator(user, perm, refctl.getProjectControl().getProjectState()), + new ChangeIdValidator(refctl, canonicalWebUrl, installCommitMsgHookCommand, sshInfo), + new ConfigValidator(refctl, rw, allUsers), new BannedCommitsValidator(rejectCommits), new PluginCommitValidationListener(pluginValidators), new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker))); } - public CommitValidators forGerritCommits(RefControl refControl, SshInfo sshInfo, RevWalk rw) { + public CommitValidators forGerritCommits( + PermissionBackend.ForRef perm, RefControl refctl, SshInfo sshInfo, RevWalk rw) { + IdentifiedUser user = refctl.getUser().asIdentifiedUser(); return new CommitValidators( ImmutableList.of( - new UploadMergesPermissionValidator(refControl), - new AmendedGerritMergeCommitValidationListener(refControl, gerritIdent), - new AuthorUploaderValidator(refControl, canonicalWebUrl), - new SignedOffByValidator(refControl), - new ChangeIdValidator( - refControl, canonicalWebUrl, installCommitMsgHookCommand, sshInfo), - new ConfigValidator(refControl, rw, allUsers), + new UploadMergesPermissionValidator(refctl), + new AmendedGerritMergeCommitValidationListener(perm, gerritIdent), + new AuthorUploaderValidator(user, perm, canonicalWebUrl), + new SignedOffByValidator(user, perm, refctl.getProjectControl().getProjectState()), + new ChangeIdValidator(refctl, canonicalWebUrl, installCommitMsgHookCommand, sshInfo), + new ConfigValidator(refctl, rw, allUsers), new PluginCommitValidationListener(pluginValidators), new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker))); } - public CommitValidators forMergedCommits(RefControl refControl) { + public CommitValidators forMergedCommits(PermissionBackend.ForRef perm, RefControl refControl) { + IdentifiedUser user = refControl.getUser().asIdentifiedUser(); // Generally only include validators that are based on permissions of the // user creating a change for a merged commit; generally exclude // validators that would require amending the change in order to correct. @@ -144,8 +156,8 @@ public class CommitValidators { return new CommitValidators( ImmutableList.of( new UploadMergesPermissionValidator(refControl), - new AuthorUploaderValidator(refControl, canonicalWebUrl), - new CommitterUploaderValidator(refControl, canonicalWebUrl))); + new AuthorUploaderValidator(user, perm, canonicalWebUrl), + new CommitterUploaderValidator(user, perm, canonicalWebUrl))); } } @@ -441,37 +453,50 @@ public class CommitValidators { } public static class SignedOffByValidator implements CommitValidationListener { - private final RefControl refControl; + private final IdentifiedUser user; + private final PermissionBackend.ForRef perm; + private final ProjectState state; - public SignedOffByValidator(RefControl refControl) { - this.refControl = refControl; + public SignedOffByValidator( + IdentifiedUser user, PermissionBackend.ForRef perm, ProjectState state) { + this.user = user; + this.perm = perm; + this.state = state; } @Override public List onCommitReceived(CommitReceivedEvent receiveEvent) throws CommitValidationException { - IdentifiedUser currentUser = refControl.getUser().asIdentifiedUser(); - final PersonIdent committer = receiveEvent.commit.getCommitterIdent(); - final PersonIdent author = receiveEvent.commit.getAuthorIdent(); - final ProjectControl projectControl = refControl.getProjectControl(); + if (!state.isUseSignedOffBy()) { + return Collections.emptyList(); + } - if (projectControl.getProjectState().isUseSignedOffBy()) { - boolean sboAuthor = false; - boolean sboCommitter = false; - boolean sboMe = false; - for (final FooterLine footer : receiveEvent.commit.getFooterLines()) { - if (footer.matches(FooterKey.SIGNED_OFF_BY)) { - final String e = footer.getEmailAddress(); - if (e != null) { - sboAuthor |= author.getEmailAddress().equals(e); - sboCommitter |= committer.getEmailAddress().equals(e); - sboMe |= currentUser.hasEmailAddress(e); - } + RevCommit commit = receiveEvent.commit; + PersonIdent committer = commit.getCommitterIdent(); + PersonIdent author = commit.getAuthorIdent(); + + boolean sboAuthor = false; + boolean sboCommitter = false; + boolean sboMe = false; + for (FooterLine footer : commit.getFooterLines()) { + if (footer.matches(FooterKey.SIGNED_OFF_BY)) { + String e = footer.getEmailAddress(); + if (e != null) { + sboAuthor |= author.getEmailAddress().equals(e); + sboCommitter |= committer.getEmailAddress().equals(e); + sboMe |= user.hasEmailAddress(e); } } - if (!sboAuthor && !sboCommitter && !sboMe && !refControl.canForgeCommitter()) { + } + if (!sboAuthor && !sboCommitter && !sboMe) { + try { + perm.check(RefPermission.FORGE_COMMITTER); + } catch (AuthException denied) { throw new CommitValidationException( "not Signed-off-by author/committer/uploader in commit message footer"); + } catch (PermissionBackendException e) { + log.error("cannot check FORGE_COMMITTER", e); + throw new CommitValidationException("internal auth error"); } } return Collections.emptyList(); @@ -480,56 +505,69 @@ public class CommitValidators { /** Require that author matches the uploader. */ public static class AuthorUploaderValidator implements CommitValidationListener { - private final RefControl refControl; + private final IdentifiedUser user; + private final PermissionBackend.ForRef perm; private final String canonicalWebUrl; - public AuthorUploaderValidator(RefControl refControl, String canonicalWebUrl) { - this.refControl = refControl; + public AuthorUploaderValidator( + IdentifiedUser user, PermissionBackend.ForRef perm, String canonicalWebUrl) { + this.user = user; + this.perm = perm; this.canonicalWebUrl = canonicalWebUrl; } @Override public List onCommitReceived(CommitReceivedEvent receiveEvent) throws CommitValidationException { - IdentifiedUser currentUser = refControl.getUser().asIdentifiedUser(); - final PersonIdent author = receiveEvent.commit.getAuthorIdent(); - - if (!currentUser.hasEmailAddress(author.getEmailAddress()) && !refControl.canForgeAuthor()) { - List messages = new ArrayList<>(); - - messages.add( - getInvalidEmailError( - receiveEvent.commit, "author", author, currentUser, canonicalWebUrl)); - throw new CommitValidationException("invalid author", messages); + PersonIdent author = receiveEvent.commit.getAuthorIdent(); + if (user.hasEmailAddress(author.getEmailAddress())) { + return Collections.emptyList(); + } + try { + perm.check(RefPermission.FORGE_AUTHOR); + return Collections.emptyList(); + } catch (AuthException e) { + throw new CommitValidationException( + "invalid author", + invalidEmail(receiveEvent.commit, "author", author, user, canonicalWebUrl)); + } catch (PermissionBackendException e) { + log.error("cannot check FORGE_AUTHOR", e); + throw new CommitValidationException("internal auth error"); } - return Collections.emptyList(); } } /** Require that committer matches the uploader. */ public static class CommitterUploaderValidator implements CommitValidationListener { - private final RefControl refControl; + private final IdentifiedUser user; + private final PermissionBackend.ForRef perm; private final String canonicalWebUrl; - public CommitterUploaderValidator(RefControl refControl, String canonicalWebUrl) { - this.refControl = refControl; + public CommitterUploaderValidator( + IdentifiedUser user, PermissionBackend.ForRef perm, String canonicalWebUrl) { + this.user = user; + this.perm = perm; this.canonicalWebUrl = canonicalWebUrl; } @Override public List onCommitReceived(CommitReceivedEvent receiveEvent) throws CommitValidationException { - IdentifiedUser currentUser = refControl.getUser().asIdentifiedUser(); - final PersonIdent committer = receiveEvent.commit.getCommitterIdent(); - if (!currentUser.hasEmailAddress(committer.getEmailAddress()) - && !refControl.canForgeCommitter()) { - List messages = new ArrayList<>(); - messages.add( - getInvalidEmailError( - receiveEvent.commit, "committer", committer, currentUser, canonicalWebUrl)); - throw new CommitValidationException("invalid committer", messages); + PersonIdent committer = receiveEvent.commit.getCommitterIdent(); + if (user.hasEmailAddress(committer.getEmailAddress())) { + return Collections.emptyList(); + } + try { + perm.check(RefPermission.FORGE_COMMITTER); + return Collections.emptyList(); + } catch (AuthException e) { + throw new CommitValidationException( + "invalid committer", + invalidEmail(receiveEvent.commit, "committer", committer, user, canonicalWebUrl)); + } catch (PermissionBackendException e) { + log.error("cannot check FORGE_COMMITTER", e); + throw new CommitValidationException("internal auth error"); } - return Collections.emptyList(); } } @@ -539,25 +577,30 @@ public class CommitValidators { */ public static class AmendedGerritMergeCommitValidationListener implements CommitValidationListener { + private final PermissionBackend.ForRef perm; private final PersonIdent gerritIdent; - private final RefControl refControl; public AmendedGerritMergeCommitValidationListener( - final RefControl refControl, final PersonIdent gerritIdent) { - this.refControl = refControl; + PermissionBackend.ForRef perm, PersonIdent gerritIdent) { + this.perm = perm; this.gerritIdent = gerritIdent; } @Override public List onCommitReceived(CommitReceivedEvent receiveEvent) throws CommitValidationException { - final PersonIdent author = receiveEvent.commit.getAuthorIdent(); - + PersonIdent author = receiveEvent.commit.getAuthorIdent(); if (receiveEvent.commit.getParentCount() > 1 && author.getName().equals(gerritIdent.getName()) - && author.getEmailAddress().equals(gerritIdent.getEmailAddress()) - && !refControl.canForgeGerritServerIdentity()) { - throw new CommitValidationException("do not amend merges not made by you"); + && author.getEmailAddress().equals(gerritIdent.getEmailAddress())) { + try { + perm.check(RefPermission.FORGE_SERVER); + } catch (AuthException denied) { + throw new CommitValidationException("do not amend merges not made by you"); + } catch (PermissionBackendException e) { + log.error("cannot check FORGE_SERVER", e); + throw new CommitValidationException("internal auth error"); + } } return Collections.emptyList(); } @@ -629,7 +672,7 @@ public class CommitValidators { } } - private static CommitValidationMessage getInvalidEmailError( + private static CommitValidationMessage invalidEmail( RevCommit c, String type, PersonIdent who, diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java index 6771ad6da4..031c0ec629 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java @@ -391,7 +391,7 @@ public class RefControl { } /** @return true if this user can forge the author line in a commit. */ - public boolean canForgeAuthor() { + private boolean canForgeAuthor() { if (canForgeAuthor == null) { canForgeAuthor = canPerform(Permission.FORGE_AUTHOR); } @@ -399,7 +399,7 @@ public class RefControl { } /** @return true if this user can forge the committer line in a commit. */ - public boolean canForgeCommitter() { + private boolean canForgeCommitter() { if (canForgeCommitter == null) { canForgeCommitter = canPerform(Permission.FORGE_COMMITTER); } @@ -407,7 +407,7 @@ public class RefControl { } /** @return true if this user can forge the server on the committer line. */ - public boolean canForgeGerritServerIdentity() { + private boolean canForgeGerritServerIdentity() { return canPerform(Permission.FORGE_SERVER); } From 10aae18ff10ee82d21afb707ee2e67e987782399 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Fri, 28 Apr 2017 22:27:05 +0200 Subject: [PATCH 3/4] Convert GetAccess and canAddRefs to PermissionBackend Introduce a new ProjectPermission CREATE_REF which supports hinting if the user may be permitted to create references within the project. This is similar to RefPermission CREATE_CHANGE where the parent scope may be needed to test support for creation of a child. Unlike CREATE_CHANGE, application code does still check RefPermission CREATE during operations on the actual proposed reference name. Change-Id: I02204500265cb97c4fb92f40f01b6c738c06b5b1 --- .../gerrit/server/access/ListAccess.java | 7 +- .../server/permissions/ProjectPermission.java | 17 ++++- .../gerrit/server/project/GetAccess.java | 70 +++++++++++++------ .../gerrit/server/project/ProjectControl.java | 5 +- 4 files changed, 73 insertions(+), 26 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/access/ListAccess.java b/gerrit-server/src/main/java/com/google/gerrit/server/access/ListAccess.java index 024c6107c9..492d0e8c4a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/access/ListAccess.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/access/ListAccess.java @@ -20,6 +20,7 @@ import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestReadView; import com.google.gerrit.extensions.restapi.TopLevelResource; import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.project.GetAccess; import com.google.inject.Inject; import java.io.IOException; @@ -48,11 +49,11 @@ public class ListAccess implements RestReadView { @Override public Map apply(TopLevelResource resource) - throws ResourceNotFoundException, ResourceConflictException, IOException { + throws ResourceNotFoundException, ResourceConflictException, IOException, + PermissionBackendException { Map access = new TreeMap<>(); for (String p : projects) { - Project.NameKey projectName = new Project.NameKey(p); - access.put(p, getAccess.apply(projectName)); + access.put(p, getAccess.apply(new Project.NameKey(p))); } return access; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/ProjectPermission.java b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/ProjectPermission.java index 85b66c4a16..098b07a439 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/ProjectPermission.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/ProjectPermission.java @@ -32,7 +32,22 @@ public enum ProjectPermission { * *

This is a stronger form of {@link #ACCESS} where no filtering is required. */ - READ(Permission.READ); + READ(Permission.READ), + + /** + * Can create at least one reference in the project. + * + *

This project level permission only validates the user may create some type of reference + * within the project. The exact reference name must be checked at creation: + * + * 

permissionBackend
+   *    .user(user)
+   *    .project(proj)
+   *    .ref(ref)
+   *    .check(RefPermission.CREATE);
+   *
+ */ + CREATE_REF; private final String name; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/GetAccess.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/GetAccess.java index 997239dddf..7c0795ee71 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/GetAccess.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/GetAccess.java @@ -14,6 +14,11 @@ package com.google.gerrit.server.project; +import static com.google.gerrit.server.permissions.GlobalPermission.ADMINISTRATE_SERVER; +import static com.google.gerrit.server.permissions.ProjectPermission.CREATE_REF; +import static com.google.gerrit.server.permissions.RefPermission.CREATE_CHANGE; +import static com.google.gerrit.server.permissions.RefPermission.READ; + import com.google.common.collect.ImmutableBiMap; import com.google.common.collect.Iterables; import com.google.gerrit.common.data.AccessSection; @@ -25,6 +30,7 @@ import com.google.gerrit.extensions.api.access.AccessSectionInfo; import com.google.gerrit.extensions.api.access.PermissionInfo; import com.google.gerrit.extensions.api.access.PermissionRuleInfo; import com.google.gerrit.extensions.api.access.ProjectAccessInfo; +import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestReadView; @@ -37,6 +43,9 @@ import com.google.gerrit.server.account.GroupControl; import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.git.ProjectConfig; +import com.google.gerrit.server.permissions.PermissionBackend; +import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.gerrit.server.permissions.RefPermission; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; @@ -63,7 +72,8 @@ public class GetAccess implements RestReadView { PermissionRule.Action.INTERACTIVE, PermissionRuleInfo.Action.INTERACTIVE); - private final Provider self; + private final Provider user; + private final PermissionBackend permissionBackend; private final GroupControl.Factory groupControlFactory; private final AllProjectsName allProjectsName; private final ProjectJson projectJson; @@ -75,6 +85,7 @@ public class GetAccess implements RestReadView { @Inject public GetAccess( Provider self, + PermissionBackend permissionBackend, GroupControl.Factory groupControlFactory, AllProjectsName allProjectsName, ProjectCache projectCache, @@ -82,7 +93,8 @@ public class GetAccess implements RestReadView { ProjectJson projectJson, ProjectControl.GenericFactory projectControlFactory, GroupBackend groupBackend) { - this.self = self; + this.user = self; + this.permissionBackend = permissionBackend; this.groupControlFactory = groupControlFactory; this.allProjectsName = allProjectsName; this.projectJson = projectJson; @@ -93,9 +105,10 @@ public class GetAccess implements RestReadView { } public ProjectAccessInfo apply(Project.NameKey nameKey) - throws ResourceNotFoundException, ResourceConflictException, IOException { + throws ResourceNotFoundException, ResourceConflictException, IOException, + PermissionBackendException { try { - return apply(new ProjectResource(projectControlFactory.controlFor(nameKey, self.get()))); + return apply(new ProjectResource(projectControlFactory.controlFor(nameKey, user.get()))); } catch (NoSuchProjectException e) { throw new ResourceNotFoundException(nameKey.get()); } @@ -103,16 +116,18 @@ public class GetAccess implements RestReadView { @Override public ProjectAccessInfo apply(ProjectResource rsrc) - throws ResourceNotFoundException, ResourceConflictException, IOException { + throws ResourceNotFoundException, ResourceConflictException, IOException, + PermissionBackendException { // Load the current configuration from the repository, ensuring it's the most // recent version available. If it differs from what was in the project // state, force a cache flush now. - // + Project.NameKey projectName = rsrc.getNameKey(); ProjectAccessInfo info = new ProjectAccessInfo(); - ProjectConfig config; ProjectControl pc = createProjectControl(projectName); - RefControl metaConfigControl = pc.controlForRef(RefNames.REFS_CONFIG); + PermissionBackend.ForProject perm = permissionBackend.user(user).project(projectName); + + ProjectConfig config; try (MetaDataUpdate md = metaDataUpdateFactory.create(projectName)) { config = ProjectConfig.read(md); @@ -121,10 +136,12 @@ public class GetAccess implements RestReadView { config.commit(md); projectCache.evict(config.getProject()); pc = createProjectControl(projectName); + perm = permissionBackend.user(user).project(projectName); } else if (config.getRevision() != null && !config.getRevision().equals(pc.getProjectState().getConfig().getRevision())) { projectCache.evict(config.getProject()); pc = createProjectControl(projectName); + perm = permissionBackend.user(user).project(projectName); } } catch (ConfigInvalidException e) { throw new ResourceConflictException(e.getMessage()); @@ -135,6 +152,7 @@ public class GetAccess implements RestReadView { info.local = new HashMap<>(); info.ownerOf = new HashSet<>(); Map visibleGroups = new HashMap<>(); + boolean checkReadConfig = check(perm, RefNames.REFS_CONFIG, READ); for (AccessSection section : config.getAccessSections()) { String name = section.getName(); @@ -143,20 +161,19 @@ public class GetAccess implements RestReadView { info.local.put(name, createAccessSection(section)); info.ownerOf.add(name); - } else if (metaConfigControl.isVisible()) { + } else if (checkReadConfig) { info.local.put(section.getName(), createAccessSection(section)); } } else if (RefConfigSection.isValid(name)) { - RefControl rc = pc.controlForRef(name); - if (rc.isOwner()) { + if (pc.controlForRef(name).isOwner()) { info.local.put(name, createAccessSection(section)); info.ownerOf.add(name); - } else if (metaConfigControl.isVisible()) { + } else if (checkReadConfig) { info.local.put(name, createAccessSection(section)); - } else if (rc.isVisible()) { + } else if (check(perm, name, READ)) { // Filter the section to only add rules describing groups that // are visible to the current-user. This includes any group the // user is a member of, as well as groups they own or that @@ -214,21 +231,32 @@ public class GetAccess implements RestReadView { info.inheritsFrom = projectJson.format(parent.getProject()); } - if (pc.getProject().getNameKey().equals(allProjectsName)) { - if (pc.isOwner()) { - info.ownerOf.add(AccessSection.GLOBAL_CAPABILITIES); - } + if (projectName.equals(allProjectsName) + && permissionBackend.user(user).testOrFalse(ADMINISTRATE_SERVER)) { + info.ownerOf.add(AccessSection.GLOBAL_CAPABILITIES); } info.isOwner = toBoolean(pc.isOwner()); info.canUpload = - toBoolean(pc.isOwner() || (metaConfigControl.isVisible() && metaConfigControl.canUpload())); - info.canAdd = toBoolean(pc.canAddRefs()); - info.configVisible = pc.isOwner() || metaConfigControl.isVisible(); + toBoolean( + pc.isOwner() + || (checkReadConfig && perm.ref(RefNames.REFS_CONFIG).testOrFalse(CREATE_CHANGE))); + info.canAdd = toBoolean(perm.testOrFalse(CREATE_REF)); + info.configVisible = checkReadConfig || pc.isOwner(); return info; } + private static boolean check(PermissionBackend.ForProject ctx, String ref, RefPermission perm) + throws PermissionBackendException { + try { + ctx.ref(ref).check(perm); + return true; + } catch (AuthException denied) { + return false; + } + } + private AccessSectionInfo createAccessSection(AccessSection section) { AccessSectionInfo accessSectionInfo = new AccessSectionInfo(); accessSectionInfo.permissions = new HashMap<>(); @@ -255,7 +283,7 @@ public class GetAccess implements RestReadView { private ProjectControl createProjectControl(Project.NameKey projectName) throws IOException, ResourceNotFoundException { try { - return projectControlFactory.controlFor(projectName, self.get()); + return projectControlFactory.controlFor(projectName, user.get()); } catch (NoSuchProjectException e) { throw new ResourceNotFoundException(projectName.get()); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java index 8773bad09f..44a3405c2a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java @@ -249,7 +249,7 @@ public class ProjectControl { return getProject().getState().equals(com.google.gerrit.extensions.client.ProjectState.HIDDEN); } - public boolean canAddRefs() { + private boolean canAddRefs() { return (canPerformOnAnyRef(Permission.CREATE) || isOwnerAnyRef()); } @@ -588,6 +588,9 @@ public class ProjectControl { case READ: return !isHidden() && allRefsAreVisible(Collections.emptySet()); + + case CREATE_REF: + return canAddRefs(); } throw new PermissionBackendException(perm + " unsupported"); } From ca1113e5bdf66a39b64598b420a2f9b16ec13b48 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Sat, 29 Apr 2017 11:10:44 -0700 Subject: [PATCH 4/4] Replace canUpload with CREATE_CHANGE Check RefPermission.CREATE_CHANGE before creating a change, instead of relying on the legacy RefControl.canUpload(). This delegates creation decisions through the PermissionBackend. Change-Id: Ic76e96ca6079a179ae65fa2de436fb6df929b6d9 --- .../rest/change/CreateChangeIT.java | 4 +- .../rpc/project/ProjectAccessFactory.java | 40 ++++++++---- .../rpc/project/ReviewProjectAccess.java | 28 +++++++-- .../gerrit/server/change/CherryPick.java | 61 ++++++++----------- .../server/change/CherryPickCommit.java | 53 ++++++++++------ .../gerrit/server/change/CreateChange.java | 30 ++++----- .../server/change/PublishChangeEdit.java | 8 +-- .../google/gerrit/server/change/Revert.java | 52 ++++++++-------- .../gerrit/server/git/ReceiveCommits.java | 8 ++- .../gerrit/server/project/ChangeControl.java | 9 +-- .../gerrit/server/project/RefControl.java | 7 +-- .../gerrit/server/project/RefControlTest.java | 22 ++++--- 12 files changed, 177 insertions(+), 145 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java index 578949a506..afd20bbf92 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java @@ -42,9 +42,9 @@ import com.google.gerrit.extensions.common.ChangeMessageInfo; import com.google.gerrit.extensions.common.CommitInfo; import com.google.gerrit.extensions.common.MergeInput; import com.google.gerrit.extensions.common.RevisionInfo; -import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.MethodNotAllowedException; +import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.UnprocessableEntityException; import com.google.gerrit.reviewdb.client.Branch; @@ -192,7 +192,7 @@ public class CreateChangeIT extends AbstractDaemonTest { ChangeInput in = newChangeInput(ChangeStatus.NEW); in.branch = "invisible-branch"; - assertCreateFails(in, AuthException.class, "cannot upload review"); + assertCreateFails(in, ResourceNotFoundException.class, ""); } @Test diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ProjectAccessFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ProjectAccessFactory.java index 3b620f1e20..afec3b634d 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ProjectAccessFactory.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ProjectAccessFactory.java @@ -14,6 +14,10 @@ package com.google.gerrit.httpd.rpc.project; +import static com.google.gerrit.server.permissions.GlobalPermission.ADMINISTRATE_SERVER; +import static com.google.gerrit.server.permissions.RefPermission.CREATE_CHANGE; +import static com.google.gerrit.server.permissions.RefPermission.READ; + import com.google.common.collect.Maps; import com.google.gerrit.common.data.AccessSection; import com.google.gerrit.common.data.GroupDescription; @@ -39,10 +43,10 @@ import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.permissions.ProjectPermission; +import com.google.gerrit.server.permissions.RefPermission; import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectControl; -import com.google.gerrit.server.project.RefControl; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.assistedinject.Assisted; @@ -122,10 +126,11 @@ class ProjectAccessFactory extends Handler { } } - final RefControl metaConfigControl = pc.controlForRef(RefNames.REFS_CONFIG); List local = new ArrayList<>(); Set ownerOf = new HashSet<>(); Map visibleGroups = new HashMap<>(); + PermissionBackend.ForProject perm = permissionBackend.user(user).project(projectName); + boolean checkReadConfig = check(perm, RefNames.REFS_CONFIG, READ); for (AccessSection section : config.getAccessSections()) { String name = section.getName(); @@ -134,20 +139,19 @@ class ProjectAccessFactory extends Handler { local.add(section); ownerOf.add(name); - } else if (metaConfigControl.isVisible()) { + } else if (checkReadConfig) { local.add(section); } } else if (RefConfigSection.isValid(name)) { - RefControl rc = pc.controlForRef(name); - if (rc.isOwner()) { + if (pc.controlForRef(name).isOwner()) { local.add(section); ownerOf.add(name); - } else if (metaConfigControl.isVisible()) { + } else if (checkReadConfig) { local.add(section); - } else if (rc.isVisible()) { + } else if (check(perm, name, READ)) { // Filter the section to only add rules describing groups that // are visible to the current-user. This includes any group the // user is a member of, as well as groups they own or that @@ -205,17 +209,17 @@ class ProjectAccessFactory extends Handler { detail.setInheritsFrom(config.getProject().getParent(allProjectsName)); - if (projectName.equals(allProjectsName)) { - if (pc.isOwner()) { - ownerOf.add(AccessSection.GLOBAL_CAPABILITIES); - } + if (projectName.equals(allProjectsName) + && permissionBackend.user(user).testOrFalse(ADMINISTRATE_SERVER)) { + ownerOf.add(AccessSection.GLOBAL_CAPABILITIES); } detail.setLocal(local); detail.setOwnerOf(ownerOf); detail.setCanUpload( - metaConfigControl.isVisible() && (pc.isOwner() || metaConfigControl.canUpload())); - detail.setConfigVisible(pc.isOwner() || metaConfigControl.isVisible()); + pc.isOwner() + || (checkReadConfig && perm.ref(RefNames.REFS_CONFIG).testOrFalse(CREATE_CHANGE))); + detail.setConfigVisible(pc.isOwner() || checkReadConfig); detail.setGroupInfo(buildGroupInfo(local)); detail.setLabelTypes(pc.getLabelTypes()); detail.setFileHistoryLinks(getConfigFileLogLinks(projectName.get())); @@ -257,4 +261,14 @@ class ProjectAccessFactory extends Handler { } return pc; } + + private static boolean check(PermissionBackend.ForProject ctx, String ref, RefPermission perm) + throws PermissionBackendException { + try { + ctx.ref(ref).check(perm); + return true; + } catch (AuthException denied) { + return false; + } + } } diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ReviewProjectAccess.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ReviewProjectAccess.java index 1a79d57518..4e2a4d3e6e 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ReviewProjectAccess.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ReviewProjectAccess.java @@ -22,6 +22,7 @@ import com.google.gerrit.common.data.GlobalCapability; import com.google.gerrit.common.data.PermissionRule; import com.google.gerrit.common.errors.PermissionDeniedException; import com.google.gerrit.extensions.api.changes.AddReviewerInput; +import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Change; @@ -39,9 +40,11 @@ import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.server.git.MetaDataUpdate; import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.group.SystemGroupBackend; +import com.google.gerrit.server.permissions.PermissionBackend; +import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.gerrit.server.permissions.RefPermission; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectControl; -import com.google.gerrit.server.project.RefControl; import com.google.gerrit.server.project.SetParent; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.UpdateException; @@ -68,6 +71,7 @@ public class ReviewProjectAccess extends ProjectAccessHandler { } private final ReviewDb db; + private final PermissionBackend permissionBackend; private final Sequences seq; private final Provider reviewersProvider; private final ProjectCache projectCache; @@ -78,6 +82,7 @@ public class ReviewProjectAccess extends ProjectAccessHandler { @Inject ReviewProjectAccess( final ProjectControl.Factory projectControlFactory, + PermissionBackend permissionBackend, GroupBackend groupBackend, MetaDataUpdate.User metaDataUpdateFactory, ReviewDb db, @@ -107,6 +112,7 @@ public class ReviewProjectAccess extends ProjectAccessHandler { message, false); this.db = db; + this.permissionBackend = permissionBackend; this.seq = seq; this.reviewersProvider = reviewersProvider; this.projectCache = projectCache; @@ -124,13 +130,23 @@ public class ReviewProjectAccess extends ProjectAccessHandler { ProjectConfig config, MetaDataUpdate md, boolean parentProjectUpdate) - throws IOException, OrmException, PermissionDeniedException { - RefControl refsMetaConfigControl = projectControl.controlForRef(RefNames.REFS_CONFIG); - if (!refsMetaConfigControl.isVisible()) { + throws IOException, OrmException, PermissionDeniedException, PermissionBackendException { + PermissionBackend.ForRef metaRef = + permissionBackend + .user(projectControl.getUser()) + .project(projectControl.getProject().getNameKey()) + .ref(RefNames.REFS_CONFIG); + try { + metaRef.check(RefPermission.READ); + } catch (AuthException denied) { throw new PermissionDeniedException(RefNames.REFS_CONFIG + " not visible"); } - if (!projectControl.isOwner() && !refsMetaConfigControl.canUpload()) { - throw new PermissionDeniedException("cannot upload to " + RefNames.REFS_CONFIG); + if (!projectControl.isOwner()) { + try { + metaRef.check(RefPermission.CREATE_CHANGE); + } catch (AuthException denied) { + throw new PermissionDeniedException("cannot create change for " + RefNames.REFS_CONFIG); + } } md.setInsertChangeId(true); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java index 95491f5f4e..ae937c96d2 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPick.java @@ -14,23 +14,21 @@ package com.google.gerrit.server.change; -import com.google.gerrit.common.data.Capable; import com.google.gerrit.extensions.api.changes.CherryPickInput; import com.google.gerrit.extensions.common.ChangeInfo; -import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.webui.UiAction; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.RefNames; -import com.google.gerrit.reviewdb.server.ReviewDb; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.git.IntegrationException; -import com.google.gerrit.server.project.ChangeControl; +import com.google.gerrit.server.permissions.PermissionBackend; +import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.gerrit.server.permissions.RefPermission; import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.NoSuchChangeException; -import com.google.gerrit.server.project.ProjectControl; -import com.google.gerrit.server.project.RefControl; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.RetryHelper; import com.google.gerrit.server.update.RetryingRestModifyView; @@ -45,59 +43,54 @@ import java.io.IOException; public class CherryPick extends RetryingRestModifyView implements UiAction { - private final Provider dbProvider; + private final PermissionBackend permissionBackend; + private final Provider user; private final CherryPickChange cherryPickChange; private final ChangeJson.Factory json; @Inject CherryPick( + PermissionBackend permissionBackend, + Provider user, RetryHelper retryHelper, - Provider dbProvider, CherryPickChange cherryPickChange, ChangeJson.Factory json) { super(retryHelper); - this.dbProvider = dbProvider; + this.permissionBackend = permissionBackend; + this.user = user; this.cherryPickChange = cherryPickChange; this.json = json; } @Override - protected ChangeInfo applyImpl( - BatchUpdate.Factory updateFactory, RevisionResource revision, CherryPickInput input) - throws OrmException, IOException, UpdateException, RestApiException { - final ChangeControl control = revision.getControl(); + public ChangeInfo applyImpl( + BatchUpdate.Factory updateFactory, RevisionResource rsrc, CherryPickInput input) + throws OrmException, IOException, UpdateException, RestApiException, + PermissionBackendException { input.parent = input.parent == null ? 1 : input.parent; - if (input.message == null || input.message.trim().isEmpty()) { throw new BadRequestException("message must be non-empty"); } else if (input.destination == null || input.destination.trim().isEmpty()) { throw new BadRequestException("destination must be non-empty"); } - if (!control.isVisible(dbProvider.get())) { - throw new AuthException("Cherry pick not permitted"); - } - - ProjectControl projectControl = control.getProjectControl(); - Capable capable = projectControl.canPushToAtLeastOneRef(); - if (capable != Capable.OK) { - throw new AuthException(capable.getMessage()); - } - - RefControl refControl = projectControl.controlForRef(RefNames.fullName(input.destination)); - if (!refControl.canUpload()) { - throw new AuthException( - "Not allowed to cherry pick " - + revision.getChange().getId().toString() - + " to " - + input.destination); - } + String refName = RefNames.fullName(input.destination); + CreateChange.checkValidCLA(rsrc.getControl().getProjectControl()); + permissionBackend + .user(user) + .project(rsrc.getChange().getProject()) + .ref(refName) + .check(RefPermission.CREATE_CHANGE); try { Change.Id cherryPickedChangeId = cherryPickChange.cherryPick( - updateFactory, revision.getChange(), revision.getPatchSet(), input, refControl); - return json.noOptions().format(revision.getProject(), cherryPickedChangeId); + updateFactory, + rsrc.getChange(), + rsrc.getPatchSet(), + input, + rsrc.getControl().getProjectControl().controlForRef(refName)); + return json.noOptions().format(rsrc.getProject(), cherryPickedChangeId); } catch (InvalidChangeOperationException e) { throw new BadRequestException(e.getMessage()); } catch (IntegrationException | NoSuchChangeException e) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickCommit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickCommit.java index 1b63cb503c..41f0463e59 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickCommit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickCommit.java @@ -15,27 +15,28 @@ package com.google.gerrit.server.change; import com.google.common.base.Strings; -import com.google.gerrit.common.data.Capable; import com.google.gerrit.extensions.api.changes.CherryPickInput; import com.google.gerrit.extensions.common.ChangeInfo; -import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.BadRequestException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.git.IntegrationException; +import com.google.gerrit.server.permissions.PermissionBackend; +import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.gerrit.server.permissions.RefPermission; import com.google.gerrit.server.project.CommitResource; import com.google.gerrit.server.project.InvalidChangeOperationException; -import com.google.gerrit.server.project.ProjectControl; -import com.google.gerrit.server.project.RefControl; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.RetryHelper; import com.google.gerrit.server.update.RetryingRestModifyView; import com.google.gerrit.server.update.UpdateException; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; +import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; import org.eclipse.jgit.revwalk.RevCommit; @@ -43,14 +44,21 @@ import org.eclipse.jgit.revwalk.RevCommit; @Singleton public class CherryPickCommit extends RetryingRestModifyView { - + private final PermissionBackend permissionBackend; + private final Provider user; private final CherryPickChange cherryPickChange; private final ChangeJson.Factory json; @Inject CherryPickCommit( - RetryHelper retryHelper, CherryPickChange cherryPickChange, ChangeJson.Factory json) { + RetryHelper retryHelper, + Provider user, + CherryPickChange cherryPickChange, + ChangeJson.Factory json, + PermissionBackend permissionBackend) { super(retryHelper); + this.permissionBackend = permissionBackend; + this.user = user; this.cherryPickChange = cherryPickChange; this.json = json; } @@ -58,35 +66,40 @@ public class CherryPickCommit @Override public ChangeInfo applyImpl( BatchUpdate.Factory updateFactory, CommitResource rsrc, CherryPickInput input) - throws OrmException, IOException, UpdateException, RestApiException { + throws OrmException, IOException, UpdateException, RestApiException, + PermissionBackendException { RevCommit commit = rsrc.getCommit(); String message = Strings.nullToEmpty(input.message).trim(); input.message = message.isEmpty() ? commit.getFullMessage() : message; String destination = Strings.nullToEmpty(input.destination).trim(); input.parent = input.parent == null ? 1 : input.parent; + Project.NameKey projectName = rsrc.getProject().getProject().getNameKey(); if (destination.isEmpty()) { throw new BadRequestException("destination must be non-empty"); } - ProjectControl projectControl = rsrc.getProject(); - Capable capable = projectControl.canPushToAtLeastOneRef(); - if (capable != Capable.OK) { - throw new AuthException(capable.getMessage()); - } - String refName = RefNames.fullName(destination); - RefControl refControl = projectControl.controlForRef(refName); - if (!refControl.canUpload()) { - throw new AuthException("Not allowed to cherry pick " + commit + " to " + destination); - } + CreateChange.checkValidCLA(rsrc.getProject()); + permissionBackend + .user(user) + .project(projectName) + .ref(refName) + .check(RefPermission.CREATE_CHANGE); - Project.NameKey project = projectControl.getProject().getNameKey(); try { Change.Id cherryPickedChangeId = cherryPickChange.cherryPick( - updateFactory, null, null, null, null, project, commit, input, refControl); - return json.noOptions().format(project, cherryPickedChangeId); + updateFactory, + null, + null, + null, + null, + projectName, + commit, + input, + rsrc.getProject().controlForRef(refName)); + return json.noOptions().format(projectName, cherryPickedChangeId); } catch (InvalidChangeOperationException e) { throw new BadRequestException(e.getMessage()); } catch (IntegrationException e) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java index 599ce5efdb..cca9cb6e48 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java @@ -52,13 +52,14 @@ import com.google.gerrit.server.config.AnonymousCowardName; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.MergeUtil; +import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.gerrit.server.permissions.RefPermission; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.ProjectControl; import com.google.gerrit.server.project.ProjectResource; import com.google.gerrit.server.project.ProjectsCollection; -import com.google.gerrit.server.project.RefControl; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.RetryHelper; import com.google.gerrit.server.update.RetryingRestModifyView; @@ -89,13 +90,13 @@ import org.eclipse.jgit.util.ChangeIdUtil; @Singleton public class CreateChange extends RetryingRestModifyView> { - private final String anonymousCowardName; private final Provider db; private final GitRepositoryManager gitManager; private final AccountCache accountCache; private final Sequences seq; private final TimeZone serverTimeZone; + private final PermissionBackend permissionBackend; private final Provider user; private final ProjectsCollection projectsCollection; private final ChangeInserter.Factory changeInserterFactory; @@ -115,6 +116,7 @@ public class CreateChange AccountCache accountCache, Sequences seq, @GerritPersonIdent PersonIdent myIdent, + PermissionBackend permissionBackend, Provider user, ProjectsCollection projectsCollection, ChangeInserter.Factory changeInserterFactory, @@ -132,6 +134,7 @@ public class CreateChange this.accountCache = accountCache; this.seq = seq; this.serverTimeZone = myIdent.getTimeZone(); + this.permissionBackend = permissionBackend; this.user = user; this.projectsCollection = projectsCollection; this.changeInserterFactory = changeInserterFactory; @@ -165,26 +168,18 @@ public class CreateChange if (input.status != ChangeStatus.NEW && input.status != ChangeStatus.DRAFT) { throw new BadRequestException("unsupported change status"); } - if (!allowDrafts && input.status == ChangeStatus.DRAFT) { throw new MethodNotAllowedException("draft workflow is disabled"); } } - String refName = RefNames.fullName(input.branch); ProjectResource rsrc = projectsCollection.parse(input.project); - - Capable r = rsrc.getControl().canPushToAtLeastOneRef(); - if (r != Capable.OK) { - throw new AuthException(r.getMessage()); - } - - RefControl refControl = rsrc.getControl().controlForRef(refName); - if (!refControl.canUpload() || !refControl.isVisible()) { - throw new AuthException("cannot upload review"); - } + checkValidCLA(rsrc.getControl()); Project.NameKey project = rsrc.getNameKey(); + String refName = RefNames.fullName(input.branch); + permissionBackend.user(user).project(project).ref(refName).check(RefPermission.CREATE_CHANGE); + try (Repository git = gitManager.openRepository(project); ObjectInserter oi = git.newObjectInserter(); ObjectReader reader = oi.newReader(); @@ -345,4 +340,11 @@ public class CreateChange private static ObjectId emptyTreeId(ObjectInserter inserter) throws IOException { return inserter.insert(new TreeFormatter()); } + + static void checkValidCLA(ProjectControl ctl) throws AuthException { + Capable capable = ctl.canPushToAtLeastOneRef(); + if (capable != Capable.OK) { + throw new AuthException(capable.getMessage()); + } + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishChangeEdit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishChangeEdit.java index 658b87b603..eab06fb0d6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishChangeEdit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishChangeEdit.java @@ -14,11 +14,9 @@ package com.google.gerrit.server.change; -import com.google.gerrit.common.data.Capable; import com.google.gerrit.extensions.api.changes.PublishChangeEditInput; import com.google.gerrit.extensions.registration.DynamicMap; import com.google.gerrit.extensions.restapi.AcceptsPost; -import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.ChildCollection; import com.google.gerrit.extensions.restapi.IdString; import com.google.gerrit.extensions.restapi.NotImplementedException; @@ -88,11 +86,7 @@ public class PublishChangeEdit protected Response applyImpl( BatchUpdate.Factory updateFactory, ChangeResource rsrc, PublishChangeEditInput in) throws IOException, OrmException, RestApiException, UpdateException { - Capable r = rsrc.getControl().getProjectControl().canPushToAtLeastOneRef(); - if (r != Capable.OK) { - throw new AuthException(r.getMessage()); - } - + CreateChange.checkValidCLA(rsrc.getControl().getProjectControl()); Optional edit = editUtil.byChange(rsrc.getChange()); if (!edit.isPresent()) { throw new ResourceConflictException( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Revert.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Revert.java index 56d54ee3ed..af06054e03 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Revert.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Revert.java @@ -14,19 +14,18 @@ package com.google.gerrit.server.change; +import static com.google.gerrit.server.permissions.RefPermission.CREATE_CHANGE; + import com.google.common.base.Strings; import com.google.gerrit.common.TimeUtil; -import com.google.gerrit.common.data.Capable; import com.google.gerrit.extensions.api.changes.RevertInput; import com.google.gerrit.extensions.common.ChangeInfo; -import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.webui.UiAction; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; -import com.google.gerrit.reviewdb.client.Change.Status; import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; @@ -43,10 +42,10 @@ import com.google.gerrit.server.extensions.events.ChangeReverted; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.mail.send.RevertedSender; import com.google.gerrit.server.notedb.ReviewerStateInternal; +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.NoSuchChangeException; -import com.google.gerrit.server.project.ProjectControl; -import com.google.gerrit.server.project.RefControl; import com.google.gerrit.server.update.BatchUpdate; import com.google.gerrit.server.update.BatchUpdateOp; import com.google.gerrit.server.update.ChangeContext; @@ -82,6 +81,8 @@ public class Revert extends RetryingRestModifyView db; + private final PermissionBackend permissionBackend; + private final Provider user; private final GitRepositoryManager repoManager; private final ChangeInserter.Factory changeInserterFactory; private final ChangeMessagesUtil cmUtil; @@ -96,6 +97,8 @@ public class Revert extends RetryingRestModifyView db, + PermissionBackend permissionBackend, + Provider user, GitRepositoryManager repoManager, ChangeInserter.Factory changeInserterFactory, ChangeMessagesUtil cmUtil, @@ -109,6 +112,8 @@ public class Revert extends RetryingRestModifyView