Skip to content

Commit

Permalink
Cleanup of PR mikenicholson#108.
Browse files Browse the repository at this point in the history
Moves the logic that calls secretOrKeyProvider out of verify_jwt.js
and back into lib/strategy.js. verify_jwt.js was intended to be a
simple call-through function for the real verfier from jsonwebtoken.

Changes the signature of secretOrKeyProvider to (request, token, done)
from (token, done) to support a wider range of use cases.

Updates tests for the above behavior.
  • Loading branch information
mikenicholson committed Jul 28, 2017
1 parent b3846ef commit a236e40
Show file tree
Hide file tree
Showing 4 changed files with 107 additions and 57 deletions.
61 changes: 36 additions & 25 deletions lib/strategy.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,12 @@ var passport = require('passport-strategy')
* Strategy constructor
*
* @param options
* secretOrKey: (REQUIRED) String or buffer containing the secret or PEM-encoded public key
* secretOrKey: String or buffer containing the secret or PEM-encoded public key. Required unless secretOrKeyProvider is provided.
* secretOrKeyProvider: callback in the format secretOrKeyProvider(request, token, done)`,
* which should call done with a secret or PEM-encoded public key
* (asymmetric) for the given token and request combination. done
* has the signature function done(err, secret).
* REQUIRED unless `secretOrKey` is provided.
* jwtFromRequest: (REQUIRED) Function that accepts a reqeust as the only parameter and returns the either JWT as a string or null
* issuer: If defined issuer will be verified against this value
* audience: If defined audience will be verified against this value
Expand All @@ -31,8 +36,8 @@ function JwtStrategy(options, verify) {
if (this._secretOrKeyProvider) {
throw new TypeError('JwtStrategy has been given both a secretOrKey and a secretOrKeyProvider');
}
this._secretOrKeyProvider = function (token, done) {
done(options.secretOrKey)
this._secretOrKeyProvider = function (request, token, done) {
done(null, options.secretOrKey)
};
}

Expand Down Expand Up @@ -91,31 +96,37 @@ JwtStrategy.prototype.authenticate = function(req, options) {
return self.fail(new Error("No auth token"));
}

// Verify the JWT
JwtStrategy.JwtVerifier(token, this._secretOrKeyProvider, this._verifOpts, function(jwt_err, payload) {
if (jwt_err) {
return self.fail(jwt_err);
this._secretOrKeyProvider(req, token, function(secretOrKeyError, secretOrKey) {
if (secretOrKeyError) {
self.fail(secretOrKeyError)
} else {
// Pass the parsed token to the user
var verified = function(err, user, info) {
if(err) {
return self.error(err);
} else if (!user) {
return self.fail(info);
// Verify the JWT
JwtStrategy.JwtVerifier(token, secretOrKey, self._verifOpts, function(jwt_err, payload) {
if (jwt_err) {
return self.fail(jwt_err);
} else {
return self.success(user, info);
// Pass the parsed token to the user
var verified = function(err, user, info) {
if(err) {
return self.error(err);
} else if (!user) {
return self.fail(info);
} else {
return self.success(user, info);
}
};

try {
if (self._passReqToCallback) {
self._verify(req, payload, verified);
} else {
self._verify(payload, verified);
}
} catch(ex) {
self.error(ex);
}
}
};

try {
if (self._passReqToCallback) {
self._verify(req, payload, verified);
} else {
self._verify(payload, verified);
}
} catch(ex) {
self.error(ex);
}
});
}
});
};
Expand Down
13 changes: 2 additions & 11 deletions lib/verify_jwt.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,5 @@
var jwt = require('jsonwebtoken');

module.exports = function(token, secretOrKeyProvider, options, callback, verify) {
if (!verify) {
verify = jwt.verify;
}
return secretOrKeyProvider(token, function (err, secretOrKey) {
if (err) {
callback(err);
return;
}
verify(token, secretOrKey, options, callback);
});
module.exports = function(token, secretOrKey, options, callback) {
return jwt.verify(token, secretOrKey, options, callback);
};
8 changes: 3 additions & 5 deletions test/strategy-validation-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,11 @@ describe('Strategy', function() {
});


it('should be given a function that calls done with the secretOrKey when passed the token', function() {
const secretOrKeyProvider = Strategy.JwtVerifier.args[0][1];
const doneSpy = sinon.spy();
secretOrKeyProvider(null, doneSpy);
expect(doneSpy.calledWith('secret')).to.be.true;
it('should call with the right secret as an argument', function() {
expect(Strategy.JwtVerifier.args[0][1]).to.equal('secret');
});


it('should call with the right issuer option', function() {
expect(Strategy.JwtVerifier.args[0][2]).to.be.an.object;
expect(Strategy.JwtVerifier.args[0][2].issuer).to.equal('TestIssuer');
Expand Down
82 changes: 66 additions & 16 deletions test/strategy-verify-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -170,26 +170,76 @@ describe('Strategy', function() {

});

describe('verify function', function() {
it('should call the secretOrKeyProvider with the token and pass it to verify', function() {
const provider = sinon.spy(function(token, done) {
done(null, 'secret');

describe('handling a request when constructed with a secretOrKeyProvider function that succeeds', function() {

var strategy, fakeSecretOrKeyProvider, expectedReqeust;

before(function(done) {
fakeSecretOrKeyProvider = sinon.spy(function(request, token, done) {
done(null, 'secret from callback');
});
const verifyStub = sinon.stub();
verify(test_data.valid_jwt.payload, provider, null, null, verifyStub);
expect(provider.calledOnce).to.be.true;
expect(provider.calledWith(test_data.valid_jwt.payload)).to.be.true;
expect(verifyStub.calledWith(test_data.valid_jwt.payload, 'secret')).to.be.true;
opts = {
secretOrKeyProvider: fakeSecretOrKeyProvider,
jwtFromRequest: function(request) {
return 'an undecoded jwt string';
}
}
strategy = new Strategy(opts, function(jwtPayload, next) {
return next(null, {user_id: 'dont care'}, {});
});

chai.passport.use(strategy)
.success(function(u, i) {
done();
})
.req(function(req) {
expectedReqeust = req;
})
.authenticate();
});

it('should call the fake secret or key provider with the reqeust', function() {
expect(fakeSecretOrKeyProvider.calledWith(expectedReqeust, sinon.match.any, sinon.match.any)).to.be.true;
});

it('should call the secretOrKeyProvider with the undecoded jwt', function() {
expect(fakeSecretOrKeyProvider.calledWith(sinon.match.any, 'an undecoded jwt string', sinon.match.any)).to.be.true;
});

it('should call the callback with an error if the secretOrKeyProvider fails to return a key', function() {
const providerStub = function(token, done) {
done(new Error('invalid key'));
};
const callback = sinon.spy();
verify(test_data.valid_jwt.payload, providerStub, null, callback);
expect(callback.calledWith(sinon.match.instanceOf(Error)));
it('should call JwtVerifier with the value returned from secretOrKeyProvider', function() {
expect(Strategy.JwtVerifier.calledWith(sinon.match.any, 'secret from callback', sinon.match.any, sinon.match.any)).to.be.true;
});
});


describe('handling a request when constructed with a secretOrKeyProvider function that errors', function() {
var errorMessage;

before(function(done) {
fakeSecretOrKeyProvider = sinon.spy(function(request, token, done) {
done('Error occurred looking for the secret');
});
opts = {
secretOrKeyProvider: fakeSecretOrKeyProvider,
jwtFromRequest: function(request) {
return 'an undecoded jwt string';
}
}
strategy = new Strategy(opts, function(jwtPayload, next) {
return next(null, {user_id: 'dont care'}, {});
});

chai.passport.use(strategy)
.fail(function(i) {
errorMessage = i;
done();
})
.authenticate();
});

it('should fail with the error message from the secretOrKeyProvider', function() {
expect(errorMessage).to.equal('Error occurred looking for the secret');
});
});
});

0 comments on commit a236e40

Please sign in to comment.