[FIX] auth_oauth: improve implicit flow implementation / compat

The current implementation is rather non-standard and largely an ad-hoc pre-RFC implementation, with a number of incompatibilities with the standard & actual real-world identity providers (IDP). Tested with the following IDP: - google oauth v1 - google oauth v3 - auth0 - okta Add support to bearer Authorization =================================== Sending the access token via "Authorization: Bearer $TOK" is strongly recommended by the RFC, and required for all IDP to support. The query parameter method is a legacy compatibility method and should be avoided. Query parameter access tokens are supported by Google (both v1 and v3), and auth0, but not okta. All three support bearer tokens. However making this the default is complicated by compatibility issues with current behavior. Use standard `sub`ject for identity =================================== The specification defines `sub` as the userinfo key providing the user identifier at the IDP. - auth0, okta, and google v3 use `sub` - google v1 uses `id` - google v1's `tokeninfo` (possibly v3 as well, not tested) uses `user_id` - odoo replicates the google v1 tokeninfo behavior, using `user_id` All the code is now standardised on `sub`, with `_auth_oauth_validate` performing unification under that key. Support non-json error bodies and WWW-Authenticate ================================================== Per-spec, there is no requirement for error (userinfo) responses to return any body, and all error information can be returned via `WWW-Authenticate`. Both auth0 and okta return empty bodies on error, though only okta returns a useful www-authenticate, or relevant 40x statuses (auth0 seems to always return 400, okta has been observed to return both 400 and 401 depending on client error). Error handling in `_auth_oauth_rpc` has been updated to only parse the body as json on success (200), and fallback on a generic error payload if `WWW-Authenticate` doesn't contain relevant information. Nonce ===== Okta requires a nonce to be provided. Misc ==== A few improvements which are in no way required but should make things simpler / clearer: - update the default scope to match the standard for the implicit flow's values (intersected with our requirements) - update the default google configuration to use the v3 endpoints and drop the tokeninfo request, remove the explicit scopes - update the label of `validation_endpoint` to match the official terminology, same with `auth_endpoint` - add a label to `body` in order to explain what it's for (as that's really confusing when the form just says `body` until you hover the field) Expected future updates ======================= These issues were left out and may lead to degraded security, but were considered too large changes fora stable compatibility-oriented update: * store and validate the nonce * request and properly validate the id token, as well as validate the access token (implicit guide sections 2.2.1, 2.2.2) * implement "basic" flow[^basic], and / or "hybrid" flow, the implicit flow[^implicit] is intended for purely client-side applications (SPAs), the "authorization code" flow is intended as the primary flow for normal web applications involving a server component, the main advantage of the hybrid flow is that the id token *can* contain the claims selected by `scope`, avoiding the need for the userinfo request[^idtoken] * remove support for query parameter requests * remove support for Google's v1 oauth and subject identifiers other than `sub`, facebook has not been tested but looks to support that key as well in the OpenGraph API[^fb], this will require migrating existing google providers to v3 implicitly (but would allow simplifying their configuration) References: RFC 6749, RFC 6750, Implicit Client Implementer's Guide 1.0 draft 23[^implicit] Closes #88618, closes #64348, fixes #63963, closes #63970, closes #69568 [^implicit]: https://openid.net/specs/openid-connect-implicit-1_0.html [^basic]: https://openid.net/specs/openid-connect-basic-1_0.html also known as "authorization code" flow [^fb]: https://www.facebook.com/.well-known/openid-configuration [^idtoken]: during testing, only auth0 returned the additional claims as part of the id token, but this may be a configuration issue closes odoo/odoo#91262 X-original-commit: fb3c4845b1549bc2e1378620a5f01e52aa4dbbdb Signed-off-by: Xavier Morel (xmo) <xmo@odoo.com>
2022-05-13 07:44:52 +02:00
parent 42a3b704f7
commit c5964f84d9
5 changed files with 54 additions and 39 deletions
@@ -1,10 +1,10 @@
 # -*- coding: utf-8 -*-
 # Part of Odoo. See LICENSE file for full copyright and licensing details.
-
+import base64
 import functools
-import logging
-
 import json
+import logging
+import os

 import werkzeug.urls
 import werkzeug.utils
@@ -65,6 +65,7 @@ class OAuthLogin(Home):
                redirect_uri=return_url,
                scope=provider['scope'],
                state=json.dumps(state),
+                nonce=base64.urlsafe_b64encode(os.urandom(16)),
            )
            provider['auth_link'] = "%s?%s" % (provider['auth_endpoint'], werkzeug.urls.url_encode(params))
        return providers
@@ -6,7 +6,6 @@
            <field name="auth_endpoint">https://accounts.odoo.com/oauth2/auth</field>
            <field name="scope">userinfo</field>
            <field name="validation_endpoint">https://accounts.odoo.com/oauth2/tokeninfo</field>
-            <field name="data_endpoint"></field>
            <field name="css_class">fa fa-fw o_custom_icon</field>
            <field name="body">Log in with Odoo.com</field>
            <field name="enabled" eval="True"/>
@@ -23,9 +22,8 @@
        <record id="provider_google" model="auth.oauth.provider">
            <field name="name">Google OAuth2</field>
            <field name="auth_endpoint">https://accounts.google.com/o/oauth2/auth</field>
-            <field name="scope">https://www.googleapis.com/auth/userinfo.email https://www.googleapis.com/auth/userinfo.profile</field>
-            <field name="validation_endpoint">https://www.googleapis.com/oauth2/v1/tokeninfo</field>
-            <field name="data_endpoint">https://www.googleapis.com/oauth2/v1/userinfo</field>
+            <field name="scope">openid profile email</field>
+            <field name="validation_endpoint">https://www.googleapis.com/oauth2/v3/userinfo</field>
            <field name="css_class">fa fa-fw fa-google</field>
            <field name="body">Log in with Google</field>
        </record>
@@ -48,12 +48,7 @@ msgstr ""

 #. module: auth_oauth
 #: model:ir.model.fields,field_description:auth_oauth.field_auth_oauth_provider__auth_endpoint
-msgid "Authentication URL"
-msgstr ""
-
-#. module: auth_oauth
-#: model:ir.model.fields,field_description:auth_oauth.field_auth_oauth_provider__body
-msgid "Body"
+msgid "Authorization URL"
 msgstr ""

 #. module: auth_oauth
@@ -89,7 +84,7 @@ msgstr ""

 #. module: auth_oauth
 #: model:ir.model.fields,field_description:auth_oauth.field_auth_oauth_provider__data_endpoint
-msgid "Data URL"
+msgid "Data Endpoint"
 msgstr ""

 #. module: auth_oauth
@@ -127,11 +122,6 @@ msgstr ""
 msgid "Last Updated on"
 msgstr ""

-#. module: auth_oauth
-#: model:ir.model.fields,help:auth_oauth.field_auth_oauth_provider__body
-msgid "Link text in Login Dialog"
-msgstr ""
-
 #. module: auth_oauth
 #: model:auth.oauth.provider,body:auth_oauth.provider_facebook
 msgid "Log in with Facebook"
@@ -147,6 +137,11 @@ msgstr ""
 msgid "Log in with Odoo.com"
 msgstr ""

+#. module: auth_oauth
+#: model:ir.model.fields,field_description:auth_oauth.field_auth_oauth_provider__body
+msgid "Login button label"
+msgstr ""
+
 #. module: auth_oauth
 #: model:ir.model.fields,field_description:auth_oauth.field_res_users__oauth_access_token
 msgid "OAuth Access Token"
@@ -225,13 +220,13 @@ msgid "System Parameter"
 msgstr ""

 #. module: auth_oauth
-#: model:ir.model,name:auth_oauth.model_res_users
-msgid "Users"
+#: model:ir.model.fields,field_description:auth_oauth.field_auth_oauth_provider__validation_endpoint
+msgid "UserInfo URL"
 msgstr ""

 #. module: auth_oauth
-#: model:ir.model.fields,field_description:auth_oauth.field_auth_oauth_provider__validation_endpoint
-msgid "Validation URL"
+#: model:ir.model,name:auth_oauth.model_res_users
+msgid "Users"
 msgstr ""

 #. module: auth_oauth
@@ -12,13 +12,13 @@ class AuthOAuthProvider(models.Model):

    name = fields.Char(string='Provider name', required=True)  # Name of the OAuth2 entity, Google, etc
    client_id = fields.Char(string='Client ID')  # Our identifier
