From 6302ccb2bbd1763be42ed763f7461e4cc8396c20 Mon Sep 17 00:00:00 2001 From: Shawn Pearce Date: Sat, 29 Apr 2017 14:23:56 -0700 Subject: [PATCH] Remove CapabilityControl from CurrentUser Drop the capabilities reference from all user objects. Most global capabilities can be checked with the PermissionBackend. QoS, query limits, and emailing reviewers still require the capability object. Bundle its factory into the call sites that need it. Continue caching the CapabilityControl in an opaque property on the CurrentUser, and also in the DefaultPermissionBackend.WithUserImpl. Both of these sites reduce evaluations for critical properties like "administrateServer". Change-Id: I5aae8200e0a579ac1295a3fb7005703fd39d2696 --- .../pgm/http/jetty/ProjectQoSFilter.java | 9 ++- .../gerrit/pgm/util/BatchProgramModule.java | 2 - .../google/gerrit/server/AnonymousUser.java | 7 -- .../com/google/gerrit/server/CurrentUser.java | 16 ---- .../google/gerrit/server/IdentifiedUser.java | 23 ------ .../google/gerrit/server/InternalUser.java | 9 --- .../google/gerrit/server/PeerDaemonUser.java | 5 +- .../com/google/gerrit/server/PluginUser.java | 5 +- .../gerrit/server/account/AccountControl.java | 63 ++++++++++----- .../server/account/CapabilityControl.java | 76 ++++++++----------- .../server/account/GetCapabilities.java | 16 ++-- .../gerrit/server/account/GroupControl.java | 47 +++++++++--- .../server/config/GerritGlobalModule.java | 2 - .../gerrit/server/git/ReceiveConfig.java | 6 +- .../gerrit/server/mail/send/ChangeEmail.java | 11 ++- .../server/mail/send/EmailArguments.java | 8 +- .../gerrit/server/mail/send/ProjectWatch.java | 2 +- .../gerrit/server/project/ChangeControl.java | 21 ++--- .../project/DefaultPermissionBackend.java | 11 ++- .../gerrit/server/project/ProjectControl.java | 19 ++++- .../gerrit/server/project/RefControl.java | 12 ++- .../gerrit/server/query/QueryProcessor.java | 9 ++- .../query/account/AccountQueryProcessor.java | 3 + .../query/change/ChangeQueryBuilder.java | 9 +-- .../query/change/ChangeQueryProcessor.java | 3 + .../server/query/change/SingleGroupUser.java | 12 +-- .../query/group/GroupQueryProcessor.java | 3 + .../gerrit/server/IdentifiedUserTest.java | 4 - .../server/index/change/FakeQueryBuilder.java | 4 +- .../notedb/AbstractChangeNotesTest.java | 5 +- .../gerrit/server/project/RefControlTest.java | 6 +- .../gerrit/sshd/CommandExecutorProvider.java | 9 ++- plugins/replication | 2 +- 33 files changed, 222 insertions(+), 217 deletions(-) diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/ProjectQoSFilter.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/ProjectQoSFilter.java index 737e5381b1..256164a97b 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/ProjectQoSFilter.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/ProjectQoSFilter.java @@ -20,6 +20,7 @@ import static java.util.concurrent.TimeUnit.MINUTES; import static javax.servlet.http.HttpServletResponse.SC_SERVICE_UNAVAILABLE; import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.git.QueueProvider; import com.google.gerrit.server.git.WorkQueue.CancelableRunnable; @@ -69,7 +70,6 @@ public class ProjectQoSFilter implements Filter { private static final Pattern URI_PATTERN = Pattern.compile(FILTER_RE); public static class Module extends ServletModule { - @Override protected void configureServlets() { bind(QueueProvider.class).to(CommandExecutorQueueProvider.class).in(SINGLETON); @@ -77,18 +77,20 @@ public class ProjectQoSFilter implements Filter { } } + private final CapabilityControl.Factory capabilityFactory; private final Provider user; private final QueueProvider queue; - private final ServletContext context; private final long maxWait; @Inject ProjectQoSFilter( + CapabilityControl.Factory capabilityFactory, Provider user, QueueProvider queue, ServletContext context, @GerritServerConfig Config cfg) { + this.capabilityFactory = capabilityFactory; this.user = user; this.queue = queue; this.context = context; @@ -137,7 +139,8 @@ public class ProjectQoSFilter implements Filter { } private ScheduledThreadPoolExecutor getExecutor() { - return queue.getQueue(user.get().getCapabilities().getQueueType()); + QueueProvider.QueueType qt = capabilityFactory.create(user.get()).getQueueType(); + return queue.getQueue(qt); } @Override diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/BatchProgramModule.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/BatchProgramModule.java index c051bff860..788f7ced3f 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/BatchProgramModule.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/util/BatchProgramModule.java @@ -32,7 +32,6 @@ import com.google.gerrit.server.account.AccountCacheImpl; import com.google.gerrit.server.account.AccountVisibility; import com.google.gerrit.server.account.AccountVisibilityProvider; import com.google.gerrit.server.account.CapabilityCollection; -import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.account.FakeRealm; import com.google.gerrit.server.account.GroupCacheImpl; import com.google.gerrit.server.account.GroupIncludeCacheImpl; @@ -164,7 +163,6 @@ public class BatchProgramModule extends FactoryModule { install(MergeabilityCacheImpl.module()); install(TagCache.module()); factory(CapabilityCollection.Factory.class); - factory(CapabilityControl.Factory.class); factory(ChangeData.Factory.class); factory(ProjectState.Factory.class); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/AnonymousUser.java b/gerrit-server/src/main/java/com/google/gerrit/server/AnonymousUser.java index de8e9a428f..c96d61aab5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/AnonymousUser.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/AnonymousUser.java @@ -14,20 +14,13 @@ package com.google.gerrit.server; -import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.account.GroupMembership; import com.google.gerrit.server.account.ListGroupMembership; import com.google.gerrit.server.group.SystemGroupBackend; -import com.google.inject.Inject; import java.util.Collections; /** An anonymous user who has not yet authenticated. */ public class AnonymousUser extends CurrentUser { - @Inject - AnonymousUser(CapabilityControl.Factory capabilityControlFactory) { - super(capabilityControlFactory); - } - @Override public GroupMembership getEffectiveGroups() { return new ListGroupMembership(Collections.singleton(SystemGroupBackend.ANONYMOUS_USERS)); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/CurrentUser.java b/gerrit-server/src/main/java/com/google/gerrit/server/CurrentUser.java index 7e1be17117..7ac8fdaca0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/CurrentUser.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/CurrentUser.java @@ -16,7 +16,6 @@ package com.google.gerrit.server; import com.google.gerrit.common.Nullable; import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.account.GroupMembership; import com.google.gerrit.server.account.externalids.ExternalId; import com.google.inject.servlet.RequestScoped; @@ -40,16 +39,9 @@ public abstract class CurrentUser { private PropertyKey() {} } - private final CapabilityControl.Factory capabilityControlFactory; private AccessPath accessPath = AccessPath.UNKNOWN; - - private CapabilityControl capabilities; private PropertyKey lastLoginExternalIdPropertyKey = PropertyKey.create(); - protected CurrentUser(CapabilityControl.Factory capabilityControlFactory) { - this.capabilityControlFactory = capabilityControlFactory; - } - /** How this user is accessing the Gerrit Code Review application. */ public final AccessPath getAccessPath() { return accessPath; @@ -98,14 +90,6 @@ public abstract class CurrentUser { return null; } - /** Capabilities available to this user account. */ - public CapabilityControl getCapabilities() { - if (capabilities == null) { - capabilities = capabilityControlFactory.create(this); - } - return capabilities; - } - /** Check if user is the IdentifiedUser */ public boolean isIdentifiedUser() { return false; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java b/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java index 5121a6fda8..e39dc69718 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java @@ -21,7 +21,6 @@ import com.google.gerrit.common.Nullable; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.AccountState; -import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.account.GroupBackend; import com.google.gerrit.server.account.GroupMembership; import com.google.gerrit.server.account.ListGroupMembership; @@ -55,7 +54,6 @@ public class IdentifiedUser extends CurrentUser { /** Create an IdentifiedUser, ignoring any per-request state. */ @Singleton public static class GenericFactory { - private final CapabilityControl.Factory capabilityControlFactory; private final AuthConfig authConfig; private final Realm realm; private final String anonymousCowardName; @@ -66,7 +64,6 @@ public class IdentifiedUser extends CurrentUser { @Inject public GenericFactory( - @Nullable CapabilityControl.Factory capabilityControlFactory, AuthConfig authConfig, Realm realm, @AnonymousCowardName String anonymousCowardName, @@ -74,7 +71,6 @@ public class IdentifiedUser extends CurrentUser { @DisableReverseDnsLookup Boolean disableReverseDnsLookup, AccountCache accountCache, GroupBackend groupBackend) { - this.capabilityControlFactory = capabilityControlFactory; this.authConfig = authConfig; this.realm = realm; this.anonymousCowardName = anonymousCowardName; @@ -86,7 +82,6 @@ public class IdentifiedUser extends CurrentUser { public IdentifiedUser create(AccountState state) { return new IdentifiedUser( - capabilityControlFactory, authConfig, realm, anonymousCowardName, @@ -110,7 +105,6 @@ public class IdentifiedUser extends CurrentUser { public IdentifiedUser runAs( SocketAddress remotePeer, Account.Id id, @Nullable CurrentUser caller) { return new IdentifiedUser( - capabilityControlFactory, authConfig, realm, anonymousCowardName, @@ -132,7 +126,6 @@ public class IdentifiedUser extends CurrentUser { */ @Singleton public static class RequestFactory { - private final CapabilityControl.Factory capabilityControlFactory; private final AuthConfig authConfig; private final Realm realm; private final String anonymousCowardName; @@ -144,7 +137,6 @@ public class IdentifiedUser extends CurrentUser { @Inject RequestFactory( - CapabilityControl.Factory capabilityControlFactory, AuthConfig authConfig, Realm realm, @AnonymousCowardName String anonymousCowardName, @@ -153,7 +145,6 @@ public class IdentifiedUser extends CurrentUser { GroupBackend groupBackend, @DisableReverseDnsLookup Boolean disableReverseDnsLookup, @RemotePeer Provider remotePeerProvider) { - this.capabilityControlFactory = capabilityControlFactory; this.authConfig = authConfig; this.realm = realm; this.anonymousCowardName = anonymousCowardName; @@ -166,7 +157,6 @@ public class IdentifiedUser extends CurrentUser { public IdentifiedUser create(Account.Id id) { return new IdentifiedUser( - capabilityControlFactory, authConfig, realm, anonymousCowardName, @@ -181,7 +171,6 @@ public class IdentifiedUser extends CurrentUser { public IdentifiedUser runAs(Account.Id id, CurrentUser caller) { return new IdentifiedUser( - capabilityControlFactory, authConfig, realm, anonymousCowardName, @@ -219,7 +208,6 @@ public class IdentifiedUser extends CurrentUser { private Map, Object> properties; private IdentifiedUser( - CapabilityControl.Factory capabilityControlFactory, AuthConfig authConfig, Realm realm, String anonymousCowardName, @@ -231,7 +219,6 @@ public class IdentifiedUser extends CurrentUser { AccountState state, @Nullable CurrentUser realUser) { this( - capabilityControlFactory, authConfig, realm, anonymousCowardName, @@ -246,7 +233,6 @@ public class IdentifiedUser extends CurrentUser { } private IdentifiedUser( - CapabilityControl.Factory capabilityControlFactory, AuthConfig authConfig, Realm realm, String anonymousCowardName, @@ -257,7 +243,6 @@ public class IdentifiedUser extends CurrentUser { @Nullable Provider remotePeerProvider, Account.Id id, @Nullable CurrentUser realUser) { - super(capabilityControlFactory); this.canonicalUrl = canonicalUrl; this.accountCache = accountCache; this.groupBackend = groupBackend; @@ -465,7 +450,6 @@ public class IdentifiedUser extends CurrentUser { * @return copy of the identified user */ public IdentifiedUser materializedCopy() { - CapabilityControl capabilities = getCapabilities(); Provider remotePeer; try { remotePeer = Providers.of(remotePeerProvider.get()); @@ -479,13 +463,6 @@ public class IdentifiedUser extends CurrentUser { }; } return new IdentifiedUser( - new CapabilityControl.Factory() { - - @Override - public CapabilityControl create(CurrentUser user) { - return capabilities; - } - }, authConfig, realm, anonymousCowardName, diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/InternalUser.java b/gerrit-server/src/main/java/com/google/gerrit/server/InternalUser.java index bc99ec148c..821a0c6135 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/InternalUser.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/InternalUser.java @@ -14,10 +14,7 @@ package com.google.gerrit.server; -import com.google.common.annotations.VisibleForTesting; -import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.account.GroupMembership; -import com.google.inject.Inject; /** * User identity for plugin code that needs an identity. @@ -33,12 +30,6 @@ public class InternalUser extends CurrentUser { InternalUser create(); } - @VisibleForTesting - @Inject - public InternalUser(CapabilityControl.Factory capabilityControlFactory) { - super(capabilityControlFactory); - } - @Override public GroupMembership getEffectiveGroups() { return GroupMembership.EMPTY; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/PeerDaemonUser.java b/gerrit-server/src/main/java/com/google/gerrit/server/PeerDaemonUser.java index 263bb5077c..8a8b67a2be 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/PeerDaemonUser.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/PeerDaemonUser.java @@ -14,7 +14,6 @@ package com.google.gerrit.server; -import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.account.GroupMembership; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; @@ -32,9 +31,7 @@ public class PeerDaemonUser extends CurrentUser { private final SocketAddress peer; @Inject - protected PeerDaemonUser( - CapabilityControl.Factory capabilityControlFactory, @Assisted SocketAddress peer) { - super(capabilityControlFactory); + protected PeerDaemonUser(@Assisted SocketAddress peer) { this.peer = peer; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/PluginUser.java b/gerrit-server/src/main/java/com/google/gerrit/server/PluginUser.java index 13e04c552b..09f50431d6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/PluginUser.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/PluginUser.java @@ -14,7 +14,6 @@ package com.google.gerrit.server; -import com.google.gerrit.server.account.CapabilityControl; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; @@ -27,9 +26,7 @@ public class PluginUser extends InternalUser { private final String pluginName; @Inject - protected PluginUser( - CapabilityControl.Factory capabilityControlFactory, @Assisted String pluginName) { - super(capabilityControlFactory); + protected PluginUser(@Assisted String pluginName) { this.pluginName = pluginName; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountControl.java index c6abb5b53f..bb118a3b56 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AccountControl.java @@ -18,12 +18,16 @@ import static java.util.stream.Collectors.toSet; import com.google.gerrit.common.data.PermissionRule; import com.google.gerrit.common.errors.NoSuchGroupException; +import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.git.AccountsSection; import com.google.gerrit.server.group.SystemGroupBackend; +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.project.ProjectCache; import com.google.inject.Inject; import com.google.inject.Provider; @@ -32,6 +36,7 @@ import java.util.Set; /** Access control management for one account's access to other accounts. */ public class AccountControl { public static class Factory { + private final PermissionBackend permissionBackend; private final ProjectCache projectCache; private final GroupControl.Factory groupControlFactory; private final Provider user; @@ -40,11 +45,13 @@ public class AccountControl { @Inject Factory( - final ProjectCache projectCache, - final GroupControl.Factory groupControlFactory, - final Provider user, - final IdentifiedUser.GenericFactory userFactory, - final AccountVisibility accountVisibility) { + PermissionBackend permissionBackend, + ProjectCache projectCache, + GroupControl.Factory groupControlFactory, + Provider user, + IdentifiedUser.GenericFactory userFactory, + AccountVisibility accountVisibility) { + this.permissionBackend = permissionBackend; this.projectCache = projectCache; this.groupControlFactory = groupControlFactory; this.user = user; @@ -54,24 +61,34 @@ public class AccountControl { public AccountControl get() { return new AccountControl( - projectCache, groupControlFactory, user.get(), userFactory, accountVisibility); + permissionBackend, + projectCache, + groupControlFactory, + user.get(), + userFactory, + accountVisibility); } } private final AccountsSection accountsSection; private final GroupControl.Factory groupControlFactory; + private final PermissionBackend.WithUser perm; private final CurrentUser user; private final IdentifiedUser.GenericFactory userFactory; private final AccountVisibility accountVisibility; + private Boolean viewAll; + AccountControl( - final ProjectCache projectCache, - final GroupControl.Factory groupControlFactory, - final CurrentUser user, - final IdentifiedUser.GenericFactory userFactory, - final AccountVisibility accountVisibility) { + PermissionBackend permissionBackend, + ProjectCache projectCache, + GroupControl.Factory groupControlFactory, + CurrentUser user, + IdentifiedUser.GenericFactory userFactory, + AccountVisibility accountVisibility) { this.accountsSection = projectCache.getAllProjects().getConfig().getAccountsSection(); this.groupControlFactory = groupControlFactory; + this.perm = permissionBackend.user(user); this.user = user; this.userFactory = userFactory; this.accountVisibility = accountVisibility; @@ -137,17 +154,16 @@ public class AccountControl { } private boolean canSee(OtherUser otherUser) { - // Special case: I can always see myself. - if (user.isIdentifiedUser() && user.getAccountId().equals(otherUser.getId())) { + if (accountVisibility == AccountVisibility.ALL) { return true; - } - if (user.getCapabilities().canViewAllAccounts()) { + } else if (user.isIdentifiedUser() && user.getAccountId().equals(otherUser.getId())) { + // I can always see myself. + return true; + } else if (viewAll()) { return true; } switch (accountVisibility) { - case ALL: - return true; case SAME_GROUP: { Set usersGroups = groupsOf(otherUser.getUser()); @@ -178,12 +194,25 @@ public class AccountControl { } case NONE: break; + case ALL: default: throw new IllegalStateException("Bad AccountVisibility " + accountVisibility); } return false; } + private boolean viewAll() { + if (viewAll == null) { + try { + perm.check(GlobalPermission.VIEW_ALL_ACCOUNTS); + viewAll = true; + } catch (AuthException | PermissionBackendException e) { + viewAll = false; + } + } + return viewAll; + } + private Set groupsOf(IdentifiedUser user) { return user.getEffectiveGroups() .getKnownGroups() diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/CapabilityControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/CapabilityControl.java index bb40c26d3d..03ffa67687 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/CapabilityControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/CapabilityControl.java @@ -30,12 +30,9 @@ import com.google.gerrit.server.git.QueueProvider; import com.google.gerrit.server.group.SystemGroupBackend; import com.google.gerrit.server.permissions.GlobalPermission; import com.google.gerrit.server.permissions.PermissionBackendException; -import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ProjectCache; -import com.google.gerrit.server.project.ProjectControl; -import com.google.gerrit.server.project.RefControl; import com.google.inject.Inject; -import com.google.inject.assistedinject.Assisted; +import com.google.inject.Singleton; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -45,41 +42,40 @@ import java.util.Map; /** Access control management for server-wide capabilities. */ public class CapabilityControl { - public interface Factory { - CapabilityControl create(CurrentUser user); + private static final CurrentUser.PropertyKey SELF = + CurrentUser.PropertyKey.create(); + + @Singleton + public static class Factory { + private final ProjectCache projectCache; + + @Inject + Factory(ProjectCache projectCache) { + this.projectCache = projectCache; + } + + public CapabilityControl create(CurrentUser user) { + CapabilityControl ctl = user.get(SELF); + if (ctl == null) { + ctl = new CapabilityControl(projectCache, user); + user.put(SELF, ctl); + } + return ctl; + } } private final CapabilityCollection capabilities; private final CurrentUser user; private final Map> effective; - private Boolean canAdministrateServer; - private Boolean canEmailReviewers; - @Inject - CapabilityControl(ProjectCache projectCache, @Assisted CurrentUser currentUser) { + private CapabilityControl(ProjectCache projectCache, CurrentUser currentUser) { capabilities = projectCache.getAllProjects().getCapabilityCollection(); user = currentUser; effective = new HashMap<>(); } - /** - * Do not use. Determine if the user can administer this server. - * - *

This method is visible only for the benefit of the following transitional classes: - * - *

    - *
  • {@link ProjectControl} - *
  • {@link RefControl} - *
  • {@link ChangeControl} - *
  • {@link GroupControl} - *
- * - * Other callers should not use this method, as it is slated to go away. - * - * @return true if the user can administer this server. - */ - public boolean isAdmin_DoNotUse() { + private boolean isAdmin() { if (canAdministrateServer == null) { if (user.getRealUser() != user) { canAdministrateServer = false; @@ -93,18 +89,9 @@ public class CapabilityControl { } /** @return true if the user can email reviewers. */ - public boolean canEmailReviewers() { - if (canEmailReviewers == null) { - canEmailReviewers = - matchAny(capabilities.emailReviewers, ALLOWED_RULE) - || !matchAny(capabilities.emailReviewers, not(ALLOWED_RULE)); - } - return canEmailReviewers; - } - - /** @return true if the user can view all accounts. */ - public boolean canViewAllAccounts() { - return canPerform(GlobalCapability.VIEW_ALL_ACCOUNTS) || isAdmin_DoNotUse(); + private boolean canEmailReviewers() { + return matchAny(capabilities.emailReviewers, ALLOWED_RULE) + || !matchAny(capabilities.emailReviewers, not(ALLOWED_RULE)); } /** @return which priority queue the user's tasks should be submitted to. */ @@ -226,7 +213,7 @@ public class CapabilityControl { } else if (perm instanceof PluginPermission) { PluginPermission pluginPermission = (PluginPermission) perm; return canPerform(pluginPermission.permissionName()) - || (pluginPermission.fallBackToAdmin() && isAdmin_DoNotUse()); + || (pluginPermission.fallBackToAdmin() && isAdmin()); } throw new PermissionBackendException(perm + " unsupported"); } @@ -234,11 +221,9 @@ public class CapabilityControl { private boolean can(GlobalPermission perm) throws PermissionBackendException { switch (perm) { case ADMINISTRATE_SERVER: - return isAdmin_DoNotUse(); + return isAdmin(); case EMAIL_REVIEWERS: return canEmailReviewers(); - case VIEW_ALL_ACCOUNTS: - return canViewAllAccounts(); case FLUSH_CACHES: case KILL_TASK: @@ -247,7 +232,7 @@ public class CapabilityControl { case VIEW_QUEUE: return canPerform(perm.permissionName()) || canPerform(GlobalCapability.MAINTAIN_SERVER) - || isAdmin_DoNotUse(); + || isAdmin(); case CREATE_ACCOUNT: case CREATE_GROUP: @@ -255,9 +240,10 @@ public class CapabilityControl { case MAINTAIN_SERVER: case MODIFY_ACCOUNT: case STREAM_EVENTS: + case VIEW_ALL_ACCOUNTS: case VIEW_CONNECTIONS: case VIEW_PLUGINS: - return canPerform(perm.permissionName()) || isAdmin_DoNotUse(); + return canPerform(perm.permissionName()) || isAdmin(); case ACCESS_DATABASE: case RUN_AS: diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetCapabilities.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetCapabilities.java index f519b6cdca..fe01b322b3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetCapabilities.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetCapabilities.java @@ -56,15 +56,18 @@ class GetCapabilities implements RestReadView { private Set query; private final PermissionBackend permissionBackend; + private final CapabilityControl.Factory capabilityFactory; private final Provider self; private final DynamicMap pluginCapabilities; @Inject GetCapabilities( PermissionBackend permissionBackend, + CapabilityControl.Factory capabilityFactory, Provider self, DynamicMap pluginCapabilities) { this.permissionBackend = permissionBackend; + this.capabilityFactory = capabilityFactory; this.self = self; this.pluginCapabilities = pluginCapabilities; } @@ -81,8 +84,10 @@ class GetCapabilities implements RestReadView { for (GlobalOrPluginPermission p : perm.test(permissionsToTest())) { have.put(p.permissionName(), true); } - addRanges(have, rsrc); - addPriority(have, rsrc); + + CapabilityControl cc = capabilityFactory.create(rsrc.getUser()); + addRanges(have, cc); + addPriority(have, cc); return OutputFormat.JSON .newGson() @@ -112,8 +117,7 @@ class GetCapabilities implements RestReadView { return query == null || query.contains(name.toLowerCase()); } - private void addRanges(Map have, AccountResource rsrc) { - CapabilityControl cc = rsrc.getUser().getCapabilities(); + private void addRanges(Map have, CapabilityControl cc) { for (String name : GlobalCapability.getRangeNames()) { if (want(name) && cc.hasExplicitRange(name)) { have.put(name, new Range(cc.getRange(name))); @@ -121,8 +125,8 @@ class GetCapabilities implements RestReadView { } } - private void addPriority(Map have, AccountResource rsrc) { - QueueProvider.QueueType queue = rsrc.getUser().getCapabilities().getQueueType(); + private void addPriority(Map have, CapabilityControl cc) { + QueueProvider.QueueType queue = cc.getQueueType(); if (queue != QueueProvider.QueueType.INTERACTIVE || (query != null && query.contains(PRIORITY))) { have.put(PRIORITY, queue); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupControl.java index 4b4c2663be..6721e11953 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupControl.java @@ -17,9 +17,13 @@ package com.google.gerrit.server.account; import com.google.gerrit.common.data.GroupDescription; import com.google.gerrit.common.data.GroupDescriptions; import com.google.gerrit.common.errors.NoSuchGroupException; +import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.permissions.GlobalPermission; +import com.google.gerrit.server.permissions.PermissionBackend; +import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; @@ -29,30 +33,38 @@ public class GroupControl { @Singleton public static class GenericFactory { + private final PermissionBackend permissionBackend; private final GroupBackend groupBackend; @Inject - GenericFactory(GroupBackend gb) { + GenericFactory(PermissionBackend permissionBackend, GroupBackend gb) { + this.permissionBackend = permissionBackend; groupBackend = gb; } public GroupControl controlFor(CurrentUser who, AccountGroup.UUID groupId) throws NoSuchGroupException { - final GroupDescription.Basic group = groupBackend.get(groupId); + GroupDescription.Basic group = groupBackend.get(groupId); if (group == null) { throw new NoSuchGroupException(groupId); } - return new GroupControl(who, group, groupBackend); + return new GroupControl(who, group, permissionBackend, groupBackend); } } public static class Factory { + private final PermissionBackend permissionBackend; private final GroupCache groupCache; private final Provider user; private final GroupBackend groupBackend; @Inject - Factory(GroupCache gc, Provider cu, GroupBackend gb) { + Factory( + PermissionBackend permissionBackend, + GroupCache gc, + Provider cu, + GroupBackend gb) { + this.permissionBackend = permissionBackend; groupCache = gc; user = cu; groupBackend = gb; @@ -79,7 +91,7 @@ public class GroupControl { } public GroupControl controlFor(GroupDescription.Basic group) { - return new GroupControl(user.get(), group, groupBackend); + return new GroupControl(user.get(), group, permissionBackend, groupBackend); } public GroupControl validateFor(AccountGroup.Id groupId) throws NoSuchGroupException { @@ -102,11 +114,17 @@ public class GroupControl { private final CurrentUser user; private final GroupDescription.Basic group; private Boolean isOwner; + private final PermissionBackend.WithUser perm; private final GroupBackend groupBackend; - GroupControl(CurrentUser who, GroupDescription.Basic gd, GroupBackend gb) { + GroupControl( + CurrentUser who, + GroupDescription.Basic gd, + PermissionBackend permissionBackend, + GroupBackend gb) { user = who; group = gd; + this.perm = permissionBackend.user(user); groupBackend = gb; } @@ -127,8 +145,8 @@ public class GroupControl { return user.isInternalUser() || groupBackend.isVisibleToAll(group.getGroupUUID()) || user.getEffectiveGroups().contains(group.getGroupUUID()) - || user.getCapabilities().isAdmin_DoNotUse() - || isOwner(); + || isOwner() + || canAdministrateServer(); } public boolean isOwner() { @@ -137,13 +155,20 @@ public class GroupControl { isOwner = false; } else if (isOwner == null) { AccountGroup.UUID ownerUUID = accountGroup.getOwnerGroupUUID(); - isOwner = - getUser().getEffectiveGroups().contains(ownerUUID) - || getUser().getCapabilities().isAdmin_DoNotUse(); + isOwner = getUser().getEffectiveGroups().contains(ownerUUID) || canAdministrateServer(); } return isOwner; } + private boolean canAdministrateServer() { + try { + perm.check(GlobalPermission.ADMINISTRATE_SERVER); + return true; + } catch (AuthException | PermissionBackendException denied) { + return false; + } + } + public boolean canAddMember() { return isOwner(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java index 366cbd1e1c..3a1e43c8a6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java @@ -85,7 +85,6 @@ import com.google.gerrit.server.account.AccountResolver; import com.google.gerrit.server.account.AccountVisibility; import com.google.gerrit.server.account.AccountVisibilityProvider; import com.google.gerrit.server.account.CapabilityCollection; -import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.account.ChangeUserName; import com.google.gerrit.server.account.EmailExpander; import com.google.gerrit.server.account.GroupCacheImpl; @@ -246,7 +245,6 @@ public class GerritGlobalModule extends FactoryModule { factory(DeleteReviewerSender.Factory.class); factory(AddKeySender.Factory.class); factory(CapabilityCollection.Factory.class); - factory(CapabilityControl.Factory.class); factory(ChangeData.Factory.class); factory(ChangeJson.AssistedFactory.class); factory(CreateChangeSender.Factory.class); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveConfig.java index 723fb6fde8..3a82456a43 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveConfig.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveConfig.java @@ -29,18 +29,20 @@ class ReceiveConfig { final boolean checkReferencedObjectsAreReachable; final boolean allowDrafts; private final int systemMaxBatchChanges; + private final CapabilityControl.Factory capabilityFactory; @Inject - ReceiveConfig(@GerritServerConfig Config config) { + ReceiveConfig(@GerritServerConfig Config config, CapabilityControl.Factory capabilityFactory) { checkMagicRefs = config.getBoolean("receive", null, "checkMagicRefs", true); checkReferencedObjectsAreReachable = config.getBoolean("receive", null, "checkReferencedObjectsAreReachable", true); allowDrafts = config.getBoolean("change", null, "allowDrafts", true); systemMaxBatchChanges = config.getInt("receive", "maxBatchChanges", 0); + this.capabilityFactory = capabilityFactory; } public int getEffectiveMaxBatchChangesLimit(CurrentUser user) { - CapabilityControl cap = user.getCapabilities(); + CapabilityControl cap = capabilityFactory.create(user); if (cap.hasExplicitRange(BATCH_CHANGES_LIMIT)) { return cap.getRange(BATCH_CHANGES_LIMIT).getMax(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java index 14512c33c3..15a4b13df9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ChangeEmail.java @@ -19,6 +19,7 @@ import com.google.common.collect.ListMultimap; import com.google.gerrit.common.errors.EmailException; import com.google.gerrit.extensions.api.changes.NotifyHandling; import com.google.gerrit.extensions.api.changes.RecipientType; +import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.reviewdb.client.Change; @@ -37,6 +38,8 @@ import com.google.gerrit.server.patch.PatchList; import com.google.gerrit.server.patch.PatchListEntry; import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; +import com.google.gerrit.server.permissions.GlobalPermission; +import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.project.ProjectState; import com.google.gerrit.server.query.change.ChangeData; import com.google.gwtorm.server.OrmException; @@ -94,8 +97,12 @@ public abstract class ChangeEmail extends NotificationEmail { super.setFrom(id); /** Is the from user in an email squelching group? */ - final IdentifiedUser user = args.identifiedUserFactory.create(id); - emailOnlyAuthors = !user.getCapabilities().canEmailReviewers(); + try { + IdentifiedUser user = args.identifiedUserFactory.create(id); + args.permissionBackend.user(user).check(GlobalPermission.EMAIL_REVIEWERS); + } catch (AuthException | PermissionBackendException e) { + emailOnlyAuthors = true; + } } public void setPatchSet(PatchSet ps) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/EmailArguments.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/EmailArguments.java index 683416f76f..3bf5db1ff5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/EmailArguments.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/EmailArguments.java @@ -24,7 +24,6 @@ import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.IdentifiedUser.GenericFactory; import com.google.gerrit.server.StarredChangesUtil; import com.google.gerrit.server.account.AccountCache; -import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.account.GroupBackend; import com.google.gerrit.server.account.GroupIncludeCache; import com.google.gerrit.server.config.AllProjectsName; @@ -36,6 +35,7 @@ import com.google.gerrit.server.mail.EmailSettings; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.patch.PatchListCache; import com.google.gerrit.server.patch.PatchSetInfoFactory; +import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.query.account.InternalAccountQuery; import com.google.gerrit.server.query.change.ChangeData; @@ -52,6 +52,7 @@ import org.eclipse.jgit.lib.PersonIdent; public class EmailArguments { final GitRepositoryManager server; final ProjectCache projectCache; + final PermissionBackend permissionBackend; final GroupBackend groupBackend; final GroupIncludeCache groupIncludes; final AccountCache accountCache; @@ -61,7 +62,6 @@ public class EmailArguments { final EmailSender emailSender; final PatchSetInfoFactory patchSetInfoFactory; final IdentifiedUser.GenericFactory identifiedUserFactory; - final CapabilityControl.Factory capabilityControlFactory; final ChangeNotes.Factory changeNotesFactory; final AnonymousUser anonymousUser; final String anonymousCowardName; @@ -86,6 +86,7 @@ public class EmailArguments { EmailArguments( GitRepositoryManager server, ProjectCache projectCache, + PermissionBackend permissionBackend, GroupBackend groupBackend, GroupIncludeCache groupIncludes, AccountCache accountCache, @@ -95,7 +96,6 @@ public class EmailArguments { EmailSender emailSender, PatchSetInfoFactory patchSetInfoFactory, GenericFactory identifiedUserFactory, - CapabilityControl.Factory capabilityControlFactory, ChangeNotes.Factory changeNotesFactory, AnonymousUser anonymousUser, @AnonymousCowardName String anonymousCowardName, @@ -116,6 +116,7 @@ public class EmailArguments { OutgoingEmailValidator validator) { this.server = server; this.projectCache = projectCache; + this.permissionBackend = permissionBackend; this.groupBackend = groupBackend; this.groupIncludes = groupIncludes; this.accountCache = accountCache; @@ -125,7 +126,6 @@ public class EmailArguments { this.emailSender = emailSender; this.patchSetInfoFactory = patchSetInfoFactory; this.identifiedUserFactory = identifiedUserFactory; - this.capabilityControlFactory = capabilityControlFactory; this.changeNotesFactory = changeNotesFactory; this.anonymousUser = anonymousUser; this.anonymousCowardName = anonymousCowardName; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ProjectWatch.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ProjectWatch.java index b459d25879..91c4834b7c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ProjectWatch.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/ProjectWatch.java @@ -141,7 +141,7 @@ public class ProjectWatch { private void add(Watchers matching, NotifyConfig nc) throws OrmException, QueryParseException { for (GroupReference ref : nc.getGroups()) { - CurrentUser user = new SingleGroupUser(args.capabilityControlFactory, ref.getUUID()); + CurrentUser user = new SingleGroupUser(ref.getUUID()); if (filterMatch(user, nc.getFilter())) { deliverToMembers(matching.list(nc.getHeader()), ref.getUUID()); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java index f4ca925b2c..d338e737bb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java @@ -247,9 +247,8 @@ public class ChangeControl { return (isOwner() // owner (aka creator) of the change can abandon || getRefControl().isOwner() // branch owner can abandon || getProjectControl().isOwner() // project owner can abandon - || getUser().getCapabilities().isAdmin_DoNotUse() // site administers are god || getRefControl().canAbandon() // user can abandon a specific ref - ) + || getProjectControl().isAdmin()) && !isPatchSetLocked(db); } @@ -266,13 +265,11 @@ public class ChangeControl { switch (status) { case DRAFT: - return isOwner() - || getRefControl().canDeleteDrafts() - || getUser().getCapabilities().isAdmin_DoNotUse(); + return isOwner() || getRefControl().canDeleteDrafts() || getProjectControl().isAdmin(); case NEW: case ABANDONED: return (isOwner() && getRefControl().canDeleteOwnChanges()) - || getUser().getCapabilities().isAdmin_DoNotUse(); + || getProjectControl().isAdmin(); case MERGED: default: return false; @@ -410,7 +407,7 @@ public class ChangeControl { if (getRefControl().canRemoveReviewer() // has removal permissions || getRefControl().isOwner() // branch owner || getProjectControl().isOwner() // project owner - || getUser().getCapabilities().isAdmin_DoNotUse()) { + || getProjectControl().isAdmin()) { return true; } } @@ -424,9 +421,8 @@ public class ChangeControl { return isOwner() // owner (aka creator) of the change can edit topic || getRefControl().isOwner() // branch owner can edit topic || getProjectControl().isOwner() // project owner can edit topic - || getUser().getCapabilities().isAdmin_DoNotUse() // site administers are god || getRefControl().canEditTopicName() // user can edit topic on a specific ref - ; + || getProjectControl().isAdmin(); } return getRefControl().canForceEditTopicName(); } @@ -437,8 +433,7 @@ public class ChangeControl { return isOwner() // owner (aka creator) of the change can edit desc || getRefControl().isOwner() // branch owner can edit desc || getProjectControl().isOwner() // project owner can edit desc - || getUser().getCapabilities().isAdmin_DoNotUse() // site administers are god - ; + || getProjectControl().isAdmin(); } return false; } @@ -455,8 +450,8 @@ public class ChangeControl { return isOwner() // owner (aka creator) of the change can edit hashtags || getRefControl().isOwner() // branch owner can edit hashtags || getProjectControl().isOwner() // project owner can edit hashtags - || getUser().getCapabilities().isAdmin_DoNotUse() // site administers are god - || getRefControl().canEditHashtags(); // user can edit hashtag on a specific ref + || getRefControl().canEditHashtags() // user can edit hashtag on a specific ref + || getProjectControl().isAdmin(); } private boolean match(String destBranch, String refPattern) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/DefaultPermissionBackend.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/DefaultPermissionBackend.java index 07be8feeec..d263a2cd30 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/DefaultPermissionBackend.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/DefaultPermissionBackend.java @@ -21,6 +21,7 @@ import com.google.gerrit.extensions.api.access.GlobalOrPluginPermission; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.permissions.FailedPermissionBackend; import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; @@ -34,10 +35,12 @@ import java.util.Set; @Singleton public class DefaultPermissionBackend extends PermissionBackend { private final ProjectCache projectCache; + private final CapabilityControl.Factory capabilityFactory; @Inject - DefaultPermissionBackend(ProjectCache projectCache) { + DefaultPermissionBackend(ProjectCache projectCache, CapabilityControl.Factory capabilityFactory) { this.projectCache = projectCache; + this.capabilityFactory = capabilityFactory; } @Override @@ -47,6 +50,7 @@ public class DefaultPermissionBackend extends PermissionBackend { class WithUserImpl extends WithUser { private final CurrentUser user; + private CapabilityControl cap; WithUserImpl(CurrentUser user) { this.user = checkNotNull(user, "user"); @@ -86,7 +90,10 @@ public class DefaultPermissionBackend extends PermissionBackend { } private boolean can(GlobalOrPluginPermission perm) throws PermissionBackendException { - return user.getCapabilities().doCanForDefaultPermissionBackend(perm); + if (cap == null) { + cap = capabilityFactory.create(user); + } + return cap.doCanForDefaultPermissionBackend(perm); } } 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 d328f8778e..223073f429 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 @@ -47,6 +47,8 @@ import com.google.gerrit.server.git.VisibleRefFilter; import com.google.gerrit.server.group.SystemGroupBackend; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.permissions.FailedPermissionBackend; +import com.google.gerrit.server.permissions.GlobalPermission; +import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackend.ForChange; import com.google.gerrit.server.permissions.PermissionBackend.ForProject; import com.google.gerrit.server.permissions.PermissionBackend.ForRef; @@ -132,6 +134,7 @@ public class ProjectControl { private final Set receiveGroups; private final String canonicalWebUrl; + private final PermissionBackend.WithUser perm; private final CurrentUser user; private final ProjectState state; private final ChangeControl.Factory changeControlFactory; @@ -157,6 +160,7 @@ public class ProjectControl { VisibleRefFilter.Factory refFilter, Provider queryProvider, @CanonicalWebUrl @Nullable String canonicalWebUrl, + PermissionBackend permissionBackend, @Assisted CurrentUser who, @Assisted ProjectState ps, Metrics metrics) { @@ -169,6 +173,7 @@ public class ProjectControl { this.canonicalWebUrl = canonicalWebUrl; this.queryProvider = queryProvider; this.metrics = metrics; + this.perm = permissionBackend.user(who); user = who; state = ps; } @@ -264,8 +269,16 @@ public class ProjectControl { /** Is this user a project owner? */ public boolean isOwner() { - return (isDeclaredOwner() && !controlForRef("refs/*").isBlocked(Permission.OWNER)) - || user.getCapabilities().isAdmin_DoNotUse(); + return (isDeclaredOwner() && !controlForRef("refs/*").isBlocked(Permission.OWNER)) || isAdmin(); + } + + boolean isAdmin() { + try { + perm.check(GlobalPermission.ADMINISTRATE_SERVER); + return true; + } catch (AuthException | PermissionBackendException e) { + return false; + } } private boolean isDeclaredOwner() { @@ -278,7 +291,7 @@ public class ProjectControl { /** Does this user have ownership on at least one reference name? */ public boolean isOwnerAnyRef() { - return canPerformOnAnyRef(Permission.OWNER) || user.getCapabilities().isAdmin_DoNotUse(); + return canPerformOnAnyRef(Permission.OWNER) || isAdmin(); } /** @return true if the user can upload to at least one reference */ diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java index cf67474bff..de22e23c06 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/RefControl.java @@ -201,8 +201,7 @@ public class RefControl { // On the AllProjects project the owner access right cannot be assigned, // this why for the AllProjects project we allow administrators to push // configuration changes if they have push without being project owner. - if (!(projectControl.getProjectState().isAllProjects() - && getUser().getCapabilities().isAdmin_DoNotUse())) { + if (!(projectControl.getProjectState().isAllProjects() && projectControl.isAdmin())) { return false; } } @@ -229,8 +228,7 @@ public class RefControl { case UNKNOWN: case WEB_BROWSER: default: - return getUser().getCapabilities().isAdmin_DoNotUse() - || (isOwner() && !isForceBlocked(Permission.PUSH)); + return (isOwner() && !isForceBlocked(Permission.PUSH)) || projectControl.isAdmin(); } } @@ -376,10 +374,10 @@ public class RefControl { case UNKNOWN: case WEB_BROWSER: default: - return getUser().getCapabilities().isAdmin_DoNotUse() - || (isOwner() && !isForceBlocked(Permission.PUSH)) + return (isOwner() && !isForceBlocked(Permission.PUSH)) || canPushWithForce() - || canPerform(Permission.DELETE); + || canPerform(Permission.DELETE) + || projectControl.isAdmin(); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/QueryProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/QueryProcessor.java index 5fb44970e2..0d0777fe2c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/QueryProcessor.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/QueryProcessor.java @@ -24,6 +24,7 @@ import com.google.gerrit.metrics.Field; import com.google.gerrit.metrics.MetricMaker; import com.google.gerrit.metrics.Timer1; import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.index.Index; import com.google.gerrit.server.index.IndexCollection; import com.google.gerrit.server.index.IndexConfig; @@ -61,6 +62,7 @@ public abstract class QueryProcessor { protected final Provider userProvider; + private final CapabilityControl.Factory capabilityFactory; private final Metrics metrics; private final SchemaDefinitions schemaDef; private final IndexConfig indexConfig; @@ -76,6 +78,7 @@ public abstract class QueryProcessor { protected QueryProcessor( Provider userProvider, + CapabilityControl.Factory capabilityFactory, Metrics metrics, SchemaDefinitions schemaDef, IndexConfig indexConfig, @@ -83,6 +86,7 @@ public abstract class QueryProcessor { IndexRewriter rewriter, String limitField) { this.userProvider = userProvider; + this.capabilityFactory = capabilityFactory; this.metrics = metrics; this.schemaDef = schemaDef; this.indexConfig = indexConfig; @@ -231,7 +235,10 @@ public abstract class QueryProcessor { private int getPermittedLimit() { if (enforceVisibility) { - return userProvider.get().getCapabilities().getRange(GlobalCapability.QUERY_LIMIT).getMax(); + return capabilityFactory + .create(userProvider.get()) + .getRange(GlobalCapability.QUERY_LIMIT) + .getMax(); } return Integer.MAX_VALUE; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java index d984e6dd1a..6a9b37c919 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java @@ -20,6 +20,7 @@ import static com.google.gerrit.server.query.account.AccountQueryBuilder.FIELD_L import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.account.AccountControl; import com.google.gerrit.server.account.AccountState; +import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.index.IndexConfig; import com.google.gerrit.server.index.IndexPredicate; import com.google.gerrit.server.index.account.AccountIndexCollection; @@ -44,6 +45,7 @@ public class AccountQueryProcessor extends QueryProcessor { @Inject protected AccountQueryProcessor( Provider userProvider, + CapabilityControl.Factory capabilityFactory, Metrics metrics, IndexConfig indexConfig, AccountIndexCollection indexes, @@ -51,6 +53,7 @@ public class AccountQueryProcessor extends QueryProcessor { AccountControl.Factory accountControlFactory) { super( userProvider, + capabilityFactory, metrics, AccountSchemaDefinitions.INSTANCE, indexConfig, diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java index 19fcbe3572..6c6179bacf 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java @@ -41,7 +41,6 @@ import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.StarredChangesUtil; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.AccountResolver; -import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.account.GroupBackend; import com.google.gerrit.server.account.GroupBackends; import com.google.gerrit.server.account.VersionedAccountDestinations; @@ -196,7 +195,6 @@ public class ChangeQueryBuilder extends QueryBuilder { final AllProjectsName allProjectsName; final AllUsersName allUsersName; final PermissionBackend permissionBackend; - final CapabilityControl.Factory capabilityControlFactory; final ChangeControl.GenericFactory changeControlGenericFactory; final ChangeData.Factory changeDataFactory; final ChangeIndex index; @@ -236,7 +234,6 @@ public class ChangeQueryBuilder extends QueryBuilder { IdentifiedUser.GenericFactory userFactory, Provider self, PermissionBackend permissionBackend, - CapabilityControl.Factory capabilityControlFactory, ChangeControl.GenericFactory changeControlGenericFactory, ChangeNotes.Factory notesFactory, ChangeData.Factory changeDataFactory, @@ -269,7 +266,6 @@ public class ChangeQueryBuilder extends QueryBuilder { userFactory, self, permissionBackend, - capabilityControlFactory, changeControlGenericFactory, notesFactory, changeDataFactory, @@ -304,7 +300,6 @@ public class ChangeQueryBuilder extends QueryBuilder { IdentifiedUser.GenericFactory userFactory, Provider self, PermissionBackend permissionBackend, - CapabilityControl.Factory capabilityControlFactory, ChangeControl.GenericFactory changeControlGenericFactory, ChangeNotes.Factory notesFactory, ChangeData.Factory changeDataFactory, @@ -335,7 +330,6 @@ public class ChangeQueryBuilder extends QueryBuilder { this.userFactory = userFactory; this.self = self; this.permissionBackend = permissionBackend; - this.capabilityControlFactory = capabilityControlFactory; this.notesFactory = notesFactory; this.changeControlGenericFactory = changeControlGenericFactory; this.changeDataFactory = changeDataFactory; @@ -372,7 +366,6 @@ public class ChangeQueryBuilder extends QueryBuilder { userFactory, Providers.of(otherUser), permissionBackend, - capabilityControlFactory, changeControlGenericFactory, notesFactory, changeDataFactory, @@ -933,7 +926,7 @@ public class ChangeQueryBuilder extends QueryBuilder { for (GroupReference ref : suggestions) { ids.add(ref.getUUID()); } - return visibleto(new SingleGroupUser(args.capabilityControlFactory, ids)); + return visibleto(new SingleGroupUser(ids)); } throw error("No user or group matches \"" + who + "\"."); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java index efe44fab62..d8c872d6c8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java @@ -21,6 +21,7 @@ import com.google.gerrit.extensions.common.PluginDefinedInfo; import com.google.gerrit.extensions.registration.DynamicMap; import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.index.IndexConfig; import com.google.gerrit.server.index.IndexPredicate; import com.google.gerrit.server.index.QueryOptions; @@ -65,6 +66,7 @@ public class ChangeQueryProcessor extends QueryProcessor @Inject ChangeQueryProcessor( Provider userProvider, + CapabilityControl.Factory capabilityFactory, Metrics metrics, IndexConfig indexConfig, ChangeIndexCollection indexes, @@ -75,6 +77,7 @@ public class ChangeQueryProcessor extends QueryProcessor DynamicMap attributeFactories) { super( userProvider, + capabilityFactory, metrics, ChangeSchemaDefinitions.INSTANCE, indexConfig, diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/SingleGroupUser.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/SingleGroupUser.java index 2661b8b6f2..a084b35b20 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/SingleGroupUser.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/SingleGroupUser.java @@ -14,25 +14,21 @@ package com.google.gerrit.server.query.change; +import com.google.common.collect.ImmutableSet; import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.server.CurrentUser; -import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.account.GroupMembership; import com.google.gerrit.server.account.ListGroupMembership; -import java.util.Collections; import java.util.Set; public final class SingleGroupUser extends CurrentUser { private final GroupMembership groups; - public SingleGroupUser( - CapabilityControl.Factory capabilityControlFactory, AccountGroup.UUID groupId) { - this(capabilityControlFactory, Collections.singleton(groupId)); + public SingleGroupUser(AccountGroup.UUID groupId) { + this(ImmutableSet.of(groupId)); } - public SingleGroupUser( - CapabilityControl.Factory capabilityControlFactory, Set groups) { - super(capabilityControlFactory); + public SingleGroupUser(Set groups) { this.groups = new ListGroupMembership(groups); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java index 1cfab202b9..6229f18ba3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java @@ -19,6 +19,7 @@ import static com.google.gerrit.server.query.group.GroupQueryBuilder.FIELD_LIMIT import com.google.gerrit.reviewdb.client.AccountGroup; import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.account.GroupControl; import com.google.gerrit.server.index.IndexConfig; import com.google.gerrit.server.index.IndexPredicate; @@ -44,6 +45,7 @@ public class GroupQueryProcessor extends QueryProcessor { @Inject protected GroupQueryProcessor( Provider userProvider, + CapabilityControl.Factory capabilityFactory, Metrics metrics, IndexConfig indexConfig, GroupIndexCollection indexes, @@ -51,6 +53,7 @@ public class GroupQueryProcessor extends QueryProcessor { GroupControl.GenericFactory groupControlFactory) { super( userProvider, + capabilityFactory, metrics, GroupSchemaDefinitions.INSTANCE, indexConfig, diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/IdentifiedUserTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/IdentifiedUserTest.java index 468968800a..b32bdc6424 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/IdentifiedUserTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/IdentifiedUserTest.java @@ -20,7 +20,6 @@ import static com.google.inject.Scopes.SINGLETON; import com.google.gerrit.common.TimeUtil; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.server.account.AccountCache; -import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.account.FakeRealm; import com.google.gerrit.server.account.GroupBackend; import com.google.gerrit.server.account.Realm; @@ -36,7 +35,6 @@ import com.google.inject.AbstractModule; import com.google.inject.Guice; import com.google.inject.Inject; import com.google.inject.Injector; -import com.google.inject.util.Providers; import java.util.Arrays; import java.util.HashSet; import java.util.Set; @@ -93,8 +91,6 @@ public class IdentifiedUserTest { .toInstance("http://localhost:8080/"); bind(AccountCache.class).toInstance(accountCache); bind(GroupBackend.class).to(SystemGroupBackend.class).in(SINGLETON); - bind(CapabilityControl.Factory.class) - .toProvider(Providers.of(null)); bind(Realm.class).toInstance(mockRealm); } }; diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/index/change/FakeQueryBuilder.java b/gerrit-server/src/test/java/com/google/gerrit/server/index/change/FakeQueryBuilder.java index 3bbd335dd2..6fda1005bd 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/index/change/FakeQueryBuilder.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/index/change/FakeQueryBuilder.java @@ -27,8 +27,8 @@ public class FakeQueryBuilder extends ChangeQueryBuilder { new FakeQueryBuilder.Definition<>(FakeQueryBuilder.class), new ChangeQueryBuilder.Arguments( null, null, null, null, null, null, null, null, null, null, null, null, null, null, - null, null, null, null, null, null, null, null, indexes, null, null, null, null, null, - null, null, null, null)); + null, null, null, null, null, null, null, indexes, null, null, null, null, null, null, + null, null, null)); } @Operator diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java index 86cce091fe..f59063add8 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java @@ -35,7 +35,6 @@ import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.InternalUser; import com.google.gerrit.server.account.AccountCache; -import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.account.FakeRealm; import com.google.gerrit.server.account.GroupBackend; import com.google.gerrit.server.account.Realm; @@ -158,8 +157,6 @@ public abstract class AbstractChangeNotesTest extends GerritBaseTests { bind(NotesMigration.class).toInstance(MIGRATION); bind(GitRepositoryManager.class).toInstance(repoManager); bind(ProjectCache.class).toProvider(Providers.of(null)); - bind(CapabilityControl.Factory.class) - .toProvider(Providers.of(null)); bind(Config.class).annotatedWith(GerritServerConfig.class).toInstance(testConfig); bind(String.class) .annotatedWith(AnonymousCowardName.class) @@ -199,7 +196,7 @@ public abstract class AbstractChangeNotesTest extends GerritBaseTests { changeOwner = userFactory.create(co.getId()); otherUser = userFactory.create(ou.getId()); otherUserId = otherUser.getAccountId(); - internalUser = new InternalUser(null); + internalUser = new InternalUser(); } private void setTimeForTesting() { 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 51bff6c42b..9b9cfff100 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 @@ -48,7 +48,6 @@ import com.google.gerrit.rules.PrologEnvironment; import com.google.gerrit.rules.RulesCache; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.account.CapabilityCollection; -import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.account.GroupMembership; import com.google.gerrit.server.account.ListGroupMembership; import com.google.gerrit.server.config.AllProjectsName; @@ -58,6 +57,7 @@ import com.google.gerrit.server.config.AllUsersNameProvider; import com.google.gerrit.server.config.SitePaths; import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.index.SingleVersionModule.SingleVersionListener; +import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.ProjectPermission; import com.google.gerrit.server.permissions.RefPermission; import com.google.gerrit.server.query.change.InternalChangeQuery; @@ -206,8 +206,8 @@ public class RefControlTest { private ChangeControl.Factory changeControlFactory; private ReviewDb db; + @Inject private PermissionBackend permissionBackend; @Inject private CapabilityCollection.Factory capabilityCollectionFactory; - @Inject private CapabilityControl.Factory capabilityControlFactory; @Inject private SchemaCreator schemaCreator; @Inject private SingleVersionListener singleVersionListener; @Inject private InMemoryDatabase schemaFactory; @@ -910,6 +910,7 @@ public class RefControlTest { null, // refFilter queryProvider, canonicalWebUrl, + permissionBackend, new MockUser(name, memberOf), newProjectState(local), metrics); @@ -925,7 +926,6 @@ public class RefControlTest { private final GroupMembership groups; MockUser(String name, AccountGroup.UUID[] groupId) { - super(capabilityControlFactory); username = name; ArrayList groupIds = Lists.newArrayList(groupId); groupIds.add(REGISTERED_USERS); diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/CommandExecutorProvider.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/CommandExecutorProvider.java index c807acf1ae..5bc59e944d 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/CommandExecutorProvider.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/CommandExecutorProvider.java @@ -15,24 +15,27 @@ package com.google.gerrit.sshd; import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.git.QueueProvider; import com.google.inject.Inject; import com.google.inject.Provider; import java.util.concurrent.ScheduledThreadPoolExecutor; class CommandExecutorProvider implements Provider { - + private final CapabilityControl.Factory capabilityFactory; private final QueueProvider queues; private final CurrentUser user; @Inject - CommandExecutorProvider(QueueProvider queues, CurrentUser user) { + CommandExecutorProvider( + CapabilityControl.Factory capabilityFactory, QueueProvider queues, CurrentUser user) { + this.capabilityFactory = capabilityFactory; this.queues = queues; this.user = user; } @Override public ScheduledThreadPoolExecutor get() { - return queues.getQueue(user.getCapabilities().getQueueType()); + return queues.getQueue(capabilityFactory.create(user).getQueueType()); } } diff --git a/plugins/replication b/plugins/replication index 2a4d0bfe10..1085b45303 160000 --- a/plugins/replication +++ b/plugins/replication @@ -1 +1 @@ -Subproject commit 2a4d0bfe10c63c79ca0d47be21756377703e46c0 +Subproject commit 1085b453039b0fe230c3584e0024a7965cc1e323