fix(auth): adjust tests with unallowed properties

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,

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,
     }
 

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):

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"],

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"],