Eshoponcontainers: [Feedback] AddToCart method relies on the posted productDetails

[efeker]

As a practice, we should never trust the info posted by the client. I believe that method should make a call to a microservice to fetch the productdetails by product id.

[CESARDELATORRE]

This is an interesting point to discuss. The thing here is that as a best practice for microservices, you want to minimize Http dependencies between the internal microservices so you'll minimize future impact in performance and DoS when one of the microservices is performing bad and you have many long chains of Http request/response cycles impacted by that particular microservice.

For this case, I would try to keep a "star" implementation instead of setting Http dependencies between internal microservices. In this case, I would implement that query to the Catalog microservice at the API Gateway level (which we don't have for simplicity in this example) or, as a similar use case, at the MVC Web application (I think this is as we have it now) which runs at the server side just like an API Gateway.
For sure, the security issue here is most of all for the SPA and Xamarin apps. For those cases I'd recommend to have an API Gateway.
I talk about this topic in the section named "Asynchronous microservice integration enforce microservice’s autonomy" within following page at the Guide:
https://docs.microsoft.com/en-us/dotnet/standard/microservices-architecture/architect-microservice-container-applications/communication-between-microservices

What I'm saying is that you should try to minimize that type of Http dependencies between the internal microservices. Sometimes, you really need it, but it is better if you are able to avoid it or at least, minimize the number of these cases.

It would be interesting to continue this discussion here for this particular case and suggest additional solutions. We're open to refactor when proposing better approaches. :)

[mathysd]

Hi any thoughts on this yet?

I just changed the price input in html(WebMvc) and the basket simply accepts the price. I don't think any eCommerce shop will be happy with customers being able to manually change the price :)

No expert on subject, but since the catalog is responsible for the unit prices maybe it should also be able to tell the price per catalog id and give a total for a list of catalog id's? So when requesting the cart view it should call the basket api and the catalog api for it's data and compose accordingly(ViewComponents or Viewmodel). Or it could even pre-calculate based on a ItemAddedToCart event.

basketService.GetBasket(basketId)
catalogService.GetPrices(basketId)

and then merge the results.

Hopes this make sense. Idea based on the webmvc app.

[ardalis]

I agree that the basketService should get its pricing data from the catalogService, not from the posted product. In order to minimize delays in processing, it should have a local cache of the pricing data, and it should be updated by the catalogService whenever prices change. Thus, there should be no need for it to call out to catalogService to get a price as part of fulfilling a transaction.

Alternately, the cartService's addToCart method could simply be responsible for noting that N units of product X are in user U's cart, and nothing more. The UI layer already has the catalog pricing data and can display the cart appropriately given this information from the cartService, I suspect. On checkout there may be more for cartService to do, and it may need pricing details at that point, but perhaps not during the addToCart step.

[mvelosop]

Hi, as far as I can tell, with the current version you can't change the unit price from the basket or, at least, I couldn't do it!.

OTOH price changes in the Catalog.API currently raise a ProductPriceChangedIntegrationEvent that updates the prices in the basket of all users, so next time the user enters the basket she/he receives a notification, Just as @ardalis pointed out:

Otherwise, it's important to be aware this is a proof of concept (POC) app to showcase architectural patterns and that's the main focus, it's not a production-ready app starter.

Anyway, it looks like the issue you pointed out has been addressed.

[eiximenis]

Hi everyone.

On branch bff (currently in development) there are one Aggregator service that handles all this situation. Basket Service is now an internal service not available from outside (from clients like SPA). The aggregator exposes a new endpoint to add items to basket, and pass only the item id and quantity. The aggregator asks catalog the price and calls the old (now internal) basket endpoint with all needed info. This aggregator also handles new situations like converting basket to order (previously on clients). This open a new range of possibilities (like viewing discounts before submitting the order, a feature that before was not possible).

In our implementation making basket service internal (and thus no visible from services) is just a deployment configuration (i.e. in k8s basket service is never routed from the ingress controller, so it is not exposed outside). In a production environment more secure policies could/should be applied.

Thanks!!!

[eiximenis]

Hi @mvelosop

I am not sure that the use of ProductPriceChangedIntegrationEvent solves the security issue. Using cURL I suspect should be possible to create a basket item with 0 price and then submit the basket to create the order (not sure, should check it).
Anyway, as I said before this is corrected in the bff branch :)

[mvelosop]

@eiximenis you're right, the CreateOrderCommandHandler takes the price from the message, so it doesn't solve the security issue you pointed out.

I just solves the keeping-the-price-updated issue @ardalis mentioned.

Good to know you are working this out with BFF ;-).

BTW looking forward to get into this "BFF thing" it looks like an interesting solution for the client-side!

