Move validation to server for put user requests

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.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 ActionRequestValidationException validate() {
         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 void passwordHash(@Nullable char[] passwordHash) {
         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,25 +129,25 @@ public char[] passwordHash() {
         return passwordHash;
     }
 
-    public void enabled(boolean enabled) {
-        this.enabled = enabled;
+    public boolean enabled() {
+        return enabled;
     }
 
     @Override
     public String[] usernames() {
         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 void readFrom(StreamInput in) throws IOException {
     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 void writeTo(StreamOutput out) throws IOException {
         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 @@
 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.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 TransportPutUserAction(Settings settings, ActionFilters actionFilters,
 
     @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 void testValidateRejectsNullUserName() throws Exception {
         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 @@
 
 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.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 void onFailure(Exception e) {
             }
         });
 
+        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.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 void testCannotCreateUserWithShortPassword() throws Exception {
             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 void testOperationsOnReservedUsers() throws Exception {
         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 void testOperationsOnReservedUsers() throws Exception {
         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());