Skip to content

SSO: Store initial nonce in cookie so that we are not requesting multiple nonces#6571

Merged
ebinnion merged 2 commits intomasterfrom
update/sso-stash-nonce
Mar 7, 2017
Merged

SSO: Store initial nonce in cookie so that we are not requesting multiple nonces#6571
ebinnion merged 2 commits intomasterfrom
update/sso-stash-nonce

Conversation

@ebinnion
Copy link
Copy Markdown
Contributor

@ebinnion ebinnion commented Mar 6, 2017

Possible fix to #6388.

After adding some debug code on the WordPress.com side, as best as I can tell so far, this seems to be a cache issue.

I added logging for when the nonces are created, and if I verify the nonce right after it is created, the nonce is valid.

I also added logging where we validate nonces, and I can see where it is failing. But, if I manually check through the terminal, the nonce returns valid.

So, for now, I'd like suggest this PR as a potential fix. Regardless of whether it fixes the issue as reported in #6388, it should get merged anyways since we don't need to make 2 nonce requests every time a user attempts to log in via SSO.

To test:

  • Ensure site is connected to WP.com
  • Turn on SSO
    -Login, then logout
  • Check cookies
  • Ensure you see:
    • jetpack_sso_nonce
    • jetpack_sso_original_request
    • jetpack_sso_redirect_to
    • jetpack_sso_wpcom_name_*
    • jetpack_sso_wpcom_gravatar_*
  • Log in with WP.com
  • Ensure those cookies disappear after logging in via WP.com or with username and password

@ebinnion ebinnion added [Feature] SSO [Status] Needs Review This PR is ready for review. Bug When a feature is broken and / or not performing as intended labels Mar 6, 2017
@ebinnion ebinnion self-assigned this Mar 6, 2017
@ebinnion ebinnion requested review from enejb, gititon and lezama March 6, 2017 20:55
@ebinnion ebinnion force-pushed the update/sso-stash-nonce branch from a0afcec to 85b0aed Compare March 6, 2017 22:46
@artpi artpi self-requested a review March 7, 2017 02:50
artpi
artpi approved these changes Mar 7, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@artpi artpi left a comment

Choose a reason for hiding this comment

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

LGTM!

@ebinnion ebinnion merged commit aabd842 into master Mar 7, 2017
@ebinnion ebinnion deleted the update/sso-stash-nonce branch March 7, 2017 04:25
@ebinnion ebinnion removed the [Status] Needs Review This PR is ready for review. label Mar 7, 2017
@jeherve jeherve mentioned this pull request Mar 7, 2017
Closed
@amitmalewar
Copy link
Copy Markdown

amitmalewar commented Mar 7, 2017

Thanks for the patch.
I have updated my code. This time I faced the same problem.

Error: Invalid Nonce.

But after error visiting https://www.domain.com/wp-admin shows me logged into the WordPress dashboard

@ebinnion
Copy link
Copy Markdown
Contributor Author

ebinnion commented Mar 7, 2017 •
edited
Loading

Hey @amitmalewar - Thanks for the extra information. Do you know if your remote Jetpack user had been linked to WP.com yet?

@amitmalewar
Copy link
Copy Markdown

amitmalewar commented Mar 7, 2017

Yea, it is linked.

@jeherve jeherve added this to the 4.7.1 milestone Mar 8, 2017
@dereksmart
Copy link
Copy Markdown
Contributor

dereksmart commented Mar 8, 2017 •
edited
Loading

Merged to branch-4.7 dc7c395 8a9a507 -- Will be in the next point

lamosty
lamosty reviewed Mar 8, 2017
View reviewed changes
modules/sso.php
static function clear_cookies_after_login() {
self::clear_wpcom_profile_cookies();
if ( isset( $_COOKIE[ 'jetpack_sso_nonce' ] ) ) {
setcookie(
Copy link
Copy Markdown
Contributor

@lamosty lamosty Mar 8, 2017

Choose a reason for hiding this comment

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

Is it safe to store the sso nonce in a cookie?

Copy link
Copy Markdown
Contributor Author

@ebinnion ebinnion Mar 8, 2017

Choose a reason for hiding this comment

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

The cookie is deleted immediately after the user successfully logs in. And if the user doesn't log in, the cookie is only good for 10 minutes.

Further, the nonce isn't tied to any user. It's used to verify intent. So, I think it's OK to store in it in the cookie temporarily.

Copy link
Copy Markdown
Contributor

@lamosty lamosty Mar 8, 2017

Choose a reason for hiding this comment

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

Ah, I see. Thanks for the explanation. :)

@ebinnion
Copy link
Copy Markdown
Contributor Author

ebinnion commented Mar 8, 2017

@amitmalewar - one of my coworkers just pushed a commit to force cache propagation on the WPCOM side. Would you mind testing again?

@amitmalewar
Copy link
Copy Markdown

amitmalewar commented Mar 9, 2017

@ebinnion - Done!!
seems to fix the issue.
Thanks for the help :)

jeherve added a commit that referenced this pull request Mar 9, 2017
@jeherve
Changelog: add #6571
7cf85be
dereksmart pushed a commit that referenced this pull request Mar 9, 2017
@jeherve @dereksmart
Jetpack 4.7.1: Changelog. (#6612)
f144a81 
* Update stable tag.

* Move 4.7 changelog to changelog.txt

* Changelog: add #6571

* Changelog: add #6600

* Changelog: add #6604

* Changelog: add #6608

* Changelog: remove PR numbers and add release date and link.

* Changelog: add #6615
dereksmart pushed a commit that referenced this pull request Mar 9, 2017
@jeherve @dereksmart
Jetpack 4.7.1: Changelog. (#6612)
c973333 
* Update stable tag.

* Move 4.7 changelog to changelog.txt

* Changelog: add #6571

* Changelog: add #6600

* Changelog: add #6604

* Changelog: add #6608

* Changelog: remove PR numbers and add release date and link.

* Changelog: add #6615
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@artpi artpi artpi approved these changes

@lezama lezama Awaiting requested review from lezama

@enejb enejb Awaiting requested review from enejb

@gititon gititon Awaiting requested review from gititon

+1 more reviewer

@lamosty lamosty lamosty left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@ebinnion ebinnion

Labels

Bug When a feature is broken and / or not performing as intended [Feature] SSO

Projects

None yet

Milestone

4.7.1

Development

Successfully merging this pull request may close these issues.

6 participants

@ebinnion @amitmalewar @dereksmart @artpi @lamosty @jeherve