)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"de54ff39696b1f7beb24de186c9209c4510fe3c3","unresolved":true,"context_lines":[{"line_number":18,"context_line":"project\u0027s admin role."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"Related-blueprint: #secure-rbac-roles"},{"line_number":21,"context_line":"Change-Id: I403ca661dceee17aff9295caf8721c4a237a58cf"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"edc81163_95b6f5cd","line":21,"updated":"2021-03-15 16:12:31.000000000","message":"It looks like this patch fixes bug 1895933 too. If I am correct, could you mention the bug?","commit_id":"eb3caeb88325fb10a81b3916575ac5e477fe1fcc"}],"neutron/policy.py":[{"author":{"_account_id":16688,"name":"Rodolfo Alonso","email":"ralonsoh@redhat.com","username":"rodolfo-alonso-hernandez"},"change_message_id":"e8f5aa8b143f67886ad06cfe7fd4f5ce12bddc8c","unresolved":true,"context_lines":[{"line_number":468,"context_line":"    \"\"\""},{"line_number":469,"context_line":"    # If we already know the context has admin rights do not perform an"},{"line_number":470,"context_line":"    # additional check and authorize the operation"},{"line_number":471,"context_line":"    if context.is_admin:"},{"line_number":472,"context_line":"        return True"},{"line_number":473,"context_line":"    if might_not_exist and not (_ENFORCER.rules and action in _ENFORCER.rules):"},{"line_number":474,"context_line":"        return True"}],"source_content_type":"text/x-python","patch_set":1,"id":"0a5fc1df_22260cd1","side":"PARENT","line":471,"updated":"2021-03-15 12:00:58.000000000","message":"I\u0027m ok with this change. Thanks to bp/secure-rbac-roles implementation, we have \"personas\" as system-admin or project-admin.\n\nMy concern is those other many places where we still just check \"if context.is_admin\": https://codesearch.openstack.org/?q\u003dif%20context.is_admin\u0026i\u003dnope\u0026files\u003d\u0026excludeFiles\u003d\u0026repos\u003dopenstack/neutron","commit_id":"5f6fbcb155e19d0a46cef0e7bbfee553f0472ef3"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"de54ff39696b1f7beb24de186c9209c4510fe3c3","unresolved":true,"context_lines":[{"line_number":468,"context_line":"    \"\"\""},{"line_number":469,"context_line":"    # If we already know the context has admin rights do not perform an"},{"line_number":470,"context_line":"    # additional check and authorize the operation"},{"line_number":471,"context_line":"    if context.is_admin:"},{"line_number":472,"context_line":"        return True"},{"line_number":473,"context_line":"    if might_not_exist and not (_ENFORCER.rules and action in _ENFORCER.rules):"},{"line_number":474,"context_line":"        return True"}],"source_content_type":"text/x-python","patch_set":1,"id":"c8d2ccc1_f705069a","side":"PARENT","line":471,"in_reply_to":"0a5fc1df_22260cd1","updated":"2021-03-15 16:12:31.000000000","message":"I agree that we perhaps need to revisit the usage of \"context.is_admin\", but I think all above usages of context.is_admin does not affect the issue fixed here.\n\nI checked other usages.\nIn the first three usages of the above search result, it is part of \"context if context.is_admin else context.elevated()\" so the resulting context is always an admin context, so it does not matter much I think.\nThe usage in l3_db.py is part of _add_interface_by_port() so the API operation is already validated by the policy, so it does not directly affect the issue tackled by this patch.","commit_id":"5f6fbcb155e19d0a46cef0e7bbfee553f0472ef3"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"3b41d7ccf98df8187eff20f1ca5d133a6cd47712","unresolved":true,"context_lines":[{"line_number":471,"context_line":"    # TODO(slaweq): Remove that is_admin check and always perform rules checks"},{"line_number":472,"context_line":"    # when old, deprecated rules will be removed and only rules with new"},{"line_number":473,"context_line":"    # personas will be supported"},{"line_number":474,"context_line":"    if not cfg.CONF.oslo_policy.enforce_new_defaults and context.is_admin:"},{"line_number":475,"context_line":"        return True"},{"line_number":476,"context_line":"    if might_not_exist and not (_ENFORCER.rules and action in _ENFORCER.rules):"},{"line_number":477,"context_line":"        return True"}],"source_content_type":"text/x-python","patch_set":2,"id":"c6d642ee_95e0df4c","line":474,"updated":"2021-03-16 06:04:51.000000000","message":"The system-scoped token is used even when enforce_new_defaults is not True.\nThe new rules are evaluated first, so I wonderr this problem happens if a user is associated with system-scope.\n\nenforce_new_defaults True means that deprecated rules aree not evaluated.\nhttps://docs.openstack.org/oslo.policy/latest/configuration/index.html#oslo_policy.enforce_new_defaults\n\nI still haven\u0027t got a good idea but we might need some new config option which controls whether we always evaluate policies even regardlesss of context.is_admin is True.","commit_id":"10eeb530b43c766806039aadda65ac077de5194f"},{"author":{"_account_id":5046,"name":"Lance Bragstad","email":"lbragstad@redhat.com","username":"ldbragst"},"change_message_id":"2254938ca9dd4a8aa3872bcb399e22af1f50499f","unresolved":true,"context_lines":[{"line_number":471,"context_line":"    # TODO(slaweq): Remove that is_admin check and always perform rules checks"},{"line_number":472,"context_line":"    # when old, deprecated rules will be removed and only rules with new"},{"line_number":473,"context_line":"    # personas will be supported"},{"line_number":474,"context_line":"    if not cfg.CONF.oslo_policy.enforce_new_defaults and context.is_admin:"},{"line_number":475,"context_line":"        return True"},{"line_number":476,"context_line":"    if might_not_exist and not (_ENFORCER.rules and action in _ENFORCER.rules):"},{"line_number":477,"context_line":"        return True"}],"source_content_type":"text/x-python","patch_set":2,"id":"e40dceb7_765c6bcb","line":474,"in_reply_to":"c0d9265e_6737c2c3","updated":"2021-03-18 01:33:09.000000000","message":"I think that makes sense. Since this behavior is config driven, I think we could add a unit test or two showcasing the behavior of each.\n\nIn theory, the unit tests should continue to work (so long as they don\u0027t rely on the is_admin property directly) as neutron continues to refactor/remove the is_admin check out of it\u0027s codebase in the future, right?","commit_id":"10eeb530b43c766806039aadda65ac077de5194f"},{"author":{"_account_id":11975,"name":"Slawek Kaplonski","email":"skaplons@redhat.com","username":"slaweq"},"change_message_id":"9f5ca9ba07c73cf5bbc37a58a58cb17684b70373","unresolved":true,"context_lines":[{"line_number":471,"context_line":"    # TODO(slaweq): Remove that is_admin check and always perform rules checks"},{"line_number":472,"context_line":"    # when old, deprecated rules will be removed and only rules with new"},{"line_number":473,"context_line":"    # personas will be supported"},{"line_number":474,"context_line":"    if not cfg.CONF.oslo_policy.enforce_new_defaults and context.is_admin:"},{"line_number":475,"context_line":"        return True"},{"line_number":476,"context_line":"    if might_not_exist and not (_ENFORCER.rules and action in _ENFORCER.rules):"},{"line_number":477,"context_line":"        return True"}],"source_content_type":"text/x-python","patch_set":2,"id":"c0d9265e_6737c2c3","line":474,"in_reply_to":"c6d642ee_95e0df4c","updated":"2021-03-16 07:43:36.000000000","message":"My understanding is that when new defaults are used we shouldn\u0027t have something like \"is_admin\" in the policy checks as there are many different \"types\" of admins, like system-admin or project-admin. Because of that in policy module we should avoid checking if context.is_admin \u003d\u003d True and always perform rules checks. Because of that in first PS I simply removed that\n\n    if context.is_admin:\n        return True\n\npart from here. And that worked fine in my devstack env with new roles but it caused failure of about 3000 unit tests which we will need to modify to make it really working like that.\nSo I\u0027m proposing now to skip checking if context.is_admin only if enforce_new_defaults \u003d\u003d True. That way if user wants to use new defaults it will work as it should. If user still wants to use deprecated rules, it will simply return True quickly here. And that is really correct behaviour IMO. Lets see e.g. \"GET /agents\" call:\n\n- when enforce_new_defaults \u003d\u003d True - it has rule SYSTEM_READER and scope_types \u003d [\u0027system\u0027] which means that project admin can\u0027t list agents, only system admin/reader persona can do it. Now in the code in case of call made by PROJECT_ADMIN persona, context.is_admin \u003d\u003d True so without that patch such person would be able to list agents but with this patch not.\n\n- now, when enforce_new_defaults \u003d\u003d False - we should also respect deprecated rule for \"GET /agents\" which is base.RULE_ADMIN_ONLY - and that is true for both system admin and project admin personas, so in such case we can check if context.is_admin \u003d\u003d True and don\u0027t perform all checks in such case.\n\nI hope my understanding of that is correct and that I explained it clearly :)","commit_id":"10eeb530b43c766806039aadda65ac077de5194f"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"35f8a250521a786c4c8135bd4f4b9dca02025565","unresolved":true,"context_lines":[{"line_number":471,"context_line":"    # TODO(slaweq): Remove that is_admin check and always perform rules checks"},{"line_number":472,"context_line":"    # when old, deprecated rules will be removed and only rules with new"},{"line_number":473,"context_line":"    # personas will be supported"},{"line_number":474,"context_line":"    if not cfg.CONF.oslo_policy.enforce_new_defaults and context.is_admin:"},{"line_number":475,"context_line":"        return True"},{"line_number":476,"context_line":"    if might_not_exist and not (_ENFORCER.rules and action in _ENFORCER.rules):"},{"line_number":477,"context_line":"        return True"}],"source_content_type":"text/x-python","patch_set":2,"id":"b698c86c_56d18f06","line":474,"in_reply_to":"e40dceb7_765c6bcb","updated":"2021-03-18 03:41:53.000000000","message":"Thanks for the detail explanation and it makes sense. I was a bit confused when I wrote my last commet.\n\nThe most important point is to ensure that PROJECT_ADMIN cannot get access by \"context.is_admin\" check here in case of enforce_new_defaults \u003d\u003d True.\nIn case of enforce_new_defaults \u003d\u003d False, both SYSTEM_READER/SYSTEM_ADMIN/PROJECT_ADMIN can access the API. For SYSTEM_READER/SYSTEM_ADMIN, the normal policy check below is applied. For PROJECT_ADMIN, we can keep the access via context.is_admin check here.\n\nI believe I got the full context. Thanks.","commit_id":"10eeb530b43c766806039aadda65ac077de5194f"}],"neutron/tests/unit/test_policy.py":[{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"de54ff39696b1f7beb24de186c9209c4510fe3c3","unresolved":true,"context_lines":[{"line_number":442,"context_line":"    def _test_enforce_adminonly_attribute(self, action, **kwargs):"},{"line_number":443,"context_line":"        admin_context \u003d context.get_admin_context()"},{"line_number":444,"context_line":"        # TODO(slaweq): admin role should be set in admin_context automatically"},{"line_number":445,"context_line":"        # by get_admin_context() method"},{"line_number":446,"context_line":"        admin_context.roles.append(\u0027admin\u0027)"},{"line_number":447,"context_line":"        target \u003d {"},{"line_number":448,"context_line":"            \u0027tenant_id\u0027: \u0027some tenant\u0027,"}],"source_content_type":"text/x-python","patch_set":1,"id":"8606681e_32103513","line":445,"updated":"2021-03-15 16:12:31.000000000","message":"Once the system scope becomes the default, \"admin\" role no longer plays this role.\nPerhaps we need more work than just setting \"admin\" role automatically.\n\nI know this is a workaround to handle our current tests which assume \"admin\" role for system admin.\nI am fine with this workaround now.","commit_id":"eb3caeb88325fb10a81b3916575ac5e477fe1fcc"},{"author":{"_account_id":841,"name":"Akihiro Motoki","email":"amotoki@gmail.com","username":"amotoki"},"change_message_id":"02ad5344f0ef184be648695532b4b802a657265a","unresolved":true,"context_lines":[{"line_number":104,"context_line":"                policy.check(project_admin_ctx, action, self.target))"},{"line_number":105,"context_line":"        else:"},{"line_number":106,"context_line":"            self.assertTrue("},{"line_number":107,"context_line":"                policy.check(project_admin_ctx, action, self.target))"},{"line_number":108,"context_line":""},{"line_number":109,"context_line":"    def test_check_only_system_admin_new_defaults(self):"},{"line_number":110,"context_line":"        self._test_check_system_admin_allowed_action(enforce_new_defaults\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":3,"id":"3434d8ab_71deb7eb","line":107,"updated":"2021-03-18 08:59:38.000000000","message":"The key point of this test is when project_admin_ctx contains \"admin\" role this check always succeeds regardless of the policy rule evaluated, so this UT can be simplified more, but I am okay with this version.","commit_id":"759027a37644b750ef0c0fda854d8a6b0dd976a6"}]}