Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Coerce blank fields to null in ApiKey requests #66240

Merged
merged 1 commit into from
Dec 15, 2020
Merged

Coerce blank fields to null in ApiKey requests #66240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions ...gin/core/src/main/java/org/elasticsearch/xpack/core/security/action/GetApiKeyRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,10 @@ public GetApiKeyRequest() {

public GetApiKeyRequest(StreamInput in) throws IOException {
super(in);
realmName = in.readOptionalString();
userName = in.readOptionalString();
apiKeyId = in.readOptionalString();
apiKeyName = in.readOptionalString();
realmName = textOrNull(in.readOptionalString());
userName = textOrNull(in.readOptionalString());
apiKeyId = textOrNull(in.readOptionalString());
apiKeyName = textOrNull(in.readOptionalString());
if (in.getVersion().onOrAfter(Version.V_7_4_0)) {
ownedByAuthenticatedUser = in.readOptionalBoolean();
} else {
Expand All @@ -49,13 +49,17 @@ public GetApiKeyRequest(StreamInput in) throws IOException {

public GetApiKeyRequest(@Nullable String realmName, @Nullable String userName, @Nullable String apiKeyId,
@Nullable String apiKeyName, boolean ownedByAuthenticatedUser) {
this.realmName = realmName;
this.userName = userName;
this.apiKeyId = apiKeyId;
this.apiKeyName = apiKeyName;
this.realmName = textOrNull(realmName);
this.userName = textOrNull(userName);
this.apiKeyId = textOrNull(apiKeyId);
this.apiKeyName = textOrNull(apiKeyName);
this.ownedByAuthenticatedUser = ownedByAuthenticatedUser;
}

private static String textOrNull(@Nullable String arg) {
return Strings.hasText(arg) ? arg : null;
}

public String getRealmName() {
return realmName;
}
Expand Down
20 changes: 12 additions & 8 deletions ...e/src/main/java/org/elasticsearch/xpack/core/security/action/InvalidateApiKeyRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,15 @@ public InvalidateApiKeyRequest() {

public InvalidateApiKeyRequest(StreamInput in) throws IOException {
super(in);
realmName = in.readOptionalString();
userName = in.readOptionalString();
realmName = textOrNull(in.readOptionalString());
userName = textOrNull(in.readOptionalString());
if (in.getVersion().onOrAfter(Version.V_7_10_0)) {
ids = in.readOptionalStringArray();
} else {
final String id = in.readOptionalString();
ids = Strings.hasText(id) == false ? null : new String[] { id };
}
name = in.readOptionalString();
name = textOrNull(in.readOptionalString());
if (in.getVersion().onOrAfter(Version.V_7_4_0)) {
ownedByAuthenticatedUser = in.readOptionalBoolean();
} else {
Expand All @@ -65,17 +65,21 @@ public InvalidateApiKeyRequest(@Nullable String realmName, @Nullable String user
throw new IllegalArgumentException("Must use either [id] or [ids], not both at the same time");
}

this.realmName = realmName;
this.userName = userName;
if (id != null) {
this.ids = new String[] {id};
this.realmName = textOrNull(realmName);
this.userName = textOrNull(userName);
if (Strings.hasText(id)) {
this.ids = new String[]{id};
} else {
this.ids = ids;
}
Comment on lines +70 to 74
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before I touched this class in 7.10, there is no check for id in either constructor. Last time I added

ids = Strings.hasText(id) == false ? null : new String[] { id };

in the constructor with StreamInput. Looking back now, this introduced a tiny inconsistency of how empty id is handled in different constructors. Your change here fixed this inconsistency. Thanks!

Now I have a new question: since a single empty string is now handled and excluded from the constructor, should we also apply the empty check for the string array of ids, i.e., should we move the validation logic of ids into here as well? On one hand, relocating the check for ids is good for consistency in that validate() is more about interactions between multiple fields. On the other hand, it complicates the constructors and also could be considered as a slight behaviour change. Overall, I think I prefer to not touch it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the cautious approach and decided to make this as close as possible to having zero visible changes.

Since the deserialisation code has a hasText check (as you note) I decided that this fix made sense, but chose not to try and filter empty values in the ids array. That seemed like a behavioural change that was outside the scope of this PR.

I'm not necessarily opposed to someone making that change I just wanted this PR to be quick and simple.

this.name = name;
this.name = textOrNull(name);
this.ownedByAuthenticatedUser = ownedByAuthenticatedUser;
}

private static String textOrNull(@Nullable String arg) {
return Strings.hasText(arg) ? arg : null;
}

public String getRealmName() {
return realmName;
}
Expand Down
17 changes: 17 additions & 0 deletions ...ore/src/test/java/org/elasticsearch/xpack/core/security/action/GetApiKeyRequestTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,13 @@
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.util.function.Supplier;

import static org.elasticsearch.test.VersionUtils.randomVersionBetween;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.nullValue;

public class GetApiKeyRequestTests extends ESTestCase {

Expand Down Expand Up @@ -144,6 +146,21 @@ public void testSerialization() throws IOException {
}
}

public void testEmptyStringsAreCoercedToNull() {
Supplier<String> randomBlankString = () -> " ".repeat(randomIntBetween(0, 5));
final GetApiKeyRequest request = new GetApiKeyRequest(
randomBlankString.get(), // realm name
randomBlankString.get(), // user name
randomBlankString.get(), // key id
randomBlankString.get(), // key name
randomBoolean() // owned by user
);
assertThat(request.getRealmName(), nullValue());
assertThat(request.getUserName(), nullValue());
assertThat(request.getApiKeyId(), nullValue());
assertThat(request.getApiKeyName(), nullValue());
}

private static String randomNullOrEmptyString() {
return randomBoolean() ? "" : null;
}
Expand Down
29 changes: 23 additions & 6 deletions .../test/java/org/elasticsearch/xpack/core/security/action/InvalidateApiKeyRequestTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,15 @@
import java.io.ByteArrayInputStream;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.util.function.Supplier;

import static org.elasticsearch.test.VersionUtils.getPreviousVersion;
import static org.elasticsearch.test.VersionUtils.randomVersionBetween;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.nullValue;

public class InvalidateApiKeyRequestTests extends ESTestCase {

Expand All @@ -36,7 +38,7 @@ public void testCannotSpecifyBothIdAndIds() {
randomAlphaOfLength(12),
randomFrom(randomNullOrEmptyString(), randomAlphaOfLength(8)),
false,
new String[] { randomAlphaOfLength(12) }));
new String[]{randomAlphaOfLength(12)}));
assertThat(e.getMessage(), containsString("Must use either [id] or [ids], not both at the same time"));
}

Expand All @@ -47,7 +49,7 @@ public void testNonNullIdsCannotBeEmptyNorContainBlankId() {
null,
randomFrom(randomNullOrEmptyString(), randomAlphaOfLength(8)),
false,
new String[] {});
new String[]{});
ActionRequestValidationException validationException = invalidateApiKeyRequest.validate();
assertNotNull(validationException);
assertThat(validationException.getMessage(), containsString("Field [ids] cannot be an empty array"));
Expand All @@ -58,14 +60,29 @@ public void testNonNullIdsCannotBeEmptyNorContainBlankId() {
null,
randomFrom(randomNullOrEmptyString(), randomAlphaOfLength(8)),
false,
new String[] {randomAlphaOfLength(12), null});
new String[]{randomAlphaOfLength(12), null});
validationException = invalidateApiKeyRequest.validate();
assertNotNull(validationException);

assertThat(validationException.getMessage(), containsString("Field [ids] must not contain blank id, "
+ "but got blank id at index position: [1]"));
}

