Fixed

packages/app/src/server/models/page.ts

@@ -1000,7 +1000,7 @@ export default (crowi: Crowi): any => {
         const shouldCheckDescendants = emptyPage != null;
         const newGrantedUserIds = grant === GRANT_OWNER ? [user._id] as IObjectId[] : undefined;
 
-        isGrantNormalized = await crowi.pageGrantService.isGrantNormalized(path, grant, newGrantedUserIds, grantUserGroupId, shouldCheckDescendants);
+        isGrantNormalized = await crowi.pageGrantService.isGrantNormalized(user, path, grant, newGrantedUserIds, grantUserGroupId, shouldCheckDescendants);
       }
       catch (err) {
         logger.error(`Failed to validate grant of page at "${path}" of grant ${grant}:`, err);
@@ -1118,9 +1118,7 @@ export default (crowi: Crowi): any => {
        */
       let isGrantNormalized = false;
       try {
-        const shouldCheckDescendants = true;
-
-        isGrantNormalized = await crowi.pageGrantService.isGrantNormalized(pageData.path, grant, grantedUserIds, grantUserGroupId, shouldCheckDescendants);
+        isGrantNormalized = await crowi.pageGrantService.isGrantNormalized(user, pageData.path, grant, grantedUserIds, grantUserGroupId, true);
       }
       catch (err) {
         logger.error(`Failed to validate grant of page at "${pageData.path}" of grant ${grant}:`, err);

packages/app/src/server/service/page-grant.ts

@@ -259,21 +259,40 @@ class PageGrantService {
    * @param targetPath string of the target path
    * @returns ComparableDescendants
    */
-  private async generateComparableDescendants(targetPath: string, includeNotMigratedPages: boolean): Promise<ComparableDescendants> {
+  private async generateComparableDescendants(targetPath: string, user, includeNotMigratedPages: boolean): Promise<ComparableDescendants> {
     const Page = mongoose.model('Page') as unknown as PageModel;
+    const UserGroupRelation = mongoose.model('UserGroupRelation') as any; // TODO: Typescriptize model
 
-    /*
-     * make granted users list of descendant's
-     */
-    const pathWithTrailingSlash = addTrailingSlash(targetPath);
-    const startsPattern = escapeStringRegexp(pathWithTrailingSlash);
+    // Build conditions
+    const $match: {$or: any} = {
+      $or: [],
+    };
+
+    const commonCondition = {
+      path: new RegExp(`^${escapeStringRegexp(addTrailingSlash(targetPath))}`, 'i'),
+      isEmpty: false,
+    };
 
-    const $match: any = {
-      path: new RegExp(`^${startsPattern}`),
-      isEmpty: { $ne: true },
+    const conditionForNormalizedPages: any = {
+      ...commonCondition,
+      parent: { $ne: null },
     };
+    $match.$or.push(conditionForNormalizedPages);
+
     if (includeNotMigratedPages) {
-      $match.parent = { $ne: null };
+      // Add grantCondition for not normalized pages
+      const userGroups = await UserGroupRelation.findAllUserGroupIdsRelatedToUser(user);
+      const grantCondition = Page.generateGrantCondition(user, userGroups);
+      const conditionForNotNormalizedPages = {
+        $and: [
+          {
+            ...commonCondition,
+            parent: null,
+          },
+          grantCondition,
+        ],
+      };
+      $match.$or.push(conditionForNotNormalizedPages);
     }
 
     const result = await Page.aggregate([
@@ -327,7 +346,7 @@ class PageGrantService {
    */
   async isGrantNormalized(
       // eslint-disable-next-line max-len
-      targetPath: string, grant, grantedUserIds?: ObjectIdLike[], grantedGroupId?: ObjectIdLike, shouldCheckDescendants = false, includeNotMigratedPages = false,
+      user, targetPath: string, grant, grantedUserIds?: ObjectIdLike[], grantedGroupId?: ObjectIdLike, shouldCheckDescendants = false, includeNotMigratedPages = false,
   ): Promise<boolean> {
     if (isTopPage(targetPath)) {
       return true;
@@ -341,7 +360,7 @@ class PageGrantService {
     }
 
     const comparableTarget = await this.generateComparableTarget(grant, grantedUserIds, grantedGroupId, true);
-    const comparableDescendants = await this.generateComparableDescendants(targetPath, includeNotMigratedPages);
+    const comparableDescendants = await this.generateComparableDescendants(targetPath, user, includeNotMigratedPages);
 
     return this.processValidation(comparableTarget, comparableAncestor, comparableDescendants);
   }
@@ -352,13 +371,11 @@ class PageGrantService {
    * @param pageIds pageIds to be tested
    * @returns a tuple with the first element of normalizable pages and the second element of NOT normalizable pages
    */
-  async separateNormalizableAndNotNormalizablePages(pages): Promise<[(PageDocument & { _id: any })[], (PageDocument & { _id: any })[]]> {
+  async separateNormalizableAndNotNormalizablePages(user, pages): Promise<[(PageDocument & { _id: any })[], (PageDocument & { _id: any })[]]> {
     if (pages.length > LIMIT_FOR_MULTIPLE_PAGE_OP) {
       throw Error(`The maximum number of pageIds allowed is ${LIMIT_FOR_MULTIPLE_PAGE_OP}.`);
     }
 
-    const Page = mongoose.model('Page') as unknown as PageModel;
-
     const shouldCheckDescendants = true;
     const shouldIncludeNotMigratedPages = true;
 
@@ -375,7 +392,7 @@ class PageGrantService {
         continue;
       }
 
-      if (await this.isGrantNormalized(path, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants, shouldIncludeNotMigratedPages)) {
+      if (await this.isGrantNormalized(user, path, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants, shouldIncludeNotMigratedPages)) {
         normalizable.push(page);
       }
       else {

packages/app/src/server/service/page.ts

@@ -486,9 +486,7 @@ class PageService {
     if (grant !== Page.GRANT_RESTRICTED) {
       let isGrantNormalized = false;
       try {
-        const shouldCheckDescendants = false;
-
-        isGrantNormalized = await this.crowi.pageGrantService.isGrantNormalized(newPagePath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
+        isGrantNormalized = await this.crowi.pageGrantService.isGrantNormalized(user, newPagePath, grant, grantedUserIds, grantedGroupId, false);
       }
       catch (err) {
         logger.error(`Failed to validate grant of page at "${newPagePath}" when renaming`, err);
@@ -942,9 +940,7 @@ class PageService {
     if (grant !== Page.GRANT_RESTRICTED) {
       let isGrantNormalized = false;
       try {
-        const shouldCheckDescendants = false;
-
-        isGrantNormalized = await this.crowi.pageGrantService.isGrantNormalized(newPagePath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
+        isGrantNormalized = await this.crowi.pageGrantService.isGrantNormalized(user, newPagePath, grant, grantedUserIds, grantedGroupId, false);
       }
       catch (err) {
         logger.error(`Failed to validate grant of page at "${newPagePath}" when duplicating`, err);
@@ -2294,7 +2290,7 @@ class PageService {
       try {
         const shouldCheckDescendants = true;
 
-        isGrantNormalized = await this.crowi.pageGrantService.isGrantNormalized(path, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
+        isGrantNormalized = await this.crowi.pageGrantService.isGrantNormalized(user, path, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
       }
       catch (err) {
         logger.error(`Failed to validate grant of page at "${path}"`, err);
@@ -2342,7 +2338,7 @@ class PageService {
     let normalizablePages;
     let nonNormalizablePages;
     try {
-      [normalizablePages, nonNormalizablePages] = await this.crowi.pageGrantService.separateNormalizableAndNotNormalizablePages(pagesToNormalize);
+      [normalizablePages, nonNormalizablePages] = await this.crowi.pageGrantService.separateNormalizableAndNotNormalizablePages(user, pagesToNormalize);
     }
     catch (err) {
       throw err;

packages/app/test/integration/service/page-grant.test.js

@@ -220,7 +220,7 @@ describe('PageGrantService', () => {
       const grantedGroupId = null;
       const shouldCheckDescendants = false;
 
-      const result = await pageGrantService.isGrantNormalized(targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
+      const result = await pageGrantService.isGrantNormalized(user1, targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
 
       expect(result).toBe(true);
     });
@@ -232,7 +232,7 @@ describe('PageGrantService', () => {
       const grantedGroupId = groupParent._id;
       const shouldCheckDescendants = false;
 
-      const result = await pageGrantService.isGrantNormalized(targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
+      const result = await pageGrantService.isGrantNormalized(user1, targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
 
       expect(result).toBe(true);
     });
@@ -244,7 +244,7 @@ describe('PageGrantService', () => {
       const grantedGroupId = null;
       const shouldCheckDescendants = false;
 
-      const result = await pageGrantService.isGrantNormalized(targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
+      const result = await pageGrantService.isGrantNormalized(user1, targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
 
       expect(result).toBe(true);
     });
@@ -256,7 +256,7 @@ describe('PageGrantService', () => {
       const grantedGroupId = groupParent._id;
       const shouldCheckDescendants = false;
 
-      const result = await pageGrantService.isGrantNormalized(targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
+      const result = await pageGrantService.isGrantNormalized(user1, targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
 
       expect(result).toBe(true);
     });
@@ -268,7 +268,7 @@ describe('PageGrantService', () => {
       const grantedGroupId = null;
       const shouldCheckDescendants = false;
 
-      const result = await pageGrantService.isGrantNormalized(targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
+      const result = await pageGrantService.isGrantNormalized(user1, targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
 
       expect(result).toBe(true);
     });
@@ -280,7 +280,7 @@ describe('PageGrantService', () => {
       const grantedGroupId = null;
       const shouldCheckDescendants = false;
 
-      const result = await pageGrantService.isGrantNormalized(targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
+      const result = await pageGrantService.isGrantNormalized(user1, targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
 
       expect(result).toBe(true);
     });
@@ -292,7 +292,7 @@ describe('PageGrantService', () => {
       const grantedGroupId = null;
       const shouldCheckDescendants = false;
 
-      const result = await pageGrantService.isGrantNormalized(targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
+      const result = await pageGrantService.isGrantNormalized(user1, targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
 
       expect(result).toBe(false);
     });
@@ -304,7 +304,7 @@ describe('PageGrantService', () => {
       const grantedGroupId = groupParent._id;
       const shouldCheckDescendants = false;
 
-      const result = await pageGrantService.isGrantNormalized(targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
+      const result = await pageGrantService.isGrantNormalized(user1, targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
 
       expect(result).toBe(false);
     });
@@ -318,7 +318,7 @@ describe('PageGrantService', () => {
       const grantedGroupId = null;
       const shouldCheckDescendants = true;
 
-      const result = await pageGrantService.isGrantNormalized(targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
+      const result = await pageGrantService.isGrantNormalized(user1, targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
 
       expect(result).toBe(true);
     });
@@ -330,7 +330,7 @@ describe('PageGrantService', () => {
       const grantedGroupId = null;
       const shouldCheckDescendants = true;
 
-      const result = await pageGrantService.isGrantNormalized(targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
+      const result = await pageGrantService.isGrantNormalized(user1, targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
 
       expect(result).toBe(true);
     });
@@ -342,7 +342,7 @@ describe('PageGrantService', () => {
       const grantedGroupId = groupParent._id;
       const shouldCheckDescendants = true;
 
-      const result = await pageGrantService.isGrantNormalized(targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
+      const result = await pageGrantService.isGrantNormalized(user1, targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
 
       expect(result).toBe(true);
     });
@@ -354,7 +354,7 @@ describe('PageGrantService', () => {
       const grantedGroupId = null;
       const shouldCheckDescendants = true;
 
-      const result = await pageGrantService.isGrantNormalized(targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
+      const result = await pageGrantService.isGrantNormalized(user1, targetPath, grant, grantedUserIds, grantedGroupId, shouldCheckDescendants);
 
       expect(result).toBe(false);
     });

packages/app/test/integration/service/v5.migration.test.js

@@ -500,11 +500,6 @@ describe('V5 page migration', () => {
           grant: Page.GRANT_USER_GROUP,
           grantedGroup: rootUserGroupId,
         },
-        {
-          path: '/normalize_21_owned/normalize_22_owned/normalize_23_owned/normalize_24_testGroup',
-          grant: Page.GRANT_USER_GROUP,
-          grantedGroup: testUser1GroupId,
-        },
       ]);
     });
 
@@ -536,7 +531,7 @@ describe('V5 page migration', () => {
       const owned15 = await Page.findOne({ path: '/normalize_13_owned/normalize_14_owned/normalize_15_owned' });
       const owned16 = await Page.findOne({ path: '/normalize_13_owned/normalize_14_owned/normalize_15_owned/normalize_16_owned' });
       const root16 = await Page.findOne(root({ path: '/normalize_13_owned/normalize_14_owned/normalize_15_owned/normalize_16_root' }));
-      const group16 = await Page.findOne(rootUserGroup({ path: '/normalize_13_owned/normalize_14_owned/normalize_15_owned/normalize_16_group' }));
+      const group16 = await Page.findOne(testUser1Group({ path: '/normalize_13_owned/normalize_14_owned/normalize_15_owned/normalize_16_group' }));
 
       expect(owned13).not.toBeNull();
       expect(owned14).not.toBeNull();
@@ -620,7 +615,6 @@ describe('V5 page migration', () => {
       const _owned24 = await Page.findOne(owned({ path: '/normalize_21_owned/normalize_22_owned/normalize_23_owned/normalize_24_owned', ...normalized }));
       const _root24 = await Page.findOne(root({ path: '/normalize_21_owned/normalize_22_owned/normalize_23_owned/normalize_24_root', ...notNormalized }));
       const _rootGroup24 = await Page.findOne(rootUserGroup({ path: '/normalize_21_owned/normalize_22_owned/normalize_23_owned/normalize_24_rootGroup', ...notNormalized }));
-      const _testGroup24 = await Page.findOne(testUser1Group({ path: '/normalize_21_owned/normalize_22_owned/normalize_23_owned/normalize_24_testGroup', ...notNormalized }));
 
       expect(_owned21).not.toBeNull();
       expect(_owned22).not.toBeNull();
@@ -629,7 +623,6 @@ describe('V5 page migration', () => {
       expect(_owned24).not.toBeNull();
       expect(_root24).not.toBeNull();
       expect(_rootGroup24).not.toBeNull();
-      expect(_testGroup24).not.toBeNull();
 
       // Normalize
       await normalizeParentRecursivelyByPages([_owned23], testUser1);
@@ -641,7 +634,6 @@ describe('V5 page migration', () => {
       const owned24 = await Page.findOne({ path: '/normalize_21_owned/normalize_22_owned/normalize_23_owned/normalize_24_owned' });
       const root24 = await Page.findOne(root({ path: '/normalize_21_owned/normalize_22_owned/normalize_23_owned/normalize_24_root' }));
       const rootGroup24 = await Page.findOne(rootUserGroup({ path: '/normalize_21_owned/normalize_22_owned/normalize_23_owned/normalize_24_rootGroup' }));
-      const testGroup24 = await Page.findOne(testUser1Group({ path: '/normalize_21_owned/normalize_22_owned/normalize_23_owned/normalize_24_testGroup' }));
 
       expect(owned21).not.toBeNull();
       expect(owned22).not.toBeNull();
@@ -650,7 +642,6 @@ describe('V5 page migration', () => {
       expect(owned24).not.toBeNull();
       expect(root24).not.toBeNull();
       expect(rootGroup24).not.toBeNull();
-      expect(testGroup24).not.toBeNull();
 
       // Check parent
       expect(owned21.parent).toStrictEqual(rootPage._id);
@@ -659,7 +650,6 @@ describe('V5 page migration', () => {
       expect(owned24.parent).toStrictEqual(owned23._id); // not empty23._id
       expect(root24.parent).toBeNull();
       expect(rootGroup24.parent).toBeNull(); // excluded from the pages to be normalized
-      expect(testGroup24.parent).toStrictEqual(owned23._id); // included in the pages to be normalized
 
       // Check isEmpty
       expect(owned21.isEmpty).toBe(false);