Merge pull request #8464 from weseek/feat/136138-140573-normal-page-delete

feat: Normal page delete fix after multi group grant page delete impl

apps/app/src/server/routes/apiv3/page-listing.ts

@@ -3,19 +3,20 @@ import type {
 } from '@growi/core';
 import { isIPageInfoForEntity } from '@growi/core';
 import { ErrorV3 } from '@growi/core/dist/models';
-import express, { Request, Router } from 'express';
+import type { Request, Router } from 'express';
+import express from 'express';
 import { query, oneOf } from 'express-validator';
 import mongoose from 'mongoose';
 
 
-import { IPageGrantService } from '~/server/service/page-grant';
+import type { IPageGrantService } from '~/server/service/page-grant';
 import loggerFactory from '~/utils/logger';
 
-import Crowi from '../../crowi';
+import type Crowi from '../../crowi';
 import { apiV3FormValidator } from '../../middlewares/apiv3-form-validator';
-import { PageModel } from '../../models/page';
+import type { PageModel } from '../../models/page';
 
-import { ApiV3Response } from './interfaces/apiv3-response';
+import type { ApiV3Response } from './interfaces/apiv3-response';
 
 const logger = loggerFactory('growi:routes:apiv3:page-tree');
 
@@ -149,7 +150,8 @@ const routerFactory = (crowi: Crowi): Router => {
         // construct isIPageInfoForListing
         const basicPageInfo = pageService.constructBasicPageInfo(page, isGuestUser);
 
-        const canDeleteCompletely = pageService.canDeleteCompletely(page, req.user, false, userRelatedGroups); // use normal delete config
+        // TODO: use pageService.getCreatorIdForCanDelete to get creatorId (https://redmine.weseek.co.jp/issues/140574)
+        const canDeleteCompletely = pageService.canDeleteCompletely(page, page.creator, req.user, false, userRelatedGroups); // use normal delete config
 
         const pageInfo = (!isIPageInfoForEntity(basicPageInfo))
           ? basicPageInfo

apps/app/src/server/routes/page.js

@@ -366,25 +366,14 @@ module.exports = function(crowi, app) {
       return res.json(ApiResponse.error('Empty pages cannot be single deleted', 'single_deletion_empty_pages'));
     }
 
-    // -- canDelete no longer needs creator,
-    //  however it might be required to retrieve the closest non-empty ancestor page's owner -- 2024.02.09 Yuki Takei
-    //
-    // let creator;
-    // if (page.isEmpty) {
-    //   // If empty, the creator is inherited from the closest non-empty ancestor page.
-    //   const notEmptyClosestAncestor = await Page.findNonEmptyClosestAncestor(page.path);
-    //   creator = notEmptyClosestAncestor.creator;
-    // }
-    // else {
-    //   creator = page.creator;
-    // }
+    const creatorId = await crowi.pageService.getCreatorIdForCanDelete(page);
 
     debug('Delete page', page._id, page.path);
 
     try {
       if (isCompletely) {
         const userRelatedGroups = await crowi.pageGrantService.getUserRelatedGroups(req.user);
-        const canDeleteCompletely = crowi.pageService.canDeleteCompletely(page, req.user, isRecursively, userRelatedGroups);
+        const canDeleteCompletely = crowi.pageService.canDeleteCompletely(page, creatorId, req.user, isRecursively, userRelatedGroups);
         if (!canDeleteCompletely) {
           return res.json(ApiResponse.error('You cannot delete this page completely', 'complete_deletion_not_allowed_for_user'));
         }
@@ -411,8 +400,8 @@ module.exports = function(crowi, app) {
           return res.json(ApiResponse.error('Someone could update this page, so couldn\'t delete.', 'outdated'));
         }
 
-        if (!crowi.pageService.canDelete(page, req.user, isRecursively)) {
-          return res.json(ApiResponse.error('You can not delete this page', 'user_not_admin'));
+        if (!crowi.pageService.canDelete(page, creatorId, req.user, isRecursively)) {
+          return res.json(ApiResponse.error('You cannot delete this page', 'user_not_admin'));
         }
 
         if (pagePathUtils.isUsersHomepage(page.path)) {

apps/app/src/server/service/page/index.ts

@@ -200,13 +200,15 @@ class PageService implements IPageService {
 
   /**
    * Check if page can be deleted completely.
-   * Use pageGrantService.getUserRelatedGroups before execution of canDeleteCompletely to get value for userRelatedGroups.
-   * Do NOT use getUserRelatedGrantedGroups inside this method, because canDeleteCompletely should not be async as for now.
-   * The reason for this is because canDeleteCompletely is called in /page-listing/info in a for loop,
+   * Use the following methods before execution of canDeleteCompletely to get params.
+   *   - pageService.getCreatorIdForCanDelete: creatorId
+   *   - pageGrantService.getUserRelatedGroups: userRelatedGroups
+   * Do NOT make this method async as for now, because canDeleteCompletely is called in /page-listing/info in a for loop,
    * and /page-listing/info should not be an execution heavy API.
    */
   canDeleteCompletely(
       page: PageDocument,
+      creatorId: ObjectIdLike | null,
       operator: any | null,
       isRecursively: boolean,
       userRelatedGroups: PopulatedGrantedGroup[],
@@ -216,25 +218,28 @@ class PageService implements IPageService {
     const pageCompleteDeletionAuthority = this.crowi.configManager.getConfig('crowi', 'security:pageCompleteDeletionAuthority');
     const pageRecursiveCompleteDeletionAuthority = this.crowi.configManager.getConfig('crowi', 'security:pageRecursiveCompleteDeletionAuthority');
 
-    if (!this.canDeleteCompletelyAsMultiGroupGrantedPage(page, operator, userRelatedGroups)) return false;
+    if (!this.canDeleteCompletelyAsMultiGroupGrantedPage(page, creatorId, operator, userRelatedGroups)) return false;
 
     const [singleAuthority, recursiveAuthority] = prepareDeleteConfigValuesForCalc(pageCompleteDeletionAuthority, pageRecursiveCompleteDeletionAuthority);
 
-    return this.canDeleteLogic(page.creator, operator, isRecursively, singleAuthority, recursiveAuthority);
+    return this.canDeleteLogic(creatorId, operator, isRecursively, singleAuthority, recursiveAuthority);
   }
 
   /**
    * If page is multi-group granted, check if operator is allowed to completely delete the page.
    * see: https://dev.growi.org/656745fa52eafe1cf1879508#%E5%AE%8C%E5%85%A8%E3%81%AB%E5%89%8A%E9%99%A4%E3%81%99%E3%82%8B%E6%93%8D%E4%BD%9C
+   * creatorId must be obtained by getCreatorIdForCanDelete
    */
-  canDeleteCompletelyAsMultiGroupGrantedPage(page: PageDocument, operator: any | null, userRelatedGroups: PopulatedGrantedGroup[]): boolean {
+  canDeleteCompletelyAsMultiGroupGrantedPage(
+      page: PageDocument, creatorId: ObjectIdLike | null, operator: any | null, userRelatedGroups: PopulatedGrantedGroup[],
+  ): boolean {
     const pageCompleteDeletionAuthority = this.crowi.configManager.getConfig('crowi', 'security:pageCompleteDeletionAuthority');
     const isAllGroupMembershipRequiredForPageCompleteDeletion = this.crowi.configManager.getConfig(
       'crowi', 'security:isAllGroupMembershipRequiredForPageCompleteDeletion',
     );
 
     const isAdmin = operator?.admin ?? false;
-    const isAuthor = operator?._id == null ? false : operator._id.equals(page.creator);
+    const isAuthor = operator?._id == null ? false : operator._id.equals(creatorId);
     const isAdminOrAuthor = isAdmin || isAuthor;
 
     if (page.grant === PageGrant.GRANT_USER_GROUP
@@ -249,7 +254,19 @@ class PageService implements IPageService {
     return true;
   }
 
-  canDelete(page: PageDocument, operator: any | null, isRecursively: boolean): boolean {
+  // When page is empty, the 'canDelete' judgement should be done using the creator of the closest non-empty ancestor page.
+  async getCreatorIdForCanDelete(page: PageDocument): Promise<ObjectIdLike | null> {
+    if (page.isEmpty) {
+      const Page = mongoose.model<IPage, PageModel>('Page');
+      const notEmptyClosestAncestor = await Page.findNonEmptyClosestAncestor(page.path);
+      return notEmptyClosestAncestor?.creator ?? null;
+    }
+
+    return page.creator ?? null;
+  }
+
+  // Use getCreatorIdForCanDelete before execution of canDelete to get creatorId.
+  canDelete(page: PageDocument, creatorId: ObjectIdLike | null, operator: any | null, isRecursively: boolean): boolean {
     if (operator == null || isTopPage(page.path) || isUsersTopPage(page.path)) return false;
 
     const pageDeletionAuthority = this.crowi.configManager.getConfig('crowi', 'security:pageDeletionAuthority');
@@ -257,7 +274,7 @@ class PageService implements IPageService {
 
     const [singleAuthority, recursiveAuthority] = prepareDeleteConfigValuesForCalc(pageDeletionAuthority, pageRecursiveDeletionAuthority);
 
-    return this.canDeleteLogic(page.creator, operator, isRecursively, singleAuthority, recursiveAuthority);
+    return this.canDeleteLogic(creatorId, operator, isRecursively, singleAuthority, recursiveAuthority);
   }
 
   canDeleteUserHomepageByConfig(): boolean {
@@ -275,7 +292,7 @@ class PageService implements IPageService {
   }
 
   private canDeleteLogic(
-      creatorId: ObjectIdLike,
+      creatorId: ObjectIdLike | null,
       operator,
       isRecursively: boolean,
       authority: IPageDeleteConfigValueToProcessValidation | null,
@@ -329,12 +346,14 @@ class PageService implements IPageService {
       pages: PageDocument[],
       user: IUserHasId,
       isRecursively: boolean,
-      canDeleteFunction: (page: PageDocument, operator: any, isRecursively: boolean, userRelatedGroups: PopulatedGrantedGroup[]) => boolean,
+      canDeleteFunction: (
+        page: PageDocument, creatorId: ObjectIdLike, operator: any, isRecursively: boolean, userRelatedGroups: PopulatedGrantedGroup[]
+      ) => boolean,
   ): Promise<PageDocument[]> {
     const userRelatedGroups = await this.pageGrantService.getUserRelatedGroups(user);
     const filteredPages = pages.filter(async(p) => {
       if (p.isEmpty) return true;
-      const canDelete = canDeleteFunction(p, user, isRecursively, userRelatedGroups);
+      const canDelete = canDeleteFunction(p, p.creator, user, isRecursively, userRelatedGroups);
       return canDelete;
     });
 
@@ -421,10 +440,12 @@ class PageService implements IPageService {
 
     const subscription = await Subscription.findByUserIdAndTargetId(user._id, pageId);
 
+    const creatorId = await this.getCreatorIdForCanDelete(page);
+
     const userRelatedGroups = await this.pageGrantService.getUserRelatedGroups(user);
 
-    const isDeletable = this.canDelete(page, user, false);
-    const isAbleToDeleteCompletely = this.canDeleteCompletely(page, user, false, userRelatedGroups); // use normal delete config
+    const isDeletable = this.canDelete(page, creatorId, user, false);
+    const isAbleToDeleteCompletely = this.canDeleteCompletely(page, creatorId, user, false, userRelatedGroups); // use normal delete config
 
     return {
       data: page,

apps/app/src/server/service/page/page-service.ts

@@ -23,7 +23,11 @@ export interface IPageService {
   findChildrenByParentPathOrIdAndViewer(parentPathOrId: string, user, userGroups?): Promise<PageDocument[]>,
   shortBodiesMapByPageIds(pageIds?: ObjectId[], user?): Promise<Record<string, string | null>>,
   constructBasicPageInfo(page: PageDocument, isGuestUser?: boolean): IPageInfo | IPageInfoForEntity,
-  canDelete(page: PageDocument, operator: any | null, isRecursively: boolean): boolean,
-  canDeleteCompletely(page: PageDocument, operator: any | null, isRecursively: boolean, userRelatedGroups: PopulatedGrantedGroup[]): boolean,
-  canDeleteCompletelyAsMultiGroupGrantedPage(page: PageDocument, operator: any | null, userRelatedGroups: PopulatedGrantedGroup[]): boolean,
+  canDelete(page: PageDocument, creatorId: ObjectIdLike | null, operator: any | null, isRecursively: boolean): boolean,
+  canDeleteCompletely(
+    page: PageDocument, creatorId: ObjectIdLike | null, operator: any | null, isRecursively: boolean, userRelatedGroups: PopulatedGrantedGroup[]
+  ): boolean,
+  canDeleteCompletelyAsMultiGroupGrantedPage(
+    page: PageDocument, creatorId: ObjectIdLike | null, operator: any | null, userRelatedGroups: PopulatedGrantedGroup[]
+  ): boolean,
 }

apps/app/test/integration/service/page.test.js

@@ -769,8 +769,9 @@ describe('PageService', () => {
         });
 
         test('is not deletable', async() => {
+          const creatorId = await crowi.pageService.getCreatorIdForCanDelete(canDeleteCompletelyTestPage);
           const userRelatedGroups = await crowi.pageGrantService.getUserRelatedGroups(testUser1);
-          const isDeleteable = crowi.pageService.canDeleteCompletely(canDeleteCompletelyTestPage, testUser1, false, userRelatedGroups);
+          const isDeleteable = crowi.pageService.canDeleteCompletely(canDeleteCompletelyTestPage, creatorId, testUser1, false, userRelatedGroups);
           expect(isDeleteable).toBe(false);
         });
       });
@@ -789,8 +790,9 @@ describe('PageService', () => {
         });
 
         test('is not deletable', async() => {
+          const creatorId = await crowi.pageService.getCreatorIdForCanDelete(canDeleteCompletelyTestPage);
           const userRelatedGroups = await crowi.pageGrantService.getUserRelatedGroups(testUser3);
-          const isDeleteable = crowi.pageService.canDeleteCompletely(canDeleteCompletelyTestPage, testUser3, false, userRelatedGroups);
+          const isDeleteable = crowi.pageService.canDeleteCompletely(canDeleteCompletelyTestPage, creatorId, testUser3, false, userRelatedGroups);
           expect(isDeleteable).toBe(true);
         });
       });
@@ -809,8 +811,9 @@ describe('PageService', () => {
         });
 
         test('is deletable', async() => {
+          const creatorId = await crowi.pageService.getCreatorIdForCanDelete(canDeleteCompletelyTestPage);
           const userRelatedGroups = await crowi.pageGrantService.getUserRelatedGroups(testUser1);
-          const isDeleteable = crowi.pageService.canDeleteCompletely(canDeleteCompletelyTestPage, testUser1, false, userRelatedGroups);
+          const isDeleteable = crowi.pageService.canDeleteCompletely(canDeleteCompletelyTestPage, creatorId, testUser1, false, userRelatedGroups);
           expect(isDeleteable).toBe(true);
         });
       });
@@ -829,8 +832,9 @@ describe('PageService', () => {
         });
 
         test('is deletable', async() => {
+          const creatorId = await crowi.pageService.getCreatorIdForCanDelete(canDeleteCompletelyTestPage);
           const userRelatedGroups = await crowi.pageGrantService.getUserRelatedGroups(testUser2);
-          const isDeleteable = crowi.pageService.canDeleteCompletely(canDeleteCompletelyTestPage, testUser2, false, userRelatedGroups);
+          const isDeleteable = crowi.pageService.canDeleteCompletely(canDeleteCompletelyTestPage, creatorId, testUser2, false, userRelatedGroups);
           expect(isDeleteable).toBe(true);
         });
       });