Add id_token validation (HS256 and RS256) #295
Conversation
|
I'm also thinking you should probably test against a generated ID token to make sure everything goes through from start to finish. It's a bit of work to get it running with RS256 but certainly possible. I think it's important to test against potential ID token problems as that's what this is meant to catch. I found, in that Rails PR, that the token verifier had a number of options that needed to be set in order to make sure all the validation was happening properly. I don't know if the Node lib you're using is the same but I think it's worth the effort. CodeCov might stop complaining as well :). |
|
Added integration tests (full id_token validation) |
src/auth/OAuthAuthenticator.js
Outdated
| @@ -12,6 +14,7 @@ var RestClient = require('rest-facade').Client; | |||
| * | |||
| * @param {Object} options Authenticator options. | |||
| * @param {String} options.baseUrl The auth0 account URL. | |||
| * @param {String} options.domain AuthenticationClient server domain | |||
There was a problem hiding this comment.
Still not indented properly
src/auth/OAuthAuthenticator.js
Outdated
| @@ -12,6 +14,7 @@ var RestClient = require('rest-facade').Client; | |||
| * | |||
| * @param {Object} options Authenticator options. | |||
| * @param {String} options.baseUrl The auth0 account URL. | |||
There was a problem hiding this comment.
"Auth0" ... capital "A"
| }); | ||
| }); | ||
| describe('#integration', function() { | ||
| it('with a HS256 id_token and `options.supportedAlgorithms===RS256`', done => { |
There was a problem hiding this comment.
it('fails with a HS256 id_token ...
| done(); | ||
| }); | ||
| }); | ||
| it('when `token.header.alg===RS256` and `options.supportedAlgorithms===HS256`', done => { |
There was a problem hiding this comment.
it('fails when 'token.header.alg===RS256 ' ...
| supportedAlgorithms: ['RS256'] | ||
| }); | ||
| oauthWithValidation.create(PARAMS, DATA, function(e, d) { | ||
| expect(e.message).to.eq('jwt issuer invalid. expected: https://auth.brucke.club/'); |
There was a problem hiding this comment.
I issue we don't have any control over the error message here? I wonder if it's bad practice to show what we were expecting.
| clientId: 'foobar', | ||
| clientSecret: CLIENT_SECRET, | ||
| domain: 'brucke.auth0.com', | ||
| supportedAlgorithms: ['RS256'] |
There was a problem hiding this comment.
How are you creating an RS256 token that can be used indefinitely?
| } | ||
| ); | ||
| var oauthWithValidation = new OAUthWithIDTokenValidationProxy(oauth, { | ||
| clientId: 'foobar', |
There was a problem hiding this comment.
How about __test_client_id__?
| var idtoken = jwt.sign({ foo: 'bar' }, c.serviceKey, { | ||
| algorithm: 'RS256', | ||
| issuer: 'wrong_issuer', | ||
| audience: 'foobar', |
There was a problem hiding this comment.
How about __test_audience__?
| done(); | ||
| }); | ||
| }); | ||
| describe('when using a valid token', function() { |
There was a problem hiding this comment.
when using a valid token
Aren't these invalid?
| done(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Can you test for nbf failing as well?
There was a problem hiding this comment.
Auth0 tokens do not return an nbf so not necessary for this use case.
| it('Calls jwt.verify with token and algs', function(done) { | ||
| var oauth = { | ||
| create: function() { | ||
| return new Promise(res => res({ id_token: 'foobar' })); |
There was a problem hiding this comment.
How about __test_id_token__? Many more below ...
|
LGTM from a security perspective |
luisrudge
changed the title
This PR adds id_token validation for all
oauth.signIn,oauth.passwordGrantandauthorizationCodeGrant. This means that if the response for those calls contains an id_token, it will be properly validated.