Merge pull request #2398 from logto-io/charles-bug-fix-patch-user-management-api

fix(core): fix field updating issues in patch user management api

packages/core/src/queries/user.ts

@@ -47,11 +47,12 @@ export const findUserByIdentity = async (target: string, userId: string) =>
     `
   );
 
-export const hasUser = async (username: string) =>
+export const hasUser = async (username: string, excludeUserId?: string) =>
   envSet.pool.exists(sql`
     select ${fields.id}
     from ${table}
     where ${fields.username}=${username}
+    ${conditionalSql(excludeUserId, (id) => sql`and ${fields.id}<>${id}`)}
   `);
 
 export const hasUserWithId = async (id: string) =>
@@ -61,18 +62,20 @@ export const hasUserWithId = async (id: string) =>
     where ${fields.id}=${id}
   `);
 
-export const hasUserWithEmail = async (email: string) =>
+export const hasUserWithEmail = async (email: string, excludeUserId?: string) =>
   envSet.pool.exists(sql`
     select ${fields.primaryEmail}
     from ${table}
     where lower(${fields.primaryEmail})=lower(${email})
+    ${conditionalSql(excludeUserId, (id) => sql`and ${fields.id}<>${id}`)}
   `);
 
-export const hasUserWithPhone = async (phone: string) =>
+export const hasUserWithPhone = async (phone: string, excludeUserId?: string) =>
   envSet.pool.exists(sql`
     select ${fields.primaryPhone}
     from ${table}
     where ${fields.primaryPhone}=${phone}
+    ${conditionalSql(excludeUserId, (id) => sql`and ${fields.id}<>${id}`)}
   `);
 
 export const hasUserWithIdentity = async (target: string, userId: string) =>

packages/core/src/routes/admin-user.test.ts

@@ -177,16 +177,29 @@ describe('adminUserRoutes', () => {
     });
   });
 
-  it('PATCH /users/:userId should allow empty avatar URL', async () => {
-    const name = 'Michael';
-    const avatar = '';
-
-    const response = await userRequest.patch('/users/foo').send({ name, avatar });
+  it('PATCH /users/:userId should allow empty string for clearable fields', async () => {
+    const response = await userRequest
+      .patch('/users/foo')
+      .send({ name: '', avatar: '', primaryEmail: '' });
     expect(response.status).toEqual(200);
     expect(response.body).toEqual({
       ...mockUserResponse,
-      name,
-      avatar,
+      name: '',
+      avatar: '',
+      primaryEmail: '',
+    });
+  });
+
+  it('PATCH /users/:userId should allow null values for clearable fields', async () => {
+    const response = await userRequest
+      .patch('/users/foo')
+      .send({ name: null, username: null, primaryPhone: null });
+    expect(response.status).toEqual(200);
+    expect(response.body).toEqual({
+      ...mockUserResponse,
+      name: null,
+      username: null,
+      primaryPhone: null,
     });
   });
 

packages/core/src/routes/admin-user.ts

@@ -170,10 +170,10 @@ export default function adminUserRoutes<T extends AuthedRouter>(router: T) {
     koaGuard({
       params: object({ userId: string() }),
       body: object({
-        username: string().regex(usernameRegEx).optional(),
-        primaryEmail: string().regex(emailRegEx).optional(),
-        primaryPhone: string().regex(phoneRegEx).optional(),
-        name: string().nullable().optional(),
+        username: string().regex(usernameRegEx).or(literal('')).nullable().optional(),
+        primaryEmail: string().regex(emailRegEx).or(literal('')).nullable().optional(),
+        primaryPhone: string().regex(phoneRegEx).or(literal('')).nullable().optional(),
+        name: string().or(literal('')).nullable().optional(),
         avatar: string().url().or(literal('')).nullable().optional(),
         customData: arbitraryObjectGuard.optional(),
         roleNames: string().array().optional(),
@@ -186,7 +186,7 @@ export default function adminUserRoutes<T extends AuthedRouter>(router: T) {
       } = ctx.guard;
 
       await findUserById(userId);
-      await checkExistingSignUpIdentifiers(body);
+      await checkExistingSignUpIdentifiers(body, userId);
 
       // Temp solution to validate the existence of input roleNames
       if (body.roleNames?.length) {

packages/core/src/routes/session/utils.ts

@@ -198,9 +198,9 @@ export const checkRequiredProfile = async (
 };
 
 export const checkRequiredSignUpIdentifiers = async (identifiers: {
-  username?: string;
-  primaryEmail?: string;
-  primaryPhone?: string;
+  username?: Nullable<string>;
+  primaryEmail?: Nullable<string>;
+  primaryPhone?: Nullable<string>;
 }) => {
   const { username, primaryEmail, primaryPhone } = identifiers;
 
@@ -224,22 +224,25 @@ export const checkRequiredSignUpIdentifiers = async (identifiers: {
 };
 /* eslint-enable complexity */
 
-export const checkExistingSignUpIdentifiers = async (identifiers: {
-  username?: string;
-  primaryEmail?: string;
-  primaryPhone?: string;
-}) => {
+export const checkExistingSignUpIdentifiers = async (
+  identifiers: {
+    username?: Nullable<string>;
+    primaryEmail?: Nullable<string>;
+    primaryPhone?: Nullable<string>;
+  },
+  excludeUserId: string
+) => {
   const { username, primaryEmail, primaryPhone } = identifiers;
 
-  if (username && (await hasUser(username))) {
+  if (username && (await hasUser(username, excludeUserId))) {
     throw new RequestError({ code: 'user.username_exists', status: 422 });
   }
 
-  if (primaryEmail && (await hasUserWithEmail(primaryEmail))) {
+  if (primaryEmail && (await hasUserWithEmail(primaryEmail, excludeUserId))) {
     throw new RequestError({ code: 'user.email_exists', status: 422 });
   }
 
-  if (primaryPhone && (await hasUserWithPhone(primaryPhone))) {
+  if (primaryPhone && (await hasUserWithPhone(primaryPhone, excludeUserId))) {
     throw new RequestError({ code: 'user.sms_exists', status: 422 });
   }
 };