Skip to content

API: Force secure endpoints#7060

Merged
lezama merged 1 commit intomasterfrom
add/force-secure-endpoint
May 4, 2017
Merged

API: Force secure endpoints#7060
lezama merged 1 commit intomasterfrom
add/force-secure-endpoint

Conversation

@enejb
Copy link
Copy Markdown
Member

@enejb enejb commented Apr 25, 2017

When calling endpoints with force=secure enforce the endpoint to relieve the request body by making a secure request to wpcom.

By doing a secondary call to WordPress.com over https to get the post body.

Changes proposed in this Pull Request:

Testing instructions:

Proposed changelog entry for your changes:

@ebinnion
Copy link
Copy Markdown
Contributor

This tested fine for me. I don't have any comments on the code.

As a note, other testers will need to set the JETPACK__WPCOM_JSON_API_HOST constant so that it points to their sandbox.

if ( !$cast_and_filter ) {
return $return;
if ( isset( $this->api->query['force'] ) && 'secure' === $this->api->query['force'] ) {
if ( isset( $return['secure_key'] ) ) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not add this conditional to the compound conditional that encloses it since nothing else happens within it? e.g.

if ( isset( $this->api->query['force'] ) &&
'secure' === $this->api->query['force'] &&
isset( $return['secure_key'] ) {

if ( isset( $this->api->query['force'] ) && 'secure' === $this->api->query['force'] ) {
if ( isset( $return['secure_key'] ) ) {
$post_body = $this->get_secure_body( $return['secure_key'] );
$this->api->post_body = $post_body;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If nothing else is being done with $body_post, why not just set $this->api->post_body = $this->get_secure_body( $return['secure_key'] ); and eliminate line 273?

return $this->cast_and_filter( $return, $this->request_format, $return_default_values );

protected function get_secure_body( $secure_key ) {
$response = Jetpack_Client::wpcom_json_api_request_as_blog( sprintf( '/sites/%d/secure-request', Jetpack_Options::get_option('id' ) ), '1.1', array( 'method' => 'POST' ), array( 'secure_key' => $secure_key ) );
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This line is exceptionally long, consider spreading it over several lines, e.g.

$response = Jetpack_Client::wpcom_json_api_request_as_blog(
sprintf( '/sites/%d/secure-request', Jetpack_Options::get_option('id' ) ),
'1.1',
array( 'method' => 'POST' ),
array( 'secure_key' => $secure_key )
);

@enejb enejb changed the title Force secure endpoints API: Force secure endpoints Apr 27, 2017
when calling endpoints with force=secure enforce the endpoint to
relieve the request body by making a secure request to wpcom
@lamosty
Copy link
Copy Markdown
Contributor

lamosty commented May 3, 2017

@ebinnion

As a note, other testers will need to set the JETPACK__WPCOM_JSON_API_HOST constant so that it points to their sandbox.

Is it enough to point public-api.wordpress.com to my sandbox by modifying the hosts file?

@ebinnion
Copy link
Copy Markdown
Contributor

ebinnion commented May 3, 2017

Is it enough to point public-api.wordpress.com to my sandbox by modifying the hosts file?

I haven't tried that. Seems reasonable though.

@enejb
Copy link
Copy Markdown
Member Author

enejb commented May 3, 2017

I would expect this not to work as expected for you. Since the secure endpoint does't currently exist in production and so when the Jetpack site ( not your browser ) talks to public-api.wordpress.com
You would get an error.

Copy link
Copy Markdown
Contributor

@rralian rralian left a comment

Choose a reason for hiding this comment

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

tested this and it's working as expected for me. neat approach. 👍

Copy link
Copy Markdown
Contributor

@roccotripaldi roccotripaldi left a comment

Choose a reason for hiding this comment

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

LGTM!



return $this->cast_and_filter( $return, $this->request_format, $return_default_values );
protected function get_secure_body( $secure_key ) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is awesome!

@jeherve jeherve 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 3, 2017
@lezama lezama merged commit 46d909f into master May 4, 2017
@lezama lezama deleted the add/force-secure-endpoint branch May 4, 2017 14:58
@matticbot matticbot removed the [Status] Ready to Merge Go ahead, you can push that green button! label May 4, 2017
@dereksmart dereksmart restored the add/force-secure-endpoint branch May 4, 2017 17:54
jeherve added a commit that referenced this pull request May 10, 2017
jeherve added a commit that referenced this pull request May 29, 2017
eliorivero pushed a commit that referenced this pull request May 30, 2017
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants