opendev/gerrit
Code Issues Proposed changes

Remove the non-permission check #isHidden from ProjectControl

Browse Source 
PermissionBackend is supposed to only contain permission related checks.
Apparently, this #isHidden is a project state check rather than a
permission check. Thus it should be removed from the ProjectControl,
which is part of the DefaultPermissionBackend.

Before removing this, "ACCESS" permission checks for hidden projects will
only succeed for the project owners. After removing, they may also succeed
for other users, e.g. internal user.

The existing checks for "ACCESS" could be divided into two categories
base on whether it's helpful for users to change the configuration of
the project state or not.

For the helpful case, this commit preserves the current behavior of
the "ACCESS" check on hidden projects by checking the "READ_CONFIG"
permission which will only succeed for the project owners. For the
other case, this commit rejects directly even for project owners if
the project is hidden.

Change-Id: I20743e6380129eea7cb942d8d9ccad314e29d187
This commit is contained in:
Changcheng Xiao 2018-03-26 16:10:18 +02:00
parent f47dd2d416
commit da44fe4ba2
15 changed files with 116 additions and 43 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
java/com/google/gerrit/httpd/GitOverHttpServlet.java
View File

@ -184,7 +184,7 @@ public class GitOverHttpServlet extends GitServlet {
try {
Project.NameKey nameKey = new Project.NameKey(projectName);
ProjectState state = projectCache.checkedGet(nameKey);
if (state == null) {
if (state == null || !state.statePermitsRead()) {
throw new RepositoryNotFoundException(nameKey.get());
}
req.setAttribute(ATT_STATE, state);

8
java/com/google/gerrit/httpd/rpc/project/ProjectAccessFactory.java
View File

@ -259,8 +259,14 @@ class ProjectAccessFactory extends Handler<ProjectAccess> {
throws NoSuchProjectException, IOException, PermissionBackendException,
ResourceConflictException {
ProjectState state = projectCache.checkedGet(projectName);
// Hidden projects(permitsRead = false) should only be accessible by the project owners.
// READ_CONFIG is checked here because it's only allowed to project owners(ACCESS may also
// be allowed for other users). Allowing project owners to access here will help them to view
// and update the config of hidden projects easily.
ProjectPermission permissionToCheck =
state.statePermitsRead() ? ProjectPermission.ACCESS : ProjectPermission.READ_CONFIG;
try {
permissionBackend.currentUser().project(projectName).check(ProjectPermission.ACCESS);
permissionBackend.currentUser().project(projectName).check(permissionToCheck);
} catch (AuthException e) {
throw new NoSuchProjectException(projectName);
}

8
java/com/google/gerrit/server/args4j/ProjectHandler.java
View File

@ -79,7 +79,13 @@ public class ProjectHandler extends OptionHandler<ProjectState> {
if (state == null) {
throw new CmdLineException(owner, String.format("project %s not found", nameWithoutSuffix));
}
permissionBackend.currentUser().project(nameKey).check(ProjectPermission.ACCESS);
// Hidden projects(permitsRead = false) should only be accessible by the project owners.
// READ_CONFIG is checked here because it's only allowed to project owners(ACCESS may also
// be allowed for other users). Allowing project owners to access here will help them to view
// and update the config of hidden projects easily.
ProjectPermission permissionToCheck =
state.statePermitsRead() ? ProjectPermission.ACCESS : ProjectPermission.READ_CONFIG;
permissionBackend.currentUser().project(nameKey).check(permissionToCheck);
} catch (AuthException e) {
throw new CmdLineException(owner, new NoSuchProjectException(nameKey).getMessage());
} catch (PermissionBackendException | IOException e) {

5
java/com/google/gerrit/server/events/EventBroker.java
View File

@ -150,6 +150,11 @@ public class EventBroker implements EventDispatcher {
protected boolean isVisibleTo(Project.NameKey project, CurrentUser user) {
try {
ProjectState state = projectCache.get(project);
if (state == null || !state.statePermitsRead()) {
return false;
}
permissionBackend.user(user).project(project).check(ProjectPermission.ACCESS);
return true;
} catch (AuthException | PermissionBackendException e) {

8
java/com/google/gerrit/server/permissions/ProjectControl.java
View File

@ -202,11 +202,6 @@ class ProjectControl {
return false;
}
/** 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) || isAdmin());
}
@ -400,8 +395,7 @@ class ProjectControl {
private boolean can(ProjectPermission perm) throws PermissionBackendException {
switch (perm) {
case ACCESS:
return (!isHidden() && (user.isInternalUser() || canPerformOnAnyRef(Permission.READ)))
|| isOwner();
return user.isInternalUser() || isOwner() || canPerformOnAnyRef(Permission.READ);
case READ:
return allRefsAreVisible(Collections.emptySet());

6
java/com/google/gerrit/server/project/ChildProjects.java
View File

@ -53,7 +53,7 @@ public class ChildProjects {
/** Gets all child projects recursively. */
public List<ProjectInfo> list(Project.NameKey parent) throws PermissionBackendException {
Map<Project.NameKey, Project> projects = readAllProjects();
Map<Project.NameKey, Project> projects = readAllReadableProjects();
Multimap<Project.NameKey, Project.NameKey> children = parentToChildren(projects);
PermissionBackend.WithUser perm = permissionBackend.currentUser();
@ -62,11 +62,11 @@ public class ChildProjects {
return results;
}
private Map<Project.NameKey, Project> readAllProjects() {
private Map<Project.NameKey, Project> readAllReadableProjects() {
Map<Project.NameKey, Project> projects = new HashMap<>();
for (Project.NameKey name : projectCache.all()) {
ProjectState c = projectCache.get(name);
if (c != null) {
if (c != null && c.statePermitsRead()) {
projects.put(c.getNameKey(), c.getProject());
}
}

6
java/com/google/gerrit/server/project/SuggestParentCandidates.java
View File

@ -44,17 +44,17 @@ public class SuggestParentCandidates {
public List<Project.NameKey> getNameKeys() throws PermissionBackendException {
return permissionBackend
.currentUser()
.filter(ProjectPermission.ACCESS, parents())
.filter(ProjectPermission.ACCESS, readableParents())
.stream()
.sorted()
.collect(toList());
}
private Set<Project.NameKey> parents() {
private Set<Project.NameKey> readableParents() {
Set<Project.NameKey> parents = new HashSet<>();
for (Project.NameKey p : projectCache.all()) {
ProjectState ps = projectCache.get(p);
if (ps != null) {
if (ps != null && ps.statePermitsRead()) {
Project.NameKey parent = ps.getProject().getParent();
if (parent != null) {
parents.add(parent);

26
java/com/google/gerrit/server/restapi/config/ListTasks.java
View File

@ -28,6 +28,8 @@ 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;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.util.IdGenerator;
import com.google.inject.Inject;
import com.google.inject.Provider;
@ -46,13 +48,18 @@ public class ListTasks implements RestReadView<ConfigResource> {
private final PermissionBackend permissionBackend;
private final WorkQueue workQueue;
private final Provider<CurrentUser> self;
private final ProjectCache projectCache;
@Inject
public ListTasks(
PermissionBackend permissionBackend, WorkQueue workQueue, Provider<CurrentUser> self) {
PermissionBackend permissionBackend,
WorkQueue workQueue,
Provider<CurrentUser> self,
ProjectCache projectCache) {
this.permissionBackend = permissionBackend;
this.workQueue = workQueue;
this.self = self;
this.projectCache = projectCache;
}
@Override
@ -77,14 +84,17 @@ public class ListTasks implements RestReadView<ConfigResource> {
if (task.projectName != null) {
Boolean visible = visibilityCache.get(task.projectName);
if (visible == null) {
try {
permissionBackend
.user(user)
.project(new Project.NameKey(task.projectName))
.check(ProjectPermission.ACCESS);
visible = true;
} catch (AuthException e) {
Project.NameKey nameKey = new Project.NameKey(task.projectName);
ProjectState state = projectCache.get(nameKey);
if (state == null || !state.statePermitsRead()) {
visible = false;
} else {
try {
permissionBackend.user(user).project(nameKey).check(ProjectPermission.ACCESS);
visible = true;
} catch (AuthException e) {
visible = false;
}
}
visibilityCache.put(task.projectName, visible);
}

25
java/com/google/gerrit/server/restapi/config/TasksCollection.java
View File

@ -18,8 +18,10 @@ import com.google.gerrit.extensions.registration.DynamicMap;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.ChildCollection;
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.RestView;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.config.ConfigResource;
import com.google.gerrit.server.config.TaskResource;
@ -30,6 +32,8 @@ 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;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
@ -41,6 +45,7 @@ public class TasksCollection implements ChildCollection<ConfigResource, TaskReso
private final WorkQueue workQueue;
private final Provider<CurrentUser> self;
private final PermissionBackend permissionBackend;
private final ProjectCache projectCache;
@Inject
TasksCollection(
@ -48,12 +53,14 @@ public class TasksCollection implements ChildCollection<ConfigResource, TaskReso
ListTasks list,
WorkQueue workQueue,
Provider<CurrentUser> self,
PermissionBackend permissionBackend) {
PermissionBackend permissionBackend,
ProjectCache projectCache) {
this.views = views;
this.list = list;
this.workQueue = workQueue;
this.self = self;
this.permissionBackend = permissionBackend;
this.projectCache = projectCache;
}
@Override
@ -63,7 +70,8 @@ public class TasksCollection implements ChildCollection<ConfigResource, TaskReso
@Override
public TaskResource parse(ConfigResource parent, IdString id)
throws ResourceNotFoundException, AuthException, PermissionBackendException {
throws ResourceNotFoundException, AuthException, PermissionBackendException,
ResourceConflictException {
CurrentUser user = self.get();
if (!user.isIdentifiedUser()) {
throw new AuthException("Authentication required");
@ -78,11 +86,16 @@ public class TasksCollection implements ChildCollection<ConfigResource, TaskReso
Task<?> task = workQueue.getTask(taskId);
if (task instanceof ProjectTask) {
Project.NameKey nameKey = ((ProjectTask<?>) task).getProjectNameKey();
ProjectState state = projectCache.get(nameKey);
if (state == null) {
throw new ResourceNotFoundException(String.format("project %s not found", nameKey));
}
state.checkStatePermitsRead();
try {
permissionBackend
.user(user)
.project(((ProjectTask<?>) task).getProjectNameKey())
.check(ProjectPermission.ACCESS);
permissionBackend.user(user).project(nameKey).check(ProjectPermission.ACCESS);
return new TaskResource(task);
} catch (AuthException e) {
// Fall through and try view queue permission.

4
java/com/google/gerrit/server/restapi/project/ListChildProjects.java
View File

@ -80,7 +80,9 @@ public class ListChildProjects implements RestReadView<ProjectResource> {
Map<Project.NameKey, Project> children = new HashMap<>();
for (Project.NameKey name : projectCache.all()) {
ProjectState c = projectCache.get(name);
if (c != null && parent.equals(c.getProject().getParent(allProjects))) {
if (c != null
&& parent.equals(c.getProject().getParent(allProjects))
&& c.statePermitsRead()) {
children.put(c.getNameKey(), c.getProject());
}
}

5
java/com/google/gerrit/server/restapi/project/ListDashboards.java
View File

@ -88,8 +88,11 @@ public class ListDashboards implements RestReadView<ProjectResource> {
private Collection<ProjectState> tree(ProjectResource rsrc) throws PermissionBackendException {
Map<Project.NameKey, ProjectState> tree = new LinkedHashMap<>();
for (ProjectState ps : rsrc.getProjectState().tree()) {
tree.put(ps.getNameKey(), ps);
if (ps.statePermitsRead()) {
tree.put(ps.getNameKey(), ps);
}
}
tree.keySet()
.retainAll(permissionBackend.currentUser().filter(ProjectPermission.ACCESS, tree.keySet()));
return tree.values();

40
java/com/google/gerrit/server/restapi/project/ListProjects.java
View File

@ -14,6 +14,7 @@
package com.google.gerrit.server.restapi.project;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.gerrit.extensions.client.ProjectState.HIDDEN;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.stream.Collectors.toList;
@ -521,11 +522,28 @@ public class ListProjects implements RestReadView<TopLevelResource> {
if (type == FilterType.PARENT_CANDIDATES) {
matches = parentsOf(matches);
}
// TODO(dborowitz): Streamified PermissionBackend#filter.
return perm.filter(ProjectPermission.ACCESS, matches.collect(toList()))
.stream()
.sorted()
.collect(toList());
List<Project.NameKey> results = new ArrayList<>();
List<Project.NameKey> projectNameKeys = matches.sorted().collect(toList());
for (Project.NameKey nameKey : projectNameKeys) {
ProjectState state = projectCache.get(nameKey);
checkNotNull(state, "Failed to load project %s", nameKey);
// Hidden projects(permitsRead = false) should only be accessible by the project owners.
// READ_CONFIG is checked here because it's only allowed to project owners(ACCESS may also
// be allowed for other users). Allowing project owners to access here will help them to view
// and update the config of hidden projects easily.
ProjectPermission permissionToCheck =
state.statePermitsRead() ? ProjectPermission.ACCESS : ProjectPermission.READ_CONFIG;
try {
perm.project(nameKey).check(permissionToCheck);
results.add(nameKey);
} catch (AuthException e) {
// Not added to results.
}
}
return results;
}
private Stream<Project.NameKey> parentsOf(Stream<Project.NameKey> matches) {
@ -551,13 +569,19 @@ public class ListProjects implements RestReadView<TopLevelResource> {
}
private boolean isParentAccessible(
Map<Project.NameKey, Boolean> checked, PermissionBackend.WithUser perm, ProjectState p)
Map<Project.NameKey, Boolean> checked, PermissionBackend.WithUser perm, ProjectState state)
throws PermissionBackendException {
Project.NameKey name = p.getNameKey();
Project.NameKey name = state.getNameKey();
Boolean b = checked.get(name);
if (b == null) {
try {
perm.project(name).check(ProjectPermission.ACCESS);
// Hidden projects(permitsRead = false) should only be accessible by the project owners.
// READ_CONFIG is checked here because it's only allowed to project owners(ACCESS may also
// be allowed for other users). Allowing project owners to access here will help them to view
// and update the config of hidden projects easily.
ProjectPermission permissionToCheck =
state.statePermitsRead() ? ProjectPermission.ACCESS : ProjectPermission.READ_CONFIG;
perm.project(name).check(permissionToCheck);
b = true;
} catch (AuthException denied) {
b = false;

8
java/com/google/gerrit/server/restapi/project/ProjectsCollection.java
View File

@ -150,8 +150,14 @@ public class ProjectsCollection
}
if (checkAccess) {
// Hidden projects(permitsRead = false) should only be accessible by the project owners.
// READ_CONFIG is checked here because it's only allowed to project owners(ACCESS may also
// be allowed for other users). Allowing project owners to access here will help them to view
// and update the config of hidden projects easily.
ProjectPermission permissionToCheck =
state.statePermitsRead() ? ProjectPermission.ACCESS : ProjectPermission.READ_CONFIG;
try {
permissionBackend.currentUser().project(nameKey).check(ProjectPermission.ACCESS);
permissionBackend.currentUser().project(nameKey).check(permissionToCheck);
} catch (AuthException e) {
return null; // Pretend like not found on access denied.
}

6
java/com/google/gerrit/sshd/commands/KillCommand.java
View File

@ -20,6 +20,7 @@ import static com.google.gerrit.common.data.GlobalCapability.MAINTAIN_SERVER;
import com.google.gerrit.extensions.annotations.RequiresAnyCapability;
import com.google.gerrit.extensions.restapi.AuthException;
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.server.config.ConfigResource;
import com.google.gerrit.server.config.TaskResource;
@ -51,7 +52,10 @@ final class KillCommand extends SshCommand {
try {
TaskResource taskRsrc = tasksCollection.parse(cfgRsrc, IdString.fromDecoded(id));
deleteTask.apply(taskRsrc, null);
} catch (AuthException | ResourceNotFoundException | PermissionBackendException e) {
} catch (AuthException
| ResourceNotFoundException
| ResourceConflictException
| PermissionBackendException e) {
stderr.print("kill: " + id + ": No such task\n");
}
}

2
plugins/replication

@ -1 +1 @@
Subproject commit dc1e7ff434fd7fce90dd6f6c4e312280bd884bd0
Subproject commit 9b08f324462e62872ac071175e16e7553ba76e2b