Remove freezeDeep; review

web/packages/shared/utils/highbar.test.ts

@@ -16,12 +16,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-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);
+  });
 });

web/packages/shared/utils/highbar.ts

@@ -60,27 +60,6 @@ export function mergeDeep(target: MergeTarget, ...sources: Array<MergeTarget>) {
   return mergeDeep(target, ...sources);
 }
 
-/** Recursively freezes an object and its contents. */
-export function freezeDeep<T extends object>(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 (

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',
     },

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,
   };

web/packages/teleport/src/Roles/RoleEditor/withDefaults.ts

@@ -56,27 +56,30 @@ export const withDefaults = (role: DeepPartial<Role>): Role => ({
 
 const optionsWithDefaults = (
   options?: DeepPartial<RoleOptions>
-): 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,

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

@@ -39,7 +39,7 @@ export type Kind =
 export type RoleResource = Resource<KindRole>;
 
 /**
- * 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<string, string[]>;
 };
 
+/**
+ * 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;
     };