Fix error message when changing the password for a user in the file realm

docs/changelog/127621.yaml

@@ -0,0 +1,5 @@
+pr: 127621
+summary: Fix error message when changing the password for a user in the file realm
+area: Security
+type: bug
+issues: []

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportChangePasswordAction.java

@@ -11,35 +11,54 @@
 import org.elasticsearch.action.ActionType;
 import org.elasticsearch.action.support.ActionFilters;
 import org.elasticsearch.action.support.HandledTransportAction;
+import org.elasticsearch.common.ValidationException;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.common.util.concurrent.EsExecutors;
 import org.elasticsearch.injection.guice.Inject;
 import org.elasticsearch.tasks.Task;
 import org.elasticsearch.transport.TransportService;
 import org.elasticsearch.xpack.core.XPackSettings;
 import org.elasticsearch.xpack.core.security.action.user.ChangePasswordRequest;
+import org.elasticsearch.xpack.core.security.authc.Realm;
+import org.elasticsearch.xpack.core.security.authc.esnative.NativeRealmSettings;
 import org.elasticsearch.xpack.core.security.authc.support.Hasher;
 import org.elasticsearch.xpack.core.security.user.AnonymousUser;
+import org.elasticsearch.xpack.core.security.user.User;
+import org.elasticsearch.xpack.security.authc.Realms;
 import org.elasticsearch.xpack.security.authc.esnative.NativeUsersStore;
 
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.List;
 import java.util.Locale;
+import java.util.concurrent.CountDownLatch;
+import java.util.function.Function;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+import static org.elasticsearch.xpack.core.security.user.UsernamesField.ELASTIC_NAME;
+import static org.elasticsearch.xpack.security.authc.esnative.NativeUsersStore.USER_NOT_FOUND_MESSAGE;
 
 public class TransportChangePasswordAction extends HandledTransportAction<ChangePasswordRequest, ActionResponse.Empty> {
 
     public static final ActionType<ActionResponse.Empty> TYPE = new ActionType<>("cluster:admin/xpack/security/user/change_password");
     private final Settings settings;
     private final NativeUsersStore nativeUsersStore;
+    private final Realms realms;
 
     @Inject
     public TransportChangePasswordAction(
         Settings settings,
         TransportService transportService,
         ActionFilters actionFilters,
-        NativeUsersStore nativeUsersStore
+        NativeUsersStore nativeUsersStore,
+        Realms realms
     ) {
         super(TYPE.name(), transportService, actionFilters, ChangePasswordRequest::new, EsExecutors.DIRECT_EXECUTOR_SERVICE);
         this.settings = settings;
         this.nativeUsersStore = nativeUsersStore;
+        this.realms = realms;
     }
 
     @Override
@@ -62,6 +81,82 @@ protected void doExecute(Task task, ChangePasswordRequest request, ActionListene
             );
             return;
         }
