shopinvader / odoo-shopinvader

Odoo Modules. Sorry Magento, Shopinvader is coming
GNU Affero General Public License v3.0
119 stars 103 forks source link

Authentication roadmap #1368

Open sbidoul opened 1 year ago

sbidoul commented 1 year ago

This issue is consolidating the efforts around shopinvader frontend partner authentication, in base_rest and fastapi services, and with the new cookie-based anonymous partner concept discussed in Valencia.

sbidoul commented 1 year ago

@lmignon @hparfr @sebastienbeau things are taking shape on the authentication front following our recent discussions in Valencia. cc/ @simahawk @acsonefho @xavier-bouquiaux @Cedric-Pigeon @marielejeune @anizr

This is the simplest I can imagine so far... and it's still too complex to my taste, but that's probably the price to pay to deploy those API in Odoo, ability to extend these API, have auth configuration per database, and compatibility between base_rest and fastapi services. Suggestions welcome. If you want to do a call to discuss with higher bandwidth, let me know.

sebastienbeau commented 1 year ago

Thanks for opening this issue to track all the work. I seem to be the right solution, indeed it's a lot of module, but it's the only way to have the two API working on the same version without breaking everything. It's ok for me

lmignon commented 1 year ago

shopinvader_auth_jwt ??? why does this not depend on base_rest_auth_jwt?

IMO it should. But it could be the result of the additional layer introduced by shopinvader on top of res.partner: shopinvader.partner. The authentication is also used to know from which website the request come from and to find the related shopinvader.backend. All this need a big clean up but it seems difficult without break breaking existing code if we do it.

shopinvader_auth_jwt ??? should we make this depend on shopinvader_anonymous_partner and return the anonymous partner if there is one?

:thinking: :thinking: :thinking: The shopinvader_anonymous_partner create a res.partner record... But the authentication is done by a lookup on the shopinvader.partner model. All this seems more and more unmanageable. A cleanup is required to get rid in a way or another of shopinvader.partner.

ping @sebastienbeau @simahawk @acsonefho

sbidoul commented 1 year ago

@lmignon my understanding is that shopinvader.partner is only used to find the res.partner for a given site. But once we have found the res.partner, it is the one we use. If correct it means that we don't need a shopinvader.partner for anonymous partners.

A given anonymous partner could have access to different sites, that should not be a problem.

So maybe the issue is not so big after all.

sbidoul commented 1 year ago

Everything is backported to 14. Juste one test CI issue to resolve in #1378. If there are enough reviews (on the PRs linked here and their dependents) I can merge until tomorrow evening.

sbidoul commented 1 year ago

Ok, everthing works. I think the last bit missing is to integrate the cookie mode with base_rest_auth_jwt.

sbidoul commented 1 year ago

Ok, everthing works. I think the last bit missing is to integrate the cookie mode with base_rest_auth_jwt.

Actually since the cookie mode is implemented in auth_jwt, base_rest_auth_jwt and shopinvader_auth_jwt should inherit it automatically. To be tested.

So unless some base_rest services require the new anonymous partner mechanism, this is complete.

To unlock the merge chain, a second review in https://github.com/OCA/server-auth/pull/531 is needed.

github-actions[bot] commented 1 week ago

There hasn't been any activity on this issue in the past 6 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 issue to never become stale, please ask a PSC member to apply the "no stale" label.