Skip to content
This repository has been archived by the owner on Aug 28, 2023. It is now read-only.

fix for 3.0.2 #244

Merged
merged 3 commits into from
Nov 9, 2016
Merged

fix for 3.0.2 #244

merged 3 commits into from
Nov 9, 2016

Conversation

lovemaths
Copy link
Contributor

(1) updated version to 3.0.2 in package.json
(2) removed oniyi-object-transform dependency
(3) set clock skew to 5 minutes as Wilson does. Will make this configurable in the future.

(1) updated version to 3.0.2 in package.json
(2) removed oniyi-object-transform dependency
(3) set clock skew to 5 minutes. will make this configurable in the future.
@@ -178,17 +179,17 @@ exports.verify = function(jwtString, PEMKey, options, callback) {
if (typeof payload.exp !== 'number')
return done(new Error('invalid exp value in payload'));
if (!options.ignoreExpiration) {
if (Math.floor(Date.now() / 1000) >= payload.exp) {
if (Math.floor(Date.now() / 1000) >= payload.exp + constants.CLOCK_SKEW) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is Date.now() in UTC?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Date.now() method returns the number of milliseconds elapsed since 1 January 1970 00:00:00 UTC.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any tests for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The time is UTC. We have tests for expiration.

return done(new Error('jwt is expired'));
}
}

// (5) nbf
// - check if it exists
if (payload.nbf) {
if (typeof payload.nbf !== 'number')
if (typeof payload.nbf !== 'number' || payload.nbf >= payload.exp)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be helpful to users if the error message indicated that nbf > exp OR nbf was not a number

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed in the second commit.

],
});
var oidc = {};
oidc.algorithms = doc.id_token_signing_alg_values_supported;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any particular reason for a name difference between 'doc' and 'oidc'? For example why auth_endpoint and authorization_endpoint (which is the name in the spec). I would stick with the names in the spec, otherwise we are always translating. Sometimes, we may have overlap. Check out the names in: https://login.microsoftonline.com/common/.well-known/openid-configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the auth_endpoint to authorization_endpoint everywhere in the second commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will keep the 'algorithms' one since it is a more natural name than 'id_token_signing_alg_values_supported'. In the library we used 'algorithms' everywhere and we didn't do any translation of that.

CONSTANTS.POLICY_REGEX = /^b2c_1_[0-9a-z._-]+$/i; // policy is case insensitive
CONSTANTS.TENANTNAME_REGEX = /^[0-9a-zA-Z]+.onmicrosoft.com$/;
CONSTANTS.TENANTID_REGEX = /^[0-9a-zA-Z-]+$/;

CONSTANTS.CLOCK_SKEW = 300; // 5 minutes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please open an issue to make CLOCK_SKEW configurable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@polita polita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but you're missing the current version update in the readme. Please add, and then :shipit:

@lovemaths lovemaths merged commit 505bfba into master Nov 9, 2016
@lovemaths lovemaths deleted the sijliu/3.0.2 branch November 9, 2016 02:19
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants