commit a3196c7a71e958c506fb027984a7e9931f829349 Author: Patrick Hiesel Date: Wed Oct 7 13:53:23 2020 +0200 CurrentUser: Factor out PropertyMap and make it immutable CurrentUser has functionality for extensions and plugins to contribute state - that is properties unknown to Gerrit core - and retrieve them in a type-safe fashion. This is used to add the last external ID key that the user used to log into Gerrit from WebSession. It's also used on googlesource.com to add an internal representation of the login information in a similar fashion. CurrentUser and it's subclasses serve two purposes: 1) They are instantiated as per-request state scoped in Guice In this case, CurrentUser represents the requestor either as IdentifiedUser or AnonymousUser.. 2) They can be instantiated for any user to be used in internal APIs and for checking permissions of that absent user. We want to restrict the option to add per-request state to (1) and even there ensure that no per-request state is ever added to AnonymousUser or future subclasses of CurrentUser. This is relevant because we need to prevent that login info, such as a session is added to a potentially shared AnonnymousUser instance OR to objects constructed outside Guice's request scope initializer. At the same time, we want to allow scoped properties not only on IdentifiedUser, but also on SingleGroupUser. The best option to to all that is to restrict property creation to the instantiation of CurrentUser and disallow any dynamic mutations, which is what this commit does. While at it, we remove the IS_ADMIN property because it's superflous optimization. That bit is already cached inside WithUser. Change-Id: I547893c852668b0970455ec5e4591807a78448e8 diff --git a/java/com/google/gerrit/httpd/CacheBasedWebSession.java b/java/com/google/gerrit/httpd/CacheBasedWebSession.java index 5c4830c..fcd2187 100644 --- a/java/com/google/gerrit/httpd/CacheBasedWebSession.java +++ b/java/com/google/gerrit/httpd/CacheBasedWebSession.java @@ -28,6 +28,7 @@ import com.google.gerrit.server.AccessPath; import com.google.gerrit.server.AnonymousUser; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.PropertyMap; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.AuthResult; import com.google.gerrit.server.account.externalids.ExternalId; @@ -161,15 +162,11 @@ public abstract class CacheBasedWebSession implements WebSession { } @Override - public ExternalId.Key getLastLoginExternalId() { - return val != null ? val.getExternalId() : null; - } - - @Override public CurrentUser getUser() { if (user == null) { if (isSignedIn()) { - user = identified.create(val.getAccountId()); + + user = identified.create(val.getAccountId(), getUserProperties(val)); } else { user = anonymousProvider.get(); } @@ -177,6 +174,15 @@ public abstract class CacheBasedWebSession implements WebSession { return user; } + private static PropertyMap getUserProperties(@Nullable WebSessionManager.Val val) { + if (val == null || val.getExternalId() == null) { + return PropertyMap.EMPTY; + } + return PropertyMap.builder() + .put(CurrentUser.LAST_LOGIN_EXTERNAL_ID_PROPERTY_KEY, val.getExternalId()) + .build(); + } + @Override public void login(AuthResult res, boolean rememberMe) { Account.Id id = res.getAccountId(); @@ -194,7 +200,7 @@ public abstract class CacheBasedWebSession implements WebSession { key = manager.createKey(id); val = manager.createVal(key, id, rememberMe, identity, null, null); saveCookie(); - user = identified.create(val.getAccountId()); + user = identified.create(val.getAccountId(), getUserProperties(val)); } /** Set the user account for this current request only. */ diff --git a/java/com/google/gerrit/httpd/WebSession.java b/java/com/google/gerrit/httpd/WebSession.java index e8b54fe..daf30ff 100644 --- a/java/com/google/gerrit/httpd/WebSession.java +++ b/java/com/google/gerrit/httpd/WebSession.java @@ -19,7 +19,6 @@ import com.google.gerrit.entities.Account; import com.google.gerrit.server.AccessPath; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.account.AuthResult; -import com.google.gerrit.server.account.externalids.ExternalId; public interface WebSession { boolean isSignedIn(); @@ -29,8 +28,6 @@ public interface WebSession { boolean isValidXGerritAuth(String keyIn); - ExternalId.Key getLastLoginExternalId(); - CurrentUser getUser(); void login(AuthResult res, boolean rememberMe); diff --git a/java/com/google/gerrit/httpd/auth/container/HttpAuthFilter.java b/java/com/google/gerrit/httpd/auth/container/HttpAuthFilter.java index 509a9f1..e20c9b9 100644 --- a/java/com/google/gerrit/httpd/auth/container/HttpAuthFilter.java +++ b/java/com/google/gerrit/httpd/auth/container/HttpAuthFilter.java @@ -35,6 +35,7 @@ import java.io.FileNotFoundException; import java.io.IOException; import java.io.OutputStream; import java.util.Locale; +import java.util.Optional; import javax.servlet.Filter; import javax.servlet.FilterChain; import javax.servlet.FilterConfig; @@ -124,8 +125,8 @@ class HttpAuthFilter implements Filter { } private static boolean correctUser(String user, WebSession session) { - ExternalId.Key id = session.getLastLoginExternalId(); - return id != null && id.equals(ExternalId.Key.create(SCHEME_GERRIT, user)); + Optional id = session.getUser().getLastLoginExternalIdKey(); + return id.map(i -> i.equals(ExternalId.Key.create(SCHEME_GERRIT, user))).orElse(false); } String getRemoteUser(HttpServletRequest req) { diff --git a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java index 665cc33..a1197ef 100644 --- a/java/com/google/gerrit/httpd/restapi/RestApiServlet.java +++ b/java/com/google/gerrit/httpd/restapi/RestApiServlet.java @@ -1639,9 +1639,6 @@ public class RestApiServlet extends HttpServlet { "Invalid authentication method. In order to authenticate, " + "prefix the REST endpoint URL with /a/ (e.g. http://example.com/a/projects/)."); } - if (user.isIdentifiedUser()) { - user.setLastLoginExternalIdKey(globals.webSession.get().getLastLoginExternalId()); - } } private List getParameterNames(HttpServletRequest req) { diff --git a/java/com/google/gerrit/server/CurrentUser.java b/java/com/google/gerrit/server/CurrentUser.java index 1955340..43d3c7b 100644 --- a/java/com/google/gerrit/server/CurrentUser.java +++ b/java/com/google/gerrit/server/CurrentUser.java @@ -14,7 +14,6 @@ package com.google.gerrit.server; -import com.google.gerrit.common.Nullable; import com.google.gerrit.entities.Account; import com.google.gerrit.server.account.GroupMembership; import com.google.gerrit.server.account.externalids.ExternalId; @@ -31,17 +30,19 @@ import java.util.function.Consumer; * @see IdentifiedUser */ public abstract class CurrentUser { - /** Unique key for plugin/extension specific data on a CurrentUser. */ - public static final class PropertyKey { - public static PropertyKey create() { - return new PropertyKey<>(); - } + public static final PropertyMap.Key LAST_LOGIN_EXTERNAL_ID_PROPERTY_KEY = + PropertyMap.key(); + + private final PropertyMap properties; + private AccessPath accessPath = AccessPath.UNKNOWN; - private PropertyKey() {} + protected CurrentUser() { + this.properties = PropertyMap.EMPTY; } - private AccessPath accessPath = AccessPath.UNKNOWN; - private PropertyKey lastLoginExternalIdPropertyKey = PropertyKey.create(); + protected CurrentUser(PropertyMap properties) { + this.properties = properties; + } /** How this user is accessing the Gerrit Code Review application. */ public final AccessPath getAccessPath() { @@ -127,29 +128,18 @@ public abstract class CurrentUser { } /** - * Lookup a previously stored property. - * - * @param key unique property key. - * @return previously stored value, or {@code Optional#empty()}. - */ - public Optional get(PropertyKey key) { - return Optional.empty(); - } - - /** - * Store a property for later retrieval. + * Lookup a stored property. * - * @param key unique property key. - * @param value value to store; or {@code null} to clear the value. + * @param key unique property key. This key has to be the same instance that was used to store the + * value when constructing the {@link PropertyMap} + * @return stored value, or {@code Optional#empty()}. */ - public void put(PropertyKey key, @Nullable T value) {} - - public void setLastLoginExternalIdKey(ExternalId.Key externalIdKey) { - put(lastLoginExternalIdPropertyKey, externalIdKey); + public Optional get(PropertyMap.Key key) { + return properties.get(key); } public Optional getLastLoginExternalIdKey() { - return get(lastLoginExternalIdPropertyKey); + return get(LAST_LOGIN_EXTERNAL_ID_PROPERTY_KEY); } /** diff --git a/java/com/google/gerrit/server/IdentifiedUser.java b/java/com/google/gerrit/server/IdentifiedUser.java index 14d74d0..c12685a 100644 --- a/java/com/google/gerrit/server/IdentifiedUser.java +++ b/java/com/google/gerrit/server/IdentifiedUser.java @@ -46,8 +46,6 @@ import java.net.MalformedURLException; import java.net.SocketAddress; import java.net.URL; import java.util.Date; -import java.util.HashMap; -import java.util.Map; import java.util.Optional; import java.util.Set; import java.util.TimeZone; @@ -121,7 +119,8 @@ public class IdentifiedUser extends CurrentUser { enableReverseDnsLookup, Providers.of(remotePeer), id, - caller); + caller, + PropertyMap.EMPTY); } } @@ -163,6 +162,10 @@ public class IdentifiedUser extends CurrentUser { } public IdentifiedUser create(Account.Id id) { + return create(id, PropertyMap.EMPTY); + } + + public IdentifiedUser create(Account.Id id, PropertyMap properties) { return new IdentifiedUser( authConfig, realm, @@ -173,7 +176,8 @@ public class IdentifiedUser extends CurrentUser { enableReverseDnsLookup, remotePeerProvider, id, - null); + null, + properties); } public IdentifiedUser runAs(Account.Id id, CurrentUser caller) { @@ -187,7 +191,8 @@ public class IdentifiedUser extends CurrentUser { enableReverseDnsLookup, remotePeerProvider, id, - caller); + caller, + PropertyMap.EMPTY); } } @@ -212,7 +217,6 @@ public class IdentifiedUser extends CurrentUser { private boolean loadedAllEmails; private Set invalidEmails; private GroupMembership effectiveGroups; - private Map, Object> properties; private IdentifiedUser( AuthConfig authConfig, @@ -235,7 +239,8 @@ public class IdentifiedUser extends CurrentUser { enableReverseDnsLookup, remotePeerProvider, state.account().id(), - realUser); + realUser, + PropertyMap.EMPTY); this.state = state; } @@ -249,7 +254,9 @@ public class IdentifiedUser extends CurrentUser { Boolean enableReverseDnsLookup, @Nullable Provider remotePeerProvider, Account.Id id, - @Nullable CurrentUser realUser) { + @Nullable CurrentUser realUser, + PropertyMap properties) { + super(properties); this.canonicalUrl = canonicalUrl; this.accountCache = accountCache; this.groupBackend = groupBackend; @@ -458,40 +465,6 @@ public class IdentifiedUser extends CurrentUser { return true; } - @Override - public synchronized Optional get(PropertyKey key) { - if (properties != null) { - @SuppressWarnings("unchecked") - T value = (T) properties.get(key); - return Optional.ofNullable(value); - } - return Optional.empty(); - } - - /** - * Store a property for later retrieval. - * - * @param key unique property key. - * @param value value to store; or {@code null} to clear the value. - */ - @Override - public synchronized void put(PropertyKey key, @Nullable T value) { - if (properties == null) { - if (value == null) { - return; - } - properties = new HashMap<>(); - } - - @SuppressWarnings("unchecked") - PropertyKey k = (PropertyKey) key; - if (value != null) { - properties.put(k, value); - } else { - properties.remove(k); - } - } - /** * Returns a materialized copy of the user with all dependencies. * diff --git a/java/com/google/gerrit/server/PropertyMap.java b/java/com/google/gerrit/server/PropertyMap.java new file mode 100644 index 0000000..da3a249 --- /dev/null +++ b/java/com/google/gerrit/server/PropertyMap.java @@ -0,0 +1,84 @@ +// Copyright (C) 2020 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; + +import com.google.common.collect.ImmutableMap; +import java.util.Optional; + +/** + * Immutable map that holds a collection of random objects allowing for a type-safe retrieval. + * + *

Intended to be used in {@link CurrentUser} when the object is constructed during login and + * holds per-request state. This functionality allows plugins/extensions to contribute specific data + * to {@link CurrentUser} that is unknown to Gerrit core. + */ +public class PropertyMap { + /** Empty instance to be referenced once per JVM. */ + public static final PropertyMap EMPTY = builder().build(); + + /** + * Typed key for {@link PropertyMap}. This class intentionally does not implement {@link + * Object#equals(Object)} and {@link Object#hashCode()} so that the same instance has to be used + * to retrieve a stored value. + * + *

We require the exact same key instance because {@link PropertyMap} is implemented in a + * type-safe fashion by using Java generics to guarantee the return type. The generic type can't + * be recovered at runtime, so there is no way to just use the type's full name as key - we'd have + * to pass additional arguments. At the same time, this is in-line with how we'd want callers to + * use {@link PropertyMap}: Instantiate a static, per-JVM key that is reused when setting and + * getting values. + */ + public static class Key {} + + public static Key key() { + return new Key<>(); + } + + public static class Builder { + private ImmutableMap.Builder mutableMap; + + private Builder() { + this.mutableMap = ImmutableMap.builder(); + } + + /** Adds the provided {@code value} to the {@link PropertyMap} that is being built. */ + public Builder put(Key key, T value) { + mutableMap.put(key, value); + return this; + } + + /** Builds and returns an immutable {@link PropertyMap}. */ + public PropertyMap build() { + return new PropertyMap(mutableMap.build()); + } + } + + private final ImmutableMap map; + + private PropertyMap(ImmutableMap map) { + this.map = map; + } + + /** Returns a new {@link Builder} instance. */ + public static Builder builder() { + return new Builder(); + } + + /** Returns the requested value wrapped as {@link Optional}. */ + @SuppressWarnings("unchecked") + public Optional get(Key key) { + return Optional.ofNullable((T) map.get(key)); + } +} diff --git a/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java b/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java index 1b029b1..49df653 100644 --- a/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java +++ b/java/com/google/gerrit/server/permissions/DefaultPermissionBackend.java @@ -41,15 +41,12 @@ import com.google.inject.Provider; import com.google.inject.Singleton; import java.util.Collection; import java.util.List; -import java.util.Optional; import java.util.Set; @Singleton public class DefaultPermissionBackend extends PermissionBackend { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); - private static final CurrentUser.PropertyKey IS_ADMIN = CurrentUser.PropertyKey.create(); - private final Provider currentUser; private final ProjectCache projectCache; private final ProjectControl.Factory projectControlFactory; @@ -197,21 +194,13 @@ public class DefaultPermissionBackend extends PermissionBackend { } private Boolean computeAdmin() { - Optional r = user.get(IS_ADMIN); - if (r.isPresent()) { - return r.get(); - } - - boolean isAdmin; if (user.isImpersonating()) { - isAdmin = false; - } else if (user instanceof PeerDaemonUser) { - isAdmin = true; - } else { - isAdmin = allow(capabilities().administrateServer); + return false; + } + if (user instanceof PeerDaemonUser) { + return true; } - user.put(IS_ADMIN, isAdmin); - return isAdmin; + return allow(capabilities().administrateServer); } private boolean canEmailReviewers() { diff --git a/javatests/com/google/gerrit/acceptance/testsuite/request/RequestScopeOperationsImplTest.java b/javatests/com/google/gerrit/acceptance/testsuite/request/RequestScopeOperationsImplTest.java index f6421a5..48fd38c 100644 --- a/javatests/com/google/gerrit/acceptance/testsuite/request/RequestScopeOperationsImplTest.java +++ b/javatests/com/google/gerrit/acceptance/testsuite/request/RequestScopeOperationsImplTest.java @@ -16,7 +16,6 @@ package com.google.gerrit.acceptance.testsuite.request; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; -import static com.google.common.truth.Truth8.assertThat; import static com.google.gerrit.testing.GerritJUnit.assertThrows; import com.google.common.collect.ImmutableSet; @@ -29,7 +28,6 @@ import com.google.gerrit.entities.Account; import com.google.gerrit.extensions.common.ChangeInput; import com.google.gerrit.server.AnonymousUser; import com.google.gerrit.server.CurrentUser; -import com.google.gerrit.server.CurrentUser.PropertyKey; import com.google.gerrit.server.notedb.Sequences; import com.google.inject.Inject; import com.google.inject.Provider; @@ -75,19 +73,6 @@ public class RequestScopeOperationsImplTest extends AbstractDaemonTest { } @Test - public void resetCurrentApiUserClearsCachedState() throws Exception { - requestScopeOperations.setApiUser(user.id()); - PropertyKey key = PropertyKey.create(); - atrScope.get().getUser().put(key, "foo"); - assertThat(atrScope.get().getUser().get(key)).hasValue("foo"); - - AcceptanceTestRequestScope.Context oldCtx = requestScopeOperations.resetCurrentApiUser(); - checkCurrentUser(user.id()); - assertThat(atrScope.get().getUser().get(key)).isEmpty(); - assertThat(oldCtx.getUser().get(key)).hasValue("foo"); - } - - @Test public void setApiUserAnonymousSetsAnonymousUser() throws Exception { fastCheckCurrentUser(admin.id()); requestScopeOperations.setApiUserAnonymous();