Jetpack Connect URL: use correct URL to auto-approve by roccotripaldi · Pull Request #8881 · Automattic/jetpack

roccotripaldi · 2018-02-19T22:00:59Z

already_authorized is a connection URL param that lets a connecting customer know they've already connected the site.

auth_approved is a connection URL param that let's a connecting customer skip
the screen that shows the terms-of-service-acceptance button.

In #8544 we meant to use auth_approved

Fixes #8783

To reproduce:

On an unconnected jetpack site, try connecting from the banner on the plugins page. Not that you get an odd "Already connected..." error when you land on wordpress.com

Then apply this patch to your jetpack site, and try again from the banner on the plugins page. You will not see the error, and you should skip the terms-of-service-acceptence and be auto authorized.

Proposed changelog entry for your changes:

We fixed an issue that sometimes resulted in a notice being shown about another user already having connected a Jetpack site when attempting to connect your site to WordPress.com

…g customer know they've already connected the site. `auth_approved` is a connection URL param that let's a connecting customer skip the screen that shows the terms-of-service-acceptance button. In #8544 we meant to use `auth_approved`

oskosk · 2018-02-19T22:52:41Z

Couldn't reproduce this the first times... And then I found a way to reproduce which is:

On a fresh site. Click the banner "Set Up Jetpack" button.
When arriving the first calypso screen, go back with the browser.
Click the Set Up Jetpack button again.
Expect to see the "already connected..." notice.

Then tested with this PR and didn't see that ^ behaviour.
But I don't see any approval screen being skipped as result of this other parameter either. I get to see this same screen regardless of the URL if I click the button.

oskosk

Approving because this actually fixes a bug which results in a dead end.

oskosk · 2018-02-19T22:58:02Z

I now got to see the step skipped after trying multiple times and going back and forward...

