Skip to content

WordPress.com Toolbar: Make Sign Out action logout of WordPress.com#6791

Merged
dereksmart merged 2 commits intomasterfrom
fix/masterbar-sign-out
Mar 31, 2017
Merged

WordPress.com Toolbar: Make Sign Out action logout of WordPress.com#6791
dereksmart merged 2 commits intomasterfrom
fix/masterbar-sign-out

Conversation

@vindl
Copy link
Copy Markdown
Member

@vindl vindl commented Mar 28, 2017 •
edited
Loading

Previously, Sign Out action in Me menu was just signing out users from their current Jetpack site, and it should be signing them out of WP.com too.

Test with: D4938-code (deployed)
Fixes #6490

Testing instructions:

  1. In Me sub-menu of WordPress.com Toolbar, click on Sign Out button.
  2. Verify that you have been logged out of you Jetpack test site.
  3. Verify that you have been logged out from WordPress.com.

@vindl vindl added [Feature] Masterbar WordPress.com Toolbar and Dashboard customizations [Status] Needs Review This PR is ready for review. Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Mar 28, 2017
@vindl vindl self-assigned this Mar 28, 2017
@vindl vindl added the Bug When a feature is broken and / or not performing as intended label Mar 28, 2017
@vindl
WordPress.com Toolbar: Sing out should affect dotcom account too
c3b4118 
Previously, clicking on Sign out action would log out user only from his current Jetpack site, while the expected result would be to log them out of WordPress.com too.
@vindl vindl force-pushed the fix/masterbar-sign-out branch from 53152ff to c3b4118 Compare March 28, 2017 09:59
jeherve
jeherve reviewed Mar 28, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

When I click to sign out, I get redirected to this error page:

screen shot 2017-03-28 at 12 40 03

If I click on the link to confirm that I want to log out, I'm logged out and redirected to https://wordpress.com/wp-login.php?loggedout=true

I would have expected to be logged out right away, and redirected to my site's home page.

@eliorivero
Copy link
Copy Markdown
Contributor

eliorivero commented Mar 28, 2017

I got the same thing than Jeremy.
After coming back in the browser and reloading, the login screen is shown. I checked wordpress.com and found that I was still logged in there. So I only got logged out of the Jetpack site but not from WordPress.com.

