Skip to content
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

WordPress.com Toolbar: fix sign out redirect #8173

Merged
merged 1 commit into from
Nov 24, 2017

Conversation

vindl
Copy link
Member

@vindl vindl commented Nov 15, 2017

Previously the user was redirected to wp-admin/undefined and would not be signed out properly.

Fixes #8142

Testing instructions:

  1. Use test Jetpack site with WordPress.com toolbar module enabled.
  2. Open the profile submenu and click on Sign Out button.
  3. You should signed out and redirected to login page.

Note: this worked properly before, but some unrelated changes in the meantime, which I didn't track down yet, caused it to break. For reference check #6791 where we tested this behavior both for Jetpack site and WP.com side.

Prevously the user was redirected to wp-admin/undefined and
would not be signed out properly. With new changes the sign
out should work as expected and the user will be redirected
to log in page.
@vindl vindl added [Feature] Masterbar WordPress.com Toolbar and Dashboard customizations [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Type] Bug When a feature is broken and / or not performing as intended labels Nov 15, 2017
@vindl vindl added this to the 5.6 milestone Nov 15, 2017
@vindl vindl self-assigned this Nov 15, 2017
@vindl vindl requested review from jeherve and a team November 15, 2017 02:20
@vindl vindl requested a review from a team as a code owner November 15, 2017 02:20
Copy link
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests well! 🚢

@oskosk oskosk added [Status] Ready to Merge Go ahead, you can push that green button! [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Status] Ready to Merge Go ahead, you can push that green button! labels Nov 24, 2017
Copy link
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this is not working right.

I always get logged out from my site successfully but
The automagic logout from .com is weird. After clicking signout, I'm logged out from my site, and I Cannot access some sites like P2s but I'm still able to use wordpress.com (i.e. calypso) and for example see /me without problems)

@oskosk
Copy link
Contributor

oskosk commented Nov 24, 2017

For the records... What I've just said before, happens if I'm proxied. Somehow my session for .com does not get invalidated. Dropping the proxy, shows that I need to login when visiting plain wordpress.com.

@vindl if this is expected behaviour, just tell so and I'll approve this PR again.

@vindl
Copy link
Member Author

vindl commented Nov 24, 2017

@oskosk not sure if it's expected, but it doesn't sound related to this PR. Could be some Calypso weirdness with logged in state while proxied.

@oskosk
Copy link
Contributor

oskosk commented Nov 24, 2017

@vindl But what's the intended behaviour ? To log me out of my site or log me out of my site and wordpress.com ? One of those is not working 100% right. I just found one condition where it does not work. Maybe there are others. Is there a resource I can read about why we log out from wordpress.com ? I don't understand 100% why we do that. This button here looks similar to Calypso's but Calypso's does not log me out of my site, it just logs me out of the system I'm looking at.

@oskosk oskosk added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Nov 24, 2017
Copy link
Contributor

@oskosk oskosk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving as the author said the issue I observed is not related to this PR. 🚢

@ehti
Copy link

ehti commented Nov 24, 2017

This previously used to log us out of WP.com account too, when proxied. It's used quite a lot in AT.

@oskosk
Copy link
Contributor

oskosk commented Nov 24, 2017

@ehti @vindl would it be more appropriate to log the user out from .com only if the user clicking the button is connected to the site via SSO instead?

@ehti
Copy link

ehti commented Nov 24, 2017

@oskosk Perhaps, though not super sure, as the case I mentioned works via SSO.

@oskosk
Copy link
Contributor

oskosk commented Nov 24, 2017

Alright, this may be the topic for another discussion. I'm going to post about it. @ehti could you approve this PR if it works well for you so we can get it merged ?

@vindl
Copy link
Member Author

vindl commented Nov 24, 2017

@vindl But what's the intended behaviour ? To log me out of my site or log me out of my site and wordpress.com ? One of those is not working 100% right.

The intended behavior is to log out both from Jetpack site and WP.com. The reason behind this decision has to do with Atomic sites - we were trying to keep the behavior consistent after the transfer (e.g. what would happen if we performed this action on a dotcom site).

@ehti @vindl should it be more appropriate to log the use from .com only if the user clicking the button is connected to the site via SSO instead?

In the context of Atomic given above, I think it makes sense to log out any WP.com connected user. On the other hand that might be not be expected in case of non-Atomic Jetpack sites.

@vindl
Copy link
Member Author

vindl commented Nov 24, 2017

Just a side note: I'm noticing some weirdness with logout on dotcom sites too (unrelated to Jetpack masterbar). If I open Calypso in one tab, and log out from front end masterbar in another, I can still keep browsing in the first tab even after a hard refresh. The POST requests to API will fail authorization though (You do not have permission to access this resource.) since the user is not really logged in anymore.

@ehti
Copy link

ehti commented Nov 24, 2017

Up to you folks how you'd like to go ahead with this PR, #6791 is the intended behavior as vindl mentioned for consistency. If that's good and the only case affected is when proxied (i.e. not signed out of .com), I think that might be separate indeed. :) (Don't think I can approve the PR? not sure.)

@oskosk
Copy link
Contributor

oskosk commented Nov 24, 2017

@ehti ah don't worry. I'm merging this one then.

, #6791 is the intended behavior as vindl mentioned for consistency

But take in account that there's no consistency on the proposal from #6791. When I logout from calypso I don't expect to be logged out from my sites at the same time (of course unless I'm SSOed)... If we're overriding the button that regular Jetpack users use to logout, it's not good to do magical stuff on the background for them. What if they're drafting or uploading something to Calypso on another tab to another site? This is just unexpected.

@oskosk oskosk merged commit 828774e into master Nov 24, 2017
@oskosk oskosk deleted the fix/8142-masterbar-sign-out branch November 24, 2017 16:08
@oskosk oskosk removed the [Status] Ready to Merge Go ahead, you can push that green button! label Nov 24, 2017
jeherve added a commit that referenced this pull request Nov 24, 2017
oskosk pushed a commit that referenced this pull request Nov 28, 2017
* Changelog 5.6: create base for changelog.

* Update changelog with 5.5.1 info.

* Changelog: add #7930 and #8238

* Changelog: add #8076

* Changelog: add #8100

* Changelog: add #8117

* Changelog: add #8141

* Changelog: add #8143

* Changelog: add #8147

* Changelog: add #8149

* Changelog: add #8153

* Changelog: add #8173

* Changelog: add #8184

* Changelog: add #8196

* Changelog: add #8199

* Changelog: add #8093

* Changelog: add #8171

* Changelog: add #8182

* Changelog: add #8202, #8222

* Changelog: add #8228

* Changelog: add #8240

* Changelog: add #8251

* remove AL card change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Masterbar WordPress.com Toolbar and Dashboard customizations [Type] Bug When a feature is broken and / or not performing as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants