Added token.verify and deprecated idToken.verify #49
Conversation
lboyette-okta
force-pushed
the
lb-99715-idToken-validation
branch
from
403c97d to
ccd2b32
Compare
| var jwt = decodeToken(token.idToken); | ||
|
|
||
| // Standard claim validation | ||
| oauthUtil.validateClaims(sdk, jwt.payload, token.clientId, token.issuer, nonce); |
There was a problem hiding this comment.
We dont expect validateClaims return any info before we proceed?
There was a problem hiding this comment.
validateClaims is a synchronous method that throws errors. validateClaims could be renamed to assertClaims, but it does additional validation that makes the term "assert" feel inaccurate.
| .then(function(res) { | ||
| expect(res).toEqual(tokens.standardIdTokenParsed); | ||
| }) | ||
| .fail(function() { |
There was a problem hiding this comment.
would fail even be hit since it is a success?
There was a problem hiding this comment.
Ideally it wouldn't, hence expect('not to be hit').toEqual(true);. If, for some reason, the id_token doesn't validate properly, we'll know.
|
@rc @lboyette-okta do we have a way to test these changes against a backend and putting up a screencast or at least make sure the js changes work fine when integrated. |
|
@magizh-okta I'm testing locally, but the end-to-end tests in the okta-signin-widget will cover most of these changes (mostly just validating any id_token when it's created). We have an open ticket to create end-to-end tests in this repo. |
lboyette-okta
force-pushed
the
lb-99715-idToken-validation
branch
from
ee24904 to
79c0759
Compare
| @@ -21,6 +23,9 @@ function verifyToken(idToken, key) { | |||
| var extractable = true; | |||
| var usages = ['verify']; | |||
|
|
|||
| // https://connect.microsoft.com/IE/feedback/details/2242108/webcryptoapi-importing-jwk-with-use-field-fails | |||
| delete key.use; | |||
There was a problem hiding this comment.
This is a metadata tag that specifies the intent of how the key should be used. It's not necessary to properly verify the key's signature, per @yuliu-okta.
lboyette-okta
force-pushed
the
lb-99715-idToken-validation
branch
2 times, most recently
from
edc1a29 to
e1b54d1
Compare
| if (!tokenDict['token'] && !tokenDict['id_token']) { | ||
| throw new AuthSdkError('Unable to parse OAuth flow response'); | ||
| } | ||
| if (!tokenDict['token'] && !tokenDict['id_token']) { |
There was a problem hiding this comment.
Not a big fan of tracking the tokenDict state outside of the promise chain approach. Couple alternatives:
- (preferred) Don't worry about response format in the preceding block. Always do the checks, and return an array of the tokens that you've extracted. In this block, just do an array sort on the tokens (by the tokenTypes), or return the token itself if the tokenType is not an array.
- Create a new tokenDict in the preceding promise block, and then return that, and accept it as an argument here.
There was a problem hiding this comment.
I'm not sure why 1 is preferred over 2. To do the sorting of the tokens (and checking for the presence of an idToken or accessToken), it seems more reasonable to return a tokenDict and build the array of tokens in this block.
lboyette-okta
force-pushed
the
lb-99715-idToken-validation
branch
from
aeec884 to
1672f66
Compare
lboyette-okta
force-pushed
the
lb-99715-idToken-validation
branch
from
d551bfa to
0454de8
Compare
Resolves: OKTA-99715
In the process of deprecating the
idToken.*methods, we weren't able to deprecateidToken.verify, because we didn't have atoken.verify. UnlikeidToken.verify,token.verifytakes a fully parsed idToken, not the base64Url idToken string.Additionally, now that we have caching, we can cache the jwks keys, so can check the signatures of idTokens often. Note: signature validation only works on browsers that support crypto.subtle.
@rchild-okta @magizh-okta