(core) be stricter when replacing documents in the presence of granular access rules

Summary:
The /replace endpoint was built with home-level access control in mind. Updates needed:
  * Only an owner can now replace a document. Only owners are permitted to change granular access rules, and a document replacement could change granular access rules.
  * For the document being substituted in: the user must have complete access to view all material within it.

Test Plan: extended test

Reviewers: georgegevoian, dsagal

Reviewed By: georgegevoian, dsagal

Differential Revision: https://phab.getgrist.com/D3694
This commit is contained in:
Paul Fitzpatrick
2022-11-09 11:49:23 -05:00
parent 101450262c
commit 42c3568835
3 changed files with 56 additions and 8 deletions

View File

@@ -680,10 +680,13 @@ export class ActiveDoc extends EventEmitter {
* shut it down, and unlist it via the DocManager. A fresh ActiveDoc can be acquired via the
* DocManager.
*/
public async replace(source: DocReplacementOptions) {
public async replace(docSession: OptDocSession, source: DocReplacementOptions) {
// During replacement, it is important for all hands to be off the document. So we
// ask the shutdown method to do the replacement when the ActiveDoc is shutdown but
// before a new one could be opened.
if (!await this._granularAccess.isOwner(docSession)) {
throw new ApiError('Only owners can replace a document.', 403);
}
return this.shutdown({
afterShutdown: () => this._docManager.storageManager.replace(this.docName, source)
});

View File

@@ -393,10 +393,15 @@ export class DocWorkerApi {
// TODO: look at download behavior if ActiveDoc is shutdown during call (cannot
// use withDoc wrapper)
this._app.get('/api/docs/:docId/download', canView, throttled(async (req, res) => {
// Support a dryRun flag to check if user has the right to download the
// full document.
const dryRun = isAffirmative(req.query.dryrun || req.query.dryRun);
const dryRunSuccess = () => res.status(200).json({dryRun: 'allowed'});
// We want to be have a way download broken docs that ActiveDoc may not be able
// to load. So, if the user owns the document, we unconditionally let them
// download.
if (await this._isOwner(req)) {
if (dryRun) { dryRunSuccess(); return; }
try {
// We carefully avoid creating an ActiveDoc for the document being downloaded,
// in case it is broken in some way. It is convenient to be able to download
@@ -419,6 +424,7 @@ export class DocWorkerApi {
if (!await activeDoc.canDownload(docSessionFromRequest(req))) {
throw new ApiError('not authorized to download this document', 403);
}
if (dryRun) { dryRunSuccess(); return; }
return this._docWorker.downloadDoc(req, res, this._docManager.storageManager);
}
}));
@@ -733,6 +739,18 @@ export class DocWorkerApi {
const options: DocReplacementOptions = {};
if (req.body.sourceDocId) {
options.sourceDocId = await this._confirmDocIdForRead(req, String(req.body.sourceDocId));
// Make sure that if we wanted to download the full source, we would be allowed.
const result = await fetch(this._grist.getHomeUrl(req, `/api/docs/${options.sourceDocId}/download?dryrun=1`), {
method: 'GET',
headers: {
...getTransitiveHeaders(req),
'Content-Type': 'application/json',
}
});
if (result.status !== 200) {
const jsonResult = await result.json();
throw new ApiError(jsonResult.error, result.status);
}
// We should make sure the source document has flushed recently.
// It may not be served by the same worker, so work through the api.
await fetch(this._grist.getHomeUrl(req, `/api/docs/${options.sourceDocId}/flush`), {
@@ -746,7 +764,8 @@ export class DocWorkerApi {
if (req.body.snapshotId) {
options.snapshotId = String(req.body.snapshotId);
}
await activeDoc.replace(options);
const docSession = docSessionFromRequest(req);
await activeDoc.replace(docSession, options);
res.json(null);
}));