Merge pull request #7094 from overleaf/jpa-redirect-admin-requests

[web] redirect admin users from admin endpoints to the admin domain

GitOrigin-RevId: a4bd7d4f998615efcb46ae9866868af9489c94f5

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,
 }

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)
 }

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

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)) {

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 => {
+          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.ensureAdmin(cb)
+              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,14 +478,19 @@ 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)
     })
 
+    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)
       })
@@ -439,8 +507,46 @@ describe('Authorization', function () {
         expectRenameProjectAccess(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 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)
+      })
+    })
+
+    describe('with admin privilege unavailable', function () {
+      beforeEach(function () {
+        settings.adminPrivilegeAvailable = false
+      })
+      afterEach(function () {
+        settings.adminPrivilegeAvailable = true
+      })
+
+      it('should not allow site admin users read access to it', function (done) {
+        expectNoReadAccess(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 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) {

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 = {