Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Authentication roadmap #1368

Open
sbidoul opened this issue Jun 16, 2023 · 7 comments
Open

Authentication roadmap #1368

sbidoul opened this issue Jun 16, 2023 · 7 comments
Assignees

Comments

@sbidoul
Copy link
Member

sbidoul commented Jun 16, 2023

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
Copy link
Member Author

sbidoul commented Jun 16, 2023

@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
Copy link
Contributor

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
Copy link
Collaborator

lmignon commented Jun 20, 2023

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?

🤔 🤔 🤔 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
Copy link
Member Author

sbidoul commented Jun 20, 2023

@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 sbidoul self-assigned this Jun 20, 2023
@sbidoul
Copy link
Member Author

sbidoul commented Jun 29, 2023

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
Copy link
Member Author

sbidoul commented Jun 30, 2023

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

@sbidoul
Copy link
Member Author

sbidoul commented Jul 1, 2023

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 OCA/server-auth#531 is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants