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

return Challenge() in [HttpPost] methods #400

Closed
TheFlo opened this issue May 8, 2023 · 4 comments
Closed

return Challenge() in [HttpPost] methods #400

TheFlo opened this issue May 8, 2023 · 4 comments
Labels
bug Something isn't working

Comments

@TheFlo
Copy link

TheFlo commented May 8, 2023

Hi GrandNode team,

Following addition of [HttpGet] and [HttpPost] attributes on controller methods in commit 791cf2b, there seems to be a breaking change on method StartCheckout in ShoppingCartController.cs with [HttpPost] attribute :

  • If the customer is not connected and anonymous checkout is not allowed, method will return Challenge(), resulting in a redirection to login?ReturnUrl=/cart/checkout url.

  • Once logged, the user will be redirected (using HttpGet) to /cart/checkout, which routes back to the ShoppingCartController.StartCheckout method but, as this method is flagged with [HttpPost], the customer is redirected to the 404 page-not-found page.

Adding a [HttpGet] method with the same ActionName seems to solve the issue:

[HttpGet]
[DenySystemAccount]
[ActionName("StartCheckout")]
public virtual IActionResult RedirectStartCheckout()
{
    return RedirectToAction("Cart");
}

Do you see a better solution in order to fix this issue ?

There are other methods with a [HttpPost] attribute using return Challenge() which might needs futher investigation.

Kind regards

@KrzysztofPajak KrzysztofPajak added the bug Something isn't working label May 9, 2023
@KrzysztofPajak
Copy link
Member

@TheFlo Thanks, it looks is a bug, we will fix it asap.

@KrzysztofPajak
Copy link
Member

Please check it #401
I think, this logic should be fixed in View/JS

@TheFlo
Copy link
Author

TheFlo commented May 10, 2023

Many thanks @KrzysztofPajak for your fast answer, the fix works like a charm 👍

In order to cover the case when no term of service is activated in the cart, I've extended and refactored the fix using the following code:

In src/Web/Grand.Web/Views/ShoppingCart/Partials/ModelScript.cshtml line 61, I've extracted the checkout logic in a dedicated function:

termsCheck(guest) {
    if (vmorder.cart.MinOrderSubtotalWarning == null) {
        if (this.terms) {
            this.vmorder.checkout(guest);
            vmorder.acceptTerms = false;
        }
        else {
            vmorder.acceptTerms = true;
        }
    }
},
checkout(guest) {
    if (guest) {
        window.location = '@Url.RouteUrl("Checkout")';
    } else {
        if (!vmorder.cart.ShowCheckoutAsGuestButton && vmorder.cart.IsGuest) {
            window.location = '@Url.RouteUrl("Login")?returnurl=@Url.RouteUrl("ShoppingCart")';
        }
        else {
            document.getElementById('shopping-cart-form').setAttribute('action', '@Url.RouteUrl("StartCheckout")');
            document.getElementById('shopping-cart-form').submit();
        }
    }
},

In src/Web/Grand.Web/Views/ShoppingCart/Partials/CartSummary.cshtml line 211, the created checkout function is called in the v-else template:

<template v-else>
    <div class="checkout-buttons flex-sm-nowrap flex-wrap text-center mt-3">
        <b-button type="button" id="checkoutasguest" v-if="vmorder.cart.ShowCheckoutAsGuestButton" @@click="vmorder.checkout(true)" variant="secondary" class="checkout-as-guest-button mx-sm-1 mx-0">
            @Loc["Account.Login.CheckoutAsGuest"]
        </b-button>
        <b-button id="checkout" name="checkout" @@click="vmorder.checkout(false)" variant="info" class="checkout-button mt-sm-0 mt-1">
            <span v-if="vmorder.cart.IsGuest">
                @Loc["Checkout.Button.Login"]
            </span>
            <span v-else>
                @Loc["Checkout.Button"]
            </span>
        </b-button>
    </div>
</template>

(I'm sorry that I'm not able to propose a PR for this at this moment...)

Would it be a good addition to your fix ?

@KrzysztofPajak
Copy link
Member

@TheFlo Thanks, you have absolutely right, I've committed changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants