Sign out at the provider before clearing anything locally - #148
Merged
Conversation
Dex publishes end_session_endpoint once sessions are enabled, and the app reads that to decide whether signing out has anywhere to go -- so enabling them turned every sign-out into an RP-initiated logout, and the order this ran in started to matter. signoutRedirect() reads the stored user for id_token_hint and removes the user itself. Clearing sessionStorage and calling removeUser() first only cost the hint, which Dex answers 400 to, and dropped isAuthenticated while the session was still live -- so RequireAuth signed straight back in against it and the click read as a page reload rather than a sign-out. The provider goes first now, and the local clear happens only where there was no session to end. That path navigates, because the signed-out flag is read at mount and nothing else brings the app back through one. Both new assertions fail against the pre-fix UserContext: one on the call order, one on the local path not navigating. sandboxes-app carried the same defect; chat-app and tracing-app already redirected first.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split out of #147, which is red for an unrelated reason:
e13ee47needsEnvironment.persistentShells, and CI regeneratessrc/gen/from the BSR where that field does not exist yet. This branch carries only the sign-out change, offmain.Enabling Dex sessions makes it publish
end_session_endpoint, which the app reads to decide whether signing out has anywhere to go — so every sign-out became an RP-initiated logout and the ordering started to matter.signoutRedirect()reads the stored user forid_token_hintand removes the user itself. Clearing sessionStorage and callingremoveUser()first only cost the hint — which Dex answers 400 to — and droppedisAuthenticatedwhile the session was still live, soRequireAuthsigned straight back in and the click read as a page reload.Verified against a Dex build with sessions on, on the local VM: logout to a registered
post_logout_redirect_uriredirects, an unregistered one is refused 400, and both new assertions fail against the pre-fixUserContext.