From 3ce3a64d1be048af353637499d4a9979035f6fb9 Mon Sep 17 00:00:00 2001 From: Kristof Bajnok Date: Thu, 14 Jul 2022 16:05:53 +0200 Subject: [PATCH 01/11] ModuleRouter: support paths in BASE If Satosa is installed under a path which is not the root of the webserver (ie. "https://example.com/satosa"), then endpoint routing must take the base path into consideration. Some modules registered some of their endpoints with the base path included, but other times the base path was omitted, thus it made the routing fail. Now all endpoint registrations include the base path in their endpoint map. Additionally, DEBUG logging was configured for the tests so that the debug logs are accessible during testing. --- src/satosa/backends/base.py | 2 +- src/satosa/base.py | 9 ++++- src/satosa/context.py | 4 -- src/satosa/frontends/base.py | 7 +++- src/satosa/frontends/openid_connect.py | 15 ++++--- src/satosa/frontends/ping.py | 3 +- src/satosa/frontends/saml2.py | 42 +++++++++++++------- src/satosa/micro_services/account_linking.py | 12 +++++- src/satosa/micro_services/base.py | 2 + src/satosa/micro_services/consent.py | 12 +++++- src/satosa/routing.py | 30 +++++++++++--- tests/conftest.py | 24 +++++++++-- tests/flows/test_account_linking.py | 5 ++- tests/flows/test_consent.py | 5 ++- 14 files changed, 127 insertions(+), 45 deletions(-) diff --git a/src/satosa/backends/base.py b/src/satosa/backends/base.py index 8d0432da8..381902eee 100644 --- a/src/satosa/backends/base.py +++ b/src/satosa/backends/base.py @@ -29,7 +29,7 @@ def __init__(self, auth_callback_func, internal_attributes, base_url, name): self.auth_callback_func = auth_callback_func self.internal_attributes = internal_attributes self.converter = AttributeMapper(internal_attributes) - self.base_url = base_url + self.base_url = base_url.rstrip("/") if base_url else "" self.name = name def start_auth(self, context, internal_request): diff --git a/src/satosa/base.py b/src/satosa/base.py index 404104920..67d4bc995 100644 --- a/src/satosa/base.py +++ b/src/satosa/base.py @@ -6,6 +6,7 @@ import uuid from saml2.s_utils import UnknownSystemEntity +from urllib.parse import urlparse from satosa import util from .context import Context @@ -38,6 +39,8 @@ def __init__(self, config): """ self.config = config + base_path = urlparse(self.config["BASE"]).path.lstrip("/") + logger.info("Loading backend modules...") backends = load_backends(self.config, self._auth_resp_callback_func, self.config["INTERNAL_ATTRIBUTES"]) @@ -63,8 +66,10 @@ def __init__(self, config): self.config["BASE"])) self._link_micro_services(self.response_micro_services, self._auth_resp_finish) - self.module_router = ModuleRouter(frontends, backends, - self.request_micro_services + self.response_micro_services) + self.module_router = ModuleRouter(frontends, + backends, + self.request_micro_services + self.response_micro_services, + base_path) def _link_micro_services(self, micro_services, finisher): if not micro_services: diff --git a/src/satosa/context.py b/src/satosa/context.py index 1cf140586..fd51ef581 100644 --- a/src/satosa/context.py +++ b/src/satosa/context.py @@ -76,10 +76,6 @@ def path(self, p): raise ValueError("path can't start with '/'") self._path = p - def target_entity_id_from_path(self): - target_entity_id = self.path.split("/")[1] - return target_entity_id - def decorate(self, key, value): """ Add information to the context diff --git a/src/satosa/frontends/base.py b/src/satosa/frontends/base.py index 52840a85c..6eb15d2e1 100644 --- a/src/satosa/frontends/base.py +++ b/src/satosa/frontends/base.py @@ -3,6 +3,9 @@ """ from ..attribute_mapping import AttributeMapper +import os.path +from urllib.parse import urlparse + class FrontendModule(object): """ @@ -23,8 +26,10 @@ def __init__(self, auth_req_callback_func, internal_attributes, base_url, name): self.auth_req_callback_func = auth_req_callback_func self.internal_attributes = internal_attributes self.converter = AttributeMapper(internal_attributes) - self.base_url = base_url + self.base_url = base_url.rstrip("/") if base_url else "" self.name = name + self.endpoint_baseurl = os.path.join(self.base_url, self.name) + self.endpoint_basepath = urlparse(self.endpoint_baseurl).path.lstrip("/") def handle_authn_response(self, context, internal_resp): """ diff --git a/src/satosa/frontends/openid_connect.py b/src/satosa/frontends/openid_connect.py index 88041b373..75573b4fe 100644 --- a/src/satosa/frontends/openid_connect.py +++ b/src/satosa/frontends/openid_connect.py @@ -97,7 +97,6 @@ def __init__(self, auth_req_callback_func, internal_attributes, conf, base_url, else: cdb = {} - self.endpoint_baseurl = "{}/{}".format(self.base_url, self.name) self.provider = _create_provider( provider_config, self.endpoint_baseurl, @@ -173,6 +172,9 @@ def register_endpoints(self, backend_names): :rtype: list[(str, ((satosa.context.Context, Any) -> satosa.response.Response, Any))] :raise ValueError: if more than one backend is configured """ + provider_config = ("^.well-known/openid-configuration$", self.provider_config) + jwks_uri = ("^{}/jwks$".format(self.endpoint_basepath), self.jwks) + backend_name = None if len(backend_names) != 1: # only supports one backend since there currently is no way to publish multiple authorization endpoints @@ -189,16 +191,13 @@ def register_endpoints(self, backend_names): else: backend_name = backend_names[0] - provider_config = ("^.well-known/openid-configuration$", self.provider_config) - jwks_uri = ("^{}/jwks$".format(self.name), self.jwks) - if backend_name: # if there is only one backend, include its name in the path so the default routing can work auth_endpoint = "{}/{}/{}/{}".format(self.base_url, backend_name, self.name, AuthorizationEndpoint.url) self.provider.configuration_information["authorization_endpoint"] = auth_endpoint auth_path = urlparse(auth_endpoint).path.lstrip("/") else: - auth_path = "{}/{}".format(self.name, AuthorizationEndpoint.url) + auth_path = "{}/{}".format(self.endpoint_basepath, AuthorizationEndpoint.url) authentication = ("^{}$".format(auth_path), self.handle_authn_request) url_map = [provider_config, jwks_uri, authentication] @@ -208,7 +207,7 @@ def register_endpoints(self, backend_names): self.endpoint_baseurl, TokenEndpoint.url ) token_endpoint = ( - "^{}/{}".format(self.name, TokenEndpoint.url), self.token_endpoint + "^{}/{}".format(self.endpoint_basepath, TokenEndpoint.url), self.token_endpoint ) url_map.append(token_endpoint) @@ -216,13 +215,13 @@ def register_endpoints(self, backend_names): "{}/{}".format(self.endpoint_baseurl, UserinfoEndpoint.url) ) userinfo_endpoint = ( - "^{}/{}".format(self.name, UserinfoEndpoint.url), self.userinfo_endpoint + "^{}/{}".format(self.endpoint_basepath, UserinfoEndpoint.url), self.userinfo_endpoint ) url_map.append(userinfo_endpoint) if "registration_endpoint" in self.provider.configuration_information: client_registration = ( - "^{}/{}".format(self.name, RegistrationEndpoint.url), + "^{}/{}".format(self.endpoint_basepath, RegistrationEndpoint.url), self.client_registration, ) url_map.append(client_registration) diff --git a/src/satosa/frontends/ping.py b/src/satosa/frontends/ping.py index 27fec279c..a84280201 100644 --- a/src/satosa/frontends/ping.py +++ b/src/satosa/frontends/ping.py @@ -1,4 +1,5 @@ import logging +import os.path import satosa.logging_util as lu from satosa.frontends.base import FrontendModule @@ -43,7 +44,7 @@ def register_endpoints(self, backend_names): :rtype: list[(str, ((satosa.context.Context, Any) -> satosa.response.Response, Any))] :raise ValueError: if more than one backend is configured """ - url_map = [("^{}".format(self.name), self.ping_endpoint)] + url_map = [("^{}".format(os.path.join(self.endpoint_basepath, self.name)), self.ping_endpoint)] return url_map diff --git a/src/satosa/frontends/saml2.py b/src/satosa/frontends/saml2.py index 655e6da68..41be65799 100644 --- a/src/satosa/frontends/saml2.py +++ b/src/satosa/frontends/saml2.py @@ -117,7 +117,7 @@ def register_endpoints(self, backend_names): if self.enable_metadata_reload(): url_map.append( - ("^%s/%s$" % (self.name, "reload-metadata"), self._reload_metadata)) + ("^%s/%s$" % (self.endpoint_basepath, "reload-metadata"), self._reload_metadata)) self.idp_config = self._build_idp_config_endpoints( self.config[self.KEY_IDP_CONFIG], backend_names) @@ -511,15 +511,19 @@ def _register_endpoints(self, providers): """ url_map = [] + backend_providers = "|".join(providers) + base_path = urlparse(self.base_url).path.lstrip("/") + if base_path: + base_path = base_path + "/" for endp_category in self.endpoints: for binding, endp in self.endpoints[endp_category].items(): - valid_providers = "" - for provider in providers: - valid_providers = "{}|^{}".format(valid_providers, provider) - valid_providers = valid_providers.lstrip("|") - parsed_endp = urlparse(endp) - url_map.append(("(%s)/%s$" % (valid_providers, parsed_endp.path), - functools.partial(self.handle_authn_request, binding_in=binding))) + endp_path = urlparse(endp).path + url_map.append( + ( + "^{}({})/{}$".format(base_path, backend_providers, endp_path), + functools.partial(self.handle_authn_request, binding_in=binding) + ) + ) if self.expose_entityid_endpoint(): logger.debug("Exposing frontend entity endpoint = {}".format(self.idp.config.entityid)) @@ -675,11 +679,18 @@ def _load_idp_dynamic_endpoints(self, context): :param context: :return: An idp server """ - target_entity_id = context.target_entity_id_from_path() + target_entity_id = self._target_entity_id_from_path(context.path) idp_conf_file = self._load_endpoints_to_config(context.target_backend, target_entity_id) idp_config = IdPConfig().load(idp_conf_file) return Server(config=idp_config) + def _target_entity_id_from_path(self, request_path): + path = request_path.lstrip("/") + base_path = urlparse(self.base_url).path.lstrip("/") + if base_path and path.startswith(base_path): + path = path[len(base_path):].lstrip("/") + return path.split("/")[1] + def _load_idp_dynamic_entity_id(self, state): """ Loads an idp server with the entity id saved in state @@ -705,7 +716,7 @@ def handle_authn_request(self, context, binding_in): :type binding_in: str :rtype: satosa.response.Response """ - target_entity_id = context.target_entity_id_from_path() + target_entity_id = self._target_entity_id_from_path(context.path) target_entity_id = urlsafe_b64decode(target_entity_id).decode() context.decorate(Context.KEY_TARGET_ENTITYID, target_entity_id) @@ -723,7 +734,7 @@ def _create_state_data(self, context, resp_args, relay_state): :rtype: dict[str, dict[str, str] | str] """ state = super()._create_state_data(context, resp_args, relay_state) - state["target_entity_id"] = context.target_entity_id_from_path() + state["target_entity_id"] = self._target_entity_id_from_path(context.path) return state def handle_backend_error(self, exception): @@ -758,13 +769,16 @@ def _register_endpoints(self, providers): """ url_map = [] + backend_providers = "|".join(providers) + base_path = urlparse(self.base_url).path.lstrip("/") + if base_path: + base_path = base_path + "/" for endp_category in self.endpoints: for binding, endp in self.endpoints[endp_category].items(): - valid_providers = "|^".join(providers) - parsed_endp = urlparse(endp) + endp_path = urlparse(endp).path url_map.append( ( - r"(^{})/\S+/{}".format(valid_providers, parsed_endp.path), + "^{}({})/\S+/{}$".format(base_path, backend_providers, endp_path), functools.partial(self.handle_authn_request, binding_in=binding) ) ) diff --git a/src/satosa/micro_services/account_linking.py b/src/satosa/micro_services/account_linking.py index 7305c3d79..9ddd7cb81 100644 --- a/src/satosa/micro_services/account_linking.py +++ b/src/satosa/micro_services/account_linking.py @@ -3,6 +3,7 @@ """ import json import logging +import os.path import requests from jwkest.jwk import rsa_load, RSAKey @@ -161,4 +162,13 @@ def register_endpoints(self): :return: A list of endpoints bound to a function """ - return [("^account_linking%s$" % self.endpoint, self._handle_al_response)] + return [ + ( + "^{}$".format( + os.path.join( + self.base_path, "account_linking", self.endpoint.lstrip("/") + ) + ), + self._handle_al_response, + ) + ] diff --git a/src/satosa/micro_services/base.py b/src/satosa/micro_services/base.py index 084cbea76..97271b013 100644 --- a/src/satosa/micro_services/base.py +++ b/src/satosa/micro_services/base.py @@ -2,6 +2,7 @@ Micro service for SATOSA """ import logging +from urllib.parse import urlparse logger = logging.getLogger(__name__) @@ -14,6 +15,7 @@ class MicroService(object): def __init__(self, name, base_url, **kwargs): self.name = name self.base_url = base_url + self.base_path = urlparse(base_url).path.lstrip("/") self.next = None def process(self, context, data): diff --git a/src/satosa/micro_services/consent.py b/src/satosa/micro_services/consent.py index a469e2189..f533bdb7d 100644 --- a/src/satosa/micro_services/consent.py +++ b/src/satosa/micro_services/consent.py @@ -4,6 +4,7 @@ import hashlib import json import logging +import os.path from base64 import urlsafe_b64encode import requests @@ -238,4 +239,13 @@ def register_endpoints(self): :return: A list of endpoints bound to a function """ - return [("^consent%s$" % self.endpoint, self._handle_consent_response)] + return [ + ( + "^{}$".format( + os.path.join( + self.base_path, "consent", self.endpoint.lstrip("/") + ) + ), + self._handle_consent_response, + ) + ] diff --git a/src/satosa/routing.py b/src/satosa/routing.py index 317b047f9..c9fa8ab8e 100644 --- a/src/satosa/routing.py +++ b/src/satosa/routing.py @@ -38,11 +38,12 @@ class UnknownEndpoint(ValueError): and handles the internal routing between frontends and backends. """ - def __init__(self, frontends, backends, micro_services): + def __init__(self, frontends, backends, micro_services, base_path=""): """ :type frontends: dict[str, satosa.frontends.base.FrontendModule] :type backends: dict[str, satosa.backends.base.BackendModule] :type micro_services: Sequence[satosa.micro_services.base.MicroService] + :type base_path: str :param frontends: All available frontends used by the proxy. Key as frontend name, value as module @@ -50,6 +51,7 @@ def __init__(self, frontends, backends, micro_services): module :param micro_services: All available micro services used by the proxy. Key as micro service name, value as module + :param base_path: Base path for endpoint mapping """ if not frontends or not backends: @@ -68,6 +70,8 @@ def __init__(self, frontends, backends, micro_services): else: self.micro_services = {} + self.base_path = base_path + logger.debug("Loaded backends with endpoints: {}".format(backends)) logger.debug("Loaded frontends with endpoints: {}".format(frontends)) logger.debug("Loaded micro services with endpoints: {}".format(micro_services)) @@ -134,6 +138,19 @@ def _find_registered_endpoint(self, context, modules): raise ModuleRouter.UnknownEndpoint(context.path) + def _find_backend(self, request_path): + """ + Tries to guess the backend in use from the request. + Returns the backend name or None if the backend was not specified. + """ + request_path = request_path.lstrip("/") + if self.base_path and request_path.startswith(self.base_path): + request_path = request_path[len(self.base_path):].lstrip("/") + backend_guess = request_path.split("/")[0] + if backend_guess in self.backends: + return backend_guess + return None + def endpoint_routing(self, context): """ Finds and returns the endpoint function bound to the path @@ -155,13 +172,12 @@ def endpoint_routing(self, context): msg = "Routing path: {path}".format(path=context.path) logline = lu.LOG_FMT.format(id=lu.get_session_id(context.state), message=msg) logger.debug(logline) - path_split = context.path.split("/") - backend = path_split[0] - if backend in self.backends: + backend = self._find_backend(context.path) + if backend is not None: context.target_backend = backend else: - msg = "Unknown backend {}".format(backend) + msg = "No backend was specified in request or no such backend {}".format(backend) logline = lu.LOG_FMT.format( id=lu.get_session_id(context.state), message=msg ) @@ -170,6 +186,8 @@ def endpoint_routing(self, context): try: name, frontend_endpoint = self._find_registered_endpoint(context, self.frontends) except ModuleRouter.UnknownEndpoint: + for frontend in self.frontends.values(): + logger.debug(f"Unable to find {context.path} in {frontend['endpoints']}") pass else: context.target_frontend = name @@ -183,7 +201,7 @@ def endpoint_routing(self, context): context.target_micro_service = name return micro_service_endpoint - if backend in self.backends: + if backend is not None: backend_endpoint = self._find_registered_backend_endpoint(context) if backend_endpoint: return backend_endpoint diff --git a/tests/conftest.py b/tests/conftest.py index f0602a028..52c4a9fc1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -11,7 +11,7 @@ from .util import create_metadata_from_config_dict from .util import generate_cert, write_cert -BASE_URL = "https://test-proxy.com" +BASE_URL = "https://test-proxy.com/satosa" @pytest.fixture(scope="session") @@ -38,7 +38,7 @@ def cert_and_key(tmpdir): @pytest.fixture def sp_conf(cert_and_key): - sp_base = "http://example.com" + sp_base = "http://example.com/test" spconfig = { "entityid": "{}/unittest_sp.xml".format(sp_base), "service": { @@ -64,7 +64,7 @@ def sp_conf(cert_and_key): @pytest.fixture def idp_conf(cert_and_key): - idp_base = "http://idp.example.com" + idp_base = "http://idp.example.com/test" idpconfig = { "entityid": "{}/{}/proxy.xml".format(idp_base, "Saml2IDP"), @@ -136,7 +136,23 @@ def satosa_config_dict(backend_plugin_config, frontend_plugin_config, request_mi "BACKEND_MODULES": [backend_plugin_config], "FRONTEND_MODULES": [frontend_plugin_config], "MICRO_SERVICES": [request_microservice_config, response_microservice_config], - "LOGGING": {"version": 1} + "LOGGING": { + "version": 1, + "handlers": { + "stdout": { + "level": "DEBUG", + "class": "logging.StreamHandler", + "stream": "ext://sys.stdout", + "formatter": "simple", + } + }, + "loggers": {"satosa": {"level": "DEBUG"}}, + "formatters": { + "simple": { + "format": "[%(asctime)s] [%(levelname)s] [%(name)s.%(funcName)s] %(message)s" + } + }, + } } return config diff --git a/tests/flows/test_account_linking.py b/tests/flows/test_account_linking.py index 94f53a431..e6d70cb76 100644 --- a/tests/flows/test_account_linking.py +++ b/tests/flows/test_account_linking.py @@ -1,4 +1,6 @@ +import os.path import responses +from urllib.parse import urlparse from werkzeug.test import Client from werkzeug.wrappers import Response @@ -13,6 +15,7 @@ def test_full_flow(self, satosa_config_dict, account_linking_module_config): account_linking_module_config["config"]["api_url"] = api_url account_linking_module_config["config"]["redirect_url"] = redirect_url satosa_config_dict["MICRO_SERVICES"].insert(0, account_linking_module_config) + base_path = urlparse(satosa_config_dict["BASE"]).path.lstrip("\n") # application test_client = Client(make_app(SATOSAConfig(satosa_config_dict)), Response) @@ -36,5 +39,5 @@ def test_full_flow(self, satosa_config_dict, account_linking_module_config): rsps.add(responses.GET, "{}/get_id".format(api_url), "test_userid", status=200) # incoming account linking response - http_resp = test_client.get("/account_linking/handle_account_linking") + http_resp = test_client.get(os.path.join("/", base_path, "account_linking/handle_account_linking")) assert http_resp.status_code == 200 diff --git a/tests/flows/test_consent.py b/tests/flows/test_consent.py index 76dff496b..bf945ea6e 100644 --- a/tests/flows/test_consent.py +++ b/tests/flows/test_consent.py @@ -1,7 +1,9 @@ import json +import os.path import re import responses +from urllib.parse import urlparse from werkzeug.test import Client from werkzeug.wrappers import Response @@ -16,6 +18,7 @@ def test_full_flow(self, satosa_config_dict, consent_module_config): consent_module_config["config"]["api_url"] = api_url consent_module_config["config"]["redirect_url"] = redirect_url satosa_config_dict["MICRO_SERVICES"].append(consent_module_config) + base_path = urlparse(satosa_config_dict["BASE"]).path.lstrip("\n") # application test_client = Client(make_app(SATOSAConfig(satosa_config_dict)), Response) @@ -43,5 +46,5 @@ def test_full_flow(self, satosa_config_dict, consent_module_config): rsps.add(responses.GET, verify_url_re, json.dumps({"foo": "bar"}), status=200) # incoming consent response - http_resp = test_client.get("/consent/handle_consent") + http_resp = test_client.get(os.path.join("/", base_path, "consent/handle_consent")) assert http_resp.status_code == 200 From b253b3043cb7c0fb3806710b216184a54de7c9c7 Mon Sep 17 00:00:00 2001 From: Kristof Bajnok Date: Mon, 13 Mar 2023 09:21:01 +0100 Subject: [PATCH 02/11] fixup! ModuleRouter: support paths in BASE Rebased to current master. When composing the paths, use os.path.join primarily, since it handles empty strings and duplicate separators logically. As long as we use the BASE_URL in the OpenID Connect frontend as an issuer, it's not possible to create multiple provider discovery URLs. Add documentation and a comment to explain this limitation. --- doc/README.md | 3 +- src/satosa/frontends/base.py | 5 +- src/satosa/frontends/openid_connect.py | 40 +++++++++++---- tests/flows/test_oidc-saml.py | 17 +++++-- tests/satosa/frontends/test_openid_connect.py | 51 +++++++++++++++---- 5 files changed, 90 insertions(+), 26 deletions(-) diff --git a/doc/README.md b/doc/README.md index 8d001847e..6fff65c7b 100644 --- a/doc/README.md +++ b/doc/README.md @@ -78,7 +78,8 @@ bind_password: !ENVFILE LDAP_BIND_PASSWORD_FILE | Parameter name | Data type | Example value | Description | | -------------- | --------- | ------------- | ----------- | -| `BASE` | string | `https://proxy.example.com` | base url of the proxy | +| `BASE` | string | `https://proxy.example.com` | The base url of the proxy. For the OIDC Frontend, this is used to set the issuer as well, and due to implementation constraints, avoid +using trailing slashes in this case. | | `COOKIE_STATE_NAME` | string | `satosa_state` | name of the cookie SATOSA uses for preserving state between requests | | `CONTEXT_STATE_DELETE` | bool | `True` | controls whether SATOSA will delete the state cookie after receiving the authentication response from the upstream IdP| | `STATE_ENCRYPTION_KEY` | string | `52fddd3528a44157` | key used for encrypting the state cookie, will be overridden by the environment variable `SATOSA_STATE_ENCRYPTION_KEY` if it is set | diff --git a/src/satosa/frontends/base.py b/src/satosa/frontends/base.py index 6eb15d2e1..d5a611995 100644 --- a/src/satosa/frontends/base.py +++ b/src/satosa/frontends/base.py @@ -17,16 +17,19 @@ def __init__(self, auth_req_callback_func, internal_attributes, base_url, name): :type auth_req_callback_func: (satosa.context.Context, satosa.internal.InternalData) -> satosa.response.Response :type internal_attributes: dict[str, dict[str, str | list[str]]] + :type base_url: str :type name: str :param auth_req_callback_func: Callback should be called by the module after the authorization response has been processed. + :param internal_attributes: attribute mapping + :param base_url: base url of the proxy :param name: name of the plugin """ self.auth_req_callback_func = auth_req_callback_func self.internal_attributes = internal_attributes self.converter = AttributeMapper(internal_attributes) - self.base_url = base_url.rstrip("/") if base_url else "" + self.base_url = base_url or "" self.name = name self.endpoint_baseurl = os.path.join(self.base_url, self.name) self.endpoint_basepath = urlparse(self.endpoint_baseurl).path.lstrip("/") diff --git a/src/satosa/frontends/openid_connect.py b/src/satosa/frontends/openid_connect.py index 75573b4fe..3c04437b2 100644 --- a/src/satosa/frontends/openid_connect.py +++ b/src/satosa/frontends/openid_connect.py @@ -4,6 +4,7 @@ import json import logging +import os.path from collections import defaultdict from urllib.parse import urlencode, urlparse @@ -172,7 +173,16 @@ def register_endpoints(self, backend_names): :rtype: list[(str, ((satosa.context.Context, Any) -> satosa.response.Response, Any))] :raise ValueError: if more than one backend is configured """ - provider_config = ("^.well-known/openid-configuration$", self.provider_config) + # See https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfig + # Unfortunately since the issuer is always `base_url` for all OIDC frontend instances, + # the discovery endpoint will be the same for every instance. + # This means that only one frontend will be usable for autodiscovery. + autoconf_path = ".well-known/openid-configuration" + base_path = urlparse(self.base_url).path.lstrip("/") + provider_config = ( + "^{}$".format(os.path.join(base_path, autoconf_path)), + self.provider_config, + ) jwks_uri = ("^{}/jwks$".format(self.endpoint_basepath), self.jwks) backend_name = None @@ -193,35 +203,47 @@ def register_endpoints(self, backend_names): if backend_name: # if there is only one backend, include its name in the path so the default routing can work - auth_endpoint = "{}/{}/{}/{}".format(self.base_url, backend_name, self.name, AuthorizationEndpoint.url) + auth_endpoint = os.path.join( + self.base_url, + backend_name, + self.name, + AuthorizationEndpoint.url, + ) self.provider.configuration_information["authorization_endpoint"] = auth_endpoint auth_path = urlparse(auth_endpoint).path.lstrip("/") else: - auth_path = "{}/{}".format(self.endpoint_basepath, AuthorizationEndpoint.url) + auth_path = os.path.join(self.endpoint_basepath, AuthorizationRequest.url) authentication = ("^{}$".format(auth_path), self.handle_authn_request) url_map = [provider_config, jwks_uri, authentication] if any("code" in v for v in self.provider.configuration_information["response_types_supported"]): - self.provider.configuration_information["token_endpoint"] = "{}/{}".format( - self.endpoint_baseurl, TokenEndpoint.url + self.provider.configuration_information["token_endpoint"] = os.path.join( + self.endpoint_baseurl, + TokenEndpoint.url, ) token_endpoint = ( - "^{}/{}".format(self.endpoint_basepath, TokenEndpoint.url), self.token_endpoint + "^{}".format(os.path.join(self.endpoint_basepath, TokenEndpoint.url)), + self.token_endpoint, ) url_map.append(token_endpoint) self.provider.configuration_information["userinfo_endpoint"] = ( - "{}/{}".format(self.endpoint_baseurl, UserinfoEndpoint.url) + os.path.join(self.endpoint_baseurl, UserinfoEndpoint.url) ) userinfo_endpoint = ( - "^{}/{}".format(self.endpoint_basepath, UserinfoEndpoint.url), self.userinfo_endpoint + "^{}".format( + os.path.join(self.endpoint_basepath, UserinfoEndpoint.url) + ), + self.userinfo_endpoint, ) url_map.append(userinfo_endpoint) if "registration_endpoint" in self.provider.configuration_information: client_registration = ( - "^{}/{}".format(self.endpoint_basepath, RegistrationEndpoint.url), + "^{}".format( + os.path.join(self.endpoint_basepath, RegistrationEndpoint.url) + ), self.client_registration, ) url_map.append(client_registration) diff --git a/tests/flows/test_oidc-saml.py b/tests/flows/test_oidc-saml.py index 2a299bfef..656a70a1f 100644 --- a/tests/flows/test_oidc-saml.py +++ b/tests/flows/test_oidc-saml.py @@ -52,7 +52,6 @@ def oidc_frontend_config(signing_key_path): "module": "satosa.frontends.openid_connect.OpenIDConnectFrontend", "name": "OIDCFrontend", "config": { - "issuer": "https://proxy-op.example.com", "signing_key_path": signing_key_path, "provider": {"response_types_supported": ["id_token"]}, "client_db_uri": DB_URI, # use mongodb for integration testing @@ -69,7 +68,6 @@ def oidc_stateless_frontend_config(signing_key_path, client_db_path): "module": "satosa.frontends.openid_connect.OpenIDConnectFrontend", "name": "OIDCFrontend", "config": { - "issuer": "https://proxy-op.example.com", "signing_key_path": signing_key_path, "client_db_path": client_db_path, "db_uri": "stateless://user:abc123@localhost", @@ -94,6 +92,12 @@ def _client_setup(self): "response_types": ["id_token"] } + def _discover_provider(self, client, provider): + discovery_path = ( + os.path.join(urlparse(provider).path, ".well-known/openid-configuration") + ) + return json.loads(client.get(discovery_path).data.decode("utf-8")) + def test_full_flow(self, satosa_config_dict, oidc_frontend_config, saml_backend_config, idp_conf): self._client_setup() subject_id = "testuser1" @@ -110,7 +114,8 @@ def test_full_flow(self, satosa_config_dict, oidc_frontend_config, saml_backend_ test_client = Client(make_app(SATOSAConfig(satosa_config_dict)), Response) # get frontend OP config info - provider_config = json.loads(test_client.get("/.well-known/openid-configuration").data.decode("utf-8")) + issuer = satosa_config_dict["BASE"] + provider_config = self._discover_provider(test_client, issuer) # create auth req claims_request = ClaimsRequest(id_token=Claims(**{k: None for k in USERS[subject_id]})) @@ -168,7 +173,8 @@ def test_full_stateless_id_token_flow(self, satosa_config_dict, oidc_stateless_f test_client = Client(make_app(SATOSAConfig(satosa_config_dict)), Response) # get frontend OP config info - provider_config = json.loads(test_client.get("/.well-known/openid-configuration").data.decode("utf-8")) + issuer = satosa_config_dict["BASE"] + provider_config = self._discover_provider(test_client, issuer) # create auth req claims_request = ClaimsRequest(id_token=Claims(**{k: None for k in USERS[subject_id]})) @@ -226,7 +232,8 @@ def test_full_stateless_code_flow(self, satosa_config_dict, oidc_stateless_front test_client = Client(make_app(SATOSAConfig(satosa_config_dict)), Response) # get frontend OP config info - provider_config = json.loads(test_client.get("/.well-known/openid-configuration").data.decode("utf-8")) + issuer = satosa_config_dict["BASE"] + provider_config = self._discover_provider(test_client, issuer) # create auth req claims_request = ClaimsRequest(id_token=Claims(**{k: None for k in USERS[subject_id]})) diff --git a/tests/satosa/frontends/test_openid_connect.py b/tests/satosa/frontends/test_openid_connect.py index f769b2c66..4331e59a0 100644 --- a/tests/satosa/frontends/test_openid_connect.py +++ b/tests/satosa/frontends/test_openid_connect.py @@ -28,7 +28,7 @@ INTERNAL_ATTRIBUTES = { "attributes": {"mail": {"saml": ["email"], "openid": ["email"]}} } -BASE_URL = "https://op.example.com" +BASE_URL = "https://op.example.com/satosa" CLIENT_ID = "client1" CLIENT_SECRET = "client_secret" EXTRA_CLAIMS = { @@ -86,10 +86,10 @@ def frontend_config_with_extra_id_token_claims(self, signing_key_path): return config - def create_frontend(self, frontend_config): + def create_frontend(self, frontend_config, issuer=BASE_URL): # will use in-memory storage instance = OpenIDConnectFrontend(lambda ctx, req: None, INTERNAL_ATTRIBUTES, - frontend_config, BASE_URL, "oidc_frontend") + frontend_config, issuer, "oidc_frontend") instance.register_endpoints(["foo_backend"]) return instance @@ -369,10 +369,30 @@ def test_jwks(self, context, frontend): jwks = json.loads(http_response.message) assert jwks == {"keys": [frontend.signing_key.serialize()]} - def test_register_endpoints_token_and_userinfo_endpoint_is_published_if_necessary(self, frontend): + @pytest.mark.parametrize("issuer", [ + "https://example.com", + "https://example.com/some/op", + "https://example.com/" + ]) + def test_register_endpoints_handles_path_in_issuer(self, frontend_config, issuer): + frontend = self.create_frontend(frontend_config, issuer) + issuer_path = urlparse(issuer).path[1:] + if issuer_path: + issuer_path += "/" urls = frontend.register_endpoints(["test"]) - assert ("^{}/{}".format(frontend.name, TokenEndpoint.url), frontend.token_endpoint) in urls - assert ("^{}/{}".format(frontend.name, UserinfoEndpoint.url), frontend.userinfo_endpoint) in urls + assert ( + "^{}{}$".format(issuer_path, ".well-known/openid-configuration"), + frontend.provider_config, + ) in urls + + assert ( + "^{}/{}".format(frontend.endpoint_basepath, TokenEndpoint.url), + frontend.token_endpoint, + ) in urls + assert ( + "^{}/{}".format(frontend.endpoint_basepath, UserinfoEndpoint.url), + frontend.userinfo_endpoint, + ) in urls def test_register_endpoints_token_and_userinfo_endpoint_is_not_published_if_only_implicit_flow( self, frontend_config, context): @@ -380,8 +400,14 @@ def test_register_endpoints_token_and_userinfo_endpoint_is_not_published_if_only frontend = self.create_frontend(frontend_config) urls = frontend.register_endpoints(["test"]) - assert ("^{}/{}".format("test", TokenEndpoint.url), frontend.token_endpoint) not in urls - assert ("^{}/{}".format("test", UserinfoEndpoint.url), frontend.userinfo_endpoint) not in urls + assert ( + "^{}/{}".format(frontend.endpoint_basepath, TokenEndpoint.url), + frontend.token_endpoint, + ) not in urls + assert ( + "^{}/{}".format(frontend.endpoint_basepath, UserinfoEndpoint.url), + frontend.userinfo_endpoint, + ) not in urls http_response = frontend.provider_config(context) provider_config = ProviderConfigurationResponse().deserialize(http_response.message, "json") @@ -397,8 +423,13 @@ def test_register_endpoints_dynamic_client_registration_is_configurable( frontend = self.create_frontend(frontend_config) urls = frontend.register_endpoints(["test"]) - assert (("^{}/{}".format(frontend.name, RegistrationEndpoint.url), - frontend.client_registration) in urls) == client_registration_enabled + assert ( + ( + "^{}/{}".format(frontend.endpoint_basepath, RegistrationEndpoint.url), + frontend.client_registration, + ) + in urls + ) == client_registration_enabled provider_info = ProviderConfigurationResponse().deserialize(frontend.provider_config(None).message, "json") assert ("registration_endpoint" in provider_info) == client_registration_enabled From fc9374c9959ddd57d69ee69f9f683f96ddede964 Mon Sep 17 00:00:00 2001 From: Kristof Bajnok Date: Mon, 13 Mar 2023 11:27:43 +0100 Subject: [PATCH 03/11] fixup! ModuleRouter: support paths in BASE Avoid messing README.md with an unwanted line break. --- doc/README.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/doc/README.md b/doc/README.md index 6fff65c7b..58b143df6 100644 --- a/doc/README.md +++ b/doc/README.md @@ -78,8 +78,7 @@ bind_password: !ENVFILE LDAP_BIND_PASSWORD_FILE | Parameter name | Data type | Example value | Description | | -------------- | --------- | ------------- | ----------- | -| `BASE` | string | `https://proxy.example.com` | The base url of the proxy. For the OIDC Frontend, this is used to set the issuer as well, and due to implementation constraints, avoid -using trailing slashes in this case. | +| `BASE` | string | `https://proxy.example.com` | The base url of the proxy. For the OIDC Frontend, this is used to set the issuer as well, and due to implementation constraints, avoid using trailing slashes in this case. | | `COOKIE_STATE_NAME` | string | `satosa_state` | name of the cookie SATOSA uses for preserving state between requests | | `CONTEXT_STATE_DELETE` | bool | `True` | controls whether SATOSA will delete the state cookie after receiving the authentication response from the upstream IdP| | `STATE_ENCRYPTION_KEY` | string | `52fddd3528a44157` | key used for encrypting the state cookie, will be overridden by the environment variable `SATOSA_STATE_ENCRYPTION_KEY` if it is set | From 8cb44d6d4fba9aa9ecebddc5be421c3dbd661729 Mon Sep 17 00:00:00 2001 From: Kristof Bajnok Date: Thu, 16 Mar 2023 09:06:32 +0100 Subject: [PATCH 04/11] amend! fixup! ModuleRouter: support paths in BASE If Satosa is installed under a path which is not the root of the webserver (ie. "https://example.com/satosa"), then endpoint routing must take the base path into consideration. Some modules registered some of their endpoints with the base path included, but other times the base path was omitted, thus it made the routing fail. Now all endpoint registrations include the base path in their endpoint map. Provide a simple implementation for joining path components, since we don't want to add the separator for empty strings and when any of the path components already have it. Additionally, DEBUG logging was configured for the tests so that the debug logs are accessible during testing. --- doc/README.md | 2 +- src/satosa/frontends/base.py | 4 +- src/satosa/frontends/openid_connect.py | 28 +++++++------- src/satosa/frontends/ping.py | 4 +- src/satosa/micro_services/account_linking.py | 6 +-- src/satosa/micro_services/consent.py | 6 +-- src/satosa/util.py | 26 +++++++++++++ tests/flows/test_account_linking.py | 6 +-- tests/flows/test_consent.py | 6 +-- tests/flows/test_oidc-saml.py | 3 +- tests/satosa/test_util.py | 39 ++++++++++++++++++++ 11 files changed, 98 insertions(+), 32 deletions(-) create mode 100644 tests/satosa/test_util.py diff --git a/doc/README.md b/doc/README.md index 58b143df6..8d001847e 100644 --- a/doc/README.md +++ b/doc/README.md @@ -78,7 +78,7 @@ bind_password: !ENVFILE LDAP_BIND_PASSWORD_FILE | Parameter name | Data type | Example value | Description | | -------------- | --------- | ------------- | ----------- | -| `BASE` | string | `https://proxy.example.com` | The base url of the proxy. For the OIDC Frontend, this is used to set the issuer as well, and due to implementation constraints, avoid using trailing slashes in this case. | +| `BASE` | string | `https://proxy.example.com` | base url of the proxy | | `COOKIE_STATE_NAME` | string | `satosa_state` | name of the cookie SATOSA uses for preserving state between requests | | `CONTEXT_STATE_DELETE` | bool | `True` | controls whether SATOSA will delete the state cookie after receiving the authentication response from the upstream IdP| | `STATE_ENCRYPTION_KEY` | string | `52fddd3528a44157` | key used for encrypting the state cookie, will be overridden by the environment variable `SATOSA_STATE_ENCRYPTION_KEY` if it is set | diff --git a/src/satosa/frontends/base.py b/src/satosa/frontends/base.py index d5a611995..08c38b79c 100644 --- a/src/satosa/frontends/base.py +++ b/src/satosa/frontends/base.py @@ -2,8 +2,8 @@ Holds a base class for frontend modules used in the SATOSA proxy. """ from ..attribute_mapping import AttributeMapper +from ..util import join_paths -import os.path from urllib.parse import urlparse @@ -31,7 +31,7 @@ def __init__(self, auth_req_callback_func, internal_attributes, base_url, name): self.converter = AttributeMapper(internal_attributes) self.base_url = base_url or "" self.name = name - self.endpoint_baseurl = os.path.join(self.base_url, self.name) + self.endpoint_baseurl = join_paths(self.base_url, self.name) self.endpoint_basepath = urlparse(self.endpoint_baseurl).path.lstrip("/") def handle_authn_response(self, context, internal_resp): diff --git a/src/satosa/frontends/openid_connect.py b/src/satosa/frontends/openid_connect.py index 3c04437b2..3b4ba9443 100644 --- a/src/satosa/frontends/openid_connect.py +++ b/src/satosa/frontends/openid_connect.py @@ -4,7 +4,6 @@ import json import logging -import os.path from collections import defaultdict from urllib.parse import urlencode, urlparse @@ -38,7 +37,7 @@ from ..response import BadRequest, Created from ..response import SeeOther, Response from ..response import Unauthorized -from ..util import rndstr +from ..util import join_paths, rndstr import satosa.logging_util as lu from satosa.internal import InternalData @@ -174,13 +173,14 @@ def register_endpoints(self, backend_names): :raise ValueError: if more than one backend is configured """ # See https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfig - # Unfortunately since the issuer is always `base_url` for all OIDC frontend instances, - # the discovery endpoint will be the same for every instance. - # This means that only one frontend will be usable for autodiscovery. + # + # We skip the scheme + host + port of the issuer URL, because we can only map the + # path for the provider config endpoint. We are safe to use urlparse().path here, + # because for issuer OIDC allows only https URLs without query and fragment parts. + issuer = self.provider.configuration_information["issuer"] autoconf_path = ".well-known/openid-configuration" - base_path = urlparse(self.base_url).path.lstrip("/") provider_config = ( - "^{}$".format(os.path.join(base_path, autoconf_path)), + "^{}$".format(join_paths(urlparse(issuer).path.lstrip("/"), autoconf_path)), self.provider_config, ) jwks_uri = ("^{}/jwks$".format(self.endpoint_basepath), self.jwks) @@ -203,7 +203,7 @@ def register_endpoints(self, backend_names): if backend_name: # if there is only one backend, include its name in the path so the default routing can work - auth_endpoint = os.path.join( + auth_endpoint = join_paths( self.base_url, backend_name, self.name, @@ -212,28 +212,28 @@ def register_endpoints(self, backend_names): self.provider.configuration_information["authorization_endpoint"] = auth_endpoint auth_path = urlparse(auth_endpoint).path.lstrip("/") else: - auth_path = os.path.join(self.endpoint_basepath, AuthorizationRequest.url) + auth_path = join_paths(self.endpoint_basepath, AuthorizationRequest.url) authentication = ("^{}$".format(auth_path), self.handle_authn_request) url_map = [provider_config, jwks_uri, authentication] if any("code" in v for v in self.provider.configuration_information["response_types_supported"]): - self.provider.configuration_information["token_endpoint"] = os.path.join( + self.provider.configuration_information["token_endpoint"] = join_paths( self.endpoint_baseurl, TokenEndpoint.url, ) token_endpoint = ( - "^{}".format(os.path.join(self.endpoint_basepath, TokenEndpoint.url)), + "^{}".format(join_paths(self.endpoint_basepath, TokenEndpoint.url)), self.token_endpoint, ) url_map.append(token_endpoint) self.provider.configuration_information["userinfo_endpoint"] = ( - os.path.join(self.endpoint_baseurl, UserinfoEndpoint.url) + join_paths(self.endpoint_baseurl, UserinfoEndpoint.url) ) userinfo_endpoint = ( "^{}".format( - os.path.join(self.endpoint_basepath, UserinfoEndpoint.url) + join_paths(self.endpoint_basepath, UserinfoEndpoint.url) ), self.userinfo_endpoint, ) @@ -242,7 +242,7 @@ def register_endpoints(self, backend_names): if "registration_endpoint" in self.provider.configuration_information: client_registration = ( "^{}".format( - os.path.join(self.endpoint_basepath, RegistrationEndpoint.url) + join_paths(self.endpoint_basepath, RegistrationEndpoint.url) ), self.client_registration, ) diff --git a/src/satosa/frontends/ping.py b/src/satosa/frontends/ping.py index a84280201..ff609cfd2 100644 --- a/src/satosa/frontends/ping.py +++ b/src/satosa/frontends/ping.py @@ -1,9 +1,9 @@ import logging -import os.path import satosa.logging_util as lu from satosa.frontends.base import FrontendModule from satosa.response import Response +from satosa.util import join_paths logger = logging.getLogger(__name__) @@ -44,7 +44,7 @@ def register_endpoints(self, backend_names): :rtype: list[(str, ((satosa.context.Context, Any) -> satosa.response.Response, Any))] :raise ValueError: if more than one backend is configured """ - url_map = [("^{}".format(os.path.join(self.endpoint_basepath, self.name)), self.ping_endpoint)] + url_map = [("^{}".format(join_paths(self.endpoint_basepath, self.name)), self.ping_endpoint)] return url_map diff --git a/src/satosa/micro_services/account_linking.py b/src/satosa/micro_services/account_linking.py index 9ddd7cb81..4cfd72c99 100644 --- a/src/satosa/micro_services/account_linking.py +++ b/src/satosa/micro_services/account_linking.py @@ -3,7 +3,6 @@ """ import json import logging -import os.path import requests from jwkest.jwk import rsa_load, RSAKey @@ -13,6 +12,7 @@ from ..exception import SATOSAAuthenticationError from ..micro_services.base import ResponseMicroService from ..response import Redirect +from ..util import join_paths import satosa.logging_util as lu logger = logging.getLogger(__name__) @@ -165,8 +165,8 @@ def register_endpoints(self): return [ ( "^{}$".format( - os.path.join( - self.base_path, "account_linking", self.endpoint.lstrip("/") + join_paths( + self.base_path, "account_linking", self.endpoint ) ), self._handle_al_response, diff --git a/src/satosa/micro_services/consent.py b/src/satosa/micro_services/consent.py index f533bdb7d..c273116d5 100644 --- a/src/satosa/micro_services/consent.py +++ b/src/satosa/micro_services/consent.py @@ -4,7 +4,6 @@ import hashlib import json import logging -import os.path from base64 import urlsafe_b64encode import requests @@ -17,6 +16,7 @@ from satosa.internal import InternalData from satosa.micro_services.base import ResponseMicroService from satosa.response import Redirect +from satosa.util import join_paths logger = logging.getLogger(__name__) @@ -242,8 +242,8 @@ def register_endpoints(self): return [ ( "^{}$".format( - os.path.join( - self.base_path, "consent", self.endpoint.lstrip("/") + join_paths( + self.base_path, "consent", self.endpoint ) ), self._handle_consent_response, diff --git a/src/satosa/util.py b/src/satosa/util.py index 9b5d63fc1..b171f1f3e 100644 --- a/src/satosa/util.py +++ b/src/satosa/util.py @@ -89,3 +89,29 @@ def rndstr(size=16, alphabet=""): if not alphabet: alphabet = string.ascii_letters[0:52] + string.digits return type(alphabet)().join(rng.choice(alphabet) for _ in range(size)) + + +def join_paths(base, *paths): + """ + Joins strings with a "/" separator, like they were path components, but + tries to avoid adding an unnecessary separator. Note that the contents of + the strings are not sanitized in any way. If any of the components begins or + ends with a "/", the separator is not inserted, and any number of empty + strings at the beginning would not add a leading slash. Any number of empty + strings at the end only add a single trailing slash. + + Raises TypeError if any of the components are not strings. + """ + sep = "/" + + path = base + try: + for p in paths: + if not path or path.endswith(sep) or p.startswith(sep): + path += p + else: + path += sep + p + except (AttributeError, TypeError) as err: + raise TypeError("Arguments must be strings") from err + + return path diff --git a/tests/flows/test_account_linking.py b/tests/flows/test_account_linking.py index e6d70cb76..ccb189593 100644 --- a/tests/flows/test_account_linking.py +++ b/tests/flows/test_account_linking.py @@ -1,4 +1,3 @@ -import os.path import responses from urllib.parse import urlparse from werkzeug.test import Client @@ -6,6 +5,7 @@ from satosa.proxy_server import make_app from satosa.satosa_config import SATOSAConfig +from satosa.util import join_paths class TestAccountLinking: @@ -15,7 +15,7 @@ def test_full_flow(self, satosa_config_dict, account_linking_module_config): account_linking_module_config["config"]["api_url"] = api_url account_linking_module_config["config"]["redirect_url"] = redirect_url satosa_config_dict["MICRO_SERVICES"].insert(0, account_linking_module_config) - base_path = urlparse(satosa_config_dict["BASE"]).path.lstrip("\n") + base_path = urlparse(satosa_config_dict["BASE"]).path.lstrip("\n/") # application test_client = Client(make_app(SATOSAConfig(satosa_config_dict)), Response) @@ -39,5 +39,5 @@ def test_full_flow(self, satosa_config_dict, account_linking_module_config): rsps.add(responses.GET, "{}/get_id".format(api_url), "test_userid", status=200) # incoming account linking response - http_resp = test_client.get(os.path.join("/", base_path, "account_linking/handle_account_linking")) + http_resp = test_client.get(join_paths("/", base_path, "account_linking/handle_account_linking")) assert http_resp.status_code == 200 diff --git a/tests/flows/test_consent.py b/tests/flows/test_consent.py index bf945ea6e..4f616e180 100644 --- a/tests/flows/test_consent.py +++ b/tests/flows/test_consent.py @@ -1,5 +1,4 @@ import json -import os.path import re import responses @@ -9,6 +8,7 @@ from satosa.proxy_server import make_app from satosa.satosa_config import SATOSAConfig +from satosa.util import join_paths class TestConsent: @@ -18,7 +18,7 @@ def test_full_flow(self, satosa_config_dict, consent_module_config): consent_module_config["config"]["api_url"] = api_url consent_module_config["config"]["redirect_url"] = redirect_url satosa_config_dict["MICRO_SERVICES"].append(consent_module_config) - base_path = urlparse(satosa_config_dict["BASE"]).path.lstrip("\n") + base_path = urlparse(satosa_config_dict["BASE"]).path.lstrip("\n/") # application test_client = Client(make_app(SATOSAConfig(satosa_config_dict)), Response) @@ -46,5 +46,5 @@ def test_full_flow(self, satosa_config_dict, consent_module_config): rsps.add(responses.GET, verify_url_re, json.dumps({"foo": "bar"}), status=200) # incoming consent response - http_resp = test_client.get(os.path.join("/", base_path, "consent/handle_consent")) + http_resp = test_client.get(join_paths("/", base_path, "consent/handle_consent")) assert http_resp.status_code == 200 diff --git a/tests/flows/test_oidc-saml.py b/tests/flows/test_oidc-saml.py index 656a70a1f..2d019b10e 100644 --- a/tests/flows/test_oidc-saml.py +++ b/tests/flows/test_oidc-saml.py @@ -17,6 +17,7 @@ from satosa.metadata_creation.saml_metadata import create_entity_descriptors from satosa.proxy_server import make_app from satosa.satosa_config import SATOSAConfig +from satosa.util import join_paths from tests.users import USERS from tests.users import OIDC_USERS from tests.util import FakeIdP @@ -94,7 +95,7 @@ def _client_setup(self): def _discover_provider(self, client, provider): discovery_path = ( - os.path.join(urlparse(provider).path, ".well-known/openid-configuration") + join_paths(urlparse(provider).path, ".well-known/openid-configuration") ) return json.loads(client.get(discovery_path).data.decode("utf-8")) diff --git a/tests/satosa/test_util.py b/tests/satosa/test_util.py new file mode 100644 index 000000000..893534b89 --- /dev/null +++ b/tests/satosa/test_util.py @@ -0,0 +1,39 @@ +import pytest +from satosa.util import join_paths + + +@pytest.mark.parametrize( + "args, expected", + [ + (["/foo", "baz", "bar"], "/foo/baz/bar"), + (["foo", "baz", "bar"], "foo/baz/bar"), + (["https://foo.baz", "bar"], "https://foo.baz/bar"), + (["https://foo.baz/", "bar"], "https://foo.baz/bar"), + (["foo", "/bar"], "foo/bar"), + (["/foo", "baz", "/bar"], "/foo/baz/bar"), + (["", "foo", "bar"], "foo/bar"), + (["", "/foo", "bar"], "/foo/bar"), + (["", "/foo/", "bar"], "/foo/bar"), + (["", "", "", "/foo", "bar"], "/foo/bar"), + (["", "", "/foo/", "", "bar"], "/foo/bar"), + (["", "", "/foo/", "", "", "bar/"], "/foo/bar/"), + (["/foo", ""], "/foo/"), + (["/foo", "", "", ""], "/foo/"), + (["/foo//", "bar"], "/foo//bar"), + (["foo"], "foo"), + ([""], ""), + (["", ""], ""), + (["'not ", "sanitized'\0/; rm -rf *"], "'not /sanitized'\0/; rm -rf *"), + (["foo/", "/bar"], "foo//bar"), + (["foo", "", "/bar"], "foo//bar"), + ([b"foo", "bar"], TypeError), + (["foo", b"bar"], TypeError), + ([None, "foo"], TypeError), + ], +) +def test_join_paths(args, expected): + if isinstance(expected, str): + assert join_paths(*args) == expected + else: + with pytest.raises(expected): + _ = join_paths(*args) From b4b8df86a9e9d435aad3385111db212568f2e909 Mon Sep 17 00:00:00 2001 From: Kristof Bajnok Date: Fri, 17 Mar 2023 18:15:26 +0100 Subject: [PATCH 05/11] frontends/openid_connect: support issuer override via provider Even though the OIDC provider configuration has an element for setting the issuer, for some reason it was rewritten to BASE unconditionally, but this has broken provider endpoint discovery when multiple OIDC frontends were in use. --- .../frontends/openid_connect_frontend.yaml.example | 5 +++++ src/satosa/frontends/openid_connect.py | 3 ++- tests/satosa/frontends/test_openid_connect.py | 11 +++++++++++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/example/plugins/frontends/openid_connect_frontend.yaml.example b/example/plugins/frontends/openid_connect_frontend.yaml.example index d7a5584d8..c517958f2 100644 --- a/example/plugins/frontends/openid_connect_frontend.yaml.example +++ b/example/plugins/frontends/openid_connect_frontend.yaml.example @@ -33,6 +33,11 @@ config: sub_hash_salt: randomSALTvalue provider: + # If you do not specify the issuer here, then BASE will be used as Issuer. + # Note that even though this setting must be specified as a full URL, + # provider discovery will only work, if the request can be routed back to + # SATOSA. + issuer: https://op.example.com/satosa/OIDC client_registration_supported: Yes response_types_supported: ["code", "id_token token"] subject_types_supported: ["pairwise"] diff --git a/src/satosa/frontends/openid_connect.py b/src/satosa/frontends/openid_connect.py index 3b4ba9443..3097c594b 100644 --- a/src/satosa/frontends/openid_connect.py +++ b/src/satosa/frontends/openid_connect.py @@ -62,7 +62,8 @@ def __init__(self, auth_req_callback_func, internal_attributes, conf, base_url, self.config = conf provider_config = self.config["provider"] - provider_config["issuer"] = base_url + if not provider_config.get("issuer"): + provider_config["issuer"] = base_url self.signing_key = RSAKey( key=rsa_load(self.config["signing_key_path"]), diff --git a/tests/satosa/frontends/test_openid_connect.py b/tests/satosa/frontends/test_openid_connect.py index 4331e59a0..5db023dcd 100644 --- a/tests/satosa/frontends/test_openid_connect.py +++ b/tests/satosa/frontends/test_openid_connect.py @@ -44,6 +44,7 @@ EXTRA_SCOPES = { "eduperson": ["eduperson_scoped_affiliation", "eduperson_principal_name"] } +ISSUER = "https://other-op.example.com/satosa/other-op" class TestOpenIDConnectFrontend(object): @pytest.fixture @@ -394,6 +395,16 @@ def test_register_endpoints_handles_path_in_issuer(self, frontend_config, issuer frontend.userinfo_endpoint, ) in urls + def test_discovery_endpoint_honours_issuer_override(self, frontend_config): + frontend_config["provider"]["issuer"] = ISSUER + frontend = self.create_frontend(frontend_config) + discovery_path = urlparse(ISSUER).path[1:] + urls = frontend.register_endpoints(["test"]) + assert ( + "^{}/{}$".format(discovery_path, ".well-known/openid-configuration"), + frontend.provider_config, + ) in urls + def test_register_endpoints_token_and_userinfo_endpoint_is_not_published_if_only_implicit_flow( self, frontend_config, context): frontend_config["provider"]["response_types_supported"] = ["id_token", "id_token token"] From 0c8ab4f8f8fa8e428d356b5afb7aa7587918d8c9 Mon Sep 17 00:00:00 2001 From: Kristof Bajnok Date: Mon, 20 Mar 2023 17:56:50 +0100 Subject: [PATCH 06/11] fixup! frontends/openid_connect: support issuer override via provider --- tests/flows/test_oidc-saml.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/flows/test_oidc-saml.py b/tests/flows/test_oidc-saml.py index 2d019b10e..6e8e8b23b 100644 --- a/tests/flows/test_oidc-saml.py +++ b/tests/flows/test_oidc-saml.py @@ -28,6 +28,7 @@ CLIENT_REDIRECT_URI = "https://client.example.com/cb" REDIRECT_URI = "https://client.example.com/cb" DB_URI = "mongodb://localhost/satosa" +EXTRA_ISSUER = "https://other-op.example.com/satosa/other/op" @pytest.fixture(scope="session") def client_db_path(tmpdir_factory): @@ -104,6 +105,7 @@ def test_full_flow(self, satosa_config_dict, oidc_frontend_config, saml_backend_ subject_id = "testuser1" # proxy config + oidc_frontend_config["config"]["provider"]["issuer"] = EXTRA_ISSUER satosa_config_dict["FRONTEND_MODULES"] = [oidc_frontend_config] satosa_config_dict["BACKEND_MODULES"] = [saml_backend_config] satosa_config_dict["INTERNAL_ATTRIBUTES"]["attributes"] = {attr_name: {"openid": [attr_name], @@ -115,7 +117,7 @@ def test_full_flow(self, satosa_config_dict, oidc_frontend_config, saml_backend_ test_client = Client(make_app(SATOSAConfig(satosa_config_dict)), Response) # get frontend OP config info - issuer = satosa_config_dict["BASE"] + issuer = EXTRA_ISSUER provider_config = self._discover_provider(test_client, issuer) # create auth req From 416d501cc491d5943c800c995f9ad3e00b2c617d Mon Sep 17 00:00:00 2001 From: Kristof Bajnok Date: Tue, 13 Jun 2023 10:29:43 +0200 Subject: [PATCH 07/11] openid_connect_frontend.yaml.example: leverage template Setting an alternative issuer should not be an encouraged setup, although provider discovery should work either way. The recommended setting is to use the BASE as the issuer, and we can leverage the agressive configuration value replacement logic, which rewrites all occurences of to the value of BASE. The unit test was modified to guarantee this behaviour, though. --- .../plugins/frontends/openid_connect_frontend.yaml.example | 6 +----- tests/flows/test_oidc-saml.py | 4 ++-- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/example/plugins/frontends/openid_connect_frontend.yaml.example b/example/plugins/frontends/openid_connect_frontend.yaml.example index c517958f2..7a9e78417 100644 --- a/example/plugins/frontends/openid_connect_frontend.yaml.example +++ b/example/plugins/frontends/openid_connect_frontend.yaml.example @@ -33,11 +33,7 @@ config: sub_hash_salt: randomSALTvalue provider: - # If you do not specify the issuer here, then BASE will be used as Issuer. - # Note that even though this setting must be specified as a full URL, - # provider discovery will only work, if the request can be routed back to - # SATOSA. - issuer: https://op.example.com/satosa/OIDC + issuer: client_registration_supported: Yes response_types_supported: ["code", "id_token token"] subject_types_supported: ["pairwise"] diff --git a/tests/flows/test_oidc-saml.py b/tests/flows/test_oidc-saml.py index 6e8e8b23b..81acef2e4 100644 --- a/tests/flows/test_oidc-saml.py +++ b/tests/flows/test_oidc-saml.py @@ -28,7 +28,7 @@ CLIENT_REDIRECT_URI = "https://client.example.com/cb" REDIRECT_URI = "https://client.example.com/cb" DB_URI = "mongodb://localhost/satosa" -EXTRA_ISSUER = "https://other-op.example.com/satosa/other/op" +EXTRA_ISSUER = "/other/op" @pytest.fixture(scope="session") def client_db_path(tmpdir_factory): @@ -117,7 +117,7 @@ def test_full_flow(self, satosa_config_dict, oidc_frontend_config, saml_backend_ test_client = Client(make_app(SATOSAConfig(satosa_config_dict)), Response) # get frontend OP config info - issuer = EXTRA_ISSUER + issuer = EXTRA_ISSUER.replace("", satosa_config_dict["BASE"]) provider_config = self._discover_provider(test_client, issuer) # create auth req From 740ba28fd633b4dff61e4ae03b4af41abdd8d8a6 Mon Sep 17 00:00:00 2001 From: Kristof Bajnok Date: Mon, 12 Jun 2023 20:02:14 +0200 Subject: [PATCH 08/11] PR #405: apply changes from review Add base_path and endpoint_basepath to backend and micro_services Co-authored-by: Ivan Kanakarakis --- src/satosa/frontends/base.py | 1 + src/satosa/frontends/saml2.py | 10 ++-------- src/satosa/micro_services/account_linking.py | 4 +--- src/satosa/micro_services/base.py | 2 ++ src/satosa/micro_services/consent.py | 4 +--- src/satosa/routing.py | 4 ++-- 6 files changed, 9 insertions(+), 16 deletions(-) diff --git a/src/satosa/frontends/base.py b/src/satosa/frontends/base.py index 08c38b79c..49224c48f 100644 --- a/src/satosa/frontends/base.py +++ b/src/satosa/frontends/base.py @@ -30,6 +30,7 @@ def __init__(self, auth_req_callback_func, internal_attributes, base_url, name): self.internal_attributes = internal_attributes self.converter = AttributeMapper(internal_attributes) self.base_url = base_url or "" + self.base_path = urlparse(self.base_url).path.lstrip("/") self.name = name self.endpoint_baseurl = join_paths(self.base_url, self.name) self.endpoint_basepath = urlparse(self.endpoint_baseurl).path.lstrip("/") diff --git a/src/satosa/frontends/saml2.py b/src/satosa/frontends/saml2.py index 41be65799..aeaa31e55 100644 --- a/src/satosa/frontends/saml2.py +++ b/src/satosa/frontends/saml2.py @@ -512,15 +512,12 @@ def _register_endpoints(self, providers): url_map = [] backend_providers = "|".join(providers) - base_path = urlparse(self.base_url).path.lstrip("/") - if base_path: - base_path = base_path + "/" for endp_category in self.endpoints: for binding, endp in self.endpoints[endp_category].items(): endp_path = urlparse(endp).path url_map.append( ( - "^{}({})/{}$".format(base_path, backend_providers, endp_path), + "^{}/({})/{}$".format(self.base_path, backend_providers, endp_path), functools.partial(self.handle_authn_request, binding_in=binding) ) ) @@ -770,15 +767,12 @@ def _register_endpoints(self, providers): url_map = [] backend_providers = "|".join(providers) - base_path = urlparse(self.base_url).path.lstrip("/") - if base_path: - base_path = base_path + "/" for endp_category in self.endpoints: for binding, endp in self.endpoints[endp_category].items(): endp_path = urlparse(endp).path url_map.append( ( - "^{}({})/\S+/{}$".format(base_path, backend_providers, endp_path), + "^{}/({})/\S+/{}$".format(self.base_path, backend_providers, endp_path), functools.partial(self.handle_authn_request, binding_in=binding) ) ) diff --git a/src/satosa/micro_services/account_linking.py b/src/satosa/micro_services/account_linking.py index 4cfd72c99..304980ff8 100644 --- a/src/satosa/micro_services/account_linking.py +++ b/src/satosa/micro_services/account_linking.py @@ -165,9 +165,7 @@ def register_endpoints(self): return [ ( "^{}$".format( - join_paths( - self.base_path, "account_linking", self.endpoint - ) + join_paths(self.endpoint_basepath, self.endpoint) ), self._handle_al_response, ) diff --git a/src/satosa/micro_services/base.py b/src/satosa/micro_services/base.py index 97271b013..72110daf3 100644 --- a/src/satosa/micro_services/base.py +++ b/src/satosa/micro_services/base.py @@ -16,6 +16,8 @@ def __init__(self, name, base_url, **kwargs): self.name = name self.base_url = base_url self.base_path = urlparse(base_url).path.lstrip("/") + self.endpoint_baseurl = join_paths(self.base_url, self.name) + self.endpoint_basepath = urlparse(self.endpoint_baseurl).path.lstrip("/") self.next = None def process(self, context, data): diff --git a/src/satosa/micro_services/consent.py b/src/satosa/micro_services/consent.py index c273116d5..cf70d4d5f 100644 --- a/src/satosa/micro_services/consent.py +++ b/src/satosa/micro_services/consent.py @@ -242,9 +242,7 @@ def register_endpoints(self): return [ ( "^{}$".format( - join_paths( - self.base_path, "consent", self.endpoint - ) + join_paths(self.endpoint_basepath, self.endpoint) ), self._handle_consent_response, ) diff --git a/src/satosa/routing.py b/src/satosa/routing.py index c9fa8ab8e..d739273ac 100644 --- a/src/satosa/routing.py +++ b/src/satosa/routing.py @@ -38,7 +38,7 @@ class UnknownEndpoint(ValueError): and handles the internal routing between frontends and backends. """ - def __init__(self, frontends, backends, micro_services, base_path=""): + def __init__(self, frontends, backends, micro_services, base_path=None): """ :type frontends: dict[str, satosa.frontends.base.FrontendModule] :type backends: dict[str, satosa.backends.base.BackendModule] @@ -70,7 +70,7 @@ def __init__(self, frontends, backends, micro_services, base_path=""): else: self.micro_services = {} - self.base_path = base_path + self.base_path = base_path if base_path else "" logger.debug("Loaded backends with endpoints: {}".format(backends)) logger.debug("Loaded frontends with endpoints: {}".format(frontends)) From f9753080f9ac7fb10230d5a7eacd4b1e905c125b Mon Sep 17 00:00:00 2001 From: Kristof Bajnok Date: Tue, 13 Jun 2023 12:07:11 +0200 Subject: [PATCH 09/11] fixup! PR #405: apply changes from review --- src/satosa/backends/base.py | 6 +++++ src/satosa/frontends/saml2.py | 23 +++++++++++++++----- src/satosa/micro_services/account_linking.py | 2 +- src/satosa/micro_services/base.py | 2 ++ src/satosa/micro_services/consent.py | 2 +- 5 files changed, 27 insertions(+), 8 deletions(-) diff --git a/src/satosa/backends/base.py b/src/satosa/backends/base.py index 381902eee..d18dfc4d6 100644 --- a/src/satosa/backends/base.py +++ b/src/satosa/backends/base.py @@ -3,6 +3,9 @@ """ from ..attribute_mapping import AttributeMapper +from ..util import join_paths + +from urllib.parse import urlparse class BackendModule(object): @@ -30,7 +33,10 @@ def __init__(self, auth_callback_func, internal_attributes, base_url, name): self.internal_attributes = internal_attributes self.converter = AttributeMapper(internal_attributes) self.base_url = base_url.rstrip("/") if base_url else "" + self.base_path = urlparse(self.base_url).path.lstrip("/") self.name = name + self.endpoint_baseurl = join_paths(self.base_url, self.name) + self.endpoint_basepath = urlparse(self.endpoint_baseurl).path.lstrip("/") def start_auth(self, context, internal_request): """ diff --git a/src/satosa/frontends/saml2.py b/src/satosa/frontends/saml2.py index aeaa31e55..2b8a1149a 100644 --- a/src/satosa/frontends/saml2.py +++ b/src/satosa/frontends/saml2.py @@ -33,6 +33,7 @@ from ..response import Response from ..response import ServiceError from ..saml_util import make_saml_response +from ..util import join_paths from satosa.exception import SATOSAError import satosa.util as util @@ -511,14 +512,18 @@ def _register_endpoints(self, providers): """ url_map = [] - backend_providers = "|".join(providers) + backend_providers = "(" + "|".join(providers) + ")" for endp_category in self.endpoints: for binding, endp in self.endpoints[endp_category].items(): endp_path = urlparse(endp).path url_map.append( ( - "^{}/({})/{}$".format(self.base_path, backend_providers, endp_path), - functools.partial(self.handle_authn_request, binding_in=binding) + "^{}$".format( + join_paths(self.base_path, backend_providers, endp_path) + ), + functools.partial( + self.handle_authn_request, binding_in=binding + ), ) ) @@ -766,14 +771,20 @@ def _register_endpoints(self, providers): """ url_map = [] - backend_providers = "|".join(providers) + backend_providers = "(" + "|".join(providers) + ")" for endp_category in self.endpoints: for binding, endp in self.endpoints[endp_category].items(): endp_path = urlparse(endp).path url_map.append( ( - "^{}/({})/\S+/{}$".format(self.base_path, backend_providers, endp_path), - functools.partial(self.handle_authn_request, binding_in=binding) + "^{}$".format( + join_paths( + self.base_path, backend_providers, "\S+", endp_path + ) + ), + functools.partial( + self.handle_authn_request, binding_in=binding + ), ) ) diff --git a/src/satosa/micro_services/account_linking.py b/src/satosa/micro_services/account_linking.py index 304980ff8..bebd77b7f 100644 --- a/src/satosa/micro_services/account_linking.py +++ b/src/satosa/micro_services/account_linking.py @@ -165,7 +165,7 @@ def register_endpoints(self): return [ ( "^{}$".format( - join_paths(self.endpoint_basepath, self.endpoint) + join_paths(self.base_path, "account_linking", self.endpoint) ), self._handle_al_response, ) diff --git a/src/satosa/micro_services/base.py b/src/satosa/micro_services/base.py index 72110daf3..b31baf9b4 100644 --- a/src/satosa/micro_services/base.py +++ b/src/satosa/micro_services/base.py @@ -4,6 +4,8 @@ import logging from urllib.parse import urlparse +from ..util import join_paths + logger = logging.getLogger(__name__) diff --git a/src/satosa/micro_services/consent.py b/src/satosa/micro_services/consent.py index cf70d4d5f..fbe8e75dc 100644 --- a/src/satosa/micro_services/consent.py +++ b/src/satosa/micro_services/consent.py @@ -242,7 +242,7 @@ def register_endpoints(self): return [ ( "^{}$".format( - join_paths(self.endpoint_basepath, self.endpoint) + join_paths(self.base_path, "consent", self.endpoint) ), self._handle_consent_response, ) From f1668691274f3ad34c452092b22caf92da87ad8b Mon Sep 17 00:00:00 2001 From: Kristof Bajnok Date: Thu, 15 Jun 2023 09:08:27 +0200 Subject: [PATCH 10/11] fixup! amend! fixup! ModuleRouter: support paths in BASE --- src/satosa/util.py | 33 +++++++++++++++------------------ tests/satosa/test_util.py | 30 +++++++++++++++++++----------- 2 files changed, 34 insertions(+), 29 deletions(-) diff --git a/src/satosa/util.py b/src/satosa/util.py index b171f1f3e..ea1728853 100644 --- a/src/satosa/util.py +++ b/src/satosa/util.py @@ -5,6 +5,7 @@ import logging import random import string +import typing logger = logging.getLogger(__name__) @@ -91,27 +92,23 @@ def rndstr(size=16, alphabet=""): return type(alphabet)().join(rng.choice(alphabet) for _ in range(size)) -def join_paths(base, *paths): +def join_paths(*paths, sep: typing.Optional[str] = None) -> str: """ - Joins strings with a "/" separator, like they were path components, but - tries to avoid adding an unnecessary separator. Note that the contents of - the strings are not sanitized in any way. If any of the components begins or - ends with a "/", the separator is not inserted, and any number of empty - strings at the beginning would not add a leading slash. Any number of empty - strings at the end only add a single trailing slash. - - Raises TypeError if any of the components are not strings. + Joins strings with a separator like they were path components. The + separator is stripped off from all path components, except for the + beginning of the first component. Empty (or falsy) components are skipped. + Note that the components are not sanitized in any other way. + + Raises TypeError if any of the components are not strings (or empty). """ - sep = "/" + sep = sep or "/" + leading = "" + if paths and paths[0] and paths[0][0] == sep: + leading = sep - path = base try: - for p in paths: - if not path or path.endswith(sep) or p.startswith(sep): - path += p - else: - path += sep + p + return leading + sep.join( + [path.strip(sep) for path in filter(lambda p: p and p.strip(sep), paths)] + ) except (AttributeError, TypeError) as err: raise TypeError("Arguments must be strings") from err - - return path diff --git a/tests/satosa/test_util.py b/tests/satosa/test_util.py index 893534b89..e74c9f842 100644 --- a/tests/satosa/test_util.py +++ b/tests/satosa/test_util.py @@ -12,23 +12,27 @@ (["foo", "/bar"], "foo/bar"), (["/foo", "baz", "/bar"], "/foo/baz/bar"), (["", "foo", "bar"], "foo/bar"), - (["", "/foo", "bar"], "/foo/bar"), - (["", "/foo/", "bar"], "/foo/bar"), - (["", "", "", "/foo", "bar"], "/foo/bar"), - (["", "", "/foo/", "", "bar"], "/foo/bar"), - (["", "", "/foo/", "", "", "bar/"], "/foo/bar/"), - (["/foo", ""], "/foo/"), - (["/foo", "", "", ""], "/foo/"), - (["/foo//", "bar"], "/foo//bar"), + (["", "/foo", "bar"], "foo/bar"), + (["", "/foo/", "bar"], "foo/bar"), + (["", "", "", "/foo", "bar"], "foo/bar"), + (["", "", "/foo/", "", "bar"], "foo/bar"), + (["", "", "/foo/", "", "", "bar/"], "foo/bar"), + (["/foo", ""], "/foo"), + (["/foo", "", "", ""], "/foo"), + (["/foo//", "bar"], "/foo/bar"), (["foo"], "foo"), ([""], ""), (["", ""], ""), (["'not ", "sanitized'\0/; rm -rf *"], "'not /sanitized'\0/; rm -rf *"), - (["foo/", "/bar"], "foo//bar"), - (["foo", "", "/bar"], "foo//bar"), + (["foo/", "/bar"], "foo/bar"), + (["foo", "", "/bar"], "foo/bar"), ([b"foo", "bar"], TypeError), (["foo", b"bar"], TypeError), - ([None, "foo"], TypeError), + ([None, "foo"], "foo"), + (["foo", [], "bar"], "foo/bar"), + (["foo", ["baz"], "bar"], TypeError), + (["/", "foo", "bar"], "/foo/bar"), + (["///foo", "bar"], "/foo/bar"), ], ) def test_join_paths(args, expected): @@ -37,3 +41,7 @@ def test_join_paths(args, expected): else: with pytest.raises(expected): _ = join_paths(*args) + + +def test_join_paths_with_separator(): + assert join_paths("this", "is", "not", "a", "path", sep="|") == "this|is|not|a|path" From e25c9e0fc071e2034629bbafacbaf7ef8a37963e Mon Sep 17 00:00:00 2001 From: Kristof Bajnok Date: Thu, 15 Jun 2023 09:17:31 +0200 Subject: [PATCH 11/11] fixup! fixup! amend! fixup! ModuleRouter: support paths in BASE --- src/satosa/util.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/satosa/util.py b/src/satosa/util.py index ea1728853..75ec1f6fc 100644 --- a/src/satosa/util.py +++ b/src/satosa/util.py @@ -108,7 +108,7 @@ def join_paths(*paths, sep: typing.Optional[str] = None) -> str: try: return leading + sep.join( - [path.strip(sep) for path in filter(lambda p: p and p.strip(sep), paths)] + path.strip(sep) for path in filter(lambda p: p and p.strip(sep), paths) ) except (AttributeError, TypeError) as err: raise TypeError("Arguments must be strings") from err