fix: Avoid user serialization on public share response (#13144)

* fix: Avoid user serialization on public share response

* fix: shares.list does not return shares from deleted users

server/presenters/share.ts

@@ -1,7 +1,21 @@
 import type { Share } from "@server/models";
 import { presentUser } from ".";
 
-export default function presentShare(share: Share, isAdmin = false) {
+interface Options {
+  /** Whether the viewer is an admin of the team that owns the share. */
+  isAdmin?: boolean;
+  /** Whether the share is presented to a viewer without access to it. */
+  isPublic?: boolean;
+}
+
+/**
+ * Serializes a share for the API.
+ *
+ * @param share the share to present.
+ * @param options options controlling which fields are included.
+ * @returns the serialized share.
+ */
+export default function presentShare(share: Share, options: Options = {}) {
   const data = {
     id: share.id,
     sourceTitle: share.collection?.name ?? share.document?.title,
@@ -13,7 +27,9 @@ export default function presentShare(share: Share, isAdmin = false) {
     published: share.published,
     url: share.canonicalUrl,
     urlId: share.urlId,
-    createdBy: presentUser(share.user),
+    ...(options.isPublic || !share.user
+      ? {}
+      : { createdBy: presentUser(share.user) }),
     includeChildDocuments: share.includeChildDocuments,
     allowIndexing: share.allowIndexing,
     allowSubscriptions: share.allowSubscriptions,
@@ -28,7 +44,7 @@ export default function presentShare(share: Share, isAdmin = false) {
     updatedAt: share.updatedAt,
   };
 
-  if (!isAdmin) {
+  if (!options.isAdmin) {
     delete data.lastAccessedAt;
   }
 

server/routes/api/shares/shares.test.ts

@@ -177,6 +177,27 @@ describe("#shares.list", () => {
     expect(body.data[0].documentTitle).toBe(document.title);
   });
 
+  it("admins should return shares created by a deleted user", async () => {
+    const team = await buildTeam();
+    const admin = await buildAdmin({ teamId: team.id });
+    const user = await buildUser({ teamId: team.id });
+    const document = await buildDocument({ userId: admin.id, teamId: team.id });
+    const share = await buildShare({
+      documentId: document.id,
+      teamId: team.id,
+      userId: user.id,
+    });
+    await user.destroy();
+
+    const res = await server.post("/api/shares.list", admin);
+    const body = await res.json();
+    expect(res.status).toEqual(200);
+    expect(body.data.length).toEqual(1);
+    expect(body.pagination.total).toEqual(1);
+    expect(body.data[0].id).toEqual(share.id);
+    expect(body.data[0].createdBy).toBeUndefined();
+  });
+
   it("admins should not return shares in collection not a member of", async () => {
     const team = await buildTeam();
     const user = await buildUser({ teamId: team.id });
@@ -703,6 +724,51 @@ describe("#shares.info", () => {
     expect(body.data.shares[0].id).toEqual(share.id);
   });
 
+  it("should not return the sharer for an unauthenticated viewer", async () => {
+    const user = await buildUser();
+    const document = await buildDocument({
+      createdById: user.id,
+      teamId: user.teamId,
+    });
+    const share = await buildShare({
+      documentId: document.id,
+      teamId: user.teamId,
+      userId: user.id,
+      published: true,
+    });
+    const res = await server.post("/api/shares.info", {
+      body: {
+        id: share.id,
+      },
+    });
+    const body = await res.json();
+    expect(res.status).toEqual(200);
+    expect(body.data.shares[0].id).toEqual(share.id);
+    expect(body.data.shares[0].createdBy).toBeUndefined();
+  });
+
+  it("should return the sharer for a viewer with access to the share", async () => {
+    const user = await buildUser();
+    const document = await buildDocument({
+      createdById: user.id,
+      teamId: user.teamId,
+    });
+    const share = await buildShare({
+      documentId: document.id,
+      teamId: user.teamId,
+      userId: user.id,
+      published: true,
+    });
+    const res = await server.post("/api/shares.info", user, {
+      body: {
+        id: share.id,
+      },
+    });
+    const body = await res.json();
+    expect(res.status).toEqual(200);
+    expect(body.data.shares[0].createdBy.id).toEqual(user.id);
+  });
+
   it("should allow reading share by documentId", async () => {
     const user = await buildUser();
     const document = await buildDocument({

server/routes/api/shares/shares.ts

@@ -104,7 +104,12 @@ router.post(
 
       ctx.body = {
         data: {
-          shares: [presentShare(share, user?.isAdmin ?? false)],
+          shares: [
+            presentShare(share, {
+              isAdmin: user?.isAdmin ?? false,
+              isPublic: cannot(user, "read", share),
+            }),
+          ],
           sharedTree,
           team: serializedTeam,
           collection: serializedCollection,
@@ -135,7 +140,9 @@ router.post(
 
       ctx.body = {
         data: {
-          shares: shares.map((s) => presentShare(s, user.isAdmin ?? false)),
+          shares: shares.map((s) =>
+            presentShare(s, { isAdmin: user.isAdmin ?? false })
+          ),
         },
         policies: presentPolicies(user, shares),
       };
@@ -221,7 +228,7 @@ router.post(
         },
         {
           model: User,
-          required: true,
+          required: false,
           as: "user",
         },
         {
@@ -245,7 +252,9 @@ router.post(
 
     ctx.body = {
       pagination: { ...ctx.state.pagination, total },
-      data: shares.map((share) => presentShare(share, user.isAdmin)),
+      data: shares.map((share) =>
+        presentShare(share, { isAdmin: user.isAdmin })
+      ),
       policies: presentPolicies(user, shares),
     };
   }
@@ -406,7 +415,7 @@ router.post(
     await share.saveWithCtx(ctx);
 
     ctx.body = {
-      data: presentShare(share, user.isAdmin),
+      data: presentShare(share, { isAdmin: user.isAdmin }),
       policies: presentPolicies(user, [share]),
     };
   }