Make RefControl package-private
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
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ChangeProjectAccess.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ChangeProjectAccess.java
index 2adf029..129ccf8 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ChangeProjectAccess.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ChangeProjectAccess.java
@@ -19,11 +19,13 @@
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 @@
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 @@
metaDataUpdateFactory,
allProjects,
setParent,
+ user.get(),
+ permissionBackend,
projectName,
base,
sectionList,
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ProjectAccessFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ProjectAccessFactory.java
index 4cd6fa0..8c873a6 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ProjectAccessFactory.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ProjectAccessFactory.java
@@ -17,6 +17,7 @@
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 @@
}
} else if (RefConfigSection.isValid(name)) {
- if (pc.controlForRef(name).isOwner()) {
+ if (check(perm, name, WRITE_CONFIG)) {
local.add(section);
ownerOf.add(name);
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ProjectAccessHandler.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ProjectAccessHandler.java
index 3fa05ab..e92af7c 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ProjectAccessHandler.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ProjectAccessHandler.java
@@ -30,12 +30,15 @@
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 @@
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 @@
MetaDataUpdate.User metaDataUpdateFactory,
AllProjectsName allProjects,
Provider<SetParent> setParent,
+ CurrentUser user,
+ PermissionBackend permissionBackend,
Project.NameKey projectName,
ObjectId base,
List<AccessSection> sectionList,
@@ -85,6 +92,8 @@
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 @@
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 @@
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 @@
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));
}
}
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ReviewProjectAccess.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ReviewProjectAccess.java
index f27b9d3..a8a120a 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ReviewProjectAccess.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ReviewProjectAccess.java
@@ -30,6 +30,7 @@
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 @@
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 @@
metaDataUpdateFactory,
allProjects,
setParent,
+ user.get(),
+ permissionBackend,
projectName,
base,
sectionList,
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
index 2dd5f2a..ed86a92 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/VisibleRefFilter.java
@@ -38,7 +38,6 @@
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 @@
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 @@
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 @@
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;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/CreateChangeSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/CreateChangeSender.java
index 6d15d6f..8956f10 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/CreateChangeSender.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/CreateChangeSender.java
@@ -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 @@
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 @@
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 @@
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);
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ProjectWatch.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ProjectWatch.java
index e1b6e36..a914a73 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ProjectWatch.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ProjectWatch.java
@@ -119,12 +119,25 @@
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:
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/RefPermission.java b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/RefPermission.java
index 8b5d8fb..9f3dda1 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/permissions/RefPermission.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/permissions/RefPermission.java
@@ -41,7 +41,19 @@
* 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;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java
index a92c0b7..f693bf5 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java
@@ -44,7 +44,7 @@
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 @@
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 @@
}
/** Is this user a ref owner? */
- public boolean isOwner() {
+ boolean isOwner() {
if (owner == null) {
if (canPerform(Permission.OWNER)) {
owner = true;
@@ -109,11 +109,6 @@
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 @@
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 @@
}
/** 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 @@
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()
