Adyen / adyen-magento2

Adyen Payment plugin for Magento2
MIT License
154 stars 210 forks source link

Customer's get payment methods endpoint shouldn't require Adyen_Payment::paymentMethods #1581

Open Fifciu opened 2 years ago

Fifciu commented 2 years ago

Describe the bug It is impossible to fetch available payment methods as a customer via Rest API if the customer signed in via GQL.

To Reproduce Steps to reproduce the behavior:

  1. Sign in via GQL
  2. Use received token in the request to the /V1/carts/mine/retrieve-adyen-payment-methods
  3. It tells the customer is not authorized to access this source.

Expected behavior Resource should be anonymous or maybe(?) self. Here. The customer is able to use native endpoints like GET /V1/customers/me or GET /V1/carts/mine with GQL token so they should be able to use POST /V1/carts/mine/retrieve-adyen-payment-methods too.

Magento version 2.4.3

Plugin version 8.2.5

Additional context I am conscious there is GQL approach for Adyen M2 integration but I don't want to be one of their early adapters.

My temporary solution I am sending cartId (as hash) and customerToken from the frontend to my middleware. There I compare if the customer is owner of the cart if so - I am fetching unmasked cart ID and sending it to the endpoint but I am using newly generated integration token instead of customer token. Is it fine in your opinion?

Morerice commented 2 years ago

Hi @Fifciu,

Just to be sure that we are on the same page are you talking about the following scenario?

  1. Log in via GQL
  2. Attempt to get the payment methods using a normal (non-graphql) request

If so, is there any particular reason why you wouldn't use the GraphQl request to get the payment methods?

Thanks, Jean Adyen

AIMAR-S commented 2 years ago

Hi here,

I'm working with a framework who use the same web-service call and I got the same blocking issue.

At this time the framework don't use directly the GQL call.

Stéphane.

RokPopov commented 2 years ago

Hi @AIMAR-S,

Thank you for confirming the issue, your feedback is highly appreciated.

@Fifciu, could you please let us know whether you are indeed logging in via GraphQl and not using a GraphQl request to retrieve the payment methods? If this is indeed the case, could you please elaborate on the reason you are not using the GraphQl request for the call?

Thank you in advance,

Kind regards, Rok, Adyen

Fifciu commented 2 years ago

Hi @RokPopov . Yes, I confirm that. So I am connecting to the integration created by a totally different team. My integration based on Rest API since the beginning and rewriting it to the GQL is too big for now.

Morerice commented 2 years ago

Hi @Fifciu,

Just to check, are you also including the Access Token mentioned in point 2(c) here when attempting the retrieve-adyen-payment-methods call?

Thanks, Jean Adyen

Fifciu commented 2 years ago

Hi @Fifciu,

Just to check, are you also including the Access Token mentioned in point 2(c) here when attempting the retrieve-adyen-payment-methods call?

Thanks, Jean Adyen

Only in "My temporary solution" (check section in the main post). I've expected user to be able to access own payment just with own customerToken

dpobel commented 2 years ago

it seems like we have the same problem as well. I'm a bit surprise that we can't access this API as a customer.

Just to check, are you also including the Access Token mentioned in point 2(c) here when attempting the retrieve-adyen-payment-methods call?

the documentation is not super clear there. @Morerice By Access Token, you mean this one:

adyen

That does not look to work with an integration that grant access access to all resources.

Morerice commented 2 years ago

Hi @dpobel,

Apologies for the delay. Yes that is the access token that we're referring to. However if you don't want to use the OAuth-based Authentication that Magento recommends, you will also need to run that console command to restore using the access token as a bearer token.

Regards, Jean Adyen

daniel-kratohvil commented 1 year ago

Hi,

