Skip to content

Commit 009671b

Browse files
visvkvisvk
committed
PKCE refactor: add option PKCEEnabled, require isPublic client parameter
1 parent 920e642 commit 009671b

9 files changed

+192
-34
lines changed

‎lib/grant-types/abstract-grant-type.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ function AbstractGrantType(options) {
3030
this.model = options.model;
3131
this.refreshTokenLifetime = options.refreshTokenLifetime;
3232
this.alwaysIssueNewRefreshToken = options.alwaysIssueNewRefreshToken;
33+
this.PKCEEnabled = options.PKCEEnabled;
3334
}
3435

3536
/**

‎lib/grant-types/authorization-code-grant-type.js

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,9 @@ AuthorizationCodeGrantType.prototype.getAuthorizationCode = function(request, cl
9090
if (!is.vschar(request.body.code)) {
9191
throw new InvalidRequestError('Invalid parameter: `code`');
9292
}
93+
9394
return promisify(this.model.getAuthorizationCode, 1).call(this.model, request.body.code)
95+
.bind(this)
9496
.then(function(code) {
9597
if (!code) {
9698
throw new InvalidGrantError('Invalid grant: authorization code is invalid');
@@ -120,7 +122,11 @@ AuthorizationCodeGrantType.prototype.getAuthorizationCode = function(request, cl
120122
throw new InvalidGrantError('Invalid grant: `redirect_uri` is not a valid URI');
121123
}
122124

123-
if (code.codeChallenge) {
125+
if (this.PKCEEnabled && client.isPublic) {
126+
if (!code.codeChallenge) {
127+
throw new ServerError('Server error: `getAuthorizationCode()` did not return a `codeChallenge` property');
128+
}
129+
124130
if (!code.codeChallengeMethod) {
125131
throw new ServerError('Server error: `getAuthorizationCode()` did not return a `codeChallengeMethod` property');
126132
}

‎lib/handlers/authorize-handler.js

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ function AuthorizeHandler(options) {
6262
this.allowEmptyState = options.allowEmptyState;
6363
this.authenticateHandler = options.authenticateHandler || new AuthenticateHandler(options);
6464
this.authorizationCodeLifetime = options.authorizationCodeLifetime;
65+
this.PKCEEnabled = options.PKCEEnabled;
6566
this.model = options.model;
6667
}
6768

@@ -101,13 +102,13 @@ AuthorizeHandler.prototype.handle = function(request, response) {
101102
return Promise.bind(this)
102103
.then(function() {
103104
scope = this.getScope(request);
104-
codeChallenge = this.getCodeChallenge(request);
105+
codeChallenge = this.getCodeChallenge(request, client);
105106

106107
if (codeChallenge) {
107-
codeChallengeMethod = this.getCodeChallengeMethod(request) || 'plain';
108+
codeChallengeMethod = this.getCodeChallengeMethod(request);
108109
}
109110

110-
return this.generateAuthorizationCode(client, user, scope, codeChallenge, codeChallengeMethod);
111+
return this.generateAuthorizationCode(client, user, scope);
111112
})
112113
.then(function(authorizationCode) {
113114
state = this.getState(request);
@@ -140,23 +141,27 @@ AuthorizeHandler.prototype.handle = function(request, response) {
140141
* Generate authorization code.
141142
*/
142143

