diff --git a/web/packages/shared/utils/highbar.test.ts b/web/packages/shared/utils/highbar.test.ts index 437a72b71010..b843e796d43b 100644 --- a/web/packages/shared/utils/highbar.test.ts +++ b/web/packages/shared/utils/highbar.test.ts @@ -16,12 +16,7 @@ * along with this program. If not, see . */ -import { - arrayObjectIsEqual, - equalsDeep, - freezeDeep, - mergeDeep, -} from './highbar'; +import { arrayObjectIsEqual, equalsDeep, mergeDeep } from './highbar'; describe('mergeDeep can merge two', () => { it('objects together', () => { @@ -227,39 +222,17 @@ describe('arrayObjectIsEqual correctly compares', () => { }); }); -describe('freezeDeep', () => { - it('freezes an empty object', () => { - const obj: { foo?: any } = freezeDeep({}); - expect(() => (obj.foo = 'bar')).toThrow(); - }); - it('freezes a flat object', () => { - const obj = freezeDeep({ foo: 'bar' }); - expect(() => (obj.foo = '123')).toThrow(); - }); - it('freezes a deep object', () => { - const obj = freezeDeep({ foo: { bar: 'baz' } }); - expect(() => (obj.foo = { bar: '123' })).toThrow(); - expect(() => (obj.foo.bar = '123')).toThrow(); - }); - it('freezes arrays', () => { - const obj = freezeDeep({ foo: ['bar'] }); - expect(() => obj.foo.push('123')).toThrow(); - }); - it('returns the argument', () => { - const obj = {}; - expect(freezeDeep(obj)).toBe(obj); - }); -}); - describe('equalsDeep', () => { it('compares primitive values', () => { expect(equalsDeep(1, 2)).toBe(false); expect(equalsDeep(2, 2)).toBe(true); }); + it('compares simple objects', () => { expect(equalsDeep({ a: 'b', c: 4 }, { c: 5, a: 'b' })).toBe(false); expect(equalsDeep({ a: 'b', c: 4 }, { c: 4, a: 'b' })).toBe(true); }); + it('compares complex objects', () => { expect( equalsDeep( @@ -311,4 +284,44 @@ describe('equalsDeep', () => { ) ).toBe(true); }); + + // A "real-world" example. + it('compares role options', () => { + const makeOptions = () => ({ + cert_format: 'standard', + create_db_user: false, + create_desktop_user: false, + desktop_clipboard: true, + desktop_directory_sharing: true, + enhanced_recording: ['command', 'network'], + forward_agent: false, + idp: { + saml: { + enabled: true, + }, + }, + max_session_ttl: '30h0m0s', + pin_source_ip: false, + port_forwarding: true, + record_session: { + default: 'best_effort', + desktop: true, + }, + ssh_file_copy: true, + }); + + expect(equalsDeep(makeOptions(), makeOptions())).toBe(true); + expect( + equalsDeep(makeOptions(), { + ...makeOptions(), + idp: { saml: { enabled: false } }, + }) + ).toBe(false); + expect( + equalsDeep(makeOptions(), { + ...makeOptions(), + unknown_option: 'foo', + }) + ).toBe(false); + }); }); diff --git a/web/packages/shared/utils/highbar.ts b/web/packages/shared/utils/highbar.ts index 245180e2a5fe..cf8f100369a2 100644 --- a/web/packages/shared/utils/highbar.ts +++ b/web/packages/shared/utils/highbar.ts @@ -60,27 +60,6 @@ export function mergeDeep(target: MergeTarget, ...sources: Array) { return mergeDeep(target, ...sources); } -/** Recursively freezes an object and its contents. */ -export function freezeDeep(obj: T): T { - // Source: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/freeze - // Any copyright is dedicated to the Public Domain: - // https://creativecommons.org/publicdomain/zero/1.0/ - - // Retrieve the property names defined on object - const propNames = Reflect.ownKeys(obj); - - // Freeze properties before freezing self - for (const name of propNames) { - const value = obj[name]; - - if ((value && typeof value === 'object') || typeof value === 'function') { - freezeDeep(value); - } - } - - return Object.freeze(obj); -} - /** Recursively compares two arrays. */ export function arrayObjectIsEqual(arr1: unknown[], arr2: unknown[]): boolean { return ( diff --git a/web/packages/teleport/src/Roles/RoleEditor/RoleEditor.test.tsx b/web/packages/teleport/src/Roles/RoleEditor/RoleEditor.test.tsx index 6a315b74a00b..78cb5d578f71 100644 --- a/web/packages/teleport/src/Roles/RoleEditor/RoleEditor.test.tsx +++ b/web/packages/teleport/src/Roles/RoleEditor/RoleEditor.test.tsx @@ -209,7 +209,7 @@ test('saving a new role', async () => { spec: { allow: {}, deny: {}, - options: defaultOptions, + options: defaultOptions(), }, version: 'v7', }, diff --git a/web/packages/teleport/src/Roles/RoleEditor/standardmodel.ts b/web/packages/teleport/src/Roles/RoleEditor/standardmodel.ts index 43d690f0ba61..6f8b9dd04537 100644 --- a/web/packages/teleport/src/Roles/RoleEditor/standardmodel.ts +++ b/web/packages/teleport/src/Roles/RoleEditor/standardmodel.ts @@ -67,7 +67,7 @@ export function newRole(): Role { spec: { allow: {}, deny: {}, - options: defaultOptions, + options: defaultOptions(), }, version: roleVersion, }; @@ -105,7 +105,7 @@ export function roleToRoleEditorModel( isEmpty(sRest) && isEmpty(allow) && isEmpty(deny) && - equalsDeep(options, defaultOptions) + equalsDeep(options, defaultOptions()) ), }; } @@ -133,7 +133,7 @@ export function roleEditorModelToRole(roleModel: RoleEditorModel): Role { spec: { allow: {}, deny: {}, - options: defaultOptions, + options: defaultOptions(), }, version, }; diff --git a/web/packages/teleport/src/Roles/RoleEditor/withDefaults.ts b/web/packages/teleport/src/Roles/RoleEditor/withDefaults.ts index f4b96786192e..15f910c99065 100644 --- a/web/packages/teleport/src/Roles/RoleEditor/withDefaults.ts +++ b/web/packages/teleport/src/Roles/RoleEditor/withDefaults.ts @@ -56,27 +56,30 @@ export const withDefaults = (role: DeepPartial): Role => ({ const optionsWithDefaults = ( options?: DeepPartial -): RoleOptions => ({ - ...defaultOptions, - ...options, +): RoleOptions => { + const defaults = defaultOptions(); + return { + ...defaults, + ...options, - idp: { - ...defaultOptions.idp, - ...options?.idp, + idp: { + ...defaults.idp, + ...options?.idp, - saml: { - ...defaultOptions.idp.saml, - ...options?.idp?.saml, + saml: { + ...defaults.idp.saml, + ...options?.idp?.saml, + }, }, - }, - record_session: { - ...defaultOptions.record_session, - ...options?.record_session, - }, -}); + record_session: { + ...defaults.record_session, + ...options?.record_session, + }, + }; +}; -export const defaultOptions: RoleOptions = freezeDeep({ +export const defaultOptions = (): RoleOptions => ({ cert_format: 'standard', create_db_user: false, create_desktop_user: false, diff --git a/web/packages/teleport/src/services/resources/types.ts b/web/packages/teleport/src/services/resources/types.ts index 9f37f99efa6d..a627185d09a3 100644 --- a/web/packages/teleport/src/services/resources/types.ts +++ b/web/packages/teleport/src/services/resources/types.ts @@ -39,7 +39,7 @@ export type Kind = export type RoleResource = Resource; /** - * Teleport role in full format. + * Teleport role in full format, as returned from Teleport API. * TODO(bl-nero): Add all fields supported on the UI side. */ export type Role = { @@ -63,6 +63,10 @@ type RoleConditions = { node_labels?: Record; }; +/** + * Teleport role options in full format, as returned from Teleport API. Note + * that its fields follow the snake case convention to match the wire format. + */ export type RoleOptions = { cert_format: string; create_db_user: boolean; @@ -72,6 +76,11 @@ export type RoleOptions = { enhanced_recording: string[]; forward_agent: boolean; idp: { + // There's a subtle quirk in `Rolev6.CheckAndSetDefaults`: if you ask + // Teleport to create a resource with `idp` field set to null, it's instead + // going to create the entire idp->saml->enabled structure. However, it's + // possible to create a role with idp set to an empty object, and the + // server will retain this state. This makes the `saml` field optional. saml?: { enabled: boolean; };