Skip to content

Commit 7d8a64d

Browse files
jaymodejaymode
authored
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
1 parent c985f50 commit 7d8a64d

File tree

5 files changed

+121
-65
lines changed

5 files changed

+121
-65
lines changed

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

Lines changed: 42 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,7 @@
1313
import org.elasticsearch.common.bytes.BytesReference;
1414
import org.elasticsearch.common.io.stream.StreamInput;
1515
import org.elasticsearch.common.io.stream.StreamOutput;
16-
import org.elasticsearch.common.settings.Settings;
1716
import org.elasticsearch.xpack.core.security.authc.support.CharArrays;
18-
import org.elasticsearch.xpack.core.security.support.MetadataUtils;
19-
import org.elasticsearch.xpack.core.security.support.Validation;
2017

2118
import java.io.IOException;
2219
import java.util.Map;
@@ -34,6 +31,7 @@ public class PutUserRequest extends ActionRequest implements UserRequest, WriteR
3431
private String email;
3532
private Map<String, Object> metadata;
3633
private char[] passwordHash;
34+
private char[] password;
3735
private boolean enabled = true;
3836
private RefreshPolicy refreshPolicy = RefreshPolicy.IMMEDIATE;
3937

@@ -45,18 +43,15 @@ public ActionRequestValidationException validate() {
4543
ActionRequestValidationException validationException = null;
4644
if (username == null) {
4745
validationException = addValidationError("user is missing", validationException);
48-
} else {
49-
Validation.Error error = Validation.Users.validateUsername(username, false, Settings.EMPTY);
50-
if (error != null) {
51-
validationException = addValidationError(error.toString(), validationException);
52-
}
5346
}
5447
if (roles == null) {
5548
validationException = addValidationError("roles are missing", validationException);
5649
}
57-
if (metadata != null && MetadataUtils.containsReservedMetadata(metadata)) {
58-
validationException = addValidationError("metadata keys may not start with [" + MetadataUtils.RESERVED_PREFIX + "]",
59-
validationException);
50+
if (metadata != null && metadata.keySet().stream().anyMatch(s -> s.startsWith("_"))) {
51+
validationException = addValidationError("metadata keys may not start with [_]", validationException);
52+
}
53+
if (password != null && passwordHash != null) {
54+
validationException = addValidationError("only one of [password, passwordHash] can be provided", validationException);
6055
}
6156
// 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
6257
return validationException;
@@ -86,8 +81,12 @@ public void passwordHash(@Nullable char[] passwordHash) {
8681
this.passwordHash = passwordHash;
8782
}
8883

89-
public boolean enabled() {
90-
return enabled;
84+
public void enabled(boolean enabled) {
85+
this.enabled = enabled;
86+
}
87+
88+
public void password(@Nullable char[] password) {
89+
this.password = password;
9190
}
9291

9392
/**
@@ -130,25 +129,25 @@ public char[] passwordHash() {
130129
return passwordHash;
131130
}
132131

133-
public void enabled(boolean enabled) {
134-
this.enabled = enabled;
132+
public boolean enabled() {
133+
return enabled;
135134
}
136135

137136
@Override
138137
public String[] usernames() {
139138
return new String[] { username };
140139
}
141140

141+
@Nullable
142+
public char[] password() {
143+
return password;
144+
}
145+
142146
@Override
143147
public void readFrom(StreamInput in) throws IOException {
144148
super.readFrom(in);
145149
username = in.readString();
146-
BytesReference passwordHashRef = in.readBytesReference();
147-
if (passwordHashRef == BytesArray.EMPTY) {
148-
passwordHash = null;
149-
} else {
150-
passwordHash = CharArrays.utf8BytesToChars(BytesReference.toBytes(passwordHashRef));
151-
}
150+
passwordHash = readCharArrayFromStream(in);
152151
roles = in.readStringArray();
153152
fullName = in.readOptionalString();
154153
email = in.readOptionalString();
@@ -161,13 +160,10 @@ public void readFrom(StreamInput in) throws IOException {
161160
public void writeTo(StreamOutput out) throws IOException {
162161
super.writeTo(out);
163162
out.writeString(username);
164-
BytesReference passwordHashRef;
165-
if (passwordHash == null) {
166-
passwordHashRef = null;
167-
} else {
168-
passwordHashRef = new BytesArray(CharArrays.toUtf8Bytes(passwordHash));
163+
writeCharArrayToStream(out, passwordHash);
164+
if (password != null) {
165+
throw new IllegalStateException("password cannot be serialized. it is only used for HL rest");
169166
}
170-
out.writeBytesReference(passwordHashRef);
171167
out.writeStringArray(roles);
172168
out.writeOptionalString(fullName);
173169
out.writeOptionalString(email);
@@ -180,4 +176,23 @@ public void writeTo(StreamOutput out) throws IOException {
180176
refreshPolicy.writeTo(out);
181177
out.writeBoolean(enabled);
182178
}
179+
180+
private static char[] readCharArrayFromStream(StreamInput in) throws IOException {
181+
BytesReference charBytesRef = in.readBytesReference();
182+
if (charBytesRef == BytesArray.EMPTY) {
183+
return null;
184+
} else {
185+
return CharArrays.utf8BytesToChars(BytesReference.toBytes(charBytesRef));
186+
}
187+
}
188+
189+
private static void writeCharArrayToStream(StreamOutput out, char[] chars) throws IOException {
190+
final BytesReference charBytesRef;
191+
if (chars == null) {
192+
charBytesRef = null;
193+
} else {
194+
charBytesRef = new BytesArray(CharArrays.toUtf8Bytes(chars));
195+
}
196+
out.writeBytesReference(charBytesRef);
197+
}
183198
}

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

Lines changed: 52 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
import org.apache.logging.log4j.message.ParameterizedMessage;
99
import org.apache.logging.log4j.util.Supplier;
1010
import org.elasticsearch.action.ActionListener;
11+
import org.elasticsearch.action.ActionRequestValidationException;
1112
import org.elasticsearch.action.support.ActionFilters;
1213
import org.elasticsearch.action.support.HandledTransportAction;
1314
import org.elasticsearch.common.inject.Inject;
@@ -18,11 +19,15 @@
1819
import org.elasticsearch.xpack.core.security.action.user.PutUserRequest;
1920
import org.elasticsearch.xpack.core.security.action.user.PutUserResponse;
2021
import org.elasticsearch.xpack.core.security.authc.esnative.ClientReservedRealm;
22+
import org.elasticsearch.xpack.core.security.support.Validation;
2123
import org.elasticsearch.xpack.core.security.user.AnonymousUser;
2224
import org.elasticsearch.xpack.core.security.user.SystemUser;
25+
import org.elasticsearch.xpack.core.security.user.XPackSecurityUser;
2326
import org.elasticsearch.xpack.core.security.user.XPackUser;
2427
import org.elasticsearch.xpack.security.authc.esnative.NativeUsersStore;
2528

29+
import static org.elasticsearch.action.ValidateActions.addValidationError;
30+
2631
public class TransportPutUserAction extends HandledTransportAction<PutUserRequest, PutUserResponse> {
2732

2833
private final NativeUsersStore usersStore;
@@ -36,37 +41,62 @@ public TransportPutUserAction(Settings settings, ActionFilters actionFilters,
3641

3742
@Override
3843
protected void doExecute(Task task, final PutUserRequest request, final ActionListener<PutUserResponse> listener) {
44+
final ActionRequestValidationException validationException = validateRequest(request);
45+
if (validationException != null) {
46+
listener.onFailure(validationException);
47+
} else {
48+
usersStore.putUser(request, new ActionListener<Boolean>() {
49+
@Override
50+
public void onResponse(Boolean created) {
51+
if (created) {
52+
logger.info("added user [{}]", request.username());
53+
} else {
54+
logger.info("updated user [{}]", request.username());
55+
}
56+
listener.onResponse(new PutUserResponse(created));
57+
}
58+
59+
@Override
60+
public void onFailure(Exception e) {
61+
logger.error((Supplier<?>) () -> new ParameterizedMessage("failed to put user [{}]", request.username()), e);
62+
listener.onFailure(e);
63+
}
64+
});
65+
}
66+
}
67+
68+
private ActionRequestValidationException validateRequest(PutUserRequest request) {
69+
ActionRequestValidationException validationException = null;
3970
final String username = request.username();
4071
if (ClientReservedRealm.isReserved(username, settings)) {
4172
if (AnonymousUser.isAnonymousUsername(username, settings)) {
42-
listener.onFailure(new IllegalArgumentException("user [" + username + "] is anonymous and cannot be modified via the API"));
43-
return;
73+
validationException =
74+
addValidationError("user [" + username + "] is anonymous and cannot be modified via the API", validationException);
4475
} else {
45-
listener.onFailure(new IllegalArgumentException("user [" + username + "] is reserved and only the " +
46-
"password can be changed"));
47-
return;
76+
validationException = addValidationError("user [" + username + "] is reserved and only the " +
77+
"password can be changed", validationException);
78+
}
79+
} else if (SystemUser.NAME.equals(username) || XPackUser.NAME.equals(username) || XPackSecurityUser.NAME.equals(username)) {
80+
validationException = addValidationError("user [" + username + "] is internal", validationException);
81+
} else {
82+
Validation.Error usernameError = Validation.Users.validateUsername(username, true, settings);
83+
if (usernameError != null) {
84+
validationException = addValidationError(usernameError.toString(), validationException);
4885
}
49-
} else if (SystemUser.NAME.equals(username) || XPackUser.NAME.equals(username)) {
50-
listener.onFailure(new IllegalArgumentException("user [" + username + "] is internal"));
51-
return;
5286
}
5387

54-
usersStore.putUser(request, new ActionListener<Boolean>() {
55-
@Override
56-
public void onResponse(Boolean created) {
57-
if (created) {
58-
logger.info("added user [{}]", request.username());
59-
} else {
60-
logger.info("updated user [{}]", request.username());
88+
if (request.roles() != null) {
89+
for (String role : request.roles()) {
90+
Validation.Error roleNameError = Validation.Roles.validateRoleName(role, true);
91+
if (roleNameError != null) {
92+
validationException = addValidationError(roleNameError.toString(), validationException);
6193
}
62-
listener.onResponse(new PutUserResponse(created));
6394
}
95+
}
6496

65-
@Override
66-
public void onFailure(Exception e) {
67-
logger.error((Supplier<?>) () -> new ParameterizedMessage("failed to put user [{}]", request.username()), e);
68-
listener.onFailure(e);
69-
}
70-
});
97+
if (request.password() != null) {
98+
validationException = addValidationError("password should never be passed to the transport action", validationException);
99+
}
100+
return validationException;
71101
}
72102
}

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

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -39,16 +39,6 @@ public void testValidateRejectsNullUserName() throws Exception {
3939
assertThat(validation.validationErrors().size(), is(1));
4040
}
4141

42-
public void testValidateRejectsUserNameThatHasInvalidCharacters() throws Exception {
43-
final PutUserRequest request = new PutUserRequest();
44-
request.username("fóóbár");
45-
request.roles("bar");
46-
final ActionRequestValidationException validation = request.validate();
47-
assertThat(validation, is(notNullValue()));
48-
assertThat(validation.validationErrors(), contains(containsString("must be")));
49-
assertThat(validation.validationErrors().size(), is(1));
50-
}
51-
5242
public void testValidateRejectsMetaDataWithLeadingUnderscore() throws Exception {
5343
final PutUserRequest request = new PutUserRequest();
5444
request.username("foo");

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

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77

88
import org.elasticsearch.ElasticsearchSecurityException;
99
import org.elasticsearch.action.ActionListener;
10+
import org.elasticsearch.action.ActionRequestValidationException;
1011
import org.elasticsearch.action.support.ActionFilters;
1112
import org.elasticsearch.action.support.PlainActionFuture;
1213
import org.elasticsearch.common.ValidationException;
@@ -37,6 +38,7 @@
3738
import java.util.Collections;
3839
import java.util.concurrent.atomic.AtomicReference;
3940

41+
import static org.hamcrest.Matchers.contains;
4042
import static org.hamcrest.Matchers.containsString;
4143
import static org.hamcrest.Matchers.instanceOf;
4244
import static org.hamcrest.Matchers.is;
@@ -194,12 +196,32 @@ public void onFailure(Exception e) {
194196
}
195197
});
196198

199+
assertThat(throwableRef.get(), is(nullValue()));
197200
assertThat(responseRef.get(), is(notNullValue()));
198201
assertThat(responseRef.get().created(), is(created));
199-
assertThat(throwableRef.get(), is(nullValue()));
200202
verify(usersStore, times(1)).putUser(eq(request), any(ActionListener.class));
201203
}
202204

205+
public void testInvalidUser() {
206+
NativeUsersStore usersStore = mock(NativeUsersStore.class);
207+
TransportService transportService = new TransportService(Settings.EMPTY, mock(Transport.class), null,
208+
TransportService.NOOP_TRANSPORT_INTERCEPTOR, x -> null, null, Collections.emptySet());
209+
TransportPutUserAction action = new TransportPutUserAction(Settings.EMPTY, mock(ActionFilters.class),
210+
usersStore, transportService);
211+
212+
final PutUserRequest request = new PutUserRequest();
213+
request.username("fóóbár");
214+
request.roles("bar");
215+
ActionRequestValidationException validation = request.validate();
216+
assertNull(validation);
217+
218+
PlainActionFuture<PutUserResponse> responsePlainActionFuture = new PlainActionFuture<>();
219+
action.doExecute(mock(Task.class), request, responsePlainActionFuture);
220+
validation = expectThrows(ActionRequestValidationException.class, responsePlainActionFuture::actionGet);
221+
assertThat(validation.validationErrors(), contains(containsString("must be")));
222+
assertThat(validation.validationErrors().size(), is(1));
223+
}
224+
203225
public void testException() {
204226
final Exception e = randomFrom(new ElasticsearchSecurityException(""), new IllegalStateException(), new ValidationException());
205227
final User user = new User("joe");

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

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313
import org.elasticsearch.action.support.PlainActionFuture;
1414
import org.elasticsearch.client.Client;
1515
import org.elasticsearch.common.Strings;
16-
import org.elasticsearch.common.ValidationException;
1716
import org.elasticsearch.common.bytes.BytesArray;
1817
import org.elasticsearch.common.collect.MapBuilder;
1918
import org.elasticsearch.common.settings.SecureString;
@@ -492,14 +491,14 @@ public void testCannotCreateUserWithShortPassword() throws Exception {
492491
client.preparePutUser("joe", randomAlphaOfLengthBetween(0, 5).toCharArray(), hasher,
493492
"admin_role").get();
494493
fail("cannot create a user without a password < 6 characters");
495-
} catch (ValidationException v) {
494+
} catch (IllegalArgumentException v) {
496495
assertThat(v.getMessage().contains("password"), is(true));
497496
}
498497
}
499498

500499
public void testCannotCreateUserWithInvalidCharactersInName() throws Exception {
501500
SecurityClient client = securityClient();
502-
ValidationException v = expectThrows(ValidationException.class,
501+
IllegalArgumentException v = expectThrows(IllegalArgumentException.class,
503502
() -> client.preparePutUser("fóóbár", "my-am@zing-password".toCharArray(), hasher,
504503
"admin_role").get()
505504
);
@@ -533,7 +532,7 @@ public void testOperationsOnReservedUsers() throws Exception {
533532
IllegalArgumentException exception = expectThrows(IllegalArgumentException.class,
534533
() -> securityClient().preparePutUser(username, randomBoolean() ? SecuritySettingsSourceField.TEST_PASSWORD.toCharArray()
535534
: null, hasher, "admin").get());
536-
assertThat(exception.getMessage(), containsString("Username [" + username + "] is reserved"));
535+
assertThat(exception.getMessage(), containsString("user [" + username + "] is reserved"));
537536

538537
exception = expectThrows(IllegalArgumentException.class,
539538
() -> securityClient().prepareDeleteUser(username).get());
@@ -551,7 +550,7 @@ public void testOperationsOnReservedUsers() throws Exception {
551550
exception = expectThrows(IllegalArgumentException.class,
552551
() -> securityClient().preparePutUser(AnonymousUser.DEFAULT_ANONYMOUS_USERNAME, "foobar".toCharArray(),
553552
hasher).get());
554-
assertThat(exception.getMessage(), containsString("Username [" + AnonymousUser.DEFAULT_ANONYMOUS_USERNAME + "] is reserved"));
553+
assertThat(exception.getMessage(), containsString("user [" + AnonymousUser.DEFAULT_ANONYMOUS_USERNAME + "] is anonymous"));
555554

556555
exception = expectThrows(IllegalArgumentException.class,
557556
() -> securityClient().preparePutUser(SystemUser.NAME, "foobar".toCharArray(), hasher).get());

0 commit comments

Comments
 (0)