[9.0] Add oauth_provider module by Garamotte · Pull Request #611 · OCA/server-tools

Garamotte · 2016-11-16T16:58:38Z

A new module that allows to use Odoo as OAuth provider.
The OAuth protocol is used to delegate authentication, and authorization to access some resources.

This module really need security reviews/tests, as I'm not fully comfortable with the OAuh protocol.

The RFC defines four profiles: Implicit grant, Authorization code grant, Password credentials grand and User credentials grant.
Only Implicit grant and Authorization code grant are implemented yet, and I don't plan to implement the others for now. The work should not be very hard, as most parts are common, but I don't actually need these profiles, and don't have time to do it for the moment.

Note: I created the PR to allow you to begin testing the module.
@pedrobaeza said that this repo will be split into several smaller repos, this PR will need to be recreated on one of the new repos when they are created.

Garamotte · 2016-11-17T07:59:41Z

Travis errors unrelated (from database_cleanup and auth_signup_verify_email modules).

Garamotte · 2016-11-21T08:59:03Z

Last little thing done (and rebased on 9.0).
You can add replace the WIP label by needs review.

lasley

Awesome thanks @sylvain-garancher - comments inline. Also my bad on leaving the WIP tag for so long, missed the comment.

lasley · 2016-11-30T17:32:13Z

+        '/oauth2/authorize', type='http', auth='user', methods=['POST'])
+    def authorize_post(self, *args, **kwargs):
+        """
+        Redirect to the requested URI during the authorization


Not a blocker or anything, just FYI - Docstrings should start on the same line that the quote is started on. The first line (in a multi) should be a complete summary, followed by a blank line, followed by more description if necessary. Check out PEP-257 for more

Didn't know that, thanks.
I'll have to change my comments on many other PRs :D

Hah it's one of those things that you can't unsee once you know it, right?!

lasley · 2016-11-30T17:34:28Z

+            http.request.httprequest.values.items())
+        headers = http.request.httprequest.headers
+
+        valid, oauthlib_request = oauth2_server.verify_request(


This token & argument retrieval followed verification block seems to be repeated a lot. It would be nice if it was just a helper method instead.

You're right, I'll provide a helper method (which will be useful for modules depending on this one too).

lasley · 2016-11-30T17:41:36Z

+        Authenticate the client
+        """
+        raise NotImplementedError
+        # return False


Please remove this commented code

I leaved this line here to remind the return value's type, but I may add this in the docstring.

Docstring would be the preferred please 😄

lasley · 2016-11-30T17:42:04Z

+        """
+        Authenticate the client
+        """
+        raise NotImplementedError


No client auth implementation?

This file is an implementation of the interface provided by the oauthlib module.
As the user is already logged on Odoo, we have the user in the request.env, so I didn't need to implement this method (unless I miss something).

Nope nope, was definitely me missing something. Can we add a docstring note that this is an interface then?

lasley · 2016-11-30T17:43:34Z

+        request.client = http.request.env['oauth.provider.client'].search([
+            ('identifier', '=', client_id),
+        ])
+        request.odoo_user = http.request.env['oauth.provider.client'].env.user


Why the env and user from oauth.provider.client? It's the same env and user isn't it, or is there some fancy context switching that I am missing?

I think this is a bad copy/paste from the previous line, I'll fix that :)

lasley · 2016-11-30T17:52:46Z

+<?xml version="1.0" encoding="UTF-8"?>
+<openerp>
+    <data>
+        ##############################################################################


Wait... how is this not breaking XML? Please use the shorter header though, and put just under the xml version. This is how I do it https://github.com/OCA/vertical-medical/blob/9.0/medical/views/medical_menu.xml#L3 - although we don't have specific guidelines in OCA on the XML part AFAIK

This is part of our own templates since at least v5, I think this is time to update our templates.
I'll change that.

lasley · 2016-11-30T17:52:55Z

@@ -0,0 +1,53 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<openerp>


<odoo> and can remove data tag

Oops, I kept the old templates for my XML files :)

lasley · 2016-11-30T17:53:09Z

@@ -0,0 +1,60 @@
+<?xml version="1.0" encoding="utf-8"?>
+<openerp>


Odoo tag and remove data

lasley · 2016-11-30T17:53:20Z

+<?xml version="1.0" encoding="utf-8"?>
+<openerp>
+    <data>
+        ##############################################################################


Same note regarding use of smaller header

lasley · 2016-11-30T17:54:14Z

+        self.assertEqual(client.grant_type, 'authorization_code')
+        self.assertEqual(client.response_type, 'code')
+
+        # Web Application


IMO the scope of all these tests is too large. It's best to make smaller test methods in order to provide a better view of things when stuff is broken vs. execution stopping on the first broken test.

You advise to split in one test method per possible value ?

I left all in a single method, because every test in this method are really close, but I don't know all best practices about tests.

Yeah the smaller the test the better. I go to great lengths to follow a 1:1 method:assertion ratio, but sometimes it's not possible given context and setup required.