Just to add more context. Up to version 7.x, the endpoint /V1/carts/mine/retrieve-adyen-payment-methods required the self resource (i.e. the Bearer token needed to authenticate this resource was the user's session token). That was OK and in line with all other Magento endpoint that has /mine/ as part of their URL and are used to request logged-in users info.

However, in version 8.0 the resource for this endpoint was changed to Adyen_Payment::paymentMethods, which is a breaking change, since any existing app now needs to send an Admin Magento token as Bearer token, instead of the user's token.

Not sure what was the rationale for that change, but in my opinion it was wrong since it has modified the API contract and broken pre-existing apps that used this endpoint (which was our case, so we have created a patch to patch it back to self)

cjnewbs commented 1 year ago

Today I had to work on a fix (well a workaround) for a headless checkout we are working on due to this bizarre architectural decision.

Looking at this definition:

    <route url="/V1/carts/mine/retrieve-adyen-payment-methods" method="POST">
        <service class="Adyen\Payment\Api\AdyenPaymentMethodManagementInterface" method="getPaymentMethods"/>
        <resources>
            <resource ref="Adyen_Payment::paymentMethods"/>
        </resources>
        <data>
            <parameter name="cartId" force="true">%cart_id%</parameter>
        </data>
    </route>

Why is the role for this Adyen_Payment::paymentMethods and not self? The role it is currently configured with is an admin/integration specific role. I'm curious if this endpoint actually works because the %cart_id% param is set to force="true" meaning there will never be a cartId passed to this method because admin/integration roles don't have a cartId available to them. On top of that shouldn't this be a GET request as we are getting the payment methods and not saving anything.

Then we looked at using this one. It's got the anonymous role so will work with %cart_id%, it's still a POST when it really should be a GET, however looking at the method signature it requires a form key? Why!?!

    <route url="/V1/internal/carts/mine/retrieve-adyen-payment-methods" method="POST">
        <service class="Adyen\Payment\Api\Internal\InternalAdyenPaymentMethodManagementInterface" method="handleInternalRequest"/>
        <resources>
            <resource ref="anonymous"/>
        </resources>
        <data>
            <parameter name="cartId" force="true">%cart_id%</parameter>
        </data>
    </route>

I feel these endpoints have been massively overcomplicated by requiring a user to follow this guide instead of simply using Magento's core functionality that will handle the current cart in the session for you.

Morerice commented 1 year ago

Hi guys,

Based on your feedback we will be reviewing the access rights of these endpoints. However these changes may only be added on our next major release which for now is scheduled for Q1 2023. They cannot be done on the current v8, since they would be breaking for any merchants that have followed our suggestion and implemented an oath integration.

Thanks, Jean Adyen

Morerice commented 1 year ago

Hey guys,

We're currently considering to implement the following resources and permissions. Do you have any feedback about this?

  1. /V1/guest-carts/:maskedCartId/retrieve-adyen-payment-methods - anonymous
  2. /V1/carts/mine/retrieve-adyen-payment-methods - self
  3. /V1/adyen/guest-carts/:maskedCartId/payment-details - anonymous
  4. /V1/adyen/carts/mine/payment-details - self
  5. /V1/adyen/guest-carts/:maskedCartId/donations - anonymous
  6. /V1/adyen/carts/mine/donations - self
  7. /V1/adyen/guest-carts/:maskedCartId/payment-status - anonymous
  8. /V1/adyen/carts/mine/payment-status - self
  9. /V1/adyen/guest-carts/:maskedCartId/state-data - anonymous
  10. /V1/adyen/carts/mine/state-data - self
  11. /V1/adyen/initiate-terminal - anonymous

Regards, Jean Adyen

Morerice commented 1 year ago

Updated proposal:

Requests that will be done while cart is active:

Resource Permission Additional info
/V1/guest-carts/:maskedCartId/retrieve-adyen-payment-methods anonymous
/V1/carts/mine/retrieve-adyen-payment-methods self
/V1/adyen/guest-carts/:maskedCartId/payment-details anonymous
/V1/adyen/carts/mine/payment-details self
/V1/adyen/guest-carts/:maskedCartId/state-data anonymous
/V1/adyen/carts/mine/state-data self

Requests that will be done while cart is inactive:

Resource Permission Additional info
/V1/adyen/orders/carts/:maskedCartId/donations anonymous Require the order to be in specific states
/V1/adyen/orders/carts/:maskedCartId/payment-status anonymous Require the order to be in specific states

Terminal requests:

Resource Permission Additional info
/V1/adyen/initiate-terminal anonymous
Fifciu commented 1 year ago

It makes sense to me. Also, it's worth emphasizing breaking changes in docs because I see:

Morerice commented 1 year ago

Thanks for your feedback @Fifciu. Yes, this will be emphasized on the respective PR, on the release notes and also on our adyen docs once it's released.

Regards, Jean Adyen

cjnewbs commented 1 year ago

@Morerice Thanks for reaching out for community feedback. I haven't had time to look into this yet (Will review when I get a moment 🙂 ) but I have shared it with the dev team at work as we use Adyen on a number of clients with varying levels of custom checkout integration.