From bcc8c51ce1ce23d1051aa8e859820b8cce90daea Mon Sep 17 00:00:00 2001 From: Vincent Hatakeyama Date: Fri, 24 Feb 2023 11:02:06 +0100 Subject: [PATCH] [ADD] auth_saml: handle redirect parameter in the URI --- auth_saml/controllers/main.py | 32 +++++++++--- auth_saml/models/auth_saml_provider.py | 6 +-- auth_saml/readme/HISTORY.rst | 5 ++ auth_saml/tests/test_pysaml.py | 67 +++++++++++++++++++++++++- auth_saml/views/auth_saml.xml | 2 +- 5 files changed, 100 insertions(+), 12 deletions(-) diff --git a/auth_saml/controllers/main.py b/auth_saml/controllers/main.py index e94fb79af1..5f7721a348 100644 --- a/auth_saml/controllers/main.py +++ b/auth_saml/controllers/main.py @@ -1,5 +1,5 @@ # Copyright (C) 2020 GlodoUK -# Copyright (C) 2010-2016, 2022 XCG Consulting +# Copyright (C) 2010-2016, 2022-2023 XCG Consulting # License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). import functools @@ -7,6 +7,7 @@ import logging import werkzeug.utils +from werkzeug.exceptions import BadRequest from werkzeug.urls import url_quote_plus import odoo @@ -53,7 +54,7 @@ def wrapper(self, req, **kw): class SAMLLogin(Home): - def _list_saml_providers_domain(self): + def _list_saml_providers_domain(self): # pylint: disable=no-self-use return [] def list_saml_providers(self, with_autoredirect: bool = False) -> models.Model: @@ -66,7 +67,8 @@ def list_saml_providers(self, with_autoredirect: bool = False) -> models.Model: if with_autoredirect: domain.append(("autoredirect", "=", True)) providers = request.env["auth.saml.provider"].sudo().search_read(domain) - + for provider in providers: + provider["auth_link"] = self._auth_saml_request_link(provider) return providers def _saml_autoredirect(self): @@ -78,11 +80,21 @@ def _saml_autoredirect(self): ) if autoredirect_providers and not disable_autoredirect: return werkzeug.utils.redirect( - "/auth_saml/get_auth_request?pid=%d" % autoredirect_providers[0]["id"], + self._auth_saml_request_link(autoredirect_providers[0]), 303, ) return None + def _auth_saml_request_link(self, provider: models.Model): + """Return the auth request link for the provided provider""" + params = { + "pid": provider["id"], + } + redirect = request.params.get("redirect") + if redirect: + params["redirect"] = redirect + return "/auth_saml/get_auth_request?%s" % werkzeug.urls.url_encode(params) + @http.route() def web_client(self, s_action=None, **kw): ensure_db() @@ -143,7 +155,6 @@ def _get_saml_extra_relaystate(self): The provider will automatically set things like the dbname, provider id, etc. """ - redirect = request.params.get("redirect") or "web" if not redirect.startswith(("//", "http://", "https://")): redirect = "{}{}".format( @@ -198,6 +209,8 @@ def signin(self, req, **kw): state = json.loads(kw["RelayState"]) provider = state["p"] dbname = state["d"] + if not http.db_filter([dbname]): + return BadRequest() context = state.get("c", {}) registry = registry_get(dbname) @@ -215,8 +228,15 @@ def signin(self, req, **kw): ) action = state.get("a") menu = state.get("m") + redirect = ( + werkzeug.urls.url_unquote_plus(state["r"]) + if state.get("r") + else False + ) url = "/" - if action: + if redirect: + url = redirect + elif action: url = "/#action=%s" % action elif menu: url = "/#menu_id=%s" % menu diff --git a/auth_saml/models/auth_saml_provider.py b/auth_saml/models/auth_saml_provider.py index 2b9ebdb363..ccd36b1e92 100644 --- a/auth_saml/models/auth_saml_provider.py +++ b/auth_saml/models/auth_saml_provider.py @@ -191,7 +191,7 @@ def _get_cert_key_path(self, field="sp_pem_public"): return keys_path - def _get_config_for_provider(self, base_url: str = None): + def _get_config_for_provider(self, base_url: str = None) -> Saml2Config: """ Internal helper to get a configured Saml2Client """ @@ -237,7 +237,7 @@ def _get_config_for_provider(self, base_url: str = None): sp_config.allow_unknown_attributes = True return sp_config - def _get_client_for_provider(self, base_url: str = None): + def _get_client_for_provider(self, base_url: str = None) -> Saml2Client: sp_config = self._get_config_for_provider(base_url) saml_client = Saml2Client(config=sp_config) return saml_client @@ -336,7 +336,7 @@ def _store_outstanding_request(self, reqid): {"saml_provider_id": self.id, "saml_request_id": reqid} ) - def _metadata_string(self, valid=None, base_url=None): + def _metadata_string(self, valid=None, base_url: str = None): self.ensure_one() sp_config = self._get_config_for_provider(base_url) diff --git a/auth_saml/readme/HISTORY.rst b/auth_saml/readme/HISTORY.rst index 75609f8a1e..c44c9887c6 100644 --- a/auth_saml/readme/HISTORY.rst +++ b/auth_saml/readme/HISTORY.rst @@ -1,3 +1,8 @@ +15.0.1.2.0 +~~~~~~~~~~ + +Handle redirect after authentification. + 15.0.1.1.0 ~~~~~~~~~~ diff --git a/auth_saml/tests/test_pysaml.py b/auth_saml/tests/test_pysaml.py index 50652ac503..c05235b747 100644 --- a/auth_saml/tests/test_pysaml.py +++ b/auth_saml/tests/test_pysaml.py @@ -1,5 +1,8 @@ +# License AGPL-3.0 or later (http://www.gnu.org/licenses/agpl). import base64 +import html import os +from unittest.mock import patch from odoo.exceptions import AccessDenied, UserError, ValidationError from odoo.tests import HttpCase, tagged @@ -85,6 +88,21 @@ def test_ensure_provider_appears_on_login(self): self.assertIn("Login with Authentic", response.text) self.assertIn(self.url_saml_request, response.text) + def test_ensure_provider_appears_on_login_with_redirect_param(self): + """Test that SAML provider is listed in the login page keeping the redirect""" + response = self.url_open( + "/web/login?redirect=%2Fweb%23action%3D37%26model%3Dir.module.module%26view" + "_type%3Dkanban%26menu_id%3D5" + ) + self.assertIn("Login with Authentic", response.text) + self.assertIn( + "/auth_saml/get_auth_request?pid={}&redirect=%2Fweb%23action%3D37%26mod" + "el%3Dir.module.module%26view_type%3Dkanban%26menu_id%3D5".format( + self.saml_provider.id + ), + response.text, + ) + def test_ensure_metadata_present(self): response = self.url_open( "/auth_saml/metadata?p=%d&d=%s" @@ -96,7 +114,7 @@ def test_ensure_metadata_present(self): def test_ensure_get_auth_request_redirects(self): response = self.url_open( - "/auth_saml/get_auth_request?pid=%d" % (self.saml_provider.id), + "/auth_saml/get_auth_request?pid=%d" % self.saml_provider.id, allow_redirects=False, ) self.assertTrue(response.ok) @@ -160,7 +178,7 @@ def test_login_with_saml(self): self.assertEqual(200, response.status_code) unpacked_response = response._unpack() - (_database, login, token) = ( + (database, login, token) = ( self.env["res.users"] .sudo() .auth_saml( @@ -168,6 +186,7 @@ def test_login_with_saml(self): ) ) + self.assertEqual(database, self.env.cr.dbname) self.assertEqual(login, self.user.login) # We should not be able to log in with the wrong token @@ -273,3 +292,47 @@ def test_disallow_user_admin_can_have_password(self): ).value = "False" # Test base.user_admin exception self.env.ref("base.user_admin").password = "nNRST4j*->sEatNGg._!" + + def test_db_filtering(self): + # change filter to only allow our db. + with patch("odoo.http.db_filter", new=lambda *args, **kwargs: []): + self.add_provider_to_user() + + redirect_url = self.saml_provider._get_auth_request() + response = self.idp.fake_login(redirect_url) + unpacked_response = response._unpack() + + for key in unpacked_response: + unpacked_response[key] = html.unescape(unpacked_response[key]) + response = self.url_open("/auth_saml/signin", data=unpacked_response) + self.assertFalse(response.ok) + self.assertIn(response.status_code, [400, 404]) + + def test_redirect_after_login(self): + """Test that providing a redirect will be kept after SAML login.""" + self.add_provider_to_user() + + redirect_url = self.saml_provider._get_auth_request( + { + "r": "%2Fweb%23action%3D37%26model%3Dir.module.module%26view_type%3Dkan" + "ban%26menu_id%3D5" + } + ) + response = self.idp.fake_login(redirect_url) + unpacked_response = response._unpack() + + for key in unpacked_response: + unpacked_response[key] = html.unescape(unpacked_response[key]) + response = self.url_open( + "/auth_saml/signin", + data=unpacked_response, + allow_redirects=True, + timeout=300, + ) + self.assertTrue(response.ok) + self.assertEqual(response.status_code, 200) + self.assertEqual( + response.url, + self.base_url() + + "/web#action=37&model=ir.module.module&view_type=kanban&menu_id=5", + ) diff --git a/auth_saml/views/auth_saml.xml b/auth_saml/views/auth_saml.xml index e0f4764a58..ab0187f098 100644 --- a/auth_saml/views/auth_saml.xml +++ b/auth_saml/views/auth_saml.xml @@ -11,7 +11,7 @@ t-foreach="saml_providers" t-as="p" class="list-group-item list-group-item-action py-2" - t-att-href="'/auth_saml/get_auth_request?pid=%s'%p['id']" + t-att-href="p['auth_link']" >