public void testEmptyStringsAreCoercedToNull() {
Supplier<String> randomBlankString = () -> " ".repeat(randomIntBetween(0, 5));
final InvalidateApiKeyRequest request = new InvalidateApiKeyRequest(
randomBlankString.get(), // realm name
randomBlankString.get(), // user name
randomBlankString.get(), // key id
randomBlankString.get(), // key name
randomBoolean() // owned by user
);
assertThat(request.getRealmName(), nullValue());
assertThat(request.getUserName(), nullValue());
assertThat(request.getIds(), nullValue());
assertThat(request.getName(), nullValue());
}

public void testRequestValidation() {
InvalidateApiKeyRequest request = InvalidateApiKeyRequest.usingApiKeyId(randomAlphaOfLength(5), randomBoolean());
ActionRequestValidationException ve = request.validate();
Expand Down Expand Up @@ -112,7 +129,7 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeOptionalString(user);
if (out.getVersion().onOrAfter(Version.V_7_10_0)) {
if (Strings.hasText(apiKeyId)) {
out.writeOptionalStringArray(new String[] { apiKeyId });
out.writeOptionalStringArray(new String[]{apiKeyId});
} else {
out.writeOptionalStringArray(null);
}
Expand Down Expand Up @@ -148,7 +165,7 @@ public void writeTo(StreamOutput out) throws IOException {

for (int caseNo = 0; caseNo < inputs.length; caseNo++) {
try (ByteArrayOutputStream bos = new ByteArrayOutputStream();
OutputStreamStreamOutput osso = new OutputStreamStreamOutput(bos)) {
OutputStreamStreamOutput osso = new OutputStreamStreamOutput(bos)) {
final Version streamVersion = randomVersionBetween(random(), Version.V_7_4_0, getPreviousVersion(Version.V_7_10_0));
Dummy d = new Dummy(inputs[caseNo]);
osso.setVersion(streamVersion);
Expand Down Expand Up @@ -218,7 +235,7 @@ public void testSerializationWillThrowWhenMultipleIdsAndOldVersionStream() {
null,
randomFrom(randomNullOrEmptyString(), randomAlphaOfLength(8)),
false,
new String[] { randomAlphaOfLength(12), randomAlphaOfLength(12) });
new String[]{randomAlphaOfLength(12), randomAlphaOfLength(12)});
ByteArrayOutputStream outBuffer = new ByteArrayOutputStream();
OutputStreamStreamOutput out = new OutputStreamStreamOutput(outBuffer);
out.setVersion(randomVersionBetween(random(), Version.V_7_4_0, getPreviousVersion(Version.V_7_10_0)));
Expand Down