[mathysd]

It's not on the cart page but on the home page where you have the '[add to cart]' buttons. Here you can inspect the HTML with your browser change price input and click on add to cart.

<input type="hidden" name="price" data-val="true" data-val-number="The field Price must be a number." data-val-required="The Price field is required." id="Price" value="1">

I know it's a poc and focused on architectural patterns.

But as it's also about microservices I think it's important to think about
what data is shared and what's not. For me, that's one of the toughest parts about microservices.

Sharing pricing information could be a problem when for example you need to start supporting multiple currencies euro's, bitcoin, etc.
This would mean that you need to adjust any service that knows something about pricing.

I was just hoping to find some guidance or discuss those kinds of things.

I will have a look at the bff branch. Thanks!

[mvelosop]

Fair enough @mathysd, I didn't get the problem was on the home page, and overlooked the architectural issue involved.

[mvelosop]

Thinking over this issue I came about a naive solution, I might be missing something important, but how about just posting the product id to CartController and having the AddToCart action get all details from ICatalogService (using a a new method) before calling basket service's AddItemToBasket?

[raboud]

@mvelosop then the Cart API is tied to the Catalog API - I believe one of the main goals is to alleviate interdependencies of the microservices

CESARDELATORRE mention possibly using a API gateway and eiximenis mention an Aggregator service
both of which eliminate the Cart Service from knowing anything about Catalog Service.

Although the API Gateway or Aggregator service would know about both Cart Service and Catalog Service thus adding some interdependencies of the microservices. Those service could also handle the price change without eventing.

[mvelosop]

@raboud I think I didn't explained myself well enough, what I meant was using the existing dependency from WebMVC to both Basket.API and Catalog.API, so CartController in WebMVC Gets the product details before adding the product to basket.

That means linking http requests, which is one of the things to avoid, but that's about the same that would happen when using the API Gateway or Aggregator, isn't it?

[eiximenis]

@mvelosop This approach solves nothing.
First it puts logic on client side (so everyclient has to grab data from Catalog API before calling the Basket API), and secondly (and more important): nothing can avoid that using cURL anyone could add basket items at the desired price (calling Basket API directly).
Currently all clients (Xamarin, SPA and MVC) are working well. The problem is not with the clients. Is with the APIs. And it has to be solved at API level.

[mvelosop]

@eiximenis, by client side, do you mean in-the-browser?

[eiximenis]

In any client. Do not matter if it is on browser side (SPA) or in a MVC Controller.

[mvelosop]

@eiximenis, rephrasing, may be there's something I'm not getting, currently, at least in the MVC app, when adding an item to the basket:

1. The browser posts all product details to the MVC app CartController
2. CartController posts all product details to the Basket.API using the user bearer token.

What I'm thinking of, is:

1. Post only productId to the MVC CartController
2. Get product details in CartController (call Catalog.API)
3. Post all product details (from CartController i.e. the server) to Basket.API using an API Key or something, but not the user token.

This way, Basket.API would only accept adding to basket with the API Key, not the user token, so you should solve the security issue.

I understand this way we'd be linking the add-to-cart post to a request, to get the product details, but then again, aren't we doing the same with the API Gateway or Aggregator service? and this looks much simpler to implement.

Am I missing something here?

[raboud]

but we have the same issue with SPA and Xamarin.....MVC is just one of many possible clients.

API Gateway or Aggregator solve all clients and eliminates the attack surface completely.

[mvelosop]

You're right, this would only solve the MVC issue!

[CESARDELATORRE]

@raboud - We're working on a sample API Gateway implementation for eShopOnContainers, precisely trying to solve these kind of problems, reducing attack surface plus data aggregation.
It'll be available soon.

[mvelosop]

Hi @eiximenis, haven't checked at the BFF PR yet, was this tackled there? Is it ok to close this as done or do we take it to the Roadmap?

[mvelosop]

Closing this issue as this is now handled by the Api Gateway.

From the CartController only the Product Id is sent to the API GW:

And the API GW gets the product details with the Catalog API:

[dev-fatih-erol]

I' am sorry to have this topic resurrected. @mvelosop , i would appreciate if you answer.
Doesn't your solution violate the rule of independence for microservices?

[mvelosop]

Hi @dev-fatih-erol. thanks for your feedback.

That's just one of the possible solutions and, as usual, there are also trade-offs to consider when deciding. The trade-offs usually involve "violating" some "rules", and you have to balance out added complexity, work capacity, issue priorities, and a bunch of other things

In the context of this project, almost two years ago, that was considered a good-enough solution, that improved on a reported issue, but was not considered important enough to spend developer effort finding a more elaborate solution.

You can say that this is just a typical business decision 😉

Hope this helps.