opendev/gerrit
Code Issues Proposed changes

Moving mapping of default capability names out of GlobalPermission

Browse Source 
The idea of GlobalPermissions, as well as the other GerritPermission
types, is entirely internal to PermissionBackend implementations. It
is basically a coincidence that GlobalPermissions map 1-to-1 with
strings in GlobalCapabilities. Once we get into the other
GerritPermission types like ProjectPermission, there are enum values
that simply aren't meaningful to the user. In fact, today, we are
actively confusing users by saying things like "readConfig not
permitted", since readConfig is not a traditional Gerrit permission and
not something mentioned in the documentation. To fix this, we need to
stop relying on the permission names in exception message.

This change begins that process with GlobalOrPluginPermissions. These
classes no longer depend on DefaultPermissionBackend-specific strings,
and the mappings are isolated in their own class. Implementations that
do want to use them can depend on the new DefaultPermissionMappings
class; this makes it explicit that those implementations are respecting
implementation details of the old class.

Mark GlobalOrPluginPermission#permissionName() as deprecated and throw
UnsupportedOperationException, to force callers to use the new methods.
In a future change, this method will be removed from GlobalPermission
entirealy.

Change-Id: Ic4eec72aa225676fee088a985ee0a0b89154575b
This commit is contained in:
Dave Borowitz
2018-04-11 13:15:05 +02:00
parent cc4d1ac051
commit 27be6b2cea
7 changed files with 146 additions and 67 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
java/com/google/gerrit/extensions/api/access/GlobalOrPluginPermission.java
View File

@@ -17,4 +17,11 @@ package com.google.gerrit.extensions.api.access;
/**
* A {@link com.google.gerrit.server.permissions.GlobalPermission} or a {@link PluginPermission}.
*/
public interface GlobalOrPluginPermission extends GerritPermission {}
public interface GlobalOrPluginPermission extends GerritPermission {
// TODO(dborowitz): Remove this method from GerritPermission.
@Override
@Deprecated
default String permissionName() {
throw new UnsupportedOperationException();
}
}

5
java/com/google/gerrit/extensions/api/access/PluginPermission.java
View File

