Skip to content

Commit a6f2ff4

Browse files
jackycutejackycute
authored
Merge pull request #1424 from hackmdio/fix/oauth2-email-may-undefined
fix server crash when use oauth2 provider with email not exists
2 parents 06f578e + 72c5b0d commit a6f2ff4

File tree

5 files changed

+147
-83
lines changed

5 files changed

+147
-83
lines changed

‎lib/auth/oauth2/index.js

Lines changed: 4 additions & 77 deletions
Original file line numberDiff line numberDiff line change
@@ -2,94 +2,21 @@
22

33
const Router = require('express').Router
44
const passport = require('passport')
5-
const { Strategy, InternalOAuthError } = require('passport-oauth2')
5+
66
const config = require('../../config')
77
const { setReturnToFromReferer, passportGeneralCallback } = require('../utils')
8+
const { OAuth2CustomStrategy } = require('./strategy')
89

910
const oauth2Auth = module.exports = Router()
1011

11-
class OAuth2CustomStrategy extends Strategy {
12-
constructor (options, verify) {
13-
options.customHeaders = options.customHeaders || {}
14-
super(options, verify)
15-
this.name = 'oauth2'
16-
this._userProfileURL = options.userProfileURL
17-
this._oauth2.useAuthorizationHeaderforGET(true)
18-
}
19-
20-
userProfile (accessToken, done) {
21-
this._oauth2.get(this._userProfileURL, accessToken, function (err, body, res) {
22-
var json
23-
24-
if (err) {
25-
return done(new InternalOAuthError('Failed to fetch user profile', err))
26-
}
27-
28-
try {
29-
json = JSON.parse(body)
30-
} catch (ex) {
31-
return done(new Error('Failed to parse user profile'))
32-
}
33-
34-
const profile = parseProfile(json)
35-
profile.provider = 'oauth2'
36-
37-
done(null, profile)
38-
})
39-
}
40-
}
41-
42-
function extractProfileAttribute (data, path) {
43-
// can handle stuff like `attrs[0].name`
44-
path = path.split('.')
45-
for (const segment of path) {
46-
const m = segment.match(/([\d\w]+)\[(.*)\]/)
47-
data = m ? data[m[1]][m[2]] : data[segment]
48-
}
49-
return data
50-
}
51-
52-
function parseProfile (data) {
53-
const username = extractProfileAttribute(data, config.oauth2.userProfileUsernameAttr)
54-
const displayName = extractProfileAttribute(data, config.oauth2.userProfileDisplayNameAttr)
55-
const email = extractProfileAttribute(data, config.oauth2.userProfileEmailAttr)
56-
57-
return {
58-
id: username,
59-
username: username,
60-
displayName: displayName,
61-
email: email
62-
}
63-
}
64-
65-
OAuth2CustomStrategy.prototype.userProfile = function (accessToken, done) {
66-
this._oauth2.get(this._userProfileURL, accessToken, function (err, body, res) {
67-
var json
68-
69-
if (err) {
70-
return done(new InternalOAuthError('Failed to fetch user profile', err))
71-
}
72-
73-
try {
74-
json = JSON.parse(body)
75-
} catch (ex) {
76-
return done(new Error('Failed to parse user profile'))
77-
}
78-
79-
const profile = parseProfile(json)
80-
profile.provider = 'oauth2'
81-
82-
done(null, profile)
83-
})
84-
}
85-
8612
passport.use(new OAuth2CustomStrategy({
8713
authorizationURL: config.oauth2.authorizationURL,
8814
tokenURL: config.oauth2.tokenURL,
8915
clientID: config.oauth2.clientID,
9016
clientSecret: config.oauth2.clientSecret,
9117
callbackURL: config.serverURL + '/auth/oauth2/callback',
92-
userProfileURL: config.oauth2.userProfileURL
18+
userProfileURL: config.oauth2.userProfileURL,
19+
scope: config.oauth2.scope
9320
}, passportGeneralCallback))
9421

