From e5c24eb5ea1cbf1a7a65ce2eeb01372382618e06 Mon Sep 17 00:00:00 2001 From: Paul Fitzpatrick Date: Fri, 11 Dec 2020 14:22:35 -0500 Subject: [PATCH] (core) revamp user attribute handling Summary: This changes how user attributes are loaded. They are now loaded directly from sqlite, with per-session caching. Optimizations considered but not addressed yet are (1) adding indexes to user attribute tables and (2) swapping in a thinner sqlite wrapper. The main benefit of this diff is that changes to user attribute tables now work. Clients whose user attributes are not changed see no effect; clients whose user attributes have changed have their document reloaded. For the purposes of testing, the diff includes a tweak to GristWSConnection to be "sticky" to a specific user when reloading (and support machinery on the server side to honor that). Until now, if a GristWSConnection reloads, it uses whatever the current default user is in the cookie-based session, which can change. This was complicating a test where multiple users were accessing the same document via different clients with occasional document reloads. Code for updating when schema or rule changes happen is moved around but not improved in any meaningful way in this diff. Test Plan: existing tests pass; extended test Reviewers: dsagal Reviewed By: dsagal Differential Revision: https://phab.getgrist.com/D2685 --- app/client/components/GristWSConnection.ts | 9 + app/gen-server/ApiServer.ts | 2 +- app/server/lib/ActiveDoc.ts | 38 ++-- app/server/lib/Authorizer.ts | 3 +- app/server/lib/BrowserSession.ts | 14 +- app/server/lib/Comm.js | 8 +- app/server/lib/DocApi.ts | 2 +- app/server/lib/GranularAccess.ts | 221 +++++++++++---------- app/server/lib/Sessions.ts | 19 +- 9 files changed, 173 insertions(+), 143 deletions(-) diff --git a/app/client/components/GristWSConnection.ts b/app/client/components/GristWSConnection.ts index cb01cfd4..df443389 100644 --- a/app/client/components/GristWSConnection.ts +++ b/app/client/components/GristWSConnection.ts @@ -50,6 +50,10 @@ export interface GristWSSettings { // Get an id associated with the client, null for "no id set yet". getClientId(assignmentId: string|null): string|null; + // Get selector for user, so if cookie auth allows multiple the correct one will be picked. + // Selector is currently just the email address. + getUserSelector(): string; + // Update the id associated with the client. Future calls to getClientId should return this. updateClientId(assignmentId: string|null, clentId: string): void; @@ -74,6 +78,10 @@ export class GristWSSettingsBrowser implements GristWSSettings { public getClientId(assignmentId: string|null) { return window.sessionStorage.getItem(`clientId_${assignmentId}`) || null; } + public getUserSelector(): string { + // TODO: find/create a more official way to get the user. + return (window as any).gristDocPageModel?.appModel.currentUser?.email || ''; + } public updateClientId(assignmentId: string|null, id: string) { window.sessionStorage.setItem(`clientId_${assignmentId}`, id); } @@ -335,6 +343,7 @@ export class GristWSConnection extends Disposable { url.searchParams.append('counter', this._clientCounter); url.searchParams.append('newClient', String(isReconnecting ? 0 : 1)); url.searchParams.append('browserSettings', JSON.stringify({timezone})); + url.searchParams.append('user', this._settings.getUserSelector()); return url.href; } diff --git a/app/gen-server/ApiServer.ts b/app/gen-server/ApiServer.ts index dd1442e7..a8d982cf 100644 --- a/app/gen-server/ApiServer.ts +++ b/app/gen-server/ApiServer.ts @@ -440,7 +440,7 @@ export class ApiServer { const userId = getUserId(mreq); const fullUser = await this._dbManager.getFullUser(userId); const domain = getOrgFromRequest(mreq); - const sessionUser = getSessionUser(mreq.session, domain || ''); + const sessionUser = getSessionUser(mreq.session, domain || '', fullUser.email); const loginMethod = sessionUser && sessionUser.profile ? sessionUser.profile.loginMethod : undefined; return {...fullUser, loginMethod}; } diff --git a/app/server/lib/ActiveDoc.ts b/app/server/lib/ActiveDoc.ts index 7f0e5e5e..e78ae5e3 100644 --- a/app/server/lib/ActiveDoc.ts +++ b/app/server/lib/ActiveDoc.ts @@ -213,7 +213,15 @@ export class ActiveDoc extends EventEmitter { public async getRecentActions(docSession: OptDocSession, summarize: boolean): Promise { const groups = await this._actionHistory.getRecentActionGroups(MAX_RECENT_ACTIONS, {client: docSession.client, summarize}); - return groups.filter(actionGroup => this._granularAccess.allowActionGroup(docSession, actionGroup)); + const permittedGroups: ActionGroup[] = []; + // Process groups serially since the work is synchronous except for some + // possible db accesses that will be serialized in any case. + for (const group of groups) { + if (await this._granularAccess.allowActionGroup(docSession, group)) { + permittedGroups.push(group); + } + } + return permittedGroups; } /** expose action history for tests */ @@ -556,9 +564,9 @@ export class ActiveDoc extends EventEmitter { } // Check if user has rights to download this doc. - public canDownload(docSession: OptDocSession) { + public async canDownload(docSession: OptDocSession) { return this._granularAccess.hasViewAccess(docSession) && - this._granularAccess.canReadEverything(docSession); + await this._granularAccess.canReadEverything(docSession); } /** @@ -585,7 +593,7 @@ export class ActiveDoc extends EventEmitter { this._inactivityTimer.ping(); // The doc is in active use; ping it to stay open longer. // If user does not have rights to access what this query is asking for, fail. - const tableAccess = this._granularAccess.getTableAccess(docSession, query.tableId); + const tableAccess = await this._granularAccess.getTableAccess(docSession, query.tableId); if (tableAccess.read === 'deny') { throw new Error('not authorized to read table'); } @@ -619,7 +627,7 @@ export class ActiveDoc extends EventEmitter { // If row-level access is being controlled, filter the data appropriately. // Likewise if column-level access is being controlled. if (tableAccess.read !== 'allow') { - this._granularAccess.filterData(docSession, data!); + await this._granularAccess.filterData(docSession, data!); } this.logInfo(docSession, "fetchQuery -> %d rows, cols: %s", data![2].length, Object.keys(data![3]).join(", ")); @@ -676,7 +684,7 @@ export class ActiveDoc extends EventEmitter { optTableId?: string): Promise { // This could leak information about private tables, so if user cannot read entire // document, do nothing. - if (!this._granularAccess.canReadEverything(docSession)) { return []; } + if (!await this._granularAccess.canReadEverything(docSession)) { return []; } this.logInfo(docSession, "findColFromValues(%s, %s, %s)", docSession, values, n); await this.waitForInitialization(); return this._pyCall('find_col_from_values', values, n, optTableId); @@ -768,16 +776,10 @@ export class ActiveDoc extends EventEmitter { localActionBundle.stored.forEach(da => docData.receiveAction(da[1])); localActionBundle.calc.forEach(da => docData.receiveAction(da[1])); const docActions = getEnvContent(localActionBundle.stored); - // TODO: call this update less indiscriminately! - // Update ACLs only when rules are touched. A suggest for later is for GranularAccess to - // listen to docData's changes that affect relevant table, and toggle a dirty flag. The - // update() can then be called whenever ACLs are needed and are dirty. - if (localActionBundle.stored.some(da => (da[1][1] === '_grist_ACLRules'))) { - await this._granularAccess.update(); - } if (docActions.some(docAction => this._onDemandActions.isSchemaAction(docAction))) { const indexes = this._onDemandActions.getDesiredIndexes(); await this.docStorage.updateIndexes(indexes); + // TODO: should probably add indexes for user attribute tables. } } @@ -829,7 +831,7 @@ export class ActiveDoc extends EventEmitter { public async autocomplete(docSession: DocSession, txt: string, tableId: string): Promise { // Autocompletion can leak names of tables and columns. - if (!this._granularAccess.canReadEverything(docSession)) { return []; } + if (!await this._granularAccess.canReadEverything(docSession)) { return []; } await this.waitForInitialization(); return this._pyCall('autocomplete', txt, tableId); } @@ -874,7 +876,7 @@ export class ActiveDoc extends EventEmitter { * ID for the fork. TODO: reconcile the two ways there are now of preparing a fork. */ public async fork(docSession: DocSession): Promise { - if (!this._granularAccess.canReadEverything(docSession)) { + if (!await this._granularAccess.canReadEverything(docSession)) { throw new Error('cannot confirm authority to copy document'); } const userId = docSession.client.getCachedUserId(); @@ -1085,7 +1087,7 @@ export class ActiveDoc extends EventEmitter { protected async _applyUserActions(docSession: OptDocSession, actions: UserAction[], options: ApplyUAOptions = {}): Promise { - if (!this._granularAccess.canMaybeApplyUserActions(docSession, actions)) { + if (!await this._granularAccess.canMaybeApplyUserActions(docSession, actions)) { throw new Error('cannot perform a requested action'); } @@ -1301,9 +1303,9 @@ export class ActiveDoc extends EventEmitter { actionGroup: ActionGroup, docActions: DocAction[] }) { - if (this._granularAccess.canReadEverything(docSession)) { return message; } + if (await this._granularAccess.canReadEverything(docSession)) { return message; } const result = { - actionGroup: this._granularAccess.filterActionGroup(docSession, message.actionGroup), + actionGroup: await this._granularAccess.filterActionGroup(docSession, message.actionGroup), docActions: await this._granularAccess.filterOutgoingDocActions(docSession, message.docActions), }; if (result.docActions.length === 0) { return null; } diff --git a/app/server/lib/Authorizer.ts b/app/server/lib/Authorizer.ts index 068654fb..744c36c5 100644 --- a/app/server/lib/Authorizer.ts +++ b/app/server/lib/Authorizer.ts @@ -189,7 +189,8 @@ export async function addRequestUser(dbManager: HomeDBManager, permitStore: IPer } // See if we have a profile linked with the active organization already. - let sessionUser: SessionUserObj|null = getSessionUser(session, mreq.org); + // TODO: implement userSelector for rest API, to allow "sticky" user selection on pages. + let sessionUser: SessionUserObj|null = getSessionUser(session, mreq.org, ''); if (!sessionUser) { // No profile linked yet, so let's elect one. diff --git a/app/server/lib/BrowserSession.ts b/app/server/lib/BrowserSession.ts index e2e038ee..1f636857 100644 --- a/app/server/lib/BrowserSession.ts +++ b/app/server/lib/BrowserSession.ts @@ -58,10 +58,17 @@ export function getSessionProfiles(session: SessionObj): UserProfile[] { * found specific to that org. * */ -export function getSessionUser(session: SessionObj, org: string): SessionUserObj|null { +export function getSessionUser(session: SessionObj, org: string, + userSelector: string): SessionUserObj|null { if (!session.users) { return null; } if (!session.users.length) { return null; } + if (userSelector) { + for (const user of session.users) { + if (user.profile?.email.toLowerCase() === userSelector.toLowerCase()) { return user; } + } + } + if (session.orgToUser && session.orgToUser[org] !== undefined && session.users.length > session.orgToUser[org]) { return session.users[session.orgToUser[org]] || null; @@ -109,7 +116,8 @@ export class ScopedSession { */ constructor(private _sessionId: string, private _sessionStore: SessionStore, - private _org: string) { + private _org: string, + private _userSelector: string) { // Assume we need to skip cache in a hosted environment. GRIST_HOST is always set there. // TODO: find a cleaner way to configure this flag. this._live = Boolean(process.env.GRIST_HOST || process.env.GRIST_HOSTED); @@ -122,7 +130,7 @@ export class ScopedSession { */ public async getScopedSession(prev?: SessionObj): Promise { const session = prev || await this._getSession(); - return getSessionUser(session, this._org) || {}; + return getSessionUser(session, this._org, this._userSelector) || {}; } /** diff --git a/app/server/lib/Comm.js b/app/server/lib/Comm.js index 38dfb871..c049bc55 100644 --- a/app/server/lib/Comm.js +++ b/app/server/lib/Comm.js @@ -122,10 +122,11 @@ Comm.prototype.getClient = function(clientId) { * Returns a LoginSession object with the given session id from the list of sessions, * or adds a new one and returns that. */ -Comm.prototype.getOrCreateSession = function(sid, req) { +Comm.prototype.getOrCreateSession = function(sid, req, userSelector) { // LoginSessions are specific to a session id / org combination. const org = req.org || ""; - return this.sessions.getOrCreateLoginSession(sid, org, this, this._instanceManager); + return this.sessions.getOrCreateLoginSession(sid, org, this, this._instanceManager, + userSelector); }; @@ -189,6 +190,7 @@ Comm.prototype._onWebSocketConnection = async function(websocket, req) { var browserSettings = urlObj.query.browserSettings ? JSON.parse(urlObj.query.browserSettings) : {}; var newClient = (parseInt(urlObj.query.newClient, 10) === 1); const counter = urlObj.query.counter; + const userSelector = urlObj.query.user || ''; // Associate an ID with each websocket, reusing the supplied one if it's valid. var client; @@ -229,7 +231,7 @@ Comm.prototype._onWebSocketConnection = async function(websocket, req) { // Add a Session object to the client. log.info(`Comm ${client}: using session ${sessionId}`); - const loginSession = this.getOrCreateSession(sessionId, req); + const loginSession = this.getOrCreateSession(sessionId, req, userSelector); client.setSession(loginSession); // Delegate message handling to the client diff --git a/app/server/lib/DocApi.ts b/app/server/lib/DocApi.ts index b3d8d238..24a68b77 100644 --- a/app/server/lib/DocApi.ts +++ b/app/server/lib/DocApi.ts @@ -198,7 +198,7 @@ export class DocWorkerApi { // If the user is not an owner, we load the document as an ActiveDoc, and then // check if the user has download permissions. const activeDoc = await this._getActiveDoc(req); - if (!activeDoc.canDownload(docSessionFromRequest(req))) { + if (!await activeDoc.canDownload(docSessionFromRequest(req))) { throw new Error('not authorized to download this document'); } return this._docWorker.downloadDoc(req, res, this._docManager.storageManager); diff --git a/app/server/lib/GranularAccess.ts b/app/server/lib/GranularAccess.ts index 0673c8e0..f19cc4b3 100644 --- a/app/server/lib/GranularAccess.ts +++ b/app/server/lib/GranularAccess.ts @@ -7,12 +7,12 @@ import { createEmptyActionSummary } from 'app/common/ActionSummary'; import { Query } from 'app/common/ActiveDocAPI'; import { AsyncCreate } from 'app/common/AsyncCreate'; import { AddRecord, BulkAddRecord, BulkColValues, BulkRemoveRecord, BulkUpdateRecord, CellValue, - ColValues, DocAction, getTableId, isSchemaAction, RemoveRecord, ReplaceTableData, UpdateRecord } from 'app/common/DocActions'; + ColValues, DocAction, getTableId, isSchemaAction, RemoveRecord, ReplaceTableData, UpdateRecord } from 'app/common/DocActions'; import { TableDataAction, UserAction } from 'app/common/DocActions'; import { DocData } from 'app/common/DocData'; import { ErrorWithCode } from 'app/common/ErrorWithCode'; import { AclMatchInput, InfoView } from 'app/common/GranularAccessClause'; -import { RuleSet, UserAttributeRule, UserInfo } from 'app/common/GranularAccessClause'; +import { RuleSet, UserInfo } from 'app/common/GranularAccessClause'; import { getSetMapValue, isObject } from 'app/common/gutil'; import { canView } from 'app/common/roles'; import { compileAclFormula } from 'app/server/lib/ACLFormula'; @@ -96,14 +96,11 @@ export class GranularAccess { // The collection of all rules, with helpful accessors. private _ruleCollection = new ACLRuleCollection(); - // Cache any tables that we need to look-up for access control decisions. - // This is an unoptimized implementation that is adequate if the tables - // are not large and don't change all that often. - private _characteristicTables = new Map(); - // Cache of PermissionInfo associated with the given docSession. It's a WeakMap, so should allow // both to be garbage-collected once docSession is no longer in use. - private _permissionInfoMap = new WeakMap(); + private _permissionInfoMap = new WeakMap>(); + private _userAttributesMap = new WeakMap(); + private _prevUserAttributesMap: WeakMap|undefined; // When broadcasting a sequence of DocAction[]s, this contains the state of // affected rows for the relevant table before and after each DocAction. It @@ -121,10 +118,9 @@ export class GranularAccess { public async update() { await this._ruleCollection.update(this._docData, {log, compile: compileAclFormula}); - // Also clear the per-docSession cache of rule evaluations. + // Also clear the per-docSession cache of rule evaluations and user attributes. this._permissionInfoMap = new WeakMap(); - // TODO: optimize this. - await this._updateCharacteristicTables(); + this._userAttributesMap = new WeakMap(); } /** @@ -137,8 +133,8 @@ export class GranularAccess { /** * Check whether user has any access to table. */ - public hasTableAccess(docSession: OptDocSession, tableId: string) { - const pset = this.getTableAccess(docSession, tableId); + public async hasTableAccess(docSession: OptDocSession, tableId: string) { + const pset = await this.getTableAccess(docSession, tableId); return pset.read !== 'deny'; } @@ -166,6 +162,28 @@ export class GranularAccess { */ public async appliedActions(docActions: DocAction[], undo: DocAction[]) { this._applied = true; + // If there is a rule change, redo from scratch for now. + // TODO: this is placeholder code. Should deal with connected clients. + if (docActions.some(docAction => getTableId(docAction) === '_grist_ACLRules' || getTableId(docAction) === '_grist_Resources')) { + await this.update(); + return; + } + if (!this._ruleCollection.haveRules()) { return; } + // If there is a schema change, redo from scratch for now. + // TODO: this is placeholder code. Should deal with connected clients. + if (docActions.some(docAction => isSchemaAction(docAction))) { + await this.update(); + return; + } + // Check if a table that affects user attributes has changed. If so, put current + // attributes aside for later comparison, and clear caches. + const attrs = new Set([...this._ruleCollection.getUserAttributeRules().values()].map(r => r.tableId)); + if (docActions.some(docAction => attrs.has(getTableId(docAction)))) { + this._prevUserAttributesMap = this._userAttributesMap; + this._permissionInfoMap = new WeakMap(); + this._userAttributesMap = new WeakMap(); + return; + } } /** @@ -176,12 +194,14 @@ export class GranularAccess { this._applied = false; if (this._rowSnapshots) { this._rowSnapshots.clear(); } this._rowSnapshots = null; + this._prevUserAttributesMap = undefined; } /** * Filter DocActions to be sent to a client. */ public async filterOutgoingDocActions(docSession: OptDocSession, docActions: DocAction[]): Promise { + await this._checkUserAttributes(docSession); const actions = await Promise.all( docActions.map((action, idx) => this._pruneOutgoingDocAction(docSession, action, idx))); return ([] as DocAction[]).concat(...actions); @@ -190,9 +210,8 @@ export class GranularAccess { /** * Filter an ActionGroup to be sent to a client. */ - public filterActionGroup(docSession: OptDocSession, actionGroup: ActionGroup): ActionGroup { - // TODO This seems a mistake -- should this check be negated? - if (!this.allowActionGroup(docSession, actionGroup)) { return actionGroup; } + public async filterActionGroup(docSession: OptDocSession, actionGroup: ActionGroup): Promise { + if (await this.allowActionGroup(docSession, actionGroup)) { return actionGroup; } // For now, if there's any nuance at all, suppress the summary and description. // TODO: create an empty action summary, to be sure not to leak anything important. const result: ActionGroup = { ...actionGroup }; @@ -205,7 +224,7 @@ export class GranularAccess { * Check whether an ActionGroup can be sent to the client. TODO: in future, we'll want * to filter acceptible parts of ActionGroup, rather than denying entirely. */ - public allowActionGroup(docSession: OptDocSession, actionGroup: ActionGroup): boolean { + public async allowActionGroup(docSession: OptDocSession, actionGroup: ActionGroup): Promise { return this.canReadEverything(docSession); } @@ -215,14 +234,17 @@ export class GranularAccess { * TODO: not smart about intermediate states, if there is a table or column rename it will * have trouble, and might forbid something that should be allowed. */ - public canMaybeApplyUserActions(docSession: OptDocSession, actions: UserAction[]): boolean { - return actions.every(action => this.canMaybeApplyUserAction(docSession, action)); + public async canMaybeApplyUserActions(docSession: OptDocSession, actions: UserAction[]): Promise { + for (const action of actions) { + if (!await this.canMaybeApplyUserAction(docSession, action)) { return false; } + } + return true; } /** * Check if user can apply a given action to the document. */ - public canMaybeApplyUserAction(docSession: OptDocSession, a: UserAction|DocAction): boolean { + public async canMaybeApplyUserAction(docSession: OptDocSession, a: UserAction|DocAction): Promise { const name = a[0] as string; if (OK_ACTIONS.has(name)) { return true; } if (SPECIAL_ACTIONS.has(name)) { @@ -242,7 +264,7 @@ export class GranularAccess { if (tableId.startsWith('_grist_')) { return !this.hasNuancedAccess(docSession); } - const tableAccess = this.getTableAccess(docSession, tableId); + const tableAccess = await this.getTableAccess(docSession, tableId); const accessFn = getAccessForActionType(a); const access = accessFn(tableAccess); // if access is mixed, leave this to be checked in detail later. @@ -264,8 +286,8 @@ export class GranularAccess { /** * Check whether user can read everything in document. */ - public canReadEverything(docSession: OptDocSession): boolean { - const permInfo = this._getAccess(docSession); + public async canReadEverything(docSession: OptDocSession): Promise { + const permInfo = await this._getAccess(docSession); return permInfo.getFullAccess().read === 'allow'; } @@ -304,10 +326,10 @@ export class GranularAccess { * references to them. * */ - public filterMetaTables(docSession: OptDocSession, - tables: {[key: string]: TableDataAction}): {[key: string]: TableDataAction} { + public async filterMetaTables(docSession: OptDocSession, + tables: {[key: string]: TableDataAction}): Promise<{[key: string]: TableDataAction}> { // If user has right to read everything, return immediately. - if (this.canReadEverything(docSession)) { return tables; } + if (await this.canReadEverything(docSession)) { return tables; } // If we are going to modify metadata, make a copy. tables = JSON.parse(JSON.stringify(tables)); // Collect a list of all tables (by tableRef) to which the user has no access. @@ -315,7 +337,7 @@ export class GranularAccess { // Collect a list of censored columns (by " "). const columnCode = (tableRef: number, colId: string) => `${tableRef} ${colId}`; const censoredColumnCodes: Set = new Set(); - const permInfo = this._getAccess(docSession); + const permInfo = await this._getAccess(docSession); for (const tableId of this._ruleCollection.getAllTableIds()) { const tableAccess = permInfo.getTableAccess(tableId); let tableRef: number|undefined = 0; @@ -399,19 +421,19 @@ export class GranularAccess { * Distill the clauses for the given session and table, to figure out the * access level and any row-level access functions needed. */ - public getTableAccess(docSession: OptDocSession, tableId: string): TablePermissionSet { - return this._getAccess(docSession).getTableAccess(tableId); + public async getTableAccess(docSession: OptDocSession, tableId: string): Promise { + return (await this._getAccess(docSession)).getTableAccess(tableId); } /** * Modify table data in place, removing any rows or columns to which access * is not granted. */ - public filterData(docSession: OptDocSession, data: TableDataAction) { - const permInfo = this._getAccess(docSession); + public async filterData(docSession: OptDocSession, data: TableDataAction) { + const permInfo = await this._getAccess(docSession); const tableId = getTableId(data); if (permInfo.getTableAccess(tableId).read === 'mixed') { - this._filterRowsAndCells(docSession, data, data, data, canRead); + await this._filterRowsAndCells(docSession, data, data, data, canRead); } // Filter columns, omitting any to which the user has no access, regardless of rows. @@ -468,8 +490,8 @@ export class GranularAccess { // after this action. We need to know both so that we can infer the state of the // client and send the correct change. const ids = new Set(getRowIdsFromDocAction(a)); - const forbiddenBefores = new Set(this._getForbiddenRows(docSession, rowsBefore, ids)); - const forbiddenAfters = new Set(this._getForbiddenRows(docSession, rowsAfter, ids)); + const forbiddenBefores = new Set(await this._getForbiddenRows(docSession, rowsBefore, ids)); + const forbiddenAfters = new Set(await this._getForbiddenRows(docSession, rowsAfter, ids)); /** * For rows forbidden before and after: just remove them. @@ -533,7 +555,7 @@ export class GranularAccess { // Return the results, also applying any cell-level access control. for (const docAction of revisedDocActions) { - this._filterRowsAndCells(docSession, rowsAfter, rowsAfter, docAction, canRead); + await this._filterRowsAndCells(docSession, rowsAfter, rowsAfter, docAction, canRead); } return revisedDocActions; } @@ -550,14 +572,14 @@ export class GranularAccess { if (!this._rowSnapshots) { throw new Error('Logic error: actions not available'); } const allRowSnapshots = await this._rowSnapshots.get(); const [rowsBefore, rowsAfter] = allRowSnapshots[idx]; - this._filterRowsAndCells(docSession, rowsBefore, rowsAfter, a, accessFn); + await this._filterRowsAndCells(docSession, rowsBefore, rowsAfter, a, accessFn); } /** * Modify action in place, scrubbing any rows and cells to which access is not granted. */ - private _filterRowsAndCells(docSession: OptDocSession, rowsBefore: TableDataAction, rowsAfter: TableDataAction, - docAction: DocAction, accessFn: AccessFn) { + private async _filterRowsAndCells(docSession: OptDocSession, rowsBefore: TableDataAction, rowsAfter: TableDataAction, + docAction: DocAction, accessFn: AccessFn) { if (docAction && isSchemaAction(docAction)) { // TODO should filter out metadata about an unavailable column, probably. return []; @@ -565,7 +587,7 @@ export class GranularAccess { const rec = new RecordView(rowsBefore, undefined); const newRec = new RecordView(rowsAfter, undefined); - const input: AclMatchInput = {user: this._getUser(docSession), rec, newRec}; + const input: AclMatchInput = {user: await this._getUser(docSession), rec, newRec}; const [, tableId, , colValues] = docAction; const rowIds = getRowIdsFromDocAction(docAction); @@ -624,9 +646,9 @@ export class GranularAccess { } // Compute which of the row ids supplied are for rows forbidden for this session. - private _getForbiddenRows(docSession: OptDocSession, data: TableDataAction, ids: Set): number[] { + private async _getForbiddenRows(docSession: OptDocSession, data: TableDataAction, ids: Set): Promise { const rec = new RecordView(data, undefined); - const input: AclMatchInput = {user: this._getUser(docSession), rec}; + const input: AclMatchInput = {user: await this._getUser(docSession), rec}; const [, tableId, rowIds] = data; const toRemove: number[] = []; @@ -684,51 +706,6 @@ export class GranularAccess { } } - /** - * When comparing user characteristics, we lowercase for the sake of email comparison. - * This is a bit weak. - */ - private _normalizeValue(value: CellValue|InfoView): string { - // If we get a record, e.g. `user.office`, interpret it as `user.office.id` (rather than try - // to use stringification of the full record). - if (value && typeof value === 'object' && !Array.isArray(value)) { - value = value.get('id'); - } - return JSON.stringify(value)?.toLowerCase() || ''; - } - - /** - * Load any tables needed for look-ups. - */ - private async _updateCharacteristicTables() { - this._characteristicTables.clear(); - for (const userChar of this._ruleCollection.getUserAttributeRules().values()) { - await this._updateCharacteristicTable(userChar); - } - } - - /** - * Load a table needed for look-up. - */ - private async _updateCharacteristicTable(clause: UserAttributeRule) { - if (this._characteristicTables.get(clause.name)) { - throw new Error(`User attribute ${clause.name} ignored: duplicate name`); - } - const data = await this._fetchQueryFromDB({tableId: clause.tableId, filters: {}}); - const rowNums = new Map(); - const matches = data[3][clause.lookupColId]; - for (let i = 0; i < matches.length; i++) { - rowNums.set(this._normalizeValue(matches[i]), i); - } - const result: CharacteristicTable = { - tableId: clause.tableId, - colId: clause.lookupColId, - rowNums, - data - }; - this._characteristicTables.set(clause.name, result); - } - /** * Get PermissionInfo for the user represented by the given docSession. The returned object * allows evaluating access level as far as possible without considering specific records. @@ -736,20 +713,46 @@ export class GranularAccess { * The result is cached in a WeakMap, and PermissionInfo does its own caching, so multiple calls * to this._getAccess(docSession).someMethod() will reuse already-evaluated results. */ - private _getAccess(docSession: OptDocSession): PermissionInfo { + private async _getAccess(docSession: OptDocSession): Promise { // TODO The intent of caching is to avoid duplicating rule evaluations while processing a // single request. Caching based on docSession is riskier since those persist across requests. - return getSetMapValue(this._permissionInfoMap as Map, docSession, - () => new PermissionInfo(this._ruleCollection, {user: this._getUser(docSession)})); + return getSetMapValue(this._permissionInfoMap as Map>, docSession, + async () => new PermissionInfo(this._ruleCollection, {user: await this._getUser(docSession)})); + } + + private _getUserAttributes(docSession: OptDocSession): UserAttributes { + // TODO Same caching intent and caveat as for _getAccess + return getSetMapValue(this._userAttributesMap as Map, docSession, + () => new UserAttributes()); + } + + /** + * Check whether user attributes have changed. If so, prompt client + * to reload the document, since we aren't sophisticated enough to + * figure out the changes to send. + */ + private async _checkUserAttributes(docSession: OptDocSession) { + if (!this._prevUserAttributesMap) { return; } + const userAttrBefore = this._prevUserAttributesMap.get(docSession); + if (!userAttrBefore) { return; } + await this._getAccess(docSession); // Makes sure user attrs have actually been computed. + const userAttrAfter = this._getUserAttributes(docSession); + for (const [tableId, rec] of Object.entries(userAttrAfter.rows)) { + const prev = userAttrBefore.rows[tableId]; + if (!prev || JSON.stringify(prev.toJSON()) !== JSON.stringify(rec.toJSON())) { + throw new ErrorWithCode('NEED_RELOAD', 'document needs reload, user attributes changed'); + } + } } /** * Construct the UserInfo needed for evaluating rules. This also enriches the user with values * created by user-attribute rules. */ - private _getUser(docSession: OptDocSession): UserInfo { + private async _getUser(docSession: OptDocSession): Promise { const access = getDocSessionAccess(docSession); const fullUser = getDocSessionUser(docSession); + const attrs = this._getUserAttributes(docSession); const user: UserInfo = {}; user.Access = access; user.UserID = fullUser?.id || null; @@ -767,16 +770,22 @@ export class GranularAccess { log.warn(`User attribute ${clause.name} ignored; conflicts with an existing one`); continue; } - user[clause.name] = new EmptyRecordView(); - const characteristicTable = this._characteristicTables.get(clause.name); - if (characteristicTable) { - // User lodash's get() that supports paths, e.g. charId of 'a.b' would look up `user.a.b`. - const character = this._normalizeValue(get(user, clause.charId) as CellValue); - const rowNum = characteristicTable.rowNums.get(character); - if (rowNum !== undefined) { - user[clause.name] = new RecordView(characteristicTable.data, rowNum); - } + if (attrs.rows[clause.name]) { + user[clause.name] = attrs.rows[clause.name]; + continue; } + let rec = new EmptyRecordView(); + let rows: TableDataAction|undefined; + try { + // Use lodash's get() that supports paths, e.g. charId of 'a.b' would look up `user.a.b`. + // TODO: add indexes to db. + rows = await this._fetchQueryFromDB({tableId: clause.tableId, filters: { [clause.lookupColId]: [get(user, clause.charId)] }}); + } catch (e) { + log.warn(`User attribute ${clause.name} failed`, e); + } + if (rows && rows[2].length > 0) { rec = new RecordView(rows, 0); } + user[clause.name] = rec; + attrs.rows[clause.name] = rec; } return user; } @@ -878,7 +887,7 @@ export class GranularAccess { */ private async _pruneOutgoingDocAction(docSession: OptDocSession, a: DocAction, idx: number): Promise { const tableId = getTableId(a); - const permInfo = this._getAccess(docSession); + const permInfo = await this._getAccess(docSession); const tableAccess = permInfo.getTableAccess(tableId); const access = tableAccess.read; if (access === 'deny') { return []; } @@ -896,7 +905,7 @@ export class GranularAccess { private async _checkIncomingDocAction(docSession: OptDocSession, a: DocAction, idx: number): Promise { const accessFn = denyIsFatal(getAccessForActionType(a)); const tableId = getTableId(a); - const permInfo = this._getAccess(docSession); + const permInfo = await this._getAccess(docSession); const tableAccess = permInfo.getTableAccess(tableId); const access = accessFn(tableAccess); if (access === 'allow') { return; } @@ -1043,14 +1052,10 @@ class EmptyRecordView implements InfoView { } /** - * A cache of a table needed for look-ups, including a map from keys to - * row numbers. Keys are produced by _getCharacteristicTableKey(). + * Cache information about user attributes. */ -interface CharacteristicTable { - tableId: string; - colId: string; - rowNums: Map; - data: TableDataAction; +class UserAttributes { + public rows: {[clauseName: string]: InfoView} = {}; } // A function for extracting one of the create/read/update/delete/schemaEdit permissions diff --git a/app/server/lib/Sessions.ts b/app/server/lib/Sessions.ts index 3f780a16..411b4b0d 100644 --- a/app/server/lib/Sessions.ts +++ b/app/server/lib/Sessions.ts @@ -46,16 +46,16 @@ export class Sessions { const sid = this.getSessionIdFromRequest(req); const org = (req as any).org; if (!sid) { throw new Error("session not found"); } - return this.getOrCreateSession(sid, org); + return this.getOrCreateSession(sid, org, ''); // TODO: allow for tying to a preferred user. } /** * Get or create a session given the session id and organization name. */ - public getOrCreateSession(sid: string, domain: string): Session { - const key = this._getSessionOrgKey(sid, domain); + public getOrCreateSession(sid: string, domain: string, userSelector: string): Session { + const key = this._getSessionOrgKey(sid, domain, userSelector); if (!this._sessions.has(key)) { - const scopedSession = new ScopedSession(sid, this._sessionStore, domain); + const scopedSession = new ScopedSession(sid, this._sessionStore, domain, userSelector); this._sessions.set(key, {scopedSession}); } return this._sessions.get(key)!; @@ -67,8 +67,9 @@ export class Sessions { * */ public getOrCreateLoginSession(sid: string, domain: string, comm: Comm, - instanceManager: IInstanceManager|null): ILoginSession { - const sess = this.getOrCreateSession(sid, domain); + instanceManager: IInstanceManager|null, + userSelector: string): ILoginSession { + const sess = this.getOrCreateSession(sid, domain, userSelector); if (!sess.loginSession) { sess.loginSession = this._server.create.LoginSession(comm, sid, domain, sess.scopedSession, instanceManager); @@ -100,8 +101,10 @@ export class Sessions { * Grist has historically cached sessions in memory by their session id. * With the introduction of per-organization identity, that cache is now * needs to be keyed by the session id and organization name. + * Also, clients may now want to be tied to a particular user available within + * a session, so we add that into key too. */ - private _getSessionOrgKey(sid: string, domain: string): string { - return `${sid}__${domain}`; + private _getSessionOrgKey(sid: string, domain: string, userSelector: string): string { + return `${sid}__${domain}__${userSelector}`; } }