From 7f722a006c8f4e239b64d78255d66be208506723 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Timoth=C3=A9e=20Alby?= Date: Thu, 28 Jul 2022 11:20:44 +0200 Subject: [PATCH] Merge pull request #8571 from overleaf/ta-token-access-page Require User Interaction on Token Access Page GitOrigin-RevId: 2f4c00ba75ebd6bd87d3e770ec8223d736344f5b --- .../Authorization/AuthorizationManager.js | 3 +- .../TokenAccess/TokenAccessController.js | 30 ++- .../web/app/views/project/token/access.pug | 25 +++ .../js/features/form-helpers/hydrate-form.js | 4 +- services/web/frontend/js/main/token-access.js | 21 ++- services/web/locales/en.json | 1 + .../test/acceptance/src/TokenAccessTests.js | 173 ++++++++++++++---- 7 files changed, 208 insertions(+), 49 deletions(-) diff --git a/services/web/app/src/Features/Authorization/AuthorizationManager.js b/services/web/app/src/Features/Authorization/AuthorizationManager.js index 7f10fbacfe..08dfa82900 100644 --- a/services/web/app/src/Features/Authorization/AuthorizationManager.js +++ b/services/web/app/src/Features/Authorization/AuthorizationManager.js @@ -69,7 +69,7 @@ async function getPrivilegeLevelForProject( opts = {} ) { if (userId) { - return getPrivilegeLevelForProjectWithUser(userId, projectId, token, opts) + return getPrivilegeLevelForProjectWithUser(userId, projectId, opts) } else { return getPrivilegeLevelForProjectWithoutUser(projectId, token, opts) } @@ -79,7 +79,6 @@ async function getPrivilegeLevelForProject( async function getPrivilegeLevelForProjectWithUser( userId, projectId, - token, opts = {} ) { const privilegeLevel = diff --git a/services/web/app/src/Features/TokenAccess/TokenAccessController.js b/services/web/app/src/Features/TokenAccess/TokenAccessController.js index 1472e78aaf..97aa24ebdf 100644 --- a/services/web/app/src/Features/TokenAccess/TokenAccessController.js +++ b/services/web/app/src/Features/TokenAccess/TokenAccessController.js @@ -19,20 +19,17 @@ const orderedPrivilegeLevels = [ PrivilegeLevels.OWNER, ] -async function _userAlreadyHasHigherPrivilege( - userId, - projectId, - token, - tokenType -) { +async function _userAlreadyHasHigherPrivilege(userId, projectId, tokenType) { if (!Object.values(TokenAccessHandler.TOKEN_TYPES).includes(tokenType)) { throw new Error('bad token type') } + if (!userId) { + return false + } const privilegeLevel = await AuthorizationManager.promises.getPrivilegeLevelForProject( userId, - projectId, - token + projectId ) return ( orderedPrivilegeLevels.indexOf(privilegeLevel) >= @@ -196,7 +193,6 @@ async function checkAndGetProjectOrResponseAction( const userHasPrivilege = await _userAlreadyHasHigherPrivilege( userId, projectId, - token, tokenType ) if (userHasPrivilege) { @@ -220,6 +216,7 @@ async function checkAndGetProjectOrResponseAction( async function grantTokenAccessReadAndWrite(req, res, next) { const { token } = req.params + const { confirmedByUser } = req.body const userId = SessionManager.getLoggedInUserId(req.session) if (!TokenAccessHandler.isReadAndWriteToken(token)) { return res.sendStatus(400) @@ -240,6 +237,13 @@ async function grantTokenAccessReadAndWrite(req, res, next) { if (!project) { return next(new Errors.NotFoundError()) } + if (!confirmedByUser) { + return res.json({ + requireAccept: { + projectName: project.name, + }, + }) + } await TokenAccessHandler.promises.addReadAndWriteUserToProject( userId, project._id @@ -261,6 +265,7 @@ async function grantTokenAccessReadAndWrite(req, res, next) { async function grantTokenAccessReadOnly(req, res, next) { const { token } = req.params + const { confirmedByUser } = req.body const userId = SessionManager.getLoggedInUserId(req.session) if (!TokenAccessHandler.isReadOnlyToken(token)) { return res.sendStatus(400) @@ -286,6 +291,13 @@ async function grantTokenAccessReadOnly(req, res, next) { if (!project) { return next(new Errors.NotFoundError()) } + if (!confirmedByUser) { + return res.json({ + requireAccept: { + projectName: project.name, + }, + }) + } await TokenAccessHandler.promises.addReadOnlyUserToProject( userId, project._id diff --git a/services/web/app/views/project/token/access.pug b/services/web/app/views/project/token/access.pug index d5b41a1157..e3c22707dd 100644 --- a/services/web/app/views/project/token/access.pug +++ b/services/web/app/views/project/token/access.pug @@ -91,6 +91,31 @@ block content a.btn.btn-primary(ng-href="{{ buildZipDownloadPath(v1ImportData.projectId) }}") | Download project zip file + .loading-screen( + ng-show="mode == 'requireAccept'" + ) + .container + .row + .col-md-8.col-md-offset-2 + .card + .page-header.text-centered + h1 #{translate("invited_to_join")} + br + em {{ getProjectName() }} + .row.text-center + .col-md-12 + p + if user + | #{translate("accepting_invite_as")} + | + em #{user.email} + .row.text-center + .col-md-12 + button.btn.btn-lg.btn-primary( + type='submit' + ng-click="postConfirmedByUser()" + ) #{translate("join_project")} + block append foot-scripts script(type="text/javascript", nonce=scriptNonce). diff --git a/services/web/frontend/js/features/form-helpers/hydrate-form.js b/services/web/frontend/js/features/form-helpers/hydrate-form.js index dc0a17e522..2eb6907e3b 100644 --- a/services/web/frontend/js/features/form-helpers/hydrate-form.js +++ b/services/web/frontend/js/features/form-helpers/hydrate-form.js @@ -44,8 +44,8 @@ function formSubmitHelper(formEl) { formEl.dispatchEvent(new Event('sent')) // Handle redirects - if (data.redir) { - window.location = data.redir + if (data.redir || data.redirect) { + window.location = data.redir || data.redirect return } diff --git a/services/web/frontend/js/main/token-access.js b/services/web/frontend/js/main/token-access.js index 0d9cd9a343..1d1b11ec62 100644 --- a/services/web/frontend/js/main/token-access.js +++ b/services/web/frontend/js/main/token-access.js @@ -3,9 +3,10 @@ App.controller( 'TokenAccessPageController', ($scope, $http, $location, localStorage) => { window.S = $scope - $scope.mode = 'accessAttempt' // 'accessAttempt' | 'v1Import' + $scope.mode = 'accessAttempt' // 'accessAttempt' | 'v1Import' | 'requireAccept' $scope.v1ImportData = null + $scope.requireAccept = null $scope.accessInFlight = false $scope.accessSuccess = false @@ -20,14 +21,20 @@ App.controller( } $scope.getProjectName = () => { - if (!$scope.v1ImportData || !$scope.v1ImportData.name) { - return 'This project' - } else { + if ($scope.v1ImportData?.name) { return $scope.v1ImportData.name + } else if ($scope.requireAccept?.projectName) { + return $scope.requireAccept.projectName + } else { + return 'This project' } } - $scope.post = () => { + $scope.postConfirmedByUser = () => { + $scope.post(true) + } + + $scope.post = (confirmedByUser = false) => { $scope.mode = 'accessAttempt' const textData = $('#overleaf-token-access-data').text() const parsedData = JSON.parse(textData) @@ -39,6 +46,7 @@ App.controller( url: postUrl, data: { _csrf: csrfToken, + confirmedByUser, }, }).then( function successCallback(response) { @@ -59,6 +67,9 @@ App.controller( } else if (data.v1Import) { $scope.mode = 'v1Import' $scope.v1ImportData = data.v1Import + } else if (data.requireAccept) { + $scope.mode = 'requireAccept' + $scope.requireAccept = data.requireAccept } else { console.warn( 'invalid data from server in success response', diff --git a/services/web/locales/en.json b/services/web/locales/en.json index 6998c11213..3cf3317033 100644 --- a/services/web/locales/en.json +++ b/services/web/locales/en.json @@ -1030,6 +1030,7 @@ "login_here": "Login here", "set_new_password": "Set new password", "user_wants_you_to_see_project": "__username__ would like you to join __projectname__", + "invited_to_join": "You have been invited to join", "join_sl_to_view_project": "Join __appName__ to view this project", "register_to_edit_template": "Please register to edit the __templateName__ template", "already_have_sl_account": "Already have an __appName__ account?", diff --git a/services/web/test/acceptance/src/TokenAccessTests.js b/services/web/test/acceptance/src/TokenAccessTests.js index 9b3cfbb0c5..59c3543b80 100644 --- a/services/web/test/acceptance/src/TokenAccessTests.js +++ b/services/web/test/acceptance/src/TokenAccessTests.js @@ -96,6 +96,70 @@ const _doTryTokenAccess = ( }) } +const tryReadOnlyTokenAccept = ( + user, + token, + testPageLoad, + testFormPost, + callback +) => { + _doTryTokenAccept( + `/read/${token}`, + user, + token, + testPageLoad, + testFormPost, + callback + ) +} + +const tryReadAndWriteTokenAccept = ( + user, + token, + testPageLoad, + testFormPost, + callback +) => { + _doTryTokenAccept( + `/${token}`, + user, + token, + testPageLoad, + testFormPost, + callback + ) +} + +const _doTryTokenAccept = ( + url, + user, + token, + testPageLoad, + testFormPost, + callback +) => { + user.request.get(url, (err, response, body) => { + if (err) { + return callback(err) + } + testPageLoad(response, body) + if (!testFormPost) { + return callback() + } + user.request.post( + `${url}/grant`, + { json: { token, confirmedByUser: true } }, + (err, response, body) => { + if (err) { + return callback(err) + } + testFormPost(response, body) + callback() + } + ) + }) +} + const tryContentAccess = (user, projcetId, test, callback) => { // The real-time service calls this end point to determine the user's // permissions. @@ -233,27 +297,25 @@ describe('TokenAccess', function () { describe('read-only token', function () { beforeEach(function (done) { - this.owner.createProject( - `token-ro-test${Math.random()}`, - (err, projectId) => { + this.projectName = `token-ro-test${Math.random()}` + this.owner.createProject(this.projectName, (err, projectId) => { + if (err != null) { + return done(err) + } + this.projectId = projectId + this.owner.makeTokenBased(this.projectId, err => { if (err != null) { return done(err) } - this.projectId = projectId - this.owner.makeTokenBased(this.projectId, err => { + this.owner.getProject(this.projectId, (err, project) => { if (err != null) { return done(err) } - this.owner.getProject(this.projectId, (err, project) => { - if (err != null) { - return done(err) - } - this.tokens = project.tokens - done() - }) + this.tokens = project.tokens + done() }) - } - ) + }) + }) }) it('allow the user read-only access to the project', function (done) { @@ -269,8 +331,34 @@ describe('TokenAccess', function () { ) }, cb => { - // use token + // try token tryReadOnlyTokenAccess( + this.other1, + this.tokens.readOnly, + (response, body) => { + expect(response.statusCode).to.equal(200) + }, + (response, body) => { + expect(response.statusCode).to.equal(200) + expect(body.requireAccept.projectName).to.equal( + this.projectName + ) + }, + cb + ) + }, + cb => { + // deny access before token is accepted + tryEditorAccess( + this.other1, + this.projectId, + expectErrorResponse.restricted.html, + cb + ) + }, + cb => { + // accept token + tryReadOnlyTokenAccept( this.other1, this.tokens.readOnly, (response, body) => { @@ -543,27 +631,25 @@ describe('TokenAccess', function () { describe('read-and-write token', function () { beforeEach(function (done) { - this.owner.createProject( - `token-rw-test${Math.random()}`, - (err, projectId) => { + this.projectName = `token-rw-test${Math.random()}` + this.owner.createProject(this.projectName, (err, projectId) => { + if (err != null) { + return done(err) + } + this.projectId = projectId + this.owner.makeTokenBased(this.projectId, err => { if (err != null) { return done(err) } - this.projectId = projectId - this.owner.makeTokenBased(this.projectId, err => { + this.owner.getProject(this.projectId, (err, project) => { if (err != null) { return done(err) } - this.owner.getProject(this.projectId, (err, project) => { - if (err != null) { - return done(err) - } - this.tokens = project.tokens - done() - }) + this.tokens = project.tokens + done() }) - } - ) + }) + }) }) it('should allow the user to access project via read-and-write token url', function (done) { @@ -577,8 +663,33 @@ describe('TokenAccess', function () { expectErrorResponse.restricted.html, cb ), + // try token cb => tryReadAndWriteTokenAccess( + this.other1, + this.tokens.readAndWrite, + (response, body) => { + expect(response.statusCode).to.equal(200) + }, + (response, body) => { + expect(response.statusCode).to.equal(200) + expect(body.requireAccept.projectName).to.equal( + this.projectName + ) + }, + cb + ), + // deny access before the token is accepted + cb => + tryEditorAccess( + this.other1, + this.projectId, + expectErrorResponse.restricted.html, + cb + ), + // accept token + cb => + tryReadAndWriteTokenAccept( this.other1, this.tokens.readAndWrite, (response, body) => { @@ -661,7 +772,7 @@ describe('TokenAccess', function () { ), cb => { // use read-only token - tryReadOnlyTokenAccess( + tryReadOnlyTokenAccept( this.other1, this.tokens.readOnly, (response, body) => { @@ -707,7 +818,7 @@ describe('TokenAccess', function () { // Then switch to read-write token // cb => - tryReadAndWriteTokenAccess( + tryReadAndWriteTokenAccept( this.other1, this.tokens.readAndWrite, (response, body) => {