opendev/gerrit
Code Issues Proposed changes

Make RefControl package-private

Browse Source 
This commit makes RefControl package-private by removing all references
by migrating all callers to PermissionBackend. It makes the following
non-trivial changes:

1) Decompose ref-ownership into READ_CONFIG and WRITE_CONFIG.
WRITE_CONFIG serves as the traditional isOwner() while READ_CONFIG can
be used to check if the user can read the ref config. This defaults to
READ on refs/meta/config for now but can be more specific in the future.

2) Add a new READ_PRIVATE_CHANGES permission to RefPermission to account
for canReadPrivateChanges() and isEditVisible(). This is used for
VisibleRefsFilter.

This commit leaves a TODO for the future on how to treat ref owners in
emails. As of now, we still upgrade owners to 'TO' when they are on
either 'CC' or 'BCC'. This will change in a follow-up change as it is
hard to support on top of a permission backend as it involves many
permission checks on every email sent as well as confusing as internally
we don't have a ref owner anymore.

Change-Id: Ia6fa468dac49588241b52b4451fe79bcf6776077
This commit is contained in:
Patrick Hiesel
2017-08-25 15:55:26 +02:00
parent e3762b84ec
commit 1851a8b786
9 changed files with 105 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ChangeProjectAccess.java
View File

