Merge pull request #15664 from overleaf/jel-token-hash-prefix

[web] Include more data when logging link sharing token hash mismatches

GitOrigin-RevId: 8f939cf3dd49497d5b21779e354ce5deff07edad

services/web/app/src/Features/TokenAccess/TokenAccessController.js

@@ -134,6 +134,7 @@ async function checkAndGetProjectOrResponseAction(
           anonWriteAccessDenied: true,
         })
       },
+      { action: 'denied anonymous read-and-write token access' },
     ]
   }
 
@@ -155,13 +156,15 @@ async function checkAndGetProjectOrResponseAction(
             res.sendStatus(404)
           }
         },
+        { action: v1ImportData ? 'import v1' : '404' },
       ]
     } else {
-      return [null, null]
+      return [null, null, { action: '404' }]
     }
   }
 
   const projectId = project._id
+
   const tokenAccessEnabled =
     TokenAccessHandler.tokenAccessEnabledForProject(project)
   if (isAnonymousUser && tokenAccessEnabled) {
@@ -177,6 +180,7 @@ async function checkAndGetProjectOrResponseAction(
               grantAnonymousAccess: tokenType,
             })
           },
+          { projectId, action: 'granting read-write anonymous access' },
         ]
       } else {
         // anonymous read-and-write token access should have been denied already
@@ -195,6 +199,7 @@ async function checkAndGetProjectOrResponseAction(
             grantAnonymousAccess: tokenType,
           })
         },
+        { projectId, action: 'granting read-only anonymous access' },
       ]
     } else {
       throw new Error('unreachable')
@@ -211,6 +216,7 @@ async function checkAndGetProjectOrResponseAction(
       () => {
         res.json({ redirect: `/project/${project._id}`, higherAccess: true })
       },
+      { projectId, action: 'user already has higher or same privilege' },
     ]
   }
   if (!tokenAccessEnabled) {
@@ -219,9 +225,10 @@ async function checkAndGetProjectOrResponseAction(
       () => {
         next(new Errors.NotFoundError())
       },
+      { projectId, action: 'token access not enabled' },
     ]
   }
