From 5efc0d3e3dddb6f44200ae56abf3c6a7003d685a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emiliano=20Su=C3=B1=C3=A9?= Date: Mon, 1 Dec 2025 15:03:45 +0100 Subject: [PATCH 1/5] v2.3.3 hotfix: ensure user_id is not null for in-memory userinfo storage (#924) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix userinfo flow for in-memory storage Signed-off-by: Emiliano Suñé * Use in-memory storage for single pod mode Signed-off-by: Emiliano Suñé * Bump project version Signed-off-by: Emiliano Suñé * Update tests Signed-off-by: Emiliano Suñé --------- Signed-off-by: Emiliano Suñé --- .../api/routers/tests/test_oidc_token.py | 32 +++++++------------ 1 file changed, 12 insertions(+), 20 deletions(-) diff --git a/oidc-controller/api/routers/tests/test_oidc_token.py b/oidc-controller/api/routers/tests/test_oidc_token.py index 9149e248..66f9b47b 100644 --- a/oidc-controller/api/routers/tests/test_oidc_token.py +++ b/oidc-controller/api/routers/tests/test_oidc_token.py @@ -224,11 +224,9 @@ async def test_claims_stored_with_presentation_sub_as_user_id( assert "vc_presented_attributes" in stored_claims # Critical: no duplicate sub assert "sub" not in stored_claims - + # Verify authz_info["user_info"] was updated for StatelessWrapper - authz_codes = ( - mock_provider.provider.authz_state.authorization_codes - ) + authz_codes = mock_provider.provider.authz_state.authorization_codes pack_call_args = authz_codes.pack.call_args[0][0] assert "user_info" in pack_call_args assert pack_call_args["user_info"] == stored_claims @@ -545,23 +543,21 @@ async def test_authz_info_user_info_updated_before_pack( await post_token(mock_request, mock_db) # Verify authz_info was packed with user_info field - authz_codes = ( - mock_provider.provider.authz_state.authorization_codes - ) + authz_codes = mock_provider.provider.authz_state.authorization_codes authz_codes.pack.assert_called_once() - + packed_authz_info = authz_codes.pack.call_args[0][0] - + # Critical: user_info field must be present and contain claims assert "user_info" in packed_authz_info user_info = packed_authz_info["user_info"] - + # Verify user_info contains the presentation claims assert "pres_req_conf_id" in user_info assert user_info["pres_req_conf_id"] == "showcase-person" assert "vc_presented_attributes" in user_info assert "acr" in user_info - + # Verify sub is NOT in user_info (it goes in authz_info["sub"]) assert "sub" not in user_info @@ -607,11 +603,9 @@ async def test_authz_info_sub_updated_with_presentation_sub( await post_token(mock_request, mock_db) # Verify authz_info["sub"] was updated before packing - authz_codes = ( - mock_provider.provider.authz_state.authorization_codes - ) + authz_codes = mock_provider.provider.authz_state.authorization_codes packed_authz_info = authz_codes.pack.call_args[0][0] - + assert packed_authz_info["sub"] == "John@showcase-person" @pytest.mark.asyncio @@ -656,12 +650,10 @@ async def test_user_info_contains_all_presentation_attributes( await post_token(mock_request, mock_db) # Get the user_info that was packed into authz_info - authz_codes = ( - mock_provider.provider.authz_state.authorization_codes - ) + authz_codes = mock_provider.provider.authz_state.authorization_codes packed_authz_info = authz_codes.pack.call_args[0][0] user_info = packed_authz_info["user_info"] - + # Verify all expected attributes are present expected_keys = [ "pres_req_conf_id", @@ -670,7 +662,7 @@ async def test_user_info_contains_all_presentation_attributes( ] for key in expected_keys: assert key in user_info, f"Missing expected key: {key}" - + # Verify values assert user_info["pres_req_conf_id"] == "showcase-person" assert user_info["acr"] == "vc_authn" From 97def545abab573c7b1a97f65bda55151f4dbb4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emiliano=20Sun=CC=83e=CC=81?= Date: Fri, 5 Dec 2025 00:03:11 +0100 Subject: [PATCH 2/5] bugfix: restore missing claims for non-consistent sub MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Emiliano Suñé --- oidc-controller/api/routers/oidc.py | 51 +++++++++++-------- .../api/routers/tests/test_oidc_token.py | 32 +++++++----- 2 files changed, 50 insertions(+), 33 deletions(-) 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 66f9b47b..9149e248 100644 --- a/oidc-controller/api/routers/tests/test_oidc_token.py +++ b/oidc-controller/api/routers/tests/test_oidc_token.py @@ -224,9 +224,11 @@ async def test_claims_stored_with_presentation_sub_as_user_id( assert "vc_presented_attributes" in stored_claims # Critical: no duplicate sub assert "sub" not in stored_claims - + # Verify authz_info["user_info"] was updated for StatelessWrapper - authz_codes = mock_provider.provider.authz_state.authorization_codes + authz_codes = ( + mock_provider.provider.authz_state.authorization_codes + ) pack_call_args = authz_codes.pack.call_args[0][0] assert "user_info" in pack_call_args assert pack_call_args["user_info"] == stored_claims @@ -543,21 +545,23 @@ async def test_authz_info_user_info_updated_before_pack( await post_token(mock_request, mock_db) # Verify authz_info was packed with user_info field - authz_codes = mock_provider.provider.authz_state.authorization_codes + authz_codes = ( + mock_provider.provider.authz_state.authorization_codes + ) authz_codes.pack.assert_called_once() - + packed_authz_info = authz_codes.pack.call_args[0][0] - + # Critical: user_info field must be present and contain claims assert "user_info" in packed_authz_info user_info = packed_authz_info["user_info"] - + # Verify user_info contains the presentation claims assert "pres_req_conf_id" in user_info assert user_info["pres_req_conf_id"] == "showcase-person" assert "vc_presented_attributes" in user_info assert "acr" in user_info - + # Verify sub is NOT in user_info (it goes in authz_info["sub"]) assert "sub" not in user_info @@ -603,9 +607,11 @@ async def test_authz_info_sub_updated_with_presentation_sub( await post_token(mock_request, mock_db) # Verify authz_info["sub"] was updated before packing - authz_codes = mock_provider.provider.authz_state.authorization_codes + authz_codes = ( + mock_provider.provider.authz_state.authorization_codes + ) packed_authz_info = authz_codes.pack.call_args[0][0] - + assert packed_authz_info["sub"] == "John@showcase-person" @pytest.mark.asyncio @@ -650,10 +656,12 @@ async def test_user_info_contains_all_presentation_attributes( await post_token(mock_request, mock_db) # Get the user_info that was packed into authz_info - authz_codes = mock_provider.provider.authz_state.authorization_codes + authz_codes = ( + mock_provider.provider.authz_state.authorization_codes + ) packed_authz_info = authz_codes.pack.call_args[0][0] user_info = packed_authz_info["user_info"] - + # Verify all expected attributes are present expected_keys = [ "pres_req_conf_id", @@ -662,7 +670,7 @@ async def test_user_info_contains_all_presentation_attributes( ] for key in expected_keys: assert key in user_info, f"Missing expected key: {key}" - + # Verify values assert user_info["pres_req_conf_id"] == "showcase-person" assert user_info["acr"] == "vc_authn" From 96e342d2ce5a8ca07832fbe38987de481597475e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emiliano=20Sun=CC=83e=CC=81?= Date: Fri, 5 Dec 2025 00:06:04 +0100 Subject: [PATCH 3/5] Bump app version MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Emiliano Suñé --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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" From 66ae699bd0cfe65c6f96658649052ca0a8a5d5fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emiliano=20Sun=CC=83e=CC=81?= Date: Fri, 5 Dec 2025 00:09:16 +0100 Subject: [PATCH 4/5] Add test for non-consistent sub scenario MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Emiliano Suñé --- .../api/routers/tests/test_oidc_token.py | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/oidc-controller/api/routers/tests/test_oidc_token.py b/oidc-controller/api/routers/tests/test_oidc_token.py index 9149e248..fdea5e2f 100644 --- a/oidc-controller/api/routers/tests/test_oidc_token.py +++ b/oidc-controller/api/routers/tests/test_oidc_token.py @@ -676,3 +676,84 @@ 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() From 4c03a206ee825e9f3d48db153e5e418e836f6347 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Emiliano=20Sun=CC=83e=CC=81?= Date: Fri, 5 Dec 2025 00:34:09 +0100 Subject: [PATCH 5/5] Fix linting errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Emiliano Suñé --- .../api/routers/tests/test_oidc_token.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/oidc-controller/api/routers/tests/test_oidc_token.py b/oidc-controller/api/routers/tests/test_oidc_token.py index fdea5e2f..4c6fabe8 100644 --- a/oidc-controller/api/routers/tests/test_oidc_token.py +++ b/oidc-controller/api/routers/tests/test_oidc_token.py @@ -682,12 +682,12 @@ 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. """ @@ -718,9 +718,9 @@ async def test_claims_stored_without_sub_field( "pres_req_conf_id": "showcase-person", "vc_presented_attributes": { "given_names": "John", - "family_name": "Doe" + "family_name": "Doe", }, - "acr": "vc_authn" + "acr": "vc_authn", # NOTE: NO "sub" field - this is the critical test case } @@ -741,12 +741,14 @@ async def test_claims_stored_without_sub_field( 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 + 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" @@ -754,6 +756,6 @@ async def test_claims_stored_without_sub_field( 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()