@eliorivero eliorivero added [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. and removed [Status] Needs Review This PR is ready for review. labels Mar 28, 2017
@vindl
Copy link
Copy Markdown
Member Author

vindl commented Mar 28, 2017

@jeherve @eliorivero I'm aware of those shortcomings of the current solution, but I'm not sure what would be the best way to overcome them.

  1. In order for redirection to Jetpack site's home page to work after logging out form WordPress.com, we'd have to allow redirects to arbitrary URLs (limited to WordPress.com now, I think for security reasons).

  2. In order to log out immediately without confirmation, we'd have to somehow obtain a valid nonce to be used with logout URL.

Any suggestions on how to resolve this would be greatly appreciated.
The existing solution just logs the user out of Jetpack site, and redirects correctly. I'm not sure if existing state makes more sense than the solution I'm proposing here.

@vindl vindl removed the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Mar 28, 2017
@zinigor
Copy link
Copy Markdown
Contributor

zinigor commented Mar 28, 2017 •
edited
Loading

I can see several possible ways out of this:

  1. not allowing signing out of WordPress.com from the Masterbar. Is there a good reason we want to do that other than feature parity? I, for one, would find it very annoyed to be logged out of WordPress.com when I'm logging out of my self-hosted site.
  2. triggering a sync action that will invalidate the current user's session on WordPress.com. @lezama or @enejb can you tell us if that's technically possible?

@vindl
Copy link
Copy Markdown
Member Author

vindl commented Mar 28, 2017

Is there a good reason we want to do that other than feature parity?

@zinigor Not that I know of, and I'm fine with leaving it as is (logging out limited to self-hosted site).

@vindl
Copy link
Copy Markdown
Member Author

vindl commented Mar 28, 2017

After giving it some more thought - this might make sense only for AT sites, and not for Jetpack sites in general.

@eliorivero
Copy link
Copy Markdown
Contributor

eliorivero commented Mar 28, 2017

That blank page asking for confirmation to log out of WordPress.com is going to be confusing for some users. Maybe that can be stylized on WordPress.com side to look more like a WordPress.com legit page and not like an warning/error page?

@eliorivero
Copy link
Copy Markdown
Contributor

eliorivero commented Mar 28, 2017

And also update the message. If we can package that action in a nice design, the users won't be confused. Right now since it has a design (or no design at all) completely different from the Jetpack UI or Calypso, it looks like an error page. An intentionally designed page will reduce the friction and will tell users they're in the right place.

The button Sign Out is also a bit confusing in its label. It's not clear for users if it means sign out of the current site or WordPress.com. Users guessing it will only log them out of WordPress.com are in for a good surprise. And on the other side, users guessing it will log them out of the current site will also be surprised that they're redirected to log them out of WordPress.com.

@lezama
Copy link
Copy Markdown
Contributor

lezama commented Mar 28, 2017

triggering a sync action that will invalidate the current user's session on WordPress.com. @lezama or @enejb can you tell us if that's technically possible?

I don't see why not 👍

@samhotchkiss samhotchkiss added [Status] Needs Review This PR is ready for review. [Pri] BLOCKER labels Mar 28, 2017
@vindl vindl requested review from enejb and lezama March 29, 2017 15:07
@vindl
Copy link
Copy Markdown
Member Author

vindl commented Mar 29, 2017 •
edited
Loading

After discussing this with @lezama, I've added a new sync action that we can use to perform WP.com logout if necessary. We can decide if we want to do this for all Jetpack sites, or for AT sites only on WP.com side.

@zinigor
Copy link
Copy Markdown
Contributor

zinigor commented Mar 29, 2017

Doesn't work for me, does it need a dotcom counterpart that hasn't been committed yet?

@vindl
Copy link
Copy Markdown
Member Author

vindl commented Mar 29, 2017 •
edited
Loading

@zinigor it does, I'm working on a diff for dotcom side (D4938-code). Update: diff deployed.

@eliorivero
Copy link
Copy Markdown
Contributor

eliorivero commented Mar 29, 2017

@vindl for consistency, it should perform the same action in Jetpack and AT sites.

@vindl
Copy link
Copy Markdown
Member Author

vindl commented Mar 29, 2017

@eliorivero roger that, that's how it works in proposed diff/PR.

@zinigor zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review This PR is ready for review. labels Mar 29, 2017
zinigor
zinigor approved these changes Mar 29, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@zinigor zinigor left a comment

Choose a reason for hiding this comment

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

Works like a charm together with the dotcom counterpart. I knew Sync was magic!

@vindl vindl mentioned this pull request Mar 29, 2017
Closed
@dereksmart
Copy link
Copy Markdown
Contributor

dereksmart commented Mar 31, 2017

Works as expected

@dereksmart dereksmart merged commit 015ecf8 into master Mar 31, 2017
@dereksmart dereksmart deleted the fix/masterbar-sign-out branch March 31, 2017 13:30
@dereksmart dereksmart removed the [Status] Ready to Merge Go ahead, you can push that green button! label Mar 31, 2017
dereksmart pushed a commit that referenced this pull request Mar 31, 2017
@vindl @dereksmart
WordPress.com Toolbar: Make Sign Out action logout of WordPress.com (#…
caea32b 
…6791)

* WordPress.com Toolbar: Sing out should affect dotcom account too

Previously, clicking on Sign out action would log out user only from his current Jetpack site, while the expected result would be to log them out of WordPress.com too.

* Add sync action to logout user from WP.com if necessary
@dereksmart
Copy link
Copy Markdown
Contributor

dereksmart commented Mar 31, 2017

Merged to 4.8 caea32b

@vindl vindl mentioned this pull request Nov 15, 2017
Merged
@jeherve jeherve mentioned this pull request May 9, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jeherve jeherve jeherve left review comments

@beaulebens beaulebens Awaiting requested review from beaulebens

@roundhill roundhill Awaiting requested review from roundhill

@eliorivero eliorivero Awaiting requested review from eliorivero

@dereksmart dereksmart Awaiting requested review from dereksmart

@lezama lezama Awaiting requested review from lezama

@enejb enejb Awaiting requested review from enejb

+1 more reviewer

@zinigor zinigor zinigor approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@vindl vindl

Labels

Bug When a feature is broken and / or not performing as intended Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Feature] Masterbar WordPress.com Toolbar and Dashboard customizations [Pri] BLOCKER

Projects

None yet

Milestone

4.8

Development

Successfully merging this pull request may close these issues.

7 participants

@vindl @eliorivero @zinigor @lezama @dereksmart @jeherve @samhotchkiss