Consider an example using these tests. If the response_type for the web application is wrong, we won't know if it's right for Mobile, Legacy, and Backend because the test will stop execution at the first failed assertion.

It's incredibly valuable to have the whole picture when debugging failing tests because it can help point us closer to the actual error, or specific edge case that is being debugged (maybe only X of the Y are failing).

Ok, I understand. I'll split the tests when it is possible ;)

Garamotte · 2016-12-01T14:31:20Z

@lasley I updated the PR with all of your comments.

I also created a common class for both controllers tests (initialization and many tests are the same for all controllers).
Is it done right ? I prefixed all methods by an _ because Odoo ran tests from the common class in addition to running them from the specific classes. I'd like to avoid the need to redefine each common test in the custom test classes, if possible, but didn't found how to do that.

Garamotte · 2016-12-01T14:54:09Z

Rebased on 9.0, but runbot seems broken.
Is it caused by the Docker build ? Do I need to update some configuration files in the repo ?

lasley

@sylvain-garancher LGTM OCA/runbot-addons#111 for Runbot issue, which is definitely not related to anything you did

moylop260

LGTM
I'll review the runbot issue

Garamotte · 2016-12-06T16:00:21Z

I'll finally have to implement the password credentials grant (token request by providing username and password), and add it in this PR.

Garamotte · 2016-12-08T08:16:37Z

A short reminder for myself : Check if everything works well with multiple databases (unauthenticated routes may not work if no database has been selected).

Garamotte · 2016-12-28T16:49:42Z

Tests split is finished, I think this is good now, so I removd the WIP tag in title.
Still waiting for a confirmation from my customer for a potential cors issue.

Garamotte · 2017-01-10T15:33:46Z

After a rebase on 9.0, Travis is failing on many other modules, but not on this module's tests.

Garamotte · 2017-01-17T08:11:53Z

After many tests, my customer has no new issue now, using the "legacy application" profile.
The cors issue doesn't seem to be linked to this module.

lasley

Thanks @sylvain-garancher - code looks good. Also confirmed that the CI fail isn't you and is instead #689

Garamotte · 2017-01-26T10:56:20Z

I have an issue with the default values for UUID fields (oauth_identifier on res.users).
The default looks like this : https://github.com/OCA/server-tools/pull/611/files#diff-4a2885ce924a60c01c46e75c1663da33R14

When installing the module, all existing users get the same UUID as identifier... which is not what we want. How could we generate a new random UUID for each user ?
This is because the default_get method is only called once for all existing records : https://github.com/odoo/odoo/blob/5f63878ff6af844b6eab8d233a3f52ee7622eccd/openerp/models.py#L2396-L2419

I initially though about a pre_init_hook which could create the column in database, then populate it (to allow the unique constraint be added on the first run), but there will still be an issue if a script creates users in batches.
Maybe overriding the create method in addtion to a pre_init_hook ? Any other idea ?

lasley · 2017-01-26T17:43:26Z

We circumvented this in medical by moving the default generation into an overrided create method instead. Looks like that would still work here? Basiccally:

class TestClass:
  def create(self, vals):
    vals['unique_key'] = uuid4()
    return super(TestClass, self).create(vals)

Downfall of this is from a UI perspective, because the default won't exist until save. We considered it not a problem in our context, and I don't think it would be here either - the val is read-only for humans anyways.

Garamotte · 2017-01-27T15:22:03Z

I added the pre_init_hook for now, as our main issue was the initial install of the module, kept in a separate commit for the review, will be squashed later.

I also added the missing copy=False attribute on the two uuid fields (oauth_identifier on res.users and identifier on oauth.provider.client). Creating user accounts with the auth_signup module is done by duplicating the template user, and all new users had the same identifier.

Do you still see a need for overriding the create method ?
When creating several users at once, either the identifier will be in data (and if the same value appears multiple times, the unique constraint will raise), or the identifier will not be in data (in this case, the default function should be called for each new user).

lasley · 2017-01-27T18:30:13Z

@> Do you still see a need for overriding the create method ?

Nope, I'd say you covered the bases. We ended up having to implement the create I think due to layers and layers of delegate inheritance at play & some connectors that bulk add data in ways a user normally wouldn't.

Looks like lint is 🔴 though. The other two failures are known, one of which is being resolved in #719 & the other should clear itself per Pedro's email earlier.

Garamotte · 2017-02-08T14:19:07Z

Lint fixed, rebased on 9.0 to benefit from the tests fixes of the database_cleanup module.
Everything looks good.

Garamotte · 2017-03-10T08:20:41Z

Rebased on 9.0 to resolve the requirements.txt conflict.
This module is now used in production by our customer, since a couple of weeks, without any issue :)

lasley

We've been in prod for a few weeks now too. Excellent work, @sylvain-garancher

Garamotte · 2017-04-11T09:55:26Z

Rebased on 9.0 to fix the requirements.txt conflict.

lasley · 2017-04-11T17:25:24Z

Thanks for another badass module @sylvain-garancher

