fix(ui): make self-service profile edits report their outcome (#5699)

* fix(ui): make self-service profile edits report their outcome

When a non-admin user saved their own profile (e.g. changing their
password via EnableUserEditing), the data provider followed the user
update with a call to the admin-only PUT /api/user/{id}/library
endpoint, which always failed with 403. The save error handler then
crashed reading error.body.errors on the plain-text response, so the
user got no notification at all - while the profile change had in fact
already been applied. This made password changes look like they were
silently ignored, and follow-up attempts failed with 'password does not
match' since the current password had already changed. Present since
the multi-library support introduced in v0.58.0 (#4181).

Only call the user-library association endpoint when the logged-in user
is an admin (the server manages assignments for self-edits), and make
the save error handler tolerate error bodies without field errors,
notifying a generic error instead of crashing.

* fix(ui): tolerate nullish rejection values in user save handler

Address review feedback: use optional chaining on the error itself in
the UserEdit save handler, so a nullish rejection value also results in
the generic error notification instead of a TypeError.

ui/src/dataProvider/wrapperDataProvider.js

@@ -137,8 +137,9 @@ const updateUser = async (params) => {
     data: userData,
   })
 
-  // Then handle library associations for non-admin users
-  if (!userData.isAdmin && libraryIds !== undefined) {
+  // Then handle library associations for non-admin users. Only admins can call
+  // this endpoint; for self-edits the server manages library assignments
+  if (isAdmin() && !userData.isAdmin && libraryIds !== undefined) {
     await handleUserLibraryAssociation(userId, libraryIds)
   }
 

ui/src/dataProvider/wrapperDataProvider.test.js

@@ -0,0 +1,90 @@
+import { describe, it, expect, vi, beforeEach } from 'vitest'
+import wrapperDataProvider from './wrapperDataProvider'
+
+const { mockProvider, mockHttpClient } = vi.hoisted(() => ({
+  mockProvider: {
+    update: vi.fn(),
+    create: vi.fn(),
+    getOne: vi.fn(),
+  },
+  mockHttpClient: vi.fn(),
+}))
+
+vi.mock('ra-data-json-server', () => ({ default: () => mockProvider }))
+vi.mock('./httpClient', () => ({ default: mockHttpClient }))
+
+describe('wrapperDataProvider', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    localStorage.clear()
+    mockProvider.update.mockResolvedValue({ data: { id: 'u1' } })
+    mockProvider.create.mockResolvedValue({ data: { id: 'u1' } })
+    mockHttpClient.mockResolvedValue({ json: [] })
+  })
+
+  describe('update user', () => {
+    it('sets library associations when an admin edits a non-admin user', async () => {
+      localStorage.setItem('role', 'admin')
+
+      await wrapperDataProvider.update('user', {
+        id: 'u1',
+        data: { name: 'Sam', isAdmin: false, libraryIds: [1] },
+      })
+
+      expect(mockProvider.update).toHaveBeenCalledWith(
+        'user',
+        expect.objectContaining({ id: 'u1' }),
+      )
+      expect(mockHttpClient).toHaveBeenCalledWith('/api/user/u1/library', {
+        method: 'PUT',
+        body: JSON.stringify({ libraryIds: [1] }),
+      })
+    })
+
+    it('does not call the admin-only library endpoint when a non-admin edits their own profile', async () => {
+      localStorage.setItem('role', 'regular')
+
+      await wrapperDataProvider.update('user', {
+        id: 'u1',
+        data: {
+          name: 'Sam',
+          isAdmin: false,
+          libraryIds: [1],
+          currentPassword: 'old',
+          password: 'new',
+        },
+      })
+
+      expect(mockProvider.update).toHaveBeenCalled()
+      expect(mockHttpClient).not.toHaveBeenCalled()
+    })
+
+    it('does not set library associations when the edited user is an admin', async () => {
+      localStorage.setItem('role', 'admin')
+
+      await wrapperDataProvider.update('user', {
+        id: 'u1',
+        data: { name: 'Sam', isAdmin: true, libraryIds: [1] },
+      })
+
+      expect(mockProvider.update).toHaveBeenCalled()
+      expect(mockHttpClient).not.toHaveBeenCalled()
+    })
+
+    it('strips libraryIds from the user update payload', async () => {
+      localStorage.setItem('role', 'admin')
+
+      await wrapperDataProvider.update('user', {
+        id: 'u1',
+        data: { name: 'Sam', isAdmin: false, libraryIds: [1] },
+      })
+
+      expect(mockProvider.update).toHaveBeenCalledWith(
+        'user',
+        expect.objectContaining({
+          data: { name: 'Sam', isAdmin: false },
+        }),
+      )
+    })
+  })
+})

