Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API: Switch site frame nonce to user+site value #12095

Merged
merged 6 commits into from
Apr 25, 2019

Conversation

mmtr
Copy link
Member

@mmtr mmtr commented Apr 19, 2019

Changes proposed in this Pull Request:

On D27129-code we're changing the way the frame nonces are generated to use both the user ID and site ID.

Since that will be an breaking change for the API (p7jreA-2hw-p2), we need to version the /sites/:site endpoint (which is where the frame nonces are serialized) in order to keep using the nonce generated from only the site ID on the existing versions (1.0 and 1.2) and switch to the user+site value on the new version (1.3).

Given Fusion is not able to apply these changes automatically, this will also need a diff in WP.com: D27153-code

Testing instructions:

  • Make an authenticated call to https://public-api.wordpress.com/rest/v1.2/me/sites?site_visibility=all&include_domain_only=true&site_activity=active&fields=ID,URL,options&options=frame_nonce_site_only,frame_nonce.
  • Verify that both frame_nonce and frame_nonce_site_only are returned with the same value for all WP.com and JP sites.
  • Make an authenticated call to https://public-api.wordpress.com/rest/v1.3/me/sites?site_visibility=all&include_domain_only=true&site_activity=active&fields=ID,URL,options&options=frame_nonce_site_only,frame_nonce.
  • For JP sites, check that frame_nonce has a different value now but frame_nonce_site_only still have the same value as before. For WP.com sites, both frame_nonce and frame_nonce_site_only still have the same value as before.

Repeat steps but replacing the API calls from /me/sites to /sites/<SITE_SLUG_OR_ID>.

Proposed changelog entry for your changes:

WPCOM API: Switch site frame nonces to new user and site specific values.

@jetpackbot
Copy link
Collaborator

jetpackbot commented Apr 19, 2019

Warnings
⚠️ Consider adding new PHP files to PHPCS whitelist for automatic linting: json-endpoints/class.wpcom-json-api-get-site-v1-3-endpoint.php

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 927a28f

@mdawaffe
Copy link
Member

nonces are generated to use both the user ID and site ID.

Why is this a breaking change in the API? Has Jetpack ever used the previous form of frame nonces?

@mmtr
Copy link
Member Author

mmtr commented Apr 19, 2019

nonces are generated to use both the user ID and site ID.

Why is this a breaking change in the API? Has Jetpack ever used the previous form of frame nonces?

It will affect the WordPress.com desktop and mobile apps as they use the old site-specific nonces for previewing posts.

Actually, we already deployed this change without versioning the API and had to revert it when we realized that the post previews were not working on those apps (p7jreA-2hw-p2).

@mmtr mmtr added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress labels Apr 20, 2019
@mmtr mmtr requested a review from a team April 20, 2019 00:01
kwight
kwight previously approved these changes Apr 23, 2019
kwight
kwight previously approved these changes Apr 25, 2019
@kwight
Copy link
Contributor

kwight commented Apr 25, 2019

@jeherve Would you or someone else mind a final look at this? This PR will bring JP back into sync with recent work on the WP.com side.

  • D26952-code
  • D26953-code
  • D26956-code
  • D27153-code

@kwight kwight force-pushed the add/api-site-endpoint-v1-3 branch from 55dbfdd to 927a28f Compare April 25, 2019 18:05
@obenland
Copy link
Member

@Automattic/jetpack-crew When one of you gets a chance, would you mind ✅ this PR to help unblock us from posting a Call for Testing?

Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

This seems to work well in my tests. Merge when ready 👍

@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Apr 25, 2019
@obenland obenland merged commit 794074f into master Apr 25, 2019
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Apr 25, 2019
@obenland obenland deleted the add/api-site-endpoint-v1-3 branch April 25, 2019 20:24
kraftbj added a commit that referenced this pull request Apr 26, 2019
kraftbj added a commit that referenced this pull request Apr 30, 2019
@kraftbj kraftbj added this to the 7.3 milestone May 2, 2019
kwight added a commit that referenced this pull request May 3, 2019
kraftbj pushed a commit that referenced this pull request May 3, 2019
kraftbj pushed a commit that referenced this pull request May 3, 2019
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