From 9dc44de7f0588e3035fb5a12747240305122f230 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Thu, 24 Jun 2021 09:27:19 +0100 Subject: [PATCH] Merge pull request #4191 from overleaf/sk-token-v1-fix TokenAccess: Fix handling of deleted projects GitOrigin-RevId: a602da335567d36b5e674ada69c1e1ab4a909d4a --- .../TokenAccess/TokenAccessController.js | 2 + .../TokenAccess/TokenAccessHandler.js | 18 +-- .../web/app/views/project/token/access.pug | 13 +-- .../test/acceptance/src/TokenAccessTests.js | 107 +++++++++++++++++- .../test/acceptance/src/mocks/MockV1Api.js | 8 ++ .../TokenAccess/TokenAccessHandlerTests.js | 46 +------- 6 files changed, 123 insertions(+), 71 deletions(-) diff --git a/services/web/app/src/Features/TokenAccess/TokenAccessController.js b/services/web/app/src/Features/TokenAccess/TokenAccessController.js index e256bac8b4..3861825ffd 100644 --- a/services/web/app/src/Features/TokenAccess/TokenAccessController.js +++ b/services/web/app/src/Features/TokenAccess/TokenAccessController.js @@ -53,6 +53,8 @@ async function _handleV1Project(token, userId) { token, userId ) + // This should not happen anymore, but it does show + // a nice "contact support" message, so it can stay if (!docInfo) { return { v1Import: { status: 'cannotImport' } } } diff --git a/services/web/app/src/Features/TokenAccess/TokenAccessHandler.js b/services/web/app/src/Features/TokenAccess/TokenAccessHandler.js index afaaa470f2..d42cbf1257 100644 --- a/services/web/app/src/Features/TokenAccess/TokenAccessHandler.js +++ b/services/web/app/src/Features/TokenAccess/TokenAccessHandler.js @@ -1,7 +1,6 @@ const { Project } = require('../../models/Project') const PublicAccessLevels = require('../Authorization/PublicAccessLevels') const PrivilegeLevels = require('../Authorization/PrivilegeLevels') -const UserGetter = require('../User/UserGetter') const { ObjectId } = require('mongodb') const Settings = require('settings-sharelatex') const logger = require('logger-sharelatex') @@ -279,23 +278,12 @@ const TokenAccessHandler = { exported: false, }) } - UserGetter.getUser(v2UserId, { overleaf: 1 }, function (err, user) { + const v1Url = `/api/v1/sharelatex/docs/${token}/info` + V1Api.request({ url: v1Url }, function (err, response, body) { if (err != null) { return callback(err) } - const v1UserId = user.overleaf != null ? user.overleaf.id : undefined - if (!v1UserId) { - return callback(null, null) - } - V1Api.request( - { url: `/api/v1/sharelatex/users/${v1UserId}/docs/${token}/info` }, - function (err, response, body) { - if (err != null) { - return callback(err) - } - callback(null, body) - } - ) + callback(null, body) }) }, } diff --git a/services/web/app/views/project/token/access.pug b/services/web/app/views/project/token/access.pug index b17dc84bb3..a269a5183c 100644 --- a/services/web/app/views/project/token/access.pug +++ b/services/web/app/views/project/token/access.pug @@ -50,7 +50,9 @@ block content .container .row .col-sm-8.col-sm-offset-2 - h1.text-center Move project to Overleaf v2 + h1.text-center + span(ng-if="v1ImportData.status != 'mustLogin'") Overleaf v1 Project + span(ng-if="v1ImportData.status == 'mustLogin'") Please Log In img.v2-import__img( src="/img/v1-import/v2-editor.png" alt="The new V2 editor." @@ -60,7 +62,7 @@ block content h2.text-center | Cannot Access Overleaf v1 Project p.text-center.row-spaced-small - | The project you are attempting to access must be imported to Overleaf v2 before it can be accessed. Please contact the project owner or + | Please contact the project owner or | a(href="/contact") contact support | @@ -68,14 +70,12 @@ block content div(ng-if="v1ImportData.status == 'mustLogin'") p.text-center.row-spaced-small - | This project has not yet been moved into the new version of - | Overleaf. You will need to log in and move it in order to - | continue working on it. + | You will need to log in to access this project. .row-spaced.text-center a.btn.btn-primary( href="/login?redir={{ currentPath() }}" - ) Log In To Move Project + ) Log In To Access Project div(ng-if="v1ImportData.status == 'canDownloadZip'") p.text-center.row-spaced.small @@ -98,4 +98,3 @@ block append foot-scripts $('.loading-screen-brand').css('height', '20%') }, 500); }); - diff --git a/services/web/test/acceptance/src/TokenAccessTests.js b/services/web/test/acceptance/src/TokenAccessTests.js index c5efaa98c8..3f4867fb71 100644 --- a/services/web/test/acceptance/src/TokenAccessTests.js +++ b/services/web/test/acceptance/src/TokenAccessTests.js @@ -1211,6 +1211,77 @@ describe('TokenAccess', function () { }) }) + describe('deleted project', function () { + beforeEach(function (done) { + settings.overleaf = { host: 'http://localhost:5000' } + this.owner.createProject( + `delete-test${Math.random()}`, + (err, projectId) => { + if (err != null) { + return done(err) + } + this.projectId = projectId + this.owner.makeTokenBased(this.projectId, err => { + if (err != null) { + return done(err) + } + this.owner.getProject(this.projectId, (err, project) => { + if (err != null) { + return done(err) + } + this.tokens = project.tokens + done() + }) + }) + } + ) + }) + + afterEach(function () { + delete settings.overleaf + }) + + it('should 404', function (done) { + async.series( + [ + // delete project + cb => { + this.owner.deleteProject(this.projectId, cb) + }, + cb => { + // use read-only token + tryReadOnlyTokenAccess( + this.other1, + this.tokens.readOnly, + (response, body) => { + expect(response.statusCode).to.equal(200) + }, + (response, body) => { + expect(response.statusCode).to.equal(404) + }, + cb + ) + }, + cb => { + // use read-write token + tryReadAndWriteTokenAccess( + this.other1, + this.tokens.readAndWrite, + (response, body) => { + expect(response.statusCode).to.equal(200) + }, + (response, body) => { + expect(response.statusCode).to.equal(404) + }, + cb + ) + }, + ], + done + ) + }) + }) + describe('unimported v1 project', function () { beforeEach(function () { settings.overleaf = { host: 'http://localhost:5000' } @@ -1220,8 +1291,15 @@ describe('TokenAccess', function () { delete settings.overleaf }) - it('should show error page for read and write token', function (done) { + it('should show download option for read and write token', function (done) { const unimportedV1Token = '123abcdefabcdef' + const docInfo = { + exists: true, + exported: false, + has_owner: true, + name: 'test', + } + MockV1Api.setDocInfo(unimportedV1Token, docInfo) tryReadAndWriteTokenAccess( this.owner, unimportedV1Token, @@ -1230,14 +1308,28 @@ describe('TokenAccess', function () { }, (response, body) => { expect(response.statusCode).to.equal(200) - expect(body).to.deep.equal({ v1Import: { status: 'cannotImport' } }) + expect(body).to.deep.equal({ + v1Import: { + hasOwner: true, + name: 'test', + projectId: unimportedV1Token, + status: 'canDownloadZip', + }, + }) }, done ) }) - it('should show error page for read only token to v1', function (done) { + it('should show download option for read only token to v1', function (done) { const unimportedV1Token = 'aaaaaabbbbbb' + const docInfo = { + exists: true, + exported: false, + has_owner: true, + name: 'test', + } + MockV1Api.setDocInfo(unimportedV1Token, docInfo) tryReadOnlyTokenAccess( this.owner, unimportedV1Token, @@ -1246,7 +1338,14 @@ describe('TokenAccess', function () { }, (response, body) => { expect(response.statusCode).to.equal(200) - expect(body).to.deep.equal({ v1Import: { status: 'cannotImport' } }) + expect(body).to.deep.equal({ + v1Import: { + hasOwner: true, + name: 'test', + projectId: unimportedV1Token, + status: 'canDownloadZip', + }, + }) }, done ) diff --git a/services/web/test/acceptance/src/mocks/MockV1Api.js b/services/web/test/acceptance/src/mocks/MockV1Api.js index ce57e81c7c..c88f3ebdfb 100644 --- a/services/web/test/acceptance/src/mocks/MockV1Api.js +++ b/services/web/test/acceptance/src/mocks/MockV1Api.js @@ -353,6 +353,14 @@ class MockV1Api extends AbstractMockApi { } ) + this.app.get('/api/v1/sharelatex/docs/:token/info', (req, res) => { + const info = this.getDocInfo(req.params.token) || { + exists: false, + exported: false, + } + res.json(info) + }) + this.app.get( '/api/v1/sharelatex/docs/read_token/:token/exists', (req, res) => { diff --git a/services/web/test/unit/src/TokenAccess/TokenAccessHandlerTests.js b/services/web/test/unit/src/TokenAccess/TokenAccessHandlerTests.js index fc7bc5b0d5..ba65af4863 100644 --- a/services/web/test/unit/src/TokenAccess/TokenAccessHandlerTests.js +++ b/services/web/test/unit/src/TokenAccess/TokenAccessHandlerTests.js @@ -38,7 +38,6 @@ describe('TokenAccessHandler', function () { mongodb: { ObjectId }, '../../models/Project': { Project: (this.Project = {}) }, 'settings-sharelatex': (this.settings = {}), - '../User/UserGetter': (this.UserGetter = {}), '../V1/V1Api': (this.V1Api = { request: sinon.stub(), }), @@ -616,7 +615,6 @@ describe('TokenAccessHandler', function () { describe('getV1DocInfo', function () { beforeEach(function () { - this.v2UserId = 123 return (this.callback = sinon.stub()) }) @@ -645,47 +643,8 @@ describe('TokenAccessHandler', function () { return (this.settings.apis = { v1: 'v1' }) }) - describe('on UserGetter.getUser success', function () { - beforeEach(function () { - this.UserGetter.getUser = sinon.stub().yields(null, { - overleaf: { id: 1 }, - }) - return this.TokenAccessHandler.getV1DocInfo( - this.token, - this.v2UserId, - this.callback - ) - }) - - it('should get user', function () { - return expect( - this.UserGetter.getUser.calledWith(this.v2UserId) - ).to.equal(true) - }) - }) - - describe('on UserGetter.getUser error', function () { - beforeEach(function () { - this.error = new Error('failed to get user') - this.UserGetter.getUser = sinon.stub().yields(this.error) - return this.TokenAccessHandler.getV1DocInfo( - this.token, - this.v2UserId, - this.callback - ) - }) - - it('should callback with error', function () { - return expect(this.callback.calledWith(this.error)).to.equal(true) - }) - }) - describe('on V1Api.request success', function () { beforeEach(function () { - this.v1UserId = 1 - this.UserGetter.getUser = sinon.stub().yields(null, { - overleaf: { id: this.v1UserId }, - }) this.V1Api.request = sinon .stub() .callsArgWith(1, null, null, 'mock-data') @@ -699,7 +658,7 @@ describe('TokenAccessHandler', function () { it('should return response body', function () { expect( this.V1Api.request.calledWith({ - url: `/api/v1/sharelatex/users/${this.v1UserId}/docs/${this.token}/info`, + url: `/api/v1/sharelatex/docs/${this.token}/info`, }) ).to.equal(true) return expect(this.callback.calledWith(null, 'mock-data')).to.equal( @@ -710,9 +669,6 @@ describe('TokenAccessHandler', function () { describe('on V1Api.request error', function () { beforeEach(function () { - this.UserGetter.getUser = sinon.stub().yields(null, { - overleaf: { id: 1 }, - }) this.V1Api.request = sinon.stub().callsArgWith(1, 'error') return this.TokenAccessHandler.getV1DocInfo( this.token,