Fix QIT warning in WooPay session endpoint #8760
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: 0 B Total Size: 1.25 MB ℹ️ View Unchanged
|
bborman22
approved these changes
May 2, 2024
There was a problem hiding this comment.
I was able to test this in a few ways and confirmed that it all worked as expected. Some of the ways I tested this:
- Confirmed that adding the URL to the woopay session list allowed it to process the
determine_current_user_for_woopayusing the Cart Token. - Confirmed that within
get_init_session_requestthe same$customer_idwas returned for each session through multiple attempts as well as through using Direct Checkout, Express Checkout, or OTP modal. - Confirmed that when testing from the sandbox the
has_valid_request_signaturewas properly verifying or failing the requests.
This seems like a strong move to centralize this logic in one place and removes the warning about a dangerous function and instead uses the more recommended determine_current_user filter.
Overall, everything tested well for me. I had one note about redundancy that you can share your opinion on, but it's not a blocker in my opinion.
| @@ -89,31 +78,6 @@ public function check_permission() { | |||
| return $this->is_request_from_woopay() && $this->has_valid_request_signature(); | |||
There was a problem hiding this comment.
These checks are redundant now, but considering leaving them in as they are low-cost and would prevent accidentally removing them from another place and leaving this open. It also satisfies the goal of having defined check permissions. Let me know what you think though about this redundancy?
There was a problem hiding this comment.
I agree with everything you said. This is now redundant, but better be redundant and safe than taking an unnecessary risk of leaving it open, especially with this redundancy being low-cost.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #8732.
Changes proposed in this Pull Request
This PR tweaks the WooPay session REST endpoint by replacing a
wp_set_current_userapproach with an existent approach designed specifically for WooPay.Testing instructions
Pre-requisites
npm run wp option update _wcpay_feature_woopay_direct_checkout 1.Test: current approach to note current user and customer
developbranch.2.1. Alternatively, you can comment this line. But if you're going down this path you'll need to build the assets:
npm run build:client.Test: determine current user approach works as expected
2.1. Alternatively, you can comment this line. But if you're going down this path you'll need to build the assets:
npm run build:client.Test: QIT warning has been fixed
npm run test:qit.class-wc-rest-woopay-session-controller.php#8732 is gone.npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge