opendev/gerrit
Code Issues Proposed changes

Merge "Add on_behalf_of to /review REST API handler"

Browse Source
This commit is contained in:
Shawn Pearce
2013-06-12 17:20:58 +00:00
committed by Gerrit Code Review
parent 43c1989285 9d78312c6c
commit fa34015afe
13 changed files with 118 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files

3
Documentation/access-control.txt
View File

@@ -747,7 +747,8 @@ Review Labels
For every configured label `My-Name` in the project, there is a
corresponding permission `label-My-Name` with a range corresponding to
the defined values.
the defined values. There is also a corresponding `labelAs-My-Name`
permission that enables editing another user's label.
Gerrit comes pre-configured with a default 'Code-Review' label that can
be granted to groups within projects, enabling functionality for that

4
Documentation/rest-api-changes.txt
View File

@@ -2705,6 +2705,10 @@ Notify handling that defines to whom email notifications should be sent
after the review is stored. +
Allowed values are `NONE`, `OWNER`, `OWNER_REVIEWERS` and `ALL`. +
If not set, the default is `ALL`.
|`on_behalf_of`|optional|
link:rest-api-accounts.html#account-id[\{account-id\}] the review
should be posted on behalf of. To use this option the caller must
have been granted `labelAs-NAME` permission for all keys of labels.
|============================
[[reviewer-info]]

46
gerrit-common/src/main/java/com/google/gerrit/common/data/Permission.java
View File