9522
oauth2Auth.get('/auth/oauth2', function (req, res, next) {

‎lib/auth/oauth2/strategy.js

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
'use strict'
2+
3+
const { Strategy, InternalOAuthError } = require('passport-oauth2')
4+
const config = require('../../config')
5+
6+
function parseProfile (data) {
7+
const username = extractProfileAttribute(data, config.oauth2.userProfileUsernameAttr)
8+
const displayName = extractProfileAttribute(data, config.oauth2.userProfileDisplayNameAttr)
9+
const email = extractProfileAttribute(data, config.oauth2.userProfileEmailAttr)
10+
11+
if (!username) {
12+
throw new Error('cannot fetch username: please set correct CMD_OAUTH2_USER_PROFILE_USERNAME_ATTR')
13+
}
14+
15+
return {
16+
id: username,
17+
username: username,
18+
displayName: displayName,
19+
email: email
20+
}
21+
}
22+
23+
function extractProfileAttribute (data, path) {
24+
if (!data) return undefined
25+
if (typeof path !== 'string') return undefined
26+
// can handle stuff like `attrs[0].name`
27+
path = path.split('.')
28+
for (const segment of path) {
29+
const m = segment.match(/([\d\w]+)\[(.*)\]/)
30+
if (!m) {
31+
data = data[segment]
32+
} else {
33+
if (m.length < 3) return undefined
34+
if (!data[m[1]]) return undefined
35+
data = data[m[1]][m[2]]
36+
}
37+
if (!data) return undefined
38+
}
39+
return data
40+
}
41+
42+
class OAuth2CustomStrategy extends Strategy {
43+
constructor (options, verify) {
44+
options.customHeaders = options.customHeaders || {}
45+
super(options, verify)
46+
this.name = 'oauth2'
47+
this._userProfileURL = options.userProfileURL
48+
this._oauth2.useAuthorizationHeaderforGET(true)
49+
}
50+
51+
userProfile (accessToken, done) {
52+
this._oauth2.get(this._userProfileURL, accessToken, function (err, body, res) {
53+
if (err) {
54+
return done(new InternalOAuthError('Failed to fetch user profile', err))
55+
}
56+
57+
let profile, json
58+
try {
59+
json = JSON.parse(body)
60+
profile = parseProfile(json)
61+
} catch (ex) {
62+
return done(new InternalOAuthError('Failed to parse user profile' + ex.toString()))
63+
}
64+
65+
profile.provider = 'oauth2'
66+
67+
done(null, profile)
68+
})
69+
}
70+
}
71+
72+
exports.OAuth2CustomStrategy = OAuth2CustomStrategy
73+
exports.parseProfile = parseProfile
74+
exports.extractProfileAttribute = extractProfileAttribute

‎lib/config/default.js

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,13 @@ module.exports = {
9393
authorizationURL: undefined,
9494
tokenURL: undefined,
9595
clientID: undefined,
96-
clientSecret: undefined
96+
clientSecret: undefined,
97+
baseURL: undefined,
98+
userProfileURL: undefined,
99+
userProfileUsernameAttr: 'username',
100+
userProfileDisplayNameAttr: 'displayName',
101+
userProfileEmailAttr: 'email',
102+
scope: 'email'
97103
},
98104
facebook: {
99105
clientID: undefined,

‎lib/config/environment.js

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -88,14 +88,15 @@ module.exports = {
8888
oauth2: {
8989
providerName: process.env.CMD_OAUTH2_PROVIDERNAME,
9090
baseURL: process.env.CMD_OAUTH2_BASEURL,
91+
clientID: process.env.CMD_OAUTH2_CLIENT_ID,
92+
clientSecret: process.env.CMD_OAUTH2_CLIENT_SECRET,
93+
authorizationURL: process.env.CMD_OAUTH2_AUTHORIZATION_URL,
94+
tokenURL: process.env.CMD_OAUTH2_TOKEN_URL,
9195
userProfileURL: process.env.CMD_OAUTH2_USER_PROFILE_URL,
96+
scope: process.env.CMD_OAUTH2_SCOPE,
9297
userProfileUsernameAttr: process.env.CMD_OAUTH2_USER_PROFILE_USERNAME_ATTR,
9398
userProfileDisplayNameAttr: process.env.CMD_OAUTH2_USER_PROFILE_DISPLAY_NAME_ATTR,
94-
userProfileEmailAttr: process.env.CMD_OAUTH2_USER_PROFILE_EMAIL_ATTR,
95-
tokenURL: process.env.CMD_OAUTH2_TOKEN_URL,
96-
authorizationURL: process.env.CMD_OAUTH2_AUTHORIZATION_URL,
97-
clientID: process.env.CMD_OAUTH2_CLIENT_ID,
98-
clientSecret: process.env.CMD_OAUTH2_CLIENT_SECRET
99+
userProfileEmailAttr: process.env.CMD_OAUTH2_USER_PROFILE_EMAIL_ATTR
99100
},
100101
dropbox: {
101102
clientID: process.env.CMD_DROPBOX_CLIENTID,

‎test/auth/oauth2/strategy.test.js

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
/* eslint-env node, mocha */
2+
'use strict'
3+
4+
const assert = require('assert')
5+
const chance = require('chance')()
6+
7+
const { extractProfileAttribute } = require('../../../lib/auth/oauth2/strategy')
8+
9+
describe('OAuth2CustomStrategy', function () {
10+
describe('#extractProfileAttribute', function () {
11+
const data = {
12+
user: {
13+
email: chance.email()
14+
},
15+
arrayData: [
16+
{
17+
email: chance.email()
18+
},
19+
{
20+
email: chance.email()
21+
}
22+
]
23+
}
24+
25+
it('should parse normal attribute correctly', function () {
26+
assert(extractProfileAttribute(data, 'user.email') === data.user.email)
27+
})
28+
29+
it('should return undefined when nested object key not exists', function () {
30+
assert(extractProfileAttribute(data, 'user.profile') === undefined)
31+
})
32+
33+
it('should return undefined when whole object key not exists', function () {
34+
assert(extractProfileAttribute(data, 'profile.email') === undefined)
35+
})
36+
37+
it('should return attribute in array correct', function () {
38+
assert(extractProfileAttribute(data, 'arrayData[0].email') === data.arrayData[0].email)
39+
assert(extractProfileAttribute(data, 'arrayData[1].email') === data.arrayData[1].email)
40+
})
41+
42+
it('should return undefined when array index out of bound', function () {
43+
assert(extractProfileAttribute(data, 'arrayData[3].email') === undefined)
44+
})
45+
46+
it('should return undefined when array key not exists', function () {
47+
assert(extractProfileAttribute(data, 'notExistsArray[5].email') === undefined)
48+
})
49+
50+
it('should return undefined when data is undefined', function () {
51+
assert(extractProfileAttribute(undefined, 'email') === undefined)
52+
assert(extractProfileAttribute(null, 'email') === undefined)
53+
assert(extractProfileAttribute({}, 'email') === undefined)
54+
})
55+
})
56+
})

0 commit comments

Comments
 (0)