Skip to content

Added a mechanism to check the existing token.#6964

Merged
zinigor merged 2 commits intomasterfrom
add/token-checking
May 2, 2017
Merged

Added a mechanism to check the existing token.#6964
zinigor merged 2 commits intomasterfrom
add/token-checking

Conversation

@zinigor
Copy link
Copy Markdown
Contributor

@zinigor zinigor commented Apr 12, 2017 •
edited
Loading

Changes proposed in this Pull Request:

  • Adds a mechanism that checks the existing token and generates a registration URL if it doesn't work.

Testing instructions:

  • Disconnect your Jetpack, make sure it appears as disconnected both in wp-admin and in Calypso.
  • Click the connect link.
  • In the approval screen just leave it without clicking "Approve".
  • Intentionally garble your token to simulate a non-functional Jetpack connection on the client side, you can do it by running this code as a filter once:
add_action( 'noop_wp_loaded', function() {
	$token = Jetpack_Options::get_option( 'blog_token' ) . 'garbage';
	Jetpack_Options::update_option( 'blog_token', $token );
} );
  • You shouldn't be able to connect from wp-admin at all. Broken tokens result in different errors, most notorious of which is the following:
Invalid request, please go back and try again.
Error Code: invalid_client
Error Message: Unknown client_id.
  • Update to this PR.
  • Now you'll be able to connect even if you have a broken token - it will get re-requested.

Proposed changelog entry for your changes:

  • Added a mechanism to detect broken connection states and resolve them.

@zinigor zinigor added General [Status] Needs Review This PR is ready for review. Bug When a feature is broken and / or not performing as intended labels Apr 12, 2017
@zinigor zinigor requested review from dereksmart and oskosk April 12, 2017 14:03
dereksmart
dereksmart previously requested changes Apr 12, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@dereksmart dereksmart left a comment

Choose a reason for hiding this comment

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

@zinigor all of the php tests are failing currently

@dereksmart
Copy link
Copy Markdown
Contributor

dereksmart commented Apr 12, 2017

I like this general approach, and it definitely solves the issues at hand. Once the CI tests are passing, I think we should go with it.

dereksmart
dereksmart reviewed Apr 12, 2017
View reviewed changes
class.jetpack.php

// Checking existing token
$response = Jetpack_Client::wpcom_json_api_request_as_blog(
sprintf( '/sites/%d', $site_id ) .'?force=wpcom',
Copy link
Copy Markdown
Contributor

@dereksmart dereksmart Apr 12, 2017

Choose a reason for hiding this comment

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

Is there any reason for this specific endpoint? Or just one chosen that we can validate with.

Copy link
Copy Markdown
Contributor Author

@zinigor zinigor Apr 12, 2017

Choose a reason for hiding this comment

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

No, there was no specific reason for choosing this endpoint. Can you think of a better candidate here? Or maybe we should even create a new endpoint for this?

Copy link
Copy Markdown
Contributor

@dereksmart dereksmart Apr 12, 2017

Choose a reason for hiding this comment

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

I can't think of a good enough reason to create a new endpoint for this.

@dereksmart dereksmart dismissed their stale review April 13, 2017 01:05

tests fixed

@kraftbj
Copy link
Copy Markdown
Contributor

kraftbj commented Apr 13, 2017 •
edited
Loading

This may fix #2218. It's been hard to find duplicative steps in that issue.

@kraftbj kraftbj mentioned this pull request Apr 13, 2017
Closed
@roccotripaldi roccotripaldi 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 May 2, 2017
@zinigor zinigor merged commit 3069668 into master May 2, 2017
@zinigor zinigor deleted the add/token-checking branch May 2, 2017 13:12
@zinigor zinigor removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 2, 2017
jeherve added a commit that referenced this pull request May 10, 2017
@jeherve
Changelog: add #6964
96f4ad4
jeherve added a commit that referenced this pull request May 29, 2017
@jeherve
Changelog: add #6964
921f096
eliorivero pushed a commit that referenced this pull request May 30, 2017
@jeherve @eliorivero
Changelog: first pass at a changelog for 5.0 (#7116)
eee27f0 
* 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.
@ebinnion ebinnion mentioned this pull request Sep 4, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dereksmart dereksmart dereksmart left review comments

@oskosk oskosk Awaiting requested review from oskosk

+1 more reviewer

@roccotripaldi roccotripaldi roccotripaldi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Bug When a feature is broken and / or not performing as intended General

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@zinigor @dereksmart @kraftbj @roccotripaldi @jeherve