Skip to content

Commit

Permalink
Move validation to server for put user requests (#32471)
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@jaymode
jaymode committed Aug 14, 2018
1 parent c76d6a7 commit 55d7293
Show file tree
Hide file tree
Showing 5 changed files with 121 additions and 65 deletions.
69 changes: 42 additions & 27 deletions .../core/src/main/java/org/elasticsearch/xpack/core/security/action/user/PutUserRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;

Expand All @@ -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;
Expand Down Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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();
Expand All @@ -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);
Expand All @@ -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);
}
}
74 changes: 52 additions & 22 deletions ...ty/src/main/java/org/elasticsearch/xpack/security/action/user/TransportPutUserAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.cluster.metadata.IndexNameExpressionResolver;
Expand All @@ -19,11 +20,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;
Expand All @@ -38,37 +43,62 @@ public TransportPutUserAction(Settings settings, ThreadPool threadPool, ActionFi

@Override
protected void doExecute(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;
}
}
10 changes: 0 additions & 10 deletions ...urity/src/test/java/org/elasticsearch/xpack/security/action/user/PutUserRequestTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
24 changes: 23 additions & 1 deletion ...c/test/java/org/elasticsearch/xpack/security/action/user/TransportPutUserActionTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.cluster.metadata.IndexNameExpressionResolver;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -196,12 +198,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(ThreadPool.class), mock(ActionFilters.class),
mock(IndexNameExpressionResolver.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(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");
Expand Down
9 changes: 4 additions & 5 deletions .../src/test/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealmIntegTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -490,14 +489,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()
);
Expand Down Expand Up @@ -531,7 +530,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());
Expand All @@ -547,7 +546,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());
Expand Down

0 comments on commit 55d7293

Please sign in to comment.