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

ADD "API keys" for password-less RPC access #33637

Closed
wants to merge 8 commits into from

Conversation

@xmo-odoo
Copy link
Collaborator

xmo-odoo commented May 24, 2019

Pre-requisite for 2FA / TOTP.

Actual API keys implementation is the third commit, the other two are improvements to make things possible or better:

  • split the auth flows between interactive (interface / web client) and non-interactive (API), so API keys can be restricted to non-interactive auth flows
  • fix invalidation of UID cache in non-interactive auth flows as the creation & removal of API keys would increase pressure on that working properly
  • actually implement API keys and user interface to them (though the user preferences / profile)
@xmo-odoo xmo-odoo requested a review from odony May 24, 2019
@robodoo robodoo added the seen 🙂 label May 24, 2019
@C3POdoo C3POdoo added the RD label May 24, 2019
odoo/addons/base/models/res_users.py Show resolved Hide resolved

name = fields.Char("Description", required=True, readonly=True)
user_id = fields.Many2one('res.users', index=True, required=True, readonly=True, ondelete="cascade")

This comment has been minimized.

Copy link
@fmdl

fmdl May 24, 2019

Contributor

idea : add the last time used (like on google.com)

This comment has been minimized.

Copy link
@xmo-odoo

xmo-odoo May 24, 2019

Author Collaborator

Might be nice. Wouldn't be very precise due to the id cache though.

This comment has been minimized.

Copy link
@fmdl

fmdl May 24, 2019

Contributor

The ormcache is never clear (one time a day ?)
Other idea : add validity date

This comment has been minimized.

Copy link
@xmo-odoo

xmo-odoo May 24, 2019

Author Collaborator

The ormcache is never clear (one time a day ?)

I don't believe we've got any such explicit cron, though it might occur as a side-effect of the autovacuum cron or something.

This comment has been minimized.

Copy link
@nhomar

nhomar Jun 6, 2019

Collaborator

Can the key be hidden in some way once it is seeing? force the regeneration?, some extra field to check the app that can use it (by IP/name/header or something like that)?

odoo/addons/base/models/res_users.py Show resolved Hide resolved
@xmo-odoo xmo-odoo force-pushed the odoo-dev:master-apikeys-xmo branch 2 times, most recently May 24, 2019
@Yenthe666

This comment was marked as off-topic.

Copy link
Collaborator

Yenthe666 commented May 24, 2019

@odony is this the task that you where talking about in your email?

@xmo-odoo xmo-odoo force-pushed the odoo-dev:master-apikeys-xmo branch May 24, 2019
@robodoo robodoo added the CI 🤖 label May 24, 2019
@xmo-odoo xmo-odoo force-pushed the odoo-dev:master-apikeys-xmo branch May 28, 2019
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels May 28, 2019
odoo/addons/base/security/base_security.xml Outdated Show resolved Hide resolved
@robodoo robodoo added CI 🤖 and removed CI 🤖 labels Jun 4, 2019
@xmo-odoo xmo-odoo force-pushed the odoo-dev:master-apikeys-xmo branch Jun 5, 2019
@robodoo robodoo removed the CI 🤖 label Jun 5, 2019
xmo-odoo added 4 commits May 13, 2019
Also improve Users.check to match Users.authenticate: no need to
bother with auth process if the current user is not active.

And remove some dead code: as far as I can tell, Session.authenticate
is never called with a uid.
Before this, invalidations to the UID cache is not synchronised
between workers because it's an ad-hoc solution (so a user changing
their password or an admin disabling a user would only lock out an
attacker currently using the API of one of possibly several
workers). Shift the entire thing to ormcache which already has proper
support for synchronising cache invalidation between workers.

Also simplify the cache invalidation mess in Users.write because the
caches have been unified into a single registry-level LRU, so the
half-dozen cache clears on specific ormcached methods & models is
pretty much the same as repeatedly calling clear_caches on the current
model.

**However** registry.cache is trivially accessible from server actions
and safe_eval as long as they provide access to a model (through
`model.pool.cache`). Which is common, and an issue given we're very
much putting sensible data in there.

Fix this by renaming `Registry.cache` to `Registry.__cache`, this
requires few editions and mangled names are not accessible from
safe_eval contexts.

The alternative would have been to add more bespoke handling of the
uid cache to hook it into the cache invalidation propagation
machinery.

After discussion with @odony, fixing LRU access and using that seems
cleaner and less error-prone.

Note on lazy_property
=====================

