From 8ddb1ecebcc6e417146beaa5517ebc53c5dbcb62 Mon Sep 17 00:00:00 2001
From: Marc Roig <marc.roig.campos@strapi.io>
Date: Mon, 22 Jul 2024 10:03:14 +0200
Subject: [PATCH] fix: inconsistent publish permissions (#20668)

* fix: inconsistent publish permissions

* fix: tests

* fix: api tests

* fix: is create should be is nil

* fix: publish and update by using locale

* fix: remove some conditions to the Document actions

---------

Co-authored-by: Simone Taeggi <startae14@gmail.com>
---
 .../EditView/components/DocumentActions.tsx   | 13 +----
 .../src/controllers/collection-types.ts       | 47 ++++++++++++++++---
 ...-permissions-custom-conditions.test.api.js |  1 +
 3 files changed, 43 insertions(+), 18 deletions(-)

diff --git a/packages/core/content-manager/admin/src/pages/EditView/components/DocumentActions.tsx b/packages/core/content-manager/admin/src/pages/EditView/components/DocumentActions.tsx
index f7f60d7c15..af6207246f 100644
--- a/packages/core/content-manager/admin/src/pages/EditView/components/DocumentActions.tsx
+++ b/packages/core/content-manager/admin/src/pages/EditView/components/DocumentActions.tsx
@@ -666,8 +666,6 @@ const PublishAction: DocumentActionComponent = ({
      *  - the document is already published & not modified
      *  - the document is being created & not modified
      *  - the user doesn't have the permission to publish
-     *  - the user doesn't have the permission to create a new document
-     *  - the user doesn't have the permission to update the document
      */
     disabled:
       isCloning ||
@@ -676,8 +674,7 @@ const PublishAction: DocumentActionComponent = ({
       activeTab === 'published' ||
       (!modified && isDocumentPublished) ||
       (!modified && !document?.documentId) ||
-      !canPublish ||
-      Boolean((!document?.documentId && !canCreate) || (document?.documentId && !canUpdate)),
+      !canPublish,
     label: formatMessage({
       id: 'app.utils.publish',
       defaultMessage: 'Publish',
@@ -754,14 +751,8 @@ const UpdateAction: DocumentActionComponent = ({
      * - the form is submitting
      * - the document is not modified & we're not cloning (you can save a clone entity straight away)
      * - the active tab is the published tab
-     * - the user doesn't have the permission to create a new document
-     * - the user doesn't have the permission to update the document
      */
-    disabled:
-      isSubmitting ||
-      (!modified && !isCloning) ||
-      activeTab === 'published' ||
-      Boolean((!documentId && !canCreate) || (documentId && !canUpdate)),
+    disabled: isSubmitting || (!modified && !isCloning) || activeTab === 'published',
     label: formatMessage({
       id: 'content-manager.containers.Edit.save',
       defaultMessage: 'Save',
diff --git a/packages/core/content-manager/server/src/controllers/collection-types.ts b/packages/core/content-manager/server/src/controllers/collection-types.ts
index 3af77b97d0..92506e7876 100644
--- a/packages/core/content-manager/server/src/controllers/collection-types.ts
+++ b/packages/core/content-manager/server/src/controllers/collection-types.ts
@@ -1,5 +1,6 @@
-import { setCreatorFields, async, errors } from '@strapi/utils';
+import { isNil } from 'lodash/fp';
 
+import { setCreatorFields, async, errors } from '@strapi/utils';
 import type { Modules, UID } from '@strapi/types';
 
 import { getService } from '../utils';
@@ -390,17 +391,49 @@ export default {
         .countRelations()
         .build();
 
-      const document = id
-        ? await updateDocument(ctx, { populate })
-        : await createDocument(ctx, { populate });
+      let document: any;
+
+      const { locale } = await getDocumentLocaleAndStatus(body, model);
+
+      /**
+       * Publish can be called on two scenarios:
+       * 1. Create a new document and publish it in one request
+       * 2. Update an existing document and publish it in one request
+       *
+       * Based on user permissions:
+       * 1. User cannot create a document, but can publish
+       *    Action will be forbidden as user cannot create a document
+       * 2. User can update and publish a document
+       *    Action will be allowed, but document will not be updated, only published with the latest draft
+       */
+      const isCreate = isNil(id);
+      if (isCreate) {
+        if (permissionChecker.cannot.create()) {
+          throw new errors.ForbiddenError();
+        }
+
+        document = await createDocument(ctx, { populate });
+      }
+
+      const isUpdate = !isCreate;
+      if (isUpdate) {
+        document = await documentManager.findOne(id!, model, { populate, locale });
+
+        if (!document) {
+          throw new errors.NotFoundError('Document not found');
+        }
+
+        // Only Update if user has update permissions
+        if (permissionChecker.can.update(document)) {
+          await updateDocument(ctx);
+        }
+      }
 
       if (permissionChecker.cannot.publish(document)) {
         throw new errors.ForbiddenError();
       }
 
-      const { locale } = await getDocumentLocaleAndStatus(body, model);
-
-      const publishResult = await documentManager.publish(document!.documentId, model, {
+      const publishResult = await documentManager.publish(document.documentId, model, {
         locale,
         // TODO: Allow setting creator fields on publish
         // data: setCreatorFields({ user, isEdition: true })({}),
diff --git a/tests/api/core/admin/admin-permissions-custom-conditions.test.api.js b/tests/api/core/admin/admin-permissions-custom-conditions.test.api.js
index d5553e2ab4..448761d536 100644
--- a/tests/api/core/admin/admin-permissions-custom-conditions.test.api.js
+++ b/tests/api/core/admin/admin-permissions-custom-conditions.test.api.js
@@ -267,6 +267,7 @@ describe('Admin Permissions - Conditions', () => {
     const res = await rq({
       method: 'POST',
       url: `/content-manager/collection-types/api::article.article/${documentId}/actions/publish`,
+      body: { ...localTestData.cheapArticle, category: localTestData.categories[0].documentId },
     });
 
     expect(res.statusCode).toBe(200);