)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"1bddf8313d9cb6a9e64a0d3118e9b8b899c3cc7f","unresolved":true,"context_lines":[{"line_number":14,"context_line":"application credentials based upon the \u0027member\u0027 role became"},{"line_number":15,"context_line":"useless as they did not explicitly include the \u0027reader\u0027 role"},{"line_number":16,"context_line":"which became mandatory for some services\u0027 API operations."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"Related-Bug: #2030061"},{"line_number":19,"context_line":"Change-Id: I2aea0b89987b24cf5ddaadeecbd06c32ad81a9bc"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"3bb1a3f0_6fb9347a","line":17,"updated":"2024-05-21 09:32:40.000000000","message":"+1 this seam like a high priority bug to fix given the move to the new policies by defualt","commit_id":"64daf40bf412cb39e69f82622447da439a6d5e59"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":31542,"name":"Andrew Bonney","email":"andrew.bonney@bbc.co.uk","username":"andrewbonney"},"change_message_id":"9417dfb723fe6647b9476ee4665df223147f162e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"b4db4702_13f0fd86","updated":"2023-09-05 14:15:24.000000000","message":"Intended to demonstrate the issue rather than be merged as-is. See the linked issue for more details.","commit_id":"7c2cbb1c0b98b1e67c3fd6be0c5afe4f0fbb5bbe"},{"author":{"_account_id":31542,"name":"Andrew Bonney","email":"andrew.bonney@bbc.co.uk","username":"andrewbonney"},"change_message_id":"878b4f181bf9d7c9f6f8f0ae9c194e2c0b110e2c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"ad910f87_54d4d0aa","updated":"2023-10-16 11:00:48.000000000","message":"Added another revision. The previous one could result in a roles list which included duplicates. This caused an integrity check failure if application credentials are used to create new application credentials, as it tries to insert duplicate roles into the database.","commit_id":"fe925d874996e0d63f2bd1ae95b8a47a11c82d1a"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"e0b32853aa34b2419f256fa626b7037c40c4d7a0","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"674cb12c_b7465b9a","updated":"2023-11-16 15:07:24.000000000","message":"FWIW, to me, this feels like a sensible thing to do and then backport.\n\nThis means operators can make _member_ imply member, when needed, and when you use just \"member\" you get the \"reader\" role as expected.\n\nQuite how we square this with API microversions is a tricker question... but this does feel like a valid bug fix given the current mess upgrades are hitting.\n\nI am curious what other people are thinking about this approach?","commit_id":"fe925d874996e0d63f2bd1ae95b8a47a11c82d1a"},{"author":{"_account_id":13478,"name":"Boris Bobrov","email":"b.bobrov@sap.com","username":"bbobrov"},"change_message_id":"44d59637807ee51208a16698cff985a1934b7fe9","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"526fd1bb_bb8a5807","updated":"2024-01-17 15:36:27.000000000","message":"There should be unit tests for the change","commit_id":"6ee7ea0d63fed272beb3806d722c2dd3585e8212"},{"author":{"_account_id":28619,"name":"Dmitriy Rabotyagov","email":"noonedeadpunk@gmail.com","username":"noonedeadpunk"},"change_message_id":"6ac4603b3dc71f1d491f6f82bab7f3e6d6b5f1a8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"3f6c83f1_52993f7a","updated":"2024-02-27 15:57:23.000000000","message":"There\u0027s an alternative patch here: https://review.opendev.org/c/openstack/keystone/+/910337\n\nSo if you could check on that - it would be nice.","commit_id":"6ee7ea0d63fed272beb3806d722c2dd3585e8212"},{"author":{"_account_id":7414,"name":"David Wilde","email":"dwilde@redhat.com","username":"d34dh0r53"},"change_message_id":"299b1022c28496c47633f11aff157ee3efcc74bc","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"20d2dda6_3bce68b3","updated":"2024-01-26 15:41:50.000000000","message":"This looks good, but I agree that this needs unit tests.","commit_id":"6ee7ea0d63fed272beb3806d722c2dd3585e8212"},{"author":{"_account_id":31542,"name":"Andrew Bonney","email":"andrew.bonney@bbc.co.uk","username":"andrewbonney"},"change_message_id":"fa58303da2fae1a5f98ff9fbcec54ad6bf210aa7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"853520ea_429956c8","in_reply_to":"20d2dda6_3bce68b3","updated":"2024-01-26 15:58:41.000000000","message":"I\u0027m afraid we don\u0027t have the time or knowledge to add these. If someone else is able to take it forward then please feel free.","commit_id":"6ee7ea0d63fed272beb3806d722c2dd3585e8212"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"1bddf8313d9cb6a9e64a0d3118e9b8b899c3cc7f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"806a69c2_31ab5bd6","updated":"2024-05-21 09:32:40.000000000","message":"+1 in general but i was almost -1 on the unit test","commit_id":"64daf40bf412cb39e69f82622447da439a6d5e59"},{"author":{"_account_id":27900,"name":"Artem Goncharov","email":"artem.goncharov@gmail.com","username":"gtema"},"change_message_id":"ea2b642355cc5c04c34b3b2c36752052cf9a2765","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"4a551063_68e4fdf3","updated":"2024-06-03 12:24:17.000000000","message":"As discussed in https://review.opendev.org/c/openstack/keystone/+/910337 I personally see a danger in this approach. Current Keystone design relies on immutability of the roles on ApplicationCredentials. It is nearly impossible to explain for user or admin why appcreds are not working one day and work next day (when somebody changes inheritance). In addition to that introducing 2 different ways of dealing with AppCreds created requesting roles explicitly (where all roles are directly auditable) and implicitly (where you can only guess) is also a wrong thing from auditability pov.","commit_id":"64daf40bf412cb39e69f82622447da439a6d5e59"},{"author":{"_account_id":3031,"name":"Sam Morrison","email":"sorrison@gmail.com","username":"sorrison"},"change_message_id":"f7d3b19e0633904d9f8fd7406320718c956a9b97","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"8de6b527_cce21f51","updated":"2024-06-04 04:03:53.000000000","message":"Wondering if any what to sort this. Currently the main issue is the default policy in glance from antelope changes things so that things like listing images needs reader role as opposed to member role. We weren\u0027t using reader role so we make this an implied role to avoid having to add it to all our users (the whole idea of implied roles) \n\nTo work around this bug at the moment we have to define custom policy in glance for about 12 rules that allow member role to pass which isn\u0027t great.\n\nAs an operator if I make a role implied I would expect this to work for all existing application credentials as well","commit_id":"64daf40bf412cb39e69f82622447da439a6d5e59"},{"author":{"_account_id":31542,"name":"Andrew Bonney","email":"andrew.bonney@bbc.co.uk","username":"andrewbonney"},"change_message_id":"48d662310a4c788cc769427b2e08dc0ecbf2fe38","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"c467f39b_963e1e6d","updated":"2024-05-21 08:17:59.000000000","message":"recheck - tempest image issue","commit_id":"64daf40bf412cb39e69f82622447da439a6d5e59"},{"author":{"_account_id":3031,"name":"Sam Morrison","email":"sorrison@gmail.com","username":"sorrison"},"change_message_id":"030efbede045dcdda023648856ca6f383ed9b19d","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":4,"id":"96d1b06f_d0a946b4","in_reply_to":"18179d3d_52690399","updated":"2024-06-12 03:04:23.000000000","message":"Yes please, we have pulled this patch into our keystone which is running antelope so be great if I don\u0027t have to carry this for long. \n\nI also think that a data migration might not be the best as there will be times in the future we refactor our implied roles and our policies and I don\u0027t want to worry about breaking APs or forgetting to run some db script etc.","commit_id":"64daf40bf412cb39e69f82622447da439a6d5e59"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"c22efae8e94389755395d106a43d0f7e30dfdc37","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":4,"id":"18179d3d_52690399","in_reply_to":"8de6b527_cce21f51","updated":"2024-06-04 11:40:41.000000000","message":"personally i think this is not jsut a bug that should be fixed on master but backported to all stable branches if that is what you mean.\n\ni would also expect implied roles to be delegated to existing application credentials and it woudl have expect that to happen as part of an online data migration when or similar on upgrade.\n\nso im not sure that doing this at runtime is the correct approch but i do feel the end result of the change i.e. existing applciation credentals have implied roles is important\n\nthe problem with doing this as an online data migration is it makes it a potential interop problem if you forget to do that when enabling the new defautls.\n\nso im not sure which is more correct but i would consider this bug to be a rellitivly high severity one and there need to be some way either automtaic or opt in via the admin to correct this for existing application creadntioal otherwisse it prevents a cloud form adopting the new defaults.","commit_id":"64daf40bf412cb39e69f82622447da439a6d5e59"}],"keystone/tests/unit/test_v3_application_credential.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"1bddf8313d9cb6a9e64a0d3118e9b8b899c3cc7f","unresolved":true,"context_lines":[{"line_number":131,"context_line":"            implied_role \u003d unit.new_role_ref(name\u003d\u0027implied_role\u0027)"},{"line_number":132,"context_line":"            PROVIDERS.role_api.create_role(implied_role[\u0027id\u0027], implied_role)"},{"line_number":133,"context_line":"            PROVIDERS.role_api.create_implied_role(self.role_id,"},{"line_number":134,"context_line":"                                                   implied_role[\u0027id\u0027])"},{"line_number":135,"context_line":"            auth_data \u003d self.build_authentication_request("},{"line_number":136,"context_line":"                app_cred_id\u003dapp_cred.json[\u0027application_credential\u0027][\u0027id\u0027],"},{"line_number":137,"context_line":"                secret\u003dapp_cred.json[\u0027application_credential\u0027][\u0027secret\u0027])"}],"source_content_type":"text/x-python","patch_set":4,"id":"89b073f6_81cb6c72","line":134,"updated":"2024-05-21 09:32:40.000000000","message":"im not familare with this code but it seam odd to me that you have to create the roles and implied roels expclitly here.\n\nshould this not be a sideffect fo the post at a minium if this was required.\n\n\nbasically this does not look like a unit test to me its doing way to much stuff.\n\nyour creating the app cred body, creating a scopet token, then doign a post \n\nnext your creting rules and implied roles and then usign that to authenticat.\n\nunit tests should generall test one functiona and mock out calles to function that eihter have sideeffect or are outside fo the current module/class that the function under test resides in.\n\nso to me this  is a functional test not a unit test.","commit_id":"64daf40bf412cb39e69f82622447da439a6d5e59"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"6b1b6c515f1c62a1404cdca87b61bfa860011424","unresolved":false,"context_lines":[{"line_number":131,"context_line":"            implied_role \u003d unit.new_role_ref(name\u003d\u0027implied_role\u0027)"},{"line_number":132,"context_line":"            PROVIDERS.role_api.create_role(implied_role[\u0027id\u0027], implied_role)"},{"line_number":133,"context_line":"            PROVIDERS.role_api.create_implied_role(self.role_id,"},{"line_number":134,"context_line":"                                                   implied_role[\u0027id\u0027])"},{"line_number":135,"context_line":"            auth_data \u003d self.build_authentication_request("},{"line_number":136,"context_line":"                app_cred_id\u003dapp_cred.json[\u0027application_credential\u0027][\u0027id\u0027],"},{"line_number":137,"context_line":"                secret\u003dapp_cred.json[\u0027application_credential\u0027][\u0027secret\u0027])"}],"source_content_type":"text/x-python","patch_set":4,"id":"59edf5d3_c3ecf463","line":134,"in_reply_to":"5298416c_4e322d03","updated":"2024-05-21 17:48:48.000000000","message":"local coding style wins over purity.\n\nwe likely would want this to be boken out in nova before merging but we try to be faily strict on the delinitation between unit and functional tests because we spent a lot of time creating that distinction.","commit_id":"64daf40bf412cb39e69f82622447da439a6d5e59"},{"author":{"_account_id":7414,"name":"David Wilde","email":"dwilde@redhat.com","username":"d34dh0r53"},"change_message_id":"d52775ac9d84ea0e69768665de77e36f45a7af54","unresolved":true,"context_lines":[{"line_number":131,"context_line":"            implied_role \u003d unit.new_role_ref(name\u003d\u0027implied_role\u0027)"},{"line_number":132,"context_line":"            PROVIDERS.role_api.create_role(implied_role[\u0027id\u0027], implied_role)"},{"line_number":133,"context_line":"            PROVIDERS.role_api.create_implied_role(self.role_id,"},{"line_number":134,"context_line":"                                                   implied_role[\u0027id\u0027])"},{"line_number":135,"context_line":"            auth_data \u003d self.build_authentication_request("},{"line_number":136,"context_line":"                app_cred_id\u003dapp_cred.json[\u0027application_credential\u0027][\u0027id\u0027],"},{"line_number":137,"context_line":"                secret\u003dapp_cred.json[\u0027application_credential\u0027][\u0027secret\u0027])"}],"source_content_type":"text/x-python","patch_set":4,"id":"5298416c_4e322d03","line":134,"in_reply_to":"89b073f6_81cb6c72","updated":"2024-05-21 13:32:58.000000000","message":"I see your point, but this test follows the pattern of the other unit tests in Keystone.  Our tests often blur the lines between unit and functional I think due to historical reasons. I don\u0027t think it\u0027s enough to prevent this spec from moving forward though.","commit_id":"64daf40bf412cb39e69f82622447da439a6d5e59"}]}