Fixing frontchannel logout in SPA app template #9590
Conversation
WalkthroughAdds conditional assignment of Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@features/admin.applications.v1/components/forms/inbound-oidc-form.tsx`:
- Around line 1694-1698: The SPA branch always reads
values.get("frontChannelLogoutUrl") which can be undefined and accidentally
clear an existing URL; update the logout.frontChannelLogoutUrl assignment to
mirror the non‑SPA guard by only using values.get("frontChannelLogoutUrl") when
the front‑channel logout field is shown (isFrontChannelLogoutEnabled /
showFrontChannelLogout), otherwise keep
initialValues?.logout?.frontChannelLogoutUrl; reference the logout object,
frontChannelLogoutUrl key, isFrontChannelLogoutEnabled (or
showFrontChannelLogout), values.get("frontChannelLogoutUrl") and
initialValues?.logout?.frontChannelLogoutUrl when making this conditional
change.
| logout: { | ||
| frontChannelLogoutUrl: isFrontChannelLogoutEnabled | ||
| ? values.get("frontChannelLogoutUrl") | ||
| : initialValues?.logout?.frontChannelLogoutUrl | ||
| }, |
There was a problem hiding this comment.
Guard SPA front‑channel logout serialization to avoid unintentionally clearing existing URL.
If showFrontChannelLogout is false (or the field is absent), values.get("frontChannelLogoutUrl") yields undefined, and the payload can serialize logout: {} — which may clear an existing front‑channel logout URL. The non‑SPA path deletes this field based on showFrontChannelLogout, but the SPA path doesn’t. Please mirror that guard or fall back only when the field is visible.
💡 Suggested fix (mirror non‑SPA guard)
let inboundConfigFormValues: any = {
accessToken: {
accessTokenAttributes: selectedAccessTokenAttributes?.map((claim: ExternalClaim) => claim.claimURI),
applicationAccessTokenExpiryInSeconds: Number(metadata?.defaultApplicationAccessTokenExpiryTime),
bindingType: values.get("bindingType"),
revokeTokensWhenIDPSessionTerminated: getRevokeStateForSPA(values),
type: values.get("type"),
userAccessTokenExpiryInSeconds: Number(values.get("userAccessTokenExpiryInSeconds")),
validateTokenBinding: isDPoPSelected || values.get("ValidateTokenBinding")?.length > 0
},
grantTypes: values.get("grant"),
idToken: {
audience: audienceUrls !== "" ? audienceUrls.split(",") : [],
expiryInSeconds: Number(values.get("idExpiryInSeconds"))
},
logout: {
frontChannelLogoutUrl: isFrontChannelLogoutEnabled
- ? values.get("frontChannelLogoutUrl")
+ ? (values.get("frontChannelLogoutUrl") ?? initialValues?.logout?.frontChannelLogoutUrl)
: initialValues?.logout?.frontChannelLogoutUrl
},
publicClient: true,
refreshToken: {
expiryInSeconds: values.get("expiryInSeconds")
? parseInt(values.get("expiryInSeconds"), 10)
: Number(metadata?.defaultRefreshTokenExpiryTime),
extendRenewedRefreshTokenExpiryTime: values.get("extendExpiryTime")?.includes("extendExpiryTime"),
renewRefreshToken: values.get("RefreshToken")?.length > 0
},
subjectToken: {
applicationSubjectTokenExpiryInSeconds : values.get("applicationSubjectTokenExpiryInSeconds")
? parseInt(values.get("applicationSubjectTokenExpiryInSeconds"), 10)
: ImpersonationConfigConstants.DEFAULT_SUBJECT_TOKEN_EXPIRY_TIME,
enable : values.get("SubjectToken")?.length > 0
}
};
+ if (!applicationConfig.inboundOIDCForm.showFrontChannelLogout) {
+ delete inboundConfigFormValues.logout;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logout: { | |
| frontChannelLogoutUrl: isFrontChannelLogoutEnabled | |
| ? values.get("frontChannelLogoutUrl") | |
| : initialValues?.logout?.frontChannelLogoutUrl | |
| }, | |
| logout: { | |
| frontChannelLogoutUrl: isFrontChannelLogoutEnabled | |
| ? (values.get("frontChannelLogoutUrl") ?? initialValues?.logout?.frontChannelLogoutUrl) | |
| : initialValues?.logout?.frontChannelLogoutUrl | |
| }, |
🤖 Prompt for AI Agents
In `@features/admin.applications.v1/components/forms/inbound-oidc-form.tsx` around
lines 1694 - 1698, The SPA branch always reads
values.get("frontChannelLogoutUrl") which can be undefined and accidentally
clear an existing URL; update the logout.frontChannelLogoutUrl assignment to
mirror the non‑SPA guard by only using values.get("frontChannelLogoutUrl") when
the front‑channel logout field is shown (isFrontChannelLogoutEnabled /
showFrontChannelLogout), otherwise keep
initialValues?.logout?.frontChannelLogoutUrl; reference the logout object,
frontChannelLogoutUrl key, isFrontChannelLogoutEnabled (or
showFrontChannelLogout), values.get("frontChannelLogoutUrl") and
initialValues?.logout?.frontChannelLogoutUrl when making this conditional
change.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9590 +/- ##
=======================================
Coverage 55.88% 55.88%
=======================================
Files 42 42
Lines 1020 1020
Branches 247 231 -16
=======================================
Hits 570 570
- Misses 416 450 +34
+ Partials 34 0 -34
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
2 participants
Purpose
Related Issues
Related PRs
Checklist
Security checks
Developer Checklist (Mandatory)
product-isissue to track any behavioral change or migration impact.Summary by CodeRabbit