fix(history): handle sanitization when fetching versions

packages/core/content-manager/admin/src/history/components/VersionInputRenderer.tsx

@@ -64,10 +64,8 @@ const CustomRelationInput = (props: RelationsFieldProps) => {
   >(props.name);
 
   /**
-   * TODO:
-   *
    * Ideally the server would return the correct shape, however, for admin user relations
-   * it sanitizes everyything out when it finds an object for the relation value.
+   * it sanitizes everything out when it finds an object for the relation value.
    */
   const formattedFieldValue = Array.isArray(field.value)
     ? {

packages/core/content-manager/server/src/history/constants.ts

@@ -5,7 +5,6 @@ export const FIELDS_TO_IGNORE = [
   'publishedAt',
   'createdBy',
   'updatedBy',
-  'locale',
   'strapi_stage',
   'strapi_assignee',
 ];

packages/core/content-manager/server/src/history/controllers/history-version.ts

@@ -77,10 +77,7 @@ const createHistoryVersionController = ({ strapi }: { strapi: Core.Strapi }) =>
         async (version: HistoryVersions.HistoryVersionDataResponse & { locale: string }) => {
           return {
             ...version,
-            data: await permissionChecker.sanitizeOutput({
-              ...version.data,
-              locale: version.locale.code,
-            }),
+            data: await permissionChecker.sanitizeOutput(version.data),
             createdBy: version.createdBy
               ? pick(['id', 'firstname', 'lastname', 'username', 'email'], version.createdBy)
               : undefined,

packages/core/content-manager/server/src/history/services/history.ts

@@ -307,7 +307,6 @@ const createHistoryService = ({ strapi }: { strapi: Core.Strapi }) => {
                   userAbility: params.state.userAbility,
                   model: attributeSchema.target,
                 });
-
                 const sanitizedEntry = await permissionChecker.sanitizeOutput(relatedEntry);
 
                 if (sanitizedEntry) {
@@ -352,12 +351,19 @@ const createHistoryService = ({ strapi }: { strapi: Core.Strapi }) => {
                   return currentRelationData;
                 }
 
+                const permissionChecker = getContentManagerService('permission-checker').create({
+                  userAbility: params.state.userAbility,
+                  model: 'plugin::upload.file',
+                });
+
                 const relatedEntry = await strapi.db
                   .query('plugin::upload.file')
                   .findOne({ where: { id: entry.id } });
 
-                if (relatedEntry) {
-                  currentRelationData.results.push(relatedEntry);
+                const sanitizedEntry = await permissionChecker.sanitizeOutput(relatedEntry);
+
+                if (sanitizedEntry) {
+                  currentRelationData.results.push(sanitizedEntry);
                 } else {
                   // The related content has been deleted
                   currentRelationData.meta.missingCount += 1;
@@ -398,41 +404,29 @@ const createHistoryService = ({ strapi }: { strapi: Core.Strapi }) => {
             ) {
               /**
                * Don't build the relations response object for relations to admin users,
-               * because pickAllowedAdminUserFields sanitizati
+               * because pickAllowedAdminUserFields will sanitize the data in the controller.
                */
               if (attributeSchema.target === 'admin::user') {
-                const sanitizedAdminUser = await Promise.all(
+                const adminUsers = await Promise.all(
                   attributeValues.map(async (userToPopulate) => {
                     if (userToPopulate == null) {
                       return null;
                     }
 
-                    const user = await strapi
+                    return strapi
                       .query('admin::user')
                       .findOne({ where: { id: userToPopulate.id } });
-
-                    const permissionChecker = getContentManagerService('permission-checker').create(
-                      {
-                        userAbility: params.state.userAbility,
-                        model: 'admin::user',
-                      }
-                    );
-                    const sanitizedUser = await permissionChecker.sanitizeOutput(user);
-
-                    return sanitizedUser;
                   })
                 );
 
                 return {
                   ...(await currentDataWithRelations),
                   /**
-                   * TODO:
-                   *
                    * Ideally we would return the same "{results: [], meta: {}}" shape, however,
                    * when sanitizing the data as a whole in the controller before sending to the client,
                    * the data for admin relation user is completely sanitized if we return an object here as opposed to an array.
                    */
-                  [attributeKey]: sanitizedAdminUser,
+                  [attributeKey]: adminUsers,
                 };
               }
 

tests/e2e/tests/content-manager/history.spec.ts

@@ -1,4 +1,4 @@
-import { test, expect } from '@playwright/test';
+import { test, expect, Page } from '@playwright/test';
 import { login } from '../../utils/login';
 import { resetDatabaseAndImportDataFromPath } from '../../utils/dts-import';
 import { describeOnCondition } from '../../utils/shared';
@@ -7,6 +7,14 @@ import { waitForRestart } from '../../utils/restart';
 
 const edition = process.env.STRAPI_DISABLE_EE === 'true' ? 'CE' : 'EE';
 
+const goToHistoryPage = async (page: Page) => {
+  const moreActionsButton = await page.getByRole('button', { name: /more actions/i });
+  await expect(moreActionsButton).toBeEnabled();
+  await moreActionsButton.click();
+  const historyButton = await page.getByRole('menuitem', { name: /content history/i });
+  await historyButton.click();
+};
+
 describeOnCondition(edition === 'EE')('History', () => {
   test.describe('Collection Type', () => {
     test.beforeEach(async ({ page }) => {
@@ -43,8 +51,8 @@ describeOnCondition(edition === 'EE')('History', () => {
       await titleInput.fill(frenchTitle);
       await page.getByRole('button', { name: 'Save' }).click();
       // Go to the history page
-      await page.getByRole('button', { name: /more actions/i }).click();
-      await page.getByRole('menuitem', { name: /content history/i }).click();
+      await goToHistoryPage(page);
+
       await page.waitForURL(HISTORY_URL);
       await expect(titleInput).toHaveValue(frenchTitle);
 
@@ -59,8 +67,7 @@ describeOnCondition(edition === 'EE')('History', () => {
       await titleInput.fill(englishTitle);
       await page.getByRole('button', { name: 'Save' }).click();
       // Go to the history page
-      await page.getByRole('button', { name: /more actions/i }).click();
-      await page.getByRole('menuitem', { name: /content history/i }).click();
+      await goToHistoryPage(page);
       await page.waitForURL(HISTORY_URL);
       const versionCards = await page.getByRole('listitem', { name: 'Version card' });
       await expect(versionCards).toHaveCount(1);
@@ -85,8 +92,7 @@ describeOnCondition(edition === 'EE')('History', () => {
       await titleInput.fill('Being from Kansas City');
       await page.getByRole('button', { name: 'Save' }).click();
       // Go to the history page
-      await page.getByRole('button', { name: /more actions/i }).click();
-      await page.getByRole('menuitem', { name: /content history/i }).click();
+      await goToHistoryPage(page);
       await page.waitForURL(HISTORY_URL);
       await expect(versionCards).toHaveCount(2);
       // Assert the most recent version is the current version
@@ -105,8 +111,8 @@ describeOnCondition(edition === 'EE')('History', () => {
        */
       await page.getByRole('button', { name: 'Publish' }).click();
       // Go to the history page
-      await page.getByRole('button', { name: /more actions/i }).click();
-      await page.getByRole('menuitem', { name: /content history/i }).click();
+      // Go to the history page
+      await goToHistoryPage(page);
       await page.waitForURL(HISTORY_URL);
       // Publish also creates a new draft so we expect the count to increase by 2
       await expect(versionCards).toHaveCount(4);
@@ -129,8 +135,7 @@ describeOnCondition(edition === 'EE')('History', () => {
       await titleInput.fill('Being from Kansas City, Missouri');
       await page.getByRole('button', { name: 'Save' }).click();
       // Go to the history page
-      await page.getByRole('button', { name: /more actions/i }).click();
-      await page.getByRole('menuitem', { name: /content history/i }).click();
+      await goToHistoryPage(page);
       await page.waitForURL(HISTORY_URL);
       await expect(versionCards).toHaveCount(5);
       // Assert the current version is the modified version
@@ -148,7 +153,6 @@ describeOnCondition(edition === 'EE')('History', () => {
       /**
        * Create an initial entry to also create an initial version
        */
-      await page.goto('/admin');
       await page.getByRole('link', { name: 'Content Manager' }).click();
       await page.getByRole('link', { name: /Create new entry/, exact: true }).click();
       await page.waitForURL(CREATE_URL);
@@ -190,8 +194,7 @@ describeOnCondition(edition === 'EE')('History', () => {
       /**
        * Go to the history page
        */
-      await page.getByRole('button', { name: /more actions/i }).click();
-      await page.getByRole('menuitem', { name: 'Content History' }).click();
+      await goToHistoryPage(page);
       await page.waitForURL(HISTORY_URL);
       const versionCards = await page.getByRole('listitem', { name: 'Version card' });
       await expect(versionCards).toHaveCount(2);
@@ -238,8 +241,7 @@ describeOnCondition(edition === 'EE')('History', () => {
       await titleInput.fill(frenchTitle);
       await page.getByRole('button', { name: 'Save' }).click();
       // Go to the history page
-      await page.getByRole('button', { name: /more actions/i }).click();
-      await page.getByRole('menuitem', { name: /content history/i }).click();
+      await goToHistoryPage(page);
       await page.waitForURL(HISTORY_URL);
       await expect(titleInput).toHaveValue(frenchTitle);
 
@@ -253,8 +255,7 @@ describeOnCondition(edition === 'EE')('History', () => {
       await titleInput.fill(englishTitle);
       await page.getByRole('button', { name: 'Save' }).click();
       // Go to the history page
-      await page.getByRole('button', { name: /more actions/i }).click();
-      await page.getByRole('menuitem', { name: /content history/i }).click();
+      await goToHistoryPage(page);
       await page.waitForURL(HISTORY_URL);
       const versionCards = await page.getByRole('listitem', { name: 'Version card' });
       await expect(versionCards).toHaveCount(1);
@@ -279,14 +280,14 @@ describeOnCondition(edition === 'EE')('History', () => {
        */
       await page.getByRole('textbox', { name: 'title' }).fill('Welcome to AFC Richmond');
       await page.getByRole('button', { name: 'Save' }).click();
-      await page.getByRole('button', { name: /more actions/i }).click();
-      await page.getByRole('menuitem', { name: /content history/i }).click();
+      // Go to the history page
+      await goToHistoryPage(page);
       await page.waitForURL(HISTORY_URL);
       await expect(versionCards).toHaveCount(2);
       // Assert the most recent version is the current version
       await expect(titleInput).toHaveValue('Welcome to AFC Richmond');
       // Assert the previous version in the list is the expected version
-      previousVersion.click();
+      await previousVersion.click();
       await expect(titleInput).toHaveValue('AFC Richmond');
 
       // Go back to the entry
@@ -296,16 +297,16 @@ describeOnCondition(edition === 'EE')('History', () => {
        * Publish
        */
       await page.getByRole('button', { name: 'Publish' }).click();
-      await page.getByRole('button', { name: /more actions/i }).click();
-      await page.getByRole('menuitem', { name: /content history/i }).click();
+      // Go to the history page
+      await goToHistoryPage(page);
       await page.waitForURL('**/content-manager/single-types/api::homepage.homepage/history**');
       // Publish also creates a new draft so we expect the count to increase by 2
       await expect(versionCards).toHaveCount(4);
       // The current version is the most recent draft
       await expect(currentVersion.getByText('Draft')).toBeVisible();
       await expect(titleInput).toHaveValue('Welcome to AFC Richmond');
       // The second in the list is the published version
-      previousVersion.click();
+      await previousVersion.click();
       await expect(previousVersion.getByText('Published')).toBeVisible();
       await expect(titleInput).toHaveValue('Welcome to AFC Richmond');
 
@@ -318,8 +319,7 @@ describeOnCondition(edition === 'EE')('History', () => {
       await titleInput.fill('Welcome to AFC Richmond!');
       await page.getByRole('button', { name: 'Save' }).click();
       // Go to the history page
-      await page.getByRole('button', { name: /more actions/i }).click();
-      await page.getByRole('menuitem', { name: /content history/i }).click();
+      await goToHistoryPage(page);
       await page.waitForURL(HISTORY_URL);
       await expect(versionCards).toHaveCount(5);
       // Assert the current version is the most recent published version
@@ -373,14 +373,13 @@ describeOnCondition(edition === 'EE')('History', () => {
       /**
        * Go to the history page
        */
-      await page.getByRole('button', { name: /more actions/i }).click();
-      await page.getByRole('menuitem', { name: 'Content History' }).click();
+      await goToHistoryPage(page);
       await page.waitForURL(HISTORY_URL);
       const versionCards = await page.getByRole('listitem', { name: 'Version card' });
       await expect(versionCards).toHaveCount(2);
 
       const previousVersion = versionCards.nth(1);
-      previousVersion.click();
+      await previousVersion.click();
 
       // Assert the unknown field is present
       await expect(page.getByText('Unknown fields')).toBeVisible();