fix ObjectId comparison

apps/app/src/server/models/interfaces/page-operation.ts

@@ -1,4 +1,4 @@
-import { GroupType } from '@growi/core';
+import { GrantedGroup } from '@growi/core';
 
 import { PageGrant } from '~/interfaces/page';
 
@@ -11,10 +11,7 @@ export type IPageForResuming = {
   parent?: ObjectIdLike,
   grant?: number,
   grantedUsers?: ObjectIdLike[],
-  grantedGroups: {
-    type: GroupType,
-    item: ObjectIdLike,
-  }[],
+  grantedGroups: GrantedGroup[],
   descendantCount: number,
   status?: number,
   revision?: ObjectIdLike,
@@ -28,20 +25,14 @@ export type IUserForResuming = {
 
 export type IOptionsForUpdate = {
   grant?: PageGrant,
-  grantUserGroupIds?: {
-    type: GroupType,
-    item: ObjectIdLike,
-  }[],
+  grantUserGroupIds?: GrantedGroup[],
   isSyncRevisionToHackmd?: boolean,
   overwriteScopesOfDescendants?: boolean,
 };
 
 export type IOptionsForCreate = {
   format?: string,
-  grantUserGroupIds?: {
-    type: GroupType,
-    item: ObjectIdLike,
-  }[],
+  grantUserGroupIds?: GrantedGroup[],
   grant?: PageGrant,
   overwriteScopesOfDescendants?: boolean,
   isSynchronously?: boolean,

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

@@ -3,6 +3,7 @@ import {
   pagePathUtils, pathUtils, pageUtils,
   PageGrant, PageGrantCanBeOnTree,
 } from '@growi/core';
+import { et } from 'date-fns/locale';
 import escapeStringRegexp from 'escape-string-regexp';
 import mongoose from 'mongoose';
 
@@ -11,7 +12,7 @@ import ExternalUserGroupRelation from '~/features/external-user-group/server/mod
 import { IRecordApplicableGrant } from '~/interfaces/page-grant';
 import { PageDocument, PageModel } from '~/server/models/page';
 import UserGroup from '~/server/models/user-group';
-import { isIncludesObjectId, excludeTestIdsFromTargetIds, hasIntersection } from '~/server/util/compare-objectId';
+import { includesObjectIds, excludeTestIdsFromTargetIds } from '~/server/util/compare-objectId';
 
 import { ObjectIdLike } from '../interfaces/mongoose-utils';
 import { divideByType } from '../util/granted-group';
@@ -141,7 +142,7 @@ class PageGrantService {
           throw Error('grantedUserIds must have one user');
         }
 
-        if (!isIncludesObjectId(ancestor.applicableUserIds, target.grantedUserIds[0])) { // GRANT_OWNER pages under GRAND_USER_GROUP page must be owned by the member of the grantedGroup of the GRAND_USER_GROUP page
+        if (!includesObjectIds(ancestor.applicableUserIds, [target.grantedUserIds[0]])) { // GRANT_OWNER pages under GRAND_USER_GROUP page must be owned by the member of the grantedGroup of the GRAND_USER_GROUP page
           return false;
         }
       }
@@ -150,8 +151,8 @@ class PageGrantService {
         if (target.grantedGroupIds == null || target.grantedGroupIds.length === 0) {
           throw Error('grantedGroupId must not be empty');
         }
-
-        if (!hasIntersection(ancestor.applicableGroupIds, target.grantedGroupIds.map(e => e.item))) { // only child groups or the same group can exist under GRANT_USER_GROUP page
+        const targetGrantedGroupStrIds = target.grantedGroupIds.map(e => (typeof e.item === 'string' ? e.item : e.item._id));
+        if (!includesObjectIds(ancestor.applicableGroupIds, targetGrantedGroupStrIds)) { // only child groups or the same group can exist under GRANT_USER_GROUP page
           return false;
         }
       }
@@ -234,7 +235,7 @@ class PageGrantService {
 
         const userGroupRelations = await UserGroupRelation.find({ relatedGroup: { $in: targetUserGroups.map(g => g._id) } });
         const externalUserGroupRelations = await ExternalUserGroupRelation.find({ relatedGroup: { $in: targetExternalUserGroups.map(g => g._id) } });
-        applicableUserIds = [...userGroupRelations, ...externalUserGroupRelations].map(u => u.relatedUser);
+        applicableUserIds = Array.from(new Set([...userGroupRelations, ...externalUserGroupRelations].map(u => u.relatedUser)));
 
         const applicableUserGroups = (await Promise.all(targetUserGroups.map((group) => {
           return UserGroup.findGroupsWithDescendantsById(group._id);
@@ -634,7 +635,7 @@ class PageGrantService {
   private calcIsAllDescendantsGrantedByOperator(operatorGrantInfo: OperatorGrantInfo, descendantPagesGrantInfo: DescendantPagesGrantInfo): boolean {
     if (descendantPagesGrantInfo.grantSet.has(PageGrant.GRANT_OWNER)) {
       const isNonApplicableOwnerExist = descendantPagesGrantInfo.grantedUserIds.size >= 2
-        || !isIncludesObjectId([...descendantPagesGrantInfo.grantedUserIds], operatorGrantInfo.userId);
+        || !includesObjectIds([...descendantPagesGrantInfo.grantedUserIds], [operatorGrantInfo.userId]);
       if (isNonApplicableOwnerExist) {
         return false;
       }

apps/app/src/server/util/compare-objectId.spec.ts

@@ -0,0 +1,28 @@
+import { Types } from 'mongoose';
+
+import { includesObjectIds } from './compare-objectId';
+
+describe('includesObjectIds', () => {
+  const id1 = new Types.ObjectId();
+  const id2 = new Types.ObjectId();
+  const id3 = new Types.ObjectId();
+  const id4 = new Types.ObjectId();
+
+  describe('When subset of array given', () => {
+    const arr = [id1, id2, id3, id4];
+    const subset = [id1, id4];
+
+    it('returns true', () => {
+      expect(includesObjectIds(arr, subset)).toBe(true);
+    });
+  });
+
+  describe('When set that intersects with array given', () => {
+    const arr = [id1, id2, id3];
+    const subset = [id1, id4];
+
+    it('returns false', () => {
+      expect(includesObjectIds(arr, subset)).toBe(false);
+    });
+  });
+});

apps/app/src/server/util/compare-objectId.ts

@@ -5,18 +5,17 @@ import { ObjectIdLike } from '~/server/interfaces/mongoose-utils';
 type IObjectId = mongoose.Types.ObjectId;
 const ObjectId = mongoose.Types.ObjectId;
 
-export const isIncludesObjectId = (arr: ObjectIdLike[], id: ObjectIdLike): boolean => {
+/**
+ * Check if array contains all specified ObjectIds
+ * @param arr array that potentially contains potentialSubset
+ * @param potentialSubset array that is potentially a subset of arr
+ * @returns Whether or not arr includes all elements of potentialSubset
+ */
+export const includesObjectIds = (arr: ObjectIdLike[], potentialSubset: ObjectIdLike[]): boolean => {
   const _arr = arr.map(i => i.toString());
-  const _id = id.toString();
+  const _potentialSubset = potentialSubset.map(i => i.toString());
 
-  return _arr.includes(_id);
-};
-
-// Check if two arrays have an intersection
-export const hasIntersection = (arr: ObjectIdLike[], arr2: ObjectIdLike[]): boolean => {
-  const _arr = arr.map(i => i.toString());
-  const _arr2 = arr2.map(i => i.toString());
-  return _arr.some(item => _arr2.includes(item));
+  return _potentialSubset.every(id => _arr.includes(id));
 };
 
 /**

packages/core/src/interfaces/page.ts

@@ -3,14 +3,16 @@ import type { HasObjectId } from './has-object-id';
 import type { IRevision, HasRevisionShortbody, IRevisionHasId } from './revision';
 import type { SubscriptionStatusType } from './subscription';
 import type { ITag } from './tag';
-import type { IUser, IUserGroupHasId, IUserHasId } from './user';
+import type {
+  IUser, IUserGroup, IUserGroupHasId, IUserHasId,
+} from './user';
 
 export const GroupType = { userGroup: 'UserGroup', externalUserGroup: 'ExternalUserGroup' } as const;
 export type GroupType = typeof GroupType[keyof typeof GroupType];
 
 export type GrantedGroup = {
   type: GroupType,
-  item: Ref<any>,
+  item: Ref<IUserGroup>,
 }
 
 export type IPage = {