From 0f128438441fb8a8ee96c1ca457c274e186d4fcb Mon Sep 17 00:00:00 2001 From: Lisa Kim Date: Wed, 26 Jun 2024 11:15:33 -0700 Subject: [PATCH 1/2] Web: fix discover user update error --- .../Shared/SetupAccess/useUserTraits.test.tsx | 97 +++++++++++-------- .../Shared/SetupAccess/useUserTraits.ts | 15 +-- web/packages/teleport/src/Users/useUsers.ts | 10 +- .../teleport/src/services/user/types.ts | 4 + .../teleport/src/services/user/user.test.ts | 79 ++++++++++++++- .../teleport/src/services/user/user.ts | 40 +++++++- 6 files changed, 191 insertions(+), 54 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 0fdad31050da5..e1d3516aa8a7d 100644 --- a/web/packages/teleport/src/Discover/Shared/SetupAccess/useUserTraits.test.tsx +++ b/web/packages/teleport/src/Discover/Shared/SetupAccess/useUserTraits.test.tsx @@ -129,10 +129,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 }, + }, + 'allTraits' + ); // Test that updating meta correctly updated the dynamic traits. const updatedMeta = spyUpdateAgentMeta.mock.results[0].value as KubeMeta; @@ -216,14 +219,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'], + }, }, - }); + 'allTraits' + ); }); test('database', async () => { @@ -292,10 +298,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 }, + }, + 'allTraits' + ); // Test that updating meta correctly updated the dynamic traits. const updatedMeta = spyUpdateAgentMeta.mock.results[0].value as DbMeta; @@ -368,14 +377,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'], + }, }, - }); + 'allTraits' + ); }); test('node', async () => { @@ -436,10 +448,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 }, + }, + 'allTraits' + ); // Test that updating meta correctly updated the dynamic traits. const updatedMeta = spyUpdateAgentMeta.mock.results[0].value as NodeMeta; @@ -521,13 +536,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]], + }, }, - }); + 'allTraits' + ); // Test that app's awsRoles field got updated with the dynamic trait. const updatedMeta = spyUpdateAgentMeta.mock.results[0].value as AppMeta; @@ -590,13 +608,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'], + }, }, - }); + 'allTraits' + ); }); }); diff --git a/web/packages/teleport/src/Discover/Shared/SetupAccess/useUserTraits.ts b/web/packages/teleport/src/Discover/Shared/SetupAccess/useUserTraits.ts index a9c3a089dbf61..d9a00c7a764b6 100644 --- a/web/packages/teleport/src/Discover/Shared/SetupAccess/useUserTraits.ts +++ b/web/packages/teleport/src/Discover/Shared/SetupAccess/useUserTraits.ts @@ -328,13 +328,16 @@ export function useUserTraits() { setAttempt({ status: 'processing' }); try { await ctx.userService - .updateUser({ - ...user, - traits: { - ...user.traits, - ...newDynamicTraits, + .updateUser( + { + ...user, + traits: { + ...user.traits, + ...newDynamicTraits, + }, }, - }) + 'allTraits' /* exclude field */ + ) .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 ab82c64861aa1..47b14bd7091d2 100644 --- a/web/packages/teleport/src/Users/useUsers.ts +++ b/web/packages/teleport/src/Users/useUsers.ts @@ -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, 'traits' /* exclude field */) + .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, 'traits' /* exclude field */, webauthnResponse) .then(result => setUsers([result, ...users])) .then(() => ctx.userService.createResetPasswordToken( diff --git a/web/packages/teleport/src/services/user/types.ts b/web/packages/teleport/src/services/user/types.ts index 9ef3736dd27f1..68ac07fe26744 100644 --- a/web/packages/teleport/src/services/user/types.ts +++ b/web/packages/teleport/src/services/user/types.ts @@ -134,6 +134,10 @@ export interface User { allTraits?: AllUserTraits; } +// Backend does not allow User field "traits" and "allTraits" +// to be specified when creating or updating a user. +export type ExludeUserField = 'traits' | '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 a545ba7540539..39c5987e1dd33 100644 --- a/web/packages/teleport/src/services/user/user.test.ts +++ b/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 { 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, 'allTraits'); + expect(api.put).toHaveBeenCalledWith(cfg.api.usersPath, { + name: 'name', + roles: [], + traits: blankTraits, + }); + + jest.clearAllMocks(); + + await user.updateUser(userReq, '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, 'allTraits'); + expect(api.post).toHaveBeenCalledWith( + cfg.api.usersPath, + { + name: 'name', + roles: [], + traits: blankTraits, + }, + null, + undefined + ); + + jest.clearAllMocks(); + + await user.createUser(userReq, 'traits'); + expect(api.post).toHaveBeenCalledWith( + cfg.api.usersPath, + { + name: 'name', + roles: [], + allTraits: {}, + }, + null, + undefined + ); +}); + +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 488e3506ebd36..5653ab6266758 100644 --- a/web/packages/teleport/src/services/user/user.ts +++ b/web/packages/teleport/src/services/user/user.ts @@ -25,7 +25,7 @@ import { WebauthnAssertionResponse } from '../auth'; import makeUserContext from './makeUserContext'; import { makeResetToken } from './makeResetToken'; import makeUser, { makeUsers } from './makeUser'; -import { User, UserContext, ResetPasswordType } from './types'; +import { User, UserContext, ResetPasswordType, ExludeUserField } from './types'; const cache = { userContext: null as UserContext, @@ -65,8 +65,10 @@ const service = { * @param user * @returns user */ - updateUser(user: User) { - return api.put(cfg.getUsersUrl(), user).then(makeUser); + updateUser(user: User, excludeUserField: ExludeUserField) { + return api + .put(cfg.getUsersUrl(), withExcludedField(user, excludeUserField)) + .then(makeUser); }, /** @@ -76,9 +78,18 @@ const service = { * @param user * @returns user */ - createUser(user: User, webauthnResponse?: WebauthnAssertionResponse) { + createUser( + user: User, + excludeUserField: ExludeUserField, + webauthnResponse?: WebauthnAssertionResponse + ) { return api - .post(cfg.getUsersUrl(), user, null, webauthnResponse) + .post( + cfg.getUsersUrl(), + withExcludedField(user, excludeUserField), + null, + webauthnResponse + ) .then(makeUser); }, @@ -118,4 +129,23 @@ const service = { }, }; +function withExcludedField(user: User, excludeUserField: ExludeUserField) { + const userReq = { ...user }; + switch (excludeUserField) { + case 'allTraits': { + delete userReq.allTraits; + break; + } + case 'traits': { + delete userReq.traits; + break; + } + default: { + excludeUserField satisfies never; + } + } + + return userReq; +} + export default service; From c5ca815058540f7c3b930dd5157b82b819a0af21 Mon Sep 17 00:00:00 2001 From: Lisa Kim Date: Wed, 26 Jun 2024 12:27:01 -0700 Subject: [PATCH 2/2] Address CRs --- .../Shared/SetupAccess/useUserTraits.test.tsx | 15 ++++++++------- .../Shared/SetupAccess/useUserTraits.ts | 8 ++++++-- web/packages/teleport/src/Users/useUsers.ts | 6 +++--- .../teleport/src/services/user/types.ts | 9 ++++++--- .../teleport/src/services/user/user.test.ts | 10 +++++----- web/packages/teleport/src/services/user/user.ts | 17 +++++++++++------ 6 files changed, 39 insertions(+), 26 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 e1d3516aa8a7d..cf86ed8fa96b1 100644 --- a/web/packages/teleport/src/Discover/Shared/SetupAccess/useUserTraits.test.tsx +++ b/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'; @@ -134,7 +135,7 @@ describe('onProceed correctly deduplicates, removes static traits, updates meta, ...mockUser, traits: { ...mockUser.traits, ...expected }, }, - 'allTraits' + ExcludeUserField.AllTraits ); // Test that updating meta correctly updated the dynamic traits. @@ -228,7 +229,7 @@ describe('onProceed correctly deduplicates, removes static traits, updates meta, kubeUsers: ['dynamicKbUser3', 'dynamicKbUser4'], }, }, - 'allTraits' + ExcludeUserField.AllTraits ); }); @@ -303,7 +304,7 @@ describe('onProceed correctly deduplicates, removes static traits, updates meta, ...mockUser, traits: { ...mockUser.traits, ...expected }, }, - 'allTraits' + ExcludeUserField.AllTraits ); // Test that updating meta correctly updated the dynamic traits. @@ -386,7 +387,7 @@ describe('onProceed correctly deduplicates, removes static traits, updates meta, databaseUsers: ['apple'], }, }, - 'allTraits' + ExcludeUserField.AllTraits ); }); @@ -453,7 +454,7 @@ describe('onProceed correctly deduplicates, removes static traits, updates meta, ...mockUser, traits: { ...mockUser.traits, ...expected }, }, - 'allTraits' + ExcludeUserField.AllTraits ); // Test that updating meta correctly updated the dynamic traits. @@ -544,7 +545,7 @@ describe('onProceed correctly deduplicates, removes static traits, updates meta, awsRoleArns: [dynamicTraits.awsRoleArns[0]], }, }, - 'allTraits' + ExcludeUserField.AllTraits ); // Test that app's awsRoles field got updated with the dynamic trait. @@ -616,7 +617,7 @@ describe('onProceed correctly deduplicates, removes static traits, updates meta, logins: ['banana', 'carrot'], }, }, - 'allTraits' + ExcludeUserField.AllTraits ); }); }); diff --git a/web/packages/teleport/src/Discover/Shared/SetupAccess/useUserTraits.ts b/web/packages/teleport/src/Discover/Shared/SetupAccess/useUserTraits.ts index d9a00c7a764b6..8687a64c1d7a1 100644 --- a/web/packages/teleport/src/Discover/Shared/SetupAccess/useUserTraits.ts +++ b/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 @@ -336,7 +340,7 @@ export function useUserTraits() { ...newDynamicTraits, }, }, - 'allTraits' /* exclude field */ + ExcludeUserField.AllTraits ) .catch((error: Error) => { emitErrorEvent(`error updating user traits: ${error.message}`); diff --git a/web/packages/teleport/src/Users/useUsers.ts b/web/packages/teleport/src/Users/useUsers.ts index 47b14bd7091d2..c17a57fb10865 100644 --- a/web/packages/teleport/src/Users/useUsers.ts +++ b/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'; @@ -78,7 +78,7 @@ export default function useUsers({ function onUpdate(u: User) { return ctx.userService - .updateUser(u, 'traits' /* exclude field */) + .updateUser(u, ExcludeUserField.Traits) .then(result => { setUsers([result, ...users.filter(i => i.name !== u.name)]); }); @@ -87,7 +87,7 @@ export default function useUsers({ async function onCreate(u: User) { const webauthnResponse = await auth.getWebauthnResponseForAdminAction(true); return ctx.userService - .createUser(u, 'traits' /* exclude field */, webauthnResponse) + .createUser(u, ExcludeUserField.Traits, webauthnResponse) .then(result => setUsers([result, ...users])) .then(() => ctx.userService.createResetPasswordToken( diff --git a/web/packages/teleport/src/services/user/types.ts b/web/packages/teleport/src/services/user/types.ts index 68ac07fe26744..150a44cf40b1c 100644 --- a/web/packages/teleport/src/services/user/types.ts +++ b/web/packages/teleport/src/services/user/types.ts @@ -134,9 +134,12 @@ export interface User { allTraits?: AllUserTraits; } -// Backend does not allow User field "traits" and "allTraits" -// to be specified when creating or updating a user. -export type ExludeUserField = 'traits' | 'allTraits'; +// 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 { diff --git a/web/packages/teleport/src/services/user/user.test.ts b/web/packages/teleport/src/services/user/user.test.ts index 39c5987e1dd33..7b65e467e6a0c 100644 --- a/web/packages/teleport/src/services/user/user.test.ts +++ b/web/packages/teleport/src/services/user/user.test.ts @@ -21,7 +21,7 @@ import cfg from 'teleport/config'; import user from './user'; import { makeTraits } from './makeUser'; -import { PasswordState, User } from './types'; +import { ExcludeUserField, PasswordState, User } from './types'; test('undefined values in context response gives proper default values', async () => { const mockContext = { @@ -383,7 +383,7 @@ test('excludeUserFields when updating user', async () => { allTraits: {}, }; - await user.updateUser(userReq, 'allTraits'); + await user.updateUser(userReq, ExcludeUserField.AllTraits); expect(api.put).toHaveBeenCalledWith(cfg.api.usersPath, { name: 'name', roles: [], @@ -392,7 +392,7 @@ test('excludeUserFields when updating user', async () => { jest.clearAllMocks(); - await user.updateUser(userReq, 'traits'); + await user.updateUser(userReq, ExcludeUserField.Traits); expect(api.put).toHaveBeenCalledWith(cfg.api.usersPath, { name: 'name', roles: [], @@ -411,7 +411,7 @@ test('excludeUserFields when creating user', async () => { allTraits: {}, }; - await user.createUser(userReq, 'allTraits'); + await user.createUser(userReq, ExcludeUserField.AllTraits); expect(api.post).toHaveBeenCalledWith( cfg.api.usersPath, { @@ -425,7 +425,7 @@ test('excludeUserFields when creating user', async () => { jest.clearAllMocks(); - await user.createUser(userReq, 'traits'); + await user.createUser(userReq, ExcludeUserField.Traits); expect(api.post).toHaveBeenCalledWith( cfg.api.usersPath, { diff --git a/web/packages/teleport/src/services/user/user.ts b/web/packages/teleport/src/services/user/user.ts index 5653ab6266758..037dd6034d1b2 100644 --- a/web/packages/teleport/src/services/user/user.ts +++ b/web/packages/teleport/src/services/user/user.ts @@ -25,7 +25,12 @@ import { WebauthnAssertionResponse } from '../auth'; import makeUserContext from './makeUserContext'; import { makeResetToken } from './makeResetToken'; import makeUser, { makeUsers } from './makeUser'; -import { User, UserContext, ResetPasswordType, ExludeUserField } from './types'; +import { + User, + UserContext, + ResetPasswordType, + ExcludeUserField, +} from './types'; const cache = { userContext: null as UserContext, @@ -65,7 +70,7 @@ const service = { * @param user * @returns user */ - updateUser(user: User, excludeUserField: ExludeUserField) { + updateUser(user: User, excludeUserField: ExcludeUserField) { return api .put(cfg.getUsersUrl(), withExcludedField(user, excludeUserField)) .then(makeUser); @@ -80,7 +85,7 @@ const service = { */ createUser( user: User, - excludeUserField: ExludeUserField, + excludeUserField: ExcludeUserField, webauthnResponse?: WebauthnAssertionResponse ) { return api @@ -129,14 +134,14 @@ const service = { }, }; -function withExcludedField(user: User, excludeUserField: ExludeUserField) { +function withExcludedField(user: User, excludeUserField: ExcludeUserField) { const userReq = { ...user }; switch (excludeUserField) { - case 'allTraits': { + case ExcludeUserField.AllTraits: { delete userReq.allTraits; break; } - case 'traits': { + case ExcludeUserField.Traits: { delete userReq.traits; break; }