Browse Source

User Profile - Detailed errors in hasPrivileges response (#89224)

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.
Yang Wang 3 years ago
parent
commit
725367e14b

+ 5 - 0
docs/changelog/89224.yaml

@@ -0,0 +1,5 @@
+pr: 89224
+summary: User Profile - Detailed errors in `hasPrivileges` response
+area: Security
+type: enhancement
+issues: []

+ 28 - 7
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_does-not-exist_0": {
+        "type": "resource_not_found_exception",
+        "reason": "profile document not found"
+      }
+    }
+  }
 }
 --------------------------------------------------
 // NOTCONSOLE

+ 15 - 13
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.StreamInput;
 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
@@ -47,31 +49,31 @@ public class ProfileHasPrivilegesResponse extends ActionResponse implements ToXC
         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 + "}";
     }
 }

+ 65 - 16
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.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> {
 
@@ -35,16 +43,19 @@ public class ProfileHasPrivilegesResponseTests extends AbstractWireSerializingTe
     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 class ProfileHasPrivilegesResponseTests extends AbstractWireSerializingTe
         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)));
+            });
         }
     }
 
@@ -86,4 +124,15 @@ public class ProfileHasPrivilegesResponseTests extends AbstractWireSerializingTe
         }
         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;
+    }
 }

+ 12 - 1
x-pack/plugin/security/qa/profile/src/javaRestTest/java/org/elasticsearch/xpack/security/profile/ProfileIT.java

@@ -115,8 +115,19 @@ public class ProfileIT extends ESRestTestCase {
         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 {

+ 2 - 2
x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/profile/ProfileIntegTests.java

@@ -586,7 +586,7 @@ public class ProfileIntegTests extends AbstractProfileIntegTestCase {
             )
         ).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 class ProfileIntegTests extends AbstractProfileIntegTestCase {
             )
         ).actionGet();
         assertThat(profileHasPrivilegesResponse.hasPrivilegeUids(), emptyIterable());
-        assertThat(profileHasPrivilegesResponse.errorUids(), emptyIterable());
+        assertThat(profileHasPrivilegesResponse.errors(), anEmptyMap());
 
         // Enable again for search
         final SetProfileEnabledRequest setProfileEnabledRequest2 = new SetProfileEnabledRequest(

+ 7 - 8
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/profile/TransportProfileHasPrivilegesAction.java

@@ -36,6 +36,7 @@ import java.util.Collections;
 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 class TransportProfileHasPrivilegesAction extends HandledTransportAction<
     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"));
@@ -107,7 +106,7 @@ public class TransportProfileHasPrivilegesAction extends HandledTransportAction<
                                 }
                             }, 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));

+ 8 - 26
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/profile/ProfileService.java

@@ -11,7 +11,6 @@ import org.apache.logging.log4j.LogManager;
 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 class ProfileService {
         }));
     }
 
