Create a `PayForOrder` service class to implement authentication checks #9401

hsingyuc · 2023-05-09T05:41:00Z

Create a PayForOrder service class to implement authentication checks for the pay-for-order checkout block.

Screenshots

	Before	After
Customer
Guest

Testing

Comment out is_wc_endpoint_url( 'order-pay' ) ( Done in this PR )
Create a pay-for-order order as a guest
Go to WC -> Orders -> find the order you just created
Click on the Customer payment page and copy the URL to incognito
See Your cart is currently empty! ( Because we don't allow guest to pay for order )
Go back to the order you just created
Change Customer to an existing customer
Go back to the Your cart is currently empty! page and refresh
See the login form

Automated Tests

Changes in this PR are covered by Automated Tests.
- Unit tests
- E2E tests

Changelog

Create a PayForOrder service class to implement authentication checks

…or order page

mikejolley · 2023-05-09T10:14:37Z

@hsingyuc What do I need to do on my store (in terms of blocks, pages etc) in order to test this?

Is there a reason why we're using content filters vs adding this login form as part of an existing block or (core) shortcode? Thanks for any background info on this.

hsingyuc · 2023-05-10T18:02:15Z

Is there a reason why we're using content filters vs adding this login form as part of an existing block or (core) shortcode? Thanks for any background info on this.

Is this what you are thinking of?

hsingyuc · 2023-05-10T22:07:35Z

What do I need to do on my store (in terms of blocks, pages etc) in order to test this?

I think that we need to continue supporting guest checkouts, which means that we will continue to show the checkout block for guests.

Noting that this PR is just one piece of the pay-for-order flow and won't show the checkout until the other PRs are merged. Specifically this one.

I updated the testing instruction. Let me know if it works for you. Thank you!

mikejolley · 2023-05-11T09:11:12Z

I think that we need to continue supporting guest checkouts, which means that we will continue to show the checkout block for guests.

If the order being paid for does not belong to a customer, do we need to require login? I think we could account for that in the authentication check.

Comment out is_wc_endpoint_url( 'order-pay' )
Is this what you are thinking of?

Okay I think I understand what you're doing now. Using checkout Block instead of the legacy shortcode when handling payments. In this case, do you think we need to move the login to the block rather than replacing blocks via the_content hook? Or is this a temporary solution to help with testing?

Noting that this PR is just one piece of the pay-for-order flow and won't show the checkout until the other PRs are merged. Specifically #8960.

I think we should either merge to a feature branch to keep this work separate from trunk, or put it behind a feature flag.

mikejolley · 2023-05-11T09:13:25Z

src/Domain/Services/PayForOrder.php

+			$order = wc_get_order( $order_id );
+
+			// Logged out customer does not have permission to pay for this order.
+			if ( ! current_user_can( 'pay_for_order', $order_id ) && ! is_user_logged_in() ) {


It looks like the current_user_can logic already handles user ID validation, and allows guests to pay for guest orders.

This check could also move above the wc_get_order to prevent loading an order if its not needed.

mikejolley · 2023-05-11T09:16:16Z

src/Domain/Services/PayForOrder.php

+
+		$order_id = ! empty( $wp->query_vars['order-pay'] ) ? $wp->query_vars['order-pay'] : null;
+
+		if ( isset( $_GET['pay_for_order'], $_GET['key'] ) && $order_id ) { // phpcs:ignore WordPress.Security.NonceVerification


This is looking to see if pay_for_order is set, but its not validating it against the actual order key.

Since we already validate the order key in this PR so we don't need to validate it again, right?

I would prefer we have the same validation logic everywhere (including that check) just in case a future developer assumes this is safe and either reuses it or inserts something that identifies an order. Could turn it into a reusable utility to avoid repetition?

hsingyuc · 2023-05-11T23:55:50Z

If the order being paid for does not belong to a customer, do we need to require login? I think we could account for that in the authentication check.

Cool. I don't think we should require login but maybe I miss understood what you mean by With that in mind, I could see an /orders endpoint being useful (to list the logged in current user's orders only), with an /order/ID route for specific order details.

do you think we need to move the login to the block rather than replacing blocks via the_content hook? Or is this a temporary solution to help with testing?

A login block is a good solution long term. For the scope of this project, I prefer to temporarily use the existing login form. We will render the normal checkout block with the order items when login is not required.

I think we should either merge to a feature branch to keep this work separate from trunk, or put it behind a feature flag.

Good idea. Updated.

mikejolley · 2023-05-12T14:21:49Z

Cool. I don't think we should require login but maybe I miss understood what you mean by #8922 (comment)

I was thinking ahead to when we'll support "my account" pages. We won't want to include order listing functionality for guests, but we can retrieve a specific order if the correct keys etc are provided.

I wouldn't be too concerned with order listing for this project.

A login block is a good solution long term. For the scope of this project, I prefer to temporarily use the existing login form. We will render the normal checkout block with the order items when login is not required.

👍🏻

github-actions · 2023-05-20T09:02:46Z

This PR has been marked as stale because it has not seen any activity within the past 7 days. Our team uses this tool to help surface pull requests that have slipped through review.

If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label.

github-actions · 2023-06-16T09:03:07Z

This PR has been marked as stale because it has not seen any activity within the past 7 days. Our team uses this tool to help surface pull requests that have slipped through review.

If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label.

github-actions · 2023-06-27T09:02:58Z

This PR has been marked as stale because it has not seen any activity within the past 7 days. Our team uses this tool to help surface pull requests that have slipped through review.

If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label.

Check authentication for logged out user on the checkout blocks pay f…

853ea0d

…or order page

hsingyuc requested a review from mikejolley May 9, 2023 05:41

mikejolley suggested changes May 11, 2023

View reviewed changes

hsingyuc changed the base branch from trunk to add/pay-for-order-flow May 11, 2023 23:34

hsingyuc mentioned this pull request May 13, 2023

Add an endpoint for getting pay for order orders #8960

Merged

4 tasks

github-actions bot added the status: stale Stale issues and PRs have had no updates for 60 days. label May 20, 2023

hsingyuc removed the status: stale Stale issues and PRs have had no updates for 60 days. label Jun 8, 2023

github-actions bot added the status: stale Stale issues and PRs have had no updates for 60 days. label Jun 16, 2023

hsingyuc removed the status: stale Stale issues and PRs have had no updates for 60 days. label Jun 20, 2023

github-actions bot added the status: stale Stale issues and PRs have had no updates for 60 days. label Jun 27, 2023

hsingyuc removed the status: stale Stale issues and PRs have had no updates for 60 days. label Jun 28, 2023

hsingyuc deleted the branch woocommerce:add/pay-for-order-flow July 19, 2023 13:49

hsingyuc closed this Jul 19, 2023

hsingyuc mentioned this pull request Jul 26, 2023

Create a PayForOrder service class to implement authentication checks #10348

Closed

3 tasks


		$order_id = ! empty( $wp->query_vars['order-pay'] ) ? $wp->query_vars['order-pay'] : null;

		if ( isset( $_GET['pay_for_order'], $_GET['key'] ) && $order_id ) { // phpcs:ignore WordPress.Security.NonceVerification

Create a PayForOrder service class to implement authentication checks #9401

Create a PayForOrder service class to implement authentication checks #9401

Uh oh!

Conversation

hsingyuc commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshots

Testing

Automated Tests

Changelog

Uh oh!

mikejolley commented May 9, 2023

Uh oh!

hsingyuc commented May 10, 2023

Uh oh!

hsingyuc commented May 10, 2023

Uh oh!

mikejolley commented May 11, 2023

Uh oh!

mikejolley May 11, 2023

Choose a reason for hiding this comment

Uh oh!

mikejolley May 11, 2023

Choose a reason for hiding this comment

Uh oh!

mikejolley May 11, 2023

Choose a reason for hiding this comment

Uh oh!

hsingyuc May 12, 2023

Choose a reason for hiding this comment

Uh oh!

mikejolley May 12, 2023

Choose a reason for hiding this comment

Uh oh!

hsingyuc commented May 11, 2023

Uh oh!

mikejolley commented May 12, 2023

Uh oh!

github-actions bot commented May 20, 2023

If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label.

Uh oh!

github-actions bot commented Jun 16, 2023

If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label.

Uh oh!

github-actions bot commented Jun 27, 2023

If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Create a `PayForOrder` service class to implement authentication checks #9401

Create a `PayForOrder` service class to implement authentication checks #9401

hsingyuc commented May 9, 2023 •

edited

Loading