-
Notifications
You must be signed in to change notification settings - Fork 493
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add jwksURI override option #717
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's missing 1 test case: When the __jwks_uri
is not overridden you need to check that the IdTokenVerifier is going to use the issuer + default jwks path, or when this changes (breaks) on the library side you won't notice it.
__tenant: { type: 'string', message: '__tenant option is required' }, | ||
__token_issuer: { type: 'string', message: '__token_issuer option is required' } | ||
__tenant: { optional: true, type: 'string', message: '__tenant option is required' }, | ||
__token_issuer: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have you run the beautifier here? the others are one liners while this one is split in 3 lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope. I'm not testing idtoken-verifier internals here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it's the automatic code formatter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not testing idtoken-verifier internals here.
This looks internal to me. So if you're already doing it to check if a value gets replaced, do the same to check that a value remains as expected 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
idtoken-verifier doesn't set the url in the constructor, it builds later on. I added a test to make sure it's undefined if you don't overwrite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then in that case both tests should ignore the idtoken-verifier
implementation and test the actual outcome:
- Have a test that succeeds to verify a token using a custom
jwks_uri
. - Have a test that succeeds to verify a token using the default
jwks_uri
(${id_token.iss}/.well-known/jwks.json
).
Probably you can manage to feed a jwks file to the library. If you don't test this this way, how can you be sure that the __jwks_uri
override is actually affecting the library verifying outcome?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the new tests.
ZD:37454