From 96fa7ad5620921a974f9981fde7a062fb4383074 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaros=C5=82aw=20Sadzi=C5=84ski?= Date: Tue, 9 Nov 2021 11:34:26 +0100 Subject: [PATCH] (core) Error message on Duplicate Document Summary: Fixing error message when user can't copy document. Test Plan: Updated tests Reviewers: paulfitz Reviewed By: paulfitz Differential Revision: https://phab.getgrist.com/D3130 --- app/client/models/NotifyModel.ts | 1 + app/client/ui/MakeCopyMenu.ts | 15 ++++++++++++--- app/server/lib/DocApi.ts | 2 +- app/server/lib/uploads.ts | 18 +++++++++++++----- 4 files changed, 27 insertions(+), 9 deletions(-) diff --git a/app/client/models/NotifyModel.ts b/app/client/models/NotifyModel.ts index a4e193fe..22eb5dd9 100644 --- a/app/client/models/NotifyModel.ts +++ b/app/client/models/NotifyModel.ts @@ -304,6 +304,7 @@ export class Notifier extends Disposable implements INotifier { // This is exposed primarily for tests. public clearAppErrors() { this._appErrorList.splice(0); + this._appErrorToast.clear(); } /** diff --git a/app/client/ui/MakeCopyMenu.ts b/app/client/ui/MakeCopyMenu.ts index 1e69190a..b5745ce0 100644 --- a/app/client/ui/MakeCopyMenu.ts +++ b/app/client/ui/MakeCopyMenu.ts @@ -154,9 +154,18 @@ class SaveCopyModal extends Disposable { const org = this._destOrg.get(); const docWorker = await api.getWorkerAPI('import'); const destName = this._destName.get() + '.grist'; - const uploadId = await docWorker.copyDoc(this._doc.id, this._asTemplate.get(), destName); - const {id} = await docWorker.importDocToWorkspace(uploadId, ws.id); - await urlState().pushUrl({org: org?.domain || undefined, doc: id, docPage: urlState().state.get().docPage}); + try { + const uploadId = await docWorker.copyDoc(this._doc.id, this._asTemplate.get(), destName); + const {id} = await docWorker.importDocToWorkspace(uploadId, ws.id); + await urlState().pushUrl({org: org?.domain || undefined, doc: id, docPage: urlState().state.get().docPage}); + } catch(err) { + // Convert access denied errors to normal Error to make it consistent with other endpoints. + // TODO: Should not allow to click this button when user doesn't have permissions. + if (err.status === 403) { + throw new Error(err.details.userError || err.message); + } + throw err; + } } public buildDom() { diff --git a/app/server/lib/DocApi.ts b/app/server/lib/DocApi.ts index 899df3ef..cf27ebfb 100644 --- a/app/server/lib/DocApi.ts +++ b/app/server/lib/DocApi.ts @@ -342,7 +342,7 @@ export class DocWorkerApi { // check if the user has download permissions. const activeDoc = await this._getActiveDoc(req); if (!await activeDoc.canDownload(docSessionFromRequest(req))) { - throw new Error('not authorized to download this document'); + throw new ApiError('not authorized to download this document', 403); } return this._docWorker.downloadDoc(req, res, this._docManager.storageManager); } diff --git a/app/server/lib/uploads.ts b/app/server/lib/uploads.ts index 4cb9a412..506c849d 100644 --- a/app/server/lib/uploads.ts +++ b/app/server/lib/uploads.ts @@ -67,12 +67,20 @@ export function addUploadRoute(server: GristServer, expressApp: Application, ... const name = optStringParam(req.query.name); if (!docId) { throw new Error('doc must be specified'); } const accessId = makeAccessId(req, getAuthorizedUserId(req)); - const uploadResult: UploadResult = await fetchDoc(server.getHomeUrl(req), docId, req, accessId, - req.query.template === '1'); - if (name) { - globalUploadSet.changeUploadName(uploadResult.uploadId, accessId, name); + try { + const uploadResult: UploadResult = await fetchDoc(server.getHomeUrl(req), docId, req, accessId, + req.query.template === '1'); + if (name) { + globalUploadSet.changeUploadName(uploadResult.uploadId, accessId, name); + } + res.status(200).send(JSON.stringify(uploadResult)); + } catch(err) { + if ((err as ApiError).status === 403) { + res.status(403).json({error:'Insufficient access to document to copy it entirely'}); + return; + } + throw err; } - res.status(200).send(JSON.stringify(uploadResult)); })); }