Update the way we generate / verify XMLRPC connections by roccotripaldi · Pull Request #7115 · Automattic/jetpack

roccotripaldi · 2017-05-03T02:51:53Z

Fixes #6879 and #6882 and ensures that each verification secret is unique.

Testing instructions:

apply this to one of your testing sites and run through the various connection flows ( see P7rd6c-Me-p2 )
test that there are no regressions when setting up a publicize connection

Proposed changelog entry for your changes:

Fixes a bug that causes connection errors.

Re-factoring Jetpack_XMLRPC_Server::verify_action to look for transients. See discussion in p2-p7rcWF-lF

Requires D5453

zinigor

Great work, thank you! I have tested it, seems to work fine! Plus, I have added some unit tests that cover edge cases.

There are several things that worry me here:

transients can fail on save, I have added a check in case we can't save a transient, but it needs proper handling - especially when generate_secrets gets called from build_connect_url, we're not really expecting it to fail anywhere. Should we throw a fatal if a transient doesn't get saved?
transients can disappear faster than their expiration time, they aren't a reliable storage solution. Do you think that's a potential problem? I know that it's probably fine because the time slot is only 10 minutes now, but still it's something to think about.

roccotripaldi · 2017-05-03T20:57:36Z

Perhaps we can use a generic option ( rather than a Jetpack Option ) instead of a transient.
I wasn't aware that transients were so unreliable. What do you think?

lezama · 2017-05-04T16:12:11Z

class.jetpack.php

 				'fallback_no_verify_ssl_certs',
 			)
 		);
-


lezama · 2017-05-04T16:16:26Z

class.jetpack.php

