[TODO] issue with public service [help needed]

[sebastienbeau]

@kevinkhao @lmignon @simahawk

• I need to add a test to show the bug
• need to think twice about the right solution. Adding sudo is maybe not the best idea, but adding extra right on public user is not a better one (as this will give to much read access on some record through native xmlrpc)

TODO solve

• when you use the auth_api_key the setting are only return if you pass the website_key (indeed as their is not auth the auth api key is not used to get the website)
• With the jwt auth we need to think twice about what is public and what is not public. For example you do not need to be authentificated to add a product to the cart. In case of auth_api_key maybe we want to keep the current behaviour and protect all service (so nothing will be public)

[kevinkhao]

LGTM

[sebastienbeau]

do not merge I need to add a test for reproducing the bug. Also I have changed the code after the approve.

[shopinvader-git-bot]

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

[shopinvader-git-bot]

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

[simahawk]

I think that for these cases using sudo is fine. Having tests would be great.

[lmignon]

@sebastienbeau IMO we should never use sudo in this case... Services are run with a technical user. Therefore, the technical user should have the right to read backend information.

[hparfr]

With the jwt auth we need to think twice about what is public and what is not public. For example you do not need to be authentificated to add a product to the cart. In case of auth_api_key maybe we want to keep the current behaviour and protect all service (so nothing will be public)

Let's clarify a little bit.

/cart (add a product, change qty...) needs the user to be authenticated. It can be with an existing account or anonymously. It's not public.

/settings (currencies, supported languages...) can be public, without any authentication.

need to think twice about the right solution. Adding sudo is maybe not the best idea, but adding extra right on public user is not a better one (as this will give to much read access on some record through native xmlrpc)

Having one shopinvader technical user is necessary to use like a public_user.
env.with_user(shopinvader_technical_user).get_backend()

We can also have 2 users (one privilegied with write access), but I do not know if it's worth the effort.

when you use the auth_api_key the setting are only return if you pass the website_key (indeed as their is not auth the auth api key is not used to get the website)

I see website_unique_key like a public identifier, not related to authentication.

[sbidoul]

@hparfr

/cart (add a product, change qty...) needs the user to be authenticated. It can be with an existing account or anonymously. It's not public

We discussed that at the sprint in Liège we concluded that /cart is public but must work with a non guessable unique cart id instead.

Indeed when working with a SPA front end, an anonymous user does not have a token at all and there is no concept of user session like with locomotive.

Hence the public_or_jwt auth method.

cc/ @lmignon

[simahawk]

I just had to fix this problem temporarely here https://github.com/shopinvader/odoo-shopinvader/pull/1108/commits

When you define a route as "public" ATM you must use sudo because the real user will be "Public User" and not the tech one.
Hence, each call to retrieve the backend or an api key will fail.

You don't really want to grant special permissions to the public user IMO and it's also tricky because there's a validation on which groups are assigned to this kind of users (eg: no internal group that might come via implied groups).

[lmignon]

@simahawk Be carefull if you use the "public" auth, you'll nerver get user info if the same service is called by an authenticated user if no odoo session is provided. That's why the public_or_jwt auth method has been developed. Moreover, with the recent changes into the rest_framework, you should reconsider the way you manage the security into your services (OCA/rest-framework#204). You service context provider is now responsible to provide the 'authenticated_partner_id'. You can see here how this information is provided when auth_jwt is used with base_rest. This information is made available into the record rule evaluation context. Therefore it's no more a good practice to use sudo into the method called by the rest_framework.

[hparfr]

Thanks for this feedback.

I'm working on these issues these days. I will do a recap of our recent findings here.

My goal is to let the client (the browser) target the shopinvader api directly — SPA scenario.

There is multiple ways to keep a unique cart in a time span :
A) - non guessable cart_id sent on each request
B) - http session
C) - jwt token

We found some implementation of B and C in the wild for major e-commerce sites.

The C) is IMO the more elegant solution. The client always use a jwt token.

The good:

• one way of doing. Easier to reason. Easier to explain to frontend devs. Less error prone when converting an anonymous user to registered user.
• always bind a JWT to a partner : it will simplify code. Settings like language are stored on the partner.
• allows new use cases, like let an anonymous partner to change his address to estimate transport fees

The bad:

• have to create cleanup strategy of partners (like for abandoned carts)
• keycloak does not support it (yet).

A) See above for "bind a cart" instead of "bind a user".
If the user open two tabs, he should have only one cart id. So we have some to do some work on frontend to ensure the cart unicity.

About permissions:

You don't really want to grant special permissions to the public user

Agree.

[github-actions]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

[sbidoul]

To close the loop here, the current approach is summarized in #1510 (comment).