From 7ff1274efa62690050a5f391475f715f036e9a23 Mon Sep 17 00:00:00 2001 From: Vincent Hatakeyama Date: Tue, 1 Oct 2024 13:52:56 +0200 Subject: [PATCH] [IMP] auth_saml: user provisioning on login - custom message when response is too old - avoid using werkzeug.urls method, they are deprecated - add missing ondelete cascade when user is deleted - attribute mapping is now also duplicated when the provider is duplicated - factorize getting SAML attribute value, allowing using subject.nameId in mapping attributes too --- auth_saml/controllers/main.py | 17 ++- .../models/auth_saml_attribute_mapping.py | 1 + auth_saml/models/auth_saml_provider.py | 105 ++++++++++++------ auth_saml/models/res_users.py | 45 +++++++- auth_saml/models/res_users_saml.py | 4 +- auth_saml/readme/CONFIGURE.md | 3 +- auth_saml/readme/HISTORY.md | 13 ++- auth_saml/tests/fake_idp.py | 13 ++- auth_saml/tests/test_pysaml.py | 70 ++++++++++-- auth_saml/views/auth_saml.xml | 10 ++ 10 files changed, 219 insertions(+), 62 deletions(-) diff --git a/auth_saml/controllers/main.py b/auth_saml/controllers/main.py index fb635d3a72..54a7d0c29f 100644 --- a/auth_saml/controllers/main.py +++ b/auth_saml/controllers/main.py @@ -5,10 +5,11 @@ import functools import json import logging +from urllib.parse import quote_plus, unquote_plus, urlencode import werkzeug.utils +from saml2.validate import ResponseLifetimeExceed from werkzeug.exceptions import BadRequest -from werkzeug.urls import url_quote_plus from odoo import ( SUPERUSER_ID, @@ -100,7 +101,7 @@ def _auth_saml_request_link(self, provider: models.Model): redirect = request.params.get("redirect") if redirect: params["redirect"] = redirect - return "/auth_saml/get_auth_request?%s" % werkzeug.urls.url_encode(params) + return "/auth_saml/get_auth_request?%s" % urlencode(params) @http.route() def web_client(self, s_action=None, **kw): @@ -132,10 +133,13 @@ def web_login(self, *args, **kw): response = super().web_login(*args, **kw) if response.is_qweb: error = request.params.get("saml_error") + # TODO c’est par là qu’il faut changer des trucs if error == "no-signup": error = _("Sign up is not allowed on this database.") elif error == "access-denied": error = _("Access Denied") + elif error == "response-lifetime-exceed": + error = _("Response Lifetime Exceeded") elif error == "expired": error = _( "You do not have access to this database. Please contact" @@ -169,7 +173,7 @@ def _get_saml_extra_relaystate(self): ) state = { - "r": url_quote_plus(redirect), + "r": quote_plus(redirect), } return state @@ -231,9 +235,7 @@ def signin(self, **kw): ) action = state.get("a") menu = state.get("m") - redirect = ( - werkzeug.urls.url_unquote_plus(state["r"]) if state.get("r") else False - ) + redirect = unquote_plus(state["r"]) if state.get("r") else False url = "/web" if redirect: url = redirect @@ -255,6 +257,9 @@ def signin(self, **kw): redirect = werkzeug.utils.redirect(url, 303) redirect.autocorrect_location_header = False return redirect + except ResponseLifetimeExceed as e: + _logger.debug("Response Lifetime Exceed - %s", str(e)) + url = "/web/login?saml_error=response-lifetime-exceed" except Exception as e: # signup error diff --git a/auth_saml/models/auth_saml_attribute_mapping.py b/auth_saml/models/auth_saml_attribute_mapping.py index 6fb6190538..ec9537b6b2 100644 --- a/auth_saml/models/auth_saml_attribute_mapping.py +++ b/auth_saml/models/auth_saml_attribute_mapping.py @@ -13,6 +13,7 @@ class AuthSamlAttributeMapping(models.Model): "auth.saml.provider", index=True, required=True, + ondelete="cascade", ) attribute_name = fields.Char( string="IDP Response Attribute", diff --git a/auth_saml/models/auth_saml_provider.py b/auth_saml/models/auth_saml_provider.py index 4b323b7c26..bf82edd42a 100644 --- a/auth_saml/models/auth_saml_provider.py +++ b/auth_saml/models/auth_saml_provider.py @@ -81,6 +81,7 @@ class AuthSamlProvider(models.Model): "auth.saml.attribute.mapping", "provider_id", string="Attribute Mapping", + copy=True, ) active = fields.Boolean(default=True) sequence = fields.Integer(index=True) @@ -136,6 +137,20 @@ class AuthSamlProvider(models.Model): default=True, help="Whether metadata should be signed or not", ) + # User creation fields + create_user = fields.Boolean( + default=False, + help="Create user if not found. The login and name will defaults to the SAML " + "user matching attribute. Use the mapping attributes to change the value " + "used.", + ) + create_user_template_id = fields.Many2one( + comodel_name="res.users", + # Template users, like base.default_user, are disabled by default so allow them + domain="[('active', 'in', (True, False))]", + default=lambda self: self.env.ref("base.default_user"), + help="When creating user, this user is used as a template", + ) @api.model def _sig_alg_selection(self): @@ -256,9 +271,7 @@ def _get_auth_request(self, extra_state=None, url_root=None): } state.update(extra_state) - sig_alg = ds.SIG_RSA_SHA1 - if self.sig_alg: - sig_alg = getattr(ds, self.sig_alg) + sig_alg = getattr(ds, self.sig_alg) saml_client = self._get_client_for_provider(url_root) reqid, info = saml_client.prepare_for_authenticate( @@ -272,6 +285,7 @@ def _get_auth_request(self, extra_state=None, url_root=None): for key, value in info["headers"]: if key == "Location": redirect_url = value + break self._store_outstanding_request(reqid) @@ -287,27 +301,15 @@ def _validate_auth_response(self, token: str, base_url: str = None): saml2.entity.BINDING_HTTP_POST, self._get_outstanding_requests_dict(), ) - matching_value = None - - if self.matching_attribute == "subject.nameId": - matching_value = response.name_id.text - else: - attrs = response.get_identity() - - for k, v in attrs.items(): - if k == self.matching_attribute: - matching_value = v - break - - if not matching_value: - raise Exception( - f"Matching attribute {self.matching_attribute} not found " - f"in user attrs: {attrs}" - ) - - if matching_value and isinstance(matching_value, list): - matching_value = next(iter(matching_value), None) - + try: + matching_value = self._get_attribute_value( + response, self.matching_attribute + ) + except KeyError: + raise Exception( + f"Matching attribute {self.matching_attribute} not found " + f"in user attrs: {response.get_identity()}" + ) from None if isinstance(matching_value, str) and self.matching_attribute_to_lower: matching_value = matching_value.lower() @@ -349,24 +351,59 @@ def _metadata_string(self, valid=None, base_url: str = None): sign=self.sign_metadata, ) + @staticmethod + def _get_attribute_value(response, attribute_name: str): + """ + + :raise: KeyError if attribute is not in the response + :param response: + :param attribute_name: + :return: value of the attribut. if the value is an empty list, return None + otherwise return the first element of the list + """ + if attribute_name == "subject.nameId": + return response.name_id.text + attrs = response.get_identity() + attribute_value = attrs[attribute_name] + if isinstance(attribute_value, list): + attribute_value = next(iter(attribute_value), None) + return attribute_value + def _hook_validate_auth_response(self, response, matching_value): self.ensure_one() vals = {} - attrs = response.get_identity() for attribute in self.attribute_mapping_ids: - if attribute.attribute_name not in attrs: - _logger.debug( + try: + vals[attribute.field_name] = self._get_attribute_value( + response, attribute.attribute_name + ) + except KeyError: + _logger.warning( "SAML attribute '%s' found in response %s", attribute.attribute_name, - attrs, + response.get_identity(), ) - continue - attribute_value = attrs[attribute.attribute_name] - if isinstance(attribute_value, list): - attribute_value = attribute_value[0] + return {"mapped_attrs": vals} - vals[attribute.field_name] = attribute_value + def _user_copy_defaults(self, validation): + """ + Returns defaults when copying the template user. - return {"mapped_attrs": vals} + Can be overridden with extra information. + :param validation: validation result + :return: a dictionary for copying template user, empty to avoid copying + """ + self.ensure_one() + if not self.create_user: + return {} + saml_uid = validation["user_id"] + return { + "name": saml_uid, + "login": saml_uid, + "active": True, + # if signature is not provided by mapped_attrs, it will be computed + # due to call to compute method in calling method. + "signature": None, + } | validation.get("mapped_attrs", {}) diff --git a/auth_saml/models/res_users.py b/auth_saml/models/res_users.py index 412b5c6994..68a1733409 100644 --- a/auth_saml/models/res_users.py +++ b/auth_saml/models/res_users.py @@ -7,7 +7,7 @@ import passlib -from odoo import SUPERUSER_ID, _, api, fields, models, registry, tools +from odoo import SUPERUSER_ID, Command, _, api, fields, models, registry, tools from odoo.exceptions import AccessDenied, ValidationError from .ir_config_parameter import ALLOW_SAML_UID_AND_PASSWORD @@ -45,19 +45,52 @@ def _auth_saml_signin(self, provider: int, validation: dict, saml_response) -> s limit=1, ) user = user_saml.user_id - if len(user) != 1: - raise AccessDenied() + user_copy_defaults = {} + if not user: + user_copy_defaults = ( + self.env["auth.saml.provider"] + .browse(provider) + ._user_copy_defaults(validation) + ) + if not user_copy_defaults: + raise AccessDenied() with registry(self.env.cr.dbname).cursor() as new_cr: new_env = api.Environment(new_cr, self.env.uid, self.env.context) + if user_copy_defaults: + new_user = ( + new_env["auth.saml.provider"] + .browse(provider) + .create_user_template_id.with_context(no_reset_password=True) + .copy( + { + **user_copy_defaults, + "saml_ids": [ + Command.create( + { + "saml_provider_id": provider, + "saml_uid": saml_uid, + "saml_access_token": saml_response, + } + ) + ], + } + ) + ) + # Update signature as needed. + new_user._compute_signature() + return new_user.login + # Update the token. Need to be committed, otherwise the token is not visible # to other envs, like the one used in login_and_redirect user_saml.with_env(new_env).write({"saml_access_token": saml_response}) - if validation.get("mapped_attrs", {}): - user.write(validation.get("mapped_attrs", {})) + # if a login is changed by a mapped attribute, it needs to be commited too + user = user.with_env(new_env) + if validation.get("mapped_attrs", {}): + user.write(validation.get("mapped_attrs", {})) - return user.login + return user.login @api.model def auth_saml(self, provider: int, saml_response: str, base_url: str = None): diff --git a/auth_saml/models/res_users_saml.py b/auth_saml/models/res_users_saml.py index d7cbd308d3..a60f493535 100644 --- a/auth_saml/models/res_users_saml.py +++ b/auth_saml/models/res_users_saml.py @@ -7,7 +7,9 @@ class ResUserSaml(models.Model): _name = "res.users.saml" _description = "User to SAML Provider Mapping" - user_id = fields.Many2one("res.users", index=True, required=True) + user_id = fields.Many2one( + "res.users", index=True, required=True, ondelete="cascade" + ) saml_provider_id = fields.Many2one( "auth.saml.provider", string="SAML Provider", index=True ) diff --git a/auth_saml/readme/CONFIGURE.md b/auth_saml/readme/CONFIGURE.md index 68072d142c..5a0f1ea84b 100644 --- a/auth_saml/readme/CONFIGURE.md +++ b/auth_saml/readme/CONFIGURE.md @@ -2,7 +2,8 @@ To use this module, you need an IDP server, properly set up. 1. Configure the module according to your IdP’s instructions (Settings \> Users & Companies \> SAML Providers). -2. Pre-create your users and set the SAML information against the user. +2. Pre-create your users and set the SAML information against the user, + or use the module ability to create users as they log in. By default, the module let users have both a password and SAML ids. To increase security, disable passwords by using the option in Settings. diff --git a/auth_saml/readme/HISTORY.md b/auth_saml/readme/HISTORY.md index 89020f8c3c..0c0341713c 100644 --- a/auth_saml/readme/HISTORY.md +++ b/auth_saml/readme/HISTORY.md @@ -1,3 +1,12 @@ -## 16.0.1.0.0 +## 17.0.1.1.0 -Initial migration for 16.0. +- custom message when response is too old +- avoid using werkzeug.urls method, they are deprecated +- add missing ondelete cascade when user is deleted +- attribute mapping is now also duplicated when the provider is duplicated +- factorize getting SAML attribute value, allowing using subject.nameId in mapping attributes too +- allow creating user if not found by copying a template user + +## 17.0.1.0.0 + +Initial migration for 17.0. diff --git a/auth_saml/tests/fake_idp.py b/auth_saml/tests/fake_idp.py index f2865b403d..6e4c91ef2e 100644 --- a/auth_saml/tests/fake_idp.py +++ b/auth_saml/tests/fake_idp.py @@ -73,13 +73,21 @@ } +class DummyNameId: + """Dummy name id with text value""" + + def __init__(self, text): + self.text = text + + class DummyResponse: - def __init__(self, status, data, headers=None): + def __init__(self, status, data, headers=None, name_id: str = ""): self.status_code = status self.text = data self.headers = headers or [] self.content = data self._identity = {} + self.name_id = DummyNameId(name_id) def _unpack(self, ver="SAMLResponse"): """ @@ -127,6 +135,7 @@ def __init__(self, metadatas=None): config.load(settings) config.allow_unknown_attributes = True Server.__init__(self, config=config) + self.mail = "test@example.com" def get_metadata(self): return create_metadata_string( @@ -163,7 +172,7 @@ def authn_request_endpoint(self, req, binding, relay_state): "surName": "Example", "givenName": "Test", "title": "Ind", - "mail": "test@example.com", + "mail": self.mail, } resp_args.update({"sign_assertion": True, "sign_response": True}) diff --git a/auth_saml/tests/test_pysaml.py b/auth_saml/tests/test_pysaml.py index 9eedaa5405..5033d41e85 100644 --- a/auth_saml/tests/test_pysaml.py +++ b/auth_saml/tests/test_pysaml.py @@ -7,6 +7,7 @@ from odoo.exceptions import AccessDenied, UserError, ValidationError from odoo.tests import HttpCase, tagged +from odoo.tools import mute_logger from .fake_idp import DummyResponse, FakeIDP @@ -101,6 +102,8 @@ def test_ensure_provider_appears_on_login_with_redirect_param(self): def test__onchange_name(self): temp = self.saml_provider.body + r = self.saml_provider._onchange_name() + self.assertEqual(self.saml_provider.body, temp) self.saml_provider.body = "" r = self.saml_provider._onchange_name() self.assertEqual(r, None) @@ -135,14 +138,15 @@ def test__compute_sp_metadata_url__provider_has_sp_baseurl(self): def test__hook_validate_auth_response(self): # Create a fake response with attributes - fake_response = DummyResponse(200, "fake_data") + fake_response = DummyResponse(200, "fake_data", name_id="new.user") fake_response.set_identity( {"email": "new_user@example.com", "first_name": "New", "last_name": "User"} ) # Add attribute mappings to the provider self.saml_provider.attribute_mapping_ids = [ - (0, 0, {"attribute_name": "email", "field_name": "login"}), + (0, 0, {"attribute_name": "email", "field_name": "email"}), + (0, 0, {"attribute_name": "subject.nameId", "field_name": "login"}), (0, 0, {"attribute_name": "first_name", "field_name": "name"}), ( 0, @@ -152,13 +156,15 @@ def test__hook_validate_auth_response(self): ] # Call the method - result = self.saml_provider._hook_validate_auth_response( - fake_response, "test@example.com" - ) + with mute_logger("odoo.addons.auth_saml.models.auth_saml_provider"): + result = self.saml_provider._hook_validate_auth_response( + fake_response, "test@example.com" + ) # Check the result self.assertIn("mapped_attrs", result) - self.assertEqual(result["mapped_attrs"]["login"], "new_user@example.com") + self.assertEqual(result["mapped_attrs"]["email"], "new_user@example.com") + self.assertEqual(result["mapped_attrs"]["login"], "new.user") self.assertEqual(result["mapped_attrs"]["name"], "New") self.assertNotIn("middle_name", result["mapped_attrs"]) @@ -235,7 +241,9 @@ def add_provider_to_user(self): def test_login_with_saml(self): self.add_provider_to_user() + self._login_with_saml() + def _login_with_saml(self): redirect_url = self.saml_provider._get_auth_request() self.assertIn("http://localhost:8000/sso/redirect?SAMLRequest=", redirect_url) @@ -252,7 +260,7 @@ def test_login_with_saml(self): ) self.assertEqual(database, self.env.cr.dbname) - self.assertEqual(login, self.user.login) + self.assertEqual(login, "test@example.com") # We should not be able to log in with the wrong token with self.assertRaises(AccessDenied): @@ -261,6 +269,23 @@ def test_login_with_saml(self): # User should now be able to log in with the token self.authenticate(user="test@example.com", password=token) + def test_login_with_saml_to_lower(self): + self.add_provider_to_user() + self.saml_provider.matching_attribute_to_lower = True + self.idp.mail = "TEST@example.com" + self._login_with_saml() + + def test_login_with_saml_non_existing_mapping_attribute(self): + self.saml_provider.matching_attribute = "nick_name" + self.add_provider_to_user() + with self.assertRaises(Exception): + self._login_with_saml() + + def test_create_user(self): + self.user.unlink() + self.saml_provider.create_user = True + self._login_with_saml() + def test_disallow_user_password_when_changing_ir_config_parameter(self): """Test that disabling users from having both a password and SAML ids remove users password.""" @@ -305,9 +330,10 @@ def test_disallow_user_password_no_password_set(self): """Test that a new user with SAML ids can not have its password set up when the disallow option is set.""" # change the option - self.browse_ref( - "auth_saml.allow_saml_uid_and_internal_password" - ).value = "False" + self.browse_ref("auth_saml.allow_saml_uid_and_internal_password").unlink() + self.env["ir.config_parameter"].set_param( + "auth_saml.allow_saml_uid_and_internal_password", "False" + ) # Create a new user with only SAML ids user = ( self.env["res.users"] @@ -341,12 +367,36 @@ def test_disallow_user_password(self): self.browse_ref( "auth_saml.allow_saml_uid_and_internal_password" ).value = "False" + # Test that existing user password still works if no saml uid is set + self.authenticate(user="test@example.com", password="Lu,ums-7vRU>0i]=YDLa") # Test that existing user password is deleted when adding an SAML provider + self.add_provider_to_user() + with self.assertRaises(AccessDenied): + self.authenticate(user="test@example.com", password="Lu,ums-7vRU>0i]=YDLa") + + def test_disallow_user_password_unlink(self): + """Test that existing user password is deleted when adding an SAML provider when + the disallow option is not present (and defaults to false).""" + # change the option + self.browse_ref("auth_saml.allow_saml_uid_and_internal_password").unlink() + # Test that existing user password still works if no saml uid is set self.authenticate(user="test@example.com", password="Lu,ums-7vRU>0i]=YDLa") + # Test that existing user password is deleted when adding an SAML provider self.add_provider_to_user() with self.assertRaises(AccessDenied): self.authenticate(user="test@example.com", password="Lu,ums-7vRU>0i]=YDLa") + def test_change_unrelated_ir_config_parameter(self): + """Test another branch, creating or writing another ir.config_parameter""" + param = self.env["ir.config_parameter"].create( + [{"key": "test", "value": "unrelated"}] + ) + self.authenticate(user="test@example.com", password="Lu,ums-7vRU>0i]=YDLa") + self.env["ir.config_parameter"].set_param("test", "False") + self.authenticate(user="test@example.com", password="Lu,ums-7vRU>0i]=YDLa") + param.unlink() + self.authenticate(user="test@example.com", password="Lu,ums-7vRU>0i]=YDLa") + def test_disallow_user_admin_can_have_password(self): """Test that admin can have its password set even if the disallow option is set.""" diff --git a/auth_saml/views/auth_saml.xml b/auth_saml/views/auth_saml.xml index 9ee7dc0335..11f0582d8b 100644 --- a/auth_saml/views/auth_saml.xml +++ b/auth_saml/views/auth_saml.xml @@ -182,6 +182,16 @@ + + + +