Allow to override permission-related checks.
This change adjusts visibility of some methods and makes other minor
changes to allow overriding permissions checks. This is required for
internal google implementation.
Google-Bug-Id: b/330832714
Release-Notes: skip
Change-Id: Idac46434228204a6825f0bd6b1e4b4535028ccca
diff --git a/java/com/google/gerrit/server/permissions/ChangeControl.java b/java/com/google/gerrit/server/permissions/ChangeControl.java
index 664ffa2..5d79d09 100644
--- a/java/com/google/gerrit/server/permissions/ChangeControl.java
+++ b/java/com/google/gerrit/server/permissions/ChangeControl.java
@@ -31,19 +31,26 @@
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.permissions.PermissionBackend.ForChange;
import com.google.gerrit.server.query.change.ChangeData;
+import com.google.inject.assistedinject.Assisted;
import java.util.Collection;
import java.util.EnumSet;
import java.util.Map;
import java.util.Set;
+import javax.inject.Inject;
/** Access control management for a user accessing a single change. */
-class ChangeControl {
+public class ChangeControl {
+ public interface Factory {
+ ChangeControl create(RefControl refControl, ChangeData changeData);
+ }
+
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final RefControl refControl;
private final ChangeData changeData;
- ChangeControl(RefControl refControl, ChangeData changeData) {
+ @Inject
+ protected ChangeControl(@Assisted RefControl refControl, @Assisted ChangeData changeData) {
this.refControl = refControl;
this.changeData = changeData;
}
diff --git a/java/com/google/gerrit/server/permissions/DefaultPermissionBackendModule.java b/java/com/google/gerrit/server/permissions/DefaultPermissionBackendModule.java
index 3f84dff..b5ec4c9 100644
--- a/java/com/google/gerrit/server/permissions/DefaultPermissionBackendModule.java
+++ b/java/com/google/gerrit/server/permissions/DefaultPermissionBackendModule.java
@@ -14,23 +14,32 @@
package com.google.gerrit.server.permissions;
-import com.google.gerrit.extensions.config.FactoryModule;
-import com.google.inject.AbstractModule;
+import com.google.inject.PrivateModule;
+import com.google.inject.assistedinject.FactoryModuleBuilder;
/** Binds the default {@link PermissionBackend}. */
-public class DefaultPermissionBackendModule extends AbstractModule {
+public class DefaultPermissionBackendModule extends PrivateModule {
+
@Override
protected void configure() {
- install(new LegacyControlsModule());
+ // TODO(hiesel) Hide ProjectControl, RefControl, ChangeControl related bindings.
+ install(new FactoryModuleBuilder().build(DefaultRefFilter.Factory.class));
+ installRefControlFactory();
+ installChangeControlFactory();
+ installProjectControlFactory();
+ // Expose only ProjectControl.Factory, so other modules can't use RefControl and ChangeControl.
+ expose(ProjectControl.Factory.class);
}
- /** Binds legacy ProjectControl, RefControl, ChangeControl. */
- public static class LegacyControlsModule extends FactoryModule {
- @Override
- protected void configure() {
- // TODO(hiesel) Hide ProjectControl, RefControl, ChangeControl related bindings.
- factory(ProjectControl.Factory.class);
- factory(DefaultRefFilter.Factory.class);
- }
+ protected void installProjectControlFactory() {
+ install(new FactoryModuleBuilder().build(ProjectControl.Factory.class));
+ }
+
+ protected void installChangeControlFactory() {
+ install(new FactoryModuleBuilder().build(ChangeControl.Factory.class));
+ }
+
+ protected void installRefControlFactory() {
+ install(new FactoryModuleBuilder().build(RefControl.Factory.class));
}
}
diff --git a/java/com/google/gerrit/server/permissions/DefaultRefFilter.java b/java/com/google/gerrit/server/permissions/DefaultRefFilter.java
index f179045..3d8ae4f 100644
--- a/java/com/google/gerrit/server/permissions/DefaultRefFilter.java
+++ b/java/com/google/gerrit/server/permissions/DefaultRefFilter.java
@@ -57,10 +57,10 @@
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
-class DefaultRefFilter {
+public class DefaultRefFilter {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
- interface Factory {
+ public interface Factory {
DefaultRefFilter create(ProjectControl projectControl);
}
diff --git a/java/com/google/gerrit/server/permissions/ProjectControl.java b/java/com/google/gerrit/server/permissions/ProjectControl.java
index 09976a5..9a6db5d 100644
--- a/java/com/google/gerrit/server/permissions/ProjectControl.java
+++ b/java/com/google/gerrit/server/permissions/ProjectControl.java
@@ -42,7 +42,6 @@
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.GitReceivePackGroups;
import com.google.gerrit.server.config.GitUploadPackGroups;
-import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.logging.Metadata;
import com.google.gerrit.server.logging.TraceContext;
@@ -69,22 +68,21 @@
import org.eclipse.jgit.lib.Repository;
/** Access control management for a user accessing a project's data. */
-class ProjectControl {
- interface Factory {
+public class ProjectControl {
+ public interface Factory {
ProjectControl create(CurrentUser who, ProjectState ps);
}
private final Set<AccountGroup.UUID> uploadGroups;
private final Set<AccountGroup.UUID> receiveGroups;
private final PermissionBackend permissionBackend;
- private final RefVisibilityControl refVisibilityControl;
- private final GitRepositoryManager repositoryManager;
private final CurrentUser user;
private final ProjectState state;
private final PermissionCollection.Factory permissionFilter;
private final DefaultRefFilter.Factory refFilterFactory;
- private final ChangeData.Factory changeDataFactory;
private final AllUsersName allUsersName;
+ private final RefControl.Factory refControlFactory;
+ private final ChangeControl.Factory changeControlFactory;
private List<SectionMatcher> allSections;
private Map<String, RefControl> refControls;
@@ -92,29 +90,27 @@
private Config cfg;
@Inject
- ProjectControl(
+ protected ProjectControl(
@GitUploadPackGroups Set<AccountGroup.UUID> uploadGroups,
@GitReceivePackGroups Set<AccountGroup.UUID> receiveGroups,
PermissionCollection.Factory permissionFilter,
PermissionBackend permissionBackend,
- RefVisibilityControl refVisibilityControl,
- GitRepositoryManager repositoryManager,
DefaultRefFilter.Factory refFilterFactory,
- ChangeData.Factory changeDataFactory,
AllUsersName allUsersName,
@GerritServerConfig Config cfg,
+ RefControl.Factory refControlFactory,
+ ChangeControl.Factory changeControlFactory,
@Assisted CurrentUser who,
@Assisted ProjectState ps) {
this.uploadGroups = uploadGroups;
this.receiveGroups = receiveGroups;
this.permissionFilter = permissionFilter;
this.permissionBackend = permissionBackend;
- this.refVisibilityControl = refVisibilityControl;
- this.repositoryManager = repositoryManager;
this.refFilterFactory = refFilterFactory;
- this.changeDataFactory = changeDataFactory;
this.allUsersName = allUsersName;
this.cfg = cfg;
+ this.refControlFactory = refControlFactory;
+ this.changeControlFactory = changeControlFactory;
user = who;
state = ps;
}
@@ -124,7 +120,7 @@
}
ChangeControl controlFor(ChangeData cd) {
- return new ChangeControl(controlForRef(cd.branchOrThrow()), cd);
+ return changeControlFactory.create(controlForRef(cd.branchOrThrow()), cd);
}
RefControl controlForRef(BranchNameKey ref) {
@@ -138,19 +134,17 @@
RefControl ctl = refControls.get(refName);
if (ctl == null) {
PermissionCollection relevant = permissionFilter.filter(access(), refName, user);
- ctl =
- new RefControl(
- changeDataFactory, refVisibilityControl, this, repositoryManager, refName, relevant);
+ ctl = refControlFactory.create(this, refName, relevant);
refControls.put(refName, ctl);
}
return ctl;
}
- CurrentUser getUser() {
+ protected CurrentUser getUser() {
return user;
}
- ProjectState getProjectState() {
+ protected ProjectState getProjectState() {
return state;
}
@@ -361,6 +355,13 @@
}
}
+ @UsedAt(UsedAt.Project.GOOGLE)
+ protected boolean canUpdateConfigWithoutCreatingChange() {
+ // In google, the implementation use more complicated logic - this is why it is placed inside
+ // a ProjectControl.
+ return !cfg.getBoolean("gerrit", "requireChangeForConfigUpdate", false);
+ }
+
private class ForProjectImpl extends ForProject {
private String resourcePath;
@@ -488,12 +489,5 @@
}
throw new PermissionBackendException(perm + " unsupported");
}
-
- @UsedAt(UsedAt.Project.GOOGLE)
- protected boolean canUpdateConfigWithoutCreatingChange() {
- // In google, the implementation use more complicated logic - this is why it is placed inside
- // a ProjectControl.
- return !cfg.getBoolean("gerrit", "requireChangeForConfigUpdate", false);
- }
}
}
diff --git a/java/com/google/gerrit/server/permissions/RefControl.java b/java/com/google/gerrit/server/permissions/RefControl.java
index 9920631..ce8548d 100644
--- a/java/com/google/gerrit/server/permissions/RefControl.java
+++ b/java/com/google/gerrit/server/permissions/RefControl.java
@@ -37,6 +37,8 @@
import com.google.gerrit.server.permissions.PermissionBackend.ForRef;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.util.MagicBranch;
+import com.google.inject.Inject;
+import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
import java.util.Collection;
import java.util.EnumSet;
@@ -47,8 +49,17 @@
import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.Repository;
-/** Manages access control for Git references (aka branches, tags). */
-class RefControl {
+/**
+ * Manages access control for Git references (aka branches, tags).
+ *
+ * <p>Do not use this class directly - instead use {@link ProjectControl} class. This class is
+ * public only because it is extended in google-owned implementation.
+ */
+public class RefControl {
+ public interface Factory {
+ RefControl create(ProjectControl projectControl, String ref, PermissionCollection relevant);
+ }
+
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final ChangeData.Factory changeDataFactory;
@@ -67,22 +78,23 @@
private Boolean canForgeCommitter;
private Boolean hasReadPermissionOnRef;
- RefControl(
+ @Inject
+ protected RefControl(
ChangeData.Factory changeDataFactory,
RefVisibilityControl refVisibilityControl,
- ProjectControl projectControl,
GitRepositoryManager repositoryManager,
- String ref,
- PermissionCollection relevant) {
+ @Assisted ProjectControl projectControl,
+ @Assisted String ref,
+ @Assisted PermissionCollection relevant) {
this.changeDataFactory = changeDataFactory;
this.refVisibilityControl = refVisibilityControl;
- this.projectControl = projectControl;
this.repositoryManager = repositoryManager;
+ this.projectControl = projectControl;
this.refName = ref;
this.relevant = relevant;
}
- ProjectControl getProjectControl() {
+ protected ProjectControl getProjectControl() {
return projectControl;
}
@@ -90,6 +102,10 @@
return projectControl.getUser();
}
+ protected String getRefName() {
+ return refName;
+ }
+
/** Is this user a ref owner? */
boolean isOwner() {
if (owner == null) {
@@ -139,7 +155,7 @@
}
/** Returns true if this user can submit patch sets to this ref */
- boolean canSubmit(boolean isChangeOwner) {
+ protected boolean canSubmit(boolean isChangeOwner) {
if (RefNames.REFS_CONFIG.equals(refName)) {
// Always allow project owners to submit configuration changes.
// Submitting configuration changes modifies the access control
@@ -297,7 +313,7 @@
return pr.getAction() == Action.BLOCK && (!pr.getForce() || withForce);
}
- private PermissionRange toRange(String permissionName, boolean isChangeOwner) {
+ protected PermissionRange toRange(String permissionName, boolean isChangeOwner) {
int blockAllowMin = Integer.MIN_VALUE, blockAllowMax = Integer.MAX_VALUE;
projectLoop:
@@ -609,98 +625,98 @@
public BooleanCondition testCond(RefPermission perm) {
return new PermissionBackendCondition.ForRef(this, perm, getUser());
}
+ }
- private boolean can(RefPermission perm) throws PermissionBackendException {
- switch (perm) {
- case READ:
- /* Internal users such as plugin users should be able to read all refs. */
- if (getUser().isInternalUser()) {
- return true;
- }
- if (refName.startsWith(Constants.R_TAGS)) {
- return isTagVisible();
- }
- return refVisibilityControl.isVisible(projectControl, refName);
- case CREATE:
- // TODO This isn't an accurate test.
- return canPerform(refPermissionName(perm));
- case DELETE:
- return canDelete();
- case UPDATE:
- return canUpdate();
- case FORCE_UPDATE:
- return canForceUpdate();
- case SET_HEAD:
- return projectControl.isOwner();
+ protected boolean can(RefPermission perm) throws PermissionBackendException {
+ switch (perm) {
+ case READ:
+ /* Internal users such as plugin users should be able to read all refs. */
+ if (getUser().isInternalUser()) {
+ return true;
+ }
+ if (refName.startsWith(Constants.R_TAGS)) {
+ return isTagVisible();
+ }
+ return refVisibilityControl.isVisible(projectControl, refName);
+ case CREATE:
+ // TODO This isn't an accurate test.
+ return canPerform(refPermissionName(perm));
+ case DELETE:
+ return canDelete();
+ case UPDATE:
+ return canUpdate();
+ case FORCE_UPDATE:
+ return canForceUpdate();
+ case SET_HEAD:
+ return projectControl.isOwner();
- case FORGE_AUTHOR:
- return canForgeAuthor();
- case FORGE_COMMITTER:
- return canForgeCommitter();
- case FORGE_SERVER:
- return canForgeGerritServerIdentity();
- case MERGE:
- return canUploadMerges();
+ case FORGE_AUTHOR:
+ return canForgeAuthor();
+ case FORGE_COMMITTER:
+ return canForgeCommitter();
+ case FORGE_SERVER:
+ return canForgeGerritServerIdentity();
+ case MERGE:
+ return canUploadMerges();
- case CREATE_CHANGE:
- return canUpload();
+ case CREATE_CHANGE:
+ return canUpload();
- case CREATE_TAG:
- case CREATE_SIGNED_TAG:
- return canPerform(refPermissionName(perm));
+ case CREATE_TAG:
+ case CREATE_SIGNED_TAG:
+ return canPerform(refPermissionName(perm));
- case UPDATE_BY_SUBMIT:
- return projectControl.controlForRef(MagicBranch.NEW_CHANGE + refName).canSubmit(true);
+ case UPDATE_BY_SUBMIT:
+ return projectControl.controlForRef(MagicBranch.NEW_CHANGE + refName).canSubmit(true);
- case READ_PRIVATE_CHANGES:
- return canPerform(Permission.VIEW_PRIVATE_CHANGES);
+ case READ_PRIVATE_CHANGES:
+ return canPerform(Permission.VIEW_PRIVATE_CHANGES);
- case READ_CONFIG:
- return projectControl
- .controlForRef(RefNames.REFS_CONFIG)
- .canPerform(RefPermission.READ.name());
- case WRITE_CONFIG:
- return isOwner();
+ case READ_CONFIG:
+ return projectControl
+ .controlForRef(RefNames.REFS_CONFIG)
+ .canPerform(RefPermission.READ.name());
+ case WRITE_CONFIG:
+ return isOwner();
- case SKIP_VALIDATION:
- return canForgeAuthor()
- && canForgeCommitter()
- && canForgeGerritServerIdentity()
- && canUploadMerges();
- }
- throw new PermissionBackendException(perm + " unsupported");
+ case SKIP_VALIDATION:
+ return canForgeAuthor()
+ && canForgeCommitter()
+ && canForgeGerritServerIdentity()
+ && canUploadMerges();
+ }
+ throw new PermissionBackendException(perm + " unsupported");
+ }
+
+ private boolean isTagVisible() throws PermissionBackendException {
+ if (projectControl.asForProject().test(ProjectPermission.READ)) {
+ // The user has READ on refs/* with no effective block permission. This is the broadest
+ // permission one can assign. There is no way to grant access to (specific) tags in Gerrit,
+ // so we have to assume that these users can see all tags because there could be tags that
+ // aren't reachable by any visible ref while the user can see all non-Gerrit refs. This
+ // matches Gerrit's historic behavior.
+ // This makes it so that these users could see commits that they can't see otherwise
+ // (e.g. a private change ref) if a tag was attached to it. Tags are meant to be used on
+ // the regular Git tree that users interact with, not on any of the Gerrit trees, so this
+ // is a negligible risk.
+ return true;
}
- private boolean isTagVisible() throws PermissionBackendException {
- if (projectControl.asForProject().test(ProjectPermission.READ)) {
- // The user has READ on refs/* with no effective block permission. This is the broadest
- // permission one can assign. There is no way to grant access to (specific) tags in Gerrit,
- // so we have to assume that these users can see all tags because there could be tags that
- // aren't reachable by any visible ref while the user can see all non-Gerrit refs. This
- // matches Gerrit's historic behavior.
- // This makes it so that these users could see commits that they can't see otherwise
- // (e.g. a private change ref) if a tag was attached to it. Tags are meant to be used on
- // the regular Git tree that users interact with, not on any of the Gerrit trees, so this
- // is a negligible risk.
- return true;
+ try (Repository repo =
+ repositoryManager.openRepository(projectControl.getProject().getNameKey())) {
+ // Tag visibility requires going through RefFilter because it entails loading all taggable
+ // refs and filtering them all by visibility.
+ Ref resolvedRef = repo.getRefDatabase().exactRef(refName);
+ if (resolvedRef == null) {
+ return false;
}
-
- try (Repository repo =
- repositoryManager.openRepository(projectControl.getProject().getNameKey())) {
- // Tag visibility requires going through RefFilter because it entails loading all taggable
- // refs and filtering them all by visibility.
- Ref resolvedRef = repo.getRefDatabase().exactRef(refName);
- if (resolvedRef == null) {
- return false;
- }
- return projectControl.asForProject()
- .filter(
- ImmutableList.of(resolvedRef), repo, PermissionBackend.RefFilterOptions.defaults())
- .stream()
- .anyMatch(r -> refName.equals(r.getName()));
- } catch (IOException e) {
- throw new PermissionBackendException(e);
- }
+ return projectControl.asForProject()
+ .filter(
+ ImmutableList.of(resolvedRef), repo, PermissionBackend.RefFilterOptions.defaults())
+ .stream()
+ .anyMatch(r -> refName.equals(r.getName()));
+ } catch (IOException e) {
+ throw new PermissionBackendException(e);
}
}
diff --git a/java/com/google/gerrit/server/permissions/RefVisibilityControl.java b/java/com/google/gerrit/server/permissions/RefVisibilityControl.java
index c2d1139..bae1fef 100644
--- a/java/com/google/gerrit/server/permissions/RefVisibilityControl.java
+++ b/java/com/google/gerrit/server/permissions/RefVisibilityControl.java
@@ -38,7 +38,7 @@
* authoritatively tell if a ref is accessible by a user.
*/
@Singleton
-class RefVisibilityControl {
+public class RefVisibilityControl {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
private final PermissionBackend permissionBackend;