-    auth_endpoint = fields.Char(string='Authentication URL', required=True)  # OAuth provider URL to authenticate users
-    scope = fields.Char()  # OAUth user data desired to access
-    validation_endpoint = fields.Char(string='Validation URL', required=True)  # OAuth provider URL to validate tokens
-    data_endpoint = fields.Char(string='Data URL')
+    auth_endpoint = fields.Char(string='Authorization URL', required=True)  # OAuth provider URL to authenticate users
+    scope = fields.Char(default='openid profile email')  # OAUth user data desired to access
+    validation_endpoint = fields.Char(string='UserInfo URL', required=True)  # OAuth provider URL to get user information
+    data_endpoint = fields.Char()
    enabled = fields.Boolean(string='Allowed')
    css_class = fields.Char(string='CSS class', default='fa fa-fw fa-sign-in text-primary')
-    body = fields.Char(required=True, help='Link text in Login Dialog', translate=True)
+    body = fields.Char(required=True, string="Login button label", help='Link text in Login Dialog', translate=True)
    sequence = fields.Integer(default=10)

    def _neutralize(self):
@@ -4,6 +4,7 @@
 import json

 import requests
+import werkzeug.http

 from odoo import api, fields, models
 from odoo.exceptions import AccessDenied, UserError
@@ -23,9 +24,21 @@ class ResUsers(models.Model):
        ('uniq_users_oauth_provider_oauth_uid', 'unique(oauth_provider_id, oauth_uid)', 'OAuth UID must be unique per provider'),
    ]

-    @api.model
    def _auth_oauth_rpc(self, endpoint, access_token):
-        return requests.get(endpoint, params={'access_token': access_token}).json()
+        if self.env['ir.config_parameter'].sudo().get_param('auth_oauth.authorization_header'):
+            response = requests.get(endpoint, headers={'Authorization': 'Bearer %s' % access_token}, timeout=10)
+        else:
+            response = requests.get(endpoint, params={'access_token': access_token}, timeout=10)
+
+        if response.ok: # nb: could be a successful failure
+            return response.json()
+
+        auth_challenge = werkzeug.http.parse_www_authenticate_header(
+            response.headers.get('WWW-Authenticate'))
+        if auth_challenge.type == 'bearer' and 'error' in auth_challenge:
+            return dict(auth_challenge)
+
+        return {'error': 'invalid_request'}

    @api.model
    def _auth_oauth_validate(self, provider, access_token):
@@ -37,11 +50,26 @@ class ResUsers(models.Model):
        if oauth_provider.data_endpoint:
            data = self._auth_oauth_rpc(oauth_provider.data_endpoint, access_token)
            validation.update(data)
+        # unify subject key under standard (sub), pop all possible and get most sensible
+        subject = next(filter(None, [
+            validation.pop(key, None)
+            for key in [
+                'sub', # standard
+                'id', # google v1 userinfo, facebook opengraph
+                'user_id', # google tokeninfo, odoo (tokeninfo)
+            ]
+        ]), None)
+        if not subject:
+            raise AccessDenied('Missing subject identity')
+        # also set on user_id for BC reasons, remove in master when the entire
+        # thing gets reworked
+        validation['sub'] = validation['user_id'] = subject
+
        return validation

    @api.model
    def _generate_signup_values(self, provider, validation, params):
-        oauth_uid = validation['user_id']
+        oauth_uid = validation['sub']
        email = validation.get('email', 'provider_%s_user_%s' % (provider, oauth_uid))
        name = validation.get('name', email)
        return {
@@ -65,7 +93,7 @@ class ResUsers(models.Model):

            This method can be overridden to add alternative signin methods.
        """
-        oauth_uid = validation['user_id']
+        oauth_uid = validation['sub']
        try:
            oauth_user = self.search([("oauth_uid", "=", oauth_uid), ('oauth_provider_id', '=', provider)])
            if not oauth_user:
@@ -94,13 +122,6 @@ class ResUsers(models.Model):
        #   continue with the process
        access_token = params.get('access_token')
        validation = self._auth_oauth_validate(provider, access_token)
-        # required check
-        if not validation.get('user_id'):
-            # Workaround: facebook does not send 'user_id' in Open Graph Api
-            if validation.get('id'):
-                validation['user_id'] = validation['id']
-            else:
-                raise AccessDenied()

        # retrieve and sign in user
        login = self._auth_oauth_signin(provider, validation, params)