diff --git a/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js b/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js index f57bc7ce15..b95eae47c0 100644 --- a/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js +++ b/services/web/app/src/Features/Authorization/AuthorizationMiddleware.js @@ -7,6 +7,19 @@ const AuthenticationController = require('../Authentication/AuthenticationContro const SessionManager = require('../Authentication/SessionManager') const TokenAccessHandler = require('../TokenAccess/TokenAccessHandler') const { expressify } = require('../../util/promises') +const { + shouldRedirectToAdminDomain, +} = require('../Helpers/AdminAuthorizationHelper') +const { getSafeAdminDomainRedirect } = require('../Helpers/UrlHelper') + +function handleAdminDomainRedirect(req, res) { + if (shouldRedirectToAdminDomain(SessionManager.getSessionUser(req.session))) { + logger.warn({ req }, 'redirecting admin user to admin domain') + res.redirect(getSafeAdminDomainRedirect(req.originalUrl)) + return true + } + return false +} async function ensureUserCanReadMultipleProjects(req, res, next) { const projectIds = (req.query.project_ids || '').split(',') @@ -137,6 +150,7 @@ async function ensureUserIsSiteAdmin(req, res, next) { logger.log({ userId }, 'allowing user admin access to site') return next() } + if (handleAdminDomainRedirect(req, res)) return logger.log({ userId }, 'denying user admin access to site') _redirectToRestricted(req, res, next) } @@ -191,5 +205,6 @@ module.exports = { ), ensureUserCanAdminProject: expressify(ensureUserCanAdminProject), ensureUserIsSiteAdmin: expressify(ensureUserIsSiteAdmin), + handleAdminDomainRedirect, restricted, } diff --git a/services/web/app/src/Features/Helpers/AdminAuthorizationHelper.js b/services/web/app/src/Features/Helpers/AdminAuthorizationHelper.js index 3330d1f1b8..585cfce9e9 100644 --- a/services/web/app/src/Features/Helpers/AdminAuthorizationHelper.js +++ b/services/web/app/src/Features/Helpers/AdminAuthorizationHelper.js @@ -2,7 +2,7 @@ const Settings = require('@overleaf/settings') module.exports = { hasAdminAccess, - shouldRedirectToAdminPanel, + shouldRedirectToAdminDomain, } function hasAdminAccess(user) { @@ -11,8 +11,9 @@ function hasAdminAccess(user) { return Boolean(user.isAdmin) } -function shouldRedirectToAdminPanel(user) { +function shouldRedirectToAdminDomain(user) { if (Settings.adminPrivilegeAvailable) return false + if (!Settings.adminUrl) return false if (!user) return false return Boolean(user.isAdmin) } diff --git a/services/web/app/src/Features/Helpers/UrlHelper.js b/services/web/app/src/Features/Helpers/UrlHelper.js index 38d825e1e0..cf686636b8 100644 --- a/services/web/app/src/Features/Helpers/UrlHelper.js +++ b/services/web/app/src/Features/Helpers/UrlHelper.js @@ -24,9 +24,14 @@ function getSafeRedirectPath(value) { return safePath } -const UrlHelper = { +function getSafeAdminDomainRedirect(path) { + return Settings.adminUrl + (getSafeRedirectPath(path) || '/') +} + +module.exports = { getCanonicalURL, getSafeRedirectPath, + getSafeAdminDomainRedirect, wrapUrlWithProxy(url) { // TODO: Consider what to do for Community and Enterprise edition? if (!Settings.apis.linkedUrlProxy.url) { @@ -42,5 +47,3 @@ const UrlHelper = { return url }, } - -module.exports = UrlHelper diff --git a/services/web/app/src/Features/TokenAccess/TokenAccessController.js b/services/web/app/src/Features/TokenAccess/TokenAccessController.js index e57f1b5c5e..8ecd1ba965 100644 --- a/services/web/app/src/Features/TokenAccess/TokenAccessController.js +++ b/services/web/app/src/Features/TokenAccess/TokenAccessController.js @@ -9,8 +9,8 @@ const { expressify } = require('../../util/promises') const AuthorizationManager = require('../Authorization/AuthorizationManager') const PrivilegeLevels = require('../Authorization/PrivilegeLevels') const { - shouldRedirectToAdminPanel, -} = require('../Helpers/AdminAuthorizationHelper') + handleAdminDomainRedirect, +} = require('../Authorization/AuthorizationMiddleware') const orderedPrivilegeLevels = [ PrivilegeLevels.NONE, @@ -86,11 +86,9 @@ async function tokenAccessPage(req, res, next) { if (!TokenAccessHandler.isValidToken(token)) { return next(new Errors.NotFoundError()) } - if (shouldRedirectToAdminPanel(SessionManager.getSessionUser(req.session))) { - const path = TokenAccessHandler.isReadOnlyToken(token) - ? `/read/${token}` - : `/${token}` - return res.redirect(settings.adminUrl + path) + if (handleAdminDomainRedirect(req, res)) { + // Admin users do not join the project, but view it on the admin domain. + return } try { if (TokenAccessHandler.isReadOnlyToken(token)) { diff --git a/services/web/test/acceptance/src/AuthorizationTests.js b/services/web/test/acceptance/src/AuthorizationTests.js index ecacf65f72..f700909f80 100644 --- a/services/web/test/acceptance/src/AuthorizationTests.js +++ b/services/web/test/acceptance/src/AuthorizationTests.js @@ -3,6 +3,7 @@ const async = require('async') const User = require('./helpers/User') const request = require('./helpers/request') const settings = require('@overleaf/settings') +const Features = require('../../../app/src/infrastructure/Features') const expectErrorResponse = require('./helpers/expectErrorResponse') @@ -75,7 +76,7 @@ function trySettingsWriteAccess(user, projectId, test, callback) { ) } -function tryAdminAccess(user, projectId, test, callback) { +function tryProjectAdminAccess(user, projectId, test, callback) { async.series( [ cb => @@ -115,6 +116,44 @@ function tryAdminAccess(user, projectId, test, callback) { ) } +function tryAdminAccess(user, test, callback) { + async.series( + [ + cb => + user.request.get( + { + uri: '/admin', + }, + (error, response, body) => { + if (error != null) { + return cb(error) + } + test(response, body) + cb() + } + ), + cb => { + if (!Features.hasFeature('saas')) { + return cb() + } + user.request.get( + { + uri: `/admin/user/${user._id}`, + }, + (error, response, body) => { + if (error != null) { + return cb(error) + } + test(response, body) + cb() + } + ) + }, + ], + callback + ) +} + function tryContentAccess(user, projectId, test, callback) { // The real-time service calls this end point to determine the user's // permissions. @@ -146,6 +185,27 @@ function tryContentAccess(user, projectId, test, callback) { ) } +function expectAdminAccess(user, callback) { + tryAdminAccess( + user, + response => expect(response.statusCode).to.be.oneOf([200, 204]), + callback + ) +} + +function expectRedirectedAdminAccess(user, callback) { + tryAdminAccess( + user, + response => { + expect(response.statusCode).to.equal(302) + expect(response.headers.location).to.equal( + settings.adminUrl + response.request.uri.pathname + ) + }, + callback + ) +} + function expectReadAccess(user, projectId, callback) { async.series( [ @@ -204,8 +264,8 @@ function expectSettingsWriteAccess(user, projectId, callback) { ) } -function expectAdminAccess(user, projectId, callback) { - tryAdminAccess( +function expectProjectAdminAccess(user, projectId, callback) { + tryProjectAdminAccess( user, projectId, (response, body) => expect(response.statusCode).to.be.oneOf([200, 204]), @@ -261,8 +321,8 @@ function expectNoRenameProjectAccess(user, projectId, callback) { ) } -function expectNoAdminAccess(user, projectId, callback) { - tryAdminAccess( +function expectNoProjectAdminAccess(user, projectId, callback) { + tryProjectAdminAccess( user, projectId, (response, body) => { @@ -272,8 +332,8 @@ function expectNoAdminAccess(user, projectId, callback) { ) } -function expectNoAnonymousAdminAccess(user, projectId, callback) { - tryAdminAccess( +function expectNoAnonymousProjectAdminAccess(user, projectId, callback) { + tryProjectAdminAccess( user, projectId, expectErrorResponse.requireLogin.json, @@ -328,11 +388,14 @@ describe('Authorization', function () { cb => this.other2.login(cb), cb => this.anon.getCsrfToken(cb), cb => { - this.site_admin.login(err => { - if (err != null) { - return cb(err) - } - return this.site_admin.ensureAdmin(cb) + this.site_admin.ensureUserExists(err => { + if (err) return cb(err) + this.site_admin.ensureAdmin(err => { + if (err != null) { + return cb(err) + } + return this.site_admin.login(cb) + }) }) }, ], @@ -367,8 +430,8 @@ describe('Authorization', function () { expectRenameProjectAccess(this.owner, this.projectId, done) }) - it('should allow the owner admin access to it', function (done) { - expectAdminAccess(this.owner, this.projectId, done) + it('should allow the owner project admin access to it', function (done) { + expectProjectAdminAccess(this.owner, this.projectId, done) }) it('should allow the owner user chat messages access', function (done) { @@ -391,8 +454,8 @@ describe('Authorization', function () { expectNoRenameProjectAccess(this.other1, this.projectId, done) }) - it('should not allow another user admin access to it', function (done) { - expectNoAdminAccess(this.other1, this.projectId, done) + it('should not allow another user project admin access to it', function (done) { + expectNoProjectAdminAccess(this.other1, this.projectId, done) }) it('should not allow another user chat messages access', function (done) { @@ -415,32 +478,75 @@ describe('Authorization', function () { expectNoRenameProjectAccess(this.anon, this.projectId, done) }) - it('should not allow anonymous user admin access to it', function (done) { - expectNoAnonymousAdminAccess(this.anon, this.projectId, done) + it('should not allow anonymous user project admin access to it', function (done) { + expectNoAnonymousProjectAdminAccess(this.anon, this.projectId, done) }) it('should not allow anonymous user chat messages access', function (done) { expectNoChatAccess(this.anon, this.projectId, done) }) - it('should allow site admin users read access to it', function (done) { - expectReadAccess(this.site_admin, this.projectId, done) + describe('with admin privilege available', function () { + beforeEach(function () { + settings.adminPrivilegeAvailable = true + }) + + it('should allow site admin users read access to it', function (done) { + expectReadAccess(this.site_admin, this.projectId, done) + }) + + it('should allow site admin users write access to its content', function (done) { + expectContentWriteAccess(this.site_admin, this.projectId, done) + }) + + it('should allow site admin users write access to its settings', function (done) { + expectSettingsWriteAccess(this.site_admin, this.projectId, done) + }) + + it('should allow site admin users to rename the project', function (done) { + expectRenameProjectAccess(this.site_admin, this.projectId, done) + }) + + it('should allow site admin users project admin access to it', function (done) { + expectProjectAdminAccess(this.site_admin, this.projectId, done) + }) + + it('should allow site admin users site admin access to site admin endpoints', function (done) { + expectAdminAccess(this.site_admin, done) + }) }) - it('should allow site admin users write access to its content', function (done) { - expectContentWriteAccess(this.site_admin, this.projectId, done) - }) + describe('with admin privilege unavailable', function () { + beforeEach(function () { + settings.adminPrivilegeAvailable = false + }) + afterEach(function () { + settings.adminPrivilegeAvailable = true + }) - it('should allow site admin users write access to its settings', function (done) { - expectSettingsWriteAccess(this.site_admin, this.projectId, done) - }) + it('should not allow site admin users read access to it', function (done) { + expectNoReadAccess(this.site_admin, this.projectId, done) + }) - it('should allow site admin users to rename the project', function (done) { - expectRenameProjectAccess(this.site_admin, this.projectId, done) - }) + it('should not allow site admin users write access to its content', function (done) { + expectNoContentWriteAccess(this.site_admin, this.projectId, done) + }) - it('should allow site admin users admin access to it', function (done) { - expectAdminAccess(this.site_admin, this.projectId, done) + it('should not allow site admin users write access to its settings', function (done) { + expectNoSettingsWriteAccess(this.site_admin, this.projectId, done) + }) + + it('should not allow site admin users to rename the project', function (done) { + expectNoRenameProjectAccess(this.site_admin, this.projectId, done) + }) + + it('should not allow site admin users project admin access to it', function (done) { + expectNoProjectAdminAccess(this.site_admin, this.projectId, done) + }) + + it('should redirect site admin users when accessing site admin endpoints', function (done) { + expectRedirectedAdminAccess(this.site_admin, done) + }) }) }) @@ -497,8 +603,8 @@ describe('Authorization', function () { expectNoRenameProjectAccess(this.ro_user, this.projectId, done) }) - it('should not allow the read-only user admin access to it', function (done) { - expectNoAdminAccess(this.ro_user, this.projectId, done) + it('should not allow the read-only user project admin access to it', function (done) { + expectNoProjectAdminAccess(this.ro_user, this.projectId, done) }) it('should allow the read-write user read access to it', function (done) { @@ -517,8 +623,8 @@ describe('Authorization', function () { expectNoRenameProjectAccess(this.rw_user, this.projectId, done) }) - it('should not allow the read-write user admin access to it', function (done) { - expectNoAdminAccess(this.rw_user, this.projectId, done) + it('should not allow the read-write user project admin access to it', function (done) { + expectNoProjectAdminAccess(this.rw_user, this.projectId, done) }) it('should allow the read-write user chat messages access', function (done) { @@ -557,8 +663,8 @@ describe('Authorization', function () { expectNoRenameProjectAccess(this.other1, this.projectId, done) }) - it('should not allow a user admin access to it', function (done) { - expectNoAdminAccess(this.other1, this.projectId, done) + it('should not allow a user project admin access to it', function (done) { + expectNoProjectAdminAccess(this.other1, this.projectId, done) }) it('should allow an anonymous user read access to it', function (done) { @@ -581,8 +687,8 @@ describe('Authorization', function () { expectNoRenameProjectAccess(this.anon, this.projectId, done) }) - it('should not allow an anonymous user admin access to it', function (done) { - expectNoAnonymousAdminAccess(this.anon, this.projectId, done) + it('should not allow an anonymous user project admin access to it', function (done) { + expectNoAnonymousProjectAdminAccess(this.anon, this.projectId, done) }) }) @@ -613,8 +719,8 @@ describe('Authorization', function () { expectNoRenameProjectAccess(this.other1, this.projectId, done) }) - it('should not allow a user admin access to it', function (done) { - expectNoAdminAccess(this.other1, this.projectId, done) + it('should not allow a user project admin access to it', function (done) { + expectNoProjectAdminAccess(this.other1, this.projectId, done) }) // NOTE: legacy readOnly access does not count as 'restricted' in the new model @@ -638,8 +744,8 @@ describe('Authorization', function () { expectNoRenameProjectAccess(this.anon, this.projectId, done) }) - it('should not allow an anonymous user admin access to it', function (done) { - expectNoAnonymousAdminAccess(this.anon, this.projectId, done) + it('should not allow an anonymous user project admin access to it', function (done) { + expectNoAnonymousProjectAdminAccess(this.anon, this.projectId, done) }) it('should not allow an anonymous user chat messages access', function (done) { diff --git a/services/web/test/unit/src/Authorization/AuthorizationMiddlewareTests.js b/services/web/test/unit/src/Authorization/AuthorizationMiddlewareTests.js index 0509cf6bac..f790c30cc2 100644 --- a/services/web/test/unit/src/Authorization/AuthorizationMiddlewareTests.js +++ b/services/web/test/unit/src/Authorization/AuthorizationMiddlewareTests.js @@ -14,6 +14,7 @@ describe('AuthorizationMiddleware', function () { this.token = 'some-token' this.AuthenticationController = {} this.SessionManager = { + getSessionUser: sinon.stub().returns(null), getLoggedInUserId: sinon.stub().returns(this.userId), isUserLoggedIn: sinon.stub().returns(true), } @@ -42,6 +43,9 @@ describe('AuthorizationMiddleware', function () { this.AuthenticationController, '../Authentication/SessionManager': this.SessionManager, '../TokenAccess/TokenAccessHandler': this.TokenAccessHandler, + '../Helpers/AdminAuthorizationHelper': { + shouldRedirectToAdminDomain: sinon.stub().returns(false), + }, }, }) this.req = {