From 1851a8b7860b15ed63e05f8d72ecdb7efac32b3f Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Fri, 25 Aug 2017 15:55:26 +0200 Subject: [PATCH] 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 --- .../rpc/project/ChangeProjectAccess.java | 6 +++ .../rpc/project/ProjectAccessFactory.java | 3 +- .../rpc/project/ProjectAccessHandler.java | 14 ++++++- .../rpc/project/ReviewProjectAccess.java | 4 ++ .../gerrit/server/git/VisibleRefFilter.java | 21 ++++++---- .../server/mail/send/CreateChangeSender.java | 40 +++++++++++-------- .../gerrit/server/mail/send/ProjectWatch.java | 13 ++++++ .../server/permissions/RefPermission.java | 14 ++++++- .../gerrit/server/project/RefControl.java | 34 ++++++++-------- 9 files changed, 105 insertions(+), 44 deletions(-) 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 2adf029655..129ccf8d1e 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.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 { Provider setParent, GitReferenceUpdated gitRefUpdated, ContributorAgreementsChecker contributorAgreements, + Provider user, + PermissionBackend permissionBackend, @Assisted("projectName") Project.NameKey projectName, @Nullable @Assisted ObjectId base, @Assisted List sectionList, @@ -75,6 +79,8 @@ class ChangeProjectAccess extends ProjectAccessHandler { 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 4cd6fa0739..8c873a6ff5 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 @@ 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 { } } 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 3fa05abe5d..e92af7cbe6 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.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 extends Handler { private final AllProjectsName allProjects; private final Provider 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 extends Handler { MetaDataUpdate.User metaDataUpdateFactory, AllProjectsName allProjects, Provider setParent, + CurrentUser user, + PermissionBackend permissionBackend, Project.NameKey projectName, ObjectId base, List sectionList, @@ -85,6 +92,8 @@ public abstract class ProjectAccessHandler extends Handler { 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 extends Handler { try (MetaDataUpdate md = metaDataUpdateFactory.create(projectName)) { ProjectConfig config = ProjectConfig.read(md, base); Set 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 extends Handler { 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 extends Handler { 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 f27b9d388c..a8a120a406 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.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 { Provider setParent, Sequences seq, ContributorAgreementsChecker contributorAgreements, + Provider user, @Assisted("projectName") Project.NameKey projectName, @Nullable @Assisted ObjectId base, @Assisted List sectionList, @@ -107,6 +109,8 @@ public class ReviewProjectAccess extends ProjectAccessHandler { 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 2dd5f2ab31..ed86a922f0 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.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 visibleChanges; @@ -141,7 +139,6 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook { Map result = new HashMap<>(); List 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; } 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 6d15d6f0cd..8956f10133 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 @@ 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); } } 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 e1b6e36a65..a914a73c65 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 @@ public class ProjectWatch { static class List { protected final Set accounts = new HashSet<>(); protected final Set
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 8b5d8fb0e8..9f3dda101d 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 @@ 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; 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 a92c0b7c31..f693bf5e4e 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.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()