From 45c9d805ddcb1b2da542f8db270a0f11b6c2c6b8 Mon Sep 17 00:00:00 2001
From: Jakob Ackermann <jakob.ackermann@overleaf.com>
Date: Mon, 4 Sep 2023 10:11:00 +0200
Subject: [PATCH] Merge pull request #14606 from overleaf/jpa-bcrypt-metrics

[web] add metrics for bcrypt operations

GitOrigin-RevId: 42bf9bedb84295ceea7f660f1daac3adb7b853d9
---
 .../Authentication/AuthenticationManager.js   | 11 +++++++++
 .../AuthenticationManagerTests.js             | 24 ++++++++++---------
 2 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/services/web/app/src/Features/Authentication/AuthenticationManager.js b/services/web/app/src/Features/Authentication/AuthenticationManager.js
index 7e730c2484..275b028413 100644
--- a/services/web/app/src/Features/Authentication/AuthenticationManager.js
+++ b/services/web/app/src/Features/Authentication/AuthenticationManager.js
@@ -70,6 +70,10 @@ const AuthenticationManager = {
       if (!user || !user.hashedPassword) {
         return callback(null, null, null)
       }
+      Metrics.inc('bcrypt', 1, {
+        method: 'compare',
+        path: bcrypt.getRounds(user.hashedPassword),
+      })
       bcrypt.compare(password, user.hashedPassword, function (error, match) {
         if (error) {
           return callback(error)
@@ -255,13 +259,16 @@ const AuthenticationManager = {
   checkRounds(user, hashedPassword, password, callback) {
     // Temporarily disable this function, TODO: re-enable this
     if (Settings.security.disableBcryptRoundsUpgrades) {
+      Metrics.inc('bcrypt_check_rounds', 1, { status: 'disabled' })
       return callback()
     }
     // check current number of rounds and rehash if necessary
     const currentRounds = bcrypt.getRounds(hashedPassword)
     if (currentRounds < BCRYPT_ROUNDS) {
+      Metrics.inc('bcrypt_check_rounds', 1, { status: 'upgrade' })
       AuthenticationManager._setUserPasswordInMongo(user, password, callback)
     } else {
+      Metrics.inc('bcrypt_check_rounds', 1, { status: 'success' })
       callback()
     }
   },
@@ -276,6 +283,10 @@ const AuthenticationManager = {
       if (error) {
         return callback(error)
       }
+      Metrics.inc('bcrypt', 1, {
+        method: 'hash',
+        path: BCRYPT_ROUNDS,
+      })
       bcrypt.hash(password, salt, callback)
     })
   },
diff --git a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js
index 68fc1e0f7d..52041d8795 100644
--- a/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js
+++ b/services/web/test/unit/src/Authentication/AuthenticationManagerTests.js
@@ -24,7 +24,9 @@ describe('AuthenticationManager', function () {
           db: (this.db = { users: {} }),
           ObjectId,
         },
-        bcrypt: (this.bcrypt = {}),
+        bcrypt: (this.bcrypt = {
+          getRounds: sinon.stub().returns(4),
+        }),
         '@overleaf/settings': this.settings,
         '../User/UserGetter': (this.UserGetter = {}),
         './AuthenticationErrors': AuthenticationErrors,
@@ -137,10 +139,6 @@ describe('AuthenticationManager', function () {
         it('should not return the user', function () {
           this.callback.calledWith(null, null).should.equal(true)
         })
-
-        it('should not send metrics', function () {
-          expect(this.metrics.inc.called).to.equal(false)
-        })
       })
 
       describe('when another request runs in parallel', function () {
@@ -307,10 +305,6 @@ describe('AuthenticationManager', function () {
           )
         })
 
-        it('should not send metrics', function () {
-          expect(this.metrics.inc.called).to.equal(false)
-        })
-
         it('should not return the user', function () {
           this.callback.calledWith(null, null).should.equal(true)
           this.UserAuditLogHandler.addEntry.callCount.should.equal(0)
@@ -374,7 +368,11 @@ describe('AuthenticationManager', function () {
         })
 
         it('should check the number of rounds', function () {
-          this.bcrypt.getRounds.called.should.equal(true)
+          expect(this.metrics.inc).to.have.been.calledWith(
+            'bcrypt_check_rounds',
+            1,
+            { status: 'upgrade' }
+          )
         })
 
         it('should set the users password (with a higher number of rounds)', function () {
@@ -408,7 +406,11 @@ describe('AuthenticationManager', function () {
         })
 
         it('should not check the number of rounds', function () {
-          this.bcrypt.getRounds.called.should.equal(false)
+          expect(this.metrics.inc).to.have.been.calledWith(
+            'bcrypt_check_rounds',
+            1,
+            { status: 'disabled' }
+          )
         })
 
         it('should not set the users password (with a higher number of rounds)', function () {