-	public function generate_secrets( $action, $exp = 600 ) {
-	    $secret = wp_generate_password( 32, false ) // secret_1
-	    		. ':' . wp_generate_password( 32, false ) // secret_2
-	    		. ':' . ( time() + $exp ) // eol ( End of Life )


what was this for?

nevermind, expiration date

@zinigor

per a Slack conversation with @zinigor.

zinigor · 2017-05-08T18:45:31Z

class.jetpack.php

 		Jetpack_Options::delete_option(
 			array(
-				'register',
+				'authorize',


Did you mean secrets here as well?

Thanks for catching that!

roccotripaldi · 2017-05-09T00:52:19Z

tests/php/test_class.jetpack.php

+	 * @author zinigor
+	 * @covers Jetpack::generate_secrets
+	 */
+	function test_generate_secrets_works_with_filters() {


Awesome test!

zinigor

I have fixed two minor issues and left some questions. But otherwise everything seems to work as it's supposed to, thanks!

zinigor · 2017-05-09T08:23:27Z

class.jetpack.php

-	    		. ':' . get_current_user_id(); // ties the secrets to the current user
-		Jetpack_Options::update_option( $action, $secret );
-	    return Jetpack_Options::get_option( $action );
+	public static function generate_secrets( $action, $exp = 600 ) {


While I don't think we'll ever need it, what do you think about adding the user ID as the second argument to the method? It will let us keep the method interfaces more consistent.

zinigor · 2017-05-09T08:25:22Z

class.jetpack.php

+		return $secrets[ $secret_name ];
+	}
+
+	public static function get_secret( $action, $user_id ) {


Let's use get_secrets here and delete_secrets down there?

zinigor · 2017-05-09T08:26:07Z

class.jetpack.php

+	public static function get_secret( $action, $user_id ) {
+		$secret_name = 'jetpack_' . $action . '_' . $user_id;
+		$secrets = Jetpack_Options::get_option( 'secrets', array() );
+		if ( isset( $secrets[ $secret_name ] ) ) {


What do you think about checking for expiration here and deleting the secret if it has expired? Currently it's sort of backwards: we check for expiration time after calling generate_secrets, when it should (and does, i think) guarantee secret freshness.

…Options instead of Jetpack_Options to avoid any cacheing issues while generating / validating secrets.

roccotripaldi · 2017-05-10T02:30:41Z

are you considering adding a get_rare_option method?

Definitely! In 038913d i switched to Jetpack_Sync_Options - I created a new issue #7160 and we can deal with the refactor in an other PR, as this one is getting largish.

…zinigor

…zinigor Added new unit tests to cover errors.

roccotripaldi · 2017-05-10T03:00:13Z

Ok, added some new commits to address feedback from @zinigor and @lezama - This could use an other round of testing, but I think we are close!

zinigor · 2017-05-10T07:26:42Z

Awesome job, thank you!

zinigor

Uh oh! Just got a fatal error trying to connect:

PHP Fatal error:  Uncaught Error: Class 'Jetpack_Sync_Options' not found in /wordpress/wp-content/plugins/jetpack/class.jetpack.php:4576
Stack trace:
#0 /wordpress/wp-content/plugins/jetpack/class.jetpack.php(4690): Jetpack::generate_secrets('register')
#1 /wordpress/wp-content/plugins/jetpack/class.jetpack.php(2950): Jetpack::register()
#2 /wordpress/wp-content/plugins/jetpack/class.jetpack.php(3709): Jetpack::try_registration()
#3 /wordpress/wp-content/plugins/jetpack/_inc/lib/admin-pages/class.jetpack-admin-page.php(113): Jetpack->admin_page_load()
#4 /wordpress/wp-includes/class-wp-hook.php(298): Jetpack_Admin_Page->admin_page_load('')
#5 /wordpress/wp-includes/class-wp-hook.php(323): WP_Hook->apply_filters(NULL, Array)
#6 /wordpress/wp-includes/plugin.php(453): WP_Hook->do_action(Array)
#7 /wordpress/wp-admin/admin.php(212): do_action('load-toplevel_p...')
#8 {main}
 thrown in /wordpress/wp-content/plugins/jetpack/class.jetpack.php on line 4576

Using `require_once` will only be a temporary measure because for 5.0 we also plan to move Jetpack_Sync_Options methods into Jetpack_Options.

dereksmart · 2017-05-17T15:29:53Z

Tested very well for the following:

Single site connections
Secondary user connections
multisite network and subsite connections
publicize connections

Thank you Rocco. 🐑 !

@see

* Changelog: first pass at a changelog for 5.0 * Changelog: delete 4.9 testing list. * Changelog: update minimum WP version to match ver. in jetpack.php Fixes #7158 * Changelog: add #6051 * Changelog: add #6753 * Changelog: add #6928 * Changelog: add #6964 * Changelog: add #7014 * Changelog: add #7057 * Changelog: add #7060 * Changelog: add #7068 * Changelog: add #7070 * Changelog: add #7072 * Changelog: add #7071 * Changelog: add release date and post shortlink. * Changelog: add #7094 * Changelog: add #7100 * Changelog: add #7108 * Changelog: add #7113 * Changelog: add #7123 * Changelog: add #7135 * Changelog: add #7143 * Changelog: add #7151 * Changelog: add #6996 * Changelog: add #7105 * Changelog: add #7132 * Changelog: add #7166 * Changelog: fix typo in 4.9 changelog. * Changelog: remove older releases' changelogs. @see p1HpG7-42e-p2 * Changelog: add #7090 * Changelog: add #7095 * Changelog: add #7112 * Changelog: add #7115 * Changelog: add #7122 * Changelog: add #7137 * Changelog: add #7138 * Changelog: add #7140 * Changelog: add #7154 * Changelog: add ##7155 * Changelog: add #7163 * Changelog: add #7167 * Changelog: add #7171 * Changelog: add #7180 * Changelog: add #7181 * Changelog: add #7183 * Changelog: add #7184 * Changelog: add #7189 * Changelog: add #7191 * Changelog: add #7193 * Changelog: add #7198 * Changelog: add #7200 * Changelog: add #7209 * Changelog: add #7212 * Testing list: add instructions for #7115 * Changelog: add #7188 * Changelog: add #7205 * Changelog: add #7225 * Changelog: add #6872 * Changelog: add #7107 * Changelog: add #7118 * Changelog: add #7142 * Changelog: add #7170 * Changelog: add #7210 * Changelog: add #7218 * Changelog: add #7232 * Changelog: add #7211 * Changelog: add #7213 * Changelog: add #7229 * Changelog: add #7230 * Changelog: add #7214 * Draft changelog for 5.0 * Changelog updates: 2nd pass at a clearer changelog. - Fix typos. - Use consistent tense and tone across all changelog. - Remove unclear items. * Changelog: add #7026 * Changelog: add #7058 * Changelog: add #7125 * Changelog: add #7249 * Changelog: add #7185 * add mentions of image widget migration * Changelog: add info about new output for CLI command. * Changelog: add WP version number matching the new Image Widget.

roccotripaldi added 2 commits May 2, 2017 21:16

Re-factoring Jetpack::generate_secrets to use set_transient.

992ddfb

Re-factoring Jetpack_XMLRPC_Server::verify_action to look for transients. See discussion in p2-p7rcWF-lF

Update the way secrets for publicize are created.

3a3ea9c

Requires D5453

roccotripaldi requested review from lezama and zinigor May 3, 2017 02:51

matticbot added the Touches WP.com Files label May 3, 2017

roccotripaldi changed the title ~~XMLRPC: update the way we generate / verify XMLR~~ XMLRPC: update the way we generate / verify XMLRPC connections May 3, 2017

roccotripaldi changed the title ~~XMLRPC: update the way we generate / verify XMLRPC connections~~ Update the way we generate / verify XMLRPC connections May 3, 2017

roccotripaldi added [Status] Needs Review This PR is ready for review. Bug When a feature is broken and / or not performing as intended labels May 3, 2017

roccotripaldi requested review from ebinnion and gravityrail May 3, 2017 02:58

roccotripaldi mentioned this pull request May 3, 2017

Alternate communications mechanism for hosts that block xmlrpc.php #7080

Closed

roccotripaldi and others added 3 commits May 2, 2017 23:27

Update unit tests

4211ca7

Cleaned up whitespace.

a1ac80d

Added test suite for generate_secrets.

3a73515

zinigor approved these changes May 3, 2017

View reviewed changes

Omitting two tests that require PHPUnit 5+ because of willReturn.

9cb4891

lezama reviewed May 4, 2017

View reviewed changes

class.jetpack.php

'fallback_no_verify_ssl_certs',

)

);

Copy link
Copy Markdown

Contributor

lezama May 4, 2017

Choose a reason for hiding this comment

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

.

lezama reviewed May 4, 2017

View reviewed changes

Refactor to use a serialized Jetpack Option instead of a transient,

f33ddd4

per a Slack conversation with @zinigor.

zinigor reviewed May 8, 2017

View reviewed changes

roccotripaldi added 2 commits May 8, 2017 16:54

Make sure we delete 'secrets' on disconnect.

11824b4

Fix incorrect operator.

4cd179f

roccotripaldi commented May 9, 2017

View reviewed changes

zinigor added 2 commits May 9, 2017 12:13

Fixed an undefined variable error.

5d95210

Using the right method to update the option.

df58e7e

zinigor suggested changes May 9, 2017

View reviewed changes

jeherve 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 May 9, 2017

adjust spacing

f6517ef

roccotripaldi force-pushed the update/connect-url-authorize-secret branch from 3cb6dfb to f6517ef Compare May 10, 2017 02:19

Based on a slack conversation with @lezama, we will use Jetpack_Sync_…

038913d

…Options instead of Jetpack_Options to avoid any cacheing issues while generating / validating secrets.

Change the way we handle expired secrets. Thanks for the suggestion @…

16b7b18

…zinigor Added new unit tests to cover errors.

zinigor approved these changes May 10, 2017

View reviewed changes

zinigor suggested changes May 10, 2017

View reviewed changes

Make sure we have access to Jetpack_Sync_Options

d72a44c

Using `require_once` will only be a temporary measure because for 5.0 we also plan to move Jetpack_Sync_Options methods into Jetpack_Options.

zinigor approved these changes May 10, 2017

View reviewed changes

zinigor added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. labels May 15, 2017

dereksmart merged commit 41205d1 into master May 17, 2017

dereksmart deleted the update/connect-url-authorize-secret branch May 17, 2017 15:30

dereksmart removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 17, 2017

jeherve added [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release and removed [Status] Needs Changelog [Status] Needs Testing We need to add this change to the testing call for this month's release labels May 18, 2017

jeherve added a commit that referenced this pull request May 23, 2017

Changelog: add #7115

6f59938

jeherve added a commit that referenced this pull request May 23, 2017

Testing list: add instructions for #7115

5434aa2

jeherve added a commit that referenced this pull request May 29, 2017

Changelog: add #7115

cdfef1a

jeherve added a commit that referenced this pull request May 29, 2017

Testing list: add instructions for #7115

b378fb5

oskosk mentioned this pull request Feb 14, 2018

Jetpack: Different API calls overwritting authorize secret #6882

Closed

Conversation

roccotripaldi commented May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing instructions:

Proposed changelog entry for your changes:

Uh oh!

zinigor left a comment

Choose a reason for hiding this comment

Uh oh!

roccotripaldi commented May 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zinigor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roccotripaldi commented May 10, 2017

Uh oh!

roccotripaldi commented May 10, 2017

Uh oh!

zinigor commented May 10, 2017

Uh oh!

zinigor left a comment

Choose a reason for hiding this comment

Uh oh!

dereksmart commented May 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

roccotripaldi commented May 3, 2017 •

edited

Loading