fix: SSR meta data for nested shared documents (#3646)

server/commands/documentLoader.ts

@@ -0,0 +1,187 @@
+import invariant from "invariant";
+import { Op } from "sequelize";
+import {
+  NotFoundError,
+  InvalidRequestError,
+  AuthorizationError,
+  AuthenticationError,
+} from "@server/errors";
+import { Collection, Document, Share, User, Team } from "@server/models";
+import { authorize, can } from "@server/policies";
+
+type Props = {
+  id?: string;
+  shareId?: string;
+  user?: User;
+};
+
+type Result = {
+  document: Document;
+  share?: Share;
+  collection: Collection;
+};
+
+export default async function loadDocument({
+  id,
+  shareId,
+  user,
+}: Props): Promise<Result> {
+  let document;
+  let collection;
+  let share;
+
+  if (!shareId && !(id && user)) {
+    throw AuthenticationError(`Authentication or shareId required`);
+  }
+
+  if (shareId) {
+    share = await Share.findOne({
+      where: {
+        revokedAt: {
+          [Op.is]: null,
+        },
+        id: shareId,
+      },
+      include: [
+        {
+          // unscoping here allows us to return unpublished documents
+          model: Document.unscoped(),
+          include: [
+            {
+              model: User,
+              as: "createdBy",
+              paranoid: false,
+            },
+            {
+              model: User,
+              as: "updatedBy",
+              paranoid: false,
+            },
+          ],
+          required: true,
+          as: "document",
+        },
+      ],
+    });
+
+    if (!share || share.document.archivedAt) {
+      throw InvalidRequestError("Document could not be found for shareId");
+    }
+
+    // It is possible to pass both an id and a shareId to the documents.info
+    // endpoint. In this case we'll load the document based on the `id` and check
+    // if the provided share token allows access. This is used by the frontend
+    // to navigate nested documents from a single share link.
+    if (id) {
+      document = await Document.findByPk(id, {
+        userId: user ? user.id : undefined,
+        paranoid: false,
+      }); // otherwise, if the user has an authenticated session make sure to load
+      // with their details so that we can return the correct policies, they may
+      // be able to edit the shared document
+    } else if (user) {
+      document = await Document.findByPk(share.documentId, {
+        userId: user.id,
+        paranoid: false,
+      });
+    } else {
+      document = share.document;
+    }
+
+    if (!document) {
+      throw NotFoundError("Document could not be found for shareId");
+    }
+
+    // If the user has access to read the document, we can just update
+    // the last access date and return the document without additional checks.
+    const canReadDocument = user && can(user, "read", document);
+
+    if (canReadDocument) {
+      await share.update({
+        lastAccessedAt: new Date(),
+      });
+
+      // Cannot use document.collection here as it does not include the
+      // documentStructure by default through the relationship.
+      collection = await Collection.findByPk(document.collectionId);
+      if (!collection) {
+        throw NotFoundError("Collection could not be found for document");
+      }
+
+      return {
+        document,
+        share,
+        collection,
+      };
+    }
+
+    // "published" === on the public internet.
+    // We already know that there's either no logged in user or the user doesn't
+    // have permission to read the document, so we can throw an error.
+    if (!share.published) {
+      throw AuthorizationError();
+    }
+
+    // It is possible to disable sharing at the collection so we must check
+    collection = await Collection.findByPk(document.collectionId);
+    invariant(collection, "collection not found");
+
+    if (!collection.sharing) {
+      throw AuthorizationError();
+    }
+
+    // If we're attempting to load a document that isn't the document originally
+    // shared then includeChildDocuments must be enabled and the document must
+    // still be active and nested within the shared document
+    if (share.document.id !== document.id) {
+      if (!share.includeChildDocuments) {
+        throw AuthorizationError();
+      }
+
+      const childDocumentIds = await share.document.getChildDocumentIds({
+        archivedAt: {
+          [Op.is]: null,
+        },
+      });
+      if (!childDocumentIds.includes(document.id)) {
+        throw AuthorizationError();
+      }
+    }
+
+    // It is possible to disable sharing at the team level so we must check
+    const team = await Team.findByPk(document.teamId);
+    invariant(team, "team not found");
+
+    if (!team.sharing) {
+      throw AuthorizationError();
+    }
+
+    await share.update({
+      lastAccessedAt: new Date(),
+    });
+  } else {
+    document = await Document.findByPk(id as string, {
+      userId: user ? user.id : undefined,
+      paranoid: false,
+    });
+
+    if (!document) {
+      throw NotFoundError();
+    }
+
+    if (document.deletedAt) {
+      // don't send data if user cannot restore deleted doc
+      user && authorize(user, "restore", document);
+    } else {
+      user && authorize(user, "read", document);
+    }
+
+    collection = document.collection;
+  }
+
+  return {
+    document,
+    share,
+    collection,
+  };
+}

server/models/Share.ts

@@ -85,6 +85,10 @@ class Share extends IdModel {
     return !!this.revokedAt;
   }
 
+  get canonicalUrl() {
+    return `${this.team.url}/share/${this.id}`;
+  }
+
   // associations
 
   @BelongsTo(() => User, "revokedById")

server/routes/api/documents.ts

@@ -5,6 +5,7 @@ import { Op, ScopeOptions, WhereOptions } from "sequelize";
 import { subtractDate } from "@shared/utils/date";
 import documentCreator from "@server/commands/documentCreator";
 import documentImporter from "@server/commands/documentImporter";
+import documentLoader from "@server/commands/documentLoader";
 import documentMover from "@server/commands/documentMover";
 import documentPermanentDeleter from "@server/commands/documentPermanentDeleter";
 import documentUpdater from "@server/commands/documentUpdater";
@@ -12,7 +13,6 @@ import { sequelize } from "@server/database/sequelize";
 import {
   NotFoundError,
   InvalidRequestError,
-  AuthorizationError,
   AuthenticationError,
 } from "@server/errors";
 import auth from "@server/middlewares/authentication";
@@ -23,13 +23,12 @@ import {
   Event,
   Revision,
   SearchQuery,
-  Share,
   Star,
   User,
   View,
   Team,
 } from "@server/models";
-import { authorize, cannot, can } from "@server/policies";
+import { authorize, cannot } from "@server/policies";
 import {
   presentCollection,
   presentDocument,
@@ -382,175 +381,6 @@ router.post("documents.drafts", auth(), pagination(), async (ctx) => {
   };
 });
 
-async function loadDocument({
-  id,
-  shareId,
-  user,
-}: {
-  id?: string;
-  shareId?: string;
-  user?: User;
-}): Promise<{
-  document: Document;
-  share?: Share;
-  collection: Collection;
-}> {
-  let document;
-  let collection;
-  let share;
-
-  if (!shareId && !(id && user)) {
-    throw AuthenticationError(`Authentication or shareId required`);
-  }
-
-  if (shareId) {
-    share = await Share.findOne({
-      where: {
-        revokedAt: {
-          [Op.is]: null,
-        },
-        id: shareId,
-      },
-      include: [
-        {
-          // unscoping here allows us to return unpublished documents
-          model: Document.unscoped(),
-          include: [
-            {
-              model: User,
-              as: "createdBy",
-              paranoid: false,
-            },
-            {
-              model: User,
-              as: "updatedBy",
-              paranoid: false,
-            },
-          ],
-          required: true,
-          as: "document",
-        },
-      ],
-    });
-
-    if (!share || share.document.archivedAt) {
-      throw InvalidRequestError("Document could not be found for shareId");
-    }
-
-    // It is possible to pass both an id and a shareId to the documents.info
-    // endpoint. In this case we'll load the document based on the `id` and check
-    // if the provided share token allows access. This is used by the frontend
-    // to navigate nested documents from a single share link.
-    if (id) {
-      document = await Document.findByPk(id, {
-        userId: user ? user.id : undefined,
-        paranoid: false,
-      }); // otherwise, if the user has an authenticated session make sure to load
-      // with their details so that we can return the correct policies, they may
-      // be able to edit the shared document
-    } else if (user) {
-      document = await Document.findByPk(share.documentId, {
-        userId: user.id,
-        paranoid: false,
-      });
-    } else {
-      document = share.document;
-    }
-
-    invariant(document, "document not found");
-
-    // If the user has access to read the document, we can just update
-    // the last access date and return the document without additional checks.
-    const canReadDocument = user && can(user, "read", document);
-
-    if (canReadDocument) {
-      await share.update({
-        lastAccessedAt: new Date(),
-      });
-
-      // Cannot use document.collection here as it does not include the
-      // documentStructure by default through the relationship.
-      collection = await Collection.findByPk(document.collectionId);
-      invariant(collection, "collection not found");
-
-      return {
-        document,
-        share,
-        collection,
-      };
-    }
-
-    // "published" === on the public internet.
-    // We already know that there's either no logged in user or the user doesn't
-    // have permission to read the document, so we can throw an error.
-    if (!share.published) {
-      throw AuthorizationError();
-    }
-
-    // It is possible to disable sharing at the collection so we must check
-    collection = await Collection.findByPk(document.collectionId);
-    invariant(collection, "collection not found");
-
-    if (!collection.sharing) {
-      throw AuthorizationError();
-    }
-
-    // If we're attempting to load a document that isn't the document originally
-    // shared then includeChildDocuments must be enabled and the document must
-    // still be active and nested within the shared document
-    if (share.document.id !== document.id) {
-      if (!share.includeChildDocuments) {
-        throw AuthorizationError();
-      }
-
-      const childDocumentIds = await share.document.getChildDocumentIds({
-        archivedAt: {
-          [Op.is]: null,
-        },
-      });
-      if (!childDocumentIds.includes(document.id)) {
-        throw AuthorizationError();
-      }
-    }
-
-    // It is possible to disable sharing at the team level so we must check
-    const team = await Team.findByPk(document.teamId);
-    invariant(team, "team not found");
-
-    if (!team.sharing) {
-      throw AuthorizationError();
-    }
-
-    await share.update({
-      lastAccessedAt: new Date(),
-    });
-  } else {
-    document = await Document.findByPk(id as string, {
-      userId: user ? user.id : undefined,
-      paranoid: false,
-    });
-
-    if (!document) {
-      throw NotFoundError();
-    }
-
-    if (document.deletedAt) {
-      // don't send data if user cannot restore deleted doc
-      user && authorize(user, "restore", document);
-    } else {
-      user && authorize(user, "read", document);
-    }
-
-    collection = document.collection;
-  }
-
-  return {
-    document,
-    share,
-    collection,
-  };
-}
-
 router.post(
   "documents.info",
   auth({
@@ -560,7 +390,7 @@ router.post(
     const { id, shareId, apiVersion } = ctx.body;
     assertPresent(id || shareId, "id or shareId is required");
     const { user } = ctx.state;
-    const { document, share, collection } = await loadDocument({
+    const { document, share, collection } = await documentLoader({
       id,
       shareId,
       user,
@@ -598,7 +428,7 @@ router.post(
     const { id, shareId } = ctx.body;
     assertPresent(id || shareId, "id or shareId is required");
     const { user } = ctx.state;
-    const { document } = await loadDocument({
+    const { document } = await documentLoader({
       id,
       shareId,
       user,
@@ -786,7 +616,7 @@ router.post(
     let response;
 
     if (shareId) {
-      const { share, document } = await loadDocument({
+      const { share, document } = await documentLoader({
         shareId,
         user,
       });

server/routes/app.ts

@@ -0,0 +1,100 @@
+import fs from "fs";
+import path from "path";
+import util from "util";
+import { Context, Next } from "koa";
+import { escape } from "lodash";
+import documentLoader from "@server/commands/documentLoader";
+import env from "@server/env";
+import presentEnv from "@server/presenters/env";
+import prefetchTags from "@server/utils/prefetchTags";
+
+const isProduction = env.ENVIRONMENT === "production";
+const isTest = env.ENVIRONMENT === "test";
+const readFile = util.promisify(fs.readFile);
+
+const readIndexFile = async (ctx: Context): Promise<Buffer> => {
+  if (isProduction) {
+    return readFile(path.join(__dirname, "../../app/index.html"));
+  }
+
+  if (isTest) {
+    return readFile(path.join(__dirname, "../static/index.html"));
+  }
+
+  const middleware = ctx.devMiddleware;
+  await new Promise((resolve) => middleware.waitUntilValid(resolve));
+  return new Promise((resolve, reject) => {
+    middleware.fileSystem.readFile(
+      `${ctx.webpackConfig.output.path}/index.html`,
+      (err: Error, result: Buffer) => {
+        if (err) {
+          return reject(err);
+        }
+
+        resolve(result);
+      }
+    );
+  });
+};
+
+export const renderApp = async (
+  ctx: Context,
+  next: Next,
+  options: { title?: string; description?: string; canonical?: string } = {}
+) => {
+  const {
+    title = "Outline",
+    description = "A modern team knowledge base for your internal documentation, product specs, support answers, meeting notes, onboarding, &amp; more…",
+    canonical = "",
+  } = options;
+
+  if (ctx.request.path === "/realtime/") {
+    return next();
+  }
+
+  const { shareId } = ctx.params;
+  const page = await readIndexFile(ctx);
+  const environment = `
+    window.env = ${JSON.stringify(presentEnv(env))};
+  `;
+  ctx.body = page
+    .toString()
+    .replace(/\/\/inject-env\/\//g, environment)
+    .replace(/\/\/inject-title\/\//g, escape(title))
+    .replace(/\/\/inject-description\/\//g, escape(description))
+    .replace(/\/\/inject-canonical\/\//g, canonical)
+    .replace(/\/\/inject-prefetch\/\//g, shareId ? "" : prefetchTags)
+    .replace(/\/\/inject-slack-app-id\/\//g, env.SLACK_APP_ID || "");
+};
+
+export const renderShare = async (ctx: Context, next: Next) => {
+  const { shareId, documentSlug } = ctx.params;
+  // Find the share record if publicly published so that the document title
+  // can be be returned in the server-rendered HTML. This allows it to appear in
+  // unfurls with more reliablity
+  let share, document;
+
+  try {
+    const result = await documentLoader({
+      id: documentSlug,
+      shareId,
+    });
+    share = result.share;
+    document = result.document;
+  } catch (err) {
+    // If the share or document does not exist, return a 404.
+    ctx.status = 404;
+  }
+
+  // Allow shares to be embedded in iframes on other websites
+  ctx.remove("X-Frame-Options");
+
+  // Inject share information in SSR HTML
+  return renderApp(ctx, next, {
+    title: document?.title,
+    description: document?.getSummary(),
+    canonical: share
+      ? `${share.canonicalUrl}${documentSlug && document ? document.url : ""}`
+      : undefined,
+  });
+};

server/routes/index.test.ts

@@ -9,20 +9,20 @@ beforeEach(() => flushdb());
 afterAll(() => server.close());
 
 describe("/share/:id", () => {
-  it("should return standard title in html when loading share", async () => {
+  it("should return standard title in html when loading unpublished share", async () => {
     const share = await buildShare({
       published: false,
     });
     const res = await server.get(`/share/${share.id}`);
     const body = await res.text();
-    expect(res.status).toEqual(200);
+    expect(res.status).toEqual(404);
     expect(body).toContain("<title>Outline</title>");
   });
 
   it("should return standard title in html when share does not exist", async () => {
     const res = await server.get(`/share/junk`);
     const body = await res.text();
-    expect(res.status).toEqual(200);
+    expect(res.status).toEqual(404);
     expect(body).toContain("<title>Outline</title>");
   });
 
@@ -30,11 +30,12 @@ describe("/share/:id", () => {
     const document = await buildDocument();
     const share = await buildShare({
       documentId: document.id,
+      teamId: document.teamId,
     });
     await document.destroy();
     const res = await server.get(`/share/${share.id}`);
     const body = await res.text();
-    expect(res.status).toEqual(200);
+    expect(res.status).toEqual(404);
     expect(body).toContain("<title>Outline</title>");
   });
 
@@ -42,6 +43,7 @@ describe("/share/:id", () => {
     const document = await buildDocument();
     const share = await buildShare({
       documentId: document.id,
+      teamId: document.teamId,
     });
     const res = await server.get(`/share/${share.id}`);
     const body = await res.text();
@@ -53,8 +55,9 @@ describe("/share/:id", () => {
     const document = await buildDocument();
     const share = await buildShare({
       documentId: document.id,
+      teamId: document.teamId,
     });
-    const res = await server.get(`/share/${share.id}/doc/test-Cl6g1AgPYn`);
+    const res = await server.get(`/share/${share.id}/doc/${document.urlId}`);
     const body = await res.text();
     expect(res.status).toEqual(200);
     expect(body).toContain(`<title>${document.title}</title>`);

server/routes/index.ts

@@ -1,111 +1,19 @@
-import fs from "fs";
 import path from "path";
-import util from "util";
+import Koa from "koa";
-import Koa, { Context, Next } from "koa";
 import Router from "koa-router";
 import send from "koa-send";
 import serve from "koa-static";
-import { escape } from "lodash";
-import isUUID from "validator/lib/isUUID";
 import { languages } from "@shared/i18n";
 import env from "@server/env";
 import { NotFoundError } from "@server/errors";
-import Share from "@server/models/Share";
 import { opensearchResponse } from "@server/utils/opensearch";
-import prefetchTags from "@server/utils/prefetchTags";
 import { robotsResponse } from "@server/utils/robots";
 import apexRedirect from "../middlewares/apexRedirect";
-import presentEnv from "../presenters/env";
+import { renderApp, renderShare } from "./app";
 
 const isProduction = env.ENVIRONMENT === "production";
-const isTest = env.ENVIRONMENT === "test";
 const koa = new Koa();
 const router = new Router();
-const readFile = util.promisify(fs.readFile);
-
-const readIndexFile = async (ctx: Context): Promise<Buffer> => {
-  if (isProduction) {
-    return readFile(path.join(__dirname, "../../app/index.html"));
-  }
-
-  if (isTest) {
-    return readFile(path.join(__dirname, "../static/index.html"));
-  }
-
-  const middleware = ctx.devMiddleware;
-  await new Promise((resolve) => middleware.waitUntilValid(resolve));
-  return new Promise((resolve, reject) => {
-    middleware.fileSystem.readFile(
-      `${ctx.webpackConfig.output.path}/index.html`,
-      (err: Error, result: Buffer) => {
-        if (err) {
-          return reject(err);
-        }
-
-        resolve(result);
-      }
-    );
-  });
-};
-
-const renderApp = async (
-  ctx: Context,
-  next: Next,
-  options: { title?: string; description?: string; canonical?: string } = {}
-) => {
-  const {
-    title = "Outline",
-    description = "A modern team knowledge base for your internal documentation, product specs, support answers, meeting notes, onboarding, &amp; more…",
-    canonical = "",
-  } = options;
-
-  if (ctx.request.path === "/realtime/") {
-    return next();
-  }
-
-  const { shareId } = ctx.params;
-  const page = await readIndexFile(ctx);
-  const environment = `
-    window.env = ${JSON.stringify(presentEnv(env))};
-  `;
-  ctx.body = page
-    .toString()
-    .replace(/\/\/inject-env\/\//g, environment)
-    .replace(/\/\/inject-title\/\//g, escape(title))
-    .replace(/\/\/inject-description\/\//g, escape(description))
-    .replace(/\/\/inject-canonical\/\//g, canonical)
-    .replace(/\/\/inject-prefetch\/\//g, shareId ? "" : prefetchTags)
-    .replace(/\/\/inject-slack-app-id\/\//g, env.SLACK_APP_ID || "");
-};
-
-const renderShare = async (ctx: Context, next: Next) => {
-  const { shareId } = ctx.params;
-  // Find the share record if publicly published so that the document title
-  // can be be returned in the server-rendered HTML. This allows it to appear in
-  // unfurls with more reliablity
-  let share;
-
-  if (isUUID(shareId)) {
-    share = await Share.findOne({
-      where: {
-        id: shareId,
-        published: true,
-      },
-    });
-  }
-
-  // Allow shares to be embedded in iframes on other websites
-  ctx.remove("X-Frame-Options");
-
-  // Inject share information in SSR HTML
-  return renderApp(ctx, next, {
-    title: share?.document?.title,
-    description: share?.document?.getSummary(),
-    canonical: share?.team
-      ? ctx.request.href.replace(ctx.request.origin, share.team.url)
-      : undefined,
-  });
-};
 
 // serve static assets
 koa.use(
@@ -174,7 +82,7 @@ router.get("/opensearch.xml", (ctx) => {
 });
 
 router.get("/share/:shareId", renderShare);
-
+router.get("/share/:shareId/doc/:documentSlug", renderShare);
 router.get("/share/:shareId/*", renderShare);
 
 // catch all for application