fix(next): version diff view not handling all field permissions correctly (#13721)

Fixes https://github.com/payloadcms/payload/issues/13286 The version diff view did not handle all field permissions correctly, leading to some fields disappearing if access control was set. This PR fixes that. --- - To see the specific tasks where the Asana app for GitHub is being used, see below: - https://app.asana.com/0/0/1211267837905375
2025-09-09 08:31:15 -07:00
parent e49005fcbd
commit 911f17a887
12 changed files with 184 additions and 69 deletions
--- a/packages/next/src/views/Version/RenderFieldsToDiff/buildVersionFields.tsx
+++ b/packages/next/src/views/Version/RenderFieldsToDiff/buildVersionFields.tsx
@@ -15,13 +15,13 @@ import {
  type PayloadComponent,
  type PayloadRequest,
  type SanitizedFieldPermissions,
+  type SanitizedFieldsPermissions,
  type VersionField,
 } from 'payload'
 import {
  fieldIsID,
  fieldShouldBeLocalized,
  getFieldPaths,
-  getFieldPermissions,
  getUniqueListBy,
  tabHasName,
 } from 'payload/shared'
@@ -34,12 +34,8 @@ export type BuildVersionFieldsArgs = {
    Record<FieldTypes, PayloadComponent<FieldDiffServerProps, FieldDiffClientProps>>
  >
  entitySlug: string
-  fieldPermissions:
-    | {
-        [key: string]: SanitizedFieldPermissions
-      }
-    | true
  fields: Field[]
+  fieldsPermissions: SanitizedFieldsPermissions
  i18n: I18nClient
  modifiedOnly: boolean
  nestingLevel?: number
@@ -64,8 +60,8 @@ export const buildVersionFields = ({
  clientSchemaMap,
  customDiffComponents,
  entitySlug,
-  fieldPermissions,
  fields,
+  fieldsPermissions,
  i18n,
  modifiedOnly,
  nestingLevel = 0,
@@ -131,12 +127,12 @@ export const buildVersionFields = ({
          customDiffComponents,
          entitySlug,
          field,
-          fieldPermissions,
          i18n,
          indexPath,
          locale,
          modifiedOnly,
          nestingLevel,
+          parentFieldsPermissions: fieldsPermissions,
          parentIsLocalized: true,
          parentPath,
          parentSchemaPath,
@@ -158,11 +154,11 @@ export const buildVersionFields = ({
        customDiffComponents,
        entitySlug,
        field,
-        fieldPermissions,
        i18n,
        indexPath,
        modifiedOnly,
        nestingLevel,
+        parentFieldsPermissions: fieldsPermissions,
        parentIsLocalized: parentIsLocalized || ('localized' in field && field.localized),
        parentPath,
        parentSchemaPath,
@@ -198,12 +194,12 @@ const buildVersionField = ({
  customDiffComponents,
  entitySlug,
  field,
-  fieldPermissions,
  i18n,
  indexPath,
  locale,
  modifiedOnly,
  nestingLevel,
+  parentFieldsPermissions,
  parentIsLocalized,
  parentPath,
  parentSchemaPath,
@@ -220,6 +216,7 @@ const buildVersionField = ({
  locale?: string
  modifiedOnly?: boolean
  nestingLevel: number
+  parentFieldsPermissions: SanitizedFieldsPermissions
  parentIsLocalized: boolean
  path: string
  schemaPath: string
@@ -227,18 +224,35 @@ const buildVersionField = ({
  valueTo: unknown
 } & Omit<
  BuildVersionFieldsArgs,
-  'fields' | 'parentIndexPath' | 'versionFromSiblingData' | 'versionToSiblingData'
+  | 'fields'
+  | 'fieldsPermissions'
+  | 'parentIndexPath'
+  | 'versionFromSiblingData'
+  | 'versionToSiblingData'
 >): BaseVersionField | null => {
-  const { permissions, read: hasReadPermission } = getFieldPermissions({
-    field,
-    operation: 'read',
-    parentName: parentPath?.includes('.')
-      ? parentPath.split('.')[parentPath.split('.').length - 1]
-      : parentPath,
-    permissions: fieldPermissions,
-  })
+  let hasReadPermission: boolean = false
+  let fieldPermissions: SanitizedFieldPermissions | undefined = undefined
+
+  if (typeof parentFieldsPermissions === 'boolean') {
+    hasReadPermission = parentFieldsPermissions
+    fieldPermissions = parentFieldsPermissions
+  } else {
+    if ('name' in field) {
+      fieldPermissions = parentFieldsPermissions?.[field.name]
+      if (typeof fieldPermissions === 'boolean') {
+        hasReadPermission = fieldPermissions
+      } else if (typeof fieldPermissions?.read === 'boolean') {
+        hasReadPermission = fieldPermissions.read
+      }
+    } else {
+      // If the field is unnamed and parentFieldsPermissions is an object, its sub-fields will decide their read permissions state.
+      // As far as this field is concerned, we are allowed to read it, as we need to reach its sub-fields to determine their read permissions.
+      hasReadPermission = true
+    }
+  }

  if (!hasReadPermission) {
+    // HasReadPermission is only valid if the field has a name. E.g. for a tabs field it would incorrectly return `false`.
    return null
  }

@@ -294,18 +308,21 @@ const buildVersionField = ({
        parentSchemaPath,
      })

-      let tabPermissions: typeof fieldPermissions = undefined
+      let tabFieldsPermissions: SanitizedFieldsPermissions = undefined

-      if (typeof permissions === 'boolean') {
-        tabPermissions = permissions
-      } else if (permissions && typeof permissions === 'object') {
-        if ('name' in tab) {
-          tabPermissions =
-            typeof permissions.fields?.[tab.name] === 'object'
-              ? permissions.fields?.[tab.name].fields
-              : permissions.fields?.[tab.name]
+      // The tabs field does not have its own permissions as it's unnamed => use parentFieldsPermissions
+      if (typeof parentFieldsPermissions === 'boolean') {
+        tabFieldsPermissions = parentFieldsPermissions
      } else {
-          tabPermissions = permissions.fields
+        if ('name' in tab) {
+          const tabPermissions = parentFieldsPermissions?.[tab.name]
+          if (typeof tabPermissions === 'boolean') {
+            tabFieldsPermissions = tabPermissions
+          } else {
+            tabFieldsPermissions = tabPermissions?.fields
+          }
+        } else {
+          tabFieldsPermissions = parentFieldsPermissions
        }
      }

@@ -315,8 +332,8 @@ const buildVersionField = ({
          clientSchemaMap,
          customDiffComponents,
          entitySlug,
-          fieldPermissions: tabPermissions,
          fields: tab.fields,
+          fieldsPermissions: tabFieldsPermissions,
          i18n,
          modifiedOnly,
          nestingLevel: nestingLevel + 1,
@@ -343,15 +360,19 @@ const buildVersionField = ({
    if (modifiedOnly && !baseVersionField.tabs.length) {
      return null
    }
-  } // At this point, we are dealing with a `row`, `collapsible`, etc
+  } // At this point, we are dealing with a `row`, `collapsible`, array`, etc
  else if ('fields' in field) {
-    let subfieldPermissions: typeof fieldPermissions = undefined
+    let subFieldsPermissions: SanitizedFieldsPermissions = undefined

-    if (typeof permissions === 'boolean') {
-      subfieldPermissions = permissions
-    } else if (permissions && typeof permissions === 'object') {
-      subfieldPermissions = permissions.fields
+    if ('name' in field && typeof fieldPermissions !== 'undefined') {
+      // Named fields like arrays
+      subFieldsPermissions =
+        typeof fieldPermissions === 'boolean' ? fieldPermissions : fieldPermissions.fields
+    } else {
+      // Unnamed fields like collapsible and row inherit directly from parent permissions
+      subFieldsPermissions = parentFieldsPermissions
    }
+
    if (field.type === 'array' && (valueTo || valueFrom)) {
      const maxLength = Math.max(
        Array.isArray(valueTo) ? valueTo.length : 0,
@@ -367,8 +388,8 @@ const buildVersionField = ({
          clientSchemaMap,
          customDiffComponents,
          entitySlug,
-          fieldPermissions: subfieldPermissions,
          fields: field.fields,
+          fieldsPermissions: subFieldsPermissions,
          i18n,
          modifiedOnly,
          nestingLevel: nestingLevel + 1,
@@ -391,8 +412,8 @@ const buildVersionField = ({
        clientSchemaMap,
        customDiffComponents,
        entitySlug,
-        fieldPermissions: subfieldPermissions,
        fields: field.fields,
+        fieldsPermissions: subFieldsPermissions,
        i18n,
        modifiedOnly,
        nestingLevel: field.type !== 'row' ? nestingLevel + 1 : nestingLevel,
@@ -449,16 +470,19 @@ const buildVersionField = ({
        }
      }

-      let blockPermissions: typeof fieldPermissions = undefined
+      let blockFieldsPermissions: SanitizedFieldsPermissions = undefined

-      if (permissions === true) {
-        blockPermissions = true
+      // fieldPermissions will be set here, as the blocks field has a name
+      if (typeof fieldPermissions === 'boolean') {
+        blockFieldsPermissions = fieldPermissions
+      } else if (typeof fieldPermissions?.blocks === 'boolean') {
+        blockFieldsPermissions = fieldPermissions.blocks
      } else {
-        const permissionsBlockSpecific = permissions?.blocks?.[blockSlugToMatch]
-        if (permissionsBlockSpecific === true) {
-          blockPermissions = true
+        const permissionsBlockSpecific = fieldPermissions?.blocks?.[blockSlugToMatch]
+        if (typeof permissionsBlockSpecific === 'boolean') {
+          blockFieldsPermissions = permissionsBlockSpecific
        } else {
-          blockPermissions = permissionsBlockSpecific?.fields
+          blockFieldsPermissions = permissionsBlockSpecific?.fields
        }
      }

@@ -466,8 +490,8 @@ const buildVersionField = ({
        clientSchemaMap,
        customDiffComponents,
        entitySlug,
-        fieldPermissions: blockPermissions,
        fields,
+        fieldsPermissions: blockFieldsPermissions,
        i18n,
        modifiedOnly,
        nestingLevel: nestingLevel + 1,
@@ -500,7 +524,8 @@ const buildVersionField = ({
     */
    diffMethod: 'diffWordsWithSpace',
    field: clientField,
-    fieldPermissions: typeof permissions === 'object' ? permissions.fields : permissions,
+    fieldPermissions:
+      typeof fieldPermissions === 'undefined' ? parentFieldsPermissions : fieldPermissions,
    parentIsLocalized,

    nestingLevel: nestingLevel ? nestingLevel : undefined,
--- a/packages/next/src/views/Version/index.tsx
+++ b/packages/next/src/views/Version/index.tsx
@@ -223,8 +223,8 @@ export async function VersionView(props: DocumentViewServerProps) {
    clientSchemaMap,
    customDiffComponents: {},
    entitySlug: collectionSlug || globalSlug,
-    fieldPermissions: docPermissions?.fields,
    fields: (collectionConfig || globalConfig)?.fields,
+    fieldsPermissions: docPermissions?.fields,
    i18n,
    modifiedOnly,
    parentIndexPath: '',
--- a/packages/payload/src/admin/forms/Diff.ts
+++ b/packages/payload/src/admin/forms/Diff.ts
@@ -5,6 +5,7 @@ import type {
  ClientFieldWithOptionalType,
  PayloadRequest,
  SanitizedFieldPermissions,
+  SanitizedFieldsPermissions,
 } from '../../index.js'

 export type VersionTab = {
@@ -54,11 +55,10 @@ export type FieldDiffClientProps<TClientField extends ClientFieldWithOptionalTyp
   */
  diffMethod: any
  field: TClientField
-  fieldPermissions:
-    | {
-        [key: string]: SanitizedFieldPermissions
-      }
-    | true
+  /**
+   * Permissions at this level of the field. If this field is unnamed, this will be `SanitizedFieldsPermissions` - if it is named, it will be `SanitizedFieldPermissions`
+   */
+  fieldPermissions: SanitizedFieldPermissions | SanitizedFieldsPermissions
  /**
   * If this field is localized, this will be the locale of the field
   */
--- a/packages/payload/src/auth/types.ts
+++ b/packages/payload/src/auth/types.ts
@@ -28,9 +28,11 @@ export type SanitizedBlockPermissions =
    }
  | true

-export type BlocksPermissions = {
+export type BlocksPermissions =
+  | {
      [blockSlug: string]: BlockPermissions
-}
+    }
+  | true

 export type SanitizedBlocksPermissions =
  | {
--- a/packages/payload/src/utilities/getFieldPermissions.ts
+++ b/packages/payload/src/utilities/getFieldPermissions.ts
@@ -1,5 +1,5 @@
 /* eslint-disable @typescript-eslint/no-explicit-any */
-import type { SanitizedFieldPermissions } from '../auth/types.js'
+import type { SanitizedFieldPermissions, SanitizedFieldsPermissions } from '../auth/types.js'
 import type { ClientField, Field } from '../fields/config/types.js'
 import type { Operation } from '../types/index.js'

@@ -19,14 +19,14 @@ export const getFieldPermissions = ({
  readonly field: ClientField | Field
  readonly operation: Operation
  readonly parentName: string
-  readonly permissions:
-    | {
-        [fieldName: string]: SanitizedFieldPermissions
-      }
-    | SanitizedFieldPermissions
+  readonly permissions: SanitizedFieldPermissions | SanitizedFieldsPermissions
 }): {
  operation: boolean
-  permissions: SanitizedFieldPermissions
+  /**
+   * The field-level permissions. This can be equal to the permissions passed to the
+   * `getFieldPermissions` function if the field has no name.
+   */
+  permissions: SanitizedFieldPermissions | SanitizedFieldsPermissions
  read: boolean
 } => ({
  operation:
--- a/packages/ui/src/elements/FieldSelect/reduceFieldOptions.ts
+++ b/packages/ui/src/elements/FieldSelect/reduceFieldOptions.ts
@@ -120,7 +120,7 @@ export const reduceFieldOptions = ({
      label: combineFieldLabel({ CustomLabel, field, prefix: labelPrefix }),
      value: {
        field,
-        fieldPermissions,
+        fieldPermissions: fieldPermissions as SanitizedFieldPermissions,
        path: createNestedClientFieldPath(path, field),
      },
    }
--- a/packages/ui/src/forms/RenderFields/index.tsx
+++ b/packages/ui/src/forms/RenderFields/index.tsx
@@ -1,5 +1,7 @@
 'use client'

+import type { SanitizedFieldPermissions } from 'payload'
+
 import { fieldIsHiddenOrDisabled, getFieldPaths, getFieldPermissions } from 'payload/shared'
 import React from 'react'

@@ -7,8 +9,8 @@ import type { RenderFieldsProps } from './types.js'

 import { RenderIfInViewport } from '../../elements/RenderIfInViewport/index.js'
 import { useOperation } from '../../providers/Operation/index.js'
-import { FieldPathContext } from './context.js'
 import './index.scss'
+import { FieldPathContext } from './context.js'
 import { RenderField } from './RenderField.js'

 const baseClass = 'render-fields'
@@ -99,7 +101,7 @@ export const RenderFields: React.FC<RenderFieldsProps> = (props) => {
                parentPath={parentPath}
                parentSchemaPath={parentSchemaPath}
                path={path}
-                permissions={fieldPermissions}
+                permissions={fieldPermissions as SanitizedFieldPermissions}
                readOnly={isReadOnly}
                schemaPath={schemaPath}
              />
--- a/packages/ui/src/views/Edit/Auth/index.tsx
+++ b/packages/ui/src/views/Edit/Auth/index.tsx
@@ -1,5 +1,7 @@
 'use client'

+import type { SanitizedFieldPermissions } from 'payload'
+
 import { getFieldPermissions } from 'payload/shared'
 import React, { Fragment, useCallback, useEffect, useMemo, useState } from 'react'
 import { toast } from 'sonner'
@@ -16,8 +18,8 @@ import { useAuth } from '../../../providers/Auth/index.js'
 import { useConfig } from '../../../providers/Config/index.js'
 import { useDocumentInfo } from '../../../providers/DocumentInfo/index.js'
 import { useTranslation } from '../../../providers/Translation/index.js'
-import { APIKey } from './APIKey.js'
 import './index.scss'
+import { APIKey } from './APIKey.js'

 const baseClass = 'auth-fields'

@@ -52,7 +54,7 @@ export const Auth: React.FC<Props> = (props) => {
    },
  } = useConfig()

-  let showPasswordFields = true
+  let showPasswordFields: SanitizedFieldPermissions = true
  let showUnlock = true
  const hasPasswordFieldOverride =
    typeof docPermissions.fields === 'object' && 'password' in docPermissions.fields
@@ -71,11 +73,13 @@ export const Auth: React.FC<Props> = (props) => {
    if (operation === 'create') {
      showPasswordFields =
        passwordPermissions === true ||
-        (typeof passwordPermissions === 'object' && passwordPermissions.create)
+        ((typeof passwordPermissions === 'object' &&
+          passwordPermissions.create) as SanitizedFieldPermissions)
    } else {
      showPasswordFields =
        passwordPermissions === true ||
-        (typeof passwordPermissions === 'object' && passwordPermissions.update)
+        ((typeof passwordPermissions === 'object' &&
+          passwordPermissions.update) as SanitizedFieldPermissions)
    }
  }

--- a/test/versions/collections/Diff/index.ts
+++ b/test/versions/collections/Diff/index.ts
@@ -91,6 +91,13 @@ export const Diff: CollectionConfig = {
                      name: 'textInUnnamedTab2InBlock',
                      type: 'text',
                    },
+                    {
+                      name: 'textInUnnamedTab2InBlockAccessFalse',
+                      type: 'text',
+                      access: {
+                        read: () => false,
+                      },
+                    },
                    {
                      type: 'row',
                      fields: [
@@ -220,6 +227,13 @@ export const Diff: CollectionConfig = {
      ],
      type: 'row',
    },
+    {
+      name: 'textCannotRead',
+      type: 'text',
+      access: {
+        read: () => false,
+      },
+    },
    {
      name: 'select',
      type: 'select',
@@ -244,6 +258,20 @@ export const Diff: CollectionConfig = {
              name: 'textInNamedTab1',
              type: 'text',
            },
+            {
+              name: 'textInNamedTab1ReadFalse',
+              type: 'text',
+              access: {
+                read: () => false,
+              },
+            },
+            {
+              name: 'textInNamedTab1UpdateFalse',
+              type: 'text',
+              access: {
+                update: () => false,
+              },
+            },
          ],
        },
        {
@@ -253,6 +281,22 @@ export const Diff: CollectionConfig = {
              name: 'textInUnnamedTab2',
              type: 'text',
            },
+            {
+              type: 'row',
+              fields: [
+                {
+                  name: 'textInRowInUnnamedTab',
+                  type: 'text',
+                },
+                {
+                  name: 'textInRowInUnnamedTabUpdateFalse',
+                  type: 'text',
+                  access: {
+                    update: () => false,
+                  },
+                },
+              ],
+            },
          ],
        },
      ],
--- a/test/versions/e2e.spec.ts
+++ b/test/versions/e2e.spec.ts
@@ -1981,6 +1981,22 @@ describe('Versions', () => {

      const hiddenField2 = page.locator('[data-field-path="textCannotRead"]')
      await expect(hiddenField2).toBeHidden()
+
+      const hiddenField3 = page.locator('[data-field-path="namedTab1.textInNamedTab1ReadFalse"]')
+      await expect(hiddenField3).toBeHidden()
+
+      const visibleFieldWithUpdateFalse1 = page.locator(
+        '[data-field-path="namedTab1.textInNamedTab1UpdateFalse"]',
+      )
+      await expect(visibleFieldWithUpdateFalse1).toBeVisible()
+
+      const visibleField2 = page.locator('[data-field-path="textInRowInUnnamedTab"]')
+      await expect(visibleField2).toBeVisible()
+
+      const visibleFieldWithUpdateFalse3 = page.locator(
+        '[data-field-path="textInRowInUnnamedTabUpdateFalse"]',
+      )
+      await expect(visibleFieldWithUpdateFalse3).toBeVisible()
    })

    test('correctly renders diff for relationship fields with deleted relation', async () => {
--- a/test/versions/payload-types.ts
+++ b/test/versions/payload-types.ts
@@ -412,6 +412,7 @@ export interface Diff {
              textInNamedTab1InBlock?: string | null;
            };
            textInUnnamedTab2InBlock?: string | null;
+            textInUnnamedTab2InBlockAccessFalse?: string | null;
            textInRowInUnnamedTab2InBlock?: string | null;
            id?: string | null;
            blockName?: string | null;
@@ -509,11 +510,16 @@ export interface Diff {
    [k: string]: unknown;
  } | null;
  textInRow?: string | null;
+  textCannotRead?: string | null;
  select?: ('option1' | 'option2') | null;
  namedTab1?: {
    textInNamedTab1?: string | null;
+    textInNamedTab1ReadFalse?: string | null;
+    textInNamedTab1UpdateFalse?: string | null;
  };
  textInUnnamedTab2?: string | null;
+  textInRowInUnnamedTab?: string | null;
+  textInRowInUnnamedTabUpdateFalse?: string | null;
  text?: string | null;
  textArea?: string | null;
  upload?: (string | null) | Media;
@@ -1030,6 +1036,7 @@ export interface DiffSelect<T extends boolean = true> {
                    textInNamedTab1InBlock?: T;
                  };
              textInUnnamedTab2InBlock?: T;
+              textInUnnamedTab2InBlockAccessFalse?: T;
              textInRowInUnnamedTab2InBlock?: T;
              id?: T;
              blockName?: T;
@@ -1057,13 +1064,18 @@ export interface DiffSelect<T extends boolean = true> {
  richtext?: T;
  richtextWithCustomDiff?: T;
  textInRow?: T;
+  textCannotRead?: T;
  select?: T;
  namedTab1?:
    | T
    | {
        textInNamedTab1?: T;
+        textInNamedTab1ReadFalse?: T;
+        textInNamedTab1UpdateFalse?: T;
      };
  textInUnnamedTab2?: T;
+  textInRowInUnnamedTab?: T;
+  textInRowInUnnamedTabUpdateFalse?: T;
  text?: T;
  textArea?: T;
  upload?: T;
--- a/test/versions/seed.ts
+++ b/test/versions/seed.ts
@@ -248,6 +248,8 @@ export async function seed(_payload: Payload, parallel: boolean = false) {
      },
      namedTab1: {
        textInNamedTab1: 'textInNamedTab1',
+        textInNamedTab1ReadFalse: 'textInNamedTab1ReadFalse',
+        textInNamedTab1UpdateFalse: 'textInNamedTab1UpdateFalse',
      },
      number: 1,
      point: [1, 2],
@@ -276,6 +278,9 @@ export async function seed(_payload: Payload, parallel: boolean = false) {
      textInCollapsible: 'textInCollapsible',
      textInRow: 'textInRow',
      textInUnnamedTab2: 'textInUnnamedTab2',
+      textInRowInUnnamedTab: 'textInRowInUnnamedTab',
+      textInRowInUnnamedTabUpdateFalse: 'textInRowInUnnamedTabUpdateFalse',
+
      textCannotRead: 'textCannotRead',
      relationshipPolymorphic: {
        relationTo: 'text',
@@ -386,6 +391,8 @@ export async function seed(_payload: Payload, parallel: boolean = false) {
      },
      namedTab1: {
        textInNamedTab1: 'textInNamedTab12',
+        textInNamedTab1ReadFalse: 'textInNamedTab1ReadFalse2',
+        textInNamedTab1UpdateFalse: 'textInNamedTab1UpdateFalse2',
      },
      number: 2,
      json: {
@@ -439,6 +446,9 @@ export async function seed(_payload: Payload, parallel: boolean = false) {
      textInRow: 'textInRow2',
      textCannotRead: 'textCannotRead2',
      textInUnnamedTab2: 'textInUnnamedTab22',
+      textInRowInUnnamedTab: 'textInRowInUnnamedTab2',
+      textInRowInUnnamedTabUpdateFalse: 'textInRowInUnnamedTabUpdateFalse2',
+
      upload: uploadedImage2,
      uploadHasMany: [uploadedImage, uploadedImage2],
    },