@@ -46,11 +46,6 @@ public class PluginPermission implements GlobalOrPluginPermission {
return fallBackToAdmin;
}
@Override
public String permissionName() {
return pluginName + '-' + capability;
}
@Override
public String describeForException() {
return capability + " for plugin " + pluginName;

11
java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java
View File

@@ -15,6 +15,7 @@
package com.google.gerrit.server.permissions;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.gerrit.server.permissions.DefaultPermissionMappings.globalPermissionName;
import static java.util.stream.Collectors.toSet;
import com.google.common.collect.Sets;
@@ -140,7 +141,7 @@ public class DefaultPermissionBackend extends PermissionBackend {
return can((GlobalPermission) perm);
} else if (perm instanceof PluginPermission) {
PluginPermission pluginPermission = (PluginPermission) perm;
return has(pluginPermission.permissionName())
return has(DefaultPermissionMappings.pluginPermissionName(pluginPermission))
|| (pluginPermission.fallBackToAdmin() && isAdmin());
}
throw new PermissionBackendException(perm + " unsupported");
@@ -158,7 +159,7 @@ public class DefaultPermissionBackend extends PermissionBackend {
case RUN_GC:
case VIEW_CACHES:
case VIEW_QUEUE:
return has(perm.permissionName()) || can(GlobalPermission.MAINTAIN_SERVER);
return has(globalPermissionName(perm)) || can(GlobalPermission.MAINTAIN_SERVER);
case CREATE_ACCOUNT:
case CREATE_GROUP:
@@ -169,11 +170,11 @@ public class DefaultPermissionBackend extends PermissionBackend {
case VIEW_ALL_ACCOUNTS:
case VIEW_CONNECTIONS:
case VIEW_PLUGINS:
return has(perm.permissionName()) || isAdmin();
return has(globalPermissionName(perm)) || isAdmin();
case ACCESS_DATABASE:
case RUN_AS:
return has(perm.permissionName());
return has(globalPermissionName(perm));
}
throw new PermissionBackendException(perm + " unsupported");
}
@@ -209,7 +210,7 @@ public class DefaultPermissionBackend extends PermissionBackend {
}
private boolean has(String permissionName) {
return allow(capabilities().getPermission(permissionName));
return allow(capabilities().getPermission(checkNotNull(permissionName)));
}
private boolean allow(Collection<PermissionRule> rules) {

94
java/com/google/gerrit/server/permissions/DefaultPermissionMappings.java Normal file
View File

@@ -0,0 +1,94 @@
// Copyright (C) 2018 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.permissions;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
import com.google.common.collect.ImmutableBiMap;
import com.google.common.collect.ImmutableMap;
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.extensions.api.access.GlobalOrPluginPermission;
import com.google.gerrit.extensions.api.access.PluginPermission;
import java.util.EnumSet;
import java.util.Optional;
import java.util.Set;
/**
* Mappings from {@link com.google.gerrit.extensions.api.access.GerritPermission} enum instances to
* the permission names used by {@link DefaultPermissionBackend}.
*
* <p>These should be considered implementation details of {@code DefaultPermissionBackend}; a
* backend that doesn't respect the default permission model will not need to consult these.
* However, implementations may also choose to respect certain aspects of the default permission
* model, so this class is provided as public to aid those implementations.
*/
public class DefaultPermissionMappings {
private static final ImmutableBiMap<GlobalPermission, String> CAPABILITIES =
ImmutableBiMap.<GlobalPermission, String>builder()
.put(GlobalPermission.ACCESS_DATABASE, GlobalCapability.ACCESS_DATABASE)
.put(GlobalPermission.ADMINISTRATE_SERVER, GlobalCapability.ADMINISTRATE_SERVER)
.put(GlobalPermission.CREATE_ACCOUNT, GlobalCapability.CREATE_ACCOUNT)
.put(GlobalPermission.CREATE_GROUP, GlobalCapability.CREATE_GROUP)
.put(GlobalPermission.CREATE_PROJECT, GlobalCapability.CREATE_PROJECT)
.put(GlobalPermission.EMAIL_REVIEWERS, GlobalCapability.EMAIL_REVIEWERS)
.put(GlobalPermission.FLUSH_CACHES, GlobalCapability.FLUSH_CACHES)
.put(GlobalPermission.KILL_TASK, GlobalCapability.KILL_TASK)
.put(GlobalPermission.MAINTAIN_SERVER, GlobalCapability.MAINTAIN_SERVER)
.put(GlobalPermission.MODIFY_ACCOUNT, GlobalCapability.MODIFY_ACCOUNT)
.put(GlobalPermission.RUN_AS, GlobalCapability.RUN_AS)
.put(GlobalPermission.RUN_GC, GlobalCapability.RUN_GC)
.put(GlobalPermission.STREAM_EVENTS, GlobalCapability.STREAM_EVENTS)
.put(GlobalPermission.VIEW_ALL_ACCOUNTS, GlobalCapability.VIEW_ALL_ACCOUNTS)
.put(GlobalPermission.VIEW_CACHES, GlobalCapability.VIEW_CACHES)
.put(GlobalPermission.VIEW_CONNECTIONS, GlobalCapability.VIEW_CONNECTIONS)
.put(GlobalPermission.VIEW_PLUGINS, GlobalCapability.VIEW_PLUGINS)
.put(GlobalPermission.VIEW_QUEUE, GlobalCapability.VIEW_QUEUE)
.build();
static {
checkMapContainsAllEnumValues(CAPABILITIES, GlobalPermission.class);
}
private static <T extends Enum<T>> void checkMapContainsAllEnumValues(
ImmutableMap<T, String> actual, Class<T> clazz) {
Set<T> expected = EnumSet.allOf(clazz);
checkState(
actual.keySet().equals(expected),
"all %s values must be defined, found: %s",
clazz.getSimpleName(),
actual.keySet());
}
public static String globalPermissionName(GlobalPermission globalPermission) {
return checkNotNull(CAPABILITIES.get(globalPermission));
}
public static Optional<GlobalPermission> globalPermission(String capabilityName) {
return Optional.ofNullable(CAPABILITIES.inverse().get(capabilityName));
}
public static String pluginPermissionName(PluginPermission pluginPermission) {
return pluginPermission.pluginName() + '-' + pluginPermission.capability();
}
public static String globalOrPluginPermissionName(GlobalOrPluginPermission permission) {
return permission instanceof GlobalPermission
? globalPermissionName((GlobalPermission) permission)
: pluginPermissionName((PluginPermission) permission);
}
private DefaultPermissionMappings() {}
}

73
java/com/google/gerrit/server/permissions/GlobalPermission.java
View File

@@ -14,9 +14,9 @@
package com.google.gerrit.server.permissions;
import com.google.common.collect.ImmutableMap;
import static com.google.gerrit.server.permissions.DefaultPermissionMappings.globalPermission;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.GlobalCapability;
import com.google.gerrit.extensions.annotations.CapabilityScope;
import com.google.gerrit.extensions.annotations.RequiresAnyCapability;
import com.google.gerrit.extensions.annotations.RequiresCapability;
@@ -25,46 +25,33 @@ import com.google.gerrit.extensions.api.access.PluginPermission;
import java.lang.annotation.Annotation;
import java.util.Collections;
import java.util.LinkedHashSet;
import java.util.Optional;
import java.util.Set;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/** Global server permissions built into Gerrit. */
public enum GlobalPermission implements GlobalOrPluginPermission {
ACCESS_DATABASE(GlobalCapability.ACCESS_DATABASE),
ADMINISTRATE_SERVER(GlobalCapability.ADMINISTRATE_SERVER),
CREATE_ACCOUNT(GlobalCapability.CREATE_ACCOUNT),
CREATE_GROUP(GlobalCapability.CREATE_GROUP),
CREATE_PROJECT(GlobalCapability.CREATE_PROJECT),
EMAIL_REVIEWERS(GlobalCapability.EMAIL_REVIEWERS),
FLUSH_CACHES(GlobalCapability.FLUSH_CACHES),
KILL_TASK(GlobalCapability.KILL_TASK),
MAINTAIN_SERVER(GlobalCapability.MAINTAIN_SERVER),
MODIFY_ACCOUNT(GlobalCapability.MODIFY_ACCOUNT),
RUN_AS(GlobalCapability.RUN_AS),
RUN_GC(GlobalCapability.RUN_GC),
STREAM_EVENTS(GlobalCapability.STREAM_EVENTS),
VIEW_ALL_ACCOUNTS(GlobalCapability.VIEW_ALL_ACCOUNTS),
VIEW_CACHES(GlobalCapability.VIEW_CACHES),
VIEW_CONNECTIONS(GlobalCapability.VIEW_CONNECTIONS),
VIEW_PLUGINS(GlobalCapability.VIEW_PLUGINS),
VIEW_QUEUE(GlobalCapability.VIEW_QUEUE);
ACCESS_DATABASE,
ADMINISTRATE_SERVER,
CREATE_ACCOUNT,
CREATE_GROUP,
CREATE_PROJECT,
EMAIL_REVIEWERS,
FLUSH_CACHES,
KILL_TASK,
MAINTAIN_SERVER,
MODIFY_ACCOUNT,
RUN_AS,
RUN_GC,
STREAM_EVENTS,
VIEW_ALL_ACCOUNTS,
VIEW_CACHES,
VIEW_CONNECTIONS,
VIEW_PLUGINS,
VIEW_QUEUE;
private static final Logger log = LoggerFactory.getLogger(GlobalPermission.class);
private static final ImmutableMap<String, GlobalPermission> BY_NAME;
static {
ImmutableMap.Builder<String, GlobalPermission> m = ImmutableMap.builder();
for (GlobalPermission p : values()) {
m.put(p.permissionName(), p);
}
BY_NAME = m.build();
}
@Nullable
public static GlobalPermission byName(String name) {
return BY_NAME.get(name);
}
/**
* Extracts the {@code @RequiresCapability} or {@code @RequiresAnyCapability} annotation.
@@ -120,18 +107,6 @@ public enum GlobalPermission implements GlobalOrPluginPermission {
return fromAnnotation(null, clazz);
}
private final String name;
GlobalPermission(String name) {
this.name = name;
}
/** @return name used in {@code project.config} permissions. */
@Override
public String permissionName() {
return name;
}
private static GlobalOrPluginPermission resolve(
@Nullable String pluginName,
String capability,
@@ -154,13 +129,13 @@ public enum GlobalPermission implements GlobalOrPluginPermission {
throw new PermissionBackendException("cannot extract permission");
}
GlobalPermission perm = byName(capability);
if (perm == null) {
Optional<GlobalPermission> perm = globalPermission(capability);
if (!perm.isPresent()) {
log.error(
String.format("Class %s requires unknown capability %s", clazz.getName(), capability));
throw new PermissionBackendException("cannot extract permission");
}
return perm;
return perm.get();
}
@Nullable

12
java/com/google/gerrit/server/restapi/account/Capabilities.java
View File

@@ -14,6 +14,9 @@
package com.google.gerrit.server.restapi.account;
import static com.google.gerrit.server.permissions.DefaultPermissionMappings.globalOrPluginPermissionName;
import static com.google.gerrit.server.permissions.DefaultPermissionMappings.globalPermission;
import com.google.gerrit.extensions.api.access.GlobalOrPluginPermission;
import com.google.gerrit.extensions.api.access.PluginPermission;
import com.google.gerrit.extensions.registration.DynamicMap;
@@ -32,6 +35,7 @@ import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
import java.util.Optional;
@Singleton
class Capabilities implements ChildCollection<AccountResource, AccountResource.Capability> {
@@ -68,16 +72,16 @@ class Capabilities implements ChildCollection<AccountResource, AccountResource.C
GlobalOrPluginPermission perm = parse(id);
if (permissionBackend.user(target).test(perm)) {
return new AccountResource.Capability(target, perm.permissionName());
return new AccountResource.Capability(target, globalOrPluginPermissionName(perm));
}
throw new ResourceNotFoundException(id);
}
private GlobalOrPluginPermission parse(IdString id) throws ResourceNotFoundException {
String name = id.get();
GlobalOrPluginPermission perm = GlobalPermission.byName(name);
if (perm != null) {
return perm;
Optional<GlobalPermission> perm = globalPermission(name);
if (perm.isPresent()) {
return perm.get();
}
int dash = name.lastIndexOf('-');

9
java/com/google/gerrit/server/restapi/account/GetCapabilities.java
View File

@@ -15,6 +15,9 @@
package com.google.gerrit.server.restapi.account;
import static com.google.gerrit.common.data.GlobalCapability.PRIORITY;
import static com.google.gerrit.server.permissions.DefaultPermissionMappings.globalOrPluginPermissionName;
import static com.google.gerrit.server.permissions.DefaultPermissionMappings.globalPermissionName;
import static com.google.gerrit.server.permissions.DefaultPermissionMappings.pluginPermissionName;
import com.google.common.collect.Iterables;
import com.google.gerrit.common.data.GlobalCapability;
@@ -86,7 +89,7 @@ class GetCapabilities implements RestReadView<AccountResource> {
Map<String, Object> have = new LinkedHashMap<>();
for (GlobalOrPluginPermission p : perm.test(permissionsToTest())) {
have.put(p.permissionName(), true);
have.put(globalOrPluginPermissionName(p), true);
}
AccountLimits limits = limitsFactory.create(rsrc.getUser());
@@ -101,7 +104,7 @@ class GetCapabilities implements RestReadView<AccountResource> {
private Set<GlobalOrPluginPermission> permissionsToTest() {
Set<GlobalOrPluginPermission> toTest = new HashSet<>();
for (GlobalPermission p : GlobalPermission.values()) {
if (want(p.permissionName())) {
if (want(globalPermissionName(p))) {
toTest.add(p);
}
}
@@ -109,7 +112,7 @@ class GetCapabilities implements RestReadView<AccountResource> {
for (String pluginName : pluginCapabilities.plugins()) {
for (String capability : pluginCapabilities.byPlugin(pluginName).keySet()) {
PluginPermission p = new PluginPermission(pluginName, capability);
if (want(p.permissionName())) {
if (want(pluginPermissionName(p))) {
toTest.add(p);
}
}