fix: policy serialization (#13211)

* fix: Membership IDs not serialized correctly in policies

* fix: Policy serialization

app/components/WebsocketProvider.tsx

@@ -63,6 +63,47 @@ function invalidateChildPolicies(
   }
 }
 
+/**
+ * Re-check the current user's access to a collection with the server and
+ * discard whatever they can no longer read. Abilities cannot be recalculated on
+ * the client, so access is never inferred from the cached policy.
+ *
+ * @param collectionId the ID of the collection access may have been lost to.
+ * @param stores the stores to remove the collection and its documents from.
+ */
+async function revokeCollectionAccess(
+  collectionId: string,
+  {
+    collections,
+    documents,
+    memberships,
+    policies,
+  }: Pick<RootStore, "collections" | "documents" | "memberships" | "policies">
+) {
+  policies.remove(collectionId);
+
+  try {
+    await collections.fetch(collectionId, { force: true });
+  } catch (err) {
+    if (err instanceof AuthorizationError || err instanceof NotFoundError) {
+      memberships.removeAll({ collectionId });
+      collections.remove(collectionId, { permanent: true });
+    } else {
+      Logger.error(
+        "Failed to fetch collection after access change",
+        toError(err)
+      );
+    }
+    return;
+  }
+
+  // Admins keep visibility of the collection itself, but may no longer be able
+  // to read the documents within it.
+  if (!policies.abilities(collectionId).readDocument) {
+    documents.removeInCollection(collectionId);
+  }
+}
+
 function useConnectionHandlers() {
   const { auth } = useStores();
 
@@ -161,8 +202,8 @@ function useEntityHandlers() {
                 err instanceof AuthorizationError ||
                 err instanceof NotFoundError
               ) {
-                documents.remove(documentId);
-                return;
+                documents.remove(documentId, { permanent: true });
+                continue;
               }
             }
 
@@ -215,8 +256,8 @@ function useEntityHandlers() {
                 err instanceof NotFoundError
               ) {
                 memberships.removeAll({ collectionId });
-                collections.remove(collectionId);
-                return;
+                collections.remove(collectionId, { permanent: true });
+                continue;
               }
             }
           }
@@ -266,7 +307,7 @@ function useDocumentHandlers() {
             !document.collectionId &&
             document.createdBy?.id !== currentUserId
           ) {
-            documents.remove(document.id);
+            documents.remove(document.id, { permanent: true });
           } else {
             documents.add(document);
           }
@@ -313,7 +354,7 @@ function useDocumentHandlers() {
     socket.on(
       "documents.permanent_delete",
       (event: WebsocketEntityDeletedEvent) => {
-        documents.remove(event.modelId);
+        documents.remove(event.modelId, { permanent: true });
       }
     );
 
@@ -352,7 +393,7 @@ function useDocumentHandlers() {
 
         const policy = policies.get(event.documentId!);
         if (policy && policy.abilities.read === false) {
-          documents.remove(event.documentId!);
+          documents.remove(event.documentId!, { permanent: true });
         }
       }
     );
@@ -385,6 +426,11 @@ function useDocumentHandlers() {
       "documents.remove_group",
       (event: PartialExcept<GroupMembership, "id">) => {
         groupMemberships.remove(event.id);
+
+        const policy = policies.get(event.documentId!);
+        if (policy && policy.abilities.read === false) {
+          documents.remove(event.documentId!, { permanent: true });
+        }
       }
     );
   };
@@ -509,12 +555,16 @@ function useCollectionHandlers() {
       }
     });
 
