remove v1 oauth (#1846)

remove oauth token v1 fallback

GitOrigin-RevId: 621e75024d8ae877c821b9bfed9b2a19fdbbf9f7

services/web/app/src/Features/Authentication/AuthenticationController.js

@@ -1,13 +1,3 @@
-/* eslint-disable
-    camelcase,
-    handle-callback-err,
-    max-len,
-    no-return-assign,
-    no-undef,
-    no-unused-vars,
-*/
-// TODO: This file was created by bulk-decaffeinate.
-// Fix any style issues and re-enable lint.
 /*
  * decaffeinate suggestions:
  * DS101: Remove unnecessary use of Array.from
@@ -17,14 +7,12 @@
  * DS207: Consider shorter variations of null checks
  * Full docs: https://github.com/decaffeinate/decaffeinate/blob/master/docs/suggestions.md
  */
-let AuthenticationController
 const AuthenticationManager = require('./AuthenticationManager')
 const LoginRateLimiter = require('../Security/LoginRateLimiter')
 const UserUpdater = require('../User/UserUpdater')
 const Metrics = require('metrics-sharelatex')
 const logger = require('logger-sharelatex')
 const querystring = require('querystring')
-const Url = require('url')
 const Settings = require('settings-sharelatex')
 const basicAuth = require('basic-auth-connect')
 const UserHandler = require('../User/UserHandler')
@@ -33,11 +21,9 @@ const Analytics = require('../Analytics/AnalyticsManager')
 const passport = require('passport')
 const NotificationsBuilder = require('../Notifications/NotificationsBuilder')
 const SudoModeHandler = require('../SudoMode/SudoModeHandler')
-const V1Api = require('../V1/V1Api')
-const { User } = require('../../models/User')
 const { URL } = require('url')
 
-module.exports = AuthenticationController = {
+const AuthenticationController = (module.exports = {
   serializeUser(user, callback) {
     const lightUser = {
       _id: user._id,
@@ -61,7 +47,7 @@ module.exports = AuthenticationController = {
 
   afterLoginSessionSetup(req, user, callback) {
     if (callback == null) {
-      callback = function(err) {}
+      callback = function() {}
     }
     return req.login(user, function(err) {
       if (err != null) {
@@ -173,7 +159,7 @@ module.exports = AuthenticationController = {
       infoList
     ) {
       if (err != null) {
-        return next(err)
+        return done(err)
       }
       const info = infoList.find(i => i != null)
       if (info != null) {
@@ -268,8 +254,8 @@ module.exports = AuthenticationController = {
   },
 
   isUserLoggedIn(req) {
-    const user_id = AuthenticationController.getLoggedInUserId(req)
-    return ![null, undefined, false].includes(user_id)
+    const userId = AuthenticationController.getLoggedInUserId(req)
+    return ![null, undefined, false].includes(userId)
   },
 
   // TODO: perhaps should produce an error if the current user is not present
@@ -309,7 +295,7 @@ module.exports = AuthenticationController = {
   requireLogin() {
     const doRequest = function(req, res, next) {
       if (next == null) {
-        next = function(error) {}
+        next = function() {}
       }
       if (!AuthenticationController.isUserLoggedIn(req)) {
         return AuthenticationController._redirectToLoginOrRegisterPage(req, res)
@@ -333,7 +319,7 @@ module.exports = AuthenticationController = {
     const Oauth2Server = require('../../../../modules/oauth2-server/app/src/Oauth2Server')
     return function(req, res, next) {
       if (next == null) {
-        next = function(error) {}
+        next = function() {}
       }
       const request = new Oauth2Server.Request(req)
       const response = new Oauth2Server.Response(res)
@@ -349,14 +335,6 @@ module.exports = AuthenticationController = {
           ) {
             err.code = 401
           }
-          // fall back to v1 on invalid token
-          if (err.code === 401) {
-            return AuthenticationController._requireOauthV1Fallback(
-              req,
-              res,
-              next
-            )
-          }
           // send all other errors
           return res
             .status(err.code)
@@ -373,42 +351,6 @@ module.exports = AuthenticationController = {
     }
   },
 
-  _requireOauthV1Fallback(req, res, next) {
-    if (req.token == null) {
-      return res.sendStatus(401)
-    }
-    const options = {
-      expectedStatusCodes: [401],
-      json: {
-        token: req.token
-      },
-      method: 'POST',
-      uri: '/api/v1/sharelatex/oauth_authorize'
-    }
-    return V1Api.request(options, function(error, response, body) {
-      if (error != null) {
-        return next(error)
-      }
-      if (!__guard__(body != null ? body.user_profile : undefined, x => x.id)) {
-        return res.status(401).json({ error: 'invalid_token' })
-      }
-      return User.findOne({ 'overleaf.id': body.user_profile.id }, function(
-        error,
-        user
-      ) {
-        if (error != null) {
-          return next(error)
-        }
-        if (user == null) {
-          return res.status(401).json({ error: 'invalid_token' })
-        }
-        req.oauth = { access_token: body.access_token }
-        req.oauth_user = user
-        return next()
-      })
-    })
-  },
-
   _globalLoginWhitelist: [],
   addEndpointToLoginWhitelist(endpoint) {
     return AuthenticationController._globalLoginWhitelist.push(endpoint)
@@ -514,12 +456,12 @@ module.exports = AuthenticationController = {
     return Metrics.inc('security.login-redirect')
   },
 
-  _recordSuccessfulLogin(user_id, callback) {
+  _recordSuccessfulLogin(userId, callback) {
     if (callback == null) {
-      callback = function(error) {}
+      callback = function() {}
     }
     return UserUpdater.updateUser(
-      user_id.toString(),
+      userId.toString(),
       {
         $set: { lastLoggedIn: new Date() },
         $inc: { loginCount: 1 }
@@ -535,11 +477,8 @@ module.exports = AuthenticationController = {
   },
 
   _recordFailedLogin(callback) {
-    if (callback == null) {
-      callback = function(error) {}
-    }
     Metrics.inc('user.login.failed')
-    return callback()
+    if (callback) callback()
   },
 
   _getRedirectFromSession(req) {
@@ -569,7 +508,7 @@ module.exports = AuthenticationController = {
     }
     return safePath
   }
-}
+})
 
 function __guard__(value, transform) {
   return typeof value !== 'undefined' && value !== null

services/web/test/unit/src/Authentication/AuthenticationControllerTests.js

@@ -69,7 +69,6 @@ describe('AuthenticationController', function() {
         '../Notifications/NotificationsBuilder': (this.NotificationsBuilder = {
           ipMatcherAffiliation: sinon.stub()
         }),
-        '../V1/V1Api': (this.V1Api = { request: sinon.stub() }),
         '../../models/User': { User: this.UserModel },
         '../../../../modules/oauth2-server/app/src/Oauth2Server': (this.Oauth2Server = {
           Request: sinon.stub(),
@@ -91,7 +90,8 @@ describe('AuthenticationController', function() {
     this.password = 'banana'
     this.req = new MockRequest()
     this.res = new MockResponse()
-    return (this.callback = this.next = sinon.stub())
+    this.callback = sinon.stub()
+    this.next = sinon.stub()
   })
 
   afterEach(() => tk.reset())
@@ -623,11 +623,10 @@ describe('AuthenticationController', function() {
 
   describe('requireOauth', function() {
     beforeEach(function() {
-      this.res.sendStatus = sinon.stub()
       this.res.send = sinon.stub()
       this.res.status = sinon.stub().returns(this.res)
       this.res.sendStatus = sinon.stub()
-      return (this.middleware = this.AuthenticationController.requireOauth())
+      this.middleware = this.AuthenticationController.requireOauth()
     })
 
     describe('when Oauth2Server authenticates', function() {
@@ -637,148 +636,38 @@ describe('AuthenticationController', function() {
           user: 'user'
         }
         this.Oauth2Server.server.authenticate.yields(null, this.token)
-        return this.middleware(this.req, this.res, this.next)
+        this.middleware(this.req, this.res, this.next)
       })
 
       it('should set oauth_token on request', function() {
-        return this.req.oauth_token.should.equal(this.token)
+        this.req.oauth_token.should.equal(this.token)
       })
 
       it('should set oauth on request', function() {
-        return this.req.oauth.access_token.should.equal(this.token.accessToken)
+        this.req.oauth.access_token.should.equal(this.token.accessToken)
       })
 
       it('should set oauth_user on request', function() {
-        return this.req.oauth_user.should.equal('user')
+        this.req.oauth_user.should.equal('user')
       })
 
-      return it('should call next', function() {
-        return this.next.should.have.been.calledOnce
+      it('should call next', function() {
+        this.next.should.have.been.calledOnce
       })
     })
 
-    return describe('when Oauth2Server does not authenticate', function() {
+    describe('when Oauth2Server returns 401 error', function() {
       beforeEach(function() {
-        return this.Oauth2Server.server.authenticate.yields({ code: 401 })
-      })
-
-      describe('when token not provided', function() {
-        beforeEach(function() {
-          return this.middleware(this.req, this.res, this.next)
-        })
-
-        return it('should return 401 error', function() {
-          return this.res.sendStatus.should.have.been.calledWith(401)
-        })
+        this.Oauth2Server.server.authenticate.yields({ code: 401 })
+        this.middleware(this.req, this.res, this.next)
       })
 
-      describe('when token provided', function() {
-        beforeEach(function() {
-          this.V1Api.request = sinon.stub().yields('error', {}, {})
-          this.req.token = 'foo'
-          return this.middleware(this.req, this.res, this.next)
-        })
-
-        return it('should make request to v1 api with token', function() {
-          return this.V1Api.request.should.have.been.calledWith({
-            expectedStatusCodes: [401],
-            json: {
-              token: 'foo'
-            },
-            method: 'POST',
-            uri: '/api/v1/sharelatex/oauth_authorize'
-          })
-        })
+      it('should return 401 error', function() {
+        this.res.status.should.have.been.calledWith(401)
       })
 
-      describe('when v1 api returns error', function() {
-        beforeEach(function() {
-          this.V1Api.request = sinon.stub().yields('error', {}, {})
-          this.req.token = 'foo'
-          return this.middleware(this.req, this.res, this.next)
-        })
-
-        return it('should return status', function() {
-          return this.next.should.have.been.calledWith('error')
-        })
-      })
-
-      describe('when v1 api status code is not 200', function() {
-        beforeEach(function() {
-          this.V1Api.request = sinon
-            .stub()
-            .yields(null, { statusCode: 401 }, {})
-          this.req.token = 'foo'
-          return this.middleware(this.req, this.res, this.next)
-        })
-
-        return it('should return status', function() {
-          return this.res.status.should.have.been.calledWith(401)
-        })
-      })
-
-      return describe('when v1 api returns authorized profile and access token', function() {
-        beforeEach(function() {
-          this.oauth_authorize = {
-            access_token: 'access_token',
-            user_profile: {
-              id: 'overleaf-id'
-            }
-          }
-          this.V1Api.request = sinon
-            .stub()
-            .yields(null, { statusCode: 200 }, this.oauth_authorize)
-          return (this.req.token = 'foo')
-        })
-
-        describe('in all cases', function() {
-          beforeEach(function() {
-            return this.middleware(this.req, this.res, this.next)
-          })
-
-          return it('should find user', function() {
-            return this.UserModel.findOne.should.have.been.calledWithMatch({
-              'overleaf.id': 'overleaf-id'
-            })
-          })
-        })
-
-        describe('when user find returns error', function() {
-          beforeEach(function() {
-            this.UserModel.findOne = sinon.stub().yields('error')
-            return this.middleware(this.req, this.res, this.next)
-          })
-
-          return it('should return error', function() {
-            return this.next.should.have.been.calledWith('error')
-          })
-        })
-
-        describe('when user is not found', function() {
-          beforeEach(function() {
-            this.UserModel.findOne = sinon.stub().yields(null, null)
-            return this.middleware(this.req, this.res, this.next)
-          })
-
-          return it('should return unauthorized', function() {
-            return this.res.status.should.have.been.calledWith(401)
-          })
-        })
-
-        return describe('when user is found', function() {
-          beforeEach(function() {
-            this.UserModel.findOne = sinon.stub().yields(null, 'user')
-            return this.middleware(this.req, this.res, this.next)
-          })
-
-          it('should add user to request', function() {
-            return this.req.oauth_user.should.equal('user')
-          })
-
-          return it('should add access_token to request', function() {
-            return this.req.oauth.access_token.should.equal('access_token')
-          })
-        })
+      it('should not call next', function() {
+        this.next.should.have.not.been.calledOnce
       })
     })
   })