From 70e000d327a5734f6d0597bdbac15a91d9f39619 Mon Sep 17 00:00:00 2001 From: Jens Langhammer Date: Wed, 21 Jul 2021 23:14:03 +0200 Subject: [PATCH] providers/saml: improve error handling for property mappings Signed-off-by: Jens Langhammer --- .../providers/saml/processors/assertion.py | 13 +++- .../saml/tests/test_auth_n_request.py | 61 +++++++++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/authentik/providers/saml/processors/assertion.py b/authentik/providers/saml/processors/assertion.py index 20e14e1bf..765947ce7 100644 --- a/authentik/providers/saml/processors/assertion.py +++ b/authentik/providers/saml/processors/assertion.py @@ -9,6 +9,7 @@ from lxml.etree import Element, SubElement # nosec from structlog.stdlib import get_logger from authentik.core.exceptions import PropertyMappingExpressionException +from authentik.events.models import Event, EventAction from authentik.lib.utils.time import timedelta_from_string from authentik.providers.saml.models import SAMLPropertyMapping, SAMLProvider from authentik.providers.saml.processors.request_parser import AuthNRequest @@ -99,7 +100,11 @@ class AssertionProcessor: attribute_statement.append(attribute) except PropertyMappingExpressionException as exc: - LOGGER.warning(str(exc)) + Event.new( + EventAction.CONFIGURATION_ERROR, + message=f"Failed to evaluate property-mapping: {str(exc)}", + mapping=mapping, + ).from_http(self.http_request) continue return attribute_statement @@ -161,7 +166,11 @@ class AssertionProcessor: name_id.text = value return name_id except PropertyMappingExpressionException as exc: - LOGGER.warning(str(exc)) + Event.new( + EventAction.CONFIGURATION_ERROR, + message=f"Failed to evaluate property-mapping: {str(exc)}", + mapping=self.provider.name_id_mapping, + ).from_http(self.http_request) return name_id if name_id.attrib["Format"] == SAML_NAME_ID_FORMAT_EMAIL: name_id.text = self.http_request.user.email diff --git a/authentik/providers/saml/tests/test_auth_n_request.py b/authentik/providers/saml/tests/test_auth_n_request.py index f217da405..45e6c9007 100644 --- a/authentik/providers/saml/tests/test_auth_n_request.py +++ b/authentik/providers/saml/tests/test_auth_n_request.py @@ -6,9 +6,12 @@ from django.http.request import QueryDict from django.test import RequestFactory, TestCase from guardian.utils import get_anonymous_user +from authentik.core.models import User from authentik.crypto.models import CertificateKeyPair +from authentik.events.models import Event, EventAction from authentik.flows.models import Flow from authentik.flows.tests.test_planner import dummy_get_response +from authentik.managed.manager import ObjectManager from authentik.providers.saml.models import SAMLPropertyMapping, SAMLProvider from authentik.providers.saml.processors.assertion import AssertionProcessor from authentik.providers.saml.processors.request_parser import AuthNRequestParser @@ -79,6 +82,7 @@ class TestAuthNRequest(TestCase): """Test AuthN Request generator and parser""" def setUp(self): + ObjectManager().run() cert = CertificateKeyPair.objects.first() self.provider: SAMLProvider = SAMLProvider.objects.create( authorization_flow=Flow.objects.get( @@ -243,3 +247,60 @@ class TestAuthNRequest(TestCase): parsed_request = AuthNRequestParser(provider).parse(POST_REQUEST) self.assertEqual(parsed_request.id, "aws_LDxLGeubpc5lx12gxCgS6uPbix1yd5re") self.assertEqual(parsed_request.name_id_policy, SAML_NAME_ID_FORMAT_EMAIL) + + def test_request_attributes(self): + """Test full SAML Request/Response flow, fully signed""" + http_request = self.factory.get("/") + http_request.user = User.objects.get(username="akadmin") + + middleware = SessionMiddleware(dummy_get_response) + middleware.process_request(http_request) + http_request.session.save() + + # First create an AuthNRequest + request_proc = RequestProcessor(self.source, http_request, "test_state") + request = request_proc.build_auth_n() + + # To get an assertion we need a parsed request (parsed by provider) + parsed_request = AuthNRequestParser(self.provider).parse( + b64encode(request.encode()).decode(), "test_state" + ) + # Now create a response and convert it to string (provider) + response_proc = AssertionProcessor(self.provider, http_request, parsed_request) + self.assertIn("akadmin", response_proc.build_response()) + + def test_request_attributes_invalid(self): + """Test full SAML Request/Response flow, fully signed""" + http_request = self.factory.get("/") + http_request.user = User.objects.get(username="akadmin") + + middleware = SessionMiddleware(dummy_get_response) + middleware.process_request(http_request) + http_request.session.save() + + # First create an AuthNRequest + request_proc = RequestProcessor(self.source, http_request, "test_state") + request = request_proc.build_auth_n() + + # Create invalid PropertyMapping + scope = SAMLPropertyMapping.objects.create( + name="test", saml_name="test", expression="q" + ) + self.provider.property_mappings.add(scope) + + # To get an assertion we need a parsed request (parsed by provider) + parsed_request = AuthNRequestParser(self.provider).parse( + b64encode(request.encode()).decode(), "test_state" + ) + # Now create a response and convert it to string (provider) + response_proc = AssertionProcessor(self.provider, http_request, parsed_request) + self.assertIn("akadmin", response_proc.build_response()) + + events = Event.objects.filter( + action=EventAction.CONFIGURATION_ERROR, + ) + self.assertTrue(events.exists()) + self.assertEqual( + events.first().context["message"], + "Failed to evaluate property-mapping: name 'q' is not defined", + )