opendev/gerrit
Code Issues Proposed changes

Push ReceiveError advice into structured exception type

Browse Source 
The ReceiveCommits.ReceiveError is very specific to the
DefaultPermissionBackend. Provide a way for other backend
implementations to provide their own advice, by enriching the
AuthException hierarchy.

We now have a PermissionDeniedException which records up to three items:
 * The description of the permission, as returned by
   GerritPermission#describeForException.
 * Free-form advice for addressing the issue.
 * The resource name, if applicable.

Now, ReceiveCommits doesn't have to know about the ReceiveError text;
DefaultPermissionBackend can just set the advice on any AuthExceptions
it throws. Currently, the advice is only displayed over the wire during
ReceiveCommits, but one could imagine that it might be helpful in bare
HTTP responses as well, for example.

Update the push permissions test to account for the new messages. Note
that they are much more consistent and follow a common format, which is
not surprising since a goal of this change was to eliminate much of the
hard-coding in ReceiveCommits.

There are a few tests in PushPermisisonsIT which still show hard-coded
error messages. These will require a bit more work to make sure they get
understandable messages and advice.

Change-Id: I34dc173ffd040829d8ff2853d4a728fa323137b5
This commit is contained in:
Han-Wen Nienhuys 2018-08-16 13:43:49 +02:00
parent 15e30e5639
commit 4e8a19ccf9
17 changed files with 303 additions and 114 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
java/com/google/gerrit/extensions/api/access/GerritPermission.java
View File

@ -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.
*
* <p>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) {

22
java/com/google/gerrit/extensions/restapi/AuthException.java
View File

@ -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<String> 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.
*
* <p>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<String> getAdvice() {
return advice;
}
}

7
java/com/google/gerrit/git/testing/PushResultSubject.java
View File

@ -54,6 +54,13 @@ public class PushResultSubject extends Subject<PushResultSubject, PushResult> {
Truth.assertThat(trimMessages()).isEqualTo(Arrays.stream(expectedLines).collect(joining("\n")));
}
public void containsMessages(String... expectedLines) {
checkArgument(expectedLines.length > 0, "use hasNoMessages()");
isNotNull();
Iterable<String> got = Splitter.on("\n").split(trimMessages());
Truth.assertThat(got).containsAllIn(expectedLines).inOrder();
}
private String trimMessages() {
return trimMessages(actual().getMessages());
}

159
java/com/google/gerrit/server/git/receive/ReceiveCommits.java
View File

@ -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<UpdateGroupsRequest> updateGroups;
private final List<ValidationMessage> messages;
private final ListMultimap<ReceiveError, String> errors;
/** Multimap of error text to refnames that produced that error. */
private final ListMultimap<String, String> errors;
private final ListMultimap<String, String> pushOptions;
private final Map<Change.Id, ReplaceRequest> 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<String> branches) {
private String buildError(String error, List<String> 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<CurrentUser> 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<AuthException> 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<AuthException> 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<AuthException> 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<AuthException> checkRefPermission(ReceiveCommand cmd, RefPermission perm)
throws PermissionBackendException {
return checkRefPermission(permissions.ref(cmd.getRefName()), perm);
}
private Optional<AuthException> 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<AuthException> 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<AuthException> 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;
}

58
java/com/google/gerrit/server/permissions/PermissionDeniedException.java Normal file
View File

@ -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<String> 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<String> getResource() {
return resource;
}
}

83
java/com/google/gerrit/server/permissions/RefControl.java
View File

@ -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;
}
}

2
javatests/com/google/gerrit/acceptance/api/project/ProjectIT.java
View File

@ -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");
}

2
javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java
View File

@ -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);

2
javatests/com/google/gerrit/acceptance/git/ForcePushIT.java
View File

@ -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

54
javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java
View File

@ -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));
}

4
javatests/com/google/gerrit/acceptance/git/SubmitOnPushIT.java
View File

@ -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

6
javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java
View File

@ -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

2
javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchIT.java
View File

@ -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();
}
}

2
javatests/com/google/gerrit/acceptance/rest/project/DeleteBranchesIT.java
View File

@ -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);

2
javatests/com/google/gerrit/acceptance/rest/project/DeleteTagIT.java
View File

@ -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();
}
}

2
javatests/com/google/gerrit/acceptance/rest/project/TagsIT.java
View File

@ -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);
}

2
plugins/hooks

@ -1 +1 @@
Subproject commit ca64db31265e985ab3cec635d6f063b0414c45e1
Subproject commit 07672f31880ba80300b38492df9d0acfcd6ee00a