From 3e0bcb53d9737d85d344da9d23da6d287000612a Mon Sep 17 00:00:00 2001 From: Holger Brunn Date: Mon, 15 Nov 2021 16:08:13 +0100 Subject: [PATCH 01/10] [IMP] auth_oidc: allow assign groups from token claims --- auth_oidc/__manifest__.py | 6 +++- auth_oidc/demo/local_keycloak.xml | 5 +++ auth_oidc/models/auth_oauth_provider.py | 39 ++++++++++++++++++++- auth_oidc/models/res_users.py | 25 +++++++++++++ auth_oidc/security/ir.model.access.csv | 2 ++ auth_oidc/tests/test_auth_oidc_auth_code.py | 6 ++++ auth_oidc/views/auth_oauth_provider.xml | 10 ++++++ 7 files changed, 91 insertions(+), 2 deletions(-) create mode 100644 auth_oidc/security/ir.model.access.csv diff --git a/auth_oidc/__manifest__.py b/auth_oidc/__manifest__.py index 5e046a73f3..50c8524e47 100644 --- a/auth_oidc/__manifest__.py +++ b/auth_oidc/__manifest__.py @@ -16,6 +16,10 @@ "summary": "Allow users to login through OpenID Connect Provider", "external_dependencies": {"python": ["python-jose"]}, "depends": ["auth_oauth"], - "data": ["views/auth_oauth_provider.xml", "data/auth_oauth_data.xml"], + "data": [ + "security/ir.model.access.csv", + "views/auth_oauth_provider.xml", + "data/auth_oauth_data.xml", + ], "demo": ["demo/local_keycloak.xml"], } diff --git a/auth_oidc/demo/local_keycloak.xml b/auth_oidc/demo/local_keycloak.xml index 919754db99..92588dc952 100644 --- a/auth_oidc/demo/local_keycloak.xml +++ b/auth_oidc/demo/local_keycloak.xml @@ -17,4 +17,9 @@ name="jwks_uri" >http://localhost:8080/auth/realms/master/protocol/openid-connect/certs + + + + token['name'] == 'test' + diff --git a/auth_oidc/models/auth_oauth_provider.py b/auth_oidc/models/auth_oauth_provider.py index ac498a7cdb..15a5ff7f5e 100644 --- a/auth_oidc/models/auth_oauth_provider.py +++ b/auth_oidc/models/auth_oauth_provider.py @@ -2,12 +2,13 @@ # Copyright 2021 ACSONE SA/NV # License: AGPL-3.0 or later (http://www.gnu.org/licenses/agpl) +import collections import logging import secrets import requests -from odoo import fields, models, tools +from odoo import api, exceptions, fields, models, tools try: from jose import jwt @@ -46,6 +47,11 @@ class AuthOauthProvider(models.Model): string="Token URL", help="Required for OpenID Connect authorization code flow." ) jwks_uri = fields.Char(string="JWKS URL", help="Required for OpenID Connect.") + group_line_ids = fields.One2many( + "auth.oauth.provider.group_line", + "provider_id", + string="Group mappings", + ) @tools.ormcache("self.jwks_uri", "kid") def _get_keys(self, kid): @@ -104,3 +110,34 @@ def _decode_id_token(self, access_token, id_token, kid): if error: raise error return {} + + +class AuthOauthProviderGroupLine(models.Model): + _name = "auth.oauth.provider.group_line" + + provider_id = fields.Many2one("auth.oauth.provider", required=True) + group_id = fields.Many2one("res.groups", required=True) + expression = fields.Char(required=True, help="Variables: user, token") + + @api.constrains("expression") + def _check_expression(self): + for this in self: + try: + this._eval_expression(self.env.user, {}) + except (AttributeError, KeyError, NameError) as e: + raise exceptions.ValidationError("\n".join(e.args)) + + def _eval_expression(self, user, token): + self.ensure_one() + + class Defaultdict2(collections.defaultdict): + def __init__(self, *args, **kwargs): + super().__init__(Defaultdict2, *args, **kwargs) + + return tools.safe_eval.safe_eval( + self.expression, + { + "user": user, + "token": Defaultdict2(token), + }, + ) diff --git a/auth_oidc/models/res_users.py b/auth_oidc/models/res_users.py index 1684480fa4..eb3bede25f 100644 --- a/auth_oidc/models/res_users.py +++ b/auth_oidc/models/res_users.py @@ -64,6 +64,12 @@ def auth_oauth(self, provider, params): _logger.error("No id_token in response.") raise AccessDenied() validation = oauth_provider._parse_id_token(id_token, access_token) + if oauth_provider.data_endpoint: + data = requests.get( + oauth_provider.data_endpoint, + headers={"Authorization": "Bearer %s" % access_token}, + ).json() + validation.update(data) # required check if "sub" in validation and "user_id" not in validation: # set user_id for auth_oauth, user_id is not an OpenID Connect standard @@ -80,3 +86,22 @@ def auth_oauth(self, provider, params): raise AccessDenied() # return user credentials return (self.env.cr.dbname, login, access_token) + + @api.model + def _auth_oauth_signin(self, provider, validation, params): + login = super()._auth_oauth_signin(provider, validation, params) + user = self.search([("login", "=", login)]) + if user: + group_updates = [] + for group_line in ( + self.env["auth.oauth.provider"].browse(provider).group_line_ids + ): + if group_line._eval_expression(user, validation): + if group_line.group_id not in user.groups_id: + group_updates.append((4, group_line.group_id.id)) + else: + if group_line.group_id in user.groups_id: + group_updates.append((3, group_line.group_id.id)) + if group_updates: + user.write({"groups_id": group_updates}) + return login diff --git a/auth_oidc/security/ir.model.access.csv b/auth_oidc/security/ir.model.access.csv new file mode 100644 index 0000000000..503e4c7529 --- /dev/null +++ b/auth_oidc/security/ir.model.access.csv @@ -0,0 +1,2 @@ +id,name,model_id:id,group_id:id,perm_read,perm_write,perm_create,perm_unlink +access_auth_oauth_provider_group_line,auth_oauth_provider,model_auth_oauth_provider_group_line,base.group_system,1,1,1,1 diff --git a/auth_oidc/tests/test_auth_oidc_auth_code.py b/auth_oidc/tests/test_auth_oidc_auth_code.py index a1a08b0a71..6e9054c884 100644 --- a/auth_oidc/tests/test_auth_oidc_auth_code.py +++ b/auth_oidc/tests/test_auth_oidc_auth_code.py @@ -308,3 +308,9 @@ def test_login_with_jwk_format(self): ) self.assertEqual(token, "122/3") self.assertEqual(login, user.login) + + def test_group_expression(self): + """Test that group expressions evaluate correctly""" + group_line = self.env.ref("auth_oidc.local_keycloak").group_line_ids[:1] + group_line.expression = 'token["test"]["test"] == 1' + self.assertFalse(group_line._eval_expression(self.env.user, {})) diff --git a/auth_oidc/views/auth_oauth_provider.xml b/auth_oidc/views/auth_oauth_provider.xml index 90c931b417..dbdeadd8ef 100644 --- a/auth_oidc/views/auth_oauth_provider.xml +++ b/auth_oidc/views/auth_oauth_provider.xml @@ -19,6 +19,16 @@ + + + + + + + + + + From 18305ea32b9cb9713e7fdfc92db164efd828755b Mon Sep 17 00:00:00 2001 From: Didier 'OdyX' Raboud Date: Fri, 6 Sep 2024 16:36:32 +0200 Subject: [PATCH 02/10] =?UTF-8?q?[IMP]=20auth=5Foidc:=20use=20Command.LINK?= =?UTF-8?q?/UNLINK=C2=A0instead=20of=20raw=20numbers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- auth_oidc/models/res_users.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/auth_oidc/models/res_users.py b/auth_oidc/models/res_users.py index eb3bede25f..6972d9af7c 100644 --- a/auth_oidc/models/res_users.py +++ b/auth_oidc/models/res_users.py @@ -8,6 +8,7 @@ from odoo import api, models from odoo.exceptions import AccessDenied +from odoo.fields import Command from odoo.http import request _logger = logging.getLogger(__name__) @@ -98,10 +99,10 @@ def _auth_oauth_signin(self, provider, validation, params): ): if group_line._eval_expression(user, validation): if group_line.group_id not in user.groups_id: - group_updates.append((4, group_line.group_id.id)) + group_updates.append((Command.LINK, group_line.group_id.id)) else: if group_line.group_id in user.groups_id: - group_updates.append((3, group_line.group_id.id)) + group_updates.append((Command.UNLINK, group_line.group_id.id)) if group_updates: user.write({"groups_id": group_updates}) return login From b360551e4d9b91843b06e96eea0b080c65f6d68b Mon Sep 17 00:00:00 2001 From: Didier 'OdyX' Raboud Date: Fri, 6 Sep 2024 16:36:54 +0200 Subject: [PATCH 03/10] [IMP] auth_oidc: add oauth.provider.group_line _description attribute --- auth_oidc/models/auth_oauth_provider.py | 1 + 1 file changed, 1 insertion(+) diff --git a/auth_oidc/models/auth_oauth_provider.py b/auth_oidc/models/auth_oauth_provider.py index 15a5ff7f5e..d2405af20f 100644 --- a/auth_oidc/models/auth_oauth_provider.py +++ b/auth_oidc/models/auth_oauth_provider.py @@ -114,6 +114,7 @@ def _decode_id_token(self, access_token, id_token, kid): class AuthOauthProviderGroupLine(models.Model): _name = "auth.oauth.provider.group_line" + _description = "OAuth mapping between an Odoo group and an expression" provider_id = fields.Many2one("auth.oauth.provider", required=True) group_id = fields.Many2one("res.groups", required=True) From 5450338485aa556a5bcbc1fa38fd3a9935f195b3 Mon Sep 17 00:00:00 2001 From: Didier 'OdyX' Raboud Date: Fri, 6 Sep 2024 17:28:54 +0200 Subject: [PATCH 04/10] =?UTF-8?q?[IMP]=20auth=5Foidc:=20use=20raise=20?= =?UTF-8?q?=E2=80=A6=20from=20=E2=80=A6=20in=20exception=20handling?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- auth_oidc/models/auth_oauth_provider.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/auth_oidc/models/auth_oauth_provider.py b/auth_oidc/models/auth_oauth_provider.py index d2405af20f..582e7c0513 100644 --- a/auth_oidc/models/auth_oauth_provider.py +++ b/auth_oidc/models/auth_oauth_provider.py @@ -126,7 +126,7 @@ def _check_expression(self): try: this._eval_expression(self.env.user, {}) except (AttributeError, KeyError, NameError) as e: - raise exceptions.ValidationError("\n".join(e.args)) + raise exceptions.ValidationError("\n".join(e.args)) from e def _eval_expression(self, user, token): self.ensure_one() From 3830ddcded49e6e465faf58d786d58c3e330f2a3 Mon Sep 17 00:00:00 2001 From: Didier 'OdyX' Raboud Date: Fri, 6 Sep 2024 17:30:25 +0200 Subject: [PATCH 05/10] [IMP] auth_oidc: use a 10 seconds' timeout for the GET to data_endpoint --- auth_oidc/models/res_users.py | 1 + 1 file changed, 1 insertion(+) diff --git a/auth_oidc/models/res_users.py b/auth_oidc/models/res_users.py index 6972d9af7c..fdb8c9c5f3 100644 --- a/auth_oidc/models/res_users.py +++ b/auth_oidc/models/res_users.py @@ -69,6 +69,7 @@ def auth_oauth(self, provider, params): data = requests.get( oauth_provider.data_endpoint, headers={"Authorization": "Bearer %s" % access_token}, + timeout=10, ).json() validation.update(data) # required check From 7e0970b833b44c4cc5a48bffc2b19d9723d04369 Mon Sep 17 00:00:00 2001 From: Didier 'OdyX' Raboud Date: Wed, 11 Sep 2024 13:34:34 +0200 Subject: [PATCH 06/10] [IMP] auth_oidc: parametrize KEYCLOAK_URL in tests --- auth_oidc/tests/test_auth_oidc_auth_code.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/auth_oidc/tests/test_auth_oidc_auth_code.py b/auth_oidc/tests/test_auth_oidc_auth_code.py index 6e9054c884..501034ba45 100644 --- a/auth_oidc/tests/test_auth_oidc_auth_code.py +++ b/auth_oidc/tests/test_auth_oidc_auth_code.py @@ -21,6 +21,7 @@ from ..controllers.main import OpenIDLogin BASE_URL = "http://localhost:%s" % odoo.tools.config["http_port"] +KEYCLOAK_URL = "http://localhost:8080" @contextlib.contextmanager @@ -110,7 +111,7 @@ def _prepare_login_test_responses( id_token_headers = {"kid": "the_key_id"} responses.add( responses.POST, - "http://localhost:8080/auth/realms/master/protocol/openid-connect/token", + KEYCLOAK_URL + "/auth/realms/master/protocol/openid-connect/token", json={ "access_token": access_token, "id_token": jwt.encode( @@ -128,7 +129,7 @@ def _prepare_login_test_responses( keys = [{"keys": [self.rsa_key_public_pem]}] responses.add( responses.GET, - "http://localhost:8080/auth/realms/master/protocol/openid-connect/certs", + KEYCLOAK_URL + "/auth/realms/master/protocol/openid-connect/certs", json={"keys": keys}, ) From ce922a23870b7ce2fd7d8c13361f47b186c85599 Mon Sep 17 00:00:00 2001 From: Didier 'OdyX' Raboud Date: Wed, 11 Sep 2024 15:25:22 +0200 Subject: [PATCH 07/10] [IMP] auth_oidc: clarify test comments --- auth_oidc/tests/test_auth_oidc_auth_code.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/auth_oidc/tests/test_auth_oidc_auth_code.py b/auth_oidc/tests/test_auth_oidc_auth_code.py index 501034ba45..4ce5520575 100644 --- a/auth_oidc/tests/test_auth_oidc_auth_code.py +++ b/auth_oidc/tests/test_auth_oidc_auth_code.py @@ -70,7 +70,7 @@ def _generate_key(): def setUp(self): super().setUp() - # search our test provider and bind the demo user to it + # search our only test provider self.provider_rec = self.env["auth.oauth.provider"].search( [("client_id", "=", "auth_oidc-test")] ) @@ -98,6 +98,7 @@ def test_auth_link(self): self.assertEqual(params["redirect_uri"], [BASE_URL + "/auth_oauth/signin"]) def _prepare_login_test_user(self): + # bind the demo user to our test provider it user = self.env.ref("base.user_demo") user.write({"oauth_provider_id": self.provider_rec.id, "oauth_uid": user.login}) return user From d87de45368d7c4cf0590bab87aa0afe2ad2ff8ce Mon Sep 17 00:00:00 2001 From: Didier 'OdyX' Raboud Date: Wed, 11 Sep 2024 14:26:33 +0200 Subject: [PATCH 08/10] [IMP] auth_oidc: test possible token values more extensively --- auth_oidc/tests/test_auth_oidc_auth_code.py | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/auth_oidc/tests/test_auth_oidc_auth_code.py b/auth_oidc/tests/test_auth_oidc_auth_code.py index 4ce5520575..72e58b0268 100644 --- a/auth_oidc/tests/test_auth_oidc_auth_code.py +++ b/auth_oidc/tests/test_auth_oidc_auth_code.py @@ -311,8 +311,23 @@ def test_login_with_jwk_format(self): self.assertEqual(token, "122/3") self.assertEqual(login, user.login) - def test_group_expression(self): - """Test that group expressions evaluate correctly""" + def test_group_expression_empty_token(self): + """Test that group expression with an empty token evaluate correctly""" group_line = self.env.ref("auth_oidc.local_keycloak").group_line_ids[:1] group_line.expression = 'token["test"]["test"] == 1' self.assertFalse(group_line._eval_expression(self.env.user, {})) + + def test_group_expressions_with_token(self): + """Test that group expression with token with groups evaluate correctly""" + group_line = self.env.ref("auth_oidc.local_keycloak").group_line_ids[:1] + + group_line.expression = "'group-a' in token['groups']" + self.assertFalse(group_line._eval_expression(self.env.user, {})) + self.assertTrue( + group_line._eval_expression( + self.env.user, {"groups": ["group-a", "group-b"]} + ) + ) + self.assertFalse( + group_line._eval_expression(self.env.user, {"groups": ["group-c"]}) + ) From fbf665f5396027f218801c4cace47a84244629a0 Mon Sep 17 00:00:00 2001 From: Didier 'OdyX' Raboud Date: Wed, 11 Sep 2024 14:34:36 +0200 Subject: [PATCH 09/10] [IMP] auth_oidc: test all possible safe_eval exceptions --- auth_oidc/models/auth_oauth_provider.py | 8 +++++-- auth_oidc/tests/test_auth_oidc_auth_code.py | 25 ++++++++++++++++++++- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/auth_oidc/models/auth_oauth_provider.py b/auth_oidc/models/auth_oauth_provider.py index 582e7c0513..338f21a22d 100644 --- a/auth_oidc/models/auth_oauth_provider.py +++ b/auth_oidc/models/auth_oauth_provider.py @@ -125,8 +125,12 @@ def _check_expression(self): for this in self: try: this._eval_expression(self.env.user, {}) - except (AttributeError, KeyError, NameError) as e: - raise exceptions.ValidationError("\n".join(e.args)) from e + except (AttributeError, KeyError, NameError, ValueError) as e: + # AttributeError: user object can be accessed via attributes: user.email + # KeyError: token is a dict of dicts + # NameError: only user and token can be used + # ValueError: for inexistant variables or attributes + raise exceptions.ValidationError(e) from e def _eval_expression(self, user, token): self.ensure_one() diff --git a/auth_oidc/tests/test_auth_oidc_auth_code.py b/auth_oidc/tests/test_auth_oidc_auth_code.py index 72e58b0268..fbfdf87e06 100644 --- a/auth_oidc/tests/test_auth_oidc_auth_code.py +++ b/auth_oidc/tests/test_auth_oidc_auth_code.py @@ -13,7 +13,7 @@ from jose.utils import long_to_base64 import odoo -from odoo.exceptions import AccessDenied +from odoo.exceptions import AccessDenied, ValidationError from odoo.tests import common from odoo.addons.website.tools import MockRequest as _MockRequest @@ -331,3 +331,26 @@ def test_group_expressions_with_token(self): self.assertFalse( group_line._eval_expression(self.env.user, {"groups": ["group-c"]}) ) + + def test_group_expression_with_inexistant_variable(self): + """Test that group expression with inexistant variable fails""" + group_line = self.env.ref("auth_oidc.local_keycloak").group_line_ids[:1] + + with self.assertRaises(ValidationError): + group_line.expression = "inexistant_variable" + + def test_group_expression_with_inexistant_attribute(self): + """Test that group expression with inexistant attribute (on user) fails""" + group_line = self.env.ref("auth_oidc.local_keycloak").group_line_ids[:1] + + with self.assertRaises(ValidationError): + group_line.expression = "user.not_an_attribute" + + def test_realistic_group_expression(self): + """Test that group expression with inexistant attribute (on user) fails""" + group_line = self.env.ref("auth_oidc.local_keycloak").group_line_ids[:1] + + group_line.expression = "user.email == token['mail']" + self.assertTrue( + group_line._eval_expression(self.env.user, {"mail": self.env.user.email}) + ) From 7587cf096b24f4f5e8598071eff9ea764dd4f58a Mon Sep 17 00:00:00 2001 From: Didier 'OdyX' Raboud Date: Fri, 13 Sep 2024 17:11:27 +0200 Subject: [PATCH 10/10] [IMP] auth_oidc: test group assignment/removal --- auth_oidc/demo/local_keycloak.xml | 13 ++++++- auth_oidc/tests/test_auth_oidc_auth_code.py | 39 +++++++++++++++++++++ 2 files changed, 51 insertions(+), 1 deletion(-) diff --git a/auth_oidc/demo/local_keycloak.xml b/auth_oidc/demo/local_keycloak.xml index 92588dc952..8a2f97a404 100644 --- a/auth_oidc/demo/local_keycloak.xml +++ b/auth_oidc/demo/local_keycloak.xml @@ -17,9 +17,20 @@ name="jwks_uri" >http://localhost:8080/auth/realms/master/protocol/openid-connect/certs - + token['name'] == 'test' + + + + 'erp_manager' in token['groups'] + diff --git a/auth_oidc/tests/test_auth_oidc_auth_code.py b/auth_oidc/tests/test_auth_oidc_auth_code.py index fbfdf87e06..8115898cc5 100644 --- a/auth_oidc/tests/test_auth_oidc_auth_code.py +++ b/auth_oidc/tests/test_auth_oidc_auth_code.py @@ -14,6 +14,7 @@ import odoo from odoo.exceptions import AccessDenied, ValidationError +from odoo.fields import Command from odoo.tests import common from odoo.addons.website.tools import MockRequest as _MockRequest @@ -149,6 +150,44 @@ def test_login(self): self.assertEqual(token, "42") self.assertEqual(login, user.login) + @responses.activate + def test_manager_login(self): + """Test that login works and assigns the user to a manager group""" + user = self._prepare_login_test_user() + self._prepare_login_test_responses( + id_token_body={"user_id": user.login, "groups": ["erp_manager"]} + ) + + params = {"state": json.dumps({})} + with MockRequest(self.env): + db, login, token = self.env["res.users"].auth_oauth( + self.provider_rec.id, + params, + ) + self.assertTrue(user.has_group("base.group_erp_manager")) + + @responses.activate + def test_ex_manager_login(self): + """Test that login works and de-assigns the user from a manager group""" + user = self._prepare_login_test_user() + # Make them a manager + user.write( + {"groups_id": [Command.link(self.env.ref("base.group_erp_manager").id)]} + ) + self.assertTrue(user.has_group("base.group_erp_manager")) + + self._prepare_login_test_responses( + id_token_body={"user_id": user.login, "groups": ["not_erp_manager"]} + ) + + params = {"state": json.dumps({})} + with MockRequest(self.env): + db, login, token = self.env["res.users"].auth_oauth( + self.provider_rec.id, + params, + ) + self.assertFalse(user.has_group("base.group_erp_manager")) + @responses.activate def test_login_without_kid(self): """Test that login works when ID Token has no kid in header"""