Polishing

Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>

Author: nibix

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

@@ -1119,7 +1119,7 @@ public Collection<Object> createComponents(
             interClusterRequestEvaluator = ReflectionHelper.instantiateInterClusterRequestEvaluator(className, settings);
         }
 
-        UserFactory userFactory = new UserFactory.Caching();
+        UserFactory userFactory = new UserFactory.Caching(settings);
 
         final PrivilegesInterceptor privilegesInterceptor;
 
@@ -2146,6 +2146,10 @@ public List<Setting<?>> getSettings() {
                     Property.Filtered
                 )
             );
+
+            settings.add(UserFactory.Caching.MAX_SIZE);
+            settings.add(UserFactory.Caching.MAX_ENTRIES);
+            settings.add(UserFactory.Caching.EXPIRE_AFTER_ACCESS);
         }
 
         return settings;

src/main/java/org/opensearch/security/auth/AuthorizationBackend.java

@@ -54,13 +54,12 @@ public interface AuthorizationBackend {
     String getType();
 
     /**
-     * Populate a {@link User} with backend roles. This method will not be called for cached users.
-     * <p/>
-     * Add them by calling either {@code user.addRole()} or {@code user.addRoles()}
-     * </P>
+     * This method is called during the auth phase to add additional backend roles to a  {@link User}. This method will not be called for cached users.
+     * <p>
+     * Implementations must use the withRoles() method and return the newly created User object.
+     *
      * @param user The authenticated user to populate with backend roles, never null
      * @param context Context data specific to the request that is currently processed.
-     * <em>This parameter is for future usage, currently always empty credentials are passed!</em>
      * @throws OpenSearchSecurityException in case when the authorization backend cannot be reached
      * or the {@code credentials} are insufficient to authenticate to the authorization backend.
      */

src/main/java/org/opensearch/security/auth/ImpersonationBackend.java

@@ -15,6 +15,9 @@
 
 import org.opensearch.security.user.User;
 
+/**
+ * If an authentication backend class implements this interface, the auth type can be used for impersonation.
+ */
 public interface ImpersonationBackend {
     /**
      * The type (name) of the impersonation backend. Only for logging.

src/main/java/org/opensearch/security/auth/internal/NoOpAuthenticationBackend.java

@@ -32,7 +32,6 @@
 import org.opensearch.common.settings.Settings;
 import org.opensearch.security.auth.AuthenticationBackend;
 import org.opensearch.security.auth.AuthenticationContext;
-import org.opensearch.security.auth.AuthenticationContext;
 import org.opensearch.security.auth.ImpersonationBackend;
 import org.opensearch.security.user.AuthCredentials;
 import org.opensearch.security.user.User;

src/main/java/org/opensearch/security/user/User.java

@@ -43,12 +43,16 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
 
+import org.opensearch.OpenSearchException;
 import org.opensearch.security.support.Base64Helper;
 
 /**
- * A authenticated user and attributes associated to them (like roles, tenant, custom attributes)
- * <p/>
- * <b>Do not subclass from this class!</b>
+ * An authenticated user and attributes associated to them (like roles, tenant, custom attributes).
+ * <p>
+ * Objects of this class are immutable. Any method that modifies an attribute, returns a modified copy of this object.
+ * As the objects are immutable, all operations on the objects are inherently thread-safe. No external synchronization is required.
+ * <p>
+ * <b>Do not subclass from this class; do not add attributes that can be modified using publicly visible methods!</b>
  *
  */
 public class User implements Serializable, CustomAttributesAware {
@@ -66,8 +70,12 @@ public class User implements Serializable, CustomAttributesAware {
 
     /**
      * Deserializes the given serialized from of a user object and returns the actual user object.
-     *
+     * <p>
      * Note: Instead of using this method, prefer to use UserFactory.Caching to benefit from already parsed user objects.
+     *
+     * @param serializedBase64 a string with a serialized form of a User object
+     * @return A User object. Never returns null.
+     * @throws OpenSearchException in case the provided string could not be processed.
      */
     public static User fromSerializedBase64(String serializedBase64) {
         User user = (User) Base64Helper.deserializeObject(serializedBase64);
@@ -86,26 +94,13 @@ public static User fromSerializedBase64(String serializedBase64) {
     private final String requestedTenant;
     private final ImmutableMap<String, String> attributes;
     private final boolean isInjected;
-    private volatile transient String serializedBase64;
+    private final transient int estimatedByteSize;
 
     /**
-     * Create a new authenticated user
-     *
-     * @param name The username (must not be null or empty)
-     * @param roles Roles of which the user is a member off (maybe null)
-     * @param customAttributes Custom attributes associated with this (maybe null)
-     * @throws IllegalArgumentException if name is null or empty
+     * This attribute caches the serialized form of the User object. As the User object is immutable,
+     * this value can be re-used when it is set.
      */
-    public User(final String name, final Collection<String> roles, final AuthCredentials customAttributes) {
-        this(
-            name,
-            ImmutableSet.copyOf(roles),
-            ImmutableSet.of(),
-            null,
-            customAttributes != null ? ImmutableMap.copyOf(customAttributes.getAttributes()) : ImmutableMap.of(),
-            false
-        );
-    }
+    private volatile transient String serializedBase64;
 
     /**
      * Create a new authenticated user without roles and attributes
@@ -114,9 +109,19 @@ public User(final String name, final Collection<String> roles, final AuthCredent
      * @throws IllegalArgumentException if name is null or empty
      */
     public User(final String name) {
-        this(name, ImmutableSet.of(), null);
+        this(name, ImmutableSet.of(), ImmutableSet.of(), null, ImmutableMap.of(), false);
     }
 
+    /**
+     * Creates a new User object. This is the main constructor, prefer using this one.
+     *
+     * @param name The username; must not be null or an empty string.
+     * @param roles The backend roles of a user. Must not be null. For empty roles, pass ImmutableSet.of().
+     * @param securityRoles The security roles of a user. Must not be null. For empty roles, pass ImmutableSet.of().
+     * @param requestedTenant The requested tenant property of the user. May be null.
+     * @param attributes The user attributes. Must not be null. For no attributes, pass ImmutableMap.of()
+     * @param isInjected A flag that indicates whether the user was injected.
+     */
     public User(
         String name,
         ImmutableSet<String> roles,
@@ -135,6 +140,7 @@ public User(
         this.requestedTenant = requestedTenant;
         this.attributes = Objects.requireNonNull(attributes);
         this.isInjected = isInjected;
+        this.estimatedByteSize = calcEstimatedByteSize();
     }
 
     public final String getName() {
@@ -150,9 +156,7 @@ public ImmutableSet<String> getRoles() {
     }
 
     /**
-     * Associate this user with a backend role
-     *
-     * @param role The backend role
+     * Returns a new User object that additionally contains the provided backend role.
      */
     public User withRole(String role) {
         return new User(
@@ -166,9 +170,7 @@ public User withRole(String role) {
     }
 
     /**
-     * Associate this user with a set of backend roles
-     *
-     * @param roles The backend roles
+     * Returns a new User object that additionally contains the provided backend roles.
      */
     public User withRoles(Collection<String> roles) {
         if (roles == null || roles.isEmpty()) {
@@ -186,9 +188,7 @@ public User withRoles(Collection<String> roles) {
     }
 
     /**
-     * Associate this user with a set of custom attributes
-     *
-     * @param attributes custom attributes
+     * Returns a new User object that additionally contains the provided map of custom attributes
      */
     public User withAttributes(Map<String, String> attributes) {
         if (attributes == null || attributes.isEmpty()) {
@@ -209,6 +209,9 @@ public final String getRequestedTenant() {
         return requestedTenant;
     }
 
+    /**
+     * Returns a new User object with the requestedTenant attribute set to the supplied value.
+     */
     public User withRequestedTenant(String requestedTenant) {
         if (Objects.equals(requestedTenant, this.requestedTenant)) {
             return this;
@@ -277,6 +280,9 @@ public ImmutableMap<String, String> getCustomAttributesMap() {
         return this.attributes;
     }
 
+    /**
+     * Returns a new user object that additionally contains the given security roles.
+     */
     public User withSecurityRoles(Collection<String> securityRoles) {
         if (securityRoles == null || securityRoles.isEmpty()) {
             return this;
@@ -315,6 +321,9 @@ public boolean isPluginUser() {
         return name != null && name.startsWith("plugin:");
     }
 
+    /**
+     * Returns a String containing serialized form of this User object. Never returns null.
+     */
     public String toSerializedBase64() {
         String result = this.serializedBase64;
 
@@ -325,11 +334,42 @@ public String toSerializedBase64() {
         return result;
     }
 
+    /**
+     * Returns a rough estimated byte size of this object. Used for cache size control.
+     */
+    public int estimatedByteSize() {
+        return this.estimatedByteSize;
+    }
+
+    private int calcEstimatedByteSize() {
+        int size = 32;
+        size += estimateStringSize(this.name);
+        size += estimateStringSize(this.requestedTenant);
+        size += this.roles.stream().mapToInt(User::estimateStringSize).sum() + 32;
+        size += this.securityRoles.stream().mapToInt(User::estimateStringSize).sum() + 32;
+        size += this.attributes.entrySet()
+            .stream()
+            .mapToInt((entry) -> estimateStringSize(entry.getKey()) + estimateStringSize(entry.getValue()))
+            .sum() + 32;
+        return size;
+    }
+
+    private static int estimateStringSize(String s) {
+        if (s != null) {
+            return 40 + s.length() * 2;
+        } else {
+            return 0;
+        }
+    }
+
     void readObject(ObjectInputStream stream) throws InvalidObjectException {
         // This object is not supposed to directly read in order to keep compatibility with older OpenSearch versions
         throw new InvalidObjectException("Use org.opensearch.security.user.serialized.User");
     }
 
+    /**
+     * Used for creating a backwards compatible object that can be used for serialization.
+     */
     @Serial
     private static final ObjectStreamField[] serialPersistentFields = {
         new ObjectStreamField("name", String.class),

src/main/java/org/opensearch/security/user/UserFactory.java

@@ -15,6 +15,14 @@
 
 import com.google.common.cache.Cache;
 import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.Weigher;
+
+import org.opensearch.OpenSearchException;
+import org.opensearch.common.settings.Setting;
+import org.opensearch.common.settings.Settings;
+import org.opensearch.common.unit.TimeValue;
+import org.opensearch.core.common.unit.ByteSizeUnit;
+import org.opensearch.core.common.unit.ByteSizeValue;
 
 /**
  * Infrastructure to create user objects. This class has two implementations:
@@ -24,6 +32,13 @@
  * </ul>
  */
 public abstract class UserFactory {
+    /**
+     * Converts a serialized form of a User object to a User obect. This might use a cache.
+     *
+     * @param serializedBase64 a string with a serialized form of a User object
+     * @return A User object. Never returns null.
+     * @throws OpenSearchException in case the provided string could not be processed.
+     */
     public abstract User fromSerializedBase64(String serializedBase64);
 
     public static class Simple extends UserFactory {
@@ -35,10 +50,45 @@ public User fromSerializedBase64(String serializedBase64) {
     }
 
     public static class Caching extends UserFactory {
+
+        /**
+         * This setting specifies the maximum estimated byte size of the cache. The size is estimated, so take it
+         * with a grain of salt. It shall just serve as a rough limit. The default is 10 MB.
+         */
+        public static Setting<ByteSizeValue> MAX_SIZE = Setting.memorySizeSetting(
+            "plugins.security.transport_user_cache.max_heap_size",
+            new ByteSizeValue(10, ByteSizeUnit.MB),
+            Setting.Property.NodeScope
+        );
+
+        /**
+         * This setting specifies the maximum number of users inside the cache. The default is 1000 users.
+         */
+        public static Setting<Integer> MAX_ENTRIES = Setting.intSetting(
+            "plugins.security.transport_user_cache.max_entries",
+            1000,
+            Setting.Property.NodeScope
+        );
+
+        /**
+         * This setting specifies the maximum time an entry is kept in the cache. This is solely for saving space;
+         * a stale cache is not possible. The default is 1 hour.
+         */
+        public static Setting<TimeValue> EXPIRE_AFTER_ACCESS = Setting.timeSetting(
+            "plugins.security.transport_user_cache.expire_after_access",
+            TimeValue.timeValueHours(1),
+            Setting.Property.NodeScope
+        );
+
         private final Cache<String, User> serializedBase64ToUserCache;
 
-        public Caching() {
-            this.serializedBase64ToUserCache = CacheBuilder.newBuilder().expireAfterAccess(Duration.ofHours(1)).build();
+        public Caching(Settings settings) {
+            this.serializedBase64ToUserCache = CacheBuilder.<String, User>newBuilder()
+                .weigher((Weigher<String, User>) (key, user) -> 16 + key.length() + user.estimatedByteSize())
+                .maximumSize(MAX_ENTRIES.get(settings))
+                .maximumWeight(MAX_SIZE.get(settings).getBytes())
+                .expireAfterAccess(Duration.ofMillis(EXPIRE_AFTER_ACCESS.get(settings).millis()))
+                .build();
         }
 
         public User fromSerializedBase64(String serializedBase64) {

src/test/java/org/opensearch/security/identity/SecurityTokenManagerTest.java

@@ -12,7 +12,6 @@
 package org.opensearch.security.identity;
 
 import java.io.IOException;
-import java.util.List;
 import java.util.Set;
 
 import org.junit.After;
@@ -202,7 +201,7 @@ public void issueOnBehalfOfToken_jwtGenerationFailure() throws Exception {
         doAnswer(invockation -> new ClusterName("cluster17")).when(cs).getClusterName();
         doAnswer(invocation -> true).when(tokenManager).issueOnBehalfOfTokenAllowed();
         final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
-        threadContext.putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, new User("Jon", List.of(), null));
+        threadContext.putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, new User("Jon"));
         when(threadPool.getThreadContext()).thenReturn(threadContext);
         final ConfigModel configModel = mock(ConfigModel.class);
         tokenManager.onConfigModelChanged(configModel);
@@ -228,7 +227,7 @@ public void issueOnBehalfOfToken_success() throws Exception {
         doAnswer(invockation -> new ClusterName("cluster17")).when(cs).getClusterName();
         doAnswer(invocation -> true).when(tokenManager).issueOnBehalfOfTokenAllowed();
         final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
-        threadContext.putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, new User("Jon", List.of(), null));
+        threadContext.putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, new User("Jon"));
         when(threadPool.getThreadContext()).thenReturn(threadContext);
         final ConfigModel configModel = mock(ConfigModel.class);
         tokenManager.onConfigModelChanged(configModel);