-
Notifications
You must be signed in to change notification settings - Fork 233
feat(jwt-verifier): Removes check of client_id from access tokens #477
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.
A few questions
@@ -95,22 +94,14 @@ describe('jwt-verifier configuration validation', () => { | |||
expect(createInstance).toThrow(); | |||
}); | |||
|
|||
it('should throw if the client_id is not provided', () => { | |||
it('should NOT throw if client_id not matching {clientId} is provided', () => { |
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.
What is this testing? That we don't throw if they provide a client_id?
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.
Asserting that the input is allowed, and is NOT checked against the claims automatically.
That's not exciting, but this documents the expected behavior for this case.
@@ -63,9 +63,10 @@ class OktaJwtVerifier { | |||
constructor(options = {}) { | |||
// Assert configuration | |||
assertIssuer(options.issuer, options.testing); | |||
assertClientId(options.clientId); | |||
if( options.clientId ) { |
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.
Do we even need assertClientId
anymore? Or should we just let this be handled by the custom claims assertion logic?
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.
The value here is catching "{clientId}" cases.
assertClientId doesn't validate the claim, it checks for a well-formed and present clientId.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
clientId/client_id was required, even for access tokens that, per spec, do not require client id
Issue Number: #367
What is the new behavior?
clientId is now optional, and will only be asserted if such an assertion is requested via customClaims
Does this PR introduce a breaking change?
Other information
Reviewers
@aarongranick-okta