Implement SHA256 support, to work with newer Adyen merchant accounts.

[willharris]

I will be happy to rebase if you merge PR #23, as I need both of them for my own site. ;-)

[Exirel]

Hi @willharris, it's me again. I read carefully your PR, and I'm a bit confused by the use of request inside the Gateway class. I can't tell exactly what was the initial intent of the authors, but it looks like to me that the Gateway should never know about the request, or any django-related objects.

However, what is clear is that we should provide a way to handle SHA256. As I said on your other PR #25 I would like first to implement a way in django-oscar-adyen to allow developers to extend the plugin, instead of adding ad-hoc code.

If you allow me, I'll work a bit more on the plugin internal structure, and see after that how we can implement new features and update the critical parts that is the security of the transactions.

[Exirel]

I like the idea of the config for that. Good point to keep.

[willharris]

Hi again @Exirel. Regarding the gateway being dependent on the request: I think the original intention here was to allow a class-based configuration to switch out config settings based on the request. In the readme, there's the following:

In more complex deployments, you will want to e.g. alter the Adyen identifier based on the request.
That is not easily implemented with Django settings, so you can alternatively set
ADYEN_CONFIG_CLASS to a config class of your own. See
Adyen.settings_config.FromSettingsConfig for an example.

But I absolutely understand wanting to keep the request out of the Gateway class completely. If you'd prefer, I can refactor this a bit so that's not needed. My original intention here was to remove a lot of the gateway-internal specific details (e.g. figuring out with response class to use) from the Facade, who doesn't need to care, and encapsulate that within the Gateway itself.

Shall I refactor to remove the request code from Gateway?

[Exirel]

You pointed out an issue I have with the code too: why does the Facade has to care about the request.method?

I mean... from the Scaffold point of view, this is something that could be handled into the scaffold. For example:

Scaffold:
    # ...

    def handle_payment_feedback(self, request):
        facade = Facade()
        if request.method == 'POST':
            result = facade.handle_payment_notification(request)
        else:
            result = facade.handle_payment_return(request)
        return self._normalize_feedback(result)

Then in Facade, each method would call a new dedicated method: get_notification_response_class and get_return_response_class.

This design should help any developer (from plugin maintainer to plugin user) to handle specific cases in better way.

What do you think of this proposal?

[willharris]

After looking at the Facade a bit more, it seems there are a lot of places where it uses details of the request, from its method, to the GET/POST parameters, to determining URLs and so forth. Unless you are going to remove any dependency of Facade on the request, I don't see too much to be gained by splitting out the switch between notification and return into the scaffold - it just means the Scaffold is more tightly coupled to the Facade.

I don't think the Facade needs to know anything about the response class - this is an internal detail for the Gateway, and I see you have already refactored this a bit in 8034744. But I also think you could leave the determination of which processing class to use as an internal detail for the Gateway. I agree that the request shouldn't be used in the Gateway, but by using the PaymentNotification and PaymentRedirection classes directly in the Facade, you are also increasing the coupling between these two modules. I would suggest it would be cleaner simply to "configure" the gateway in either notification or redirection mode, and let the gateway itself figure out which classes it needs to use.

[Exirel]

it just means the Scaffold is more tightly coupled to the Facade.

Yep. This is something I struggle a bit with. To me, it makes sense to have, say, a Scaffold and a Gateway, or a Facade and a Gateway. But all 3 at the same time? I don't see why we need both Scaffold, Facade, and Gateway.

I wasn't in the original discussions that lead to this design decision, so I assume there were good reasons for that. Today, all I can say is that:

• There are too many layers, and/or,
• The layers have too many dependencies to each others, and/or,
• The responsibilities and roles of each layer is not clear enough and they are not properly separated.

My gut feeling would say that it's a bit of all these issues. 3 layers might be a bit over-engineered, but they also have a lot of dependencies, and it's not clear why both the Scaffold and the Facade rely on the Django request, and why both the Facade and the Gateway rely on response objects.

There is something that needs to be addressed before we can really move things here and there.

[ckepper]

Do you have plans for resolving these issues? I am implementing a new shop with Adyen payment (and SHA256) and consider using this module.

IMHO, the architecture could benefit from some cleanup. Personally, I find the structure of django-oscar-paypal (express checkout) much cleaner and would use this as guidance for refactoring / reorganizing this module. But maybe I am not seeing the whole picture...

[Exirel]

IMHO, the architecture could benefit from some cleanup. Personally, I find the structure of django-oscar-paypal (express checkout) much cleaner and would use this as guidance for refactoring / reorganizing this module. But maybe I am not seeing the whole picture...

You're right, this module is too complex and not easy to extend. I'm more or less the maintainer "by default" on this project - I wasn't here at its creation and now I'm kind of alone at Oscaro to work on this.

I can not make any promise for the future of this module. Oscaro is changing things internally, so I can not tell if this module will be maintained in the future.

[ckepper]

Thank you for the heads-up. Over the last few days I have implemented the module for my own needs based on the pull request of @willharris (because I needed SHA256) but I haven't made any structural changes. I still feel that I don't understand Adyen well enough yet to take it to the next level, but I certainly would prefer if this module would be maintained further.

[Exirel]

@willharris and @ckepper I got time to work on the topic. You may want to read #28 where I add the concept of "signer".

I wanted to remove the "sign & verify" part from the payment form & payment return data objects, and to delegate that to another object, which is easier to configure and extend.

I didn't wrote a SHA-256 implementation, but I think it's easy to do now with this new structure.

If you don't mind, I'll close this PR for now.