Fix silent refresh iframe with multiple idps #2107
Conversation
|
|
||
| if (isAuthenticated) { | ||
|
|
||
| const authResult = callbackContext && 'authResult' in callbackContext |
There was a problem hiding this comment.
const authResult = callbackContext?.authResult ?? null;
There was a problem hiding this comment.
I tried this approach, but the types are not working out. callbackContext can be of type {configId: string}, which does not contain authResult , so in this case callbackContext.authResult is not nullish, but a type error. That is why we need 'authResult' in callbackContext to test we are not in case of {configId: string}. At least this is my understanding of the matter.
I have added another approach to make this part look nicer. let me know if the alternative makes sense
| result: unknown, | ||
| isRenewProcess: boolean | ||
| isRenewProcess: boolean, | ||
| config: OpenIdConfiguration |
There was a problem hiding this comment.
If only the configId is needed, please only pass the configId in.
| stateValidationResult: StateValidationResult, | ||
| isRenewProcess: boolean | ||
| isRenewProcess: boolean, | ||
| config: OpenIdConfiguration |
There was a problem hiding this comment.
If only configId is needed, please only pass the id in.
| stateValidationResult: StateValidationResult | null, | ||
| isRenewProcess: boolean | ||
| isRenewProcess: boolean, | ||
| config: OpenIdConfiguration |
There was a problem hiding this comment.
Please only pass configId in.
| stateValidationResult: StateValidationResult | null, | ||
| isRenewProcess: boolean | ||
| isRenewProcess: boolean, | ||
| config: OpenIdConfiguration |
There was a problem hiding this comment.
Please only pass configId in.
| const eventData = e.detail; | ||
|
|
||
| if (typeof eventData === 'object' && eventData.configId && eventData.instanceId) { | ||
| if (eventData.configId === config.configId && eventData.instanceId !== instanceId) { |
There was a problem hiding this comment.
please extract a method.
There was a problem hiding this comment.
this refers, I guess, to the same hunk as the above comment.
| config: OpenIdConfiguration | ||
| ): boolean { | ||
|
|
||
| if (e?.detail == null) { |
There was a problem hiding this comment.
===. But I think undefined should also be checked? if (!e?.detail)
| return false; | ||
| } | ||
|
|
||
| if (e.detail.srcFrameId != null) { |
There was a problem hiding this comment.
same here. We are only checking for null, so undefined is a valid value? Just do if(e.deailt.srcFrameId)
There was a problem hiding this comment.
e.detail.srcFrameId != null is null or undefined, notice != rather than !==. I will change however to if(e.deailt.srcFrameId). This is more consistent with the rest of the codebase.
| if (e.detail.srcFrameId != null) { | ||
|
|
||
| const frameIdPrefix = `${IFRAME_FOR_SILENT_RENEW_IDENTIFIER}_`; | ||
| let eventConfigId: string | null = null; |
There was a problem hiding this comment.
Would consider let as a code smell. Easy fixable by extracting a method, return the value and assign to a const here :-)
There was a problem hiding this comment.
I have made this part of code more concise.
|
|
||
| private convertToLegacyEvent(e: CustomEvent): CustomEvent { | ||
| // If event has the new format with url property, convert it to legacy format | ||
| if (e?.detail?.url != null) { |
There was a problem hiding this comment.
undefined is a valid value here? Better do if(e?.detail?.url)
|
What a PR. Thanks a lot!!!! Incredible work. |
This fixes compatibility issues with Angular 20+ projects created with --standalone=false flag.
|
Thanks for the review points. Working on improvements. |
2 participants
Current implementation does not work correctly with multiple ips using iframe renew.
The PR is related to the following feature request #2105