diff --git a/oidc-controller/api/routers/oidc.py b/oidc-controller/api/routers/oidc.py index 280e113c..680d35bb 100644 --- a/oidc-controller/api/routers/oidc.py +++ b/oidc-controller/api/routers/oidc.py @@ -376,15 +376,21 @@ async def post_token(request: Request, db: Database = Depends(get_db)): ), ) + # CRITICAL: For StatelessWrapper (in-memory storage), claims MUST be + # stored INSIDE the authorization code in authz_info["user_info"] before + # packing, otherwise PyOP will retrieve empty claims when generating the + # ID token. This must happen regardless of whether claims contain a "sub" + # field. For RedisWrapper, this field is not used (claims are in Redis). + authz_info = provider.provider.authz_state.authorization_codes[ + form_dict["code"] + ] + authz_info["user_info"] = claims + # Replace auto-generated sub with one coming from proof, if available # The Redis storage uses a shared data store, so a new item can be # added and the reference in the form needs to be updated with the # new key value if claims.get("sub"): - authz_info = provider.provider.authz_state.authorization_codes[ - form_dict["code"] - ] - # Update the subject with the one from the presentation presentation_sub = claims.pop("sub") @@ -408,29 +414,14 @@ async def post_token(request: Request, db: Database = Depends(get_db)): # Update our local reference to the new user_id user_id = presentation_sub - # CRITICAL: For StatelessWrapper (in-memory storage), claims are - # stored INSIDE the authorization code in authz_info["user_info"]. - # We must update this with the actual claims before repacking, - # otherwise PyOP will retrieve empty claims when generating the ID token. - # For RedisWrapper, this field is not used (claims are in Redis). - authz_info["user_info"] = claims - # Create subject identifier mapping: subject -> user_id # This is needed because PyOP will look up the user_id for this # subject. Store the public subject identifier with Redis-aware # persistence store_subject_identifier(user_id, "public", presentation_sub) - new_code = provider.provider.authz_state.authorization_codes.pack( - authz_info - ) - form_dict["code"] = new_code - - # NOTE: Do NOT add sub back to claims dict. PyOP will get the - # sub from authz_info["sub"] when generating the ID token. - # If we include sub in claims as well, PyOP will receive it - # twice and raise: "TypeError: IdToken() got multiple values - # for keyword argument 'sub'" + # Need to update user_info again after removing "sub" from claims + authz_info["user_info"] = claims logger.info( "Replaced authorization code with presentation subject", @@ -442,6 +433,24 @@ async def post_token(request: Request, db: Database = Depends(get_db)): updated_user_info_in_code=True, ) + # Pack the authorization code with updated authz_info + new_code = provider.provider.authz_state.authorization_codes.pack(authz_info) + form_dict["code"] = new_code + + # NOTE: Do NOT add sub back to claims dict. PyOP will get the + # sub from authz_info["sub"] when generating the ID token. + # If we include sub in claims as well, PyOP will receive it + # twice and raise: "TypeError: IdToken() got multiple values + # for keyword argument 'sub'" + + logger.info( + "Updated authorization code with claims for StatelessWrapper", + operation="update_auth_code_claims", + auth_session_id=str(auth_session.id), + user_id=user_id, + claims_keys=list(claims.keys()), + ) + # Store claims in VCUserinfo so PyOP can retrieve them when # generating the ID token. This is critical - without this, # get_claims_for will return empty dict. diff --git a/oidc-controller/api/routers/tests/test_oidc_token.py b/oidc-controller/api/routers/tests/test_oidc_token.py index 9149e248..4c6fabe8 100644 --- a/oidc-controller/api/routers/tests/test_oidc_token.py +++ b/oidc-controller/api/routers/tests/test_oidc_token.py @@ -676,3 +676,86 @@ async def test_user_info_contains_all_presentation_attributes( assert user_info["acr"] == "vc_authn" assert "given_names" in user_info["vc_presented_attributes"] assert "family_name" in user_info["vc_presented_attributes"] + + @pytest.mark.asyncio + async def test_claims_stored_without_sub_field( + self, mock_db, mock_auth_session, mock_provider + ): + """Test claims are stored in authz_info when no sub field exists. + + This covers the scenario where: + - generate_consistent_identifier = False (no hash-based sub) + - subject_identifier is empty or doesn't match (no attribute-based sub) + Result: Token.get_claims() returns claims WITHOUT a "sub" field + + Bug fix: authz_info["user_info"] must be set BEFORE checking if sub exists, + otherwise claims are lost in StatelessWrapper mode. + """ + from api.authSessions.crud import AuthSessionCRUD + from api.routers.oidc import post_token + from api.verificationConfigs.crud import VerificationConfigCRUD + + # Config that won't generate a sub + mock_config = MagicMock() + mock_config.subject_identifier = "" # Empty - won't create sub from attribute + mock_config.generate_consistent_identifier = False # Won't create hash sub + mock_config.include_v1_attributes = False + + with patch.object( + AuthSessionCRUD, + "get_by_pyop_auth_code", + return_value=mock_auth_session, + ): + with patch.object(VerificationConfigCRUD, "get", return_value=mock_config): + with patch.object( + AuthSessionCRUD, + "update_pyop_user_id", + new_callable=AsyncMock, + ) as mock_update: + # Mock jwt.decode - return claims WITHOUT sub field + with patch("jwt.decode") as mock_decode: + mock_decode.return_value = { + "pres_req_conf_id": "showcase-person", + "vc_presented_attributes": { + "given_names": "John", + "family_name": "Doe", + }, + "acr": "vc_authn", + # NOTE: NO "sub" field - this is the critical test case + } + + mock_request = MagicMock() + mock_form = MagicMock() + mock_form._dict = { + "code": "test-auth-code", + "grant_type": "authorization_code", + } + mock_request.form = MagicMock( + return_value=MagicMock( + __aenter__=AsyncMock(return_value=mock_form), + __aexit__=AsyncMock(return_value=None), + ) + ) + mock_request.headers = {} + + await post_token(mock_request, mock_db) + + # Verify authz_info["user_info"] was populated despite no sub + pack_call = ( + mock_provider.provider.authz_state.authorization_codes.pack + ) + assert pack_call.called + + authz_info = pack_call.call_args[0][0] + assert "user_info" in authz_info + + user_info = authz_info["user_info"] + assert user_info["pres_req_conf_id"] == "showcase-person" + assert user_info["acr"] == "vc_authn" + assert "given_names" in user_info["vc_presented_attributes"] + assert "family_name" in user_info["vc_presented_attributes"] + # Verify no sub was added + assert "sub" not in user_info + + # Verify pyop_user_id was NOT updated (no sub to replace it with) + mock_update.assert_not_called() diff --git a/pyproject.toml b/pyproject.toml index edfc65b1..217a847d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "acapy-vc-authn-oidc" -version = "2.3.4" +version = "2.3.5" description = "Verifiable Credential Identity Provider for OpenID Connect." authors = [ { name = "OpenWallet Foundation" } ] license = "Apache-2.0"