)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":38589,"name":"Moutaz Chaara","display_name":"Moutaz Chaara","email":"moutaz.chaara@sap.com","username":"tz3","status":"SAP SE"},"change_message_id":"ee5490965f671ca9c38e5dec175764cfb1228ab2","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"a91706e0_db592234","updated":"2025-11-14 10:41:39.000000000","message":"Hi Artem,\n\nI\u0027ve added you as a reviewer since you recently worked on identity backend \ncaching issues. This patch addresses a related problem where federated users\u0027 \nrole assignments don\u0027t update when their group membership changes in the IdP.\n\nWould appreciate your feedback on the cache invalidation approach!\n\nThanks,\nMoutaz","commit_id":"a1a01ea0e181db9a75cd809dfbb03d6914365cb3"},{"author":{"_account_id":27900,"name":"Artem Goncharov","email":"artem.goncharov@gmail.com","username":"gtema"},"change_message_id":"47103cdd8a8ea72c4237fb985fb6c8f8520ad7d6","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"4cb5a020_28e8b60c","updated":"2025-11-17 14:55:24.000000000","message":"I am wondering whether you should try to invalidate the cache with your logic in the place suggested in the ticket itself. Refreshing group memberships now additionally in a different place feels weird. And sadly we of course do not have any usable tests to check whether the change breaks the flow or whether it fixes reported issue. Have you had a chance to look whether introducing any tests validating the functionality (the issue provides sensible test scenario).","commit_id":"a1a01ea0e181db9a75cd809dfbb03d6914365cb3"},{"author":{"_account_id":38589,"name":"Moutaz Chaara","display_name":"Moutaz Chaara","email":"moutaz.chaara@sap.com","username":"tz3","status":"SAP SE"},"change_message_id":"da7e117ad6948d89ad660a8161dbefe204727eb9","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":7,"id":"da5a0da7_f821b45a","in_reply_to":"4cb5a020_28e8b60c","updated":"2025-11-17 20:20:31.000000000","message":"Hi Artem,\n\nThanks for looking at this. I\u0027ve refactored it to put the cache invalidation right in shadow_federated_user() where it is suggested.\n\nThe big change is I made it conditional - cache only invalidates when group membership actually changes, not on every auth. Modified add_user_to_group_expires() to return true/false so we can tell new memberships apart from TTL refreshes. This way the cache stays effective for repeated logins with the same groups.\n\nRegarding tests- yeah, this is honestly pretty hard to test properly. You\u0027d need to mock the IdP, configure caching, simulate membership changes between auths, and somehow verify the cache state. The current federation tests don\u0027t really have that infrastructure. I can take a look at it but wanted to get your thoughts first - is this worth the effort or should we rely on manual validation for now?","commit_id":"a1a01ea0e181db9a75cd809dfbb03d6914365cb3"},{"author":{"_account_id":38589,"name":"Moutaz Chaara","display_name":"Moutaz Chaara","email":"moutaz.chaara@sap.com","username":"tz3","status":"SAP SE"},"change_message_id":"b32d56616e4991f336de632965f1fc4582e22000","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"abbdbb21_811d0a29","updated":"2025-11-27 13:53:37.000000000","message":"Hi Artem,\nI could debug it and trace the execution path for it and you were right about the shared memcache, but my initial thoughts were actually correct - I do need the notification because `COMPUTED_ASSIGNMENTS_REGION` and `TOKENS_REGION `are separate cache regions with different RegionInvalidationManager instances.\n\nWhen I invalidate the assignment region, it doesn\u0027t touch the token region at all. The cached token validations still have the old group memberships.\n\nThe notification is what triggers the token provider\u0027s callback to invalidate TOKENS_REGION.\n\n**Here\u0027s the execution trace from my local patch:**\n\n1. Group membership change detected at `keystone/identity/core.py:1782`.\n\n2. Calls `invalidate_user_role_assignments_cache()` at `keystone/assignment/core.py:1296-1311`.\n\n3. Invalidates `COMPUTED_ASSIGNMENTS_REGION`, then calls `invalidate_token_cache_notification()` at https://opendev.org/openstack/keystone/src/branch/master/keystone/notifications.py#L325-L351\n\n4. This calls `Audit._emit()` at https://opendev.org/openstack/keystone/src/branch/master/keystone/notifications.py#L345 which calls `_send_notification()` at https://opendev.org/openstack/keystone/src/branch/master/keystone/notifications.py#L162-L169, which then calls `notify_event_callbacks()` at https://opendev.org/openstack/keystone/src/branch/master/keystone/notifications.py#L466\n5. `notify_event_callbacks()` triggers the registered callbacks: https://opendev.org/openstack/keystone/src/branch/master/keystone/token/provider.py#L96-L114\n6. The `_drop_token_cache()` callback executes: https://opendev.org/openstack/keystone/src/branch/master/keystone/token/provider.py#L121-L130 and invalidates `TOKENS_REGION`\n\n**This same pattern already exists:** For example, when a user is enabled/disabled (https://opendev.org/openstack/keystone/src/branch/master/keystone/identity/core.py#L1385) which call `invalidate_token_cache_notification()` which triggers the same callback chain to invalidate the token cache.\n\nWithout the notification, steps 4-6 never happen and the token cache keeps stale validation results.\n\nWDYT?","commit_id":"7fd031aab2dbccfc61c20e35185357441ae473d8"},{"author":{"_account_id":38589,"name":"Moutaz Chaara","display_name":"Moutaz Chaara","email":"moutaz.chaara@sap.com","username":"tz3","status":"SAP SE"},"change_message_id":"f0782df0c1251c2257e926dcf3007693497cb407","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":11,"id":"675e015b_2a26b461","updated":"2025-11-26 09:42:52.000000000","message":"Hi Artem,\nI\u0027ve pushed an updated version with unit tests as you suggested. Could you take another look when you have a chance?\nThanks!","commit_id":"7fd031aab2dbccfc61c20e35185357441ae473d8"},{"author":{"_account_id":27900,"name":"Artem Goncharov","email":"artem.goncharov@gmail.com","username":"gtema"},"change_message_id":"2feb4a7e957123c4014716e29b56998635e4067d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"6c273826_5b6dd727","in_reply_to":"1e358394_dace8c3b","updated":"2025-12-04 09:28:27.000000000","message":"To a discussion from yesterday, I think let\u0027s invalidate tokens with -1 sec due to the time race.\nThis would also definitely require a release note since this is a pretty hard UX change","commit_id":"7fd031aab2dbccfc61c20e35185357441ae473d8"},{"author":{"_account_id":38589,"name":"Moutaz Chaara","display_name":"Moutaz Chaara","email":"moutaz.chaara@sap.com","username":"tz3","status":"SAP SE"},"change_message_id":"eaf0f2c9b0912d3a1dca4f76a916d74ccc4cdfa5","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"c086d7ae_5203a7fa","in_reply_to":"6c273826_5b6dd727","updated":"2025-12-04 16:11:34.000000000","message":"I\u0027ve implemented the -1 second approach as you suggested. By setting `issued_before \u003d current_time - 1 second`, we now revoke only tokens issued before the group membership change, leaving new tokens valid.","commit_id":"7fd031aab2dbccfc61c20e35185357441ae473d8"},{"author":{"_account_id":27900,"name":"Artem Goncharov","email":"artem.goncharov@gmail.com","username":"gtema"},"change_message_id":"4cf80a3bd5dcc823221bba3c55ad3741dcee6efe","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"d07b980c_74ef91a1","in_reply_to":"abbdbb21_811d0a29","updated":"2025-11-27 15:43:16.000000000","message":"let\u0027s do a pragmatic test (or is it exactly what you did?):\n- authenticate user to get a token with initial set of groups (token1)\n- change group relations (add/remove) on the IdP\n- authenticate user to get a new token with updated set of groups and corresponding roles (token2)\n- invoke token validation endpoint (GET /v3/auth/tokens) for the token1 and ensure that roles and group memberships are updated.\n\nIf you see that this works - no token invalidation is necessary. Otherwise it is necessary.\n\nHonestly this is so tricky that it is nearly impossible to really check that by looking at the code, therefore the test will show the truth. I do not think that roles would be a problem, but group memberships themselves might be since group list is technically part of the token payload. That would force us not only to invalidate the cache, but to actually forcibly invalidate (revoke) all tokens of the user (add is not itself a problem, but removal is). This does not happen when you only invalidate the cache.\n\nSummary: I agree that token cache invalidation may be necessary, but now I think it may be not even sufficient.","commit_id":"7fd031aab2dbccfc61c20e35185357441ae473d8"},{"author":{"_account_id":38589,"name":"Moutaz Chaara","display_name":"Moutaz Chaara","email":"moutaz.chaara@sap.com","username":"tz3","status":"SAP SE"},"change_message_id":"c096e1e5520c0668a8bcd77526568dc32c6dda3b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"1e358394_dace8c3b","in_reply_to":"d07b980c_74ef91a1","updated":"2025-12-02 09:13:52.000000000","message":"Hi,\n\nI\u0027ve done the test and you were right - token cache invalidation is necessary and it is not sufficient so i included the revocation for the tokens.\n\nThe patch now handles all three scenarios:\n\n1. Group Addition - when user gains new groups from IdP\n2. Group Removal - via `cleanup_stale_group_memberships()`\n3. Token Invalidation - both trigger `invalidate_user_cache_on_group_change()`\n\nThis invalidates/clears when there is a change \u0026 the user is already exists:\n\n- COMPUTED_ASSIGNMENTS_REGION.\n\n- Token cache (TOKENS_REGION).\n\n- Revocation events - Via `PERSIST_REVOCATION_EVENT_FOR_USER`.\n\nWDYT?","commit_id":"7fd031aab2dbccfc61c20e35185357441ae473d8"},{"author":{"_account_id":14250,"name":"Grzegorz Grasza","email":"xek@redhat.com","username":"xek"},"change_message_id":"b1874578c85e15a3d18020b2ce5a04e90ff232c3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":18,"id":"f617c325_fcb16185","updated":"2025-12-09 09:57:42.000000000","message":"LGTM!","commit_id":"ad87d8212a1bab654e4a6d791d7271e6b3bcd1b5"},{"author":{"_account_id":27900,"name":"Artem Goncharov","email":"artem.goncharov@gmail.com","username":"gtema"},"change_message_id":"c02abfacfa10ca30112e4fe62832cc3c456c8565","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":18,"id":"40d75b2f_c8f302b6","updated":"2025-12-05 15:59:46.000000000","message":"it does look good to me. @b.bobrov@sap.com would appreciate your view on that","commit_id":"ad87d8212a1bab654e4a6d791d7271e6b3bcd1b5"}]}