From 6f367b8035e5b8cca5ec79b77ea117122e3899f9 Mon Sep 17 00:00:00 2001 From: aldbr Date: Tue, 17 Sep 2024 16:52:46 +0200 Subject: [PATCH] fix(auth): adjust tests with unallowed properties --- .../src/diracx/routers/auth/token.py | 26 +-- .../src/diracx/routers/auth/utils.py | 8 +- .../tests/auth/test_legacy_exchange.py | 4 +- diracx-routers/tests/auth/test_standard.py | 151 +++++++++++++++--- diracx-testing/src/diracx/testing/__init__.py | 4 + 5 files changed, 152 insertions(+), 41 deletions(-) diff --git a/diracx-routers/src/diracx/routers/auth/token.py b/diracx-routers/src/diracx/routers/auth/token.py index b64e792d..14103add 100644 --- a/diracx-routers/src/diracx/routers/auth/token.py +++ b/diracx-routers/src/diracx/routers/auth/token.py @@ -98,7 +98,6 @@ async def token( raise NotImplementedError(f"Grant type not implemented {grant_type}") # Get a TokenResponse to return to the user - return await exchange_token( auth_db, scope, @@ -360,7 +359,13 @@ async def exchange_token( ) -> TokenResponse: """Method called to exchange the OIDC token for a DIRAC generated access token.""" # Extract dirac attributes from the OIDC scope - parsed_scope = parse_and_validate_scope(scope, config, available_properties) + try: + parsed_scope = parse_and_validate_scope(scope, config, available_properties) + except ValueError as e: + raise HTTPException( + status_code=status.HTTP_400_BAD_REQUEST, + detail=e.args[0], + ) from e vo = parsed_scope["vo"] dirac_group = parsed_scope["group"] properties = parsed_scope["properties"] @@ -380,15 +385,18 @@ async def exchange_token( # Check that the subject is part of the dirac users if sub not in config.Registry[vo].Groups[dirac_group].Users: - raise ValueError( - f"User is not a member of the requested group ({preferred_username}, {dirac_group})" + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail=f"User is not a member of the requested group ({preferred_username}, {dirac_group})", ) + # Check that the user properties are valid allowed_user_properties = get_allowed_user_properties(config, sub, vo) - if not set(properties).issubset(allowed_user_properties): - raise ValueError( - f"{set(properties) - allowed_user_properties} are not valid properties for user {preferred_username}, " - f"available values: {' '.join(allowed_user_properties)}" + if not properties.issubset(allowed_user_properties): + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail=f"{' '.join(properties - allowed_user_properties)} are not valid properties " + f"for user {preferred_username}, available values: {' '.join(allowed_user_properties)}", ) # Merge the VO with the subject to get a unique DIRAC sub @@ -420,7 +428,7 @@ async def exchange_token( "sub": sub, "vo": vo, "iss": issuer, - "dirac_properties": parsed_scope["properties"], + "dirac_properties": list(properties), "jti": str(uuid4()), "preferred_username": preferred_username, "dirac_group": dirac_group, diff --git a/diracx-routers/src/diracx/routers/auth/utils.py b/diracx-routers/src/diracx/routers/auth/utils.py index f4f50f3c..3b881361 100644 --- a/diracx-routers/src/diracx/routers/auth/utils.py +++ b/diracx-routers/src/diracx/routers/auth/utils.py @@ -36,7 +36,7 @@ class GrantType(StrEnum): class ScopeInfoDict(TypedDict): group: str - properties: list[str] + properties: set[str] vo: str @@ -217,9 +217,7 @@ def parse_and_validate_scope( raise ValueError(f"{group} not in {vo} groups") allowed_properties = config.Registry[vo].Groups[group].Properties - if not properties: - # If there are no properties set get the defaults from the CS - properties = [str(p) for p in allowed_properties] + properties.extend([str(p) for p in allowed_properties]) if not set(properties).issubset(available_properties): raise ValueError( @@ -228,7 +226,7 @@ def parse_and_validate_scope( return { "group": group, - "properties": sorted(properties), + "properties": set(sorted(properties)), "vo": vo, } diff --git a/diracx-routers/tests/auth/test_legacy_exchange.py b/diracx-routers/tests/auth/test_legacy_exchange.py index 62a4acdf..551e3133 100644 --- a/diracx-routers/tests/auth/test_legacy_exchange.py +++ b/diracx-routers/tests/auth/test_legacy_exchange.py @@ -79,7 +79,9 @@ async def test_valid(test_client, legacy_credentials, expires_seconds): assert user_info["sub"] == "lhcb:b824d4dc-1f9d-4ee8-8df5-c0ae55d46041" assert user_info["vo"] == "lhcb" assert user_info["dirac_group"] == "lhcb_user" - assert user_info["properties"] == ["NormalUser", "PrivateLimitedDelegation"] + assert sorted(user_info["properties"]) == sorted( + ["PrivateLimitedDelegation", "NormalUser"] + ) async def test_refresh_token(test_client, legacy_credentials): diff --git a/diracx-routers/tests/auth/test_standard.py b/diracx-routers/tests/auth/test_standard.py index 58b35500..4f4a24d4 100644 --- a/diracx-routers/tests/auth/test_standard.py +++ b/diracx-routers/tests/auth/test_standard.py @@ -115,6 +115,12 @@ async def test_authorization_flow(test_client, auth_httpx_mock: HTTPXMock): .replace("=", "") ) + # The scope is valid and should return a token with the following claims + # vo:lhcb group:lhcb_user (default group) property:[NormalUser,ProductionManagement] + # Note: the property ProductionManagement is not part of the lhcb_user group properties + # but the user has the right to have it. + scope = "vo:lhcb property:ProductionManagement" + # Initiate the authorization flow with a wrong client ID # Check that the client ID is not recognized r = test_client.get( @@ -125,7 +131,7 @@ async def test_authorization_flow(test_client, auth_httpx_mock: HTTPXMock): "code_challenge_method": "S256", "client_id": "Unknown client ID", "redirect_uri": "http://diracx.test.invalid:8000/api/docs/oauth2-redirect", - "scope": "vo:lhcb property:NormalUser", + "scope": scope, "state": "external-state", }, follow_redirects=False, @@ -142,7 +148,7 @@ async def test_authorization_flow(test_client, auth_httpx_mock: HTTPXMock): "code_challenge_method": "S256", "client_id": DIRAC_CLIENT_ID, "redirect_uri": "http://diracx.test.unrecognized:8000/api/docs/oauth2-redirect", - "scope": "vo:lhcb property:NormalUser", + "scope": scope, "state": "external-state", }, follow_redirects=False, @@ -158,7 +164,7 @@ async def test_authorization_flow(test_client, auth_httpx_mock: HTTPXMock): "code_challenge_method": "S256", "client_id": DIRAC_CLIENT_ID, "redirect_uri": "http://diracx.test.invalid:8000/api/docs/oauth2-redirect", - "scope": "vo:lhcb property:NormalUser", + "scope": scope, "state": "external-state", }, follow_redirects=False, @@ -237,13 +243,19 @@ async def test_authorization_flow(test_client, auth_httpx_mock: HTTPXMock): async def test_device_flow(test_client, auth_httpx_mock: HTTPXMock): + # The scope is valid and should return a token with the following claims + # vo:lhcb group:lhcb_user (default group) property:[NormalUser,ProductionManagement] + # Note: the property ProductionManagement is not part of the lhcb_user group properties + # but the user has the right to have it. + scope = "vo:lhcb property:ProductionManagement" + # Initiate the device flow with a wrong client ID # Check that the client ID is not recognized r = test_client.post( "/api/auth/device", params={ "client_id": "Unknown client ID", - "scope": "vo:lhcb property:NormalUser", + "scope": scope, }, ) assert r.status_code == 400, r.json() @@ -253,7 +265,7 @@ async def test_device_flow(test_client, auth_httpx_mock: HTTPXMock): "/api/auth/device", params={ "client_id": DIRAC_CLIENT_ID, - "scope": "vo:lhcb group:lhcb_user property:NormalUser", + "scope": scope, }, ) assert r.status_code == 200, r.json() @@ -331,11 +343,14 @@ async def test_device_flow(test_client, auth_httpx_mock: HTTPXMock): assert r.json()["detail"] == "Code was already used" -async def test_flows_with_unallowed_properties(test_client): +async def test_authorization_flow_with_unallowed_properties( + test_client, auth_httpx_mock: HTTPXMock +): """Test the authorization flow and the device flow with unallowed properties.""" - unallowed_property = "FileCatalogManagement" + # ProxyManagement is a valid property but not allowed for the user + unallowed_property = "ProxyManagement" - # Initiate the authorization flow + # Initiate the authorization flow: should not fail code_verifier = secrets.token_hex() code_challenge = ( base64.urlsafe_b64encode(hashlib.sha256(code_verifier.encode()).digest()) @@ -355,12 +370,43 @@ async def test_flows_with_unallowed_properties(test_client): }, follow_redirects=False, ) + assert r.status_code == 307, r.json() + query_parameters = parse_qs(urlparse(r.headers["Location"]).query) + redirect_uri = query_parameters["redirect_uri"][0] + state = query_parameters["state"][0] + + r = test_client.get( + redirect_uri, + params={"code": "valid-code", "state": state}, + follow_redirects=False, + ) + assert r.status_code == 307, r.text + query_parameters = parse_qs(urlparse(r.headers["Location"]).query) + code = query_parameters["code"][0] + + request_data = { + "grant_type": "authorization_code", + "code": code, + "state": state, + "client_id": DIRAC_CLIENT_ID, + "redirect_uri": "http://diracx.test.invalid:8000/api/docs/oauth2-redirect", + "code_verifier": code_verifier, + } + # Ensure the token request doesn't work because of the unallowed property + r = test_client.post("/api/auth/token", data=request_data) assert r.status_code == 403, r.json() assert ( - f"Attempted to access properties {{'{unallowed_property}'}} which are not allowed." - in r.json()["detail"] + f"{unallowed_property} are not valid properties for user" in r.json()["detail"] ) + +async def test_device_flow_with_unallowed_properties( + test_client, auth_httpx_mock: HTTPXMock +): + """Test the authorization flow and the device flow with unallowed properties.""" + # ProxyManagement is a valid property but not allowed for the user + unallowed_property = "ProxyManagement" + # Initiate the device flow r = test_client.post( "/api/auth/device", @@ -369,10 +415,36 @@ async def test_flows_with_unallowed_properties(test_client): "scope": f"vo:lhcb group:lhcb_user property:{unallowed_property} property:NormalUser", }, ) + assert r.status_code == 200, r.json() + + data = r.json() + assert data["user_code"] + assert data["device_code"] + assert data["verification_uri_complete"] + assert data["verification_uri"] + assert data["expires_in"] == 600 + + r = test_client.get(data["verification_uri_complete"], follow_redirects=False) + assert r.status_code == 307, r.text + login_url = r.headers["Location"] + query_parameters = parse_qs(urlparse(login_url).query) + redirect_uri = query_parameters["redirect_uri"][0] + state = query_parameters["state"][0] + + r = test_client.get(redirect_uri, params={"code": "valid-code", "state": state}) + assert r.status_code == 200, r.text + + request_data = { + "grant_type": "urn:ietf:params:oauth:grant-type:device_code", + "device_code": data["device_code"], + "client_id": DIRAC_CLIENT_ID, + } + + # Ensure the token request doesn't work a second time + r = test_client.post("/api/auth/token", data=request_data) assert r.status_code == 403, r.json() assert ( - f"Attempted to access properties {{'{unallowed_property}'}} which are not allowed." - in r.json()["detail"] + f"{unallowed_property} are not valid properties for user" in r.json()["detail"] ) @@ -787,40 +859,61 @@ def _get_and_check_token_response(test_client, request_data): @pytest.mark.parametrize( "vos, groups, scope, expected", [ - # Classic use case, we ask for a vo and a group, we get the properties of the group + # We ask for a vo, we get the properties of the default group + [ + {"lhcb": {"default_group": "lhcb_user"}}, + { + "lhcb_user": {"properties": ["NormalUser"]}, + "lhcb_admin": {"properties": ["ProxyManagement"]}, + "lhcb_production": {"properties": ["ProductionManagement"]}, + }, + "vo:lhcb", + {"group": "lhcb_user", "properties": {"NormalUser"}, "vo": "lhcb"}, + ], + # We ask for a vo and a group, we get the properties of the group [ {"lhcb": {"default_group": "lhcb_user"}}, - {"lhcb_user": {"properties": ["NormalUser"]}}, - "vo:lhcb group:lhcb_user", - {"group": "lhcb_user", "properties": ["NormalUser"], "vo": "lhcb"}, + { + "lhcb_user": {"properties": ["NormalUser"]}, + "lhcb_admin": {"properties": ["ProxyManagement"]}, + "lhcb_production": {"properties": ["ProductionManagement"]}, + }, + "vo:lhcb group:lhcb_admin", + {"group": "lhcb_admin", "properties": {"ProxyManagement"}, "vo": "lhcb"}, ], - # We ask for a vo and a group with additional property - # We get the properties of the group + the additional property + # We ask for a vo, no group, and an additional existing property + # We get the default group with its properties along with with the extra properties we asked for # Authorization to access the additional property is checked later when user effectively requests a token [ {"lhcb": {"default_group": "lhcb_user"}}, { "lhcb_user": {"properties": ["NormalUser"]}, - "lhcb_admin": {"properties": ["AdminUser"]}, + "lhcb_admin": {"properties": ["ProxyManagement"]}, + "lhcb_production": {"properties": ["ProductionManagement"]}, }, - "vo:lhcb group:lhcb_user property:AdminUser", + "vo:lhcb property:ProxyManagement", { "group": "lhcb_user", - "properties": ["NormalUser", "AdminUser"], + "properties": {"NormalUser", "ProxyManagement"}, "vo": "lhcb", }, ], - # We ask for a vo, no group, and an additional existing property - # We get the default group but not its properties: only the one we asked for + # We ask for a vo and a group with additional property + # We get the properties of the group + the additional property # Authorization to access the additional property is checked later when user effectively requests a token [ {"lhcb": {"default_group": "lhcb_user"}}, { "lhcb_user": {"properties": ["NormalUser"]}, - "lhcb_admin": {"properties": ["AdminUser"]}, + "lhcb_admin": {"properties": ["ProxyManagement"]}, + "lhcb_production": {"properties": ["ProductionManagement"]}, + }, + "vo:lhcb group:lhcb_admin property:ProductionManagement", + { + "group": "lhcb_admin", + "properties": {"ProductionManagement", "ProxyManagement"}, + "vo": "lhcb", }, - "vo:lhcb property:AdminUser", - {"group": "lhcb_user", "properties": ["AdminUser"], "vo": "lhcb"}, ], ], ) @@ -859,6 +952,12 @@ def test_parse_scopes(vos, groups, scope, expected): "group:lhcb_user undefinedscope:undefined", "Unrecognised scopes", ], + [ + ["lhcb"], + ["lhcb_user", "lhcb_admin"], + "vo:lhcb group:lhcb_user property:undefined_property", + "{'undefined_property'} are not valid properties", + ], [ ["lhcb"], ["lhcb_user"], diff --git a/diracx-testing/src/diracx/testing/__init__.py b/diracx-testing/src/diracx/testing/__init__.py index 373e3310..9b38c787 100644 --- a/diracx-testing/src/diracx/testing/__init__.py +++ b/diracx-testing/src/diracx/testing/__init__.py @@ -443,6 +443,10 @@ def with_config_repo(tmp_path_factory): "c935e5ed-2g0e-5ff9-9eg6-d1bf66e57152", ], }, + "lhcb_prmgr": { + "Properties": ["NormalUser", "ProductionManagement"], + "Users": ["b824d4dc-1f9d-4ee8-8df5-c0ae55d46041"], + }, "lhcb_tokenmgr": { "Properties": ["NormalUser", "ProxyManagement"], "Users": ["c935e5ed-2g0e-5ff9-9eg6-d1bf66e57152"],