diff --git a/CHANGELOG.md b/CHANGELOG.md index 459906df..8ba3b5d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Access to authenticated endpoints requires the "early adopter" role now (through EGI Check-in `eduPersonEntitlement`) (EP-3969) - Work with aggregator-specific OIDC provider settings (e.g. dedicated default clients) (EP-4046, [#7](https://github.com/Open-EO/openeo-aggregator/issues/7)) - Disable `egi-dev` OIDC provider (EP-4046, [#7](https://github.com/Open-EO/openeo-aggregator/issues/7)) +- Improve "early adopter check" error messages ([openEOPlatform/architecture-docs#105](https://github.com/openEOPlatform/architecture-docs/issues/105)) ### Fixed diff --git a/setup.py b/setup.py index 12543621..6c324d74 100644 --- a/setup.py +++ b/setup.py @@ -22,7 +22,7 @@ install_requires=[ "requests", "openeo>=0.7.1a1.*", - "openeo_driver>=0.14.2a1.*", + "openeo_driver>=0.14.3a1.*", "flask~=2.0", "gunicorn~=20.0", ], diff --git a/src/openeo_aggregator/backend.py b/src/openeo_aggregator/backend.py index 2f9d6cdc..159b1a80 100644 --- a/src/openeo_aggregator/backend.py +++ b/src/openeo_aggregator/backend.py @@ -14,7 +14,7 @@ from openeo_aggregator.connection import MultiBackendConnection, BackendConnection, streaming_flask_response from openeo_aggregator.egi import is_early_adopter from openeo_aggregator.errors import BackendLookupFailureException -from openeo_aggregator.utils import TtlCache, MultiDictGetter +from openeo_aggregator.utils import TtlCache, MultiDictGetter, subdict from openeo_driver.ProcessGraphDeserializer import SimpleProcessing from openeo_driver.backend import OpenEoBackendImplementation, AbstractCollectionCatalog, LoadParameters, Processing, \ OidcProvider, BatchJobs, BatchJobMetadata @@ -491,7 +491,7 @@ def __init__(self, backends: MultiBackendConnection, config: AggregatorConfig): user_defined_processes=None, ) self._cache = TtlCache(default_ttl=CACHE_TTL_DEFAULT) - self._auth_entitlement_check = config.auth_entitlement_check + self._auth_entitlement_check: Union[bool, dict] = config.auth_entitlement_check self._configured_oidc_providers: List[OidcProvider] = config.configured_oidc_providers def oidc_providers(self) -> List[OidcProvider]: @@ -528,16 +528,28 @@ def merge(formats: dict, to_add: dict): def user_access_validation(self, user: User, request: flask.Request) -> User: if self._auth_entitlement_check: - if user.internal_auth_data["authentication_method"] != "OIDC": - raise PermissionsInsufficientException("OIDC auth required") + base_error_message = "Not a valid openEO Platform user" + int_data = user.internal_auth_data + issuer_whitelist = self._auth_entitlement_check.get("oidc_issuer_whitelist", {"https://aai.egi.eu/oidc"}) + if not ( + int_data["authentication_method"] == "OIDC" + and int_data["oidc_issuer"].rstrip("/").lower() in issuer_whitelist + ): + message = f"{base_error_message}: OIDC authentication with EGI Check-in is required." + debug_info = subdict(int_data, keys=["authentication_method", "oidc_issuer"]) + _log.warning(f"{message} debug_info:{debug_info} whitelist:{issuer_whitelist}") + raise PermissionsInsufficientException(message) try: eduperson_entitlements = user.info["oidc_userinfo"]["eduperson_entitlement"] except KeyError as e: - _log.warning(f"No eduperson_entitlement data ({e!r})") - raise PermissionsInsufficientException("No eduperson_entitlement data") + message = f"{base_error_message}: missing entitlement data." + # Note: just log userinfo keys to avoid leaking sensitive user data. + _log.warning(f"{message} {e!r} {user.info.keys()} {user.info.get('oidc_userinfo', {}).keys()}") + raise PermissionsInsufficientException(message) if not any(is_early_adopter(e) for e in eduperson_entitlements): - _log.warning(f"User {user.user_id} has no early adopter role: {eduperson_entitlements}") - raise PermissionsInsufficientException("No early adopter role") + message = f"{base_error_message}: no early adopter role." + _log.warning(f"{message} user:{user.user_id} entitlements:{eduperson_entitlements}") + raise PermissionsInsufficientException(message) # TODO: list multiple roles/levels? Better "status" signaling? user.info["roles"] = ["EarlyAdopter"] diff --git a/src/openeo_aggregator/config.py b/src/openeo_aggregator/config.py index 807db181..2d3fe69f 100644 --- a/src/openeo_aggregator/config.py +++ b/src/openeo_aggregator/config.py @@ -34,7 +34,7 @@ class AggregatorConfig(dict): # TODO: add validation/normalization to make sure we have a real list of OidcProvider objects? configured_oidc_providers: List[OidcProvider] = dict_item(default=[]) - auth_entitlement_check = dict_item(default=True) + auth_entitlement_check: Union[bool, dict] = dict_item() @classmethod def from_json(cls, data: str): @@ -67,7 +67,7 @@ def from_json_file(cls, path: Union[str, Path]): "eodc": "https://openeo.eodc.eu/v1.0/", # "eodcdev": "https://openeo-dev.eodc.eu/v1.0/", }, - auth_entitlement_check=True, + auth_entitlement_check={"oidc_issuer_whitelist": {"https://aai.egi.eu/oidc"}}, configured_oidc_providers=[ OidcProvider( id="egi", diff --git a/src/openeo_aggregator/utils.py b/src/openeo_aggregator/utils.py index a790fba4..e5083201 100644 --- a/src/openeo_aggregator/utils.py +++ b/src/openeo_aggregator/utils.py @@ -104,3 +104,11 @@ def first(self, key, default=None): def select(self, key: str) -> 'MultiDictGetter': """Create new getter, one step deeper in the dictionary hierarchy.""" return MultiDictGetter(d for d in self.get(key=key) if isinstance(d, dict)) + + +def subdict(d: dict, *args, keys: list = None, default=None) -> dict: + """Extract dict with only selected keys from given dict""" + keys = set(keys or []) | set(args) + # TODO: way to not provide default and raise KeyError on missing keys + # TODO: move to openeo-python-driver? + return {k: d.get(k, default) for k in keys} diff --git a/tests/conftest.py b/tests/conftest.py index 93c4b00c..c42e8b35 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -81,7 +81,7 @@ def api100(flask_app: flask.Flask) -> ApiTester: @pytest.fixture def api100_with_entitlement_check(config: AggregatorConfig) -> ApiTester: - config.auth_entitlement_check = True + config.auth_entitlement_check = {"oidc_issuer_whitelist": {"https://egi.test", "https://egi.test/oidc"}} return get_api100(get_flask_app(config)) diff --git a/tests/test_utils.py b/tests/test_utils.py index dd224aa6..2b277227 100644 --- a/tests/test_utils.py +++ b/tests/test_utils.py @@ -1,6 +1,6 @@ import pytest -from openeo_aggregator.utils import TtlCache, CacheMissException, MultiDictGetter +from openeo_aggregator.utils import TtlCache, CacheMissException, MultiDictGetter, subdict class FakeClock: @@ -129,3 +129,15 @@ def test_select(self): assert list(getter.select("b").select("ba").get("x")) == [] assert list(getter.select("b").select("bb").get("bba")) == [5] assert list(getter.select("x").select("y").select("z").get("z")) == [] + + +def test_subdict(): + d = {"foo": "bar", "meh": 3} + assert subdict(d) == {} + assert subdict(d, "foo") == {"foo": "bar"} + assert subdict(d, "foo", "meh") == {"foo": "bar", "meh": 3} + assert subdict(d, "foo", "bar") == {"foo": "bar", "bar": None} + assert subdict(d, keys=["foo"]) == {"foo": "bar"} + assert subdict(d, keys=["foo", "meh"]) == {"foo": "bar", "meh": 3} + assert subdict(d, keys=["foo", "bar"]) == {"foo": "bar", "bar": None} + assert subdict(d, "foo", keys=["meh", "bar"]) == {"foo": "bar", "meh": 3, "bar": None} diff --git a/tests/test_views.py b/tests/test_views.py index 85f523d7..d9e37866 100644 --- a/tests/test_views.py +++ b/tests/test_views.py @@ -101,7 +101,10 @@ class TestAuthEntitlementCheck: def test_basic_auth(self, api100_with_entitlement_check): api100_with_entitlement_check.set_auth_bearer_token(token=TEST_USER_BEARER_TOKEN) res = api100_with_entitlement_check.get("/me") - res.assert_error(403, "PermissionsInsufficient", message="OIDC auth required") + res.assert_error( + 403, "PermissionsInsufficient", + message="Not a valid openEO Platform user: OIDC authentication with EGI Check-in is required." + ) def test_oidc_no_entitlement_data(self, api100_with_entitlement_check, requests_mock): def get_userinfo(request: requests.Request, context): @@ -115,7 +118,10 @@ def get_userinfo(request: requests.Request, context): api100_with_entitlement_check.set_auth_bearer_token(token="oidc/egi/funiculifunicula") res = api100_with_entitlement_check.get("/me") - res.assert_error(403, "PermissionsInsufficient", message="No eduperson_entitlement data") + res.assert_error( + 403, "PermissionsInsufficient", + message="Not a valid openEO Platform user: missing entitlement data." + ) def test_oidc_no_early_adopter(self, api100_with_entitlement_check, requests_mock): def get_userinfo(request: requests.Request, context): @@ -135,7 +141,10 @@ def get_userinfo(request: requests.Request, context): api100_with_entitlement_check.set_auth_bearer_token(token="oidc/egi/funiculifunicula") res = api100_with_entitlement_check.get("/me") - res.assert_error(403, "PermissionsInsufficient", message="No early adopter role") + res.assert_error( + 403, "PermissionsInsufficient", + message="Not a valid openEO Platform user: no early adopter role." + ) def test_oidc_early_adopter(self, api100_with_entitlement_check, requests_mock): def get_userinfo(request: requests.Request, context):