Skip to content

Commit

Permalink
Coerce blank fields to null in ApiKey requests
Browse files Browse the repository at this point in the history 
API Key request objects (`InvalidateApiKeyRequest` and
`GetApiKeyRequest`) support multiple key-selection parameters such as
realm-name, user-name, key-id and key-name.

The specified behaviour is that if any of these are _blank_ then they
act as a wildcard and do not restrict the search criteria.

This change moves the "is blank" logic into the constructor for these
requests so that there is a single consistent way to determine blank
(`Strings.hasText(arg) == false`) and all usage of these fields can
rely on the getter returning either `null` or a _real value_, and
never a non-null blank.

Relates: elastic#62916
Backport of: elastic#66240
  • Loading branch information
@tvernum
tvernum committed Dec 15, 2020
1 parent a18aa3f commit bbc9b70
Show file tree
Hide file tree
Showing 4 changed files with 64 additions and 22 deletions.
20 changes: 12 additions & 8 deletions ...gin/core/src/main/java/org/elasticsearch/xpack/core/security/action/GetApiKeyRequest.java
View file
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
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;
}
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
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 = () -> randomBoolean() ? "" : " ";
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
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 = () -> randomBoolean() ? "" : " ";
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

0 comments on commit bbc9b70

Please sign in to comment.