@@ -19,11 +19,13 @@ import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.ProjectAccess;
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.account.GroupBackend;
import com.google.gerrit.server.config.AllProjectsName;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
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.project.ContributorAgreementsChecker;
import com.google.gerrit.server.project.NoSuchProjectException;
@@ -64,6 +66,8 @@ class ChangeProjectAccess extends ProjectAccessHandler<ProjectAccess> {
Provider<SetParent> setParent,
GitReferenceUpdated gitRefUpdated,
ContributorAgreementsChecker contributorAgreements,
Provider<CurrentUser> user,
PermissionBackend permissionBackend,
@Assisted("projectName") Project.NameKey projectName,
@Nullable @Assisted ObjectId base,
@Assisted List<AccessSection> sectionList,
@@ -75,6 +79,8 @@ class ChangeProjectAccess extends ProjectAccessHandler<ProjectAccess> {
metaDataUpdateFactory,
allProjects,
setParent,
user.get(),
permissionBackend,
projectName,
base,
sectionList,

3
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ProjectAccessFactory.java
View File

@@ -17,6 +17,7 @@ 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 static com.google.gerrit.server.permissions.RefPermission.WRITE_CONFIG;
import com.google.common.collect.Maps;
import com.google.gerrit.common.data.AccessSection;
@@ -145,7 +146,7 @@ class ProjectAccessFactory extends Handler<ProjectAccess> {
}
} else if (RefConfigSection.isValid(name)) {
if (pc.controlForRef(name).isOwner()) {
if (check(perm, name, WRITE_CONFIG)) {
local.add(section);
ownerOf.add(name);

14
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ProjectAccessHandler.java
View File

@@ -30,12 +30,15 @@ import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.httpd.rpc.Handler;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.account.GroupBackends;
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.gerrit.server.project.ContributorAgreementsChecker;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectControl;
@@ -59,6 +62,8 @@ public abstract class ProjectAccessHandler<T> extends Handler<T> {
private final AllProjectsName allProjects;
private final Provider<SetParent> setParent;
private final ContributorAgreementsChecker contributorAgreements;
private final CurrentUser user;
private final PermissionBackend permissionBackend;
protected final Project.NameKey projectName;
protected final ObjectId base;
@@ -73,6 +78,8 @@ public abstract class ProjectAccessHandler<T> extends Handler<T> {
MetaDataUpdate.User metaDataUpdateFactory,
AllProjectsName allProjects,
Provider<SetParent> setParent,
CurrentUser user,
PermissionBackend permissionBackend,
Project.NameKey projectName,
ObjectId base,
List<AccessSection> sectionList,
@@ -85,6 +92,8 @@ public abstract class ProjectAccessHandler<T> extends Handler<T> {
this.metaDataUpdateFactory = metaDataUpdateFactory;
this.allProjects = allProjects;
this.setParent = setParent;
this.user = user;
this.permissionBackend = permissionBackend;
this.projectName = projectName;
this.base = base;
@@ -111,6 +120,7 @@ public abstract class ProjectAccessHandler<T> extends Handler<T> {
try (MetaDataUpdate md = metaDataUpdateFactory.create(projectName)) {
ProjectConfig config = ProjectConfig.read(md, base);
Set<String> toDelete = scanSectionNames(config);
PermissionBackend.ForProject forProject = permissionBackend.user(user).project(projectName);
for (AccessSection section : mergeSections(sectionList)) {
String name = section.getName();
@@ -122,7 +132,7 @@ public abstract class ProjectAccessHandler<T> extends Handler<T> {
replace(config, toDelete, section);
} else if (AccessSection.isValid(name)) {
if (checkIfOwner && !projectControl.controlForRef(name).isOwner()) {
if (checkIfOwner && !forProject.ref(name).test(RefPermission.WRITE_CONFIG)) {
continue;
}
@@ -138,7 +148,7 @@ public abstract class ProjectAccessHandler<T> extends Handler<T> {
config.remove(config.getAccessSection(name));
}
} else if (!checkIfOwner || projectControl.controlForRef(name).isOwner()) {
} else if (!checkIfOwner || forProject.ref(name).test(RefPermission.WRITE_CONFIG)) {
config.remove(config.getAccessSection(name));
}
}

4
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ReviewProjectAccess.java
View File

@@ -30,6 +30,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
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.Sequences;
import com.google.gerrit.server.account.GroupBackend;
import com.google.gerrit.server.change.ChangeInserter;
@@ -96,6 +97,7 @@ public class ReviewProjectAccess extends ProjectAccessHandler<Change.Id> {
Provider<SetParent> setParent,
Sequences seq,
ContributorAgreementsChecker contributorAgreements,
Provider<CurrentUser> user,
@Assisted("projectName") Project.NameKey projectName,
@Nullable @Assisted ObjectId base,
@Assisted List<AccessSection> sectionList,
@@ -107,6 +109,8 @@ public class ReviewProjectAccess extends ProjectAccessHandler<Change.Id> {
metaDataUpdateFactory,
allProjects,
setParent,
user.get(),
permissionBackend,
projectName,
base,
sectionList,

21
gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
View File

@@ -38,7 +38,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.ProjectControl;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
@@ -80,7 +79,6 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook {
private final PermissionBackend.ForProject perm;
private final ProjectState projectState;
private final Repository git;
private ProjectControl projectCtl;
private boolean showMetadata = true;
private String userEditPrefix;
private Map<Change.Id, Branch.NameKey> visibleChanges;
@@ -141,7 +139,6 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook {
Map<String, Ref> result = new HashMap<>();
List<Ref> deferredTags = new ArrayList<>();
projectCtl = projectState.controlFor(user.get());
for (Ref ref : refs.values()) {
String name = ref.getName();
Change.Id changeId;
@@ -263,10 +260,20 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook {
if (visibleChanges == null) {
visible(id);
}
if (id != null) {
return (userEditPrefix != null && name.startsWith(userEditPrefix) && visible(id))
|| (visibleChanges.containsKey(id)
&& projectCtl.controlForRef(visibleChanges.get(id)).isEditVisible());
if (id == null) {
return false;
}
if (userEditPrefix != null && name.startsWith(userEditPrefix) && visible(id)) {
return true;
}
if (visibleChanges.containsKey(id)) {
try {
// Default to READ_PRIVATE_CHANGES as there is no special permission for reading edits.
perm.ref(visibleChanges.get(id).get()).check(RefPermission.READ_PRIVATE_CHANGES);
return true;
} catch (PermissionBackendException | AuthException e) {
return false;
}
}
return false;
}

40
gerrit-server/src/main/java/com/google/gerrit/server/mail/send/CreateChangeSender.java
View File

@@ -14,17 +14,20 @@
package com.google.gerrit.server.mail.send;
import com.google.common.collect.Iterables;
import com.google.gerrit.common.errors.EmailException;
import com.google.gerrit.extensions.api.changes.RecipientType;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.WatchConfig.NotifyType;
import com.google.gerrit.server.mail.send.ProjectWatch.Watchers;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.RefPermission;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.util.stream.StreamSupport;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -36,11 +39,20 @@ public class CreateChangeSender extends NewChangeSender {
CreateChangeSender create(Project.NameKey project, Change.Id id);
}
private final IdentifiedUser.GenericFactory identifiedUserFactory;
private final PermissionBackend permissionBackend;
@Inject
public CreateChangeSender(
EmailArguments ea, @Assisted Project.NameKey project, @Assisted Change.Id id)
EmailArguments ea,
IdentifiedUser.GenericFactory identifiedUserFactory,
PermissionBackend permissionBackend,
@Assisted Project.NameKey project,
@Assisted Change.Id id)
throws OrmException {
super(ea, newChangeData(ea, project, id));
this.identifiedUserFactory = identifiedUserFactory;
this.permissionBackend = permissionBackend;
}
@Override
@@ -48,16 +60,13 @@ public class CreateChangeSender extends NewChangeSender {
super.init();
try {
// Try to mark interested owners with TO and CC or BCC line.
// Upgrade watching owners from CC and BCC to TO.
Watchers matching =
getWatchers(NotifyType.NEW_CHANGES, !change.isWorkInProgress() && !change.isPrivate());
for (Account.Id user :
Iterables.concat(matching.to.accounts, matching.cc.accounts, matching.bcc.accounts)) {
if (isOwnerOfProjectOrBranch(user)) {
add(RecipientType.TO, user);
}
}
// TODO(hiesel): Remove special handling for owners
StreamSupport.stream(matching.all().accounts.spliterator(), false)
.filter(acc -> isOwnerOfProjectOrBranch(acc))
.forEach(acc -> add(RecipientType.TO, acc));
// Add everyone else. Owners added above will not be duplicated.
add(RecipientType.TO, matching.to);
add(RecipientType.CC, matching.cc);
@@ -72,11 +81,10 @@ public class CreateChangeSender extends NewChangeSender {
includeWatchers(NotifyType.NEW_PATCHSETS, !change.isWorkInProgress() && !change.isPrivate());
}
private boolean isOwnerOfProjectOrBranch(Account.Id user) {
return projectState != null
&& projectState
.controlFor(args.identifiedUserFactory.create(user))
.controlForRef(change.getDest())
.isOwner();
private boolean isOwnerOfProjectOrBranch(Account.Id userId) {
return permissionBackend
.user(identifiedUserFactory.create(userId))
.ref(change.getDest())
.testOrFalse(RefPermission.WRITE_CONFIG);
}
}

13
gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ProjectWatch.java
View File

@@ -119,12 +119,25 @@ public class ProjectWatch {
static class List {
protected final Set<Account.Id> accounts = new HashSet<>();
protected final Set<Address> emails = new HashSet<>();
private static List union(List... others) {
List union = new List();
for (List other : others) {
union.accounts.addAll(other.accounts);
union.emails.addAll(other.emails);
}
return union;
}
}
protected final List to = new List();
protected final List cc = new List();
protected final List bcc = new List();
List all() {
return List.union(to, cc, bcc);
}
List list(NotifyConfig.Header header) {
switch (header) {
case TO:

14
gerrit-server/src/main/java/com/google/gerrit/server/permissions/RefPermission.java
View File

@@ -41,7 +41,19 @@ public enum RefPermission {
* according to the submit strategy, which may include cherry-pick or rebase. By creating changes
* for each commit, automatic server side rebase, and post-update review are enabled.
*/
UPDATE_BY_SUBMIT;
UPDATE_BY_SUBMIT,
/**
* Can read all private changes on the ref. Typically granted to CI systems if they should run on
* private changes.
*/
READ_PRIVATE_CHANGES(Permission.VIEW_PRIVATE_CHANGES),
/** Read access to ref's config section in {@code project.config}. */
READ_CONFIG,
/** Write access to ref's config section in {@code project.config}. */
WRITE_CONFIG;
private final String name;

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

@@ -44,7 +44,7 @@ import java.util.Map;
import java.util.Set;
/** Manages access control for Git references (aka branches, tags). */
public class RefControl {
class RefControl {
private final ProjectControl projectControl;
private final String refName;
@@ -66,19 +66,19 @@ public class RefControl {
this.effective = new HashMap<>();
}
public String getRefName() {
String getRefName() {
return refName;
}
public ProjectControl getProjectControl() {
ProjectControl getProjectControl() {
return projectControl;
}
public CurrentUser getUser() {
CurrentUser getUser() {
return projectControl.getUser();
}
public RefControl forUser(CurrentUser who) {
RefControl forUser(CurrentUser who) {
ProjectControl newCtl = projectControl.forUser(who);
if (relevant.isUserSpecific()) {
return newCtl.controlForRef(getRefName());
@@ -87,7 +87,7 @@ public class RefControl {
}
/** Is this user a ref owner? */
public boolean isOwner() {
boolean isOwner() {
if (owner == null) {
if (canPerform(Permission.OWNER)) {
owner = true;
@@ -109,11 +109,6 @@ public class RefControl {
return isVisible;
}
/** Can this user see other users change edits? */
public boolean isEditVisible() {
return canViewPrivateChanges();
}
private boolean canUpload() {
return projectControl.controlForRef("refs/for/" + getRefName()).canPerform(Permission.PUSH)
&& isProjectStatePermittingWrite();
@@ -275,11 +270,6 @@ public class RefControl {
return canPerform(Permission.ABANDON);
}
/** @return true if this user can remove a reviewer for a change. */
boolean canRemoveReviewer() {
return canPerform(Permission.REMOVE_REVIEWER);
}
/** @return true if this user can view private changes. */
boolean canViewPrivateChanges() {
return canPerform(Permission.VIEW_PRIVATE_CHANGES);
@@ -395,7 +385,7 @@ public class RefControl {
}
/** True if the user is blocked from using this permission. */
public boolean isBlocked(String permissionName) {
boolean isBlocked(String permissionName) {
return !doCanPerform(permissionName, false, true);
}
@@ -577,6 +567,16 @@ public class RefControl {
case UPDATE_BY_SUBMIT:
return projectControl.controlForRef("refs/for/" + getRefName()).canSubmit(true);
case READ_PRIVATE_CHANGES:
return canViewPrivateChanges();
case READ_CONFIG:
return projectControl
.controlForRef(RefNames.REFS_CONFIG)
.canPerform(RefPermission.READ.name());
case WRITE_CONFIG:
return isOwner();
case SKIP_VALIDATION:
return canForgeAuthor()
&& canForgeCommitter()