Make Registry.cache / Registry.__cache into a regular attribute: the
overhead of the LRU is not that high (compared to that of the registry
itself), it's rare that we *don't* need it, and it's assumed to be a
persisted attribute (it's not just a cache) so making it a normal
attribute seems fine; and lazy_property doesn't work for mangled
names: the name of the property is mangled using the name of the
definition class, but the name of the symbol (fget) is not mangled so
lazy_property would set the __cache attribute but then Python would
lookup _Registry__cache, creating a new cache every access.

And we can't (always) mangle things correctly on `__get__(obj,
owner)`: `owner` is just `type(obj)`, meaning in the case of
inheritance the type we get is the type through which the property is
accessed rather than the one it's defined on. So it would work in the
cases where no inheritance is involved (such as Registry.__cache) but
not in general (lest we want to play around walking the MRO ourselves
to find the definition source, which doesn't seem worth it).

lazy_property *could* be made to work properly on Python 3.6+: the
descriptor protocol gains `__set_name__(name, owner)`, which is called
with the properly mangled name — and with the definition class to boot
(though there might still be issues when overriding lazy properties as
the override will be mangled & named differently... or maybe that's a
feature?). However we're still supporting 3.5 at this point, AFAIK, so
that's not an option. Plus it feels unnecessary / not very useful.

However add an assertion to `lazy_property` so it signals when we try
to use it on a mangled method (as otherwise it kinda sorta work in the
sense that the property / object is accessible but is in effect a
slower way to write a regular property).
* ability for a user to request / create keys associated to their user
* can require authentication solely though API keys, use a computed
  field so it can be overridden (to require API auth even in
  situations where the user has not requested it themselves)
* hash keys just in case as we can do so and might as well, add a
  cleartext index (first 4 bytes of 20) to avoid blowing up the DB if
  a user decides to create millions of keys for some daft reason
* users can delete their own keys, admins can delete (invalidate)
  anyone's keys
@xmo-odoo xmo-odoo force-pushed the odoo-dev:master-apikeys-xmo branch Jun 6, 2019
@xmo-odoo xmo-odoo mentioned this pull request Jun 6, 2019
6 of 9 tasks complete
@xmo-odoo

This comment has been minimized.

Copy link
Collaborator Author

xmo-odoo commented Jun 6, 2019

@odony updated version which changes the _check_credential protocol (rolling back the split) & hooks in partial sessions / MFA, the totp stuff built on top is at #33928

xmo-odoo added 3 commits Jun 4, 2019
…long

Means api keys can't really preempt other auth methods.
* migrate website to overriding web_login less (still needed to flag it
  as website-enabled) and use _login_redirect for its login redirection
  override needs
* modify portal and website to not replace / shortcut the redirection
  workflow, so it's possible to override those properly if / as
  necessary
@xmo-odoo xmo-odoo force-pushed the odoo-dev:master-apikeys-xmo branch to 1e3d9bc Jun 6, 2019
@robodoo robodoo added the CI 🤖 label Jun 6, 2019
@nhomar

This comment has been minimized.

Copy link
Collaborator

nhomar commented Jun 6, 2019

Oooooooooooooooh! We implemented this in several ways since v5....

this is a huge and nice tool.....

api_key_ids = fields.One2many('res.users.apikeys', 'user_id', string="API Keys")

def __init__(self, pool, cr):
""" Override of __init__ to allow user to set its github_login. """

This comment has been minimized.

Copy link
@nhomar

nhomar Jun 6, 2019

Collaborator

s/github_login/token_login/g


name = fields.Char("Description", required=True, readonly=True)
user_id = fields.Many2one('res.users', index=True, required=True, readonly=True, ondelete="cascade")

This comment has been minimized.

Copy link
@nhomar

nhomar Jun 6, 2019

Collaborator

Can the key be hidden in some way once it is seeing? force the regeneration?, some extra field to check the app that can use it (by IP/name/header or something like that)?

odoo/addons/base/models/res_users.py Show resolved Hide resolved
odoo/addons/base/models/res_users.py Show resolved Hide resolved
@xmo-odoo

This comment has been minimized.

Copy link
Collaborator Author

xmo-odoo commented Jun 7, 2019

Can the key be hidden in some way once it is seeing?

The actual secret is never stored in cleartext, hashed and that hash isn't even visible to Odoo by default (there's no field for it). The only thing hiding the record would do is preclude auditing and deleting existing keys.

force the regeneration?

Delete the key.

@robodoo robodoo removed the CI 🤖 label Jun 17, 2019
@xmo-odoo xmo-odoo closed this Sep 19, 2019
@xmo-odoo xmo-odoo deleted the odoo-dev:master-apikeys-xmo branch Sep 19, 2019
@robodoo robodoo added the closed 💔 label Sep 19, 2019
@Yenthe666

This comment has been minimized.

Copy link
Collaborator

Yenthe666 commented Sep 19, 2019

@xmo-odoo could you give us a high level insight into why this was closed off and deleted? 🙏

@xmo-odoo

This comment has been minimized.

Copy link
Collaborator Author

xmo-odoo commented Sep 19, 2019

Because the entire thing is part of #33928, they're probably not going to be merged separately, and having PRs built on one another is less than helpful.

@Yenthe666

This comment has been minimized.

Copy link
Collaborator

Yenthe666 commented Sep 19, 2019

Aha, thanks for the link to the other PR!

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

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.