-    public void getProfileSubjects(Collection<String> uids, ActionListener<MultiProfileSubjectResponse> listener) {
+    public void getProfileSubjects(Collection<String> uids, ActionListener<ResultsAndErrors<Map.Entry<String, Subject>>> 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 @@ public class ProfileService {
                         for (MultiGetItemResponse itemResponse : multiGetResponse.getResponses()) {
                             final String profileUid = docIdToUid(itemResponse.getId());
                             if (itemResponse.isFailed()) {
+                                logger.debug("Failed to retrieve profile [{}]", profileUid);
                                 errors.put(profileUid, itemResponse.getFailure().getFailure());
                             } else if (itemResponse.getResponse() != null) {
                                 if (itemResponse.getResponse().isExists()) {
@@ -407,16 +409,6 @@ public class ProfileService {
                             }
                         }
                         final ResultsAndErrors<VersionedDocument> resultsAndErrors = new ResultsAndErrors<>(retrievedDocs, errors);
-                        if (logger.isDebugEnabled() && false == resultsAndErrors.errors().isEmpty()) {
-                            Exception loggedException = null;
-                            final List<String> 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 @@ public class ProfileService {
         );
     }
 
-    private static List<String> errorUidsExcludingNotFound(Map<String, Exception> 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 @@ public class ProfileService {
         }
 
     }
-
-    public record MultiProfileSubjectResponse(Map<String, Subject> profileUidToSubject, Set<String> failureProfileUids) {}
 }

+ 17 - 12
x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/profile/TransportProfileHasPrivilegesActionTests.java

@@ -20,6 +20,7 @@ import org.elasticsearch.test.ESTestCase;
 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.core.security.user.User;
 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.Map;
 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.elasticsearch.xpack.core.security.authz.AuthorizationEngine.Pr
 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 class TransportProfileHasPrivilegesActionTests extends ESTestCase {
             for (String uid : uidsArg) {
                 profileUidToSubject.put(uid, new Subject(new User("user_for_profile_" + uid), mock(Authentication.RealmRef.class)));
             }
-            final ActionListener<MultiProfileSubjectResponse> listener = (ActionListener<MultiProfileSubjectResponse>) invocation
-                .getArguments()[1];
-            listener.onResponse(new MultiProfileSubjectResponse(profileUidToSubject, Set.of()));
+            final var listener = (ActionListener<ResultsAndErrors<Map.Entry<String, Subject>>>) invocation.getArguments()[1];
+            listener.onResponse(new ResultsAndErrors<>(profileUidToSubject.entrySet(), Map.of()));
             return null;
         }).when(profileService).getProfileSubjects(anyCollection(), anyActionListener());
 
@@ -152,7 +154,7 @@ public class TransportProfileHasPrivilegesActionTests extends ESTestCase {
         transportProfileHasPrivilegesAction.doExecute(mock(CancellableTask.class), request, listener);
 
         ProfileHasPrivilegesResponse response = listener.get();
-        assertThat(response.errorUids(), is(errorProfileUids));
+        assertThat(response.errors().keySet(), equalTo(errorProfileUids));
         Set<String> hasPrivilegeUids = new HashSet<>(allProfileUids);
         hasPrivilegeUids.removeAll(errorProfileUids);
         hasPrivilegeUids.removeAll(noPrivilegesProfileUids);
@@ -170,9 +172,13 @@ public class TransportProfileHasPrivilegesActionTests extends ESTestCase {
         );
 
         doAnswer(invocation -> {
-            final ActionListener<MultiProfileSubjectResponse> listener = (ActionListener<MultiProfileSubjectResponse>) invocation
-                .getArguments()[1];
-            listener.onResponse(new MultiProfileSubjectResponse(Map.of(), errorProfileUids));
+            final var listener = (ActionListener<ResultsAndErrors<Map.Entry<String, Subject>>>) 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 class TransportProfileHasPrivilegesActionTests extends ESTestCase {
 
         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 class TransportProfileHasPrivilegesActionTests extends ESTestCase {
             for (String uid : uidsArg) {
                 profileUidToSubject.put(uid, new Subject(new User("user_for_profile_" + uid), mock(Authentication.RealmRef.class)));
             }
-            final ActionListener<MultiProfileSubjectResponse> listener = (ActionListener<MultiProfileSubjectResponse>) invocation
-                .getArguments()[1];
-            listener.onResponse(new MultiProfileSubjectResponse(profileUidToSubject, Set.of()));
+            final var listener = (ActionListener<ResultsAndErrors<Map.Entry<String, Subject>>>) invocation.getArguments()[1];
+            listener.onResponse(new ResultsAndErrors<>(profileUidToSubject.entrySet(), Map.of()));
             return null;
         }).when(profileService).getProfileSubjects(anyCollection(), anyActionListener());
 

+ 35 - 55
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.cluster.service.ClusterService;
 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.ScoreSortBuilder;
 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.HashMap;
 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.Security.SECURITY_CRYPTO_THREAD_P
 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 class ProfileServiceTests extends ESTestCase {
     @SuppressWarnings("unchecked")
     public void testGetProfileSubjectsNoIndex() throws Exception {
         when(profileIndex.indexExists()).thenReturn(false);
-        PlainActionFuture<ProfileService.MultiProfileSubjectResponse> future = new PlainActionFuture<>();
+        PlainActionFuture<ResultsAndErrors<Map.Entry<String, Subject>>> 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<Map.Entry<String, Subject>> 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<ProfileService.MultiProfileSubjectResponse> future2 = new PlainActionFuture<>();
+        PlainActionFuture<ResultsAndErrors<Map.Entry<String, Subject>>> future2 = new PlainActionFuture<>();
         profileService.getProfileSubjects(randomList(1, 5, () -> randomAlphaOfLength(20)), future2);
         ExecutionException e = expectThrows(ExecutionException.class, () -> future2.get());
         assertThat(e.getCause(), is(unavailableException));
-        PlainActionFuture<ProfileService.MultiProfileSubjectResponse> future3 = new PlainActionFuture<>();
+        PlainActionFuture<ResultsAndErrors<Map.Entry<String, Subject>>> 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<String> allProfileUids = randomList(1, 5, () -> randomAlphaOfLength(20));
         final Collection<String> missingProfileUids = randomSubsetOf(allProfileUids);
         doAnswer(invocation -> {
@@ -338,14 +333,19 @@ public class ProfileServiceTests extends ESTestCase {
             return null;
         }).when(client).execute(eq(MultiGetAction.INSTANCE), any(MultiGetRequest.class), anyActionListener());
 
-        final PlainActionFuture<ProfileService.MultiProfileSubjectResponse> future = new PlainActionFuture<>();
+        final PlainActionFuture<ResultsAndErrors<Map.Entry<String, Subject>>> future = new PlainActionFuture<>();
         profileService.getProfileSubjects(allProfileUids, future);
 
-        ProfileService.MultiProfileSubjectResponse multiProfileSubjectResponse = future.get();
+        ResultsAndErrors<Map.Entry<String, Subject>> 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<String, Subject> 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<String, Subject> 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 class ProfileServiceTests extends ESTestCase {
             listener.onFailure(mGetException);
             return null;
         }).when(client).execute(eq(MultiGetAction.INSTANCE), any(MultiGetRequest.class), anyActionListener());
-        final PlainActionFuture<ProfileService.MultiProfileSubjectResponse> future = new PlainActionFuture<>();
+        final PlainActionFuture<ResultsAndErrors<Map.Entry<String, Subject>>> 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<String> missingProfileUids = randomList(1, 5, () -> randomAlphaOfLength(20));
-        final Collection<String> 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<String> allProfileUids = randomList(1, 5, () -> randomAlphaOfLength(20));
+        final Collection<String> errorProfileUids = randomSubsetOf(allProfileUids);
+        final Collection<String> missingProfileUids = Sets.difference(Set.copyOf(allProfileUids), Set.copyOf(errorProfileUids));
         doAnswer(invocation -> {
             assertThat(
                 threadPool.getThreadContext().getTransient(ACTION_ORIGIN_TRANSIENT_NAME),
@@ -416,19 +400,15 @@ public class ProfileServiceTests extends ESTestCase {
             return null;
         }).when(client).execute(eq(MultiGetAction.INSTANCE), any(MultiGetRequest.class), anyActionListener());
 
-        try {
-            Loggers.addAppender(logger, mockLogAppender);
-            final PlainActionFuture<ProfileService.MultiProfileSubjectResponse> 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<ResultsAndErrors<Map.Entry<String, Subject>>> future2 = new PlainActionFuture<>();
+        profileService.getProfileSubjects(allProfileUids, future2);
+
+        ResultsAndErrors<Map.Entry<String, Subject>> 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() {

+ 9 - 0
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" }