-        nativeUsersStore.changePassword(request, listener.safeMap(v -> ActionResponse.Empty.INSTANCE));
+
+        // check if user exists in the native realm
+        nativeUsersStore.getUser(username, new ActionListener<>() {
+            @Override
+            public void onResponse(User user) {
+                if (user == null) {
+                    List<Realm> nonNativeRealms = realms.getActiveRealms()
+                        .stream()
+                        .filter(t -> NativeRealmSettings.TYPE.equalsIgnoreCase(t.type()) == false)
+                        .toList();
+                    if (nonNativeRealms.isEmpty()) {
+                        listener.onFailure(createUserNotFoundException());
+                    }
+                    CountDownLatch latch = new CountDownLatch(nonNativeRealms.size());
+                    List<Exception> realmErrors = Collections.synchronizedList(new ArrayList<>());
+                    realms.stream().forEach(realm -> {
+                        // we should check if this request is mistakenly trying to reset a user in some other realm
+                        realm.lookupUser(username, new ActionListener<>() {
+                            @Override
+                            public void onResponse(User user) {
+                                if (user != null) {
+                                    ValidationException validationException = new ValidationException();
+                                    validationException.addValidationError(
+                                        "user ["
+                                            + username
+                                            + "] is not a native user and cannot be managed via this API."
+                                            + (ELASTIC_NAME.equalsIgnoreCase(username)
+                                                ? " To update the '" + ELASTIC_NAME + "' user in a cloud deployment, use the console."
+                                                : "")
+                                    );
+                                    onFailure(validationException);
+                                } else {
+                                    onFailure(null);
+                                }
+                            }
+
+                            @Override
+                            public void onFailure(Exception e) {
+                                if (e != null) {
+                                    realmErrors.add(e);
+                                }
+                                latch.countDown();
+                            }
+                        });
+                    });
+                    try {
+                        latch.await();
+                        // combine errors across realms
+                        ValidationException validationException = new ValidationException();
+                        List<String> errors = realmErrors.stream()
+                            .map(t -> ((ValidationException) t).validationErrors())
+                            .flatMap((Function<List<String>, Stream<String>>) Collection::stream)
+                            .collect(Collectors.toList());
+                        validationException.addValidationErrors(errors);
+                        listener.onFailure(validationException);
+                    } catch (InterruptedException e) {
+                        logger.info("Interrupted while waiting for user lookups across realms", e);
+                        listener.onFailure(e);
+                    }
+                } else {
+                    // safe to proceed
+                    nativeUsersStore.changePassword(request, listener.safeMap(v -> ActionResponse.Empty.INSTANCE));
+                }
+            }
+
+            @Override
+            public void onFailure(Exception e) {
+                listener.onFailure(e);
+            }
+        });
+
+    }
+
+    private static ValidationException createUserNotFoundException() {
+        ValidationException validationException = new ValidationException();
+        validationException.addValidationError(USER_NOT_FOUND_MESSAGE);
+        return validationException;
     }
 }

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/NativeUsersStore.java

