Skip to content

Move XContent generation to HasPrivilegesResponse #35616

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

Merged
merged 3 commits into from
Nov 21, 2018
Merged

Move XContent generation to HasPrivilegesResponse #35616

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8c0bd41
Move XContent generation to HasPrivilegesResponse
tvernum Nov 16, 2018
024ea7d
Merge branch 'master' into has-privileges-xcontent-test
tvernum Nov 20, 2018
061b509
Switch nested "if" to "&&"
tvernum Nov 20, 2018
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
100 changes: 95 additions & 5 deletions ...rc/main/java/org/elasticsearch/xpack/core/security/action/user/HasPrivilegesResponse.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,14 @@
import org.elasticsearch.action.ActionResponse;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.xcontent.ToXContentObject;
import org.elasticsearch.common.xcontent.XContentBuilder;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand All @@ -22,24 +25,35 @@
/**
* Response for a {@link HasPrivilegesRequest}
*/
public class HasPrivilegesResponse extends ActionResponse {
public class HasPrivilegesResponse extends ActionResponse implements ToXContentObject {
private String username;
private boolean completeMatch;
private Map<String, Boolean> cluster;
private List<ResourcePrivileges> index;
private Map<String, List<ResourcePrivileges>> application;

public HasPrivilegesResponse() {
this(true, Collections.emptyMap(), Collections.emptyList(), Collections.emptyMap());
this("", true, Collections.emptyMap(), Collections.emptyList(), Collections.emptyMap());
}

public HasPrivilegesResponse(boolean completeMatch, Map<String, Boolean> cluster, Collection<ResourcePrivileges> index,
public HasPrivilegesResponse(String username, boolean completeMatch, Map<String, Boolean> cluster, Collection<ResourcePrivileges> index,
Map<String, Collection<ResourcePrivileges>> application) {
super();
this.username = username;
this.completeMatch = completeMatch;
this.cluster = new HashMap<>(cluster);
this.index = new ArrayList<>(index);
this.index = sorted(new ArrayList<>(index));
Copy link
Contributor

@albertzaharovits albertzaharovits Nov 19, 2018

Choose a reason for hiding this comment

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

I would be using a Set in this circumstances.

Copy link
Contributor Author

@tvernum tvernum Nov 20, 2018

Choose a reason for hiding this comment

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

If I was starting from scratch, then so would I, but that would require changing the return types on getIndexPrivileges and getApplicationPrivileges which seemed more invasive than was justified by this PR.

I'm happy to revist that choice if you want - I went back-and-forth many times while making the change.

Copy link
Contributor

@albertzaharovits albertzaharovits Nov 20, 2018

Choose a reason for hiding this comment

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

It's good like it is , it's more civilized, otherwise it would indeed indeed violate the scope of this PR. I think I see the privilege code as new code where creative destruction is more loosely permitted.

But I still think it would be nice to do this change in a follow-up. I am happy to pick it up if you wish.

Copy link
Contributor Author

@tvernum tvernum Nov 21, 2018

Choose a reason for hiding this comment

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

It's all yours :)

this.application = new HashMap<>();
application.forEach((key, val) -> this.application.put(key, Collections.unmodifiableList(new ArrayList<>(val))));
application.forEach((key, val) -> this.application.put(key, Collections.unmodifiableList(sorted(new ArrayList<>(val)))));
}

private static List<ResourcePrivileges> sorted(List<ResourcePrivileges> resources) {
Collections.sort(resources, Comparator.comparing(o -> o.resource));
return resources;
}

public String getUsername() {
return username;
}

public boolean isCompleteMatch() {
Expand All @@ -62,13 +76,40 @@ public Map<String, List<ResourcePrivileges>> getApplicationPrivileges() {
return Collections.unmodifiableMap(application);
}

@Override
public boolean equals(Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
final HasPrivilegesResponse response = (HasPrivilegesResponse) o;
return completeMatch == response.completeMatch
&& Objects.equals(username, response.username)
&& Objects.equals(cluster, response.cluster)
&& Objects.equals(index, response.index)
&& Objects.equals(application, response.application);
}

@Override
public int hashCode() {
return Objects.hash(username, completeMatch, cluster, index, application);
}

public void readFrom(StreamInput in) throws IOException {
super.readFrom(in);
completeMatch = in.readBoolean();
if (in.getVersion().onOrAfter(Version.V_7_0_0)) {
cluster = in.readMap(StreamInput::readString, StreamInput::readBoolean);
}
index = readResourcePrivileges(in);
if (in.getVersion().onOrAfter(Version.V_6_4_0)) {
application = in.readMap(StreamInput::readString, HasPrivilegesResponse::readResourcePrivileges);
}
if (in.getVersion().onOrAfter(Version.V_7_0_0)) {
username = in.readString();
}
}

private static List<ResourcePrivileges> readResourcePrivileges(StreamInput in) throws IOException {
Expand All @@ -86,10 +127,16 @@ private static List<ResourcePrivileges> readResourcePrivileges(StreamInput in) t
public void writeTo(StreamOutput out) throws IOException {
super.writeTo(out);
out.writeBoolean(completeMatch);
if (out.getVersion().onOrAfter(Version.V_7_0_0)) {
out.writeMap(cluster, StreamOutput::writeString, StreamOutput::writeBoolean);
}
Copy link
Contributor

@albertzaharovits albertzaharovits Nov 19, 2018

Choose a reason for hiding this comment

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

I understand this is the oversight you've mentioned

Copy link
Contributor Author

@tvernum tvernum Nov 20, 2018

Choose a reason for hiding this comment

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

Yes - I actually noticed it while working on application privileges, but forgot to come back and fix it.

writeResourcePrivileges(out, index);
if (out.getVersion().onOrAfter(Version.V_6_4_0)) {
out.writeMap(application, StreamOutput::writeString, HasPrivilegesResponse::writeResourcePrivileges);
}
if (out.getVersion().onOrAfter(Version.V_7_0_0)) {
out.writeString(username);
}
}

private static void writeResourcePrivileges(StreamOutput out, List<ResourcePrivileges> privileges) throws IOException {
Expand All @@ -100,6 +147,49 @@ private static void writeResourcePrivileges(StreamOutput out, List<ResourcePrivi
}
}

@Override
public String toString() {
return getClass().getSimpleName() + "{"
+ "username=" + username + ","
+ "completeMatch=" + completeMatch + ","
+ "cluster=" + cluster + ","
+ "index=" + index + ","
+ "application=" + application
+ "}";
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject()
.field("username", username)
.field("has_all_requested", completeMatch);

builder.field("cluster");
builder.map(cluster);

appendResources(builder, "index", index);

builder.startObject("application");
for (String app : application.keySet()) {
appendResources(builder, app, application.get(app));
}
builder.endObject();

builder.endObject();
return builder;
}

private void appendResources(XContentBuilder builder, String field, List<HasPrivilegesResponse.ResourcePrivileges> privileges)
throws IOException {
builder.startObject(field);
for (HasPrivilegesResponse.ResourcePrivileges privilege : privileges) {
builder.field(privilege.getResource());
builder.map(privilege.getPrivileges());
}
builder.endObject();
}


public static class ResourcePrivileges {
private final String resource;
private final Map<String, Boolean> privileges;
Expand Down
96 changes: 88 additions & 8 deletions ...st/java/org/elasticsearch/xpack/core/security/action/user/HasPrivilegesResponseTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,30 +7,42 @@
package org.elasticsearch.xpack.core.security.action.user;

import org.elasticsearch.Version;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.collect.MapBuilder;
import org.elasticsearch.common.io.stream.BytesStreamOutput;
import org.elasticsearch.common.io.stream.StreamInput;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.protocol.AbstractHlrcStreamableXContentTestCase;
import org.elasticsearch.test.VersionUtils;
import org.hamcrest.Matchers;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.stream.Collectors;

import static org.hamcrest.Matchers.equalTo;

public class HasPrivilegesResponseTests extends ESTestCase {
public class HasPrivilegesResponseTests
extends AbstractHlrcStreamableXContentTestCase<HasPrivilegesResponse, org.elasticsearch.client.security.HasPrivilegesResponse> {

public void testSerializationV64OrLater() throws IOException {
public void testSerializationV64OrV65() throws IOException {
final HasPrivilegesResponse original = randomResponse();
final Version version = VersionUtils.randomVersionBetween(random(), Version.V_6_4_0, Version.CURRENT);
final Version version = VersionUtils.randomVersionBetween(random(), Version.V_6_4_0, Version.V_6_5_1);
final HasPrivilegesResponse copy = serializeAndDeserialize(original, version);

assertThat(copy.isCompleteMatch(), equalTo(original.isCompleteMatch()));
// assertThat(copy.getClusterPrivileges(), equalTo(original.getClusterPrivileges()));
assertThat(copy.getClusterPrivileges().entrySet(), Matchers.emptyIterable());
assertThat(copy.getIndexPrivileges(), equalTo(original.getIndexPrivileges()));
assertThat(copy.getApplicationPrivileges(), equalTo(original.getApplicationPrivileges()));
}
Expand All @@ -40,11 +52,79 @@ public void testSerializationV63() throws IOException {
final HasPrivilegesResponse copy = serializeAndDeserialize(original, Version.V_6_3_0);

assertThat(copy.isCompleteMatch(), equalTo(original.isCompleteMatch()));
// assertThat(copy.getClusterPrivileges(), equalTo(original.getClusterPrivileges()));
assertThat(copy.getClusterPrivileges().entrySet(), Matchers.emptyIterable());
assertThat(copy.getIndexPrivileges(), equalTo(original.getIndexPrivileges()));
assertThat(copy.getApplicationPrivileges(), equalTo(Collections.emptyMap()));
}

public void testToXContent() throws Exception {
final HasPrivilegesResponse response = new HasPrivilegesResponse("daredevil", false,
Collections.singletonMap("manage", true),
Arrays.asList(
new HasPrivilegesResponse.ResourcePrivileges("staff",
MapBuilder.<String, Boolean>newMapBuilder(new LinkedHashMap<>())
.put("read", true).put("index", true).put("delete", false).put("manage", false).map()),
new HasPrivilegesResponse.ResourcePrivileges("customers",
MapBuilder.<String, Boolean>newMapBuilder(new LinkedHashMap<>())
.put("read", true).put("index", true).put("delete", true).put("manage", false).map())
), Collections.emptyMap());

final XContentBuilder builder = XContentBuilder.builder(XContentType.JSON.xContent());
response.toXContent(builder, ToXContent.EMPTY_PARAMS);
BytesReference bytes = BytesReference.bytes(builder);

final String json = bytes.utf8ToString();
assertThat(json, equalTo("{" +
"\"username\":\"daredevil\"," +
"\"has_all_requested\":false," +
"\"cluster\":{\"manage\":true}," +
"\"index\":{" +
"\"customers\":{\"read\":true,\"index\":true,\"delete\":true,\"manage\":false}," +
"\"staff\":{\"read\":true,\"index\":true,\"delete\":false,\"manage\":false}" +
"}," +
"\"application\":{}" +
"}"));
}

@Override
protected boolean supportsUnknownFields() {
// Because we have nested objects with { string : boolean }, unknown fields cause parsing problems
return false;
}

@Override
protected HasPrivilegesResponse createBlankInstance() {
return new HasPrivilegesResponse();
}

@Override
protected HasPrivilegesResponse createTestInstance() {
return randomResponse();
}

@Override
public org.elasticsearch.client.security.HasPrivilegesResponse doHlrcParseInstance(XContentParser parser) throws IOException {
return org.elasticsearch.client.security.HasPrivilegesResponse.fromXContent(parser);
}

@Override
public HasPrivilegesResponse convertHlrcToInternal(org.elasticsearch.client.security.HasPrivilegesResponse hlrc) {
return new HasPrivilegesResponse(
hlrc.getUsername(),
hlrc.hasAllRequested(),
hlrc.getClusterPrivileges(),
toResourcePrivileges(hlrc.getIndexPrivileges()),
hlrc.getApplicationPrivileges().entrySet().stream()
.collect(Collectors.toMap(Map.Entry::getKey, e -> toResourcePrivileges(e.getValue())))
);
}

private static List<HasPrivilegesResponse.ResourcePrivileges> toResourcePrivileges(Map<String, Map<String, Boolean>> map) {
return map.entrySet().stream()
.map(e -> new HasPrivilegesResponse.ResourcePrivileges(e.getKey(), e.getValue()))
.collect(Collectors.toList());
}

private HasPrivilegesResponse serializeAndDeserialize(HasPrivilegesResponse original, Version version) throws IOException {
logger.info("Test serialize/deserialize with version {}", version);
final BytesStreamOutput out = new BytesStreamOutput();
Expand All @@ -60,6 +140,7 @@ private HasPrivilegesResponse serializeAndDeserialize(HasPrivilegesResponse orig
}

private HasPrivilegesResponse randomResponse() {
final String username = randomAlphaOfLengthBetween(4, 12);
final Map<String, Boolean> cluster = new HashMap<>();
for (String priv : randomArray(1, 6, String[]::new, () -> randomAlphaOfLengthBetween(3, 12))) {
cluster.put(priv, randomBoolean());
Expand All @@ -69,7 +150,7 @@ private HasPrivilegesResponse randomResponse() {
for (String app : randomArray(1, 3, String[]::new, () -> randomAlphaOfLengthBetween(3, 6).toLowerCase(Locale.ROOT))) {
application.put(app, randomResourcePrivileges());
}
return new HasPrivilegesResponse(randomBoolean(), cluster, index, application);
return new HasPrivilegesResponse(username, randomBoolean(), cluster, index, application);
}

private Collection<HasPrivilegesResponse.ResourcePrivileges> randomResourcePrivileges() {
Expand All @@ -83,5 +164,4 @@ private Collection<HasPrivilegesResponse.ResourcePrivileges> randomResourcePrivi
}
return list;
}

}
2 changes: 1 addition & 1 deletion .../main/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ private void checkPrivileges(HasPrivilegesRequest request, Role userRole,
privilegesByApplication.put(applicationName, appPrivilegesByResource.values());
}

listener.onResponse(new HasPrivilegesResponse(allMatch, cluster, indices.values(), privilegesByApplication));
listener.onResponse(new HasPrivilegesResponse(request.username(), allMatch, cluster, indices.values(), privilegesByApplication));
}

private boolean testIndexMatch(String checkIndex, String checkPrivilegeName, Role userRole,
Expand Down
Loading