Merge pull request #1424 from hackmdio/fix/oauth2-email-may-undefined

fix server crash when use oauth2 provider with email not exists

lib/auth/oauth2/index.js

@@ -2,94 +2,21 @@
 
 const Router = require('express').Router
 const passport = require('passport')
-const { Strategy, InternalOAuthError } = require('passport-oauth2')
+
 const config = require('../../config')
 const { setReturnToFromReferer, passportGeneralCallback } = require('../utils')
+const { OAuth2CustomStrategy } = require('./strategy')
 
 const oauth2Auth = module.exports = Router()
 
-class OAuth2CustomStrategy extends Strategy {
-  constructor (options, verify) {
-    options.customHeaders = options.customHeaders || {}
-    super(options, verify)
-    this.name = 'oauth2'
-    this._userProfileURL = options.userProfileURL
-    this._oauth2.useAuthorizationHeaderforGET(true)
-  }
-
-  userProfile (accessToken, done) {
-    this._oauth2.get(this._userProfileURL, accessToken, function (err, body, res) {
-      var json
-
-      if (err) {
-        return done(new InternalOAuthError('Failed to fetch user profile', err))
-      }
-
-      try {
-        json = JSON.parse(body)
-      } catch (ex) {
-        return done(new Error('Failed to parse user profile'))
-      }
-
-      const profile = parseProfile(json)
-      profile.provider = 'oauth2'
-
-      done(null, profile)
-    })
-  }
-}
-
-function extractProfileAttribute (data, path) {
-  // can handle stuff like `attrs[0].name`
-  path = path.split('.')
-  for (const segment of path) {
-    const m = segment.match(/([\d\w]+)\[(.*)\]/)
-    data = m ? data[m[1]][m[2]] : data[segment]
-  }
-  return data
-}
-
-function parseProfile (data) {
-  const username = extractProfileAttribute(data, config.oauth2.userProfileUsernameAttr)
-  const displayName = extractProfileAttribute(data, config.oauth2.userProfileDisplayNameAttr)
-  const email = extractProfileAttribute(data, config.oauth2.userProfileEmailAttr)
-
-  return {
-    id: username,
-    username: username,
-    displayName: displayName,
-    email: email
-  }
-}
-
-OAuth2CustomStrategy.prototype.userProfile = function (accessToken, done) {
-  this._oauth2.get(this._userProfileURL, accessToken, function (err, body, res) {
-    var json
-
-    if (err) {
-      return done(new InternalOAuthError('Failed to fetch user profile', err))
-    }
-
-    try {
-      json = JSON.parse(body)
-    } catch (ex) {
-      return done(new Error('Failed to parse user profile'))
-    }
-
-    const profile = parseProfile(json)
-    profile.provider = 'oauth2'
-
-    done(null, profile)
-  })
-}
-
 passport.use(new OAuth2CustomStrategy({
   authorizationURL: config.oauth2.authorizationURL,
   tokenURL: config.oauth2.tokenURL,
   clientID: config.oauth2.clientID,
   clientSecret: config.oauth2.clientSecret,
   callbackURL: config.serverURL + '/auth/oauth2/callback',
-  userProfileURL: config.oauth2.userProfileURL
+  userProfileURL: config.oauth2.userProfileURL,
+  scope: config.oauth2.scope
 }, passportGeneralCallback))
 
 oauth2Auth.get('/auth/oauth2', function (req, res, next) {

lib/auth/oauth2/strategy.js

@@ -0,0 +1,74 @@
+'use strict'
+
+const { Strategy, InternalOAuthError } = require('passport-oauth2')
+const config = require('../../config')
+
+function parseProfile (data) {
+  const username = extractProfileAttribute(data, config.oauth2.userProfileUsernameAttr)
+  const displayName = extractProfileAttribute(data, config.oauth2.userProfileDisplayNameAttr)
+  const email = extractProfileAttribute(data, config.oauth2.userProfileEmailAttr)
+
+  if (!username) {
+    throw new Error('cannot fetch username: please set correct CMD_OAUTH2_USER_PROFILE_USERNAME_ATTR')
+  }
+
+  return {
+    id: username,
+    username: username,
+    displayName: displayName,
+    email: email
+  }
+}
+
+function extractProfileAttribute (data, path) {
+  if (!data) return undefined
+  if (typeof path !== 'string') return undefined
+  // can handle stuff like `attrs[0].name`
+  path = path.split('.')
+  for (const segment of path) {
+    const m = segment.match(/([\d\w]+)\[(.*)\]/)
+    if (!m) {
+      data = data[segment]
+    } else {
+      if (m.length < 3) return undefined
+      if (!data[m[1]]) return undefined
+      data = data[m[1]][m[2]]
+    }
+    if (!data) return undefined
+  }
+  return data
+}
+
+class OAuth2CustomStrategy extends Strategy {
+  constructor (options, verify) {
+    options.customHeaders = options.customHeaders || {}
+    super(options, verify)
+    this.name = 'oauth2'
+    this._userProfileURL = options.userProfileURL
+    this._oauth2.useAuthorizationHeaderforGET(true)
+  }
+
+  userProfile (accessToken, done) {
+    this._oauth2.get(this._userProfileURL, accessToken, function (err, body, res) {
+      if (err) {
+        return done(new InternalOAuthError('Failed to fetch user profile', err))
+      }
+
+      let profile, json
+      try {
+        json = JSON.parse(body)
+        profile = parseProfile(json)
+      } catch (ex) {
+        return done(new InternalOAuthError('Failed to parse user profile' + ex.toString()))
+      }
+
+      profile.provider = 'oauth2'
+
+      done(null, profile)
+    })
+  }
+}
+
+exports.OAuth2CustomStrategy = OAuth2CustomStrategy
+exports.parseProfile = parseProfile
+exports.extractProfileAttribute = extractProfileAttribute

lib/config/default.js

@@ -93,7 +93,13 @@ module.exports = {
     authorizationURL: undefined,
     tokenURL: undefined,
     clientID: undefined,
-    clientSecret: undefined
+    clientSecret: undefined,
+    baseURL: undefined,
+    userProfileURL: undefined,
+    userProfileUsernameAttr: 'username',
+    userProfileDisplayNameAttr: 'displayName',
+    userProfileEmailAttr: 'email',
+    scope: 'email'
   },
   facebook: {
     clientID: undefined,

lib/config/environment.js

@@ -88,14 +88,15 @@ module.exports = {
   oauth2: {
     providerName: process.env.CMD_OAUTH2_PROVIDERNAME,
     baseURL: process.env.CMD_OAUTH2_BASEURL,
+    clientID: process.env.CMD_OAUTH2_CLIENT_ID,
+    clientSecret: process.env.CMD_OAUTH2_CLIENT_SECRET,
+    authorizationURL: process.env.CMD_OAUTH2_AUTHORIZATION_URL,
+    tokenURL: process.env.CMD_OAUTH2_TOKEN_URL,
     userProfileURL: process.env.CMD_OAUTH2_USER_PROFILE_URL,
+    scope: process.env.CMD_OAUTH2_SCOPE,
     userProfileUsernameAttr: process.env.CMD_OAUTH2_USER_PROFILE_USERNAME_ATTR,
     userProfileDisplayNameAttr: process.env.CMD_OAUTH2_USER_PROFILE_DISPLAY_NAME_ATTR,
-    userProfileEmailAttr: process.env.CMD_OAUTH2_USER_PROFILE_EMAIL_ATTR,
+    userProfileEmailAttr: process.env.CMD_OAUTH2_USER_PROFILE_EMAIL_ATTR
-    tokenURL: process.env.CMD_OAUTH2_TOKEN_URL,
-    authorizationURL: process.env.CMD_OAUTH2_AUTHORIZATION_URL,
-    clientID: process.env.CMD_OAUTH2_CLIENT_ID,
-    clientSecret: process.env.CMD_OAUTH2_CLIENT_SECRET
   },
   dropbox: {
     clientID: process.env.CMD_DROPBOX_CLIENTID,

test/auth/oauth2/strategy.test.js

@@ -0,0 +1,56 @@
+/* eslint-env node, mocha */
+'use strict'
+
+const assert = require('assert')
+const chance = require('chance')()
+
+const { extractProfileAttribute } = require('../../../lib/auth/oauth2/strategy')
+
+describe('OAuth2CustomStrategy', function () {
+  describe('#extractProfileAttribute', function () {
+    const data = {
+      user: {
+        email: chance.email()
+      },
+      arrayData: [
+        {
+          email: chance.email()
+        },
+        {
+          email: chance.email()
+        }
+      ]
+    }
+
+    it('should parse normal attribute correctly', function () {
+      assert(extractProfileAttribute(data, 'user.email') === data.user.email)
+    })
+
+    it('should return undefined when nested object key not exists', function () {
+      assert(extractProfileAttribute(data, 'user.profile') === undefined)
+    })
+
+    it('should return undefined when whole object key not exists', function () {
+      assert(extractProfileAttribute(data, 'profile.email') === undefined)
+    })
+
+    it('should return attribute in array correct', function () {
+      assert(extractProfileAttribute(data, 'arrayData[0].email') === data.arrayData[0].email)
+      assert(extractProfileAttribute(data, 'arrayData[1].email') === data.arrayData[1].email)
+    })
+
+    it('should return undefined when array index out of bound', function () {
+      assert(extractProfileAttribute(data, 'arrayData[3].email') === undefined)
+    })
+
+    it('should return undefined when array key not exists', function () {
+      assert(extractProfileAttribute(data, 'notExistsArray[5].email') === undefined)
+    })
+
+    it('should return undefined when data is undefined', function () {
+      assert(extractProfileAttribute(undefined, 'email') === undefined)
+      assert(extractProfileAttribute(null, 'email') === undefined)
+      assert(extractProfileAttribute({}, 'email') === undefined)
+    })
+  })
+})