Remove ProjectControl#isOwnerAnyRef()
In most cases, this method was used as a fallback in case no ref owner
was defined. Comments in the code suggested that it was always intended
to just fall back to ADMINISTRATE_SERVER.
Looking at all use cases individually, it seems safe to just fall back
to ADMINISTRATE_SERVER directly.
Change-Id: I3c7726c3720492f36f737edf7c0e4e7e64c5e6f1
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 070eefd..4cd6fa0 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
@@ -40,6 +40,7 @@
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.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.ProjectPermission;
@@ -193,10 +194,9 @@
}
}
- if (ownerOf.isEmpty() && pc.isOwnerAnyRef()) {
+ if (ownerOf.isEmpty() && isAdmin()) {
// Special case: If the section list is empty, this project has no current
- // access control information. Rely on what ProjectControl determines
- // is ownership, which probably means falling back to site administrators.
+ // access control information. Fall back to site administrators.
ownerOf.add(AccessSection.ALL);
}
@@ -271,4 +271,13 @@
return false;
}
}
+
+ private boolean isAdmin() throws PermissionBackendException {
+ try {
+ permissionBackend.user(user).check(GlobalPermission.ADMINISTRATE_SERVER);
+ return true;
+ } catch (AuthException e) {
+ return false;
+ }
+ }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/GetAccess.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/GetAccess.java
index 00190bb..07c52f9 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/GetAccess.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/GetAccess.java
@@ -46,6 +46,7 @@
import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.group.GroupJson;
+import com.google.gerrit.server.permissions.GlobalPermission;
import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.RefPermission;
@@ -220,10 +221,10 @@
}
}
- if (info.ownerOf.isEmpty() && pc.isOwnerAnyRef()) {
+ if (info.ownerOf.isEmpty()
+ && permissionBackend.user(user).test(GlobalPermission.ADMINISTRATE_SERVER)) {
// Special case: If the section list is empty, this project has no current
- // access control information. Rely on what ProjectControl determines
- // is ownership, which probably means falling back to site administrators.
+ // access control information. Fall back to site administrators.
info.ownerOf.add(AccessSection.ALL);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
index c5425e8..a443cdb 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
@@ -257,18 +257,13 @@
return false;
}
- /** Does this user have ownership on at least one reference name? */
- public boolean isOwnerAnyRef() {
- return canPerformOnAnyRef(Permission.OWNER) || isAdmin();
- }
-
/** Returns whether the project is hidden. */
private boolean isHidden() {
return getProject().getState().equals(com.google.gerrit.extensions.client.ProjectState.HIDDEN);
}
private boolean canAddRefs() {
- return (canPerformOnAnyRef(Permission.CREATE) || isOwnerAnyRef());
+ return (canPerformOnAnyRef(Permission.CREATE) || isAdmin());
}
private boolean canCreateChanges() {
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java
index 6bff1839..b7a3788 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java
@@ -102,10 +102,6 @@
assertThat(u.isOwner()).named("not owner").isFalse();
}
- private void assertOwnerAnyRef(ProjectControl u) {
- assertThat(u.isOwnerAnyRef()).named("owns ref").isTrue();
- }
-
private void assertNotOwner(String ref, ProjectControl u) {
assertThat(u.controlForRef(ref).isOwner()).named("NOT OWN " + ref).isFalse();
}
@@ -356,7 +352,6 @@
ProjectControl uDev = user(local, DEVS);
assertNotOwner(uDev);
- assertOwnerAnyRef(uDev);
assertOwner("refs/heads/x/*", uDev);
assertOwner("refs/heads/x/y", uDev);
@@ -375,7 +370,6 @@
ProjectControl uDev = user(local, DEVS);
assertNotOwner(uDev);
- assertOwnerAnyRef(uDev);
assertOwner("refs/heads/x/*", uDev);
assertOwner("refs/heads/x/y", uDev);
@@ -385,7 +379,6 @@
ProjectControl uFix = user(local, fixers);
assertNotOwner(uFix);
- assertOwnerAnyRef(uFix);
assertOwner("refs/heads/x/y/*", uFix);
assertOwner("refs/heads/x/y/bar", uFix);
