RefControl: Make project state checks in READ explicit

The majority of code in {Project,Ref,Change}Control is now about
permissions, but not all. Exceptions include checks for a project's
state. This is confusing, because users are presented with an exception
telling them that they lack some kind of permission while the real
reason for the failed operation is that the project's current state
doesn't permit the operation.

This is part of a series of commits to remove all project state checks
from *Control classes and make explicit checks instead.

Calls from resources in restapi/change/* need no explicit check if the
project state permits reads as this is checked in ChangesCollection.

Change-Id: I85836c102eadd33335f923c7a8f9aac97de4f654
This commit is contained in:
Patrick Hiesel
2018-01-16 13:49:59 +01:00
parent a9a057e1db
commit 7a1604395b
11 changed files with 27 additions and 8 deletions

View File

@@ -17,6 +17,7 @@ package com.google.gerrit.httpd.rpc.project;
import com.google.gerrit.common.Nullable; import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.AccessSection; import com.google.gerrit.common.data.AccessSection;
import com.google.gerrit.common.data.ProjectAccess; import com.google.gerrit.common.data.ProjectAccess;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.CreateGroupPermissionSyncer; import com.google.gerrit.server.CreateGroupPermissionSyncer;
@@ -98,7 +99,7 @@ class ChangeProjectAccess extends ProjectAccessHandler<ProjectAccess> {
protected ProjectAccess updateProjectConfig( protected ProjectAccess updateProjectConfig(
ProjectConfig config, MetaDataUpdate md, boolean parentProjectUpdate) ProjectConfig config, MetaDataUpdate md, boolean parentProjectUpdate)
throws IOException, NoSuchProjectException, ConfigInvalidException, throws IOException, NoSuchProjectException, ConfigInvalidException,
PermissionBackendException { PermissionBackendException, ResourceConflictException {
RevCommit commit = config.commit(md); RevCommit commit = config.commit(md);
gitRefUpdated.fire( gitRefUpdated.fire(

View File

@@ -30,6 +30,7 @@ import com.google.gerrit.common.data.RefConfigSection;
import com.google.gerrit.common.data.WebLinkInfoCommon; import com.google.gerrit.common.data.WebLinkInfoCommon;
import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.common.errors.NoSuchGroupException;
import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.httpd.rpc.Handler; import com.google.gerrit.httpd.rpc.Handler;
import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.AccountGroup;
import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.Project;
@@ -103,7 +104,7 @@ class ProjectAccessFactory extends Handler<ProjectAccess> {
@Override @Override
public ProjectAccess call() public ProjectAccess call()
throws NoSuchProjectException, IOException, ConfigInvalidException, throws NoSuchProjectException, IOException, ConfigInvalidException,
PermissionBackendException { PermissionBackendException, ResourceConflictException {
ProjectState projectState = checkProjectState(); ProjectState projectState = checkProjectState();
// Load the current configuration from the repository, ensuring its the most // Load the current configuration from the repository, ensuring its the most
@@ -260,13 +261,15 @@ class ProjectAccessFactory extends Handler<ProjectAccess> {
} }
private ProjectState checkProjectState() private ProjectState checkProjectState()
throws NoSuchProjectException, IOException, PermissionBackendException { throws NoSuchProjectException, IOException, PermissionBackendException,
ResourceConflictException {
ProjectState state = projectCache.checkedGet(projectName); ProjectState state = projectCache.checkedGet(projectName);
try { try {
permissionBackend.user(user).project(projectName).check(ProjectPermission.ACCESS); permissionBackend.user(user).project(projectName).check(ProjectPermission.ACCESS);
} catch (AuthException e) { } catch (AuthException e) {
throw new NoSuchProjectException(projectName); throw new NoSuchProjectException(projectName);
} }
state.checkStatePermitsRead();
return state; return state;
} }

View File

@@ -180,7 +180,8 @@ public class EventBroker implements EventDispatcher {
if (pe == null) { if (pe == null) {
return false; return false;
} }
return permissionBackend.user(user).ref(branchName).test(RefPermission.READ); return pe.statePermitsRead()
&& permissionBackend.user(user).ref(branchName).test(RefPermission.READ);
} }
protected boolean isVisibleTo(Event event, CurrentUser user) protected boolean isVisibleTo(Event event, CurrentUser user)

View File

@@ -373,13 +373,13 @@ public class VisibleRefFilter extends AbstractAdvertiseRefsHook {
private boolean canReadRef(String ref) { private boolean canReadRef(String ref) {
try { try {
perm.ref(ref).check(RefPermission.READ); perm.ref(ref).check(RefPermission.READ);
return true;
} catch (AuthException e) { } catch (AuthException e) {
return false; return false;
} catch (PermissionBackendException e) { } catch (PermissionBackendException e) {
log.error("unable to check permissions", e); log.error("unable to check permissions", e);
return false; return false;
} }
return projectState.statePermitsRead();
} }
private boolean checkProjectPermission( private boolean checkProjectPermission(

View File

@@ -490,7 +490,7 @@ class RefControl {
private boolean can(RefPermission perm) throws PermissionBackendException { private boolean can(RefPermission perm) throws PermissionBackendException {
switch (perm) { switch (perm) {
case READ: case READ:
return isVisible() && getProjectControl().getProjectState().statePermitsRead(); return isVisible();
case CREATE: case CREATE:
// TODO This isn't an accurate test. // TODO This isn't an accurate test.
return canPerform(perm.permissionName().get()); return canPerform(perm.permissionName().get());

View File

@@ -75,6 +75,7 @@ public class BranchesCollection
throws RestApiException, IOException, PermissionBackendException { throws RestApiException, IOException, PermissionBackendException {
parent.getProjectState().checkStatePermitsRead(); parent.getProjectState().checkStatePermitsRead();
Project.NameKey project = parent.getNameKey(); Project.NameKey project = parent.getNameKey();
parent.getProjectState().checkStatePermitsRead();
try (Repository repo = repoManager.openRepository(project)) { try (Repository repo = repoManager.openRepository(project)) {
Ref ref = repo.exactRef(RefNames.fullName(id.get())); Ref ref = repo.exactRef(RefNames.fullName(id.get()));
if (ref == null) { if (ref == null) {

View File

@@ -60,6 +60,7 @@ public class CheckAccess implements RestModifyView<ProjectResource, AccessCheckI
throws OrmException, PermissionBackendException, RestApiException, IOException, throws OrmException, PermissionBackendException, RestApiException, IOException,
ConfigInvalidException { ConfigInvalidException {
permissionBackend.user(rsrc.getUser()).check(GlobalPermission.ADMINISTRATE_SERVER); permissionBackend.user(rsrc.getUser()).check(GlobalPermission.ADMINISTRATE_SERVER);
rsrc.getProjectState().checkStatePermitsRead();
if (input == null) { if (input == null) {
throw new BadRequestException("input is required"); throw new BadRequestException("input is required");

View File

@@ -29,6 +29,7 @@ import com.google.gerrit.extensions.restapi.AcceptsCreate;
import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ChildCollection; import com.google.gerrit.extensions.restapi.ChildCollection;
import com.google.gerrit.extensions.restapi.IdString; import com.google.gerrit.extensions.restapi.IdString;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.extensions.restapi.RestModifyView;
@@ -143,7 +144,8 @@ public class DashboardsCollection
private DashboardResource parse( private DashboardResource parse(
ProjectState parent, ProjectState current, CurrentUser user, DashboardInfo info) ProjectState parent, ProjectState current, CurrentUser user, DashboardInfo info)
throws ResourceNotFoundException, IOException, AmbiguousObjectException, throws ResourceNotFoundException, IOException, AmbiguousObjectException,
IncorrectObjectTypeException, ConfigInvalidException, PermissionBackendException { IncorrectObjectTypeException, ConfigInvalidException, PermissionBackendException,
ResourceConflictException {
String ref = normalizeDashboardRef(info.ref); String ref = normalizeDashboardRef(info.ref);
try { try {
permissionBackend.user(user).project(parent.getNameKey()).ref(ref).check(RefPermission.READ); permissionBackend.user(user).project(parent.getNameKey()).ref(ref).check(RefPermission.READ);
@@ -155,6 +157,8 @@ public class DashboardsCollection
throw new ResourceNotFoundException(info.id); throw new ResourceNotFoundException(info.id);
} }
current.checkStatePermitsRead();
try (Repository git = gitManager.openRepository(parent.getNameKey())) { try (Repository git = gitManager.openRepository(parent.getNameKey())) {
ObjectId objId = git.resolve(ref + ":" + info.path); ObjectId objId = git.resolve(ref + ":" + info.path);
if (objId == null) { if (objId == null) {

View File

@@ -174,6 +174,13 @@ public class GetAccess implements RestReadView<ProjectResource> {
boolean canReadConfig = check(perm, RefNames.REFS_CONFIG, READ); boolean canReadConfig = check(perm, RefNames.REFS_CONFIG, READ);
boolean canWriteConfig = check(perm, ProjectPermission.WRITE_CONFIG); boolean canWriteConfig = check(perm, ProjectPermission.WRITE_CONFIG);
// Check if the project state permits read only when the user is not allowed to write the config
// (=owner). This is so that the owner can still read (and in the next step write) the project's
// config to set the project state to any state that is not HIDDEN.
if (!canWriteConfig) {
projectState.checkStatePermitsRead();
}
for (AccessSection section : config.getAccessSections()) { for (AccessSection section : config.getAccessSections()) {
String name = section.getName(); String name = section.getName();
if (AccessSection.GLOBAL_CAPABILITIES.equals(name)) { if (AccessSection.GLOBAL_CAPABILITIES.equals(name)) {

View File

@@ -54,6 +54,7 @@ public class GetHead implements RestReadView<ProjectResource> {
@Override @Override
public String apply(ProjectResource rsrc) public String apply(ProjectResource rsrc)
throws AuthException, ResourceNotFoundException, IOException, PermissionBackendException { throws AuthException, ResourceNotFoundException, IOException, PermissionBackendException {
rsrc.getProjectState().statePermitsRead();
try (Repository repo = repoManager.openRepository(rsrc.getNameKey())) { try (Repository repo = repoManager.openRepository(rsrc.getNameKey())) {
Ref head = repo.getRefDatabase().exactRef(Constants.HEAD); Ref head = repo.getRefDatabase().exactRef(Constants.HEAD);
if (head == null) { if (head == null) {

View File

@@ -109,7 +109,7 @@ public class ListDashboards implements RestReadView<ProjectResource> {
RevWalk rw = new RevWalk(git)) { RevWalk rw = new RevWalk(git)) {
List<DashboardInfo> all = new ArrayList<>(); List<DashboardInfo> all = new ArrayList<>();
for (Ref ref : git.getRefDatabase().getRefs(REFS_DASHBOARDS).values()) { for (Ref ref : git.getRefDatabase().getRefs(REFS_DASHBOARDS).values()) {
if (perm.ref(ref.getName()).test(RefPermission.READ)) { if (perm.ref(ref.getName()).test(RefPermission.READ) && state.statePermitsRead()) {
all.addAll(scanDashboards(state.getProject(), git, rw, ref, project, setDefault)); all.addAll(scanDashboards(state.getProject(), git, rw, ref, project, setDefault));
} }
} }