-  return [project, null]
+  return [project, null, { projectId, action: 'continue' }]
 }
 
 async function grantTokenAccessReadAndWrite(req, res, next) {
@@ -234,7 +241,7 @@ async function grantTokenAccessReadAndWrite(req, res, next) {
   const tokenType = TokenAccessHandler.TOKEN_TYPES.READ_AND_WRITE
 
   try {
-    const [project, action] = await checkAndGetProjectOrResponseAction(
+    const [project, action, logData] = await checkAndGetProjectOrResponseAction(
       tokenType,
       token,
       userId,
@@ -248,7 +255,7 @@ async function grantTokenAccessReadAndWrite(req, res, next) {
       tokenHashPrefix,
       tokenType,
       userId,
-      project?._id
+      logData
     )
 
     if (action) {
@@ -312,7 +319,7 @@ async function grantTokenAccessReadOnly(req, res, next) {
     return res.json({ redirect: docPublishedInfo.published_path })
   }
   try {
-    const [project, action] = await checkAndGetProjectOrResponseAction(
+    const [project, action, logData] = await checkAndGetProjectOrResponseAction(
       tokenType,
       token,
       userId,
@@ -326,7 +333,7 @@ async function grantTokenAccessReadOnly(req, res, next) {
       tokenHashPrefix,
       tokenType,
       userId,
-      project?._id
+      logData
     )
 
     if (action) {

services/web/app/src/Features/TokenAccess/TokenAccessHandler.js

@@ -287,7 +287,7 @@ const TokenAccessHandler = {
     return hash.digest('hex').slice(0, 6)
   },
 
-  checkTokenHashPrefix(token, tokenHashPrefix, type, userId, projectId) {
+  checkTokenHashPrefix(token, tokenHashPrefix, type, userId, logData = {}) {
     let hashPrefixStatus
 
     if (tokenHashPrefix) {
@@ -307,7 +307,7 @@ const TokenAccessHandler = {
 
     if (hashPrefixStatus === 'mismatch') {
       logger.info(
-        { tokenHashPrefix, hashPrefixStatus, userId, projectId },
+        { tokenHashPrefix, hashPrefixStatus, userId, ...logData, type },
         'mismatched token hash prefix'
       )
     }

services/web/test/unit/src/TokenAccess/TokenAccessControllerTests.js

@@ -11,6 +11,7 @@ const MODULE_PATH =
 
 describe('TokenAccessController', function () {
   beforeEach(function () {
+    this.token = 'abc123'
     this.user = { _id: ObjectId() }
     this.project = {
       _id: ObjectId(),
@@ -30,11 +31,14 @@ describe('TokenAccessController', function () {
       isReadOnlyToken: sinon.stub().returns(true),
       tokenAccessEnabledForProject: sinon.stub().returns(true),
       checkTokenHashPrefix: sinon.stub(),
+      makeTokenUrl: sinon.stub().returns('/'),
+      grantSessionTokenAccess: sinon.stub(),
       promises: {
         addReadAndWriteUserToProject: sinon.stub().resolves(),
         addReadOnlyUserToProject: sinon.stub().resolves(),
         getProjectByToken: sinon.stub().resolves(this.project),
         getV1DocPublishedInfo: sinon.stub().resolves({ allow: true }),
+        getV1DocInfo: sinon.stub(),
       },
     }
 
@@ -42,7 +46,9 @@ describe('TokenAccessController', function () {
       getLoggedInUserId: sinon.stub().returns(this.user._id),
     }
 
-    this.AuthenticationController = {}
+    this.AuthenticationController = {
+      setRedirectInSession: sinon.stub(),
+    }
 
     this.AuthorizationManager = {
       promises: {
@@ -71,6 +77,7 @@ describe('TokenAccessController', function () {
         '../Authorization/AuthorizationMiddleware':
           this.AuthorizationMiddleware,
         '../Project/ProjectAuditLogHandler': this.ProjectAuditLogHandler,
+        '../Errors/Errors': (this.Errors = { NotFoundError: sinon.stub() }),
       },
     })
   })
@@ -114,7 +121,7 @@ describe('TokenAccessController', function () {
           'prefix',
           'readAndWrite',
           this.user._id,
-          this.project._id
+          { projectId: this.project._id, action: 'continue' }
         )
       })
     })
@@ -136,6 +143,18 @@ describe('TokenAccessController', function () {
         expect(this.ProjectAuditLogHandler.promises.addEntry).to.not.have.been
           .called
       })
+
+      it('checks token hash', function () {
+        expect(
+          this.TokenAccessHandler.checkTokenHashPrefix
+        ).to.have.been.calledWith(
+          this.token,
+          undefined,
+          'readAndWrite',
+          this.user._id,
+          { projectId: this.project._id, action: 'continue' }
+        )
+      })
     })
 
     describe('hash prefix missing in request', function () {
@@ -156,7 +175,34 @@ describe('TokenAccessController', function () {
         ).to.have.been.calledWith(this.user._id, this.project._id)
       })
 
-      it('sends missing hash to metrics', function () {
+      it('checks the hash prefix', function () {
+        expect(
+          this.TokenAccessHandler.checkTokenHashPrefix
+        ).to.have.been.calledWith(
+          this.token,
+          undefined,
+          'readAndWrite',
+          this.user._id,
+          { projectId: this.project._id, action: 'continue' }
+        )
+      })
+    })
+
+    describe('user is owner of project', function () {
+      beforeEach(function (done) {
+        this.AuthorizationManager.promises.getPrivilegeLevelForProject.returns(
+          PrivilegeLevels.OWNER
+        )
+        this.req.params = { token: this.token }
+        this.req.body = {}
+        this.res.callback = done
+        this.TokenAccessController.grantTokenAccessReadAndWrite(
+          this.req,
+          this.res,
+          done
+        )
+      })
+      it('checks token hash and includes log data', function () {
         expect(
           this.TokenAccessHandler.checkTokenHashPrefix
         ).to.have.been.calledWith(
@@ -164,10 +210,237 @@ describe('TokenAccessController', function () {
           undefined,
           'readAndWrite',
           this.user._id,
-          this.project._id
+          {
+            projectId: this.project._id,
+            action: 'user already has higher or same privilege',
+          }
+        )
+      })
+    })
+
+    describe('when user is not logged in', function () {
+      beforeEach(function () {
+        this.SessionManager.getLoggedInUserId.returns(null)
+        this.req.params = { token: this.token }
+        this.req.body = { tokenHashPrefix: 'prefix' }
+      })
+      describe('ANONYMOUS_READ_AND_WRITE_ENABLED is undefined', function () {
+        beforeEach(function (done) {
+          this.res.callback = done
+          this.TokenAccessController.grantTokenAccessReadAndWrite(
+            this.req,
+            this.res,
+            done
+          )
+        })
+
+        it('redirects to restricted', function () {
+          expect(this.res.json).to.have.been.calledWith({
+            redirect: '/restricted',
+            anonWriteAccessDenied: true,
+          })
+        })
+
+        it('checks the hash prefix and includes log data', function () {
+          expect(
+            this.TokenAccessHandler.checkTokenHashPrefix
+          ).to.have.been.calledWith(
+            this.token,
+            'prefix',
+            'readAndWrite',
+            null,
+            {
+              action: 'denied anonymous read-and-write token access',
+            }
+          )
+        })
+      })
+
+      describe('ANONYMOUS_READ_AND_WRITE_ENABLED is true', function () {
+        beforeEach(function (done) {
+          this.TokenAccessHandler.ANONYMOUS_READ_AND_WRITE_ENABLED = true
+          this.res.callback = done
+
+          this.TokenAccessController.grantTokenAccessReadAndWrite(
+            this.req,
+            this.res,
+            done
+          )
+        })
+
+        it('redirects to project', function () {
+          expect(this.res.json).to.have.been.calledWith({
+            redirect: `/project/${this.project._id}`,
+            grantAnonymousAccess: 'readAndWrite',
+          })
+        })
+
+        it('checks the hash prefix and includes log data', function () {
+          expect(
+            this.TokenAccessHandler.checkTokenHashPrefix
+          ).to.have.been.calledWith(
+            this.token,
+            'prefix',
+            'readAndWrite',
+            null,
+            {
+              projectId: this.project._id,
+              action: 'granting read-write anonymous access',
+            }
+          )
+        })
+      })
+    })
+
+    describe('when Overleaf SaaS', function () {
+      beforeEach(function () {
+        this.Settings.overleaf = {}
+      })
+      describe('when token is for v1 project', function () {
+        beforeEach(function (done) {
+          this.TokenAccessHandler.promises.getProjectByToken.resolves(undefined)
+          this.TokenAccessHandler.promises.getV1DocInfo.resolves({
+            exists: true,
+            has_owner: true,
+          })
+          this.req.params = { token: this.token }
+          this.req.body = { tokenHashPrefix: 'prefix' }
+          this.res.callback = done
+          this.TokenAccessController.grantTokenAccessReadAndWrite(
+            this.req,
+            this.res,
+            done
+          )
+        })
+        it('returns v1 import data', function () {
+          expect(this.res.json).to.have.been.calledWith({
+            v1Import: {
+              status: 'canDownloadZip',
+              projectId: this.token,
+              hasOwner: true,
+              name: 'Untitled',
+              brandInfo: undefined,
+            },
+          })
+        })
+        it('checks the hash prefix and includes log data', function () {
+          expect(
+            this.TokenAccessHandler.checkTokenHashPrefix
+          ).to.have.been.calledWith(
+            this.token,
+            'prefix',
+            'readAndWrite',
+            this.user._id,
+            {
+              action: 'import v1',
+            }
+          )
+        })
+      })
+
+      describe('when token is not for a v1 or v2 project', function () {
+        beforeEach(function (done) {
+          this.TokenAccessHandler.promises.getProjectByToken.resolves(undefined)
+          this.TokenAccessHandler.promises.getV1DocInfo.resolves({
+            exists: false,
+          })
+          this.req.params = { token: this.token }
+          this.req.body = { tokenHashPrefix: 'prefix' }
+          this.res.callback = done
+          this.TokenAccessController.grantTokenAccessReadAndWrite(
+            this.req,
+            this.res,
+            done
+          )
+        })
+        it('returns 404', function () {
+          expect(this.res.sendStatus).to.have.been.calledWith(404)
+        })
+        it('checks the hash prefix and includes log data', function () {
+          expect(
+            this.TokenAccessHandler.checkTokenHashPrefix
+          ).to.have.been.calledWith(
+            this.token,
+            'prefix',
+            'readAndWrite',
+            this.user._id,
+            {
+              action: '404',
+            }
+          )
+        })
+      })
+    })
+
+    describe('not Overleaf SaaS', function () {
+      beforeEach(function () {
+        this.TokenAccessHandler.promises.getProjectByToken.resolves(undefined)
+        this.req.params = { token: this.token }
+        this.req.body = { tokenHashPrefix: 'prefix' }
+      })
+      it('passes Errors.NotFoundError to next when project not found and still checks token hash', function (done) {
+        this.TokenAccessController.grantTokenAccessReadAndWrite(
+          this.req,
+          this.res,
+          args => {
+            expect(args).to.be.instanceof(this.Errors.NotFoundError)
+
+            expect(
+              this.TokenAccessHandler.checkTokenHashPrefix
+            ).to.have.been.calledWith(
+              this.token,
+              'prefix',
+              'readAndWrite',
+              this.user._id,
+              {
+                action: '404',
+              }
+            )
+
+            done()
+          }
         )
       })
     })
+
+    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 }
+      this.req.body = { tokenHashPrefix: 'prefix' }
+      this.TokenAccessController.grantTokenAccessReadAndWrite(
+        this.req,
+        this.res,
+        args => {
+          expect(args).to.be.instanceof(this.Errors.NotFoundError)
+
+          expect(
+            this.TokenAccessHandler.checkTokenHashPrefix
+          ).to.have.been.calledWith(
+            this.token,
+            'prefix',
+            'readAndWrite',
+            this.user._id,
+            {
+              projectId: this.project._id,
+              action: 'token access not enabled',
+            }
+          )
+
+          done()
+        }
+      )
+    })
+
+    it('returns 400 when not using a read write token', function () {
+      this.TokenAccessHandler.isReadAndWriteToken.returns(false)
+      this.req.params = { token: this.token }
+      this.req.body = { tokenHashPrefix: 'prefix' }
+      this.TokenAccessController.grantTokenAccessReadAndWrite(
+        this.req,
+        this.res
+      )
+      expect(this.res.sendStatus).to.have.been.calledWith(400)
+    })
   })
 
   describe('grantTokenAccessReadOnly', function () {
@@ -209,7 +482,7 @@ describe('TokenAccessController', function () {
           'prefix',
           'readOnly',
           this.user._id,
-          this.project._id
+          { projectId: this.project._id, action: 'continue' }
         )
       })
     })
@@ -240,9 +513,103 @@ describe('TokenAccessController', function () {
           undefined,
           'readOnly',
           this.user._id,
-          this.project._id
+          { projectId: this.project._id, action: 'continue' }
+        )
+      })
+    })
+
+    it('returns 400 when not using a read only token', function () {
+      this.TokenAccessHandler.isReadOnlyToken.returns(false)
+      this.req.params = { token: this.token }
+      this.req.body = { tokenHashPrefix: 'prefix' }
+      this.TokenAccessController.grantTokenAccessReadOnly(this.req, this.res)
+      expect(this.res.sendStatus).to.have.been.calledWith(400)
+    })
+
+    describe('anonymous users', function () {
+      beforeEach(function (done) {
+        this.req.params = { token: this.token }
+        this.SessionManager.getLoggedInUserId.returns(null)
+        this.res.callback = done
+
+        this.TokenAccessController.grantTokenAccessReadOnly(
+          this.req,
+          this.res,
+          done
+        )
+      })
+
+      it('allows anonymous users and checks the token hash', function () {
+        expect(this.res.json).to.have.been.calledWith({
+          redirect: `/project/${this.project._id}`,
+          grantAnonymousAccess: 'readOnly',
+        })
+
+        expect(
+          this.TokenAccessHandler.checkTokenHashPrefix
+        ).to.have.been.calledWith(this.token, undefined, 'readOnly', null, {
+          projectId: this.project._id,
+          action: 'granting read-only anonymous access',
+        })
+      })
+    })
+
+    describe('user is owner of project', function () {
+      beforeEach(function (done) {
+        this.AuthorizationManager.promises.getPrivilegeLevelForProject.returns(
+          PrivilegeLevels.OWNER
+        )
+        this.req.params = { token: this.token }
+        this.req.body = {}
+        this.res.callback = done
+        this.TokenAccessController.grantTokenAccessReadOnly(
+          this.req,
+          this.res,
+          done
+        )
+      })
+      it('checks token hash and includes log data', function () {
+        expect(
+          this.TokenAccessHandler.checkTokenHashPrefix
+        ).to.have.been.calledWith(
+          this.token,
+          undefined,
+          'readOnly',
+          this.user._id,
+          {
+            projectId: this.project._id,
+            action: 'user already has higher or same privilege',
+          }
         )
       })
     })
+
+    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 }
+      this.req.body = { tokenHashPrefix: 'prefix' }
+      this.TokenAccessController.grantTokenAccessReadOnly(
+        this.req,
+        this.res,
+        args => {
+          expect(args).to.be.instanceof(this.Errors.NotFoundError)
+
+          expect(
+            this.TokenAccessHandler.checkTokenHashPrefix
+          ).to.have.been.calledWith(
+            this.token,
+            'prefix',
+            'readOnly',
+            this.user._id,
+            {
+              projectId: this.project._id,
+              action: 'token access not enabled',
+            }
+          )
+
+          done()
+        }
+      )
+    })
   })
 })

services/web/test/unit/src/TokenAccess/TokenAccessHandlerTests.js

@@ -680,7 +680,7 @@ describe('TokenAccessHandler', function () {
         `#${prefix}`,
         'readOnly',
         userId,
-        projectId
+        { projectId }
       )
 
       expect(this.Metrics.inc).to.have.been.calledWith(
@@ -696,6 +696,7 @@ describe('TokenAccessHandler', function () {
           hashPrefixStatus: 'mismatch',
           userId,
           projectId,
+          type: 'readOnly',
         },
         'mismatched token hash prefix'
       )