The only thing that made it work, was switching branches on Jetpack Beta from this branch to Bleeding Edge and back. After that, clicking the Setup Jetpack button I got to skip the authorize screen...
Then, disconnected again, clicked the banner button again (with this PR's changes) and got presented with the authorize screen. So looks like the feature is working randomly.

sirreal

It seems that this is the problem.

Thanks @roccotripaldi !

sirreal · 2018-02-20T10:10:30Z

I spent some time tracking down why this only happens before connection has been attempted.

Here's the short version:

The connection banner naïvely adds a query arg to the build_connection_url:

jetpack/class.jetpack-connection-banner.php

Lines 82 to 87 in b33be10

    
           $url = Jetpack::init()->build_connect_url( 
        
           	true, 
        
           	false, 
        
           	sprintf( 'banner-%s-slide-%s-%s', $jp_version_banner_added, $slide_num, $current_screen->base ) 
        
           ); 
        
           return add_query_arg( 'already_authorized', 'true', $url );

The URL may be WP Admin, which is required to get a client_id before going through the connection, but if the client_id is already known it will directly point to jetpack.wordpress.com:

jetpack/class.jetpack.php

Lines 4428 to 4432 in b33be10

    
           function build_connect_url( $raw = false, $redirect = false, $from = false, $register = false ) { 
        
           	$site_id = Jetpack_Options::get_option( 'id' ); 
        
           	$token = Jetpack_Options::get_option( 'blog_token' ); 
        
           	if ( $register || ! $token || ! $site_id ) {

That's why you must hit the endpoint at WP Admin once before this bug manifests.

The WP Admin endpoint silently discards unknown params and rebuilds the connection URL for redirection. Ideally this param would be part of build_connect_url.

I've pushed a commit to maintain the param through the WP Admin redirection (d8e7a6c)

See p1519119812000072-slack-luna

This banners may include `auth_approved` which is intended to be sent to the connection API endpoint. Ensure relevant `auth_approved` params are included if provided when redirecting to the connection endpoint.

sirreal

I've tested and this works well, on first (WP Admin) and subsequent (jetpack.wordpress.com) auths.

I do think that the logic should be moved into build_connect_url.

oskosk · 2018-02-20T12:08:33Z

I do think that the logic should be moved into build_connect_url.

@tyxla what do you think about it? Something similar happened with the onboarding param, but in this case it was problematic when the logic was inside build_connect_url() and we ended up pulling it out.

tyxla

Tests well 👍 thank you!

tyxla · 2018-02-20T12:21:23Z

@oskosk it was problematic because we were adding some extra parameters every time the user has an onboarding token. That's not the case here, so it looks safe from that point of view 👍

I'd only be hesitant to introduce it to build_connect_url just because it's used in so many places, and we'd need to test a lot more cases before making sure it doesn't break anything else.

So, if we decide to move it to build_connect_url, let's make sure to test all places where it's called thoroughly and in an organized manner.

* update changelog.txt * Update readme.txt with scaffolding for 5.9 changelog and release draft shortlink * Add changelog entry for #8243 * Add changelog entry for #8296 * Add changelog entry for #8367 * Add changelog entry for #8686 * Add changelog entry for #8707 * Add changelog entry for #8709 and #8714 * Add changelog entry for #8729 * Add changelog entry for #8777 * Add changelog entry for #8780 * Add changelog entry for #8786 * Add changelog entry for #8787 * Add changelog entry for #8801 #8805 #8832 #8865 and #8804 * Add changelog entry for #8817 * Add changelog entry for #8822 * Add changelog entry for #8823 * Add changelog entry for #8829 * Add changelog entry for #8834 * move some items to major enhancements * Add changelog entry for #8836 * Add changelog entry for #8839 * Add changelog entry for #8861 * Add changelog entry for #8862 * Add changelog entry for #8863 * Add changelog entry for #8866 * Add changelog entry for #8870 * Add changelog entry for #8874 * Add changelog entry for #8875 * Add changelog entry for #8881 * Add changelog entry for #8890 * Add changelog entry for #8911 * Add changelog entry for #8927 * Add changelog entry for #8931 * Add changelog entry for #8933 * Add changelog entry for #8930 * fix wording * typo * minor fixes * replace partner scripts for Jetpack Start in changelog entry * Update to-test.md * Update to-test.md * minor style fixes to to-test.md * minor style fixes to to-test.md * minor fixes on to-test.md * Add changelog entry for #8868 * Add changelog entry for #8844 * Add changelog entry for #8664 * Add changelog entry for #8935 * Add changelog entry for #8425 * Add changelog entry for #8625

roccotripaldi added Bug When a feature is broken and / or not performing as intended [Pri] High labels Feb 19, 2018

roccotripaldi requested review from johnHackworth, seear and sirreal February 19, 2018 22:00

roccotripaldi requested a review from a team as a code owner February 19, 2018 22:01

oskosk added this to the 5.9 milestone Feb 19, 2018

oskosk approved these changes Feb 19, 2018

View reviewed changes

sirreal approved these changes Feb 20, 2018

View reviewed changes

jeherve added [Status] Needs Review This PR is ready for review. General labels Feb 20, 2018

Keep auth_approved param from WP Admin connect redir

d8e7a6c

This banners may include `auth_approved` which is intended to be sent to the connection API endpoint. Ensure relevant `auth_approved` params are included if provided when redirecting to the connection endpoint.

sirreal force-pushed the fix/8783-jetpack-connection-bug branch from a017ea1 to d8e7a6c Compare February 20, 2018 10:33

sirreal approved these changes Feb 20, 2018

View reviewed changes

sirreal mentioned this pull request Feb 20, 2018

Connection: Giving Incorrect Error about Site Connected to Another Account when Connection Fails #8783

Closed

oskosk 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 Feb 20, 2018

tyxla approved these changes Feb 20, 2018

View reviewed changes

oskosk merged commit a4f4f2d into master Feb 20, 2018

matticbot added the [Status] Needs Changelog label Feb 20, 2018

oskosk deleted the fix/8783-jetpack-connection-bug branch February 20, 2018 12:28

oskosk added [Status] Has Changelog and removed [Status] Needs Changelog labels Feb 27, 2018

oskosk added a commit that referenced this pull request Feb 27, 2018

Add changelog entry for #8881

8e496aa

kraftbj removed the [Status] Ready to Merge Go ahead, you can push that green button! label Oct 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Jetpack Connect URL: use correct URL to auto-approve#8881

Jetpack Connect URL: use correct URL to auto-approve#8881
oskosk merged 2 commits intomasterfrom
fix/8783-jetpack-connection-bug

roccotripaldi commented Feb 19, 2018 •

edited by oskosk

Loading

Uh oh!

oskosk commented Feb 19, 2018

Uh oh!

oskosk left a comment

Uh oh!

oskosk commented Feb 19, 2018

Uh oh!

sirreal left a comment

Uh oh!

sirreal commented Feb 20, 2018 •

edited

Loading

Uh oh!

sirreal left a comment

Uh oh!

oskosk commented Feb 20, 2018

Uh oh!

tyxla left a comment

Uh oh!

tyxla commented Feb 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

roccotripaldi commented Feb 19, 2018 • edited by oskosk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changelog entry for your changes:

Uh oh!

oskosk commented Feb 19, 2018

Uh oh!

oskosk left a comment

Choose a reason for hiding this comment

Uh oh!

oskosk commented Feb 19, 2018

Uh oh!

sirreal left a comment

Choose a reason for hiding this comment

Uh oh!

sirreal commented Feb 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sirreal left a comment

Choose a reason for hiding this comment

Uh oh!

oskosk commented Feb 20, 2018

Uh oh!

tyxla left a comment

Choose a reason for hiding this comment

Uh oh!

tyxla commented Feb 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

roccotripaldi commented Feb 19, 2018 •

edited by oskosk

Loading

sirreal commented Feb 20, 2018 •

edited

Loading