Skip to content

Commit

Permalink
User Profile - Detailed errors in hasPrivileges response (#89224)
Browse files Browse the repository at this point in the history
This PR adds a new `errors` field in the ProfilehasPrivileges response
to report detailed errors encountered, including missing UIDs. It also
removes the existing `errors_uids` field since this is redundant after
the change.
  • Loading branch information
ywangd committed Aug 18, 2022
1 parent 3bde177 commit 725367e
Show file tree
Hide file tree
Showing 11 changed files with 203 additions and 140 deletions.
5 changes: 5 additions & 0 deletions docs/changelog/89224.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 89224
summary: User Profile - Detailed errors in `hasPrivileges` response
area: Security
type: enhancement
issues: []
Original file line number Diff line number Diff line change
Expand Up @@ -68,14 +68,21 @@ Note that the `privileges` section above is identical to the
==== {api-response-body-title}

A successful has privileges user profile API call returns a JSON structure that contains
two list fields:
two fields:

`has_privilege_uids`:: (list) The subset of the requested profile IDs of the users that have
**all** the requested privileges.

`error_uids`:: (list) The subset of the requested profile IDs for which an error was
encountered. It does **not** include the missing profile IDs or the profile IDs of
the users that do not have all the requested privileges. This field is absent if empty.
`errors`:: (object) Errors encountered while fulfilling the request. This field is absent if there is no error.
It does **not** include the profile IDs of the users that do not have all the requested privileges.
+
.Properties of objects in `errors`
[%collapsible%open]
====
`count`:: (number) Total number of errors
`details`:: (object) The detailed error report with keys being profile IDs and values being the exact errors.
====

[[security-api-has-privileges-user-profile-example]]
==== {api-examples-title}
Expand All @@ -87,7 +94,11 @@ requested set of cluster, index, and application privileges:
--------------------------------------------------
POST /_security/user/_has_privileges
{
"uids": ["u_LQPnxDxEjIH0GOUoFkZr5Y57YUwSkL9Joiq-g4OCbPc_0", "u_rzRnxDgEHIH0GOUoFkZr5Y27YUwSk19Joiq=g4OCxxB_1"],
"uids": [
"u_LQPnxDxEjIH0GOUoFkZr5Y57YUwSkL9Joiq-g4OCbPc_0",
"u_rzRnxDgEHIH0GOUoFkZr5Y27YUwSk19Joiq=g4OCxxB_1",
"u_does-not-exist_0"
],
"cluster": [ "monitor", "create_snapshot", "manage_ml" ],
"index" : [
{
Expand All @@ -110,12 +121,22 @@ POST /_security/user/_has_privileges
--------------------------------------------------
// TEST[skip:TODO setup and tests will be possible once the profile uid is predictable]

The following example output indicates that only one of the two users has all the privileges:
The following example output indicates that only one of the three users has all the privileges
and one of them is not found:

[source,js]
--------------------------------------------------
{
"has_privilege_uids": ["u_rzRnxDgEHIH0GOUoFkZr5Y27YUwSk19Joiq=g4OCxxB_1"]
"has_privilege_uids": ["u_rzRnxDgEHIH0GOUoFkZr5Y27YUwSk19Joiq=g4OCxxB_1"],
"errors": {
"count": 1,
"details": {
"u_does-not-exist_0": {
"type": "resource_not_found_exception",
"reason": "profile document not found"
}
}
}
}
--------------------------------------------------
// NOTCONSOLE
Original file line number Diff line number Diff line change
Expand Up @@ -12,66 +12,68 @@
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.xcontent.ToXContentObject;
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xpack.core.security.xcontent.XContentUtils;

import java.io.IOException;
import java.util.Map;
import java.util.Objects;
import java.util.Set;

public class ProfileHasPrivilegesResponse extends ActionResponse implements ToXContentObject {

private Set<String> hasPrivilegeUids;
private Set<String> errorUids;
private final Map<String, Exception> errors;

public ProfileHasPrivilegesResponse(StreamInput in) throws IOException {
super(in);
this.hasPrivilegeUids = in.readSet(StreamInput::readString);
this.errorUids = in.readSet(StreamInput::readString);
this.errors = in.readMap(StreamInput::readString, StreamInput::readException);
}

public ProfileHasPrivilegesResponse(Set<String> hasPrivilegeUids, Set<String> errorUids) {
public ProfileHasPrivilegesResponse(Set<String> hasPrivilegeUids, Map<String, Exception> errors) {
super();
this.hasPrivilegeUids = Objects.requireNonNull(hasPrivilegeUids);
this.errorUids = Objects.requireNonNull(errorUids);
this.errors = Objects.requireNonNull(errors);
}

public Set<String> hasPrivilegeUids() {
return hasPrivilegeUids;
}

public Set<String> errorUids() {
return errorUids;
public Map<String, Exception> errors() {
return errors;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
ProfileHasPrivilegesResponse that = (ProfileHasPrivilegesResponse) o;
return hasPrivilegeUids.equals(that.hasPrivilegeUids) && errorUids.equals(that.errorUids);
// Only compare the keys (profile uids) of the errors, actual error types do not matter
return hasPrivilegeUids.equals(that.hasPrivilegeUids) && errors.keySet().equals(that.errors.keySet());
}

@Override
public int hashCode() {
return Objects.hash(hasPrivilegeUids, errorUids);
// Only include the keys (profile uids) of the errors, actual error types do not matter
return Objects.hash(hasPrivilegeUids, errors.keySet());
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
XContentBuilder xContentBuilder = builder.startObject().stringListField("has_privilege_uids", hasPrivilegeUids);
if (false == errorUids.isEmpty()) {
xContentBuilder.stringListField("error_uids", errorUids);
}
XContentUtils.maybeAddErrorDetails(builder, errors);
return xContentBuilder.endObject();
}

@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeStringCollection(hasPrivilegeUids);
out.writeStringCollection(errorUids);
out.writeMap(errors, StreamOutput::writeString, StreamOutput::writeException);
}

@Override
public String toString() {
return getClass().getSimpleName() + "{" + "has_privilege_uids=" + hasPrivilegeUids + ", error_uids=" + errorUids + "}";
return getClass().getSimpleName() + "{" + "has_privilege_uids=" + hasPrivilegeUids + ", errors=" + errors + "}";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,22 +7,30 @@

package org.elasticsearch.xpack.core.security.action.profile;

import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.ResourceNotFoundException;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.test.AbstractWireSerializingTestCase;
import org.elasticsearch.xcontent.ToXContent;
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xcontent.XContentFactory;
import org.elasticsearch.xcontent.json.JsonXContent;
import org.elasticsearch.xpack.core.security.action.user.ProfileHasPrivilegesResponse;

import java.io.IOException;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.TreeMap;
import java.util.function.Supplier;
import java.util.stream.IntStream;

import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasEntry;

public class ProfileHasPrivilegesResponseTests extends AbstractWireSerializingTestCase<ProfileHasPrivilegesResponse> {

Expand All @@ -35,16 +43,19 @@ protected Writeable.Reader<ProfileHasPrivilegesResponse> instanceReader() {
protected ProfileHasPrivilegesResponse createTestInstance() {
return new ProfileHasPrivilegesResponse(
randomUnique(() -> randomAlphaOfLengthBetween(0, 5), randomIntBetween(0, 5)),
randomUnique(() -> randomAlphaOfLengthBetween(0, 5), randomIntBetween(0, 5))
randomErrors()
);
}

@Override
protected ProfileHasPrivilegesResponse mutateInstance(ProfileHasPrivilegesResponse instance) throws IOException {
return randomFrom(
new ProfileHasPrivilegesResponse(newMutatedSet(instance.hasPrivilegeUids()), instance.errorUids()),
new ProfileHasPrivilegesResponse(instance.hasPrivilegeUids(), newMutatedSet(instance.errorUids())),
new ProfileHasPrivilegesResponse(newMutatedSet(instance.hasPrivilegeUids()), newMutatedSet(instance.errorUids()))
new ProfileHasPrivilegesResponse(newMutatedSet(instance.hasPrivilegeUids()), instance.errors()),
new ProfileHasPrivilegesResponse(instance.hasPrivilegeUids(), randomValueOtherThan(instance.errors(), this::randomErrors)),
new ProfileHasPrivilegesResponse(
newMutatedSet(instance.hasPrivilegeUids()),
randomValueOtherThan(instance.errors(), this::randomErrors)
)
);
}

Expand All @@ -55,20 +66,47 @@ public void testToXContent() throws IOException {
final Map<String, Object> responseMap = XContentHelper.convertToMap(BytesReference.bytes(builder), false, builder.contentType())
.v2();

if (response.errorUids().isEmpty()) {
if (response.errors().isEmpty()) {
assertThat(responseMap, equalTo(Map.of("has_privilege_uids", new ArrayList<>(response.hasPrivilegeUids()))));
} else {
assertThat(
responseMap,
equalTo(
Map.of(
"has_privilege_uids",
new ArrayList<>(response.hasPrivilegeUids()),
"error_uids",
new ArrayList<>(response.errorUids())
)
)
);
assertThat(responseMap, hasEntry("has_privilege_uids", List.copyOf(response.hasPrivilegeUids())));
@SuppressWarnings("unchecked")
final Map<String, Object> errorsMap = (Map<String, Object>) responseMap.get("errors");
assertThat(errorsMap.get("count"), equalTo(response.errors().size()));
@SuppressWarnings("unchecked")
final Map<String, Object> detailsMap = (Map<String, Object>) errorsMap.get("details");
assertThat(detailsMap.keySet(), equalTo(response.errors().keySet()));

detailsMap.forEach((k, v) -> {
final String errorString;
final Exception e = response.errors().get(k);
if (e instanceof IllegalArgumentException illegalArgumentException) {
errorString = """
{
"type": "illegal_argument_exception",
"reason": "%s"
}""".formatted(illegalArgumentException.getMessage());
} else if (e instanceof ResourceNotFoundException resourceNotFoundException) {
errorString = """
{
"type": "resource_not_found_exception",
"reason": "%s"
}""".formatted(resourceNotFoundException.getMessage());
} else if (e instanceof ElasticsearchException elasticsearchException) {
errorString = """
{
"type": "exception",
"reason": "%s",
"caused_by": {
"type": "illegal_argument_exception",
"reason": "%s"
}
}""".formatted(elasticsearchException.getMessage(), elasticsearchException.getCause().getMessage());
} else {
throw new IllegalArgumentException("unknown exception type: " + e);
}
assertThat(v, equalTo(XContentHelper.convertToMap(JsonXContent.jsonXContent, errorString, false)));
});
}
}

Expand All @@ -86,4 +124,15 @@ private Set<String> newMutatedSet(Set<String> in) {
}
return mutated;
}

private Map<String, Exception> randomErrors() {
final Map<String, Exception> errors = new TreeMap<>();
final Supplier<Exception> randomExceptionSupplier = () -> randomFrom(
new IllegalArgumentException(randomAlphaOfLengthBetween(0, 18)),
new ResourceNotFoundException(randomAlphaOfLengthBetween(0, 18)),
new ElasticsearchException(randomAlphaOfLengthBetween(0, 18), new IllegalArgumentException(randomAlphaOfLengthBetween(0, 18)))
);
IntStream.range(0, randomIntBetween(0, 3)).forEach(i -> errors.put(randomAlphaOfLength(20) + i, randomExceptionSupplier.get()));
return errors;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,19 @@ public void testProfileHasPrivileges() throws IOException {
final Response profileHasPrivilegesResponse = adminClient().performRequest(profileHasPrivilegesRequest);
assertOK(profileHasPrivilegesResponse);
Map<String, Object> profileHasPrivilegesResponseMap = responseAsMap(profileHasPrivilegesResponse);
assertThat(profileHasPrivilegesResponseMap.keySet(), contains("has_privilege_uids"));
assertThat(profileHasPrivilegesResponseMap.keySet(), contains("has_privilege_uids", "errors"));
assertThat(((List<String>) profileHasPrivilegesResponseMap.get("has_privilege_uids")), contains(profileUid));
assertThat(
profileHasPrivilegesResponseMap.get("errors"),
equalTo(
Map.of(
"count",
1,
"details",
Map.of("some_missing_profile", Map.of("type", "resource_not_found_exception", "reason", "profile document not found"))
)
)
);
}

public void testGetProfiles() throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -586,7 +586,7 @@ public void testProfileAPIsWhenIndexNotCreated() {
)
).actionGet();
assertThat(profileHasPrivilegesResponse.hasPrivilegeUids(), emptyIterable());
assertThat(profileHasPrivilegesResponse.errorUids(), emptyIterable());
assertThat(profileHasPrivilegesResponse.errors(), anEmptyMap());

// Ensure index does not exist
assertThat(getProfileIndexResponse().getIndices(), not(hasItemInArray(INTERNAL_SECURITY_PROFILE_INDEX_8)));
Expand Down Expand Up @@ -650,7 +650,7 @@ public void testSetEnabled() {
)
).actionGet();
assertThat(profileHasPrivilegesResponse.hasPrivilegeUids(), emptyIterable());
assertThat(profileHasPrivilegesResponse.errorUids(), emptyIterable());
assertThat(profileHasPrivilegesResponse.errors(), anEmptyMap());

// Enable again for search
final SetProfileEnabledRequest setProfileEnabledRequest2 = new SetProfileEnabledRequest(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.stream.Collectors;

Expand Down Expand Up @@ -75,20 +76,18 @@ public TransportProfileHasPrivilegesAction(
protected void doExecute(Task task, ProfileHasPrivilegesRequest request, ActionListener<ProfileHasPrivilegesResponse> listener) {
assert task instanceof CancellableTask : "task must be cancellable";
profileService.getProfileSubjects(request.profileUids(), ActionListener.wrap(profileSubjectsAndFailures -> {
if (profileSubjectsAndFailures.profileUidToSubject().isEmpty()) {
listener.onResponse(new ProfileHasPrivilegesResponse(Set.of(), profileSubjectsAndFailures.failureProfileUids()));
if (profileSubjectsAndFailures.results().isEmpty()) {
listener.onResponse(new ProfileHasPrivilegesResponse(Set.of(), profileSubjectsAndFailures.errors()));
return;
}
final Set<String> hasPrivilegeProfiles = Collections.synchronizedSet(new HashSet<>());
final Set<String> errorProfiles = Collections.synchronizedSet(new HashSet<>(profileSubjectsAndFailures.failureProfileUids()));
final Collection<Map.Entry<String, Subject>> profileUidAndSubjects = profileSubjectsAndFailures.profileUidToSubject()
.entrySet();
final AtomicInteger counter = new AtomicInteger(profileUidAndSubjects.size());
final Map<String, Exception> errorProfiles = new ConcurrentHashMap<>(profileSubjectsAndFailures.errors());
final AtomicInteger counter = new AtomicInteger(profileSubjectsAndFailures.results().size());
assert counter.get() > 0;
resolveApplicationPrivileges(
request,
ActionListener.wrap(applicationPrivilegeDescriptors -> threadPool.generic().execute(() -> {
for (Map.Entry<String, Subject> profileUidToSubject : profileUidAndSubjects) {
for (Map.Entry<String, Subject> profileUidToSubject : profileSubjectsAndFailures.results()) {
// return the partial response if the "has privilege" task got cancelled in the meantime
if (((CancellableTask) task).isCancelled()) {
listener.onFailure(new TaskCancelledException("has privilege task cancelled"));
Expand All @@ -107,7 +106,7 @@ protected void doExecute(Task task, ProfileHasPrivilegesRequest request, ActionL
}
}, checkPrivilegesException -> {
logger.debug(() -> "Failed to check privileges for profile [" + profileUid + "]", checkPrivilegesException);
errorProfiles.add(profileUid);
errorProfiles.put(profileUid, checkPrivilegesException);
}), () -> {
if (counter.decrementAndGet() == 0) {
listener.onResponse(new ProfileHasPrivilegesResponse(hasPrivilegeProfiles, errorProfiles));
Expand Down

0 comments on commit 725367e

Please sign in to comment.