)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":14250,"name":"Grzegorz Grasza","email":"xek@redhat.com","username":"xek"},"change_message_id":"4a7de1913f85f9d209307ffde4ba41dd42aacd65","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"4ff7a485_e4db28b5","updated":"2026-09-14 11:22:01.000000000","message":"This should reference the CVE and the LP bug","commit_id":"b230bc793fa05158156d26a95a0a738aa5da5bd7"},{"author":{"_account_id":27900,"name":"Artem Goncharov","email":"artem.goncharov@gmail.com","username":"gtema"},"change_message_id":"4505790970ea19925446a58897e808b2f9af4d03","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"8542b052_20f0be48","in_reply_to":"4ff7a485_e4db28b5","updated":"2026-09-14 12:02:52.000000000","message":"done","commit_id":"b230bc793fa05158156d26a95a0a738aa5da5bd7"},{"author":{"_account_id":14250,"name":"Grzegorz Grasza","email":"xek@redhat.com","username":"xek"},"change_message_id":"de3f9cc76afd15a2ea1d70bc0f4204995cf538a0","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"e081dc34_b983c8b8","updated":"2026-09-14 11:23:15.000000000","message":"otherwise looks good, here is an AI review (IMO minor issues):\n\n8 findings — 1 medium, 7 low. No critical/high.\n\nMedium\n- `keystone/tests/unit/test_contrib_s3_core.py:108-114` [test] — _post_s3_token_capturing_id \nreads captured[\u0027id\u0027] without asserting issue_token was called. If ever invoked with a non-OK expected_status, the endpoint fails before issue_token, leaving captured empty → uncaught KeyError. Suggests self.assertEqual(1, len(captured)) after the mock context.\n\nLow (mostly maintainability / hardening nits)\n- `EC2_S3_Resource.py:72-79` [performance] — the \u0027ec2credential\u0027 not in CONF.auth.methods \nfail-closed guard runs per-request; consider moving to startup to fail fast.\n- `EC2_S3_Resource.py:78-86` [security] — the 503 ServiceUnavailable message reveals that \nec2credential is a config option; suggests a more generic error to avoid config oracle.\n- `auth/plugins/core.py` [maintainability] — construct_method_map_from_config() recomputes \nthe bitmask every token issue/validate; suggests caching.\n- `auth_context.py:459-462` [maintainability] — comment asserts JWS stores method names \nverbatim; suggests a JWS test case to lock the assumption.\n- `exception.py:711-717` [maintainability] — generic 503 message makes Keystone misconfig vs \nother 503s hard to distinguish in logs.\n- `conf/auth.py:108-124` [maintainability] — help-text wording (\"while it is disabled\") \ncould be clearer about the actual refuse-to-issue behavior.\n- `test_contrib_ec2_core.py:403-405` [test] — test_disabled_method_refuses_to_issue_token \ndoesn\u0027t assert on the returned response status at test level.","commit_id":"26c5f5275eb1f35e634ff4bcec16b3bfc1b0f039"},{"author":{"_account_id":27900,"name":"Artem Goncharov","email":"artem.goncharov@gmail.com","username":"gtema"},"change_message_id":"4505790970ea19925446a58897e808b2f9af4d03","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"3aece05a_681bc01a","in_reply_to":"e081dc34_b983c8b8","updated":"2026-09-14 12:02:52.000000000","message":"\u003e otherwise looks good, here is an AI review (IMO minor issues):\n\u003e \n\u003e 8 findings — 1 medium, 7 low. No critical/high.\n\u003e \n\u003e Medium\n\u003e - `keystone/tests/unit/test_contrib_s3_core.py:108-114` [test] — _post_s3_token_capturing_id \n\u003e reads captured[\u0027id\u0027] without asserting issue_token was called. If ever invoked with a non-OK expected_status, the endpoint fails before issue_token, leaving captured empty → uncaught KeyError. Suggests self.assertEqual(1, len(captured)) after the mock context.\n\nDone\n\n\u003e \n\u003e Low (mostly maintainability / hardening nits)\n\u003e - `EC2_S3_Resource.py:72-79` [performance] — the \u0027ec2credential\u0027 not in CONF.auth.methods \n\u003e fail-closed guard runs per-request; consider moving to startup to fail fast.\n\nNot reasonable: the check is a membership test on a ~7-string list — negligible vs the fernet crypto on the same path. Moving it to startup would turn \"two endpoints fail closed\" into \"Keystone refuses to boot\", breaking deployments that legitimately omit the method\n\n\u003e - `EC2_S3_Resource.py:78-86` [security] — the 503 ServiceUnavailable message reveals that \n\u003e ec2credential is a config option; suggests a more generic error to avoid config oracle.\n\nNot done: the message only appears in the fail-closed state, where it tells an attacker nothing actionable — the endpoint refuses to issue tokens anyway. The specific message is what the operator needs to repair the config; OpenStack convention surfaces concrete config errors in API responses. On the other side further comment suggests more details for maintainability\n\n\u003e - `auth/plugins/core.py` [maintainability] — construct_method_map_from_config() recomputes \n\u003e the bitmask every token issue/validate; suggests caching.\n\nNote done: sure makes sense, but is not touched with the change - should not be squashed here\n\n\u003e - `auth_context.py:459-462` [maintainability] — comment asserts JWS stores method names \n\u003e verbatim; suggests a JWS test case to lock the assumption.\n\nalready done in test_jws_provider.py::test_ec2credential_method_preserved.\n\n\u003e - `exception.py:711-717` [maintainability] — generic 503 message makes Keystone misconfig vs \n\u003e other 503s hard to distinguish in logs.\n\nthe raise site always passes the specific message, so logs/responses show it; the class default is only an unused fallback.\n\n\u003e - `conf/auth.py:108-124` [maintainability] — help-text wording (\"while it is disabled\") \n\u003e could be clearer about the actual refuse-to-issue behavior.\n\nDone\n\n\u003e - `test_contrib_ec2_core.py:403-405` [test] — test_disabled_method_refuses_to_issue_token \n\u003e doesn\u0027t assert on the returned response status at test level.\n\nalready asserted — the helper passes expected_status\u003dSERVICE_UNAVAILABLE to self.post(), and webtest\u0027s status\u003d kwarg (rest.py:97) asserts it.","commit_id":"26c5f5275eb1f35e634ff4bcec16b3bfc1b0f039"},{"author":{"_account_id":27900,"name":"Artem Goncharov","email":"artem.goncharov@gmail.com","username":"gtema"},"change_message_id":"5e84928aeb047b0b52535a45a23f9b1ceae798db","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"bc12e2d6_ea6e8fe0","updated":"2026-09-14 16:36:57.000000000","message":"restore privs before syntax rst links fix in releasenote to fix failing releasenotes job in gate","commit_id":"0e2166742f7749d8f93200b4ccd995347912b4fa"}]}