chore: fixes during conformance testing #150
Conversation
| //fixme: Create correlationId if not provided. Might need to be deferred to registry though | ||
| this._correlationId = args.correlationId | ||
| this._timestamp = Date.now() | ||
| this._subject = args.subject | ||
| this._redirectUri = args.redirectUri |
There was a problem hiding this comment.
Huh, we have a generic subject. Why are we creating something very specific (a redirectUri) on every event type. That should not happen
There was a problem hiding this comment.
It's optional and it's not every event type, just AuthorizationEvent types.
await this.emitEvent(AuthorizationEvents.ON_AUTH_RESPONSE_SENT_SUCCESS, { correlationId, subject: response, redirectUri: redirectUri })
Alternatively I can create a new type wrapping response & redirectUri. That would be a breaking change, but I do not think we are using that event anywhere else in our codebase.
There was a problem hiding this comment.
I just saw there is a simpler solution without using the event. Will revert
| @@ -96,10 +96,10 @@ export const getRequestObjectJwtVerifier = async ( | |||
| if (jwt.payload.redirect_uri && jwt.payload.redirect_uri !== clientId) { | |||
There was a problem hiding this comment.
There probably should be a else if here, that does the exact same check but then for jwt.payload,response_uri (the new name) instead of redirect_uri
There was a problem hiding this comment.
Is client_id_scheme still redirect_uri in case jwt.payload,response_uri is used?
I don't see redirect_uri as valid value for client_id_scheme
There was a problem hiding this comment.
There probably should be a else if here, that does the exact same check but then for jwt.payload,response_uri (the new name) instead of redirect_uri
This was resolved BTW
| @@ -96,10 +96,10 @@ export const getRequestObjectJwtVerifier = async ( | |||
| if (jwt.payload.redirect_uri && jwt.payload.redirect_uri !== clientId) { | |||
| throw new Error(SIOPErrors.INVALID_CLIENT_ID_MUST_MATCH_REDIRECT_URI) | |||
| } | |||
| const parts = options.raw.split('.') | |||
| /*const parts = options.raw.split('.') this can be signed and execution shouldn't even be here when alg = none | |||
There was a problem hiding this comment.
Why are we removing this, instead of not checking it when alg === none ?
There was a problem hiding this comment.
Execution never gets here if alg is none
line 83
if (!clientIdScheme || jwt.header.alg === 'none') {
return getJwtVerifierWithContext(jwt, { type })
}
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #150 +/- ##
========================================
Coverage 49.11% 49.11%
========================================
Files 74 74
Lines 4976 4976
Branches 1769 1769
========================================
Hits 2444 2444
Misses 2529 2529
Partials 3 3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
No description provided.