diff --git a/java/com/google/gerrit/extensions/api/access/GerritPermission.java b/java/com/google/gerrit/extensions/api/access/GerritPermission.java index 133de3199c..02afbdc773 100644 --- a/java/com/google/gerrit/extensions/api/access/GerritPermission.java +++ b/java/com/google/gerrit/extensions/api/access/GerritPermission.java @@ -18,7 +18,13 @@ import java.util.Locale; /** Gerrit permission for hosts, projects, refs, changes, labels and plugins. */ public interface GerritPermission { - /** @return readable identifier of this permission for exception message. */ + /** + * A description in the context of an exception message. + * + *

Should be grammatical when used in the construction "not permitted: [description] on + * [resource]", although individual {@code PermissionBackend} implementations may vary the + * wording. + */ String describeForException(); static String describeEnumValue(Enum value) { diff --git a/java/com/google/gerrit/extensions/restapi/AuthException.java b/java/com/google/gerrit/extensions/restapi/AuthException.java index 0b4f459b6d..fe1744bbb9 100644 --- a/java/com/google/gerrit/extensions/restapi/AuthException.java +++ b/java/com/google/gerrit/extensions/restapi/AuthException.java @@ -14,10 +14,17 @@ package com.google.gerrit.extensions.restapi; +import static com.google.common.base.Preconditions.checkArgument; + +import com.google.common.base.Strings; +import java.util.Optional; + /** Caller cannot perform the request operation (HTTP 403 Forbidden). */ public class AuthException extends RestApiException { private static final long serialVersionUID = 1L; + private Optional advice = Optional.empty(); + /** @param msg message to return to the client. */ public AuthException(String msg) { super(msg); @@ -30,4 +37,19 @@ public class AuthException extends RestApiException { public AuthException(String msg, Throwable cause) { super(msg, cause); } + + public void setAdvice(String advice) { + checkArgument(!Strings.isNullOrEmpty(advice)); + this.advice = Optional.of(advice); + } + + /** + * Advice that the user can follow to acquire authorization to perform the action. + * + *

This may be long-form text with newlines, and may be printed to a terminal, for example in + * the message stream in response to a push. + */ + public Optional getAdvice() { + return advice; + } } diff --git a/java/com/google/gerrit/git/testing/PushResultSubject.java b/java/com/google/gerrit/git/testing/PushResultSubject.java index c5163d1e12..929e1826e8 100644 --- a/java/com/google/gerrit/git/testing/PushResultSubject.java +++ b/java/com/google/gerrit/git/testing/PushResultSubject.java @@ -54,6 +54,13 @@ public class PushResultSubject extends Subject { Truth.assertThat(trimMessages()).isEqualTo(Arrays.stream(expectedLines).collect(joining("\n"))); } + public void containsMessages(String... expectedLines) { + checkArgument(expectedLines.length > 0, "use hasNoMessages()"); + isNotNull(); + Iterable got = Splitter.on("\n").split(trimMessages()); + Truth.assertThat(got).containsAllIn(expectedLines).inOrder(); + } + private String trimMessages() { return trimMessages(actual().getMessages()); } diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index eb61afac66..3bc0c56cb4 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -61,7 +61,6 @@ import com.google.gerrit.common.Nullable; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.common.data.LabelType; import com.google.gerrit.common.data.LabelTypes; -import com.google.gerrit.common.data.PermissionRule; import com.google.gerrit.extensions.api.changes.HashtagsInput; import com.google.gerrit.extensions.api.changes.NotifyHandling; import com.google.gerrit.extensions.api.changes.RecipientType; @@ -133,6 +132,7 @@ import com.google.gerrit.server.permissions.ChangePermission; import com.google.gerrit.server.permissions.GlobalPermission; import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; +import com.google.gerrit.server.permissions.PermissionDeniedException; import com.google.gerrit.server.permissions.ProjectPermission; import com.google.gerrit.server.permissions.RefPermission; import com.google.gerrit.server.project.CreateRefControl; @@ -249,6 +249,10 @@ class ReceiveCommits { } } + private static final String CANNOT_DELETE_CHANGES = "Cannot delete from '" + REFS_CHANGES + "'"; + private static final String CANNOT_DELETE_CONFIG = + "Cannot delete project configuration from '" + RefNames.REFS_CONFIG + "'"; + interface Factory { ReceiveCommits create( ProjectState projectState, @@ -368,7 +372,9 @@ class ReceiveCommits { // Collections populated during processing. private final List updateGroups; private final List messages; - private final ListMultimap errors; + /** Multimap of error text to refnames that produced that error. */ + private final ListMultimap errors; + private final ListMultimap pushOptions; private final Map replaceByChange; @@ -594,7 +600,7 @@ class ReceiveCommits { if (!errors.isEmpty()) { logger.atFine().log("Handling error conditions: %s", errors.keySet()); - for (ReceiveError error : errors.keySet()) { + for (String error : errors.keySet()) { receivePack.sendMessage(buildError(error, errors.get(error))); } receivePack.sendMessage(String.format("User: %s", user.getLoggableName())); @@ -809,11 +815,11 @@ class ReceiveCommits { } } - private String buildError(ReceiveError error, List branches) { + private String buildError(String error, List branches) { StringBuilder sb = new StringBuilder(); if (branches.size() == 1) { sb.append("Branch ").append(branches.get(0)).append(":\n"); - sb.append(error.get()); + sb.append(error); return sb.toString(); } sb.append("Branches"); @@ -822,7 +828,7 @@ class ReceiveCommits { sb.append(delim).append(branch); delim = ", "; } - return sb.append(":\n").append(error.get()).toString(); + return sb.append(":\n").append(error).toString(); } /** Parses push options specified as "git push -o OPTION" */ @@ -1093,7 +1099,10 @@ class ReceiveCommits { // Must pass explicit user instead of injecting a provider into CreateRefControl, since // Provider within ReceiveCommits will always return anonymous. createRefControl.checkCreateRef(Providers.of(user), receivePack.getRepository(), branch, obj); - } catch (AuthException | ResourceConflictException denied) { + } catch (AuthException denied) { + rejectProhibited(cmd, denied); + return; + } catch (ResourceConflictException denied) { reject(cmd, "prohibited by Gerrit: " + denied.getMessage()); return; } @@ -1109,14 +1118,8 @@ class ReceiveCommits { private void parseUpdate(ReceiveCommand cmd) throws PermissionBackendException { logger.atFine().log("Updating %s", cmd); - boolean ok; - try { - permissions.ref(cmd.getRefName()).check(RefPermission.UPDATE); - ok = true; - } catch (AuthException err) { - ok = false; - } - if (ok) { + Optional err = checkRefPermission(cmd, RefPermission.UPDATE); + if (!err.isPresent()) { if (isHead(cmd) && !isCommit(cmd)) { return; } @@ -1126,12 +1129,7 @@ class ReceiveCommits { validateNewCommits(new Branch.NameKey(project.getNameKey(), cmd.getRefName()), cmd); actualCommands.add(cmd); } else { - if (RefNames.REFS_CONFIG.equals(cmd.getRefName())) { - errors.put(ReceiveError.CONFIG_UPDATE, RefNames.REFS_CONFIG); - } else { - errors.put(ReceiveError.UPDATE, cmd.getRefName()); - } - reject(cmd, "prohibited by Gerrit: ref update access denied"); + rejectProhibited(cmd, err.get()); } } @@ -1156,32 +1154,21 @@ class ReceiveCommits { private void parseDelete(ReceiveCommand cmd) throws PermissionBackendException { logger.atFine().log("Deleting %s", cmd); if (cmd.getRefName().startsWith(REFS_CHANGES)) { - errors.put(ReceiveError.DELETE_CHANGES, cmd.getRefName()); + errors.put(CANNOT_DELETE_CHANGES, cmd.getRefName()); reject(cmd, "cannot delete changes"); - } else if (canDelete(cmd)) { + } else if (isConfigRef(cmd.getRefName())) { + errors.put(CANNOT_DELETE_CONFIG, cmd.getRefName()); + reject(cmd, "cannot delete project configuration"); + } + + Optional err = checkRefPermission(cmd, RefPermission.DELETE); + if (!err.isPresent()) { if (!validRefOperation(cmd)) { return; } actualCommands.add(cmd); - } else if (RefNames.REFS_CONFIG.equals(cmd.getRefName())) { - reject(cmd, "cannot delete project configuration"); } else { - errors.put(ReceiveError.DELETE, cmd.getRefName()); - reject(cmd, "cannot delete references"); - } - } - - private boolean canDelete(ReceiveCommand cmd) throws PermissionBackendException { - if (isConfigRef(cmd.getRefName())) { - // Never allow to delete the meta config branch. - return false; - } - - try { - permissions.ref(cmd.getRefName()).check(RefPermission.DELETE); - return projectState.statePermitsWrite(); - } catch (AuthException e) { - return false; + rejectProhibited(cmd, err.get()); } } @@ -1206,23 +1193,51 @@ class ReceiveCommits { } } - boolean ok; - try { - permissions.ref(cmd.getRefName()).check(RefPermission.FORCE_UPDATE); - ok = true; - } catch (AuthException err) { - ok = false; - } - if (ok) { + Optional err = checkRefPermission(cmd, RefPermission.FORCE_UPDATE); + if (!err.isPresent()) { if (!validRefOperation(cmd)) { return; } actualCommands.add(cmd); } else { - cmd.setResult(REJECTED_OTHER_REASON, "need '" + PermissionRule.FORCE_PUSH + "' privilege."); + rejectProhibited(cmd, err.get()); } } + private Optional checkRefPermission(ReceiveCommand cmd, RefPermission perm) + throws PermissionBackendException { + return checkRefPermission(permissions.ref(cmd.getRefName()), perm); + } + + private Optional checkRefPermission( + PermissionBackend.ForRef forRef, RefPermission perm) throws PermissionBackendException { + try { + forRef.check(perm); + return Optional.empty(); + } catch (AuthException e) { + return Optional.of(e); + } + } + + private void rejectProhibited(ReceiveCommand cmd, AuthException err) { + err.getAdvice().ifPresent(a -> errors.put(a, cmd.getRefName())); + reject(cmd, prohibited(err, cmd.getRefName())); + } + + private static String prohibited(AuthException e, String alreadyDisplayedResource) { + String msg = e.getMessage(); + if (e instanceof PermissionDeniedException) { + PermissionDeniedException pde = (PermissionDeniedException) e; + if (pde.getResource().isPresent() + && pde.getResource().get().equals(alreadyDisplayedResource)) { + // Avoid repeating resource name if exactly the given name was already displayed by the + // generic git push machinery. + msg = PermissionDeniedException.MESSAGE_PREFIX + pde.describePermission(); + } + } + return "prohibited by Gerrit: " + msg; + } + static class MagicBranchInput { private static final Splitter COMMAS = Splitter.on(',').omitEmptyStrings(); @@ -1550,11 +1565,9 @@ class ReceiveCommits { magicBranch.dest = new Branch.NameKey(project.getNameKey(), ref); magicBranch.perm = permissions.ref(ref); - try { - magicBranch.perm.check(RefPermission.CREATE_CHANGE); - } catch (AuthException denied) { - errors.put(ReceiveError.CODE_REVIEW, ref); - reject(cmd, denied.getMessage()); + Optional err = checkRefPermission(magicBranch.perm, RefPermission.CREATE_CHANGE); + if (err.isPresent()) { + rejectProhibited(cmd, err.get()); return; } if (!projectState.statePermitsWrite()) { @@ -1565,7 +1578,7 @@ class ReceiveCommits { // TODO(davido): Remove legacy support for drafts magic branch option // after repo-tool supports private and work-in-progress changes. if (magicBranch.draft && !receiveConfig.allowDrafts) { - errors.put(ReceiveError.CODE_REVIEW, ref); + errors.put(ReceiveError.CODE_REVIEW.get(), ref); reject(cmd, "draft workflow is disabled"); return; } @@ -1598,10 +1611,9 @@ class ReceiveCommits { } if (magicBranch.submit) { - try { - permissions.ref(ref).check(RefPermission.UPDATE_BY_SUBMIT); - } catch (AuthException e) { - reject(cmd, e.getMessage()); + err = checkRefPermission(magicBranch.perm, RefPermission.UPDATE_BY_SUBMIT); + if (err.isPresent()) { + rejectProhibited(cmd, err.get()); return; } } @@ -2801,20 +2813,21 @@ class ReceiveCommits { && !(MagicBranch.isMagicBranch(cmd.getRefName()) || NEW_PATCHSET_PATTERN.matcher(cmd.getRefName()).matches()) && pushOptions.containsKey(PUSH_OPTION_SKIP_VALIDATION)) { - try { - if (projectState.is(BooleanProjectConfig.USE_SIGNED_OFF_BY)) { - throw new AuthException( - "requireSignedOffBy prevents option " + PUSH_OPTION_SKIP_VALIDATION); - } - - perm.check(RefPermission.SKIP_VALIDATION); - if (!Iterables.isEmpty(rejectCommits)) { - throw new AuthException("reject-commits prevents " + PUSH_OPTION_SKIP_VALIDATION); - } - logger.atFine().log("Short-circuiting new commit validation"); - } catch (AuthException denied) { - reject(cmd, denied.getMessage()); + if (projectState.is(BooleanProjectConfig.USE_SIGNED_OFF_BY)) { + reject(cmd, "requireSignedOffBy prevents option " + PUSH_OPTION_SKIP_VALIDATION); + return; } + + Optional err = + checkRefPermission(permissions.ref(branch.get()), RefPermission.SKIP_VALIDATION); + if (err.isPresent()) { + rejectProhibited(cmd, err.get()); + return; + } + if (!Iterables.isEmpty(rejectCommits)) { + reject(cmd, "reject-commits prevents " + PUSH_OPTION_SKIP_VALIDATION); + } + logger.atFine().log("Short-circuiting new commit validation"); return; } diff --git a/java/com/google/gerrit/server/permissions/PermissionDeniedException.java b/java/com/google/gerrit/server/permissions/PermissionDeniedException.java new file mode 100644 index 0000000000..601826339f --- /dev/null +++ b/java/com/google/gerrit/server/permissions/PermissionDeniedException.java @@ -0,0 +1,58 @@ +// Copyright (C) 2018 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.permissions; + +import static com.google.common.base.Preconditions.checkNotNull; + +import com.google.gerrit.extensions.api.access.GerritPermission; +import com.google.gerrit.extensions.restapi.AuthException; +import java.util.Optional; + +/** + * This signals that some permission check failed. The message is short so it can print on a + * single-line in the Git output. + */ +public class PermissionDeniedException extends AuthException { + private static final long serialVersionUID = 1L; + + public static final String MESSAGE_PREFIX = "not permitted: "; + + private final GerritPermission permission; + private final Optional resource; + + public PermissionDeniedException(GerritPermission permission) { + super(MESSAGE_PREFIX + checkNotNull(permission).describeForException()); + this.permission = permission; + this.resource = Optional.empty(); + } + + public PermissionDeniedException(GerritPermission permission, String resource) { + super( + MESSAGE_PREFIX + + checkNotNull(permission).describeForException() + + " on " + + checkNotNull(resource)); + this.permission = permission; + this.resource = Optional.of(resource); + } + + public String describePermission() { + return permission.describeForException(); + } + + public Optional getResource() { + return resource; + } +} diff --git a/java/com/google/gerrit/server/permissions/RefControl.java b/java/com/google/gerrit/server/permissions/RefControl.java index 3bd2817e8a..fe441f47da 100644 --- a/java/com/google/gerrit/server/permissions/RefControl.java +++ b/java/com/google/gerrit/server/permissions/RefControl.java @@ -449,7 +449,88 @@ class RefControl { @Override public void check(RefPermission perm) throws AuthException, PermissionBackendException { if (!can(perm)) { - throw new AuthException(perm.describeForException() + " not permitted for " + refName); + PermissionDeniedException pde = new PermissionDeniedException(perm, refName); + switch (perm) { + case UPDATE: + if (refName.equals(RefNames.REFS_CONFIG)) { + pde.setAdvice( + "Configuration changes can only be pushed by project owners\n" + + "who also have 'Push' rights on " + + RefNames.REFS_CONFIG); + } else { + pde.setAdvice("To push into this reference you need 'Push' rights."); + } + break; + case DELETE: + pde.setAdvice( + "You need 'Delete Reference' rights or 'Push' rights with the \n" + + "'Force Push' flag set to delete references."); + break; + case CREATE_CHANGE: + // This is misleading in the default permission backend, since "create change" on a + // branch is encoded as "push" on refs/for/DESTINATION. + pde.setAdvice( + "You need 'Create Change' rights to upload code review requests.\n" + + "Verify that you are pushing to the right branch."); + break; + case CREATE: + pde.setAdvice("You need 'Create' rights to create new references."); + break; + case CREATE_SIGNED_TAG: + pde.setAdvice("You need 'Create Signed Tag' rights to push a signed tag."); + break; + case CREATE_TAG: + pde.setAdvice("You need 'Create Tag' rights to push a normal tag."); + break; + case FORCE_UPDATE: + pde.setAdvice( + "You need 'Push' rights with 'Force' flag set to do a non-fastforward push."); + break; + case FORGE_AUTHOR: + pde.setAdvice( + "You need 'Forge Author' rights to push commits with another user as author."); + break; + case FORGE_COMMITTER: + pde.setAdvice( + "You need 'Forge Committer' rights to push commits with another user as committer."); + break; + case FORGE_SERVER: + pde.setAdvice( + "You need 'Forge Server' rights to push merge commits authored by the server."); + break; + case MERGE: + pde.setAdvice( + "You need 'Push Merge' in addition to 'Push' rights to push merge commits."); + break; + + case READ: + pde.setAdvice("You need 'Read' rights to fetch or clone this ref."); + break; + + case READ_CONFIG: + pde.setAdvice("You need 'Read' rights on refs/meta/config to see the configuration."); + break; + case READ_PRIVATE_CHANGES: + pde.setAdvice("You need 'Read Private Changes' to see private changes."); + break; + case SET_HEAD: + pde.setAdvice("You need 'Set HEAD' rights to set the default branch."); + break; + case SKIP_VALIDATION: + pde.setAdvice( + "You need 'Forge Author', 'Forge Server', 'Forge Committer'\n" + + "and 'Push Merge' rights to skip validation."); + break; + case UPDATE_BY_SUBMIT: + pde.setAdvice( + "You need 'Submit' rights on refs/for/ to submit changes during change upload."); + break; + + case WRITE_CONFIG: + pde.setAdvice("You need 'Write' rights on refs/meta/config."); + break; + } + throw pde; } } diff --git a/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java b/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java index 8479dd1db5..6a9e41bfa8 100644 --- a/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java +++ b/javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java @@ -368,7 +368,7 @@ public class ProjectIT extends AbstractDaemonTest { gApi.projects().name(project.get()).branch("test").create(new BranchInput()); setApiUser(user); exception.expect(AuthException.class); - exception.expectMessage("set HEAD not permitted for refs/heads/test"); + exception.expectMessage("not permitted: set HEAD on refs/heads/test"); gApi.projects().name(project.get()).head("test"); } diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java index b4ae8a237c..f55ad4679e 100644 --- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java @@ -2079,7 +2079,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { allowGlobalCapabilities(REGISTERED_USERS, GlobalCapability.ACCESS_DATABASE); pr = pushOne(testRepo, c.name(), ref, false, false, opts); - assertPushRejected(pr, ref, "prohibited by Gerrit: create not permitted for " + ref); + assertPushRejected(pr, ref, "prohibited by Gerrit: not permitted: create"); grant(project, "refs/changes/*", Permission.CREATE); grant(project, "refs/changes/*", Permission.PUSH); diff --git a/javatests/com/google/gerrit/acceptance/git/ForcePushIT.java b/javatests/com/google/gerrit/acceptance/git/ForcePushIT.java index 87ac0229eb..d80faa8ee1 100644 --- a/javatests/com/google/gerrit/acceptance/git/ForcePushIT.java +++ b/javatests/com/google/gerrit/acceptance/git/ForcePushIT.java @@ -51,7 +51,7 @@ public class ForcePushIT extends AbstractDaemonTest { pushFactory.create(db, admin.getIdent(), testRepo, "change2", "b.txt", "content"); push2.setForce(true); PushOneCommit.Result r2 = push2.to("refs/heads/master"); - r2.assertErrorStatus("need 'Force Push' privilege."); + r2.assertErrorStatus("not permitted: force update"); } @Test diff --git a/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java b/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java index af74657375..fa9298fd18 100644 --- a/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java +++ b/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java @@ -83,11 +83,10 @@ public class PushPermissionsIT extends AbstractDaemonTest { PushResult r = push("HEAD:refs/heads/master"); assertThat(r) .onlyRef("refs/heads/master") - .isRejected("prohibited by Gerrit: ref update access denied"); + .isRejected("prohibited by Gerrit: not permitted: update"); assertThat(r) .hasMessages( "Branch refs/heads/master:", - "You are not allowed to perform this operation.", "To push into this reference you need 'Push' rights.", "User: admin", "Contact an administrator to fix the permissions"); @@ -98,16 +97,18 @@ public class PushPermissionsIT extends AbstractDaemonTest { public void nonFastForwardUpdateDenied() throws Exception { ObjectId commit = testRepo.commit().create(); PushResult r = push("+" + commit.name() + ":refs/heads/master"); - assertThat(r).onlyRef("refs/heads/master").isRejected("need 'Force Push' privilege."); - assertThat(r).hasNoMessages(); - // TODO(dborowitz): Why does this not mention refs? - assertThat(r).hasProcessed(ImmutableMap.of()); + assertThat(r) + .onlyRef("refs/heads/master") + .isRejected("prohibited by Gerrit: not permitted: force update"); + assertThat(r).hasProcessed(ImmutableMap.of("refs", 1)); } @Test public void deleteDenied() throws Exception { PushResult r = push(":refs/heads/master"); - assertThat(r).onlyRef("refs/heads/master").isRejected("cannot delete references"); + assertThat(r) + .onlyRef("refs/heads/master") + .isRejected("prohibited by Gerrit: not permitted: delete"); assertThat(r) .hasMessages( "Branch refs/heads/master:", @@ -124,8 +125,8 @@ public class PushPermissionsIT extends AbstractDaemonTest { PushResult r = push("HEAD:refs/heads/newbranch"); assertThat(r) .onlyRef("refs/heads/newbranch") - .isRejected("prohibited by Gerrit: create not permitted for refs/heads/newbranch"); - assertThat(r).hasNoMessages(); + .isRejected("prohibited by Gerrit: not permitted: create"); + assertThat(r).containsMessages("You need 'Create' rights to create new references."); assertThat(r).hasProcessed(ImmutableMap.of("refs", 1)); } @@ -139,18 +140,17 @@ public class PushPermissionsIT extends AbstractDaemonTest { testRepo.branch("HEAD").commit().create(); PushResult r = push(":refs/heads/foo", ":refs/heads/bar", "HEAD:refs/heads/master"); - assertThat(r).ref("refs/heads/foo").isRejected("cannot delete references"); - assertThat(r).ref("refs/heads/bar").isRejected("cannot delete references"); + assertThat(r).ref("refs/heads/foo").isRejected("prohibited by Gerrit: not permitted: delete"); + assertThat(r).ref("refs/heads/bar").isRejected("prohibited by Gerrit: not permitted: delete"); assertThat(r) .ref("refs/heads/master") - .isRejected("prohibited by Gerrit: ref update access denied"); + .isRejected("prohibited by Gerrit: not permitted: update"); assertThat(r) .hasMessages( "Branches refs/heads/foo, refs/heads/bar:", "You need 'Delete Reference' rights or 'Push' rights with the ", "'Force Push' flag set to delete references.", "Branch refs/heads/master:", - "You are not allowed to perform this operation.", "To push into this reference you need 'Push' rights.", "User: admin", "Contact an administrator to fix the permissions"); @@ -185,11 +185,10 @@ public class PushPermissionsIT extends AbstractDaemonTest { // ReceiveCommits theoretically has a different message when a WRITE_CONFIG check fails, but // it never gets there, since DefaultPermissionBackend special-cases refs/meta/config and // denies UPDATE if the user is not a project owner. - .isRejected("prohibited by Gerrit: ref update access denied"); + .isRejected("prohibited by Gerrit: not permitted: update"); assertThat(r) .hasMessages( "Branch refs/meta/config:", - "You are not allowed to perform this operation.", "Configuration changes can only be pushed by project owners", "who also have 'Push' rights on refs/meta/config", "User: admin", @@ -212,14 +211,12 @@ public class PushPermissionsIT extends AbstractDaemonTest { PushResult r = push("HEAD:refs/for/master"); assertThat(r) .onlyRef("refs/for/master") - .isRejected("create change not permitted for refs/heads/master"); + .isRejected("prohibited by Gerrit: not permitted: create change on refs/heads/master"); assertThat(r) - .hasMessages( - "Branch refs/heads/master:", - "You need 'Push' rights to upload code review requests.", - "Verify that you are pushing to the right branch.", - "User: admin", - "Contact an administrator to fix the permissions"); + .containsMessages( + "Branch refs/for/master:", + "You need 'Create Change' rights to upload code review requests.", + "Verify that you are pushing to the right branch."); assertThat(r).hasProcessed(ImmutableMap.of("refs", 1)); } @@ -234,8 +231,10 @@ public class PushPermissionsIT extends AbstractDaemonTest { PushResult r = push("HEAD:refs/for/master%submit"); assertThat(r) .onlyRef("refs/for/master%submit") - .isRejected("update by submit not permitted for refs/heads/master"); - assertThat(r).hasNoMessages(); + .isRejected("prohibited by Gerrit: not permitted: update by submit on refs/heads/master"); + assertThat(r) + .containsMessages( + "You need 'Submit' rights on refs/for/ to submit changes during change upload."); assertThat(r).hasProcessed(ImmutableMap.of("refs", 1)); } @@ -269,8 +268,11 @@ public class PushPermissionsIT extends AbstractDaemonTest { push(c -> c.setPushOptions(ImmutableList.of("skip-validation")), "HEAD:refs/heads/master"); assertThat(r) .onlyRef("refs/heads/master") - .isRejected("skip validation not permitted for refs/heads/master"); - assertThat(r).hasNoMessages(); + .isRejected("prohibited by Gerrit: not permitted: skip validation"); + assertThat(r) + .containsMessages( + "You need 'Forge Author', 'Forge Server', 'Forge Committer'", + "and 'Push Merge' rights to skip validation."); assertThat(r).hasProcessed(ImmutableMap.of("refs", 1)); } diff --git a/javatests/com/google/gerrit/acceptance/git/SubmitOnPushIT.java b/javatests/com/google/gerrit/acceptance/git/SubmitOnPushIT.java index 0705dca978..e2614dd45d 100644 --- a/javatests/com/google/gerrit/acceptance/git/SubmitOnPushIT.java +++ b/javatests/com/google/gerrit/acceptance/git/SubmitOnPushIT.java @@ -158,7 +158,7 @@ public class SubmitOnPushIT extends AbstractDaemonTest { @Test public void submitOnPushNotAllowed_Error() throws Exception { PushOneCommit.Result r = pushTo("refs/for/master%submit"); - r.assertErrorStatus("update by submit not permitted"); + r.assertErrorStatus("not permitted: update by submit"); } @Test @@ -170,7 +170,7 @@ public class SubmitOnPushIT extends AbstractDaemonTest { push( "refs/for/master%submit", PushOneCommit.SUBJECT, "a.txt", "other content", r.getChangeId()); - r.assertErrorStatus("update by submit not permitted"); + r.assertErrorStatus("not permitted: update by submit "); } @Test diff --git a/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java b/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java index 0017e08fde..df896864e5 100644 --- a/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java @@ -61,7 +61,7 @@ public class CreateBranchIT extends AbstractDaemonTest { @Test public void createBranch_Forbidden() throws Exception { setApiUser(user); - assertCreateFails(testBranch, AuthException.class, "create not permitted for refs/heads/test"); + assertCreateFails(testBranch, AuthException.class, "not permitted: create on refs/heads/test"); } @Test @@ -85,7 +85,7 @@ public class CreateBranchIT extends AbstractDaemonTest { @Test public void createBranchByAdminCreateReferenceBlocked_Forbidden() throws Exception { blockCreateReference(); - assertCreateFails(testBranch, AuthException.class, "create not permitted for refs/heads/test"); + assertCreateFails(testBranch, AuthException.class, "not permitted: create on refs/heads/test"); } @Test @@ -93,7 +93,7 @@ public class CreateBranchIT extends AbstractDaemonTest { grantOwner(); blockCreateReference(); setApiUser(user); - assertCreateFails(testBranch, AuthException.class, "create not permitted for refs/heads/test"); + assertCreateFails(testBranch, AuthException.class, "not permitted: create on refs/heads/test"); } @Test diff --git a/javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchIT.java b/javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchIT.java index 5e1b0bf566..b426a37085 100644 --- a/javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchIT.java @@ -193,7 +193,7 @@ public class DeleteBranchIT extends AbstractDaemonTest { private void assertDeleteForbidden(Branch.NameKey branch) throws Exception { assertThat(branch(branch).get().canDelete).isNull(); exception.expect(AuthException.class); - exception.expectMessage("delete not permitted"); + exception.expectMessage("not permitted: delete"); branch(branch).delete(); } } diff --git a/javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java b/javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java index 330f2b8399..c1bd8f1ca9 100644 --- a/javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java @@ -75,7 +75,7 @@ public class DeleteBranchesIT extends AbstractDaemonTest { project().deleteBranches(input); fail("Expected AuthException"); } catch (AuthException e) { - assertThat(e).hasMessageThat().isEqualTo("delete not permitted for refs/heads/test-1"); + assertThat(e).hasMessageThat().isEqualTo("not permitted: delete on refs/heads/test-1"); } setApiUser(admin); assertBranches(BRANCHES); diff --git a/javatests/com/google/gerrit/acceptance/rest/project/DeleteTagIT.java b/javatests/com/google/gerrit/acceptance/rest/project/DeleteTagIT.java index 0cbbe44206..3ae0b44001 100644 --- a/javatests/com/google/gerrit/acceptance/rest/project/DeleteTagIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/project/DeleteTagIT.java @@ -125,7 +125,7 @@ public class DeleteTagIT extends AbstractDaemonTest { private void assertDeleteForbidden() throws Exception { assertThat(tag().get().canDelete).isNull(); exception.expect(AuthException.class); - exception.expectMessage("delete not permitted"); + exception.expectMessage("not permitted: delete"); tag().delete(); } } diff --git a/javatests/com/google/gerrit/acceptance/rest/project/TagsIT.java b/javatests/com/google/gerrit/acceptance/rest/project/TagsIT.java index 714751db3f..d4edc0de95 100644 --- a/javatests/com/google/gerrit/acceptance/rest/project/TagsIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/project/TagsIT.java @@ -254,7 +254,7 @@ public class TagsIT extends AbstractDaemonTest { TagInput input = new TagInput(); input.ref = "test"; exception.expect(AuthException.class); - exception.expectMessage("create not permitted"); + exception.expectMessage("not permitted: create"); tag(input.ref).create(input); } diff --git a/plugins/hooks b/plugins/hooks index ca64db3126..07672f3188 160000 --- a/plugins/hooks +++ b/plugins/hooks @@ -1 +1 @@ -Subproject commit ca64db31265e985ab3cec635d6f063b0414c45e1 +Subproject commit 07672f31880ba80300b38492df9d0acfcd6ee00a