opendev/gerrit
Code Issues Proposed changes

Remove ProjectControl#isOwnerAnyRef()

Browse Source 
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
This commit is contained in:
Patrick Hiesel
2017-09-29 09:53:21 +02:00
committed by David Pursehouse
parent f119e29e85
commit bae538bd84
4 changed files with 17 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/project/ProjectAccessFactory.java
View File

@@ -40,6 +40,7 @@ import com.google.gerrit.server.account.GroupControl;
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 @@ class ProjectAccessFactory extends Handler<ProjectAccess> {
}
}
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 @@ class ProjectAccessFactory extends Handler<ProjectAccess> {
return false;
}
}
private boolean isAdmin() throws PermissionBackendException {
try {
permissionBackend.user(user).check(GlobalPermission.ADMINISTRATE_SERVER);
return true;
} catch (AuthException e) {
return false;
}
}
}

7
gerrit-server/src/main/java/com/google/gerrit/server/project/GetAccess.java
View File

@@ -46,6 +46,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.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 @@ public class GetAccess implements RestReadView<ProjectResource> {
}
}
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);
}

7
gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
View File

@@ -257,18 +257,13 @@ public class ProjectControl {
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() {

7
gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java
View File

@@ -102,10 +102,6 @@ public class RefControlTest {
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 @@ public class RefControlTest {
ProjectControl uDev = user(local, DEVS);
assertNotOwner(uDev);
assertOwnerAnyRef(uDev);
assertOwner("refs/heads/x/*", uDev);
assertOwner("refs/heads/x/y", uDev);
@@ -375,7 +370,6 @@ public class RefControlTest {
ProjectControl uDev = user(local, DEVS);
assertNotOwner(uDev);
assertOwnerAnyRef(uDev);
assertOwner("refs/heads/x/*", uDev);
assertOwner("refs/heads/x/y", uDev);
@@ -385,7 +379,6 @@ public class RefControlTest {
ProjectControl uFix = user(local, fixers);
assertNotOwner(uFix);
assertOwnerAnyRef(uFix);
assertOwner("refs/heads/x/y/*", uFix);
assertOwner("refs/heads/x/y/bar", uFix);