[10.0][MIG] oauth_provider #889

lasley · 2017-07-06T23:27:42Z

This is a complete overhaul of the OAuth2 provider module, as well as a migration to v10. I added an abstract REST API as well, following are the docs:

REST API

This module also includes a basic REST-style CRUD interface backed by OAuth2 authentication.

List

Return allowed information from all records, with optional query.

Route: /api/rest/1.0/<string:model>

Accepts: GET

Args:
access_token (str): OAuth2 access token to utilize for the
operation.

model (str): Name of model to operate on. domain (array, optional): Domain to apply to the search, in the standard Odoo format. This will be appended to the scope’s pre-existing filter.

Returns:
array of objs: List of data mappings matching query.

Example:

Bash:

curl -H "Content-Type: application/json" \
   -X GET \
   http://localhost:8060/api/gogo/1.0/res.partner?access_token=2J391BkHipPmM9nXv2BF6V07fehWtM&domain=%5B%5B%22company_id%22%2C%20%22%3D%22%2C%201%5D%5D

{"jsonrpc": "2.0", "id": 5602, "result": [{"name": "Test Partner", "id": 5602}]}

Python:

json_data = {
    'access_token': token['access_token'],
    'domain': [('company_id', '=', 1)],
}
response = requests.get(
    'http://localhost:8060/api/gogo/1.0/res.partner/',
    params=json_data,
)
response_data = response.json()

{u'jsonrpc': u'2.0', u'id': 5602, u'result': [{u'name': u'Test Partner', u'id': 5602}]}

Read

Return allowed information from specific records.

Route: /api/rest/1.0/<string:model>/<int:record_id>

Accepts: GET

Args:
access_token (str): OAuth2 access token to utilize for the
operation.

model (str): Name of model to operate on. record_id (int): ID of record to get.

Returns:
obj: Record data.

Example:

Bash:

curl -H "Content-Type: application/json" \
   -X GET \
   http://localhost:8060/api/gogo/1.0/res.partner/5602?access_token=2J391BkHipPmM9nXv2BF6V07fehWtM

{"jsonrpc": "2.0", "id": 5602, "result": [{"name": "Test Partner", "id": 5602}]}

Python:

json_data = {
    'access_token': token['access_token'],
}
response = requests.post(
    'http://localhost:8060/api/gogo/1.0/res.partner/5602',
   params=json_data,
)
response_data = response.json()

{u'jsonrpc': u'2.0', u'id': 5602, u'result': [{u'name': u'Test Partner', u'id': 5602}]}

Create

Return allowed information from specific records.

Route: /api/rest/1.0/<string:model>

Accepts: POST

Args:
access_token (str): OAuth2 access token to utilize for the
operation.

model (str): Name of model to operate on. kwargs (mixed): All other named arguments are used as the data for record mutation.

Returns:
obj: Record data.

Example:

Bash:

curl -H "Content-Type: application/json" \
   -X POST \
   -d '{ "params": { "access_token": "2J391BkHipPmM9nXv2BF6V07fehWtM", "name": "Test Partner" }' \
   http://localhost:8060/api/gogo/1.0/res.partner