ui/src/user/UserEdit.jsx

@@ -96,9 +96,10 @@ const UserEdit = (props) => {
         })
         permissions === 'admin' ? redirect('/user') : refresh()
       } catch (error) {
-        if (error.body.errors) {
+        if (error?.body?.errors) {
           return error.body.errors
         }
+        notify('ra.page.error', 'warning')
       }
     },
     [mutate, notify, permissions, redirect, refresh],

ui/src/user/UserEdit.test.jsx

@@ -27,6 +27,14 @@ const adminUser = {
   isAdmin: true,
 }
 
+const hooks = vi.hoisted(() => ({
+  save: null,
+  mutate: vi.fn(),
+  notify: vi.fn(),
+  redirect: vi.fn(),
+  refresh: vi.fn(),
+}))
+
 // Mock React-Admin completely with simpler implementations
 vi.mock('react-admin', () => ({
   Edit: ({ children, title }) => (
@@ -35,9 +43,10 @@ vi.mock('react-admin', () => ({
       {children}
     </div>
   ),
-  SimpleForm: ({ children }) => (
-    <form data-testid="simple-form">{children}</form>
-  ),
+  SimpleForm: ({ children, save }) => {
+    hooks.save = save
+    return <form data-testid="simple-form">{children}</form>
+  },
   TextInput: ({ source }) => <input data-testid={`text-input-${source}`} />,
   BooleanInput: ({ source }) => (
     <input type="checkbox" data-testid={`boolean-input-${source}`} />
@@ -54,10 +63,10 @@ vi.mock('react-admin', () => ({
   Typography: ({ children }) => <p>{children}</p>,
   required: () => () => null,
   email: () => () => null,
-  useMutation: () => [vi.fn()],
-  useNotify: () => vi.fn(),
-  useRedirect: () => vi.fn(),
-  useRefresh: () => vi.fn(),
+  useMutation: () => [hooks.mutate],
+  useNotify: () => hooks.notify,
+  useRedirect: () => hooks.redirect,
+  useRefresh: () => hooks.refresh,
   usePermissions: () => ({ permissions: 'admin' }),
   useTranslate: () => (key) => key,
 }))
@@ -127,4 +136,60 @@ describe('<UserEdit />', () => {
     expect(screen.getByTestId('text-input-name')).toBeInTheDocument()
     expect(screen.getByTestId('text-input-email')).toBeInTheDocument()
   })
+
+  describe('save', () => {
+    beforeEach(() => {
+      vi.clearAllMocks()
+      hooks.save = null
+    })
+
+    it('notifies success and redirects when the update succeeds', async () => {
+      hooks.mutate.mockResolvedValue({ data: defaultUser })
+      render(<UserEdit id="user1" permissions="admin" />)
+
+      await hooks.save({ id: 'user1', name: 'New Name' })
+
+      expect(hooks.notify).toHaveBeenCalledWith(
+        'resources.user.notifications.updated',
+        'info',
+        { smart_count: 1 },
+      )
+      expect(hooks.redirect).toHaveBeenCalledWith('/user')
+    })
+
+    it('returns field errors when the update fails validation', async () => {
+      const fieldErrors = { currentPassword: 'ra.validation.required' }
+      hooks.mutate.mockRejectedValue({ body: { errors: fieldErrors } })
+      render(<UserEdit id="user1" permissions="admin" />)
+
+      const result = await hooks.save({ id: 'user1' })
+
+      expect(result).toEqual(fieldErrors)
+      expect(hooks.notify).not.toHaveBeenCalledWith(
+        'resources.user.notifications.updated',
+        'info',
+        { smart_count: 1 },
+      )
+    })
+
+    it('notifies an error when the update fails without field errors', async () => {
+      hooks.mutate.mockRejectedValue(new Error('Forbidden'))
+      render(<UserEdit id="user1" permissions="admin" />)
+
+      await hooks.save({ id: 'user1' })
+
+      expect(hooks.notify).toHaveBeenCalledWith('ra.page.error', 'warning')
+      expect(hooks.redirect).not.toHaveBeenCalled()
+    })
+
+    it('notifies an error when the update rejects with a non-object error', async () => {
+      hooks.mutate.mockRejectedValue(undefined)
+      render(<UserEdit id="user1" permissions="admin" />)
+
+      await hooks.save({ id: 'user1' })
+
+      expect(hooks.notify).toHaveBeenCalledWith('ra.page.error', 'warning')
+      expect(hooks.redirect).not.toHaveBeenCalled()
+    })
+  })
 })