@@ -82,6 +82,7 @@ public class NativeUsersStore {
 
     public static final String USER_DOC_TYPE = "user";
     public static final String RESERVED_USER_TYPE = "reserved-user";
+    public static final String USER_NOT_FOUND_MESSAGE = "user must exist in order to change password";
     private static final Logger logger = LogManager.getLogger(NativeUsersStore.class);
 
     private final Settings settings;
@@ -316,7 +317,7 @@ public void onFailure(Exception e) {
                             } else {
                                 logger.debug(() -> format("failed to change password for user [%s]", request.username()), e);
                                 ValidationException validationException = new ValidationException();
-                                validationException.addValidationError("user must exist in order to change password");
+                                validationException.addValidationError(USER_NOT_FOUND_MESSAGE);
                                 listener.onFailure(validationException);
                             }
                         } else {

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportChangePasswordActionTests.java

@@ -10,6 +10,7 @@
 import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.action.ActionResponse;
 import org.elasticsearch.action.support.ActionFilters;
+import org.elasticsearch.common.ValidationException;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.tasks.Task;
 import org.elasticsearch.test.ESTestCase;
@@ -25,11 +26,16 @@
 import org.elasticsearch.xpack.core.security.user.ElasticUser;
 import org.elasticsearch.xpack.core.security.user.KibanaUser;
 import org.elasticsearch.xpack.core.security.user.User;
+import org.elasticsearch.xpack.security.authc.Realms;
 import org.elasticsearch.xpack.security.authc.esnative.NativeUsersStore;
+import org.elasticsearch.xpack.security.authc.file.FileRealm;
+import org.mockito.Mockito;
 
 import java.util.Collections;
+import java.util.List;
 import java.util.Locale;
 import java.util.concurrent.atomic.AtomicReference;
+import java.util.stream.Stream;
 
 import static org.elasticsearch.test.ActionListenerUtils.anyActionListener;
 import static org.elasticsearch.test.SecurityIntegTestCase.getFastStoredHashAlgoForTests;
@@ -39,6 +45,7 @@
 import static org.hamcrest.Matchers.notNullValue;
 import static org.hamcrest.Matchers.nullValue;
 import static org.hamcrest.Matchers.sameInstance;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.mock;
@@ -55,7 +62,9 @@ public void testAnonymousUser() {
             .put(XPackSettings.PASSWORD_HASHING_ALGORITHM.getKey(), hasher.name())
             .build();
         AnonymousUser anonymousUser = new AnonymousUser(settings);
+
         NativeUsersStore usersStore = mock(NativeUsersStore.class);
+
         TransportService transportService = new TransportService(
             Settings.EMPTY,
             mock(Transport.class),
@@ -69,7 +78,8 @@ public void testAnonymousUser() {
             settings,
             transportService,
             mock(ActionFilters.class),
-            usersStore
+            usersStore,
+            mockRealms()
         );
         // Request will fail before the request hashing algorithm is checked, but we use the same algorithm as in settings for consistency
         ChangePasswordRequest request = new ChangePasswordRequest();
@@ -96,6 +106,64 @@ public void onFailure(Exception e) {
         verifyNoMoreInteractions(usersStore);
     }
 
+    public void testFileRealmUser() {
+        final Hasher hasher = getFastStoredHashAlgoForTests();
+        Settings settings = Settings.builder()
+            .put(AnonymousUser.ROLES_SETTING.getKey(), "superuser")
+            .put(XPackSettings.PASSWORD_HASHING_ALGORITHM.getKey(), hasher.name())
+            .build();
+        ElasticUser elasticUser = new ElasticUser(true);
+        NativeUsersStore usersStore = mock(NativeUsersStore.class);
+
+        TransportService transportService = new TransportService(
+            Settings.EMPTY,
+            mock(Transport.class),
+            mock(ThreadPool.class),
+            TransportService.NOOP_TRANSPORT_INTERCEPTOR,
+            x -> null,
+            null,
+            Collections.emptySet()
+        );
+        TransportChangePasswordAction action = new TransportChangePasswordAction(
+            settings,
+            transportService,
+            mock(ActionFilters.class),
+            usersStore,
+            mockRealms(mockFileRealm(elasticUser))
+        );
+        ChangePasswordRequest request = new ChangePasswordRequest();
+        request.username(elasticUser.principal());
+        request.passwordHash(hasher.hash(SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING));
+
+        doAnswer(invocation -> {
+            Object[] args = invocation.getArguments();
+            assert args.length == 2;
+            @SuppressWarnings("unchecked")
+            ActionListener<User> listener = (ActionListener<User>) args[1];
+            listener.onResponse(null);
+            return null;
+        }).when(usersStore).getUser(eq(request.username()), anyActionListener());
+
+        final AtomicReference<Throwable> throwableRef = new AtomicReference<>();
+        final AtomicReference<ActionResponse.Empty> responseRef = new AtomicReference<>();
+        action.doExecute(mock(Task.class), request, new ActionListener<>() {
+            @Override
+            public void onResponse(ActionResponse.Empty changePasswordResponse) {
+                responseRef.set(changePasswordResponse);
+            }
+
+            @Override
+            public void onFailure(Exception e) {
+                throwableRef.set(e);
+            }
+        });
+
+        assertThat(responseRef.get(), is(nullValue()));
+        assertThat(throwableRef.get(), instanceOf(ValidationException.class));
+        assertThat(throwableRef.get().getMessage(), containsString("is not a native user and cannot be managed via this API"));
+        verify(usersStore, times(0)).changePassword(any(ChangePasswordRequest.class), anyActionListener());
+    }
+
     public void testValidUser() {
         testValidUser(randomFrom(new ElasticUser(true), new KibanaUser(true), new User("joe")));
     }
@@ -127,12 +195,21 @@ private void testValidUser(User user) {
             null,
             Collections.emptySet()
         );
+        doAnswer(invocation -> {
+            Object[] args = invocation.getArguments();
+            assert args.length == 2;
+            @SuppressWarnings("unchecked")
+            ActionListener<User> listener = (ActionListener<User>) args[1];
+            listener.onResponse(user);
+            return null;
+        }).when(usersStore).getUser(eq(request.username()), anyActionListener());
         Settings passwordHashingSettings = Settings.builder().put(XPackSettings.PASSWORD_HASHING_ALGORITHM.getKey(), hasher.name()).build();
         TransportChangePasswordAction action = new TransportChangePasswordAction(
             passwordHashingSettings,
             transportService,
             mock(ActionFilters.class),
-            usersStore
+            usersStore,
+            mockRealms()
         );
         final AtomicReference<Throwable> throwableRef = new AtomicReference<>();
         final AtomicReference<ActionResponse.Empty> responseRef = new AtomicReference<>();
@@ -177,7 +254,8 @@ public void testWithPasswordThatsNotAHash() {
             passwordHashingSettings,
             transportService,
             mock(ActionFilters.class),
-            usersStore
+            usersStore,
+            mockRealms()
         );
         action.doExecute(mock(Task.class), request, new ActionListener<>() {
             @Override
@@ -215,6 +293,14 @@ public void testWithDifferentPasswordHashingAlgorithm() {
             listener.onResponse(null);
             return null;
         }).when(usersStore).changePassword(eq(request), anyActionListener());
+        doAnswer(invocation -> {
+            Object[] args = invocation.getArguments();
+            assert args.length == 2;
+            @SuppressWarnings("unchecked")
+            ActionListener<User> listener = (ActionListener<User>) args[1];
+            listener.onResponse(user);
+            return null;
+        }).when(usersStore).getUser(eq(request.username()), anyActionListener());
         final AtomicReference<Throwable> throwableRef = new AtomicReference<>();
         final AtomicReference<ActionResponse.Empty> responseRef = new AtomicReference<>();
         TransportService transportService = new TransportService(
@@ -235,7 +321,8 @@ public void testWithDifferentPasswordHashingAlgorithm() {
             passwordHashingSettings,
             transportService,
             mock(ActionFilters.class),
-            usersStore
+            usersStore,
+            mockRealms()
         );
         action.doExecute(mock(Task.class), request, new ActionListener<>() {
             @Override
@@ -271,6 +358,14 @@ public void testException() {
             listener.onFailure(e);
             return null;
         }).when(usersStore).changePassword(eq(request), anyActionListener());
+        doAnswer(invocation -> {
+            Object[] args = invocation.getArguments();
+            assert args.length == 2;
+            @SuppressWarnings("unchecked")
+            ActionListener<User> listener = (ActionListener<User>) args[1];
+            listener.onResponse(user);
+            return null;
+        }).when(usersStore).getUser(eq(request.username()), anyActionListener());
         TransportService transportService = new TransportService(
             Settings.EMPTY,
             mock(Transport.class),
@@ -285,7 +380,8 @@ public void testException() {
             passwordHashingSettings,
             transportService,
             mock(ActionFilters.class),
-            usersStore
+            usersStore,
+            mockRealms()
         );
         final AtomicReference<Throwable> throwableRef = new AtomicReference<>();
         final AtomicReference<ActionResponse.Empty> responseRef = new AtomicReference<>();
@@ -306,4 +402,31 @@ public void onFailure(Exception e) {
         assertThat(throwableRef.get(), sameInstance(e));
         verify(usersStore, times(1)).changePassword(eq(request), anyActionListener());
     }
+
+    private static FileRealm mockFileRealm(User fileRealmUser) {
+
+        FileRealm fileRealm = Mockito.mock(FileRealm.class);
+        Mockito.when(fileRealm.type()).thenReturn("file");
+        doAnswer(invocation -> {
+            Object[] args = invocation.getArguments();
+            assert args.length == 2;
+            @SuppressWarnings("unchecked")
+            ActionListener<User> listener = (ActionListener<User>) args[1];
+            listener.onResponse(fileRealmUser);
+            return null;
+        }).when(fileRealm).lookupUser(Mockito.any(String.class), anyActionListener());
+
+        return fileRealm;
+    }
+
+    private static Realms mockRealms() {
+        return mockRealms(mockFileRealm(null));
+    }
+
+    private static Realms mockRealms(FileRealm realm) {
+        Realms realms = mock(Realms.class);
+        Mockito.when(realms.stream()).thenReturn(realm == null ? Stream.of() : Stream.of(realm));
+        Mockito.when(realms.getActiveRealms()).thenReturn(realm == null ? List.of() : List.of(realm));
+        return realms;
+    }
 }