Syncing from upstream OCA/server-tools (12.0)

lasley added the work in progress label Nov 16, 2016

Garamotte force-pushed the 9.0-add-oauth_provider-module branch 6 times, most recently from 6840486 to f82b89e Compare November 16, 2016 17:47

Garamotte force-pushed the 9.0-add-oauth_provider-module branch 3 times, most recently from 5270939 to 4e6857e Compare November 21, 2016 08:57

Garamotte force-pushed the 9.0-add-oauth_provider-module branch from 4e6857e to 7dfe322 Compare November 21, 2016 10:39

Garamotte changed the title ~~[9.0][WIP] Add oauth_provider module~~ [9.0] Add oauth_provider module Nov 21, 2016

Garamotte force-pushed the 9.0-add-oauth_provider-module branch from 7dfe322 to a5dc3af Compare November 28, 2016 10:52

lasley added needs review and removed work in progress labels Nov 30, 2016

lasley added this to the 9.0 milestone Nov 30, 2016

lasley mentioned this pull request Nov 30, 2016

[RFC] Single Sign On YannickB/odoo-hosting#166

Open

lasley suggested changes Nov 30, 2016

View reviewed changes

Garamotte force-pushed the 9.0-add-oauth_provider-module branch 2 times, most recently from f3b8cb7 to 21677c1 Compare December 1, 2016 14:16

Garamotte force-pushed the 9.0-add-oauth_provider-module branch from 21677c1 to 9bb61bf Compare December 1, 2016 14:41

lasley mentioned this pull request Dec 1, 2016

Dockerized Runbot - Missing Image OCA/runbot-addons#111

Closed

lasley approved these changes Dec 1, 2016

View reviewed changes

moylop260 approved these changes Dec 1, 2016

View reviewed changes

Garamotte force-pushed the 9.0-add-oauth_provider-module branch from 3680b62 to e2a3631 Compare December 28, 2016 16:46

Garamotte changed the title ~~[9.0][WIP] Add oauth_provider module~~ [9.0] Add oauth_provider module Dec 28, 2016

lasley added needs review and removed work in progress labels Dec 28, 2016

Garamotte force-pushed the 9.0-add-oauth_provider-module branch 2 times, most recently from 0af84ff to 0c95d22 Compare January 10, 2017 10:24

lasley approved these changes Jan 17, 2017

View reviewed changes

Garamotte force-pushed the 9.0-add-oauth_provider-module branch from 0c95d22 to 5a177fe Compare January 26, 2017 11:08

Garamotte force-pushed the 9.0-add-oauth_provider-module branch 2 times, most recently from 1efa5a2 to c00a067 Compare January 27, 2017 15:17

Garamotte force-pushed the 9.0-add-oauth_provider-module branch from c00a067 to 2e3bada Compare January 27, 2017 19:07

Garamotte force-pushed the 9.0-add-oauth_provider-module branch from 2e3bada to d0b2501 Compare February 8, 2017 14:02

Garamotte force-pushed the 9.0-add-oauth_provider-module branch from d0b2501 to f59ef9a Compare March 10, 2017 08:19

lasley approved these changes Mar 10, 2017

View reviewed changes

[ADD] Add oauth_provider module

0aa4b7e

Garamotte force-pushed the 9.0-add-oauth_provider-module branch from f59ef9a to 0aa4b7e Compare April 11, 2017 09:54

lasley merged commit 0dbdf73 into OCA:9.0 Apr 11, 2017

Garamotte deleted the 9.0-add-oauth_provider-module branch April 12, 2017 07:00

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

Merge pull request OCA#611 from OCA/12.0

ecc0a24

Syncing from upstream OCA/server-tools (12.0)

		@@ -0,0 +1,53 @@
		<?xml version="1.0" encoding="UTF-8"?>
		<openerp>

		@@ -0,0 +1,60 @@
		<?xml version="1.0" encoding="utf-8"?>
		<openerp>

Uh oh!

Uh oh!

Conversation

Garamotte commented Nov 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Garamotte commented Nov 17, 2016

Uh oh!

Garamotte commented Nov 21, 2016

Uh oh!

lasley 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

lasley Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Garamotte commented Dec 1, 2016

Uh oh!

Garamotte commented Dec 1, 2016

Uh oh!

lasley left a comment

Choose a reason for hiding this comment

Uh oh!

moylop260 left a comment

Choose a reason for hiding this comment

Uh oh!

Garamotte commented Dec 6, 2016

Uh oh!

Garamotte commented Dec 8, 2016

Uh oh!

Garamotte commented Dec 28, 2016

Uh oh!

Garamotte commented Jan 10, 2017

Uh oh!

Garamotte commented Jan 17, 2017

Garamotte commented Nov 16, 2016 •

edited

Loading

lasley Nov 30, 2016 •

edited

Loading

lasley left a comment •

edited

Loading

lasley commented Jan 26, 2017 •

edited

Loading

lasley commented Jan 27, 2017 •

edited

Loading