-
Notifications
You must be signed in to change notification settings - Fork 47
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
fix(javascript-sdk): add logout redirect for pingone #443
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit cb6977d. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 2 targetsSent with 💌 from NxCloud. |
becd96e
to
b76dec3
Compare
} catch (error) { | ||
FRLogger.warn('Session logout was not successful'); | ||
} | ||
|
||
try { | ||
await OAuth2Client.revokeToken(configOptions); |
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.
Is this specifically needed for the Ping redirect logout? I see we try/catch it so it won't break older flows, but i'm curious
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.
We just moved this up. There's no code change for this method call.
ed8d603
to
48145b9
Compare
@@ -89,7 +89,8 @@ export async function setupAndGo( | |||
}); | |||
|
|||
page.on('request', async (req) => { | |||
const headers = await req.allHeaders(); | |||
const headers = await req.headers(); |
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.
This removed the "pseudo-headers" and fixed the weird error with :authority
.
48145b9
to
5302c68
Compare
* @function endSession - call authorization server to end associated session | ||
* @param options {LogoutOptions} - an extension of ConfigOptions, but with two additional props | ||
* @param options.logoutRedirectUri {string} - the URL you want the AS to redirect to after signout | ||
* @param options.redirect {boolean} - to explicitly deactivate redirect, pass `false` |
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've moved the signoutUrl
from the central config to the method options, keeping the central config clean.
5302c68
to
561a2c9
Compare
we need the idToken for pingone signout and we also remove it in case we are not in a pingone signout. so we will store the idToken and pass it in, incase. default to empty string
45d9a28
to
3330490
Compare
redirect test case in autoscript
@@ -79,6 +79,7 @@ abstract class Config { | |||
'Missing well-known property. Use `Config.set` method if not using well-known endpoint.', | |||
); | |||
} | |||
// @ts-expect-error safety against runtimes without typescript |
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 left this here because while typescript says we can not have this happen, i figured it was safer for us to keep this check in the code for runtime and using expect error just means we expect this line to error (because we are checking a property that can't exist on the interface)
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.
Is this being used? I don't see callback.html
in the redirectUri
config property.
await page.getByRole('link', { name: 'AuthN: Central Logout Ping' }).click(); | ||
const btn = page.getByRole('button', { name: 'Login' }); | ||
await btn.isVisible(); | ||
await btn.click({ clickCount: 1, delay: 300 }); |
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.
Isn't this quite fragile? Shouldn't we use a wait until element is visible or some non-time-based check? I'm a bit worried that pure time based "sleeps" are not reliable.
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.
we do wait for the button to be visible above, the delay was the only way for me to consistently click the button in the test. for some reason
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.
LGTM
JIRA Ticket
https://bugster.forgerock.org/jira/browse/SDKS-3211
Description
In order to support full, complete logout with OIDC on PingOne, including termination of session and removal of cookie in third-party context, logout redirect is required. This PR adds the redirect, but only in the condition a
logoutRedirectUri
is present in thelogout
options argument, or detection of PingOne endpoint of/as/signoff
inendSession
endpoint URL.There's a "safety switch" that allows the deactivation of this logout redirect regardless of config or endpoint URL, and that's the function's options argument. If you pass in
redirect: false
, it will disable the redirect explicitly in theendSession
method.This is so, in the rare case we have an issue with endpoint naming schemes, there's a way to turn it off.
Type of Change
Please Delete options that are not relevant
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Definition of Done
Check all that apply
Documentation