Browse Source

Fix NPE when GetUser with profile uid before profile index exists (#98961)

This PR fixes a NullPointerException bug for the GetUser API. The NPE
happens when the API is called with the with_profile_uid parameter
before the profile index is created.

Relates: #89570
Yang Wang 2 years ago
parent
commit
596a56e8a9

+ 5 - 0
docs/changelog/98961.yaml

@@ -0,0 +1,5 @@
+pr: 98961
+summary: Fix NPE when `GetUser` with profile uid before profile index exists
+area: Security
+type: bug
+issues: []

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

@@ -443,14 +443,18 @@ public class ProfileIT extends ESRestTestCase {
         final Request putUserRequest = new Request("PUT", "_security/user/" + username);
         putUserRequest.setJsonEntity("{\"password\":\"x-pack-test-password\",\"roles\":[\"superuser\"]}");
         assertOK(adminClient().performRequest(putUserRequest));
-        final Map<String, Object> profile = doActivateProfile(username, "x-pack-test-password");
 
+        // Get user with profile uid before profile index exists will not show any profile_uid
         final Request getUserRequest = new Request("GET", "_security/user" + (randomBoolean() ? "/" + username : ""));
         getUserRequest.addParameter("with_profile_uid", "true");
-        final Response getUserResponse = adminClient().performRequest(getUserRequest);
-        assertOK(getUserResponse);
+        final Response getUserResponse1 = adminClient().performRequest(getUserRequest);
+        assertOK(getUserResponse1);
+        responseAsMap(getUserResponse1).forEach((k, v) -> assertThat(castToMap(v), not(hasKey("profile_uid"))));
 
-        responseAsMap(getUserResponse).forEach((k, v) -> {
+        // The profile_uid is retrieved for the user after the profile gets activated
+        final Map<String, Object> profile = doActivateProfile(username, "x-pack-test-password");
+        final Response getUserResponse2 = adminClient().performRequest(getUserRequest);
+        responseAsMap(getUserResponse2).forEach((k, v) -> {
             if (username.equals(k)) {
                 assertThat(castToMap(v).get("profile_uid"), equalTo(profile.get("uid")));
             } else {

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

@@ -834,6 +834,17 @@ public class ProfileIntegTests extends AbstractProfileIntegTestCase {
         );
     }
 
+    public void testGetUsersWithProfileUidWhenProfileIndexDoesNotExists() {
+        final GetUsersRequest getUsersRequest = new GetUsersRequest();
+        getUsersRequest.setWithProfileUid(true);
+        if (randomBoolean()) {
+            getUsersRequest.usernames(ElasticUser.NAME, RAC_USER_NAME);
+        }
+        final GetUsersResponse getUsersResponse = client().execute(GetUsersAction.INSTANCE, getUsersRequest).actionGet();
+        // When profile index does not exist, profile lookup is null
+        assertThat(getUsersResponse.getProfileUidLookup(), nullValue());
+    }
+
     private SuggestProfilesResponse.ProfileHit[] doSuggest(String name) {
         return doSuggest(name, Set.of());
     }

+ 4 - 1
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportGetUsersAction.java

@@ -144,7 +144,10 @@ public class TransportGetUsersAction extends HandledTransportAction<GetUsersRequ
         }).toList();
 
         profileService.searchProfilesForSubjects(subjects, ActionListener.wrap(resultsAndErrors -> {
-            if (resultsAndErrors.errors().isEmpty()) {
+            if (resultsAndErrors == null) {
+                // profile index does not exist
+                listener.onResponse(null);
+            } else if (resultsAndErrors.errors().isEmpty()) {
                 assert users.size() == resultsAndErrors.results().size();
                 final Map<String, String> profileUidLookup = resultsAndErrors.results()
                     .stream()

+ 12 - 5
x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportGetUsersActionTests.java

@@ -83,6 +83,7 @@ public class TransportGetUsersActionTests extends ESTestCase {
     private boolean hasAnonymousProfile;
     private boolean hasReservedProfile;
     private boolean hasNativeProfile;
+    private boolean profileIndexExists;
 
     @Before
     public void maybeEnableAnonymous() {
@@ -96,6 +97,7 @@ public class TransportGetUsersActionTests extends ESTestCase {
         hasAnonymousProfile = randomBoolean();
         hasReservedProfile = randomBoolean();
         hasNativeProfile = randomBoolean();
+        profileIndexExists = randomBoolean();
     }
 
     @After
@@ -163,7 +165,7 @@ public class TransportGetUsersActionTests extends ESTestCase {
         } else {
             assertThat("expected an empty array but got: " + Arrays.toString(users), users, emptyArray());
         }
-        if (withProfileUid) {
+        if (profileIndexExists && withProfileUid) {
             assertThat(
                 responseRef.get().getProfileUidLookup(),
                 equalTo(
@@ -248,7 +250,7 @@ public class TransportGetUsersActionTests extends ESTestCase {
         assertThat(throwableRef.get(), is(nullValue()));
         assertThat(responseRef.get(), is(notNullValue()));
         assertThat(users, arrayContaining(reservedUsers.toArray(new User[reservedUsers.size()])));
-        if (withProfileUid) {
+        if (profileIndexExists && withProfileUid) {
             assertThat(responseRef.get().getProfileUidLookup(), equalTo(reservedUsers.stream().filter(user -> {
                 if (user instanceof AnonymousUser) {
                     return hasAnonymousProfile;
@@ -340,7 +342,7 @@ public class TransportGetUsersActionTests extends ESTestCase {
         assertThat(throwableRef.get(), is(nullValue()));
         assertThat(responseRef.get(), is(notNullValue()));
         assertThat(responseRef.get().users(), arrayContaining(expectedList.toArray(new User[expectedList.size()])));
-        if (withProfileUid) {
+        if (profileIndexExists && withProfileUid) {
             assertThat(responseRef.get().getProfileUidLookup(), equalTo(expectedList.stream().filter(user -> {
                 if (user instanceof AnonymousUser) {
                     return hasAnonymousProfile;
@@ -399,6 +401,7 @@ public class TransportGetUsersActionTests extends ESTestCase {
             null,
             Collections.emptySet()
         );
+        profileIndexExists = true; // profile index must exist to simulate exception on search
         TransportGetUsersAction action = new TransportGetUsersAction(
             Settings.EMPTY,
             mock(ActionFilters.class),
@@ -496,7 +499,7 @@ public class TransportGetUsersActionTests extends ESTestCase {
         assertThat(throwableRef.get(), is(nullValue()));
         assertThat(responseRef.get(), is(notNullValue()));
         assertThat(responseRef.get().users(), arrayContaining(expectedList.toArray(new User[expectedList.size()])));
-        if (withProfileUid) {
+        if (profileIndexExists && withProfileUid) {
             assertThat(
                 responseRef.get().getProfileUidLookup(),
                 equalTo(
@@ -611,8 +614,12 @@ public class TransportGetUsersActionTests extends ESTestCase {
     private ProfileService mockProfileService(boolean randomException) {
         final ProfileService profileService = mock(ProfileService.class);
         doAnswer(invocation -> {
-            final List<Subject> subjects = (List<Subject>) invocation.getArguments()[0];
             final var listener = (ActionListener<SubjectSearchResultsAndErrors<Profile>>) invocation.getArguments()[1];
+            if (false == profileIndexExists) {
+                listener.onResponse(null);
+                return null;
+            }
+            final List<Subject> subjects = (List<Subject>) invocation.getArguments()[0];
             List<Tuple<Subject, Profile>> results = subjects.stream().map(subject -> {
                 final User user = subject.getUser();
                 if (user instanceof AnonymousUser) {