-    socket.on("collections.remove_user", (event: Membership) => {
+    socket.on("collections.remove_user", async (event: Membership) => {
       memberships.remove(event.id);
 
-      const policy = policies.get(event.collectionId);
-      if (policy && policy.abilities.read === false) {
-        collections.remove(event.collectionId);
+      if (event.userId === currentUserId) {
+        await revokeCollectionAccess(event.collectionId, {
+          collections,
+          documents,
+          memberships,
+          policies,
+        });
       }
     });
 
@@ -535,12 +585,30 @@ function useCollectionHandlers() {
     socket.on("collections.remove_group", async (event: GroupMembership) => {
       groupMemberships.remove(event.id);
 
+      // The event reaches everyone with access to the collection, so the policy
+      // narrows it to those that may have held it through the group.
       const policy = policies.get(event.collectionId!);
-      if (policy && policy.abilities.read === false) {
-        collections.remove(event.collectionId!);
+      if (!policy || policy.abilities.read === false) {
+        await revokeCollectionAccess(event.collectionId!, {
+          collections,
+          documents,
+          memberships,
+          policies,
+        });
       }
     });
 
+    socket.on(
+      "collections.revoke_access",
+      async (event: WebsocketEntityDeletedEvent) =>
+        revokeCollectionAccess(event.modelId, {
+          collections,
+          documents,
+          memberships,
+          policies,
+        })
+    );
+
     socket.on(
       "collections.update_index",
       action((event: WebsocketCollectionUpdateIndexEvent) => {

app/models/GroupMembership.ts

@@ -3,7 +3,6 @@ import type { CollectionPermission, DocumentPermission } from "@shared/types";
 import Collection from "./Collection";
 import Document from "./Document";
 import Group from "./Group";
-import { AfterRemove } from "./decorators/Lifecycle";
 import Relation from "./decorators/Relation";
 import NavigableModel from "./base/NavigableModel";
 
@@ -57,13 +56,6 @@ class GroupMembership extends NavigableModel {
       ...options,
     });
   }
-
-  // hooks
-
-  @AfterRemove
-  public static removeFromPolicies(model: GroupMembership) {
-    model.store.rootStore.policies.removeForMembership(model.id);
-  }
 }
 
 export default GroupMembership;

app/models/Membership.ts

@@ -3,7 +3,6 @@ import type { CollectionPermission } from "@shared/types";
 import Collection from "./Collection";
 import User from "./User";
 import Model from "./base/Model";
-import { AfterRemove } from "./decorators/Lifecycle";
 import Relation from "./decorators/Relation";
 
 class Membership extends Model {
@@ -21,13 +20,6 @@ class Membership extends Model {
 
   @observable
   permission: CollectionPermission;
-
-  // hooks
-
-  @AfterRemove
-  public static removeFromPolicies(model: Membership) {
-    model.store.rootStore.policies.removeForMembership(model.id);
-  }
 }
 
 export default Membership;

app/models/UserMembership.ts

@@ -4,7 +4,6 @@ import type UserMembershipsStore from "~/stores/UserMembershipsStore";
 import Document from "./Document";
 import User from "./User";
 import Field from "./decorators/Field";
-import { AfterRemove } from "./decorators/Lifecycle";
 import Relation from "./decorators/Relation";
 import NavigableModel from "./base/NavigableModel";
 
@@ -85,13 +84,6 @@ class UserMembership extends NavigableModel {
     const index = memberships.indexOf(this);
     return memberships[index + 1];
   }
-
-  // hooks
-
-  @AfterRemove
-  public static removeFromPolicies(model: UserMembership) {
-    model.store.rootStore.policies.removeForMembership(model.id);
-  }
 }
 
 export default UserMembership;

app/stores/DocumentsStore.ts

@@ -191,6 +191,19 @@ export default class DocumentsStore extends Store<Document> {
     return orderBy(this.inCollection(collectionId), "popularityScore", "desc");
   }
 
+  /**
+   * Evict every document belonging to a collection from the store, for use when
+   * the current user has lost access to the collection's documents.
+   *
+   * @param collectionId the ID of the collection to evict documents for.
+   */
+  @action
+  removeInCollection(collectionId: string) {
+    this.orderedData
+      .filter((document) => document.collectionId === collectionId)
+      .forEach((document) => this.remove(document.id, { permanent: true }));
+  }
+
   get(id: string): Document | undefined {
     return id
       ? (this.data.get(id) ??

app/stores/GroupMembershipsStore.ts

@@ -21,6 +21,17 @@ export default class GroupMembershipsStore extends Store<GroupMembership> {
     super(rootStore, GroupMembership);
   }
 
+  /**
+   * Remove a membership, and the access that it granted.
+   *
+   * @param id the ID of the membership to remove.
+   */
+  @action
+  remove(id: string, options?: { permanent?: boolean }): void {
+    super.remove(id, options);
+    this.rootStore.policies.removeForMembership(id);
+  }
+
   @action
   fetchPage = async ({
     collectionId,

app/stores/MembershipsStore.ts

@@ -18,6 +18,17 @@ export default class MembershipsStore extends Store<Membership> {
     super(rootStore, Membership);
   }
 
+  /**
+   * Remove a membership, and the access that it granted.
+   *
+   * @param id the ID of the membership to remove.
+   */
+  @action
+  remove(id: string, options?: { permanent?: boolean }): void {
+    super.remove(id, options);
+    this.rootStore.policies.removeForMembership(id);
+  }
+
   @action
   fetchPage = async (
     params: (PaginationParams & { id?: string }) | undefined

app/stores/UserMembershipsStore.ts

@@ -18,6 +18,17 @@ export default class UserMembershipsStore extends Store<UserMembership> {
     super(rootStore, UserMembership);
   }
 
+  /**
+   * Remove a membership, and the access that it granted.
+   *
+   * @param id the ID of the membership to remove.
+   */
+  @action
+  remove(id: string, options?: { permanent?: boolean }): void {
+    super.remove(id, options);
+    this.rootStore.policies.removeForMembership(id);
+  }
+
   @action
   fetchPage = async (params?: PaginationParams): Promise<UserMembership[]> => {
     this.isFetching = true;

app/stores/base/Store.ts

@@ -171,8 +171,16 @@ export default abstract class Store<T extends Model> {
     return item;
   };
 
+  /**
+   * Remove a model, and any models that cascade from it, from the store.
+   *
+   * @param id the ID of the model to remove.
+   * @param options.permanent whether soft-deletable models should be evicted
+   * from the store entirely rather than marked as deleted. Use when the model
+   * is gone for good, or is no longer accessible to the current user.
+   */
   @action
-  remove(id: string): void {
+  remove(id: string, options?: { permanent?: boolean }): void {
     const model = this.data.get(id);
     if (!model) {
       return;
@@ -195,7 +203,7 @@ export default abstract class Store<T extends Model> {
           }
 
           if (deleteBehavior === "cascade") {
-            store.remove(item.id);
+            store.remove(item.id, options);
           } else if (deleteBehavior === "null") {
             // @ts-expect-error TODO
             item[relation.idKey] = null;
@@ -211,7 +219,7 @@ export default abstract class Store<T extends Model> {
 
     LifecycleManager.executeHooks(model.constructor, "beforeRemove", model);
 
-    if (model instanceof ParanoidModel) {
+    if (model instanceof ParanoidModel && !options?.permanent) {
       model.deletedAt = new Date().toISOString();
     } else {
       this.data.delete(id);

server/policies/cancan.ts

@@ -11,11 +11,17 @@ type Policy = Record<string, boolean | string[]>;
 /** Default so option-free calls share one object and stay cacheable. */
 const noOptions: Readonly<Record<string, never>> = Object.freeze({});
 
+/**
+ * The result of an ability condition. Strings are the IDs of the memberships
+ * that granted access, and may be nested in arrays by the `and` / `or` helpers.
+ */
+type ConditionResult = boolean | string | ConditionResult[];
+
 type Condition<T extends Constructor, P extends Constructor> = (
   performer: InstanceType<P>,
   target: InstanceType<T> | null,
   options?: unknown
-) => boolean | string;
+) => ConditionResult;
 
 type Ability = {
   model: Constructor;
@@ -281,28 +287,37 @@ export class CanCan {
     );
 
     // Check conditions only for matching abilities
-    const seenConditions = new Set<boolean | string>();
     const membershipIds: string[] = [];
     let hasNonMembershipMatch = false;
 
+    const collect = (result: ConditionResult) => {
+      if (!result) {
+        return;
+      }
+      if (typeof result === "string") {
+        if (!membershipIds.includes(result)) {
+          membershipIds.push(result);
+        }
+        return;
+      }
+      // Conditions are composed with the `and` / `or` helpers, which return the
+      // operands themselves rather than a boolean, so the membership IDs that
+      // granted access can be nested at any depth.
+      if (Array.isArray(result)) {
+        for (const value of result) {
+          collect(value);
+        }
+        return;
+      }
+      hasNonMembershipMatch = true;
+    };
+
     for (const ability of matchingAbilities) {
       if (!ability.condition) {
         continue;
       }
 
-      const result = ability.condition(performer, target, options);
-
-      if (!result || seenConditions.has(result)) {
-        continue;
-      }
-
-      seenConditions.add(result);
-
-      if (typeof result === "string") {
-        membershipIds.push(result);
-      } else {
-        hasNonMembershipMatch = true;
-      }
+      collect(ability.condition(performer, target, options));
     }
 
     return membershipIds.length > 0 ? membershipIds : hasNonMembershipMatch;

server/policies/document.test.ts

@@ -270,6 +270,53 @@ describe("private collection", () => {
   });
 });
 
+describe("membership ids", () => {
+  it("should return the collection membership that grants access", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({ teamId: team.id });
+    const collection = await buildCollection({
+      teamId: team.id,
+      permission: null,
+    });
+    const membership = await UserMembership.create({
+      collectionId: collection.id,
+      userId: user.id,
+      createdById: user.id,
+      permission: CollectionPermission.ReadWrite,
+    });
+    const doc = await buildDocument({
+      teamId: team.id,
+      collectionId: collection.id,
+    });
+    const document = await Document.findByPk(doc.id, { userId: user.id });
+    const abilities = serialize(user, document);
+    expect(abilities.read).toEqual([membership.id]);
+    expect(abilities.update).toEqual([membership.id]);
+  });
+
+  it("should return the document membership that grants access", async () => {
+    const team = await buildTeam();
+    const user = await buildUser({ teamId: team.id });
+    const collection = await buildCollection({
+      teamId: team.id,
+      permission: null,
+    });
+    const doc = await buildDocument({
+      teamId: team.id,
+      collectionId: collection.id,
+    });
+    const membership = await UserMembership.create({
+      documentId: doc.id,
+      userId: user.id,
+      createdById: user.id,
+      permission: DocumentPermission.Read,
+    });
+    const document = await Document.findByPk(doc.id, { userId: user.id });
+    const abilities = serialize(user, document);
+    expect(abilities.read).toEqual([membership.id]);
+  });
+});
+
 describe("no collection", () => {
   it("should allow no permissions for team member", async () => {
     const team = await buildTeam();

server/queues/processors/WebsocketsProcessor.ts

@@ -203,11 +203,12 @@ export default class WebsocketsProcessor {
       }
 
       case "documents.permanent_delete": {
-        return socketio
-          .to(`collection-${event.collectionId}`)
-          .emit(event.name, {
-            modelId: event.documentId,
-          });
+        // The document is already gone, so the channels it was published to
+        // cannot be resolved. The payload is an ID only, so it is broadcast to
+        // the team to ensure everyone holding a copy discards it.
+        return socketio.to(`team-${event.teamId}`).emit(event.name, {
+          modelId: event.documentId,
+        });
       }
 
       case "documents.archive":
@@ -231,8 +232,19 @@ export default class WebsocketsProcessor {
           },
           paranoid: false,
         });
+        // Documents are invalidated in both the source and destination
+        // collections – members of the former may no longer have access to
+        // them. This is distinct from the collection invalidation below, which
+        // only refreshes the document structure.
+        const documentChannels = uniq(
+          concat(
+            event.data.collectionIds.map((id) => `collection-${id}`),
+            `collection-${event.collectionId}`
+          )
+        );
+
         documents.forEach((document) => {
-          socketio.to(`collection-${document.collectionId}`).emit("entities", {
+          socketio.to(documentChannels).emit("entities", {
             event: event.name,
             invalidatedPolicies: [document.id],
             documentIds: [
@@ -369,10 +381,6 @@ export default class WebsocketsProcessor {
         // the collection became private, rebuild the channel from explicit
         // memberships only.
         if (attributes?.permission === null && previous?.permission) {
-          socketio
-            .in(`collection-${collection.id}`)
-            .socketsLeave(`collection-${collection.id}`);
-
           const [memberships, groupMemberships] = await Promise.all([
             UserMembership.findAll({
               where: { collectionId: collection.id },
@@ -385,6 +393,18 @@ export default class WebsocketsProcessor {
             ...memberships.map((m) => `user-${m.userId}`),
             ...groupMemberships.map((m) => `group-${m.groupId}`),
           ];
+
+          // Everyone in the channel without an explicit membership has lost
+          // access to the documents within and must discard them.
+          socketio
+            .in(`collection-${collection.id}`)
+            .except(rooms)
+            .emit("collections.revoke_access", { modelId: collection.id });
+
+          socketio
+            .in(`collection-${collection.id}`)
+            .socketsLeave(`collection-${collection.id}`);
+
           if (rooms.length) {
             socketio.in(rooms).socketsJoin(`collection-${collection.id}`);
           }