143-
AuthorizeHandler.prototype.generateAuthorizationCode = function(client, user, scope, codeChallenge, codeChallengeMethod) {
144+
AuthorizeHandler.prototype.generateAuthorizationCode = function(client, user, scope) {
144145
if (this.model.generateAuthorizationCode) {
145-
return promisify(this.model.generateAuthorizationCode, 5).call(this.model, client, user, scope, codeChallenge, codeChallengeMethod);
146+
return promisify(this.model.generateAuthorizationCode, 3).call(this.model, client, user, scope);
146147
}
147148
return tokenUtil.generateRandomToken();
148149
};
149150

150-
AuthorizeHandler.prototype.getCodeChallenge = function(request) {
151+
AuthorizeHandler.prototype.getCodeChallenge = function(request, client) {
151152
var codeChallenge = request.body.code_challenge || request.query.code_challenge;
152153

154+
if (this.PKCEEnabled && client.isPublic && _.isEmpty(codeChallenge)) {
155+
throw new InvalidRequestError('Missing parameter: `code_challenge`. Public clients must include a code_challenge');
156+
}
157+
153158
return codeChallenge;
154159
};
155160

156161
AuthorizeHandler.prototype.getCodeChallengeMethod = function(request) {
157-
var codeChallengeMethod = request.body.code_challenge_method || request.query.code_challenge_method;
162+
var codeChallengeMethod = request.body.code_challenge_method || request.query.code_challenge_method || 'plain';
158163

159-
if (codeChallengeMethod && !_.includes(['S256', 'plain'], codeChallengeMethod)) {
164+
if (!_.includes(['S256', 'plain'], codeChallengeMethod)) {
160165
throw new InvalidRequestError('Invalid parameter: `code_challenge_method`');
161166
}
162167

@@ -195,6 +200,7 @@ AuthorizeHandler.prototype.getClient = function(request) {
195200
throw new InvalidRequestError('Invalid request: `redirect_uri` is not a valid URI');
196201
}
197202
return promisify(this.model.getClient, 2).call(this.model, clientId, null)
203+
.bind(this)
198204
.then(function(client) {
199205
if (!client) {
200206
throw new InvalidClientError('Invalid client: client credentials are invalid');
@@ -215,6 +221,16 @@ AuthorizeHandler.prototype.getClient = function(request) {
215221
if (redirectUri && !_.includes(client.redirectUris, redirectUri)) {
216222
throw new InvalidClientError('Invalid client: `redirect_uri` does not match client value');
217223
}
224+
225+
if (this.PKCEEnabled) {
226+
if (client.isPublic === undefined) {
227+
throw new InvalidClientError('Invalid client: missing client `isPublic`');
228+
}
229+
230+
if (typeof client.isPublic !== 'boolean') {
231+
throw new InvalidClientError('Invalid client: `isPublic` must be a boolean');
232+
}
233+
}
218234
return client;
219235
});
220236
};

‎lib/handlers/token-handler.js

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,7 @@ function TokenHandler(options) {
6262
this.allowExtendedTokenAttributes = options.allowExtendedTokenAttributes;
6363
this.requireClientAuthentication = options.requireClientAuthentication || {};
6464
this.alwaysIssueNewRefreshToken = options.alwaysIssueNewRefreshToken !== false;
65+
this.PKCEEnabled = options.PKCEEnabled;
6566
}
6667

6768
/**
@@ -133,6 +134,7 @@ TokenHandler.prototype.getClient = function(request, response) {
133134
}
134135

135136
return promisify(this.model.getClient, 2).call(this.model, credentials.clientId, credentials.clientSecret)
137+
.bind(this)
136138
.then(function(client) {
137139
if (!client) {
138140
throw new InvalidClientError('Invalid client: client is invalid');
@@ -146,6 +148,16 @@ TokenHandler.prototype.getClient = function(request, response) {
146148
throw new ServerError('Server error: `grants` must be an array');
147149
}
148150

151+
if (this.PKCEEnabled) {
152+
if (client.isPublic === undefined) {
153+
throw new ServerError('Server error: missing client `isPublic`');
154+
}
155+
156+
if (typeof client.isPublic !== 'boolean') {
157+
throw new ServerError('Server error: invalid client, `isPublic` must be a boolean');
158+
}
159+
}
160+
149161
return client;
150162
})
151163
.catch(function(e) {
@@ -224,7 +236,8 @@ TokenHandler.prototype.handleGrantType = function(request, client) {
224236
accessTokenLifetime: accessTokenLifetime,
225237
model: this.model,
226238
refreshTokenLifetime: refreshTokenLifetime,
227-
alwaysIssueNewRefreshToken: this.alwaysIssueNewRefreshToken
239+
alwaysIssueNewRefreshToken: this.alwaysIssueNewRefreshToken,
240+
PKCEenabled: this.PKCEenabled
228241
};
229242

230243
return new Type(options)

‎lib/server.js

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,8 @@ OAuth2Server.prototype.authenticate = function(request, response, options, callb
5151
OAuth2Server.prototype.authorize = function(request, response, options, callback) {
5252
options = _.assign({
5353
allowEmptyState: false,
54-
authorizationCodeLifetime: 5 * 60 // 5 minutes.
54+
authorizationCodeLifetime: 5 * 60, // 5 minutes.
55+
PKCEEnabled: false
5556
}, this.options, options);
5657

5758
return new AuthorizeHandler(options)
@@ -68,7 +69,8 @@ OAuth2Server.prototype.token = function(request, response, options, callback) {
6869
accessTokenLifetime: 60 * 60, // 1 hour.
6970
refreshTokenLifetime: 60 * 60 * 24 * 14, // 2 weeks.
7071
allowExtendedTokenAttributes: false,
71-
requireClientAuthentication: {} // defaults to true for all grant types
72+
requireClientAuthentication: {}, // defaults to true for all grant types
73+
PKCEEnabled: false
7274
}, this.options, options);
7375

7476
return new TokenHandler(options)

‎package.json

Lines changed: 26 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,32 @@
77
"oauth2"
88
],
99
"contributors": [
10-
{ "name": "Thom Seddon", "email": "thom@seddonmedia.co.uk" },
11-
{ "name": "Lars F. Karlström" , "email": "lars@lfk.io" },
12-
{ "name": "Rui Marinho", "email": "ruipmarinho@gmail.com" },
13-
{ "name" : "Tiago Ribeiro", "email": "tiago.ribeiro@gmail.com" },
14-
{ "name": "Michael Salinger", "email": "mjsalinger@gmail.com" },
15-
{ "name": "Nuno Sousa" },
16-
{ "name": "Max Truxa" }
10+
{
11+
"name": "Thom Seddon",
12+
"email": "thom@seddonmedia.co.uk"
13+
},
14+
{
15+
"name": "Lars F. Karlström",
16+
"email": "lars@lfk.io"
17+
},
18+
{
19+
"name": "Rui Marinho",
20+
"email": "ruipmarinho@gmail.com"
21+
},
22+
{
23+
"name": "Tiago Ribeiro",
24+
"email": "tiago.ribeiro@gmail.com"
25+
},
26+
{
27+
"name": "Michael Salinger",
28+
"email": "mjsalinger@gmail.com"
29+
},
30+
{
31+
"name": "Nuno Sousa"
32+
},
33+
{
34+
"name": "Max Truxa"
35+
}
1736
],
1837
"main": "index.js",
1938
"dependencies": {

‎test/integration/grant-types/authorization-code-grant-type_test.js

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -373,13 +373,13 @@ describe('AuthorizationCodeGrantType integration', function() {
373373
codeChallengeMethod: 'S256',
374374
codeChallenge: stringUtil.base64URLEncode(crypto.createHash('sha256').update(codeVerifier).digest())
375375
};
376-
var client = { id: 'foobar' };
376+
var client = { id: 'foobar', isPublic: true };
377377
var model = {
378378
getAuthorizationCode: function() { return authorizationCode; },
379379
revokeAuthorizationCode: function() {},
380380
saveToken: function() {}
381381
};
382-
var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model });
382+
var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model, PKCEEnabled: true });
383383
var request = new Request({ body: { code: 12345, code_verifier: 'foo' }, headers: {}, method: {}, query: {} });
384384

385385
return grantType.getAuthorizationCode(request, client)
@@ -399,13 +399,13 @@ describe('AuthorizationCodeGrantType integration', function() {
399399
codeChallengeMethod: 'plain',
400400
codeChallenge: 'baz'
401401
};
402-
var client = { id: 'foobar' };
402+
var client = { id: 'foobar', isPublic: true };
403403
var model = {
404404
getAuthorizationCode: function() { return authorizationCode; },
405405
revokeAuthorizationCode: function() {},
406406
saveToken: function() {}
407407
};
408-
var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model });
408+
var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model, PKCEEnabled: true });
409409
var request = new Request({ body: { code: 12345, code_verifier: 'foo' }, headers: {}, method: {}, query: {} });
410410

411411
return grantType.getAuthorizationCode(request, client)
@@ -420,19 +420,19 @@ describe('AuthorizationCodeGrantType integration', function() {
420420
var codeVerifier = stringUtil.base64URLEncode(crypto.randomBytes(32));
421421
var authorizationCode = {
422422
authorizationCode: 12345,
423-
client: { id: 'foobar' },
423+
client: { id: 'foobar', isPublic: true },
424424
expiresAt: new Date(new Date() * 2),
425425
user: {},
426426
codeChallengeMethod: 'S256',
427427
codeChallenge: stringUtil.base64URLEncode(crypto.createHash('sha256').update(codeVerifier).digest())
428428
};
429-
var client = { id: 'foobar' };
429+
var client = { id: 'foobar', isPublic: true };
430430
var model = {
431431
getAuthorizationCode: function() { return authorizationCode; },
432432
revokeAuthorizationCode: function() {},
433433
saveToken: function() {}
434434
};
435-
var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model });
435+
var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model, PKCEEnabled: true });
436436
var request = new Request({ body: { code: 12345, code_verifier: codeVerifier }, headers: {}, method: {}, query: {} });
437437

438438
return grantType.getAuthorizationCode(request, client)
@@ -451,13 +451,13 @@ describe('AuthorizationCodeGrantType integration', function() {
451451
codeChallengeMethod: 'plain',
452452
codeChallenge: 'baz'
453453
};
454-
var client = { id: 'foobar' };
454+
var client = { id: 'foobar', isPublic: true };
455455
var model = {
456456
getAuthorizationCode: function() { return authorizationCode; },
457457
revokeAuthorizationCode: function() {},
458458
saveToken: function() {}
459459
};
460-
var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model });
460+
var grantType = new AuthorizationCodeGrantType({ accessTokenLifetime: 123, model: model, PKCEEnabled: true });
461461
var request = new Request({ body: { code: 12345, code_verifier: 'baz' }, headers: {}, method: {}, query: {} });
462462

463463
return grantType.getAuthorizationCode(request, client)

‎test/integration/handlers/authorize-handler_test.js

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -694,6 +694,44 @@ describe('AuthorizeHandler integration', function() {
694694
e.message.should.equal('Invalid client: `redirect_uri` does not match client value');
695695
});
696696
});
697+
describe('with PKCEEnabled', function() {
698+
it('should throw an error if `client.isPublic` is missing`', function() {
699+
var model = {
700+
getAccessToken: function() {},
701+
getClient: function() {
702+
return { grants: ['authorization_code'], redirectUris: ['https://example.com'] };
703+
},
704+
saveAuthorizationCode: function() {}
705+
};
706+
var handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model: model, PKCEEnabled: true });
707+
var request = new Request({ body: { client_id: 12345, response_type: 'code', redirect_uri: 'https://example.com' }, headers: {}, method: {}, query: {} });
708+
709+
return handler.getClient(request)
710+
.then(should.fail)
711+
.catch(function(e) {
712+
e.should.be.an.instanceOf(InvalidClientError);
713+
e.message.should.equal('Invalid client: missing client `isPublic`');
714+
});
715+
});
716+
it('should throw an error if `client.isPublic` is invalid`', function() {
717+
var model = {
718+
getAccessToken: function() {},
719+
getClient: function() {
720+
return { grants: ['authorization_code'], redirectUris: ['https://example.com'], isPublic: 'foo' };
721+
},
722+
saveAuthorizationCode: function() {}
723+
};
724+
var handler = new AuthorizeHandler({ authorizationCodeLifetime: 120, model: model, PKCEEnabled: true });
725+
var request = new Request({ body: { client_id: 12345, response_type: 'code', redirect_uri: 'https://example.com' }, headers: {}, method: {}, query: {} });
726+
727+
return handler.getClient(request)
728+
.then(should.fail)
729+
.catch(function(e) {
730+
e.should.be.an.instanceOf(InvalidClientError);
731+
e.message.should.equal('Invalid client: `isPublic` must be a boolean');
732+
});
733+
});
734+
});
697735

698736
it('should support promises', function() {
699737
var model = {

0 commit comments

Comments
 (0)