From dcc782e292e16a0ac53c556915e156937fec7b8e Mon Sep 17 00:00:00 2001 From: Lisa Kim Date: Wed, 26 Jun 2024 12:38:31 -0700 Subject: [PATCH] Web: Fix Discover setup access screen erroring when updating user (#43558) * Web: fix discover user update error * Address CRs --- .../Shared/SetupAccess/useUserTraits.test.tsx | 52 ++++++++------ .../Shared/SetupAccess/useUserTraits.ts | 27 ++++---- web/packages/teleport/src/Users/useUsers.ts | 12 ++-- .../teleport/src/services/user/types.ts | 7 ++ .../teleport/src/services/user/user.test.ts | 68 +++++++++++++++++++ .../teleport/src/services/user/user.ts | 38 +++++++++-- 6 files changed, 161 insertions(+), 43 deletions(-) diff --git a/web/packages/teleport/src/Discover/Shared/SetupAccess/useUserTraits.test.tsx b/web/packages/teleport/src/Discover/Shared/SetupAccess/useUserTraits.test.tsx index aee12a1e8ad16..06052cc345ceb 100644 --- a/web/packages/teleport/src/Discover/Shared/SetupAccess/useUserTraits.test.tsx +++ b/web/packages/teleport/src/Discover/Shared/SetupAccess/useUserTraits.test.tsx @@ -29,6 +29,7 @@ import { defaultResourceSpec, } from 'teleport/Discover/Fixtures/fixtures'; import TeleportContext from 'teleport/teleportContext'; +import { ExcludeUserField } from 'teleport/services/user'; import { ResourceKind } from '../ResourceKind'; @@ -124,10 +125,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; @@ -207,10 +211,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; @@ -283,15 +290,17 @@ describe('onProceed correctly deduplicates, removes static traits, updates meta, // Test that we are updating the user with the correct traits. const mockUser = getMockUser(); - const { databaseUsers, databaseNames } = result.current.dynamicTraits; - expect(teleCtx.userService.updateUser).toHaveBeenCalledWith({ - ...mockUser, - traits: { - ...result.current.dynamicTraits, - databaseNames: [...databaseNames, 'banana', 'carrot'], - databaseUsers: [...databaseUsers, 'apple'], + expect(teleCtx.userService.updateUser).toHaveBeenCalledWith( + { + ...mockUser, + traits: { + ...result.current.dynamicTraits, + databaseNames: ['banana', 'carrot'], + databaseUsers: ['apple'], + }, }, - }); + ExcludeUserField.AllTraits + ); }); test('node', async () => { @@ -352,10 +361,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; diff --git a/web/packages/teleport/src/Discover/Shared/SetupAccess/useUserTraits.ts b/web/packages/teleport/src/Discover/Shared/SetupAccess/useUserTraits.ts index 44a225b9b3220..e78811981a811 100644 --- a/web/packages/teleport/src/Discover/Shared/SetupAccess/useUserTraits.ts +++ b/web/packages/teleport/src/Discover/Shared/SetupAccess/useUserTraits.ts @@ -21,11 +21,15 @@ import { arrayStrDiff } from 'teleport/lib/util'; import useTeleport from 'teleport/useTeleport'; import { Option } from 'teleport/Discover/Shared/SelectCreatable'; import { useDiscover } from 'teleport/Discover/useDiscover'; +import { + ExcludeUserField, + type User, + type UserTraits, +} from 'teleport/services/user'; import { ResourceKind } from '../ResourceKind'; import type { DbMeta, 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 @@ -152,9 +156,6 @@ export function useUserTraits() { case ResourceKind.Database: let newDynamicDbUsers = new Set(); - if (wantAutoDiscover) { - newDynamicDbUsers = new Set(dynamicTraits.databaseUsers); - } traitOpts.databaseUsers.forEach(o => { if (!staticTraits.databaseUsers.includes(o.value)) { newDynamicDbUsers.add(o.value); @@ -162,9 +163,6 @@ export function useUserTraits() { }); let newDynamicDbNames = new Set(); - if (wantAutoDiscover) { - newDynamicDbNames = new Set(dynamicTraits.databaseNames); - } traitOpts.databaseNames.forEach(o => { if (!staticTraits.databaseNames.includes(o.value)) { newDynamicDbNames.add(o.value); @@ -259,13 +257,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; diff --git a/web/packages/teleport/src/Users/useUsers.ts b/web/packages/teleport/src/Users/useUsers.ts index 8ffaa774bf8c8..e543ec164fbfb 100644 --- a/web/packages/teleport/src/Users/useUsers.ts +++ b/web/packages/teleport/src/Users/useUsers.ts @@ -17,7 +17,7 @@ import { ReactElement, useState, useEffect } 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'; export default function useUsers({ @@ -74,14 +74,16 @@ 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)]); + }); } function onCreate(u: User) { return ctx.userService - .createUser(u) + .createUser(u, ExcludeUserField.Traits) .then(result => setUsers([result, ...users])) .then(() => ctx.userService.createResetPasswordToken(u.name, 'invite')); } diff --git a/web/packages/teleport/src/services/user/types.ts b/web/packages/teleport/src/services/user/types.ts index 1ab8d35151606..d705c8c270171 100644 --- a/web/packages/teleport/src/services/user/types.ts +++ b/web/packages/teleport/src/services/user/types.ts @@ -112,6 +112,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 diff --git a/web/packages/teleport/src/services/user/user.test.ts b/web/packages/teleport/src/services/user/user.test.ts index c8e74653f8a91..09d0c49fba768 100644 --- a/web/packages/teleport/src/services/user/user.test.ts +++ b/web/packages/teleport/src/services/user/user.test.ts @@ -15,9 +15,11 @@ */ import api from 'teleport/services/api'; +import cfg from 'teleport/config'; import user from './user'; import { makeTraits } from './makeUser'; +import { ExcludeUserField, User } from './types'; test('undefined values in context response gives proper default values', async () => { const mockContext = { @@ -358,3 +360,69 @@ 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, + }); + + jest.clearAllMocks(); + + await user.createUser(userReq, ExcludeUserField.Traits); + expect(api.post).toHaveBeenCalledWith(cfg.api.usersPath, { + name: 'name', + roles: [], + allTraits: {}, + }); +}); + +const blankTraits = { + logins: [], + databaseUsers: [], + databaseNames: [], + kubeUsers: [], + kubeGroups: [], + windowsLogins: [], + awsRoleArns: [], +}; diff --git a/web/packages/teleport/src/services/user/user.ts b/web/packages/teleport/src/services/user/user.ts index 97d9d0c6817c1..afb0c9f2fa941 100644 --- a/web/packages/teleport/src/services/user/user.ts +++ b/web/packages/teleport/src/services/user/user.ts @@ -21,7 +21,12 @@ import session from 'teleport/services/websession'; import makeUserContext from './makeUserContext'; import { makeResetToken } from './makeResetToken'; import makeUser, { makeUsers } from './makeUser'; -import { User, UserContext, ResetPasswordType } from './types'; +import { + User, + UserContext, + ResetPasswordType, + ExcludeUserField, +} from './types'; const cache = { userContext: null as UserContext, @@ -61,8 +66,10 @@ const service = { * @param user * @returns user */ - updateUser(user: User) { - return api.put(cfg.getUsersUrl(), user).then(makeUser); + updateUser(user: User, excludeUserField: ExcludeUserField) { + return api + .put(cfg.getUsersUrl(), withExcludedField(user, excludeUserField)) + .then(makeUser); }, /** @@ -72,8 +79,10 @@ const service = { * @param user * @returns user */ - createUser(user: User) { - return api.post(cfg.getUsersUrl(), user).then(makeUser); + createUser(user: User, excludeUserField: ExcludeUserField) { + return api + .post(cfg.getUsersUrl(), withExcludedField(user, excludeUserField)) + .then(makeUser); }, createResetPasswordToken(name: string, type: ResetPasswordType) { @@ -103,4 +112,23 @@ const service = { }, }; +function withExcludedField(user: User, excludeUserField: ExcludeUserField) { + const userReq = { ...user }; + switch (excludeUserField) { + case ExcludeUserField.AllTraits: { + delete userReq.allTraits; + break; + } + case ExcludeUserField.Traits: { + delete userReq.traits; + break; + } + default: { + excludeUserField satisfies never; + } + } + + return userReq; +} + export default service;