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.

Sign up for GitHub

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

Jump to bottom

Instagram Gallery: Add the local user ID as state to the connect URL #15594

Closed
wants to merge 3 commits into from
Closed

Instagram Gallery: Add the local user ID as state to the connect URL #15594

wants to merge 3 commits into from

Conversation

pablinos
Copy link
Contributor

If the current user is logged out of WPCOM then the nonce checks fail
when they try to connect to Instagram. By adding the local user ID to
the connect URL the changes in D42574-code will run the nonce checks
against the connected Jetpack user account ID.

Testing instructions:

Using this branch and having applied D42574-code. Sandbox the API, and test connecting the Instagram gallery block to Instagram while your connected Jetpack user is logged out of WPCOM.

Everything should work as expected!

@pablinos pablinos requested a review from Copons April 28, 2020 16:51
@pablinos pablinos self-assigned this Apr 28, 2020
@Copons Copons force-pushed the update/instagram-gallery-reuse-token branch from 7fbe423 to 1dc4be9 Compare April 28, 2020 18:09
@jeherve jeherve added [Block] Latest Instagram Posts [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it labels Apr 29, 2020
@jeherve jeherve added this to the 8.6 milestone Apr 29, 2020
@jeherve jeherve added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Apr 29, 2020
jeherve
jeherve requested changes Apr 29, 2020
View reviewed changes
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 is going to need a rebase now.

@pablinos pablinos force-pushed the fix/instagram-gallery-logged-out branch from acee7e4 to 0087ff5 Compare April 29, 2020 11:55
@pablinos pablinos changed the base branch from update/instagram-gallery-reuse-token to master April 29, 2020 11:55
@pablinos pablinos changed the base branch from master to update/instagram-gallery-reuse-token April 29, 2020 11:56
@pablinos pablinos force-pushed the fix/instagram-gallery-logged-out branch from 0087ff5 to 680e5d9 Compare April 29, 2020 12:01
@pablinos pablinos requested review from kraftbj and a team as code owners April 29, 2020 12:01
scruffian
scruffian previously approved these changes Apr 29, 2020
View reviewed changes
Copy link
Member

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

This looks good to me and tests well. The only question I had was whether the endpoints are authenticated. did a cursory look and couldn't see how they were but I assume they are :)

@scruffian scruffian force-pushed the fix/instagram-gallery-logged-out branch from 680e5d9 to ec1190f Compare April 29, 2020 12:09
@scruffian scruffian force-pushed the update/instagram-gallery-reuse-token branch from 1dc4be9 to 89f5ffe Compare April 29, 2020 12:10
@pablinos @scruffian
Instagram Gallery: Add the local user ID as state to the connect URL
99a0f8a 
If the current user is logged out of WPCOM then the nonce checks fail
when they try to connect to Instagram. By adding the local user ID to
the connect URL the the changes in D42574-code will run the nonce checks
against the connected Jetpack user account ID.
@scruffian scruffian force-pushed the fix/instagram-gallery-logged-out branch from ec1190f to 99a0f8a Compare April 29, 2020 12:11
@scruffian
Copy link
Member

Rebase ✅

@scruffian scruffian added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Status] Needs Team Review labels Apr 29, 2020
Copons
Copons previously approved these changes Apr 29, 2020
View reviewed changes
Copy link
Contributor

@Copons Copons left a comment

Choose a reason for hiding this comment

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

COOL!

jeherve
jeherve previously approved these changes Apr 29, 2020
View reviewed changes
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 should be good to merge, but I would recommend committing the WordPress.com diff first. 👍

@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 Crew. Label will be renamed soon. labels Apr 29, 2020
@pablinos
Copy link
Contributor Author

Just to say that I'm just waiting on a code/security review of D42574-code. It wouldn't break anything if this got merged first, but it might be best to wait. p1YhhO-M2-p2

@pablinos pablinos changed the base branch from update/instagram-gallery-reuse-token to master April 30, 2020 19:24
@pablinos pablinos dismissed stale reviews from jeherve, Copons, and scruffian April 30, 2020 19:24

The base branch was changed.

@pablinos pablinos changed the base branch from master to update/instagram-gallery-reuse-token April 30, 2020 19:25
@Copons
Copy link
Contributor

Copons commented Apr 30, 2020 •
edited

@pablinos I've noticed this branch is set to merge onto update/instagram-gallery-reuse-token, which has been merged since.
We should rebase and set it to merge onto master.

Also note that there is a missed fix from #15583, addressed in 54adbee.
It shouldn't affect this change, but also the block might not work flawlessly as we came to expect. 🙂

@kraftbj kraftbj changed the base branch from update/instagram-gallery-reuse-token to master May 1, 2020 16:06
@kraftbj kraftbj added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels May 1, 2020
@kraftbj
Copy link
Contributor

kraftbj commented May 1, 2020

I changed the base to master and marked it for a fresh review to make sure nothing odd changes.

@Copons
Copy link
Contributor

Copons commented May 1, 2020 •
edited

Just FYI, we are exploring a different approach in D42794-code that doesn't need any JP change. 🙂

@jeherve jeherve added [Status] Blocked / Hold and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels May 4, 2020
@pablinos
Copy link
Contributor Author

pablinos commented May 4, 2020

I've abandoned D42574-code in favour of D42794-code, so this is no longer needed.

@pablinos pablinos closed this May 4, 2020
@Copons Copons deleted the fix/instagram-gallery-logged-out branch May 4, 2020 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scruffian scruffian scruffian left review comments

@jeherve jeherve jeherve left review comments

@Copons Copons Copons left review comments

@kraftbj kraftbj Awaiting requested review from kraftbj

Assignees

@pablinos pablinos

Labels
[Block] Latest Instagram Posts [Status] Blocked / Hold Touches WP.com Files [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Milestone
8.6
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@pablinos @scruffian @Copons @kraftbj @jeherve @matticbot