From f4a7b1f298ace297db4a2ed2d1cfb240d16335f3 Mon Sep 17 00:00:00 2001 From: Liangjun Song <146005915+adai26@users.noreply.github.com> Date: Mon, 15 Jul 2024 11:23:03 +0100 Subject: [PATCH] bypass linking sharing admin redirect for internal projects (#19314) * disable linking sharing admin redirect * address comments * remove ignoreSiteAdmin * load admin domains from settings * add acceptance test * more tests * fix tests and restore admin domain * use adminDomains as array GitOrigin-RevId: 5acb62e1b6ada0aaeceab6db6a6635f82e30833f --- .../Authorization/AuthorizationMiddleware.js | 5 +- .../TokenAccess/TokenAccessController.js | 53 ++++++++++++-- .../src/AdminPrivilegeAvailableTests.js | 67 ++++++++++++++--- .../TokenAccess/TokenAccessControllerTests.js | 73 ++++++++++++++++++- 4 files changed, 177 insertions(+), 21 deletions(-) diff --git a/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js b/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js index e45dc23556..36f75741b2 100644 --- a/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js +++ b/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js @@ -12,7 +12,7 @@ const { } = require('../Helpers/AdminAuthorizationHelper') const { getSafeAdminDomainRedirect } = require('../Helpers/UrlHelper') -function handleAdminDomainRedirect(req, res) { +function _handleAdminDomainRedirect(req, res) { if (canRedirectToAdminDomain(SessionManager.getSessionUser(req.session))) { logger.warn({ req }, 'redirecting admin user to admin domain') res.redirect(getSafeAdminDomainRedirect(req.originalUrl)) @@ -150,7 +150,7 @@ async function ensureUserIsSiteAdmin(req, res, next) { logger.debug({ userId }, 'allowing user admin access to site') return next() } - if (handleAdminDomainRedirect(req, res)) return + if (_handleAdminDomainRedirect(req, res)) return logger.debug({ userId }, 'denying user admin access to site') _redirectToRestricted(req, res, next) } @@ -205,6 +205,5 @@ module.exports = { ), ensureUserCanAdminProject: expressify(ensureUserCanAdminProject), ensureUserIsSiteAdmin: expressify(ensureUserIsSiteAdmin), - handleAdminDomainRedirect, restricted, } diff --git a/services/web/app/src/Features/TokenAccess/TokenAccessController.js b/services/web/app/src/Features/TokenAccess/TokenAccessController.js index a78d13beb3..dbfea873f9 100644 --- a/services/web/app/src/Features/TokenAccess/TokenAccessController.js +++ b/services/web/app/src/Features/TokenAccess/TokenAccessController.js @@ -8,9 +8,6 @@ const OError = require('@overleaf/o-error') const { expressify } = require('@overleaf/promise-utils') const AuthorizationManager = require('../Authorization/AuthorizationManager') const PrivilegeLevels = require('../Authorization/PrivilegeLevels') -const { - handleAdminDomainRedirect, -} = require('../Authorization/AuthorizationMiddleware') const ProjectAuditLogHandler = require('../Project/ProjectAuditLogHandler') const SplitTestHandler = require('../SplitTests/SplitTestHandler') const CollaboratorsHandler = require('../Collaborators/CollaboratorsHandler') @@ -19,6 +16,12 @@ const CollaboratorsGetter = require('../Collaborators/CollaboratorsGetter') const ProjectGetter = require('../Project/ProjectGetter') const AsyncFormHelper = require('../Helpers/AsyncFormHelper') const AnalyticsManager = require('../Analytics/AnalyticsManager') +const { + canRedirectToAdminDomain, +} = require('../Helpers/AdminAuthorizationHelper') +const { getSafeAdminDomainRedirect } = require('../Helpers/UrlHelper') +const UserGetter = require('../User/UserGetter') +const Settings = require('@overleaf/settings') const orderedPrivilegeLevels = [ PrivilegeLevels.NONE, @@ -86,15 +89,20 @@ async function _handleV1Project(token, userId) { } } +async function _isOverleafStaff(userId) { + const emails = await UserGetter.promises.getUserConfirmedEmails(userId) + const adminDomains = Settings.adminDomains ?? [] + return emails.some(email => + adminDomains.some(adminDomain => email.email.endsWith(`@${adminDomain}`)) + ) +} + async function tokenAccessPage(req, res, next) { const { token } = req.params if (!TokenAccessHandler.isValidToken(token)) { return next(new Errors.NotFoundError()) } - if (handleAdminDomainRedirect(req, res)) { - // Admin users do not join the project, but view it on the admin domain. - return - } + try { if (TokenAccessHandler.isReadOnlyToken(token)) { const docPublishedInfo = @@ -231,6 +239,37 @@ async function checkAndGetProjectOrResponseAction( { projectId, action: 'user already has higher or same privilege' }, ] } + + // Handle admin redirect + // If the project owner is an internal staff (using @overleaf.com email), + // the admin will join the project "for real". + // If the project owner is a external user + // the admin will be redirect to admin domain to view the project. + if (canRedirectToAdminDomain(SessionManager.getSessionUser(req.session))) { + const isProjectOwnerOverleafStaff = await _isOverleafStaff( + project.owner_ref + ) + if (isProjectOwnerOverleafStaff) { + logger.warn( + { projectId, userId }, + 'letting admin user join staff project' + ) + } else { + let projectUrlWithToken = TokenAccessHandler.makeTokenUrl(token) + if (tokenHashPrefix && tokenHashPrefix.startsWith('#')) { + projectUrlWithToken += `${tokenHashPrefix}` + } + return [ + null, + () => + res.json({ + redirect: getSafeAdminDomainRedirect(projectUrlWithToken), + }), + { projectId, action: 'redirect admin user to admin domain' }, + ] + } + } + if (!tokenAccessEnabled) { return [ null, diff --git a/services/web/test/acceptance/src/AdminPrivilegeAvailableTests.js b/services/web/test/acceptance/src/AdminPrivilegeAvailableTests.js index 8924965ed0..7c7ce052bb 100644 --- a/services/web/test/acceptance/src/AdminPrivilegeAvailableTests.js +++ b/services/web/test/acceptance/src/AdminPrivilegeAvailableTests.js @@ -1,6 +1,9 @@ const Settings = require('@overleaf/settings') const { expect } = require('chai') const User = require('./helpers/User').promises +const { + getSafeAdminDomainRedirect, +} = require('../../../app/src/Features/Helpers/UrlHelper') describe('AdminPrivilegeAvailable', function () { let adminUser, otherUser @@ -22,7 +25,10 @@ describe('AdminPrivilegeAvailable', function () { }) beforeEach('create other user and project', async function () { - otherUser = new User() + otherUser = new User({ + email: 'test@non-staff.com', + confirmedAt: new Date(), + }) await otherUser.login() otherUsersProjectId = await otherUser.createProject('other users project') @@ -66,6 +72,16 @@ describe('AdminPrivilegeAvailable', function () { it('should display token access page for regular user', async function () { await displayTokenAccessPage(otherUser) }) + it('should redirect a token grant request to project page', async function () { + const { response } = await adminUser.doRequest('POST', { + url: `${otherUsersProjectTokenAccessURL}/grant`, + json: { + confirmedByUser: true, + }, + }) + expect(response.statusCode).to.equal(200) + expect(response.body.redirect).to.equal(`/project/${otherUsersProjectId}`) + }) }) describe('adminPrivilegeAvailable=false', function () { @@ -78,18 +94,49 @@ describe('AdminPrivilegeAvailable', function () { it('should block the admin from non-owned project', async function () { expect(await hasAccess(otherUsersProjectId)).to.equal(false) }) - it('should redirect a token access request to admin panel', async function () { - const { response } = await adminUser.doRequest( - 'GET', - otherUsersProjectTokenAccessURL - ) - expect(response.statusCode).to.equal(302) - expect(response.headers.location).to.equal( - Settings.adminUrl + otherUsersProjectTokenAccessURL - ) + it('should display token access page for admin', async function () { + displayTokenAccessPage(adminUser) }) it('should display token access page for regular user', async function () { await displayTokenAccessPage(otherUser) }) + it('should redirect a token grant request to admin panel if belongs to non-staff', async function () { + const { response } = await adminUser.doRequest('POST', { + url: `${otherUsersProjectTokenAccessURL}/grant`, + json: { + confirmedByUser: true, + }, + }) + expect(response.statusCode).to.equal(200) + expect(response.body.redirect).to.equal( + getSafeAdminDomainRedirect(otherUsersProjectTokenAccessURL) + ) + }) + + it('should redirect a token grant request to project page if belongs to staff', async function () { + const staff = new User({ + email: `test@${Settings.adminDomains[0]}`, + confirmedAt: new Date(), + }) + await staff.ensureUserExists() + await staff.ensureAdmin() + await staff.login() + + const staffProjectId = await staff.createProject('staff user project') + await staff.makeTokenBased(staffProjectId) + const { + tokens: { readOnly: readOnlyTokenAdmin }, + } = await staff.getProject(staffProjectId) + const staffProjectTokenAccessURL = `/read/${readOnlyTokenAdmin}` + + const { response } = await adminUser.doRequest('POST', { + url: `${staffProjectTokenAccessURL}/grant`, + json: { + confirmedByUser: true, + }, + }) + expect(response.statusCode).to.equal(200) + expect(response.body.redirect).to.equal(`/project/${staffProjectId}`) + }) }) }) diff --git a/services/web/test/unit/src/TokenAccess/TokenAccessControllerTests.js b/services/web/test/unit/src/TokenAccess/TokenAccessControllerTests.js index 12cd17477a..c010be0b54 100644 --- a/services/web/test/unit/src/TokenAccess/TokenAccessControllerTests.js +++ b/services/web/test/unit/src/TokenAccess/TokenAccessControllerTests.js @@ -23,7 +23,12 @@ describe('TokenAccessController', function () { this.res = new MockResponse() this.next = sinon.stub().returns() - this.Settings = {} + this.Settings = { + siteUrl: 'https://www.dev-overleaf.com', + adminPrivilegeAvailable: false, + adminUrl: 'https://admin.dev-overleaf.com', + adminDomains: ['overleaf.com'], + } this.TokenAccessHandler = { TOKEN_TYPES: { READ_ONLY: 'readOnly', @@ -48,6 +53,7 @@ describe('TokenAccessController', function () { this.SessionManager = { getLoggedInUserId: sinon.stub().returns(this.user._id), + getSessionUser: sinon.stub().returns(this.user._id), } this.AuthenticationController = { @@ -104,6 +110,18 @@ describe('TokenAccessController', function () { recordEventForSession: sinon.stub(), } + this.UserGetter = { + promises: { + getUser: sinon.stub().callsFake(async (userId, filter) => { + if (userId === this.userId) { + return this.user + } else { + return null + } + }), + }, + } + this.TokenAccessController = SandboxedModule.require(MODULE_PATH, { requires: { '@overleaf/settings': this.Settings, @@ -125,6 +143,7 @@ describe('TokenAccessController', function () { redirect: sinon.stub(), }), '../Analytics/AnalyticsManager': this.AnalyticsManager, + '../User/UserGetter': this.UserGetter, }, }) }) @@ -540,6 +559,58 @@ describe('TokenAccessController', function () { }) }) + describe('when user is admin', function () { + const admin = { _id: new ObjectId(), isAdmin: true } + beforeEach(function () { + this.SessionManager.getLoggedInUserId.returns(admin._id) + this.SessionManager.getSessionUser.returns(admin) + this.req.params = { token: this.token } + this.req.body = { confirmedByUser: true, tokenHashPrefix: '#prefix' } + }) + + it('redirects if project owner is non-admin', function () { + this.UserGetter.promises.getUserConfirmedEmails = sinon + .stub() + .resolves([{ email: 'test@not-overleaf.com' }]) + this.res.callback = () => { + expect(this.res.json).to.have.been.calledWith({ + redirect: `${this.Settings.adminUrl}/#prefix`, + }) + } + this.TokenAccessController.grantTokenAccessReadAndWrite( + this.req, + this.res + ) + }) + + it('grants access if project owner is an internal staff', function () { + const internalStaff = { _id: new ObjectId(), isAdmin: true } + const projectFromInternalStaff = { + _id: new ObjectId(), + name: 'test', + tokenAccessReadAndWrite_refs: [], + tokenAccessReadOnly_refs: [], + owner_ref: internalStaff._id, + } + this.UserGetter.promises.getUser = sinon.stub().resolves(internalStaff) + this.UserGetter.promises.getUserConfirmedEmails = sinon + .stub() + .resolves([{ email: 'test@overleaf.com' }]) + this.TokenAccessHandler.promises.getProjectByToken = sinon + .stub() + .resolves(projectFromInternalStaff) + this.res.callback = () => { + expect( + this.TokenAccessHandler.promises.addReadAndWriteUserToProject + ).to.have.been.calledWith(admin._id, projectFromInternalStaff._id) + } + this.TokenAccessController.grantTokenAccessReadAndWrite( + this.req, + this.res + ) + }) + }) + it('passes Errors.NotFoundError to next when token access is not enabled but still checks token hash', function (done) { this.TokenAccessHandler.tokenAccessEnabledForProject.returns(false) this.req.params = { token: this.token }