Move validation to server for put user requests (#32471)

This change moves the validation for values of usernames and passwords
from the request to the transport action. This is done to prevent
the need to move more classes into protocol once we add this API to the
high level rest client. Additionally, this resolves an issue where
validation depends on settings and we always pass empty settings
instead of the actual settings.

Relates #32332

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/user/PutUserRequest.java

@@ -13,10 +13,7 @@ import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.io.stream.StreamInput;
 import org.elasticsearch.common.io.stream.StreamOutput;
-import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.xpack.core.security.authc.support.CharArrays;
-import org.elasticsearch.xpack.core.security.support.MetadataUtils;
-import org.elasticsearch.xpack.core.security.support.Validation;
 
 import java.io.IOException;
 import java.util.Map;
@@ -34,6 +31,7 @@ public class PutUserRequest extends ActionRequest implements UserRequest, WriteR
     private String email;
     private Map<String, Object> metadata;
     private char[] passwordHash;
+    private char[] password;
     private boolean enabled = true;
     private RefreshPolicy refreshPolicy = RefreshPolicy.IMMEDIATE;
 
@@ -45,18 +43,15 @@ public class PutUserRequest extends ActionRequest implements UserRequest, WriteR
         ActionRequestValidationException validationException = null;
         if (username == null) {
             validationException = addValidationError("user is missing", validationException);
-        } else {
-            Validation.Error error = Validation.Users.validateUsername(username, false, Settings.EMPTY);
-            if (error != null) {
-                validationException = addValidationError(error.toString(), validationException);
-            }
         }
         if (roles == null) {
             validationException = addValidationError("roles are missing", validationException);
         }
-        if (metadata != null && MetadataUtils.containsReservedMetadata(metadata)) {
-            validationException = addValidationError("metadata keys may not start with [" + MetadataUtils.RESERVED_PREFIX + "]",
-                    validationException);
+        if (metadata != null && metadata.keySet().stream().anyMatch(s -> s.startsWith("_"))) {
+            validationException = addValidationError("metadata keys may not start with [_]", validationException);
+        }
+        if (password != null && passwordHash != null) {
+            validationException = addValidationError("only one of [password, passwordHash] can be provided", validationException);
         }
         // we do not check for a password hash here since it is possible that the user exists and we don't want to update the password
         return validationException;
@@ -86,8 +81,12 @@ public class PutUserRequest extends ActionRequest implements UserRequest, WriteR
         this.passwordHash = passwordHash;
     }
 
-    public boolean enabled() {
-        return enabled;
+    public void enabled(boolean enabled) {
+        this.enabled = enabled;
+    }
+
+    public void password(@Nullable char[] password) {
+        this.password = password;
     }
 
     /**
@@ -130,8 +129,8 @@ public class PutUserRequest extends ActionRequest implements UserRequest, WriteR
         return passwordHash;
     }
 
-    public void enabled(boolean enabled) {
-        this.enabled = enabled;
+    public boolean enabled() {
+        return enabled;
     }
 
     @Override
@@ -139,16 +138,16 @@ public class PutUserRequest extends ActionRequest implements UserRequest, WriteR
         return new String[] { username };
     }
 
+    @Nullable
+    public char[] password() {
+        return password;
+    }
+
     @Override
     public void readFrom(StreamInput in) throws IOException {
         super.readFrom(in);
         username = in.readString();
-        BytesReference passwordHashRef = in.readBytesReference();
-        if (passwordHashRef == BytesArray.EMPTY) {
-            passwordHash = null;
-        } else {
-            passwordHash = CharArrays.utf8BytesToChars(BytesReference.toBytes(passwordHashRef));
-        }
+        passwordHash = readCharArrayFromStream(in);
         roles = in.readStringArray();
         fullName = in.readOptionalString();
         email = in.readOptionalString();
@@ -161,13 +160,10 @@ public class PutUserRequest extends ActionRequest implements UserRequest, WriteR
     public void writeTo(StreamOutput out) throws IOException {
         super.writeTo(out);
         out.writeString(username);
-        BytesReference passwordHashRef;
-        if (passwordHash == null) {
-            passwordHashRef = null;
-        } else {
-            passwordHashRef = new BytesArray(CharArrays.toUtf8Bytes(passwordHash));
+        writeCharArrayToStream(out, passwordHash);
+        if (password != null) {
+            throw new IllegalStateException("password cannot be serialized. it is only used for HL rest");
         }
-        out.writeBytesReference(passwordHashRef);
         out.writeStringArray(roles);
         out.writeOptionalString(fullName);
         out.writeOptionalString(email);
@@ -180,4 +176,23 @@ public class PutUserRequest extends ActionRequest implements UserRequest, WriteR
         refreshPolicy.writeTo(out);
         out.writeBoolean(enabled);
     }
+
+    private static char[] readCharArrayFromStream(StreamInput in) throws IOException {
+        BytesReference charBytesRef = in.readBytesReference();
+        if (charBytesRef == BytesArray.EMPTY) {
+            return null;
+        } else {
+            return CharArrays.utf8BytesToChars(BytesReference.toBytes(charBytesRef));
+        }
+    }
+
+    private static void writeCharArrayToStream(StreamOutput out, char[] chars) throws IOException {
+        final BytesReference charBytesRef;
+        if (chars == null) {
+            charBytesRef = null;
+        } else {
+            charBytesRef = new BytesArray(CharArrays.toUtf8Bytes(chars));
+        }
+        out.writeBytesReference(charBytesRef);
+    }
 }

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportPutUserAction.java

@@ -8,6 +8,7 @@ package org.elasticsearch.xpack.security.action.user;
 import org.apache.logging.log4j.message.ParameterizedMessage;
 import org.apache.logging.log4j.util.Supplier;
 import org.elasticsearch.action.ActionListener;
+import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.action.support.ActionFilters;
 import org.elasticsearch.action.support.HandledTransportAction;
 import org.elasticsearch.common.inject.Inject;
@@ -18,11 +19,15 @@ import org.elasticsearch.xpack.core.security.action.user.PutUserAction;
 import org.elasticsearch.xpack.core.security.action.user.PutUserRequest;
 import org.elasticsearch.xpack.core.security.action.user.PutUserResponse;
 import org.elasticsearch.xpack.core.security.authc.esnative.ClientReservedRealm;
+import org.elasticsearch.xpack.core.security.support.Validation;
 import org.elasticsearch.xpack.core.security.user.AnonymousUser;
 import org.elasticsearch.xpack.core.security.user.SystemUser;
+import org.elasticsearch.xpack.core.security.user.XPackSecurityUser;
 import org.elasticsearch.xpack.core.security.user.XPackUser;
 import org.elasticsearch.xpack.security.authc.esnative.NativeUsersStore;
 
+import static org.elasticsearch.action.ValidateActions.addValidationError;
+
 public class TransportPutUserAction extends HandledTransportAction<PutUserRequest, PutUserResponse> {
 
     private final NativeUsersStore usersStore;
@@ -36,37 +41,62 @@ public class TransportPutUserAction extends HandledTransportAction<PutUserReques
 
     @Override
     protected void doExecute(Task task, final PutUserRequest request, final ActionListener<PutUserResponse> listener) {
+        final ActionRequestValidationException validationException = validateRequest(request);
+        if (validationException != null) {
+            listener.onFailure(validationException);
+        } else {
+            usersStore.putUser(request, new ActionListener<Boolean>() {
+                @Override
+                public void onResponse(Boolean created) {
+                    if (created) {
+                        logger.info("added user [{}]", request.username());
+                    } else {
+                        logger.info("updated user [{}]", request.username());
+                    }
+                    listener.onResponse(new PutUserResponse(created));
+                }
+
+                @Override
+                public void onFailure(Exception e) {
+                    logger.error((Supplier<?>) () -> new ParameterizedMessage("failed to put user [{}]", request.username()), e);
+                    listener.onFailure(e);
+                }
+            });
+        }
+    }
+
+    private ActionRequestValidationException validateRequest(PutUserRequest request) {
+        ActionRequestValidationException validationException = null;
         final String username = request.username();
         if (ClientReservedRealm.isReserved(username, settings)) {
             if (AnonymousUser.isAnonymousUsername(username, settings)) {
-                listener.onFailure(new IllegalArgumentException("user [" + username + "] is anonymous and cannot be modified via the API"));
-                return;
+                validationException =
+                    addValidationError("user [" + username + "] is anonymous and cannot be modified via the API", validationException);
             } else {
-                listener.onFailure(new IllegalArgumentException("user [" + username + "] is reserved and only the " +
-                        "password can be changed"));
-                return;
+                validationException = addValidationError("user [" + username + "] is reserved and only the " +
+                    "password can be changed", validationException);
+            }
+        } else if (SystemUser.NAME.equals(username) || XPackUser.NAME.equals(username) || XPackSecurityUser.NAME.equals(username)) {
+            validationException = addValidationError("user [" + username + "] is internal", validationException);
+        } else {
+            Validation.Error usernameError = Validation.Users.validateUsername(username, true, settings);
+            if (usernameError != null) {
+                validationException = addValidationError(usernameError.toString(), validationException);
             }
-        } else if (SystemUser.NAME.equals(username) || XPackUser.NAME.equals(username)) {
-            listener.onFailure(new IllegalArgumentException("user [" + username + "] is internal"));
-            return;
         }
 
-        usersStore.putUser(request, new ActionListener<Boolean>() {
-            @Override
-            public void onResponse(Boolean created) {
-                if (created) {
-                    logger.info("added user [{}]", request.username());
-                } else {
-                    logger.info("updated user [{}]", request.username());
+        if (request.roles() != null) {
+            for (String role : request.roles()) {
+                Validation.Error roleNameError = Validation.Roles.validateRoleName(role, true);
+                if (roleNameError != null) {
+                    validationException = addValidationError(roleNameError.toString(), validationException);
                 }
-                listener.onResponse(new PutUserResponse(created));
             }
+        }
 
-            @Override
-            public void onFailure(Exception e) {
-                logger.error((Supplier<?>) () -> new ParameterizedMessage("failed to put user [{}]", request.username()), e);
-                listener.onFailure(e);
-            }
-        });
+        if (request.password() != null) {
+            validationException = addValidationError("password should never be passed to the transport action", validationException);
+        }
+        return validationException;
     }
 }

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/PutUserRequestTests.java

@@ -39,16 +39,6 @@ public class PutUserRequestTests extends ESTestCase {
         assertThat(validation.validationErrors().size(), is(1));
     }
 
-    public void testValidateRejectsUserNameThatHasInvalidCharacters() throws Exception {
-        final PutUserRequest request = new PutUserRequest();
-        request.username("fóóbár");
-        request.roles("bar");
-        final ActionRequestValidationException validation = request.validate();
-        assertThat(validation, is(notNullValue()));
-        assertThat(validation.validationErrors(), contains(containsString("must be")));
-        assertThat(validation.validationErrors().size(), is(1));
-    }
-
     public void testValidateRejectsMetaDataWithLeadingUnderscore() throws Exception {
         final PutUserRequest request = new PutUserRequest();
         request.username("foo");

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportPutUserActionTests.java

@@ -7,6 +7,7 @@ package org.elasticsearch.xpack.security.action.user;
 
 import org.elasticsearch.ElasticsearchSecurityException;
 import org.elasticsearch.action.ActionListener;
+import org.elasticsearch.action.ActionRequestValidationException;
 import org.elasticsearch.action.support.ActionFilters;
 import org.elasticsearch.action.support.PlainActionFuture;
 import org.elasticsearch.common.ValidationException;
@@ -37,6 +38,7 @@ import java.util.Collection;
 import java.util.Collections;
 import java.util.concurrent.atomic.AtomicReference;
 
+import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.is;
@@ -194,12 +196,32 @@ public class TransportPutUserActionTests extends ESTestCase {
             }
         });
 
+        assertThat(throwableRef.get(), is(nullValue()));
         assertThat(responseRef.get(), is(notNullValue()));
         assertThat(responseRef.get().created(), is(created));
-        assertThat(throwableRef.get(), is(nullValue()));
         verify(usersStore, times(1)).putUser(eq(request), any(ActionListener.class));
     }
 
+    public void testInvalidUser() {
+        NativeUsersStore usersStore = mock(NativeUsersStore.class);
+        TransportService transportService = new TransportService(Settings.EMPTY, mock(Transport.class), null,
+            TransportService.NOOP_TRANSPORT_INTERCEPTOR, x -> null, null, Collections.emptySet());
+        TransportPutUserAction action = new TransportPutUserAction(Settings.EMPTY, mock(ActionFilters.class),
+            usersStore, transportService);
+
+        final PutUserRequest request = new PutUserRequest();
+        request.username("fóóbár");
+        request.roles("bar");
+        ActionRequestValidationException validation = request.validate();
+        assertNull(validation);
+
+        PlainActionFuture<PutUserResponse> responsePlainActionFuture = new PlainActionFuture<>();
+        action.doExecute(mock(Task.class), request, responsePlainActionFuture);
+        validation = expectThrows(ActionRequestValidationException.class, responsePlainActionFuture::actionGet);
+        assertThat(validation.validationErrors(), contains(containsString("must be")));
+        assertThat(validation.validationErrors().size(), is(1));
+    }
+
     public void testException() {
         final Exception e = randomFrom(new ElasticsearchSecurityException(""), new IllegalStateException(), new ValidationException());
         final User user = new User("joe");

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java

@@ -13,7 +13,6 @@ import org.elasticsearch.action.search.SearchResponse;
 import org.elasticsearch.action.support.PlainActionFuture;
 import org.elasticsearch.client.Client;
 import org.elasticsearch.common.Strings;
-import org.elasticsearch.common.ValidationException;
 import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.collect.MapBuilder;
 import org.elasticsearch.common.settings.SecureString;
@@ -492,14 +491,14 @@ public class NativeRealmIntegTests extends NativeRealmIntegTestCase {
             client.preparePutUser("joe", randomAlphaOfLengthBetween(0, 5).toCharArray(), hasher,
                 "admin_role").get();
             fail("cannot create a user without a password < 6 characters");
-        } catch (ValidationException v) {
+        } catch (IllegalArgumentException v) {
             assertThat(v.getMessage().contains("password"), is(true));
         }
     }
 
     public void testCannotCreateUserWithInvalidCharactersInName() throws Exception {
         SecurityClient client = securityClient();
-        ValidationException v = expectThrows(ValidationException.class,
+        IllegalArgumentException v = expectThrows(IllegalArgumentException.class,
             () -> client.preparePutUser("fóóbár", "my-am@zing-password".toCharArray(), hasher,
                 "admin_role").get()
         );
@@ -533,7 +532,7 @@ public class NativeRealmIntegTests extends NativeRealmIntegTestCase {
         IllegalArgumentException exception = expectThrows(IllegalArgumentException.class,
                 () -> securityClient().preparePutUser(username, randomBoolean() ? SecuritySettingsSourceField.TEST_PASSWORD.toCharArray()
                     : null, hasher, "admin").get());
-        assertThat(exception.getMessage(), containsString("Username [" + username + "] is reserved"));
+        assertThat(exception.getMessage(), containsString("user [" + username + "] is reserved"));
 
         exception = expectThrows(IllegalArgumentException.class,
                 () -> securityClient().prepareDeleteUser(username).get());
@@ -551,7 +550,7 @@ public class NativeRealmIntegTests extends NativeRealmIntegTestCase {
         exception = expectThrows(IllegalArgumentException.class,
             () -> securityClient().preparePutUser(AnonymousUser.DEFAULT_ANONYMOUS_USERNAME, "foobar".toCharArray(),
                 hasher).get());
-        assertThat(exception.getMessage(), containsString("Username [" + AnonymousUser.DEFAULT_ANONYMOUS_USERNAME + "] is reserved"));
+        assertThat(exception.getMessage(), containsString("user [" + AnonymousUser.DEFAULT_ANONYMOUS_USERNAME + "] is anonymous"));
 
         exception = expectThrows(IllegalArgumentException.class,
             () -> securityClient().preparePutUser(SystemUser.NAME, "foobar".toCharArray(), hasher).get());