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

[mod] Redirect after logout if `r` URI argument exists #109

Merged
merged 1 commit into from Oct 27, 2018

Conversation

Projects
None yet
3 participants
@tituspijean
Contributor

tituspijean commented Sep 15, 2018

The problem

An app cannot ask for a logout without breaking UX: users end up on SSOwat login form, whereas going back to the app could be preferable in some cases.

Solution

This PR addresses YunoHost/issues#1201.
The modification makes logout() helper test if r URI argument exists after logging out users. If so, the base64 address is decoded and nginx is tasked with the redirection.

PR Status

Ready to be reviewed.

Testing

Example: Flarum package is bundled with an SSOwat extension to support YNH users. It already puts an r argument while disconnecting YNH users and redirection has been successfully tested.

Validation

  • Principle agreement 0/2 :
  • Quick review 0/1 :
  • Simple test 0/1 :
  • Deep review 0/1 :
@alexAubin

Anyone has an opinion on this ? (including @YunoHost/apps ?)

Naively it looks legit to me but I haven't digged the issue / I don't have all use cases in mind

@alexAubin

This comment has been minimized.

Member

alexAubin commented Oct 27, 2018

Yolomerging because this looks legit~
Thanks !

@alexAubin alexAubin merged commit 3ad2151 into YunoHost:stretch-unstable Oct 27, 2018

-- Redirect to portal anyway
return redirect(conf.portal_url)
-- Redirect with the `r` URI argument if it exists or redirect to portal
if args.r then

This comment has been minimized.

@zamentur

zamentur Nov 4, 2018

Contributor

I think it should be in the "if is_logged_in() then" No ?

This comment has been minimized.

@tituspijean

tituspijean Nov 4, 2018

Contributor

No, I don't think so. The function logout() already assumes that the user is logged in (as per line 884) and delete their cookies/session. Then my proposed code adds a check for the r argument to use it for redirection, and otherwise defaults to the initial redirection to portal.

@alexAubin alexAubin modified the milestones: 3.x, 3.3.x Nov 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment