From 989419a40b65d1d08684dba3ffa7ab057e3d66c0 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Wed, 10 Aug 2022 14:28:46 +1000 Subject: [PATCH 1/4] User Profile - Detailed errors in hasPrivileges response 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. --- .../has-privileges-user-profile.asciidoc | 35 ++++++-- .../user/ProfileHasPrivilegesResponse.java | 28 +++--- .../ProfileHasPrivilegesResponseTests.java | 81 +++++++++++++---- .../xpack/security/profile/ProfileIT.java | 13 ++- .../security/profile/ProfileIntegTests.java | 4 +- .../TransportProfileHasPrivilegesAction.java | 15 ++-- .../security/profile/ProfileService.java | 34 ++----- ...nsportProfileHasPrivilegesActionTests.java | 29 +++--- .../security/profile/ProfileServiceTests.java | 90 ++++++++----------- .../test/user_profile/40_has_privileges.yml | 9 ++ 10 files changed, 198 insertions(+), 140 deletions(-) diff --git a/x-pack/docs/en/rest-api/security/has-privileges-user-profile.asciidoc b/x-pack/docs/en/rest-api/security/has-privileges-user-profile.asciidoc index de0524ea4cac2..cb6401fb4cf73 100644 --- a/x-pack/docs/en/rest-api/security/has-privileges-user-profile.asciidoc +++ b/x-pack/docs/en/rest-api/security/has-privileges-user-profile.asciidoc @@ -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} @@ -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" : [ { @@ -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_doesnotexist_0": { + "type": "resource_not_found_exception", + "reason": "profile document not found" + } + } + } } -------------------------------------------------- // NOTCONSOLE diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/user/ProfileHasPrivilegesResponse.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/user/ProfileHasPrivilegesResponse.java index 1fbe31c6ae9c9..0f2380f1c0fb2 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/user/ProfileHasPrivilegesResponse.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/user/ProfileHasPrivilegesResponse.java @@ -12,34 +12,36 @@ 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 hasPrivilegeUids; - private Set errorUids; + private final Map 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 hasPrivilegeUids, Set errorUids) { + public ProfileHasPrivilegesResponse(Set hasPrivilegeUids, Map errors) { super(); this.hasPrivilegeUids = Objects.requireNonNull(hasPrivilegeUids); - this.errorUids = Objects.requireNonNull(errorUids); + this.errors = Objects.requireNonNull(errors); } public Set hasPrivilegeUids() { return hasPrivilegeUids; } - public Set errorUids() { - return errorUids; + public Map errors() { + return errors; } @Override @@ -47,31 +49,31 @@ 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 + "}"; } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/profile/ProfileHasPrivilegesResponseTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/profile/ProfileHasPrivilegesResponseTests.java index 08ad8d06698ff..67e18206cdcb8 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/profile/ProfileHasPrivilegesResponseTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/profile/ProfileHasPrivilegesResponseTests.java @@ -7,6 +7,8 @@ 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; @@ -14,15 +16,21 @@ 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 { @@ -35,16 +43,19 @@ protected Writeable.Reader 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) + ) ); } @@ -55,20 +66,47 @@ public void testToXContent() throws IOException { final Map 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 errorsMap = (Map) responseMap.get("errors"); + assertThat(errorsMap.get("count"), equalTo(response.errors().size())); + @SuppressWarnings("unchecked") + final Map detailsMap = (Map) 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))); + }); } } @@ -86,4 +124,15 @@ private Set newMutatedSet(Set in) { } return mutated; } + + private Map randomErrors() { + final Map errors = new TreeMap<>(); + final Supplier 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; + } } diff --git a/x-pack/plugin/security/qa/profile/src/javaRestTest/java/org/elasticsearch/xpack/security/profile/ProfileIT.java b/x-pack/plugin/security/qa/profile/src/javaRestTest/java/org/elasticsearch/xpack/security/profile/ProfileIT.java index be60db248fede..8644529fb859e 100644 --- a/x-pack/plugin/security/qa/profile/src/javaRestTest/java/org/elasticsearch/xpack/security/profile/ProfileIT.java +++ b/x-pack/plugin/security/qa/profile/src/javaRestTest/java/org/elasticsearch/xpack/security/profile/ProfileIT.java @@ -115,8 +115,19 @@ public void testProfileHasPrivileges() throws IOException { final Response profileHasPrivilegesResponse = adminClient().performRequest(profileHasPrivilegesRequest); assertOK(profileHasPrivilegesResponse); Map profileHasPrivilegesResponseMap = responseAsMap(profileHasPrivilegesResponse); - assertThat(profileHasPrivilegesResponseMap.keySet(), contains("has_privilege_uids")); + assertThat(profileHasPrivilegesResponseMap.keySet(), contains("has_privilege_uids", "errors")); assertThat(((List) 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 { diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/profile/ProfileIntegTests.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/profile/ProfileIntegTests.java index b7ccd0b1698fe..b3d25cd210850 100644 --- a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/profile/ProfileIntegTests.java +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/profile/ProfileIntegTests.java @@ -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))); @@ -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( diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/profile/TransportProfileHasPrivilegesAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/profile/TransportProfileHasPrivilegesAction.java index 66360dd4381ae..ebe84fffbd4b4 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/profile/TransportProfileHasPrivilegesAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/profile/TransportProfileHasPrivilegesAction.java @@ -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; @@ -75,20 +76,18 @@ public TransportProfileHasPrivilegesAction( protected void doExecute(Task task, ProfileHasPrivilegesRequest request, ActionListener 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 hasPrivilegeProfiles = Collections.synchronizedSet(new HashSet<>()); - final Set errorProfiles = Collections.synchronizedSet(new HashSet<>(profileSubjectsAndFailures.failureProfileUids())); - final Collection> profileUidAndSubjects = profileSubjectsAndFailures.profileUidToSubject() - .entrySet(); - final AtomicInteger counter = new AtomicInteger(profileUidAndSubjects.size()); + final Map 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 profileUidToSubject : profileUidAndSubjects) { + for (Map.Entry 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")); @@ -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)); diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/profile/ProfileService.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/profile/ProfileService.java index 6c13806241081..278c570b04149 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/profile/ProfileService.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/profile/ProfileService.java @@ -11,7 +11,6 @@ import org.apache.logging.log4j.Logger; import org.apache.lucene.search.TotalHits; import org.elasticsearch.ElasticsearchException; -import org.elasticsearch.ExceptionsHelper; import org.elasticsearch.ResourceNotFoundException; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.DocWriteRequest; @@ -150,19 +149,21 @@ public void getProfiles(List uids, Set dataKeys, ActionListener< })); } - public void getProfileSubjects(Collection uids, ActionListener listener) { + public void getProfileSubjects(Collection uids, ActionListener>> listener) { getVersionedDocuments(uids, listener.map(resultsAndErrors -> { if (resultsAndErrors != null) { - return new MultiProfileSubjectResponse( + // convert the list of profile document to a list of "uid to subject" entries + return new ResultsAndErrors<>( resultsAndErrors.results() .stream() .map(VersionedDocument::doc) .filter(ProfileDocument::enabled) - .collect(Collectors.toMap(ProfileDocument::uid, profileDoc -> profileDoc.user().toSubject())), - Set.copyOf(errorUidsExcludingNotFound(resultsAndErrors.errors())) + .map(doc -> Map.entry(doc.uid(), doc.user().toSubject())) + .toList(), + resultsAndErrors.errors() ); } else { - return new MultiProfileSubjectResponse(Map.of(), Set.of()); + return new ResultsAndErrors<>(List.of(), Map.of()); } })); } @@ -386,6 +387,7 @@ private void getVersionedDocuments(Collection uids, ActionListener uids, ActionListener resultsAndErrors = new ResultsAndErrors<>(retrievedDocs, errors); - if (logger.isDebugEnabled() && false == resultsAndErrors.errors().isEmpty()) { - Exception loggedException = null; - final List errorUids = errorUidsExcludingNotFound(resultsAndErrors.errors()); - for (String uid : errorUids) { - loggedException = ExceptionsHelper.useOrSuppress(loggedException, resultsAndErrors.errors().get(uid)); - } - if (loggedException != null) { - logger.debug(() -> format("Failed to retrieve profiles %s", errorUids), loggedException); - } - } listener.onResponse(resultsAndErrors); }, listener::onFailure)) ); @@ -839,14 +831,6 @@ private static ProfileDocument updateWithSubject(ProfileDocument doc, Subject su ); } - private static List errorUidsExcludingNotFound(Map errors) { - return errors.entrySet() - .stream() - .filter(entry -> entry.getValue() != null && false == entry.getValue() instanceof ResourceNotFoundException) - .map(Map.Entry::getKey) - .toList(); - } - // Package private for testing record VersionedDocument(ProfileDocument doc, long primaryTerm, long seqNo) { @@ -874,6 +858,4 @@ Profile toProfile(Set dataKeys) { } } - - public record MultiProfileSubjectResponse(Map profileUidToSubject, Set failureProfileUids) {} } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/profile/TransportProfileHasPrivilegesActionTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/profile/TransportProfileHasPrivilegesActionTests.java index 1e5feb22b4697..ed01d0ca6fc40 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/profile/TransportProfileHasPrivilegesActionTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/profile/TransportProfileHasPrivilegesActionTests.java @@ -20,6 +20,7 @@ import org.elasticsearch.threadpool.TestThreadPool; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; +import org.elasticsearch.xpack.core.common.ResultsAndErrors; import org.elasticsearch.xpack.core.security.SecurityContext; import org.elasticsearch.xpack.core.security.action.user.ProfileHasPrivilegesRequest; import org.elasticsearch.xpack.core.security.action.user.ProfileHasPrivilegesResponse; @@ -32,7 +33,6 @@ import org.elasticsearch.xpack.security.authz.AuthorizationService; import org.elasticsearch.xpack.security.authz.store.NativePrivilegeStore; import org.elasticsearch.xpack.security.profile.ProfileService; -import org.elasticsearch.xpack.security.profile.ProfileService.MultiProfileSubjectResponse; import org.junit.After; import org.junit.Before; @@ -45,6 +45,8 @@ import java.util.Set; import java.util.concurrent.ExecutionException; import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Function; +import java.util.stream.Collectors; import static org.elasticsearch.test.ActionListenerUtils.anyActionListener; import static org.elasticsearch.xpack.core.security.action.profile.ProfileHasPrivilegesRequestTests.randomValidPrivilegesToCheckRequest; @@ -53,6 +55,7 @@ import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.emptyIterable; +import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.instanceOf; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.notNullValue; @@ -117,9 +120,8 @@ public void testMultipleConcurrentCheckPrivileges() throws Exception { for (String uid : uidsArg) { profileUidToSubject.put(uid, new Subject(new User("user_for_profile_" + uid), mock(Authentication.RealmRef.class))); } - final ActionListener listener = (ActionListener) invocation - .getArguments()[1]; - listener.onResponse(new MultiProfileSubjectResponse(profileUidToSubject, Set.of())); + final var listener = (ActionListener>>) invocation.getArguments()[1]; + listener.onResponse(new ResultsAndErrors<>(profileUidToSubject.entrySet(), Map.of())); return null; }).when(profileService).getProfileSubjects(anyCollection(), anyActionListener()); @@ -152,7 +154,7 @@ public void testMultipleConcurrentCheckPrivileges() throws Exception { transportProfileHasPrivilegesAction.doExecute(mock(CancellableTask.class), request, listener); ProfileHasPrivilegesResponse response = listener.get(); - assertThat(response.errorUids(), is(errorProfileUids)); + assertThat(response.errors().keySet(), equalTo(errorProfileUids)); Set hasPrivilegeUids = new HashSet<>(allProfileUids); hasPrivilegeUids.removeAll(errorProfileUids); hasPrivilegeUids.removeAll(noPrivilegesProfileUids); @@ -170,9 +172,13 @@ public void testNoProfileSubjectsFound() throws Exception { ); doAnswer(invocation -> { - final ActionListener listener = (ActionListener) invocation - .getArguments()[1]; - listener.onResponse(new MultiProfileSubjectResponse(Map.of(), errorProfileUids)); + final var listener = (ActionListener>>) invocation.getArguments()[1]; + listener.onResponse( + new ResultsAndErrors<>( + List.of(), + errorProfileUids.stream().collect(Collectors.toMap(Function.identity(), uid -> mock(Exception.class))) + ) + ); return null; }).when(profileService).getProfileSubjects(anyCollection(), anyActionListener()); @@ -195,7 +201,7 @@ public void testNoProfileSubjectsFound() throws Exception { ProfileHasPrivilegesResponse response = listener.get(); assertThat(response.hasPrivilegeUids(), emptyIterable()); - assertThat(response.errorUids(), is(errorProfileUids)); + assertThat(response.errors().keySet(), equalTo(errorProfileUids)); } public void testDLSQueryIndicesPrivilegesRequestValidation() { @@ -236,9 +242,8 @@ public void testCancellation() throws Exception { for (String uid : uidsArg) { profileUidToSubject.put(uid, new Subject(new User("user_for_profile_" + uid), mock(Authentication.RealmRef.class))); } - final ActionListener listener = (ActionListener) invocation - .getArguments()[1]; - listener.onResponse(new MultiProfileSubjectResponse(profileUidToSubject, Set.of())); + final var listener = (ActionListener>>) invocation.getArguments()[1]; + listener.onResponse(new ResultsAndErrors<>(profileUidToSubject.entrySet(), Map.of())); return null; }).when(profileService).getProfileSubjects(anyCollection(), anyActionListener()); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/profile/ProfileServiceTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/profile/ProfileServiceTests.java index 5595f2550c1a6..d662aa723a50b 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/profile/ProfileServiceTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/profile/ProfileServiceTests.java @@ -7,10 +7,8 @@ package org.elasticsearch.xpack.security.profile; -import org.apache.logging.log4j.Level; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; import org.elasticsearch.ElasticsearchException; +import org.elasticsearch.ResourceNotFoundException; import org.elasticsearch.Version; import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.bulk.BulkAction; @@ -37,9 +35,9 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.hash.MessageDigests; -import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.Fuzziness; +import org.elasticsearch.common.util.set.Sets; import org.elasticsearch.core.Tuple; import org.elasticsearch.index.get.GetResult; import org.elasticsearch.index.query.BoolQueryBuilder; @@ -52,7 +50,6 @@ import org.elasticsearch.search.sort.SortOrder; import org.elasticsearch.tasks.TaskId; import org.elasticsearch.test.ESTestCase; -import org.elasticsearch.test.MockLogAppender; import org.elasticsearch.test.VersionUtils; import org.elasticsearch.threadpool.FixedExecutorBuilder; import org.elasticsearch.threadpool.TestThreadPool; @@ -87,7 +84,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.TreeSet; import java.util.concurrent.ExecutionException; import java.util.function.Consumer; import java.util.stream.Collectors; @@ -102,7 +98,6 @@ import static org.elasticsearch.xpack.security.support.SecuritySystemIndices.SECURITY_PROFILE_ALIAS; import static org.hamcrest.Matchers.anEmptyMap; import static org.hamcrest.Matchers.arrayWithSize; -import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.empty; import static org.hamcrest.Matchers.equalTo; @@ -271,29 +266,29 @@ public void testGetProfilesEmptyUids() { @SuppressWarnings("unchecked") public void testGetProfileSubjectsNoIndex() throws Exception { when(profileIndex.indexExists()).thenReturn(false); - PlainActionFuture future = new PlainActionFuture<>(); + PlainActionFuture>> future = new PlainActionFuture<>(); profileService.getProfileSubjects(randomList(1, 5, () -> randomAlphaOfLength(20)), future); - ProfileService.MultiProfileSubjectResponse multiProfileSubjectResponse = future.get(); - assertThat(multiProfileSubjectResponse.profileUidToSubject().size(), is(0)); - assertThat(multiProfileSubjectResponse.failureProfileUids().size(), is(0)); + ResultsAndErrors> resultsAndErrors = future.get(); + assertThat(resultsAndErrors.results().size(), is(0)); + assertThat(resultsAndErrors.errors().size(), is(0)); when(profileIndex.indexExists()).thenReturn(true); ElasticsearchException unavailableException = new ElasticsearchException("mock profile index unavailable"); when(profileIndex.isAvailable()).thenReturn(false); when(profileIndex.getUnavailableReason()).thenReturn(unavailableException); - PlainActionFuture future2 = new PlainActionFuture<>(); + PlainActionFuture>> future2 = new PlainActionFuture<>(); profileService.getProfileSubjects(randomList(1, 5, () -> randomAlphaOfLength(20)), future2); ExecutionException e = expectThrows(ExecutionException.class, () -> future2.get()); assertThat(e.getCause(), is(unavailableException)); - PlainActionFuture future3 = new PlainActionFuture<>(); + PlainActionFuture>> future3 = new PlainActionFuture<>(); profileService.getProfileSubjects(List.of(), future3); - multiProfileSubjectResponse = future3.get(); - assertThat(multiProfileSubjectResponse.profileUidToSubject().size(), is(0)); - assertThat(multiProfileSubjectResponse.failureProfileUids().size(), is(0)); + resultsAndErrors = future3.get(); + assertThat(resultsAndErrors.results().size(), is(0)); + assertThat(resultsAndErrors.errors().size(), is(0)); verify(profileIndex, never()).checkIndexVersionThenExecute(any(Consumer.class), any(Runnable.class)); } @SuppressWarnings("unchecked") - public void testGetProfileSubjectsWithMissingNoFailures() throws Exception { + public void testGetProfileSubjectsWithMissingUids() throws Exception { final Collection allProfileUids = randomList(1, 5, () -> randomAlphaOfLength(20)); final Collection missingProfileUids = randomSubsetOf(allProfileUids); doAnswer(invocation -> { @@ -338,14 +333,19 @@ public void testGetProfileSubjectsWithMissingNoFailures() throws Exception { return null; }).when(client).execute(eq(MultiGetAction.INSTANCE), any(MultiGetRequest.class), anyActionListener()); - final PlainActionFuture future = new PlainActionFuture<>(); + final PlainActionFuture>> future = new PlainActionFuture<>(); profileService.getProfileSubjects(allProfileUids, future); - ProfileService.MultiProfileSubjectResponse multiProfileSubjectResponse = future.get(); + ResultsAndErrors> resultsAndErrors = future.get(); verify(profileIndex).checkIndexVersionThenExecute(any(Consumer.class), any(Runnable.class)); - assertThat(multiProfileSubjectResponse.failureProfileUids().isEmpty(), is(true)); - assertThat(multiProfileSubjectResponse.profileUidToSubject().size(), is(allProfileUids.size() - missingProfileUids.size())); - for (Map.Entry profileIdAndSubject : multiProfileSubjectResponse.profileUidToSubject().entrySet()) { + assertThat(resultsAndErrors.errors().size(), equalTo(missingProfileUids.size())); + resultsAndErrors.errors().forEach((uid, e) -> { + assertThat(missingProfileUids, hasItem(uid)); + assertThat(e, instanceOf(ResourceNotFoundException.class)); + }); + + assertThat(resultsAndErrors.results().size(), is(allProfileUids.size() - missingProfileUids.size())); + for (Map.Entry profileIdAndSubject : resultsAndErrors.results()) { assertThat(allProfileUids, hasItem(profileIdAndSubject.getKey())); assertThat(missingProfileUids, not(hasItem(profileIdAndSubject.getKey()))); assertThat(profileIdAndSubject.getValue().getUser().principal(), is("foo_username_" + profileIdAndSubject.getKey())); @@ -366,29 +366,13 @@ public void testGetProfileSubjectWithFailures() throws Exception { listener.onFailure(mGetException); return null; }).when(client).execute(eq(MultiGetAction.INSTANCE), any(MultiGetRequest.class), anyActionListener()); - final PlainActionFuture future = new PlainActionFuture<>(); + final PlainActionFuture>> future = new PlainActionFuture<>(); profileService.getProfileSubjects(randomList(1, 5, () -> randomAlphaOfLength(20)), future); ExecutionException e = expectThrows(ExecutionException.class, () -> future.get()); assertThat(e.getCause(), is(mGetException)); - final Collection missingProfileUids = randomList(1, 5, () -> randomAlphaOfLength(20)); - final Collection errorProfileUids = randomSubsetOf(missingProfileUids); - final MockLogAppender mockLogAppender = new MockLogAppender(); - if (false == errorProfileUids.isEmpty()) { - mockLogAppender.addExpectation( - new MockLogAppender.SeenEventExpectation( - "message", - "org.elasticsearch.xpack.security.profile.ProfileService", - Level.DEBUG, - "Failed to retrieve profiles " - + missingProfileUids.stream() - .filter(v -> errorProfileUids.contains(v)) - .collect(Collectors.toCollection(TreeSet::new)) - ) - ); - } - mockLogAppender.start(); - final Logger logger = LogManager.getLogger(ProfileService.class); - Loggers.setLevel(logger, Level.DEBUG); + final Collection allProfileUids = randomList(1, 5, () -> randomAlphaOfLength(20)); + final Collection errorProfileUids = randomSubsetOf(allProfileUids); + final Collection missingProfileUids = Sets.difference(Set.copyOf(allProfileUids), Set.copyOf(errorProfileUids)); doAnswer(invocation -> { assertThat( threadPool.getThreadContext().getTransient(ACTION_ORIGIN_TRANSIENT_NAME), @@ -416,19 +400,15 @@ public void testGetProfileSubjectWithFailures() throws Exception { return null; }).when(client).execute(eq(MultiGetAction.INSTANCE), any(MultiGetRequest.class), anyActionListener()); - try { - Loggers.addAppender(logger, mockLogAppender); - final PlainActionFuture future2 = new PlainActionFuture<>(); - profileService.getProfileSubjects(missingProfileUids, future2); - - ProfileService.MultiProfileSubjectResponse multiProfileSubjectResponse = future2.get(); - assertThat(multiProfileSubjectResponse.profileUidToSubject().isEmpty(), is(true)); - assertThat(multiProfileSubjectResponse.failureProfileUids(), containsInAnyOrder(errorProfileUids.toArray(String[]::new))); - mockLogAppender.assertAllExpectationsMatched(); - } finally { - Loggers.removeAppender(logger, mockLogAppender); - mockLogAppender.stop(); - } + final PlainActionFuture>> future2 = new PlainActionFuture<>(); + profileService.getProfileSubjects(allProfileUids, future2); + + ResultsAndErrors> resultsAndErrors = future2.get(); + assertThat(resultsAndErrors.results().isEmpty(), is(true)); + assertThat(resultsAndErrors.errors().size(), equalTo(allProfileUids.size())); + assertThat(resultsAndErrors.errors().keySet(), equalTo(Set.copyOf(allProfileUids))); + missingProfileUids.forEach(uid -> assertThat(resultsAndErrors.errors().get(uid), instanceOf(ResourceNotFoundException.class))); + errorProfileUids.forEach(uid -> assertThat(resultsAndErrors.errors().get(uid), instanceOf(ElasticsearchException.class))); } public void testActivateProfileShouldFailIfSubjectTypeIsNotUser() { diff --git a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/user_profile/40_has_privileges.yml b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/user_profile/40_has_privileges.yml index 5f017c223243f..24ef7fdd10478 100644 --- a/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/user_profile/40_has_privileges.yml +++ b/x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/user_profile/40_has_privileges.yml @@ -137,6 +137,10 @@ teardown: - length: { has_privilege_uids: 2 } - match: { has_privilege_uids.0 : "/^(${profile_uid1}|${profile_uid2})$/" } - match: { has_privilege_uids.1 : "/^(${profile_uid1}|${profile_uid2})$/" } + - match: { errors.count: 1 } + - length: { errors.details: 1 } + - is_true: errors.details.dummy_missing + - match: { errors.details.dummy_missing.type: "resource_not_found_exception" } - do: security.has_privileges_user_profile: @@ -153,6 +157,7 @@ teardown: - all - read - length: { "has_privilege_uids": 0 } + - is_false: errors - do: security.has_privileges_user_profile: @@ -174,3 +179,7 @@ teardown: - read - length: { "has_privilege_uids": 1 } - match: { "has_privilege_uids.0" : $profile_uid1 } + - match: { errors.count: 1 } + - length: { errors.details: 1 } + - is_true: errors.details.dummy + - match: { errors.details.dummy.type: "resource_not_found_exception" } From 6ad9868ebd782c6a3c95a85f9230ef6cf65d3380 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Wed, 10 Aug 2022 14:32:07 +1000 Subject: [PATCH 2/4] tweak --- .../en/rest-api/security/has-privileges-user-profile.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/docs/en/rest-api/security/has-privileges-user-profile.asciidoc b/x-pack/docs/en/rest-api/security/has-privileges-user-profile.asciidoc index cb6401fb4cf73..a934dc80b3586 100644 --- a/x-pack/docs/en/rest-api/security/has-privileges-user-profile.asciidoc +++ b/x-pack/docs/en/rest-api/security/has-privileges-user-profile.asciidoc @@ -131,7 +131,7 @@ and one of them is not found: "errors": { "count": 1, "details": { - "u_doesnotexist_0": { + "u_does-not-exist_0": { "type": "resource_not_found_exception", "reason": "profile document not found" } From a222f3aa5a44b54a7aced17597d0bbca8c35f73f Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Wed, 10 Aug 2022 14:32:38 +1000 Subject: [PATCH 3/4] Update docs/changelog/89224.yaml --- docs/changelog/89224.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 docs/changelog/89224.yaml diff --git a/docs/changelog/89224.yaml b/docs/changelog/89224.yaml new file mode 100644 index 0000000000000..8704eb3717aec --- /dev/null +++ b/docs/changelog/89224.yaml @@ -0,0 +1,5 @@ +pr: 89224 +summary: User Profile - Detailed errors in `hasPrivileges` response +area: Security +type: enhancement +issues: [] From 21dfbffc78e006ff54538974633bd99fd2ce06df Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Wed, 10 Aug 2022 14:32:58 +1000 Subject: [PATCH 4/4] more tweak --- .../en/rest-api/security/has-privileges-user-profile.asciidoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/docs/en/rest-api/security/has-privileges-user-profile.asciidoc b/x-pack/docs/en/rest-api/security/has-privileges-user-profile.asciidoc index a934dc80b3586..3bb4a19787952 100644 --- a/x-pack/docs/en/rest-api/security/has-privileges-user-profile.asciidoc +++ b/x-pack/docs/en/rest-api/security/has-privileges-user-profile.asciidoc @@ -81,7 +81,7 @@ It does **not** include the profile IDs of the users that do not have all the re ==== `count`:: (number) Total number of errors -``details``:: (object) The detailed error report with keys being profile IDs and values being the exact 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]]