{"jsonrpc": "2.0", "id": 5602, "result": [{"name": "T

lasley · 2017-07-08T17:05:03Z

@sylvain-garancher - I'm going to change some things (detailed below), which will give us a full featured CRUD API. I was wondering, however, if we could relicense this to LGPL?

The reasoning for this is that I need to design an opinionated API based on some proprietary vendor data structures & I will not be able to release the final product as Open Source due to contractual obligations. I'd love to use this module as a base, and have planned a nice Open Source REST API module to sit on top of this one, that my proprietary one will sit on top of. I'm between a rock and a hard place with this vendor though - they're pretty tight about their IP.

Following are my plans for this module in order to support my requirements:

I am planning on shifting the public facing API in the following ways:
- Change /oauth2/otherinfo route to /oauth2/data
- Add POST, PUT, and DELETE functionality to /oauth2/data, allowing for full CRUD API
- Add a domain argument to the GET path, allowing for queries
Add create, read, update, delete Boolean fields to OAuthProviderScope
Break OauthProviderScope.get_data_for_model into a few helper methods. Public facing operation would be identical
Add create_record, write_record, delete_record to OauthProviderScope, with passthrough methods in OauthProviderToken (similar to the get_data_for_model architecture)
- The write and delete methods will validate scope by first trying to get the record. Validate found record fields returned against the fields intended to be modified
Change the name of get_data_for_model to get_records to match the new method naming a bit better

Some interesting design considerations that I'm not too sure of yet:

Should we validate scope against the written record? IE if the record does not match the scope after write (or create), should the transaction be rolled back
I wonder what it would take to implement the client credentials grant type

Garamotte · 2017-07-08T21:35:59Z

Hello,

I'm not against a licence change, if it allows the module to be improved instead of writing a new one to replace it. Maybe the OCA has to give an opinion, as the module is in OCA's repository ? @pedrobaeza @gurneyalex

The current version of the module only includes what my customer needed : login from a mobile application and retrieving some information about a subscription. Providing a full CRUD API is a great improvement idea :D
For the URL change, maybe you could add a new one, instead of a simple replacement, and make the old one deprecated (with a warning somewhere ?), if this is easy to do.
I agree on the method name change from get_data_for_model to get_records and split it in several small helper functions, if it can help in terms of modularity :)

About the create/read/write/delete fields, I wonder if it can be managed using ir.rule instead of modifying the model ?

For the scope validation, would it be possible to add a selection field to let the user define if it has to be valid after/before/both ?
I don't think the client credentials grant type needs much work to be implemented, the hardest part should be to write the tests. A big part already exist (even tests), as there are many common parts with other grant types.

lasley · 2017-07-08T23:39:40Z

For the URL change, maybe you could add a new one, instead of a simple replacement, and make the old one deprecated (with a warning somewhere ?), if this is easy to do.

Easy enough, yeah.

About the create/read/write/delete fields, I wonder if it can be managed using ir.rule instead of modifying the model ?

Technically ir.rules should already be covering these operations, due to the .sudo(user=oauth_provider_token.user_id). I figured that the extra granularity would be appreciated, but in hindsight I think you're right that we should just leave it at those. Otherwise the two layers could just cause confusion.

For the scope validation, would it be possible to add a selection field to let the user define if it has to be valid after/before/both ?

Good idea - I'll add that the to oauth.provider.scope model

I don't think the client credentials grant type needs much work to be implemented, the hardest part should be to write the tests. A big part already exist (even tests), as there are many common parts with other grant types.

Sweet - I thought it looked like a lot of it was already hooked up, glad you confirmed. It's not totally required for my project, but I would prefer to deploy with those vs. legacy tokens.

pedrobaeza · 2017-07-09T09:52:54Z

No problem in the license change if all the code authors agree and the target license is OSI.

lasley · 2017-07-10T16:18:58Z

Alright I've now made a dependent API & I would say that the structure of this is now finalized if anyone wants to take a look. I'll just be working on tests now 🚀

lasley · 2017-08-29T16:04:09Z

Do you know why the user_threshold module needs a separate cursor ?
Could we use a savepoint instead of a cursor ?

I honestly don't even think that is necessary. We'll find out - #955

lasley · 2017-08-29T16:21:26Z

Good call @sylvain-garancher, looks like we have a 🍏 when based on #955.

I tested too and it seems the logic in that module works while operating in the same cursor, so I'm not sure what the commit was about TBH

lasley · 2017-08-29T16:22:41Z

I'm curious - how did you figure out which module combination was required to replicate this?

Garamotte · 2017-08-29T16:33:54Z

First, I reproduced the issue on my system (basically by copy/pasting TravisCI's commands).

Next step, while Odoo was blocked, I tried a kill -3, which didn't work (the process was fully blocked), so I checked running SQL queries using SELECT * FROM pg_stat_activity WHERE datname = 'openerp_test', and found two queries (one idle, and the other blocked).

Then, the hardest part should have been to find the module from the SQL query, but I had the luck to find the threshold keyword in the query :)

Finally, knowing the line in the python code which was blocked, I easily found what was the real issue.

lasley · 2017-08-29T21:37:06Z

Thanks for the breakdown @sylvain-garancher - this will help next time I run into something similar. I'm a bit embarrassed how horrible my strategy was for this TBH

Garamotte

Some small comments inline.

The CRUD API seems OK, but not fully tested yet on my side. I'm not sure about the URLs format, waiting for opinions from someone else (although this can be an endless discussion).

Garamotte · 2017-10-02T11:47:44Z

oauth_provider/models/oauth_provider_scope.py

+                dictionary will be returned without nesting.
+        """
+
+        if isinstance(record_ids, int):


I'd prefer to avoid variables with several allowed types for an argument when possible.
Removing the if will require to change some calls to this method (basically replace record.id by record.ids or record_id by [record_id]), but I think this is better.

Garamotte · 2017-10-02T11:51:01Z

oauth_provider/models/oauth_provider_scope.py

+
+        if record_ids is None:
+            record_ids = []
+        elif isinstance(record_ids, int):


Same remark as the get_data method about several allowed types for an argument.

Garamotte · 2017-10-02T12:31:56Z

oauth_provider/models/oauth_provider_scope.py

+            bool: Indicating whether the records are within scope.
+        """
+
+        scoped_records = self.env[records._name].browse()


I didn't know calling browse() without any argument worked.
You can simply use scoped_records = self.env['records._name'] to get an empty recordset.

Hmmm for some reason I thought there was a difference, but it seems you are correct:

>>> env['res.partner'] res.partner() >>> env['res.partner'].browse() res.partner()

Garamotte · 2017-10-02T12:34:45Z

oauth_provider/models/oauth_provider_scope.py

+
+        scoped_records = self.env[records._name].browse()
+        for scope in self:
+            scoped_records += scope._get_scoped_records(


Prefer using the |= operator, as it removes duplicates. The += operator is not really helpful with Odoo's recordsets...

>>> partners = self.env['res.partner'] >>> partners += self.env['res.partner'].browse([1, 1]) >>> partners res.partner(1, 1) >>> partners |= self.env['res.partner'].browse([1, 1, 2]) >>> partners res.partner(1, 2)

Garamotte · 2017-10-02T13:06:35Z

oauth_provider/models/oauth_provider_scope.py

+            bool: Whether the values are within the scope.
+        """
+        scopes = self.filter_by_model(model)
+        field_names = scopes.mapped('field_ids').mapped('name')


You can call mapped only once: field_names = scopes.mapped('field_ids.name')

lasley · 2017-10-03T15:27:38Z

I'm open to changing the URI schema if some good arguments are exposed. We haven't designed anything specific on top of the REST API yet, so it would be an easy change. You are correct that this could be an endless discussion though 😆 🚀

lasley · 2017-10-04T23:02:47Z

Comments handled @sylvain-garancher

simahawk · 2017-10-05T09:03:34Z

@lasley just a ping in case you missed this #1011

Garamotte

I finally took time to carefully review the code, great work @lasley !
The docstrings are really good too, thanks for that (that's something I'm not yet good at).

Still not enough time to make some functional tests :(

Garamotte · 2017-12-02T11:29:01Z

oauth_provider/models/oauth_provider_scope.py

+            OauthProviderScope: Recordsets associated to the model.
+        """
+        if isinstance(model, models.BaseModel):
+            model = model._name


I find it cleaner to avoid using isinstance when it's not needed. Here, we can force the argument to be a string.

When we have a BaseModel instance, then we can simply call filter_by_model(our_model._name).

Garamotte · 2017-12-02T16:15:25Z

oauth_provider/models/oauth_provider_scope.py

+        return data
+
+    @api.multi
+    def create_record(self, model_name, vals):


I don't understand why this method is in the oauth.provider.scope model, instead of calling the three methods (_validate_scope_field, self.env['model_name'].create and _validate_scope_record) directly in the create_record method of oauth.provider.token ?
What's the benefit of having this method on the scope ?

Same question about write and delete, obviously.

Garamotte · 2017-12-02T16:18:35Z

oauth_provider/models/oauth_provider_scope.py

+
+        scoped_records = self.env[records._name]
+        for scope in self:
+            scoped_records |= scope._get_scoped_records(


I guess this will not work if you have scopes using different models on the same token.

lasley · 2017-12-02T20:01:27Z

Thanks for the review @sylvain-garancher - I'll tend to the comments soon. I actually moved some of this code (the stuff to allow for proper receipt of JSON requests) into OCA/webhook#3 and will need to switch this code to use that. I think I'm about to make it a Python package instead though.

Regarding functional tests - I haven't used the REST API provided here much, but we do have quite a few proprietary APIs built on top of it that have been working quite well for some time. It's really helped us for automation.

* Bump version * Rename manifest * Change openerp references to odoo

oca-clabot · 2019-05-19T18:49:28Z

Hey @lasley, thank you for your Pull Request.

It looks like some users haven't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here: http://odoo-community.org/page/cla
Here is a list of the users:

Sylvain GARANCHER sylvain.garancher@syleam.fr (no github login found)

Appreciation of efforts,
OCA CLAbot

eggu68 · 2020-12-31T14:31:38Z

Hello, what is the status on this module migration to newer versions? I see that it was in the 9.0 branch but not later? Any plans to have this oauth_provider in Odoo 13 or 14? Or are there other alternatives?

github-actions · 2022-07-24T12:33:23Z

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.

Syncing from upstream OCA/server-tools (13.0)

lasley added the work in progress label Jul 6, 2017

lasley added this to the 10.0 milestone Jul 6, 2017

lasley force-pushed the release/10.0/LABS-44-update-oauth_provider-to-v10 branch from e462403 to 91a367d Compare July 6, 2017 23:58

lasley mentioned this pull request Jul 7, 2017

[10.0][MIG] oauth_provider_jwt #890

Closed

1 task

lasley added needs review work in progress and removed work in progress needs review labels Jul 7, 2017

lasley force-pushed the release/10.0/LABS-44-update-oauth_provider-to-v10 branch 6 times, most recently from 4473389 to d55ba72 Compare July 8, 2017 19:28

lasley force-pushed the release/10.0/LABS-44-update-oauth_provider-to-v10 branch 4 times, most recently from 3fbf9f8 to dd1216f Compare July 8, 2017 23:31

lasley force-pushed the release/10.0/LABS-44-update-oauth_provider-to-v10 branch 2 times, most recently from 0055fff to 9378037 Compare July 9, 2017 01:30

lasley force-pushed the release/10.0/LABS-44-update-oauth_provider-to-v10 branch 4 times, most recently from 68b3114 to d970605 Compare July 10, 2017 00:38

lasley force-pushed the release/10.0/LABS-44-update-oauth_provider-to-v10 branch from d970605 to 17b3016 Compare July 10, 2017 16:28

lasley mentioned this pull request Aug 29, 2017

[FIX] user_threshold: Don't create or commit new cursor #955

Merged

lasley force-pushed the release/10.0/LABS-44-update-oauth_provider-to-v10 branch 3 times, most recently from 1201a71 to 4a09119 Compare August 29, 2017 16:17

lasley force-pushed the release/10.0/LABS-44-update-oauth_provider-to-v10 branch from 4a09119 to a398aac Compare September 6, 2017 21:29

lasley force-pushed the release/10.0/LABS-44-update-oauth_provider-to-v10 branch from ed3500f to fcf8b7e Compare September 23, 2017 20:23

This was referenced Sep 25, 2017

web: can't implement custom HTTP controller if request has application/json mime-type odoo/odoo#7766

Closed

[Security] passwords are stored as cleartext in session odoo/odoo#8029

Closed

Garamotte suggested changes Oct 2, 2017

View reviewed changes

lasley mentioned this pull request Oct 10, 2017

[ADD] base_json_request: Module to allow standard JSON requests OCA/webhook#3

Open

Garamotte reviewed Dec 2, 2017

View reviewed changes

Sylvain GARANCHER and others added 3 commits May 19, 2019 11:48

Add oauth_provider module

b2757ec

OCA Transbot updated translations from Transifex

5c1825c

[MIG] oauth_provider: Upgrade to v10

ca54f57

* Bump version * Rename manifest * Change openerp references to odoo

lasley force-pushed the release/10.0/LABS-44-update-oauth_provider-to-v10 branch from 90e3122 to ca54f57 Compare May 19, 2019 18:49

github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jul 24, 2022

github-actions bot closed this Aug 28, 2022

SiesslPhillip pushed a commit to grueneerde/OCA-server-tools that referenced this pull request Nov 20, 2024

Merge pull request OCA#889 from OCA/13.0

278c8a1

Syncing from upstream OCA/server-tools (13.0)

Uh oh!

[10.0][MIG] oauth_provider #889

[10.0][MIG] oauth_provider #889

Uh oh!

Conversation

lasley commented Jul 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

REST API

List

Read

Create

Uh oh!

lasley commented Jul 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Garamotte commented Jul 8, 2017

Uh oh!

lasley commented Jul 8, 2017

Uh oh!

pedrobaeza commented Jul 9, 2017

Uh oh!

lasley commented Jul 10, 2017

Uh oh!

lasley commented Aug 29, 2017

Uh oh!

lasley commented Aug 29, 2017

Uh oh!

lasley commented Aug 29, 2017

Uh oh!

Garamotte commented Aug 29, 2017

Uh oh!

lasley commented Aug 29, 2017

Uh oh!

Garamotte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lasley commented Oct 3, 2017

Uh oh!

lasley commented Oct 4, 2017

Uh oh!

simahawk commented Oct 5, 2017

Uh oh!

Garamotte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lasley commented Dec 2, 2017

Uh oh!

oca-clabot commented May 19, 2019

Uh oh!

eggu68 commented Dec 31, 2020

Uh oh!

github-actions bot commented Jul 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lasley commented Jul 6, 2017 •

edited

Loading

lasley commented Jul 8, 2017 •

edited

Loading