From 546096fcc90b2de6da56aa5b48280e4eb89c1980 Mon Sep 17 00:00:00 2001 From: Alex Hall Date: Thu, 24 Mar 2022 12:59:47 +0200 Subject: [PATCH] (core) Clean up and refactor uses of HomeDBManager.getDoc Summary: Firstly I just wanted some more consistency and less repetition in places where Documents are retrieved from the DB, so it's more obvious when code differs from the norm. Main changes for that part: - Let HomeDBManager accept a `Request` directly and convert it to a `Scope`, and use this in a few places. - `getScope` tries `req.docAuth.docId` if `req.params` doesn't have a docId. I also refactored how `_createActiveDoc` gets the document URL, separating out getting the document from getting a URL for it. This is because I want to use that document object in a future diff, but I also just find it cleaner. Notable changes for that: - Extracted a new method `HomeDBManager.getRawDocById` as an alternative to `getDoc` that's explicitly for when you only have a document ID. - Removed the interface method `GristServer.getDocUrl` and its two implementations because it wasn't used elsewhere and it didn't really add anything on top of getting a doc (now done by `getRawDocById`) and `getResourceUrl`. - Between `cachedDoc` and `getRawDocById` (which represent previously existing code paths) also try `getDoc(getScope(docSession.req))`, which is new, because it seems better to only `getRawDocById` as a last resort. Test Plan: Existing tests Reviewers: georgegevoian Reviewed By: georgegevoian Differential Revision: https://phab.getgrist.com/D3328 --- app/gen-server/ApiServer.ts | 2 +- app/gen-server/lib/HomeDBManager.ts | 57 +++++++++++---- app/server/lib/DocApi.ts | 107 +++++++++++++++------------- app/server/lib/DocManager.ts | 45 +++++++++--- app/server/lib/DocWorker.ts | 9 ++- app/server/lib/FlexServer.ts | 14 ---- app/server/lib/GristServer.ts | 1 - app/server/lib/requestUtils.ts | 4 +- 8 files changed, 147 insertions(+), 92 deletions(-) diff --git a/app/gen-server/ApiServer.ts b/app/gen-server/ApiServer.ts index 8c358622..b51253f6 100644 --- a/app/gen-server/ApiServer.ts +++ b/app/gen-server/ApiServer.ts @@ -287,7 +287,7 @@ export class ApiServer { // GET /api/docs/:did // Get information about a document. this._app.get('/api/docs/:did', expressWrap(async (req, res) => { - const query = await this._dbManager.getDoc(getDocScope(req)); + const query = await this._dbManager.getDoc(req); return sendOkReply(req, res, query); })); diff --git a/app/gen-server/lib/HomeDBManager.ts b/app/gen-server/lib/HomeDBManager.ts index a6fd5722..a690b06a 100644 --- a/app/gen-server/lib/HomeDBManager.ts +++ b/app/gen-server/lib/HomeDBManager.ts @@ -7,12 +7,23 @@ import {FullUser, UserProfile} from 'app/common/LoginSessionAPI'; import {checkSubdomainValidity} from 'app/common/orgNameUtils'; import {UserOrgPrefs} from 'app/common/Prefs'; import * as roles from 'app/common/roles'; -// TODO: API should implement UserAPI -import {ANONYMOUS_USER_EMAIL, DocumentProperties, EVERYONE_EMAIL, getRealAccess, - ManagerDelta, NEW_DOCUMENT_CODE, OrganizationProperties, - Organization as OrgInfo, PermissionData, PermissionDelta, SUPPORT_EMAIL, UserAccessData, - UserOptions, - WorkspaceProperties} from "app/common/UserAPI"; +import {StringUnion} from 'app/common/StringUnion'; +import { + ANONYMOUS_USER_EMAIL, + DocumentProperties, + EVERYONE_EMAIL, + getRealAccess, + ManagerDelta, + NEW_DOCUMENT_CODE, + OrganizationProperties, + Organization as OrgInfo, + PermissionData, + PermissionDelta, + SUPPORT_EMAIL, + UserAccessData, + UserOptions, + WorkspaceProperties +} from "app/common/UserAPI"; import {AclRule, AclRuleDoc, AclRuleOrg, AclRuleWs} from "app/gen-server/entity/AclRule"; import {Alias} from "app/gen-server/entity/Alias"; import {BillingAccount, ExternalBillingOptions} from "app/gen-server/entity/BillingAccount"; @@ -29,19 +40,34 @@ import {Workspace} from "app/gen-server/entity/Workspace"; import {Permissions} from 'app/gen-server/lib/Permissions'; import {scrubUserFromOrg} from "app/gen-server/lib/scrubUserFromOrg"; import {applyPatch} from 'app/gen-server/lib/TypeORMPatches'; -import {bitOr, getRawAndEntities, hasAtLeastOneOfTheseIds, hasOnlyTheseIdsOrNull, - now, readJson} from 'app/gen-server/sqlUtils'; +import { + bitOr, + getRawAndEntities, + hasAtLeastOneOfTheseIds, + hasOnlyTheseIdsOrNull, + now, + readJson +} from 'app/gen-server/sqlUtils'; import {makeId} from 'app/server/lib/idUtils'; import * as log from 'app/server/lib/log'; import {Permit} from 'app/server/lib/Permit'; +import {getScope} from 'app/server/lib/requestUtils'; import {WebHookSecret} from "app/server/lib/Triggers"; -import {StringUnion} from 'app/common/StringUnion'; import {EventEmitter} from 'events'; +import {Request} from "express"; +import { + Brackets, + Connection, + createConnection, + DatabaseType, + EntityManager, + getConnection, + SelectQueryBuilder, + WhereExpression +} from "typeorm"; +import * as uuidv4 from "uuid/v4"; import flatten = require('lodash/flatten'); import pick = require('lodash/pick'); -import {Brackets, Connection, createConnection, DatabaseType, EntityManager, - getConnection, SelectQueryBuilder, WhereExpression} from "typeorm"; -import * as uuidv4 from "uuid/v4"; // Support transactions in Sqlite in async code. This is a monkey patch, affecting // the prototypes of various TypeORM classes. @@ -1038,7 +1064,8 @@ export class HomeDBManager extends EventEmitter { // Calls getDocImpl() and returns the Document from that, caching a fresh DocAuthResult along // the way. Note that we only cache the access level, not Document itself. - public async getDoc(scope: Scope): Promise { + public async getDoc(reqOrScope: Request | Scope): Promise { + const scope = "params" in reqOrScope ? getScope(reqOrScope) : reqOrScope; const key = getDocAuthKeyFromScope(scope); const promise = this.getDocImpl(key); await mapSetOrClear(this._docAuthCache, stringifyDocAuthKey(key), makeDocAuthResult(promise)); @@ -1052,6 +1079,10 @@ export class HomeDBManager extends EventEmitter { return doc; } + public async getRawDocById(docId: string) { + return await this.getDoc({urlId: docId, userId: this.getPreviewerUserId(), showAll: true}); + } + // Returns access info for the given doc and user, caching the results for DOC_AUTH_CACHE_TTL // ms. This helps reduce database load created by liberal authorization requests. public async getDocAuthCached(key: DocAuthKey): Promise { diff --git a/app/server/lib/DocApi.ts b/app/server/lib/DocApi.ts index 302264d1..9a209e89 100644 --- a/app/server/lib/DocApi.ts +++ b/app/server/lib/DocApi.ts @@ -1,55 +1,70 @@ -import { createEmptyActionSummary } from "app/common/ActionSummary"; -import { ApiError } from 'app/common/ApiError'; -import { BrowserSettings } from "app/common/BrowserSettings"; -import { - BulkColValues, ColValues, fromTableDataAction, TableColValues, TableRecordValue, -} from 'app/common/DocActions'; +import {createEmptyActionSummary} from "app/common/ActionSummary"; +import {ApiError} from 'app/common/ApiError'; +import {BrowserSettings} from "app/common/BrowserSettings"; +import {BulkColValues, ColValues, fromTableDataAction, TableColValues, TableRecordValue} from 'app/common/DocActions'; import {isRaisedException} from "app/common/gristTypes"; -import { isAffirmative } from "app/common/gutil"; -import { SortFunc } from 'app/common/SortFunc'; -import { DocReplacementOptions, DocState, DocStateComparison, DocStates, NEW_DOCUMENT_CODE} from 'app/common/UserAPI'; +import {isAffirmative} from "app/common/gutil"; +import {SortFunc} from 'app/common/SortFunc'; +import {Sort} from 'app/common/SortSpec'; +import {DocReplacementOptions, DocState, DocStateComparison, DocStates, NEW_DOCUMENT_CODE} from 'app/common/UserAPI'; +import {HomeDBManager, makeDocAuthResult} from 'app/gen-server/lib/HomeDBManager'; +import * as Types from "app/plugin/DocApiTypes"; +import DocApiTypesTI from "app/plugin/DocApiTypes-ti"; import GristDataTI from 'app/plugin/GristData-ti'; -import { HomeDBManager, makeDocAuthResult } from 'app/gen-server/lib/HomeDBManager'; -import { OpOptions } from "app/plugin/TableOperations"; -import { handleSandboxErrorOnPlatform, TableOperationsImpl, - TableOperationsPlatform } from 'app/plugin/TableOperationsImpl'; -import { concatenateSummaries, summarizeAction } from "app/server/lib/ActionSummary"; -import { ActiveDoc, tableIdToRef } from "app/server/lib/ActiveDoc"; -import { assertAccess, getOrSetDocAuth, getTransitiveHeaders, getUserId, isAnonymousUser, - RequestWithLogin } from 'app/server/lib/Authorizer'; -import { DocManager } from "app/server/lib/DocManager"; -import { docSessionFromRequest, makeExceptionalDocSession, OptDocSession } from "app/server/lib/DocSession"; -import { DocWorker } from "app/server/lib/DocWorker"; -import { IDocWorkerMap } from "app/server/lib/DocWorkerMap"; -import { parseExportParameters } from "app/server/lib/Export"; -import { downloadCSV, DownloadCSVOptions } from "app/server/lib/ExportCSV"; -import { downloadXLSX, DownloadXLSXOptions } from "app/server/lib/ExportXLSX"; -import { expressWrap } from 'app/server/lib/expressWrap'; -import { filterDocumentInPlace } from "app/server/lib/filterUtils"; -import { googleAuthTokenMiddleware } from "app/server/lib/GoogleAuth"; -import { exportToDrive } from "app/server/lib/GoogleExport"; -import { GristServer } from 'app/server/lib/GristServer'; -import { HashUtil } from 'app/server/lib/HashUtil'; -import { makeForkIds } from "app/server/lib/idUtils"; +import {OpOptions} from "app/plugin/TableOperations"; import { - getDocId, getDocScope, integerParam, isParameterOn, optStringParam, - sendOkReply, sendReply, stringParam } from 'app/server/lib/requestUtils'; + handleSandboxErrorOnPlatform, + TableOperationsImpl, + TableOperationsPlatform +} from 'app/plugin/TableOperationsImpl'; +import {concatenateSummaries, summarizeAction} from "app/server/lib/ActionSummary"; +import {ActiveDoc, tableIdToRef} from "app/server/lib/ActiveDoc"; +import { + assertAccess, + getOrSetDocAuth, + getTransitiveHeaders, + getUserId, + isAnonymousUser, + RequestWithLogin +} from 'app/server/lib/Authorizer'; +import {DocManager} from "app/server/lib/DocManager"; +import {docSessionFromRequest, makeExceptionalDocSession, OptDocSession} from "app/server/lib/DocSession"; +import {DocWorker} from "app/server/lib/DocWorker"; +import {IDocWorkerMap} from "app/server/lib/DocWorkerMap"; +import {parseExportParameters} from "app/server/lib/Export"; +import {downloadCSV, DownloadCSVOptions} from "app/server/lib/ExportCSV"; +import {downloadXLSX, DownloadXLSXOptions} from "app/server/lib/ExportXLSX"; +import {expressWrap} from 'app/server/lib/expressWrap'; +import {filterDocumentInPlace} from "app/server/lib/filterUtils"; +import {googleAuthTokenMiddleware} from "app/server/lib/GoogleAuth"; +import {exportToDrive} from "app/server/lib/GoogleExport"; +import {GristServer} from 'app/server/lib/GristServer'; +import {HashUtil} from 'app/server/lib/HashUtil'; +import {makeForkIds} from "app/server/lib/idUtils"; +import { + getDocId, + getDocScope, + getScope, + integerParam, + isParameterOn, + optStringParam, + sendOkReply, + sendReply, + stringParam +} from 'app/server/lib/requestUtils'; +import {ServerColumnGetters} from 'app/server/lib/ServerColumnGetters'; import {localeFromRequest} from "app/server/lib/ServerLocale"; import {allowedEventTypes, isUrlAllowed, WebhookAction, WebHookSecret} from "app/server/lib/Triggers"; -import { handleOptionalUpload, handleUpload } from "app/server/lib/uploads"; -import DocApiTypesTI from "app/plugin/DocApiTypes-ti"; -import * as Types from "app/plugin/DocApiTypes"; +import {handleOptionalUpload, handleUpload} from "app/server/lib/uploads"; import * as contentDisposition from 'content-disposition'; -import { Application, NextFunction, Request, RequestHandler, Response } from "express"; +import {Application, NextFunction, Request, RequestHandler, Response} from "express"; import * as _ from "lodash"; import * as LRUCache from 'lru-cache'; import fetch from 'node-fetch'; import * as path from 'path'; -import * as uuidv4 from "uuid/v4"; import * as t from "ts-interface-checker"; -import { Checker } from "ts-interface-checker"; -import { ServerColumnGetters } from 'app/server/lib/ServerColumnGetters'; -import { Sort } from 'app/common/SortSpec'; +import {Checker} from "ts-interface-checker"; +import * as uuidv4 from "uuid/v4"; // Cap on the number of requests that can be outstanding on a single document via the // rest doc api. When this limit is exceeded, incoming requests receive an immediate @@ -631,8 +646,7 @@ export class DocWorkerApi { this._app.get('/api/docs/:docId/download/csv', canView, withDoc(async (activeDoc, req, res) => { // Query DB for doc metadata to get the doc title. - const {name: docTitle} = - await this._dbManager.getDoc({userId: getUserId(req), org: req.org, urlId: getDocId(req)}); + const {name: docTitle} = await this._dbManager.getDoc(req); const params = parseExportParameters(req); const filename = docTitle + (params.tableId === docTitle ? '' : '-' + params.tableId); @@ -647,8 +661,7 @@ export class DocWorkerApi { this._app.get('/api/docs/:docId/download/xlsx', canView, withDoc(async (activeDoc, req, res) => { // Query DB for doc metadata to get the doc title (to use as the filename). - const {name: filename} = - await this._dbManager.getDoc({userId: getUserId(req), org: req.org, urlId: getDocId(req)}); + const {name: filename} = await this._dbManager.getDoc(req); const options: DownloadXLSXOptions = {filename}; @@ -707,9 +720,7 @@ export class DocWorkerApi { * request. */ private async _confirmDocIdForRead(req: Request, urlId: string): Promise { - const userId = getUserId(req); - const org = (req as RequestWithLogin).org; - const docAuth = await makeDocAuthResult(this._dbManager.getDoc({urlId, userId, org})); + const docAuth = await makeDocAuthResult(this._dbManager.getDoc({...getScope(req), urlId})); if (docAuth.error) { throw docAuth.error; } assertAccess('viewers', docAuth); return docAuth.docId!; diff --git a/app/server/lib/DocManager.ts b/app/server/lib/DocManager.ts index c462208f..21caa0fa 100644 --- a/app/server/lib/DocManager.ts +++ b/app/server/lib/DocManager.ts @@ -1,4 +1,6 @@ import * as pidusage from '@gristlabs/pidusage'; +import {Document} from 'app/gen-server/entity/Document'; +import {getScope} from 'app/server/lib/requestUtils'; import * as bluebird from 'bluebird'; import {EventEmitter} from 'events'; import * as path from 'path'; @@ -464,23 +466,50 @@ export class DocManager extends EventEmitter { return activeDoc; } - private async _createActiveDoc(docSession: OptDocSession, docName: string, safeMode?: boolean) { - // Get URL for document for use with SELF_HYPERLINK(). + private async _getDoc(docSession: OptDocSession, docName: string) { const cachedDoc = getDocSessionCachedDoc(docSession); - let docUrl: string|undefined; + if (cachedDoc) { + return cachedDoc; + } + + let db: HomeDBManager; try { - if (cachedDoc) { - docUrl = await this.gristServer.getResourceUrl(cachedDoc); - } else { - docUrl = await this.gristServer.getDocUrl(docName); + // For the sake of existing tests, get the db from gristServer where it may not exist and we should give up, + // rather than using this._homeDbManager which may exist and then it turns out the document itself doesn't. + db = this.gristServer.getHomeDBManager(); + } catch (e) { + if (e.message === "no db") { + return; } + throw e; + } + + if (docSession.req) { + const scope = getScope(docSession.req); + if (scope.urlId) { + return db.getDoc(scope); + } + } + + return await db.getRawDocById(docName); + } + + private async _getDocUrl(doc: Document) { + try { + return await this.gristServer.getResourceUrl(doc); } catch (e) { // If there is no home url, we cannot construct links. Accept this, for the benefit // of legacy tests. - if (!String(e).match(/need APP_HOME_URL/)) { + if (e.message !== "need APP_HOME_URL") { throw e; } } + } + + private async _createActiveDoc(docSession: OptDocSession, docName: string, safeMode?: boolean) { + const doc = await this._getDoc(docSession, docName); + // Get URL for document for use with SELF_HYPERLINK(). + const docUrl = doc && await this._getDocUrl(doc); return this.gristServer.create.ActiveDoc(this, docName, {docUrl, safeMode}); } diff --git a/app/server/lib/DocWorker.ts b/app/server/lib/DocWorker.ts index b1bc4408..a7e78274 100644 --- a/app/server/lib/DocWorker.ts +++ b/app/server/lib/DocWorker.ts @@ -4,14 +4,14 @@ */ import {HomeDBManager} from 'app/gen-server/lib/HomeDBManager'; import {ActionHistoryImpl} from 'app/server/lib/ActionHistoryImpl'; -import {assertAccess, getOrSetDocAuth, getUserId, RequestWithLogin} from 'app/server/lib/Authorizer'; +import {assertAccess, getOrSetDocAuth, RequestWithLogin} from 'app/server/lib/Authorizer'; import {Client} from 'app/server/lib/Client'; import * as Comm from 'app/server/lib/Comm'; import {DocSession, docSessionFromRequest} from 'app/server/lib/DocSession'; import {filterDocumentInPlace} from 'app/server/lib/filterUtils'; import {IDocStorageManager} from 'app/server/lib/IDocStorageManager'; import * as log from 'app/server/lib/log'; -import {integerParam, optStringParam, stringParam} from 'app/server/lib/requestUtils'; +import {getDocId, integerParam, optStringParam, stringParam} from 'app/server/lib/requestUtils'; import {OpenMode, quoteIdent, SQLiteDB} from 'app/server/lib/SQLiteDB'; import * as contentDisposition from 'content-disposition'; import * as express from 'express'; @@ -58,11 +58,10 @@ export class DocWorker { public async downloadDoc(req: express.Request, res: express.Response, storageManager: IDocStorageManager): Promise { const mreq = req as RequestWithLogin; - if (!mreq.docAuth || !mreq.docAuth.docId) { throw new Error('Cannot find document'); } - const docId = mreq.docAuth.docId; + const docId = getDocId(mreq); // Query DB for doc metadata to get the doc title. - const doc = await this._dbManager.getDoc({userId: getUserId(req), org: mreq.org, urlId: docId}); + const doc = await this._dbManager.getDoc(req); const docTitle = doc.name; // Get a copy of document for downloading. diff --git a/app/server/lib/FlexServer.ts b/app/server/lib/FlexServer.ts index daed0925..1abd13d2 100644 --- a/app/server/lib/FlexServer.ts +++ b/app/server/lib/FlexServer.ts @@ -1223,20 +1223,6 @@ export class FlexServer implements GristServer { return makeGristConfig(this.getDefaultHomeUrl(), {}, this._defaultBaseDomain); } - /** - * Get a url for a document. The id provided should be a genuine docId, since we query - * the db for document details without including organization disambiguation. - */ - public async getDocUrl(docId: string): Promise { - if (!this._dbManager) { throw new Error('database missing'); } - const doc = await this._dbManager.getDoc({ - userId: this._dbManager.getPreviewerUserId(), - urlId: docId, - showAll: true - }); - return this.getResourceUrl(doc); - } - /** * Get a url for a team site. */ diff --git a/app/server/lib/GristServer.ts b/app/server/lib/GristServer.ts index ef7389f5..e2a15c77 100644 --- a/app/server/lib/GristServer.ts +++ b/app/server/lib/GristServer.ts @@ -25,7 +25,6 @@ export interface GristServer { getHomeUrl(req: express.Request, relPath?: string): string; getHomeUrlByDocId(docId: string, relPath?: string): Promise; getOwnUrl(): string; - getDocUrl(docId: string): Promise; getOrgUrl(orgKey: string|number): Promise; getMergedOrgUrl(req: RequestWithLogin, pathname?: string): string; getResourceUrl(resource: Organization|Workspace|Document): Promise; diff --git a/app/server/lib/requestUtils.ts b/app/server/lib/requestUtils.ts index 1f76b503..bae8aa88 100644 --- a/app/server/lib/requestUtils.ts +++ b/app/server/lib/requestUtils.ts @@ -150,10 +150,10 @@ export function getDocScope(req: Request): DocScope { * is limited to docs/workspaces that have been removed. */ export function getScope(req: Request): Scope { - const urlId = req.params.did || req.params.docId; + const {specialPermit, docAuth} = (req as RequestWithLogin); + const urlId = req.params.did || req.params.docId || docAuth?.docId || undefined; const userId = getUserId(req); const org = (req as RequestWithOrg).org; - const {specialPermit} = (req as RequestWithLogin); const includeSupport = isParameterOn(req.query.includeSupport); const showRemoved = isParameterOn(req.query.showRemoved); return {urlId, userId, org, includeSupport, showRemoved, specialPermit};