-
-
Notifications
You must be signed in to change notification settings - Fork 414
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
[12.0][MIG] auth_api_key from 10 to 12 #56
Conversation
…custom one from shopinvader
Get rid of keychain dependency
@sbidoul The name of the migrated module is |
6da46a7
to
8edf0bc
Compare
🍏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seem good to me, just a small request
continue | ||
|
||
login_name = serv_config.get(section, "user") | ||
uid = self.env["res.users"].search( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbidoul Is it possible to modify to search also the active=False users?
Note: The suggestion below is 83 char (too long), but suggestion on multi lines isn't possible
uid = self.env["res.users"].search( | |
uid = self.env["res.users"].with_context(active_test=False).search( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. I kind of remember people (was it @guewen?) saying running code under an inactive user was a bad idea, but I'm don't remember why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbidoul I do not know on further versions but at least on V10.0 I have absolutely no troubles while running code with an inactive technical user....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lmignon any opinion on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sbidoul IMO we must keep the possibility to select an inactive user (it's under the responsibility of the integrator/administrator to select the most appropriate one active or inactive). We must keep in mind that (at least in odoo < 12) active users are paid users. That's why we use inactive users in such use cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. I kind of remember people (was it @guewen?) saying running code under an inactive user was a bad idea, but I'm don't remember why.
The problem with inactive users is that they will be removed from the group that they are in as soon as you edit the user list on an ir.group. The m2m widget for users on the group will not include them - as they are inactive, so if you changes the list, it will remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not know on further versions but at least on V10.0 I have absolutely no troubles while running code with an inactive technical user....
Neither on other versions, until you edit the other users on a group and wonder why suddenly it doesn't work anymore ;)
@gurneyalex this addon depends on server_environment which is not yet migrated to 12.0 (or 11.0) in https://github.com/OCA/server-tools i could add the code suggestion from our private repo where we removed the dependency if that is ok |
@ap-wtioit server_environment is merged in https://github.com/OCA/server-env |
Co-Authored-By: qgroulard <43472442+qgroulard@users.noreply.github.com>
@gurneyalex I've cherry-picked the consteq commit of @ap-wtioit, so this is now ready to merge. |
Is everyone agreed to merge? |
Blunt question: How many sessions are created by this implementation for 1.000.000 requests during a time span of 7 days? - I wonder if it's 1.000.000 or if I'm missing something... |
@ychirino In my experience, there is no sessions issue, as long as the client handles cookies properly. For python clients, using It might be worth mentioning in the readme of |
@sbidoul Yeah, I wondered. Thanks. My main concern is a bearer token approach for an external API client. I can't guarantee in no way that it would stick to this pretty please recommendation. For more details see this comment. |
Maybe a k/v map in memory assigning each token to a session? It's not a canonical solution but would reduce that problem by some orders of magnitude. |
@ychirino multi-db is not really supported by this module at the moment. Regarding the session, I'm not sure what problem you want to solve. It's easy to ask clients to return cookies. |
@ychirino api keys and bearer tokens are for different use cases. api keys are meant for cooperating clients indeed. If you need bearer tokens that's for another module. BTW, this is a migration PR, so please do not pollute it with unrelated comments. |
.. |badge1| image:: https://img.shields.io/badge/maturity-Beta-yellow.png | ||
:target: https://odoo-community.org/page/development-status | ||
:alt: Beta | ||
.. |badge2| image:: https://img.shields.io/badge/licence-AGPL--3-blue.png |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose that the licence badge will be fixed once the PR will be merged and the readme re generated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
@lmignon Thanks for the clarification indeed, is a more extensive topic. |
Migration of #51
TODO