Add #absentUser() to ForProject, ForRef and ForChange
We used to think adding this method to the top level is enough
and could avoid misuses. But it turns out we still need them in
other permission backend classes so that we only need to rescope
a user when necessary, which could provide better performance.
See I67b72b59 as an example.
Change-Id: I9ef30fb38315250fa63fb8b3d27e19e19d5e3e22
diff --git a/java/com/google/gerrit/server/permissions/ChangeControl.java b/java/com/google/gerrit/server/permissions/ChangeControl.java
index b13d921..3a17965 100644
--- a/java/com/google/gerrit/server/permissions/ChangeControl.java
+++ b/java/com/google/gerrit/server/permissions/ChangeControl.java
@@ -29,6 +29,7 @@
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.PermissionBackend.ForChange;
import com.google.gerrit.server.query.change.ChangeData;
@@ -47,11 +48,16 @@
static class Factory {
private final ChangeData.Factory changeDataFactory;
private final ChangeNotes.Factory notesFactory;
+ private final IdentifiedUser.GenericFactory identifiedUserFactory;
@Inject
- Factory(ChangeData.Factory changeDataFactory, ChangeNotes.Factory notesFactory) {
+ Factory(
+ ChangeData.Factory changeDataFactory,
+ ChangeNotes.Factory notesFactory,
+ IdentifiedUser.GenericFactory identifiedUserFactory) {
this.changeDataFactory = changeDataFactory;
this.notesFactory = notesFactory;
+ this.identifiedUserFactory = identifiedUserFactory;
}
ChangeControl create(
@@ -61,17 +67,22 @@
}
ChangeControl create(RefControl refControl, ChangeNotes notes) {
- return new ChangeControl(changeDataFactory, refControl, notes);
+ return new ChangeControl(changeDataFactory, identifiedUserFactory, refControl, notes);
}
}
private final ChangeData.Factory changeDataFactory;
+ private final IdentifiedUser.GenericFactory identifiedUserFactory;
private final RefControl refControl;
private final ChangeNotes notes;
private ChangeControl(
- ChangeData.Factory changeDataFactory, RefControl refControl, ChangeNotes notes) {
+ ChangeData.Factory changeDataFactory,
+ IdentifiedUser.GenericFactory identifiedUserFactory,
+ RefControl refControl,
+ ChangeNotes notes) {
this.changeDataFactory = changeDataFactory;
+ this.identifiedUserFactory = identifiedUserFactory;
this.refControl = refControl;
this.notes = notes;
}
@@ -84,7 +95,8 @@
if (getUser().equals(who)) {
return this;
}
- return new ChangeControl(changeDataFactory, refControl.forUser(who), notes);
+ return new ChangeControl(
+ changeDataFactory, identifiedUserFactory, refControl.forUser(who), notes);
}
private CurrentUser getUser() {
@@ -261,6 +273,11 @@
}
@Override
+ public ForChange absentUser(Account.Id id) {
+ return user(identifiedUserFactory.create(id));
+ }
+
+ @Override
public String resourcePath() {
if (resourcePath == null) {
resourcePath =
diff --git a/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java b/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java
index 02eed30..490b45e 100644
--- a/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java
+++ b/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java
@@ -79,8 +79,8 @@
}
@Override
- public WithUser absentUser(Account.Id user) {
- IdentifiedUser identifiedUser = identifiedUserFactory.create(checkNotNull(user, "user"));
+ public WithUser absentUser(Account.Id id) {
+ IdentifiedUser identifiedUser = identifiedUserFactory.create(checkNotNull(id, "user"));
return new WithUserImpl(identifiedUser);
}
diff --git a/java/com/google/gerrit/server/permissions/FailedPermissionBackend.java b/java/com/google/gerrit/server/permissions/FailedPermissionBackend.java
index 35b6e0d..431bfd9 100644
--- a/java/com/google/gerrit/server/permissions/FailedPermissionBackend.java
+++ b/java/com/google/gerrit/server/permissions/FailedPermissionBackend.java
@@ -15,6 +15,7 @@
package com.google.gerrit.server.permissions;
import com.google.gerrit.extensions.api.access.GlobalOrPluginPermission;
+import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
@@ -129,6 +130,11 @@
}
@Override
+ public ForProject absentUser(Account.Id id) {
+ return this;
+ }
+
+ @Override
public String resourcePath() {
throw new UnsupportedOperationException(
"FailedPermissionBackend is not scoped to a resource");
@@ -182,6 +188,11 @@
}
@Override
+ public ForRef absentUser(Account.Id id) {
+ return this;
+ }
+
+ @Override
public String resourcePath() {
throw new UnsupportedOperationException(
"FailedPermissionBackend is not scoped to a resource");
@@ -234,6 +245,11 @@
}
@Override
+ public ForChange absentUser(Account.Id id) {
+ return this;
+ }
+
+ @Override
public String resourcePath() {
throw new UnsupportedOperationException(
"FailedPermissionBackend is not scoped to a resource");
diff --git a/java/com/google/gerrit/server/permissions/PermissionBackend.java b/java/com/google/gerrit/server/permissions/PermissionBackend.java
index 4cbd77e..8cdb61d 100644
--- a/java/com/google/gerrit/server/permissions/PermissionBackend.java
+++ b/java/com/google/gerrit/server/permissions/PermissionBackend.java
@@ -112,7 +112,7 @@
*
* <p>Usage should be very limited as this can expose a group-oracle.
*/
- public abstract WithUser absentUser(Account.Id user);
+ public abstract WithUser absentUser(Account.Id id);
/**
* Check whether this {@code PermissionBackend} respects the same global capabilities as the
@@ -305,6 +305,9 @@
/** Returns a new instance rescoped to same project, but different {@code user}. */
public abstract ForProject user(CurrentUser user);
+ /** @see PermissionBackend#absentUser(Account.Id) */
+ public abstract ForProject absentUser(Account.Id id);
+
/** Returns an instance scoped for {@code ref} in this project. */
public abstract ForRef ref(String ref);
@@ -413,6 +416,9 @@
/** Returns a new instance rescoped to same reference, but different {@code user}. */
public abstract ForRef user(CurrentUser user);
+ /** @see PermissionBackend#absentUser(Account.Id) */
+ public abstract ForRef absentUser(Account.Id id);
+
/** Returns an instance scoped to change. */
public abstract ForChange change(ChangeData cd);
@@ -471,6 +477,9 @@
/** Returns a new instance rescoped to same change, but different {@code user}. */
public abstract ForChange user(CurrentUser user);
+ /** @see PermissionBackend#absentUser(Account.Id) */
+ public abstract ForChange absentUser(Account.Id id);
+
/** Verify scoped user can {@code perm}, throwing if denied. */
public abstract void check(ChangePermissionOrLabel perm)
throws AuthException, PermissionBackendException;
diff --git a/java/com/google/gerrit/server/permissions/ProjectControl.java b/java/com/google/gerrit/server/permissions/ProjectControl.java
index dbd60ea..2d2a64d 100644
--- a/java/com/google/gerrit/server/permissions/ProjectControl.java
+++ b/java/com/google/gerrit/server/permissions/ProjectControl.java
@@ -20,6 +20,7 @@
import com.google.gerrit.common.data.Permission;
import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
@@ -27,6 +28,7 @@
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.IdentifiedUser;
import com.google.gerrit.server.account.GroupMembership;
import com.google.gerrit.server.config.GitReceivePackGroups;
import com.google.gerrit.server.config.GitUploadPackGroups;
@@ -67,6 +69,7 @@
private final ChangeControl.Factory changeControlFactory;
private final PermissionCollection.Factory permissionFilter;
private final DefaultRefFilter.Factory refFilterFactory;
+ private final IdentifiedUser.GenericFactory identifiedUserFactory;
private List<SectionMatcher> allSections;
private Map<String, RefControl> refControls;
@@ -80,6 +83,7 @@
ChangeControl.Factory changeControlFactory,
PermissionBackend permissionBackend,
DefaultRefFilter.Factory refFilterFactory,
+ IdentifiedUser.GenericFactory identifiedUserFactory,
@Assisted CurrentUser who,
@Assisted ProjectState ps) {
this.changeControlFactory = changeControlFactory;
@@ -88,6 +92,7 @@
this.permissionFilter = permissionFilter;
this.permissionBackend = permissionBackend;
this.refFilterFactory = refFilterFactory;
+ this.identifiedUserFactory = identifiedUserFactory;
user = who;
state = ps;
}
@@ -101,6 +106,7 @@
changeControlFactory,
permissionBackend,
refFilterFactory,
+ identifiedUserFactory,
who,
state);
// Not per-user, and reusing saves lookup time.
@@ -132,7 +138,7 @@
RefControl ctl = refControls.get(refName);
if (ctl == null) {
PermissionCollection relevant = permissionFilter.filter(access(), refName, user);
- ctl = new RefControl(this, refName, relevant);
+ ctl = new RefControl(identifiedUserFactory, this, refName, relevant);
refControls.put(refName, ctl);
}
return ctl;
@@ -327,6 +333,11 @@
}
@Override
+ public ForProject absentUser(Account.Id id) {
+ return user(identifiedUserFactory.create(id));
+ }
+
+ @Override
public String resourcePath() {
if (resourcePath == null) {
resourcePath = "/projects/" + getProjectState().getName();
diff --git a/java/com/google/gerrit/server/permissions/RefControl.java b/java/com/google/gerrit/server/permissions/RefControl.java
index 28781ea..cd1f84a 100644
--- a/java/com/google/gerrit/server/permissions/RefControl.java
+++ b/java/com/google/gerrit/server/permissions/RefControl.java
@@ -21,10 +21,12 @@
import com.google.gerrit.common.data.PermissionRule;
import com.google.gerrit.common.data.PermissionRule.Action;
import com.google.gerrit.extensions.restapi.AuthException;
+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.reviewdb.client.RefNames;
import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.permissions.PermissionBackend.ForChange;
import com.google.gerrit.server.permissions.PermissionBackend.ForRef;
@@ -39,6 +41,7 @@
/** Manages access control for Git references (aka branches, tags). */
class RefControl {
+ private final IdentifiedUser.GenericFactory identifiedUserFactory;
private final ProjectControl projectControl;
private final String refName;
@@ -52,7 +55,12 @@
private Boolean canForgeCommitter;
private Boolean isVisible;
- RefControl(ProjectControl projectControl, String ref, PermissionCollection relevant) {
+ RefControl(
+ IdentifiedUser.GenericFactory identifiedUserFactory,
+ ProjectControl projectControl,
+ String ref,
+ PermissionCollection relevant) {
+ this.identifiedUserFactory = identifiedUserFactory;
this.projectControl = projectControl;
this.refName = ref;
this.relevant = relevant;
@@ -71,7 +79,7 @@
if (relevant.isUserSpecific()) {
return newCtl.controlForRef(refName);
}
- return new RefControl(newCtl, refName, relevant);
+ return new RefControl(identifiedUserFactory, newCtl, refName, relevant);
}
/** Is this user a ref owner? */
@@ -404,6 +412,11 @@
}
@Override
+ public ForRef absentUser(Account.Id id) {
+ return user(identifiedUserFactory.create(id));
+ }
+
+ @Override
public String resourcePath() {
if (resourcePath == null) {
resourcePath =
diff --git a/javatests/com/google/gerrit/server/config/ListCapabilitiesTest.java b/javatests/com/google/gerrit/server/config/ListCapabilitiesTest.java
index 935dfc6..fd9c925 100644
--- a/javatests/com/google/gerrit/server/config/ListCapabilitiesTest.java
+++ b/javatests/com/google/gerrit/server/config/ListCapabilitiesTest.java
@@ -87,7 +87,7 @@
}
@Override
- public WithUser absentUser(Id user) {
+ public WithUser absentUser(Id id) {
throw new UnsupportedOperationException();
}
diff --git a/javatests/com/google/gerrit/server/extensions/webui/UiActionsTest.java b/javatests/com/google/gerrit/server/extensions/webui/UiActionsTest.java
index d242962..834f658 100644
--- a/javatests/com/google/gerrit/server/extensions/webui/UiActionsTest.java
+++ b/javatests/com/google/gerrit/server/extensions/webui/UiActionsTest.java
@@ -78,6 +78,11 @@
}
@Override
+ public ForProject absentUser(Account.Id id) {
+ throw new UnsupportedOperationException("not implemented");
+ }
+
+ @Override
public ForRef ref(String ref) {
throw new UnsupportedOperationException("not implemented");
}
diff --git a/javatests/com/google/gerrit/server/permissions/RefControlTest.java b/javatests/com/google/gerrit/server/permissions/RefControlTest.java
index c30803a..7890de8 100644
--- a/javatests/com/google/gerrit/server/permissions/RefControlTest.java
+++ b/javatests/com/google/gerrit/server/permissions/RefControlTest.java
@@ -47,6 +47,7 @@
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.CapabilityCollection;
import com.google.gerrit.server.account.GroupMembership;
import com.google.gerrit.server.account.ListGroupMembership;
@@ -202,6 +203,7 @@
@Inject private InMemoryDatabase schemaFactory;
@Inject private ThreadLocalRequestContext requestContext;
@Inject private DefaultRefFilter.Factory refFilterFactory;
+ @Inject private IdentifiedUser.GenericFactory identifiedUserFactory;
@Before
public void setUp() throws Exception {
@@ -986,6 +988,7 @@
changeControlFactory,
permissionBackend,
refFilterFactory,
+ identifiedUserFactory,
new MockUser(name, memberOf),
newProjectState(local));
}
