Web: Fix Discover setup access screen erroring when updating user (#4…

…3558)

* Web: fix discover user update error

* Address CRs

web/packages/teleport/src/Discover/Shared/SetupAccess/useUserTraits.test.tsx

@@ -33,6 +33,7 @@ import {
 } from 'teleport/Discover/Fixtures/fixtures';
 import TeleportContext from 'teleport/teleportContext';
 import { app } from 'teleport/Discover/AwsMangementConsole/fixtures';
+import { ExcludeUserField } from 'teleport/services/user';
 
 import { ResourceKind } from '../ResourceKind';
 
@@ -129,10 +130,13 @@ describe('onProceed correctly deduplicates, removes static traits, updates meta,
 
     // Test that we are updating the user with the correct traits.
     const mockUser = getMockUser();
-    expect(teleCtx.userService.updateUser).toHaveBeenCalledWith({
-      ...mockUser,
-      traits: { ...mockUser.traits, ...expected },
-    });
+    expect(teleCtx.userService.updateUser).toHaveBeenCalledWith(
+      {
+        ...mockUser,
+        traits: { ...mockUser.traits, ...expected },
+      },
+      ExcludeUserField.AllTraits
+    );
 
     // Test that updating meta correctly updated the dynamic traits.
     const updatedMeta = spyUpdateAgentMeta.mock.results[0].value as KubeMeta;
@@ -216,14 +220,17 @@ describe('onProceed correctly deduplicates, removes static traits, updates meta,
 
     // Test that we are updating the user with the correct traits.
     const mockUser = getMockUser();
-    expect(teleCtx.userService.updateUser).toHaveBeenCalledWith({
-      ...mockUser,
-      traits: {
-        ...result.current.dynamicTraits,
-        kubeGroups: ['dynamicKbGroup3', 'dynamicKbGroup4'],
-        kubeUsers: ['dynamicKbUser3', 'dynamicKbUser4'],
+    expect(teleCtx.userService.updateUser).toHaveBeenCalledWith(
+      {
+        ...mockUser,
+        traits: {
+          ...result.current.dynamicTraits,
+          kubeGroups: ['dynamicKbGroup3', 'dynamicKbGroup4'],
+          kubeUsers: ['dynamicKbUser3', 'dynamicKbUser4'],
+        },
       },
-    });
+      ExcludeUserField.AllTraits
+    );
   });
 
   test('database', async () => {
@@ -292,10 +299,13 @@ describe('onProceed correctly deduplicates, removes static traits, updates meta,
 
     // Test that we are updating the user with the correct traits.
     const mockUser = getMockUser();
-    expect(teleCtx.userService.updateUser).toHaveBeenCalledWith({
-      ...mockUser,
-      traits: { ...mockUser.traits, ...expected },
-    });
+    expect(teleCtx.userService.updateUser).toHaveBeenCalledWith(
+      {
+        ...mockUser,
+        traits: { ...mockUser.traits, ...expected },
+      },
+      ExcludeUserField.AllTraits
+    );
 
     // Test that updating meta correctly updated the dynamic traits.
     const updatedMeta = spyUpdateAgentMeta.mock.results[0].value as DbMeta;
@@ -368,14 +378,17 @@ describe('onProceed correctly deduplicates, removes static traits, updates meta,
 
     // Test that we are updating the user with the correct traits.
     const mockUser = getMockUser();
-    expect(teleCtx.userService.updateUser).toHaveBeenCalledWith({
-      ...mockUser,
-      traits: {
-        ...result.current.dynamicTraits,
-        databaseNames: ['banana', 'carrot'],
-        databaseUsers: ['apple'],
+    expect(teleCtx.userService.updateUser).toHaveBeenCalledWith(
+      {
+        ...mockUser,
+        traits: {
+          ...result.current.dynamicTraits,
+          databaseNames: ['banana', 'carrot'],
+          databaseUsers: ['apple'],
+        },
       },
-    });
+      ExcludeUserField.AllTraits
+    );
   });
 
   test('node', async () => {
@@ -436,10 +449,13 @@ describe('onProceed correctly deduplicates, removes static traits, updates meta,
 
     // Test that we are updating the user with the correct traits.
     const mockUser = getMockUser();
-    expect(teleCtx.userService.updateUser).toHaveBeenCalledWith({
-      ...mockUser,
-      traits: { ...mockUser.traits, ...expected },
-    });
+    expect(teleCtx.userService.updateUser).toHaveBeenCalledWith(
+      {
+        ...mockUser,
+        traits: { ...mockUser.traits, ...expected },
+      },
+      ExcludeUserField.AllTraits
+    );
 
     // Test that updating meta correctly updated the dynamic traits.
     const updatedMeta = spyUpdateAgentMeta.mock.results[0].value as NodeMeta;
@@ -521,13 +537,16 @@ describe('onProceed correctly deduplicates, removes static traits, updates meta,
 
     // Test that we are updating the user with the correct traits.
     const mockUser = getMockUser();
-    expect(teleCtx.userService.updateUser).toHaveBeenCalledWith({
-      ...mockUser,
-      traits: {
-        ...mockUser.traits,
-        awsRoleArns: [dynamicTraits.awsRoleArns[0]],
+    expect(teleCtx.userService.updateUser).toHaveBeenCalledWith(
+      {
+        ...mockUser,
+        traits: {
+          ...mockUser.traits,
+          awsRoleArns: [dynamicTraits.awsRoleArns[0]],
+        },
       },
-    });
+      ExcludeUserField.AllTraits
+    );
 
     // Test that app's awsRoles field got updated with the dynamic trait.
     const updatedMeta = spyUpdateAgentMeta.mock.results[0].value as AppMeta;
@@ -590,13 +609,16 @@ describe('onProceed correctly deduplicates, removes static traits, updates meta,
     });
     // Test that we are updating the user with the correct traits.
     const mockUser = getMockUser();
-    expect(teleCtx.userService.updateUser).toHaveBeenCalledWith({
-      ...mockUser,
-      traits: {
-        ...result.current.dynamicTraits,
-        logins: ['banana', 'carrot'],
+    expect(teleCtx.userService.updateUser).toHaveBeenCalledWith(
+      {
+        ...mockUser,
+        traits: {
+          ...result.current.dynamicTraits,
+          logins: ['banana', 'carrot'],
+        },
       },
-    });
+      ExcludeUserField.AllTraits
+    );
   });
 });
 

web/packages/teleport/src/Discover/Shared/SetupAccess/useUserTraits.ts

@@ -24,6 +24,11 @@ import useTeleport from 'teleport/useTeleport';
 import { Option } from 'teleport/Discover/Shared/SelectCreatable';
 import { useDiscover } from 'teleport/Discover/useDiscover';
 import { splitAwsIamArn } from 'teleport/services/integrations/aws';
+import {
+  ExcludeUserField,
+  type User,
+  type UserTraits,
+} from 'teleport/services/user';
 
 import { ResourceKind } from '../ResourceKind';
 
@@ -33,7 +38,6 @@ import type {
   KubeMeta,
   NodeMeta,
 } from 'teleport/Discover/useDiscover';
-import type { User, UserTraits } from 'teleport/services/user';
 
 // useUserTraits handles:
 //  - retrieving the latest user (for the dynamic traits) from the backend
@@ -328,13 +332,16 @@ export function useUserTraits() {
     setAttempt({ status: 'processing' });
     try {
       await ctx.userService
-        .updateUser({
-          ...user,
-          traits: {
-            ...user.traits,
-            ...newDynamicTraits,
+        .updateUser(
+          {
+            ...user,
+            traits: {
+              ...user.traits,
+              ...newDynamicTraits,
+            },
           },
-        })
+          ExcludeUserField.AllTraits
+        )
         .catch((error: Error) => {
           emitErrorEvent(`error updating user traits: ${error.message}`);
           throw error;

web/packages/teleport/src/Users/useUsers.ts

@@ -19,7 +19,7 @@
 import { ReactElement, useEffect, useState } from 'react';
 import { useAttempt } from 'shared/hooks';
 
-import { User } from 'teleport/services/user';
+import { ExcludeUserField, User } from 'teleport/services/user';
 import useTeleport from 'teleport/useTeleport';
 import auth from 'teleport/services/auth/auth';
 
@@ -77,15 +77,17 @@ export default function useUsers({
   }
 
   function onUpdate(u: User) {
-    return ctx.userService.updateUser(u).then(result => {
-      setUsers([result, ...users.filter(i => i.name !== u.name)]);
-    });
+    return ctx.userService
+      .updateUser(u, ExcludeUserField.Traits)
+      .then(result => {
+        setUsers([result, ...users.filter(i => i.name !== u.name)]);
+      });
   }
 
   async function onCreate(u: User) {
     const webauthnResponse = await auth.getWebauthnResponseForAdminAction(true);
     return ctx.userService
-      .createUser(u, webauthnResponse)
+      .createUser(u, ExcludeUserField.Traits, webauthnResponse)
       .then(result => setUsers([result, ...users]))
       .then(() =>
         ctx.userService.createResetPasswordToken(

web/packages/teleport/src/services/user/types.ts

@@ -134,6 +134,13 @@ export interface User {
   allTraits?: AllUserTraits;
 }
 
+// Backend does not allow User fields "traits" and "allTraits"
+// both to be specified in the same request when creating or updating a user.
+export enum ExcludeUserField {
+  Traits = 'traits',
+  AllTraits = 'allTraits',
+}
+
 // UserTraits contain fields that define traits for local accounts.
 export interface UserTraits {
   // logins is the list of logins that this user is allowed to

web/packages/teleport/src/services/user/user.test.ts

@@ -17,10 +17,11 @@
  */
 
 import api from 'teleport/services/api';
+import cfg from 'teleport/config';
 
 import user from './user';
 import { makeTraits } from './makeUser';
-import { PasswordState } from './types';
+import { ExcludeUserField, PasswordState, User } from './types';
 
 test('undefined values in context response gives proper default values', async () => {
   const mockContext = {
@@ -370,3 +371,79 @@ test('makeTraits', async () => {
     color: [],
   });
 });
+
+test('excludeUserFields when updating user', async () => {
+  // we are not testing the reply, so reply doesn't matter.
+  jest.spyOn(api, 'put').mockResolvedValue({} as any);
+
+  const userReq: User = {
+    name: 'name',
+    roles: [],
+    traits: blankTraits,
+    allTraits: {},
+  };
+
+  await user.updateUser(userReq, ExcludeUserField.AllTraits);
+  expect(api.put).toHaveBeenCalledWith(cfg.api.usersPath, {
+    name: 'name',
+    roles: [],
+    traits: blankTraits,
+  });
+
+  jest.clearAllMocks();
+
+  await user.updateUser(userReq, ExcludeUserField.Traits);
+  expect(api.put).toHaveBeenCalledWith(cfg.api.usersPath, {
+    name: 'name',
+    roles: [],
+    allTraits: {},
+  });
+});
+
+test('excludeUserFields when creating user', async () => {
+  // we are not testing the reply, so reply doesn't matter.
+  jest.spyOn(api, 'post').mockResolvedValue({} as any);
+
+  const userReq: User = {
+    name: 'name',
+    roles: [],
+    traits: blankTraits,
+    allTraits: {},
+  };
+
+  await user.createUser(userReq, ExcludeUserField.AllTraits);
+  expect(api.post).toHaveBeenCalledWith(
+    cfg.api.usersPath,
+    {
+      name: 'name',
+      roles: [],
+      traits: blankTraits,
+    },
+    null,
+    undefined
+  );
+
+  jest.clearAllMocks();
+
+  await user.createUser(userReq, ExcludeUserField.Traits);
+  expect(api.post).toHaveBeenCalledWith(
+    cfg.api.usersPath,
+    {
+      name: 'name',
+      roles: [],
+      allTraits: {},
+    },
+    null,
+    undefined
+  );
+});
+
+const blankTraits = {
+  logins: [],
+  databaseUsers: [],
+  databaseNames: [],
+  kubeUsers: [],
+  kubeGroups: [],
+  windowsLogins: [],
+  awsRoleArns: [],
+};