From 2d159b7ca90802bcc2783fd0d5e8a050984cb823 Mon Sep 17 00:00:00 2001 From: Vincent Hatakeyama Date: Thu, 19 Feb 2026 16:46:30 +0100 Subject: [PATCH] [IMP] auth_saml: add option to display a specific error page when login fails --- auth_saml/__manifest__.py | 3 +- auth_saml/controllers/main.py | 56 +++++++++---- auth_saml/data/ir_config_parameter.xml | 4 + auth_saml/models/res_config_settings.py | 7 ++ auth_saml/readme/CONFIGURE.md | 7 +- .../+optional-split-error-page.feature | 1 + auth_saml/templates/webclient.xml | 43 ++++++++++ auth_saml/tests/test_pysaml.py | 79 +++++++++++++++++-- auth_saml/views/res_config_settings.xml | 36 +++++++-- 9 files changed, 207 insertions(+), 29 deletions(-) create mode 100644 auth_saml/readme/newsfragments/+optional-split-error-page.feature create mode 100644 auth_saml/templates/webclient.xml diff --git a/auth_saml/__manifest__.py b/auth_saml/__manifest__.py index 34be739cf0..95b74ea093 100644 --- a/auth_saml/__manifest__.py +++ b/auth_saml/__manifest__.py @@ -1,5 +1,5 @@ # Copyright (C) 2020 GlodoUK -# Copyright (C) 2010-2016, 2022 XCG Consulting +# Copyright (C) 2010-2016, 2022, 2026 XCG Consulting # License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). { @@ -21,6 +21,7 @@ "data": [ "data/ir_config_parameter.xml", "security/ir.model.access.csv", + "templates/webclient.xml", "views/auth_saml.xml", "views/res_config_settings.xml", "views/res_users.xml", diff --git a/auth_saml/controllers/main.py b/auth_saml/controllers/main.py index 6cdd118cd8..c8f7b36d60 100644 --- a/auth_saml/controllers/main.py +++ b/auth_saml/controllers/main.py @@ -7,6 +7,7 @@ import logging import werkzeug.utils +from saml2.validate import ResponseLifetimeExceed from werkzeug.exceptions import BadRequest from werkzeug.urls import url_quote_plus @@ -54,6 +55,16 @@ def wrapper(self, **kw): return wrapper +def _error_message(error: str) -> str | None: + if error == "access-denied": + return _("Access Denied") + if error == "response-lifetime-exceed": + return _("Response Lifetime Exceeded") + if error == "expired": + return _("You do not have access to this database. Please contact support.") + return None + + # ---------------------------------------------------------- # Controller # ---------------------------------------------------------- @@ -131,19 +142,7 @@ def web_login(self, *args, **kw): response = super().web_login(*args, **kw) if response.is_qweb: - error = request.params.get("saml_error") - if error == "no-signup": - error = _("Sign up is not allowed on this database.") - elif error == "access-denied": - error = _("Access Denied") - elif error == "expired": - error = _( - "You do not have access to this database. Please contact" - " support." - ) - else: - error = None - + error = _error_message(request.params.get("saml_error")) response.qcontext["saml_providers"] = providers if error: @@ -173,6 +172,17 @@ def _get_saml_extra_relaystate(self): } return state + def _get_saml_error_url(self, saml_error: str) -> str: + """Return the URL of the SAML error page. + This module provides a configuration option to use another page. + """ + base = ( + request.env["ir.config_parameter"] + .sudo() + .get_param("auth_saml.saml_error_page", "/web/login") + ) + return f"{base}?saml_error={saml_error}" + @http.route("/auth_saml/get_auth_request", type="http", auth="none") def get_auth_request(self, pid): provider_id = int(pid) @@ -251,15 +261,17 @@ def signin(self, **kw): # user could be on a temporary session _logger.info("SAML2: access denied") - url = "/web/login?saml_error=expired" - redirect = werkzeug.utils.redirect(url, 303) + redirect = werkzeug.utils.redirect(self._get_saml_error_url("expired"), 303) redirect.autocorrect_location_header = False return redirect + except ResponseLifetimeExceed as e: + _logger.debug("Response Lifetime Exceed - %s", str(e)) + url = self._get_saml_error_url("response-lifetime-exceed") except Exception as e: # signup error _logger.exception("SAML2: failure - %s", str(e)) - url = "/web/login?saml_error=access-denied" + url = self._get_saml_error_url("access-denied") redirect = request.redirect(url, 303) redirect.autocorrect_location_header = False @@ -291,3 +303,15 @@ def saml_metadata(self, **kw): ), [("Content-Type", "text/xml")], ) + + @http.route("/web/login/saml_error", type="http", auth="none", csrf=False) + def saml_error(self, redirect=None, **kw): + saml_error = request.params.get("saml_error") + error = _error_message(saml_error) + if not error: + return request.redirect(redirect or "/") + response = request.render("auth_saml.login_error", {"error": error}) + response.headers["Cache-Control"] = "no-cache" + response.headers["X-Frame-Options"] = "SAMEORIGIN" + response.headers["Content-Security-Policy"] = "frame-ancestors 'self'" + return response diff --git a/auth_saml/data/ir_config_parameter.xml b/auth_saml/data/ir_config_parameter.xml index c65089f451..610586e54e 100644 --- a/auth_saml/data/ir_config_parameter.xml +++ b/auth_saml/data/ir_config_parameter.xml @@ -4,4 +4,8 @@ auth_saml.allow_saml_uid_and_internal_password True + + auth_saml.saml_error_page + /web/login + diff --git a/auth_saml/models/res_config_settings.py b/auth_saml/models/res_config_settings.py index 36b78d959e..fce49dd5f9 100644 --- a/auth_saml/models/res_config_settings.py +++ b/auth_saml/models/res_config_settings.py @@ -9,7 +9,14 @@ class ResConfigSettings(models.TransientModel): _inherit = "res.config.settings" + module_auth_saml = fields.Boolean("SAML Authentication") allow_saml_uid_and_internal_password = fields.Boolean( "Allow SAML users to possess an Odoo password (warning: decreases security)", config_parameter=ALLOW_SAML_UID_AND_PASSWORD, ) + saml_error_page = fields.Selection( + [("/web/login", "Login Page"), ("/web/login/saml_error", "Error Page")], + "SAML Error Page", + config_parameter="auth_saml.saml_error_page", + required=True, + ) diff --git a/auth_saml/readme/CONFIGURE.md b/auth_saml/readme/CONFIGURE.md index 68072d142c..9804619a5a 100644 --- a/auth_saml/readme/CONFIGURE.md +++ b/auth_saml/readme/CONFIGURE.md @@ -15,6 +15,11 @@ automatic redirection in the provider settings. The autoredirection will only be done on the active provider with the highest priority. It is still possible to access the login without redirection by using the query parameter `disable_autoredirect`, as in -`https://example.com/web/login?disable_autoredirect=` The login is also +`https://example.com/web/login?disable_autoredirect=`. + +The login is also displayed if there is an error with SAML login, in order to display any error message. +There is an option to use a page that only displays the error message +in a separate page instead of the login form. +This is useful when displaying the login form is not desired. diff --git a/auth_saml/readme/newsfragments/+optional-split-error-page.feature b/auth_saml/readme/newsfragments/+optional-split-error-page.feature new file mode 100644 index 0000000000..89f5bf8ceb --- /dev/null +++ b/auth_saml/readme/newsfragments/+optional-split-error-page.feature @@ -0,0 +1 @@ +Add an option to use another page for displaying SAML error. diff --git a/auth_saml/templates/webclient.xml b/auth_saml/templates/webclient.xml new file mode 100644 index 0000000000..fbeee3678c --- /dev/null +++ b/auth_saml/templates/webclient.xml @@ -0,0 +1,43 @@ + + + + diff --git a/auth_saml/tests/test_pysaml.py b/auth_saml/tests/test_pysaml.py index 78d87b0b20..7cfc6cbbfc 100644 --- a/auth_saml/tests/test_pysaml.py +++ b/auth_saml/tests/test_pysaml.py @@ -2,11 +2,14 @@ import base64 import html import os -import urllib from unittest.mock import patch +from urllib.parse import urlencode, urljoin, urlparse + +from saml2.validate import ResponseLifetimeExceed 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 @@ -123,10 +126,8 @@ def test__compute_sp_metadata_url__provider_has_sp_baseurl(self): temp = self.saml_provider.sp_baseurl self.saml_provider.sp_baseurl = "http://example.com" self.saml_provider._compute_sp_metadata_url() - expected_qs = urllib.parse.urlencode( - {"p": self.saml_provider.id, "d": self.env.cr.dbname} - ) - expected_url = urllib.parse.urljoin( + expected_qs = urlencode({"p": self.saml_provider.id, "d": self.env.cr.dbname}) + expected_url = urljoin( "http://example.com", ("/auth_saml/metadata?%s" % expected_qs) ) # Assert that sp_metadata_url is set correctly @@ -412,6 +413,74 @@ def test_redirect_after_login(self): + "/web#action=37&model=ir.module.module&view_type=kanban&menu_id=5", ) + def test_auth_errors(self): + self.add_provider_to_user() + + auth_request = self.saml_provider._get_auth_request() + response = self.idp.fake_login(auth_request) + unpacked_response = response._unpack() + + for key in unpacked_response: + unpacked_response[key] = html.unescape(unpacked_response[key]) + self.user.active = False + with mute_logger("odoo.addons.auth_saml.controllers"): + response = self.url_open( + "/auth_saml/signin", + data=unpacked_response, + allow_redirects=True, + ) + self.assertEqual( + response.url, f"{self.base_url()}/web/login?saml_error=expired" + ) + self.user.active = True + # invalidate saml response + unpacked_response["SAMLResponse"] = "" + with mute_logger("odoo.addons.auth_saml.controllers"): + response = self.url_open( + "/auth_saml/signin", + data=unpacked_response, + allow_redirects=True, + ) + self.assertEqual( + response.url, f"{self.base_url()}/web/login?saml_error=access-denied" + ) + self.browse_ref( + "auth_saml.saml_error_page_config_parameter" + ).value = "/web/login/saml_error" + with mute_logger("odoo.addons.auth_saml.controllers"): + response = self.url_open( + "/auth_saml/signin", + data=unpacked_response, + allow_redirects=True, + ) + self.assertEqual( + response.url, + f"{self.base_url()}/web/login/saml_error?saml_error=access-denied", + ) + + # Not an error easy to reproduce so use a patch to raise it. + def auth_saml(*args, **kwargs): + raise ResponseLifetimeExceed() + + with mute_logger("odoo.addons.auth_saml.controllers"), patch( + "odoo.addons.auth_saml.models.res_users.ResUser.auth_saml", auth_saml + ): + response = self.url_open( + "/auth_saml/signin", + data=unpacked_response, + allow_redirects=True, + ) + self.assertEqual( + response.url, + f"{self.base_url()}/web/login/saml_error?saml_error=response-lifetime-exceed", + ) + + def test_saml_error_page_redirect(self): + """Test that accessing the page without an error redirects to root page.""" + response = self.url_open("/web/login/saml_error") + path = urlparse(response.url).path + self.assertEqual(path, "/web/login") + def test_disallow_user_password_when_changing_settings(self): """Test that disabling the setting will remove passwords from related users""" # We activate the settings to allow password login diff --git a/auth_saml/views/res_config_settings.xml b/auth_saml/views/res_config_settings.xml index 7e27733c27..d86845bebb 100644 --- a/auth_saml/views/res_config_settings.xml +++ b/auth_saml/views/res_config_settings.xml @@ -7,12 +7,36 @@ - - + + +
+
+ + +
+
+
+
+
+