@@ -29,6 +29,7 @@ public class Permission implements Comparable<Permission> {
public static final String FORGE_COMMITTER = "forgeCommitter";
public static final String FORGE_SERVER = "forgeServerAsCommitter";
public static final String LABEL = "label-";
public static final String LABEL_AS = "labelAs-";
public static final String OWNER = "owner";
public static final String PUBLISH_DRAFTS = "publishDrafts";
public static final String PUSH = "push";
@@ -43,6 +44,7 @@ public class Permission implements Comparable<Permission> {
private static final List<String> NAMES_LC;
private static final int labelIndex;
private static final int labelAsIndex;
static {
NAMES_LC = new ArrayList<String>();
@@ -58,6 +60,7 @@ public class Permission implements Comparable<Permission> {
NAMES_LC.add(PUSH_TAG.toLowerCase());
NAMES_LC.add(PUSH_SIGNED_TAG.toLowerCase());
NAMES_LC.add(LABEL.toLowerCase());
NAMES_LC.add(LABEL_AS.toLowerCase());
NAMES_LC.add(REBASE.toLowerCase());
NAMES_LC.add(REMOVE_REVIEWER.toLowerCase());
NAMES_LC.add(SUBMIT.toLowerCase());
@@ -67,15 +70,18 @@ public class Permission implements Comparable<Permission> {
NAMES_LC.add(PUBLISH_DRAFTS.toLowerCase());
labelIndex = NAMES_LC.indexOf(Permission.LABEL);
labelAsIndex = NAMES_LC.indexOf(Permission.LABEL_AS.toLowerCase());
}
/** @return true if the name is recognized as a permission name. */
public static boolean isPermission(String varName) {
String lc = varName.toLowerCase();
if (lc.startsWith(LABEL)) {
return LABEL.length() < lc.length();
return isLabel(varName)
|| isLabelAs(varName)
|| NAMES_LC.contains(varName.toLowerCase());
}
return NAMES_LC.contains(lc);
public static boolean hasRange(String varName) {
return isLabel(varName) || isLabelAs(varName);
}
/** @return true if the permission name is actually for a review label. */
@@ -83,11 +89,30 @@ public class Permission implements Comparable<Permission> {
return varName.startsWith(LABEL) && LABEL.length() < varName.length();
}
/** @return true if the permission is for impersonated review labels. */
public static boolean isLabelAs(String var) {
return var.startsWith(LABEL_AS) && LABEL_AS.length() < var.length();
}
/** @return permission name for the given review label. */
public static String forLabel(String labelName) {
return LABEL + labelName;
}
/** @return permission name to apply a label for another user. */
public static String forLabelAs(String labelName) {
return LABEL_AS + labelName;
}
public static String extractLabel(String varName) {
if (isLabel(varName)) {
return varName.substring(LABEL.length());
} else if (isLabelAs(varName)) {
return varName.substring(LABEL_AS.length());
}
return null;
}
public static boolean canBeOnAllProjects(String ref, String permissionName) {
if (AccessSection.ALL.equals(ref)) {
return !OWNER.equals(permissionName);
@@ -110,15 +135,8 @@ public class Permission implements Comparable<Permission> {
return name;
}
public boolean isLabel() {
return isLabel(getName());
}
public String getLabel() {
if (isLabel()) {
return getName().substring(LABEL.length());
}
return null;
return extractLabel(getName());
}
public Boolean getExclusiveGroup() {
@@ -223,8 +241,10 @@ public class Permission implements Comparable<Permission> {
}
private static int index(Permission a) {
if (a.isLabel()) {
if (isLabel(a.getName())) {
return labelIndex;
} else if (isLabelAs(a.getName())) {
return labelAsIndex;
}
int index = NAMES_LC.indexOf(a.getName().toLowerCase());

2
gerrit-common/src/main/java/com/google/gerrit/common/data/PermissionRange.java
View File

@@ -86,7 +86,7 @@ public class PermissionRange implements Comparable<PermissionRange> {
}
public String getLabel() {
return isLabel() ? getName().substring(Permission.LABEL.length()) : null;
return Permission.extractLabel(getName());
}
public int getMin() {

5
gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AccessSectionEditor.java
View File

@@ -226,7 +226,10 @@ public class AccessSectionEditor extends Composite implements
}
} else if (RefConfigSection.isValid(value.getName())) {
for (LabelType t : projectAccess.getLabelTypes().getLabelTypes()) {
addPermission(Permission.LABEL + t.getName(), perms);
addPermission(Permission.forLabel(t.getName()), perms);
}
for (LabelType t : projectAccess.getLabelTypes().getLabelTypes()) {
addPermission(Permission.forLabelAs(t.getName()), perms);
}
for (String varName : Util.C.permissionNames().keySet()) {
addPermission(varName, perms);

1
gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminMessages.java
View File

@@ -19,6 +19,7 @@ import com.google.gwt.i18n.client.Messages;
public interface AdminMessages extends Messages {
String group(String name);
String label(String name);
String labelAs(String name);
String project(String name);
String deletedGroup(int id);

1
gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/AdminMessages.properties
View File

@@ -1,5 +1,6 @@
group = Group {0}
label = Label {0}
labelAs = Label {0} (On Behalf Of)
project = Project {0}
deletedGroup = Deleted Group {0}
deletedReference = Reference {0} was deleted

2
gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionEditor.java
View File

@@ -265,7 +265,7 @@ public class PermissionEditor extends Composite implements Editor<Permission>,
public void setValue(Permission value) {
this.value = value;
if (value.isLabel()) {
if (Permission.hasRange(value.getName())) {
LabelType lt = labelTypes.byLabel(value.getLabel());
if (lt != null) {
validRange = new PermissionRange.WithDefaults(

6
gerrit-gwtui/src/main/java/com/google/gerrit/client/admin/PermissionNameRenderer.java
View File

@@ -40,8 +40,10 @@ class PermissionNameRenderer implements Renderer<String> {
@Override
public String render(String varName) {
if (Permission.isLabel(varName)) {
return Util.M.label(new Permission(varName).getLabel());
if (Permission.isLabelAs(varName)) {
return Util.M.labelAs(Permission.extractLabel(varName));
} else if (Permission.isLabel(varName)) {
return Util.M.label(Permission.extractLabel(varName));
}
String desc = permissions.get(varName);

2
gerrit-server/src/main/java/com/google/gerrit/server/account/AccountsCollection.java
View File

@@ -78,7 +78,7 @@ public class AccountsCollection implements
* "Full Name <email@example.com>", just the email address, a full name
* if it is unique, an account ID, a user name or 'self' for the
* calling user
* @return the project
* @return the user, never null.
* @throws UnprocessableEntityException thrown if the account ID cannot be
* resolved or if the account is not visible to the calling user
*/

62
gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
View File

@@ -30,6 +30,7 @@ import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BadRequestException;
import com.google.gerrit.extensions.restapi.DefaultInput;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.extensions.restapi.Url;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
@@ -39,6 +40,7 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountsCollection;
import com.google.gerrit.server.change.PostReview.Input;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gwtorm.server.OrmException;
@@ -81,6 +83,18 @@ public class PostReview implements RestModifyView<RevisionResource, Input> {
/** Who to send email notifications to after review is stored. */
public NotifyHandling notify = NotifyHandling.ALL;
/**
* Account ID, name, email address or username of another user. The review
* will be posted/updated on behalf of this named user instead of the
* caller. Caller must have the labelAs-$NAME permission granted for each
* label that appears in {@link #labels}. This is in addition to the named
* user also needing to have permission to use the labels.
* <p>
* {@link #strictLabels} impacts how labels is processed for the named user,
* not the caller.
*/
public String onBehalfOf;
}
public static enum DraftHandling {
@@ -104,6 +118,7 @@ public class PostReview implements RestModifyView<RevisionResource, Input> {
}
private final ReviewDb db;
private final AccountsCollection accounts;
private final EmailReviewComments.Factory email;
@Deprecated private final ChangeHooks hooks;
@@ -116,16 +131,22 @@ public class PostReview implements RestModifyView<RevisionResource, Input> {
@Inject
PostReview(ReviewDb db,
AccountsCollection accounts,
EmailReviewComments.Factory email,
ChangeHooks hooks) {
this.db = db;
this.accounts = accounts;
this.email = email;
this.hooks = hooks;
}
@Override
public Object apply(RevisionResource revision, Input input)
throws AuthException, BadRequestException, OrmException {
throws AuthException, BadRequestException, OrmException,
UnprocessableEntityException {
if (input.onBehalfOf != null) {
revision = onBehalfOf(revision, input);
}
if (input.labels != null) {
checkLabels(revision, input.strictLabels, input.labels);
}
@@ -171,6 +192,45 @@ public class PostReview implements RestModifyView<RevisionResource, Input> {
return output;
}
private RevisionResource onBehalfOf(RevisionResource rev, Input in)
throws BadRequestException, AuthException, UnprocessableEntityException,
OrmException {
if (in.labels == null || in.labels.isEmpty()) {
throw new AuthException(String.format(
"label required to post review on behalf of \"%s\"",
in.onBehalfOf));
}
ChangeControl caller = rev.getControl();
Iterator<Map.Entry<String, Short>> itr = in.labels.entrySet().iterator();
while (itr.hasNext()) {
Map.Entry<String, Short> ent = itr.next();
LabelType type = caller.getLabelTypes().byLabel(ent.getKey());
if (type == null && in.strictLabels) {
throw new BadRequestException(String.format(
"label \"%s\" is not a configured label", ent.getKey()));
} else if (type == null) {
itr.remove();
continue;
}
PermissionRange r = caller.getRange(Permission.forLabelAs(type.getName()));
if (r == null || r.isEmpty() || !r.contains(ent.getValue())) {
throw new AuthException(String.format(
"not permitted to modify label \"%s\" on behalf of \"%s\"",
ent.getKey(), in.onBehalfOf));
}
}
if (in.labels.isEmpty()) {
throw new AuthException(String.format(
"label required to post review on behalf of \"%s\"",
in.onBehalfOf));
}
ChangeControl target = caller.forUser(accounts.parse(in.onBehalfOf));
return new RevisionResource(new ChangeResource(target), rev.getPatchSet());
}
private void checkLabels(RevisionResource revision, boolean strict,
Map<String, Short> labels) throws BadRequestException, AuthException {
ChangeControl ctl = revision.getControl();

4
gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java
View File

@@ -512,7 +512,7 @@ public class ProjectConfig extends VersionedMetaData {
if (isPermission(varName)) {
Permission perm = as.getPermission(varName, true);
loadPermissionRules(rc, ACCESS, refName, varName, groupsByName,
perm, perm.isLabel());
perm, Permission.hasRange(varName));
}
}
}
@@ -869,7 +869,7 @@ public class ProjectConfig extends VersionedMetaData {
for (Permission permission : sort(as.getPermissions())) {
have.add(permission.getName().toLowerCase());
boolean needRange = permission.isLabel();
boolean needRange = Permission.hasRange(permission.getName());
List<String> rules = new ArrayList<String>();
for (PermissionRule rule : sort(permission.getRules())) {
GroupReference group = rule.getGroup();

2
gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java
View File

@@ -395,7 +395,7 @@ public class RefControl {
/** The range of permitted values associated with a label permission. */
public PermissionRange getRange(String permission) {
if (Permission.isLabel(permission)) {
if (Permission.hasRange(permission)) {
return toRange(permission, access(permission));
}
return null;