Merge pull request #5545 from weseek/fix/normalize-parent-exclude-unchecked-pages

fix: Normalize parent as excluding unchecked pages

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

@@ -44,7 +44,7 @@ type TargetAndAncestorsResult = {
 export type CreateMethod = (path: string, body: string, user, options) => Promise<PageDocument & { _id: any }>
 export interface PageModel extends Model<PageDocument> {
   [x: string]: any; // for obsolete methods
-  createEmptyPagesByPaths(paths: string[], onlyMigratedAsExistingPages?: boolean, publicOnly?: boolean): Promise<void>
+  createEmptyPagesByPaths(paths: string[], user: any | null, onlyMigratedAsExistingPages?: boolean, andFilter?): Promise<void>
   getParentAndFillAncestors(path: string, user): Promise<PageDocument & { _id: any }>
   findByIdsAndViewer(pageIds: ObjectIdLike[], user, userGroups?, includeEmpty?: boolean): Promise<PageDocument[]>
   findByPathAndViewer(path: string | null, user, userGroups?, useFindOne?: boolean, includeEmpty?: boolean): Promise<PageDocument[]>
@@ -140,6 +140,27 @@ class PageQueryBuilder {
     }
   }
 
+  /**
+   * Used for filtering the pages at specified paths not to include unintentional pages.
+   * @param pathsToFilter The paths to have additional filters as to be applicable
+   * @returns PageQueryBuilder
+   */
+  addConditionToFilterByApplicableAncestors(pathsToFilter: string[]) {
+    this.query = this.query
+      .and(
+        {
+          $or: [
+            { path: '/' },
+            { path: { $in: pathsToFilter }, grant: GRANT_PUBLIC, status: STATUS_PUBLISHED },
+            { path: { $in: pathsToFilter }, parent: { $ne: null }, status: STATUS_PUBLISHED },
+            { path: { $nin: pathsToFilter }, status: STATUS_PUBLISHED },
+          ],
+        },
+      );
+
+    return this;
+  }
+
   addConditionToExcludeTrashed() {
     this.query = this.query
       .and({
@@ -409,23 +430,39 @@ class PageQueryBuilder {
  * @param onlyMigratedAsExistingPages Determine whether to include non-migrated pages as existing pages. If a page exists,
  * an empty page will not be created at that page's path.
  */
-schema.statics.createEmptyPagesByPaths = async function(paths: string[], user: any | null, onlyMigratedAsExistingPages = true): Promise<void> {
-  // find existing parents
-  const builder = new PageQueryBuilder(this.find({}, { _id: 0, path: 1 }), true);
-
-  await this.addConditionToFilteringByViewerToEdit(builder, user);
-
+schema.statics.createEmptyPagesByPaths = async function(paths: string[], user: any | null, onlyMigratedAsExistingPages = true, filter?): Promise<void> {
+  const aggregationPipeline: any[] = [];
+  // 1. Filter by paths
+  aggregationPipeline.push({ $match: { path: { $in: paths } } });
+  // 2. Normalized condition
   if (onlyMigratedAsExistingPages) {
-    builder.addConditionAsMigrated();
+    aggregationPipeline.push({
+      $match: {
+        $or: [
+          { parent: { $ne: null } },
+          { path: '/' },
+        ],
+      },
+    });
+  }
+  // 3. Add custom pipeline
+  if (filter != null) {
+    aggregationPipeline.push({ $match: filter });
+  }
+  // 4. Add grant conditions
+  let userGroups = null;
+  if (user != null) {
+    const UserGroupRelation = mongoose.model('UserGroupRelation') as any;
+    userGroups = await UserGroupRelation.findAllUserGroupIdsRelatedToUser(user);
   }
+  const grantCondition = this.generateGrantCondition(user, userGroups);
+  aggregationPipeline.push({ $match: grantCondition });
 
-  const existingPages = await builder
-    .addConditionToListByPathsArray(paths)
-    .query
-    .lean()
-    .exec();
-  const existingPagePaths = existingPages.map(page => page.path);
+  // Run aggregation
+  const existingPages = await this.aggregate(aggregationPipeline);
 
+
+  const existingPagePaths = existingPages.map(page => page.path);
   // paths to create empty pages
   const notExistingPagePaths = paths.filter(path => !existingPagePaths.includes(path));
 
@@ -973,7 +1010,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);
@@ -1091,9 +1128,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);
@@ -2281,7 +2277,7 @@ class PageService {
     } = page;
 
     // check if any page exists at target path already
-    const existingPage = await Page.findOne({ path });
+    const existingPage = await Page.findOne({ path, parent: { $ne: null } });
     if (existingPage != null && !existingPage.isEmpty) {
       throw Error('Page already exists. Please rename the page to continue.');
     }
@@ -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;
@@ -2543,8 +2539,12 @@ class PageService {
   async normalizeParentRecursively(paths: string[], user: any | null): Promise<void> {
     const Page = mongoose.model('Page') as unknown as PageModel;
 
-    const ancestorPaths = paths.flatMap(p => collectAncestorPaths(p, [p]));
-    const regexps = paths.map(p => new RegExp(`^${escapeStringRegexp(addTrailingSlash(p))}`, 'i'));
+    const ancestorPaths = paths.flatMap(p => collectAncestorPaths(p, []));
+    // targets' descendants
+    const pathAndRegExpsToNormalize: (RegExp | string)[] = paths
+      .map(p => new RegExp(`^${escapeStringRegexp(addTrailingSlash(p))}`, 'i'));
+    // include targets' path
+    pathAndRegExpsToNormalize.push(...paths);
 
     // determine UserGroup condition
     let userGroups = null;
@@ -2555,11 +2555,13 @@ class PageService {
 
     const grantFiltersByUser: { $or: any[] } = Page.generateGrantCondition(user, userGroups);
 
-    return this._normalizeParentRecursively(regexps, ancestorPaths, grantFiltersByUser, user);
+    return this._normalizeParentRecursively(pathAndRegExpsToNormalize, ancestorPaths, grantFiltersByUser, user);
   }
 
   // TODO: use websocket to show progress
-  private async _normalizeParentRecursively(regexps: RegExp[], pathsToInclude: string[], grantFiltersByUser: { $or: any[] }, user): Promise<void> {
+  private async _normalizeParentRecursively(
+      pathOrRegExps: (RegExp | string)[], publicPathsToNormalize: string[], grantFiltersByUser: { $or: any[] }, user,
+  ): Promise<void> {
     const BATCH_SIZE = 100;
     const PAGES_LIMIT = 1000;
 
@@ -2567,7 +2569,7 @@ class PageService {
     const { PageQueryBuilder } = Page;
 
     // Build filter
-    const filter: any = {
+    const andFilter: any = {
       $and: [
         {
           parent: null,
@@ -2576,25 +2578,36 @@ class PageService {
         },
       ],
     };
-    let pathCondition: (RegExp | string)[] = [];
-    if (regexps.length > 0) {
-      pathCondition = [...regexps];
-    }
-    if (pathsToInclude.length > 0) {
-      pathCondition = [...pathCondition, ...pathsToInclude];
-    }
-    if (pathCondition.length > 0) {
-      filter.$and.push({
-        parent: null,
-        status: Page.STATUS_PUBLISHED,
-        path: { $in: pathCondition },
-      });
+    const orFilter: any = { $or: [] };
+    // specified pathOrRegExps
+    if (pathOrRegExps.length > 0) {
+      orFilter.$or.push(
+        {
+          path: { $in: pathOrRegExps },
+        },
+      );
     }
+    // not specified but ancestors of specified pathOrRegExps
+    if (publicPathsToNormalize.length > 0) {
+      orFilter.$or.push(
+        {
+          path: { $in: publicPathsToNormalize },
+          grant: Page.GRANT_PUBLIC, // use only public pages to complete the tree
+        },
+      );
+    }
+
+    // Merge filters
+    const mergedFilter = {
+      $and: [
+        { $and: [grantFiltersByUser, ...andFilter.$and] },
+        { $or: orFilter.$or },
+      ],
+    };
 
     let baseAggregation = Page
       .aggregate([
-        { $match: grantFiltersByUser },
-        { $match: filter },
+        { $match: mergedFilter },
         {
           $project: { // minimize data to fetch
             _id: 1,
@@ -2604,7 +2617,7 @@ class PageService {
       ]);
 
     // Limit pages to get
-    const total = await Page.countDocuments(filter);
+    const total = await Page.countDocuments(mergedFilter);
     if (total > PAGES_LIMIT) {
       baseAggregation = baseAggregation.limit(Math.floor(total * 0.3));
     }
@@ -2643,16 +2656,29 @@ class PageService {
         });
 
         await Page.bulkWrite(resetParentOperations);
-
         await Page.removeEmptyPages(pageIdsToNotDelete, emptyPagePathsToDelete);
 
         // 2. Create lacking parents as empty pages
-        await Page.createEmptyPagesByPaths(parentPaths, user, false);
+        const orFilters = [
+          { path: '/' },
+          { path: { $in: publicPathsToNormalize }, grant: Page.GRANT_PUBLIC, status: Page.STATUS_PUBLISHED },
+          { path: { $in: publicPathsToNormalize }, parent: { $ne: null }, status: Page.STATUS_PUBLISHED },
+          { path: { $nin: publicPathsToNormalize }, status: Page.STATUS_PUBLISHED },
+        ];
+        const filterForApplicableAncestors = { $or: orFilters };
+        await Page.createEmptyPagesByPaths(parentPaths, user, false, filterForApplicableAncestors);
 
         // 3. Find parents
-        const builder2 = new PageQueryBuilder(Page.find({}, { _id: 1, path: 1 }), true);
+        const addGrantCondition = (builder) => {
+          builder.query = builder.query.and(grantFiltersByUser);
+
+          return builder;
+        };
+        const builder2 = new PageQueryBuilder(Page.find(), true);
+        addGrantCondition(builder2);
         const parents = await builder2
           .addConditionToListByPathsArray(parentPaths)
+          .addConditionToFilterByApplicableAncestors(publicPathsToNormalize)
           .query
           .lean()
           .exec();
@@ -2668,6 +2694,7 @@ class PageService {
               {
                 path: { $regex: new RegExp(`^${parentPathEscaped}(\\/[^/]+)\\/?$`, 'i') }, // see: regexr.com/6889f (e.g. /parent/any_child or /any_level1)
               },
+              filterForApplicableAncestors,
               grantFiltersByUser,
             ],
           };
@@ -2717,9 +2744,8 @@ class PageService {
 
     await streamToPromise(migratePagesStream);
 
-    const existsFilter = { $and: [grantFiltersByUser, ...filter.$and] };
-    if (await Page.exists(existsFilter) && shouldContinue) {
-      return this._normalizeParentRecursively(regexps, pathsToInclude, grantFiltersByUser, user);
+    if (await Page.exists(mergedFilter) && shouldContinue) {
+      return this._normalizeParentRecursively(pathOrRegExps, publicPathsToNormalize, grantFiltersByUser, user);
     }
 
   }

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);
     });