)]}'
{"nova/policies/base.py":[{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"f81eadaef51a30d25b8aa7415779e111e78bd437","unresolved":false,"context_lines":[{"line_number":20,"context_line":"RULE_ANY \u003d \u0027@\u0027"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"rules \u003d ["},{"line_number":23,"context_line":"    policy.RuleDefault(\u0027context_is_admin\u0027, \u0027role:admin or is_admin:True\u0027),"},{"line_number":24,"context_line":"    policy.RuleDefault("},{"line_number":25,"context_line":"        \u0027admin_api\u0027,"},{"line_number":26,"context_line":"        \u0027(role:admin and is_admin_project:True) or is_admin:True\u0027),"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa6399be_8a3789af","line":23,"updated":"2016-10-11 18:36:50.000000000","message":"This redundant right? is_admin:True is set by the \u0027context_is_admin\u0027 policy passing.","commit_id":"a0be147bc55f6689eb8b4cf1309e63a445f9425a"},{"author":{"_account_id":2218,"name":"Adam Young","email":"adam@younglogic.com","username":"ayoung"},"change_message_id":"a21a6710c7ab6248de9898d2f721f2c31ffa1a25","unresolved":false,"context_lines":[{"line_number":20,"context_line":"RULE_ANY \u003d \u0027@\u0027"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"rules \u003d ["},{"line_number":23,"context_line":"    policy.RuleDefault(\u0027context_is_admin\u0027, \u0027role:admin or is_admin:True\u0027),"},{"line_number":24,"context_line":"    policy.RuleDefault("},{"line_number":25,"context_line":"        \u0027admin_api\u0027,"},{"line_number":26,"context_line":"        \u0027(role:admin and is_admin_project:True) or is_admin:True\u0027),"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa6399be_ac9cfbc0","line":23,"in_reply_to":"fa6399be_07c78e64","updated":"2016-10-13 20:09:26.000000000","message":"Excpet that then the unit tests all fail.  I think some are explicitly passing in is_admin true to the tests, and I don\u0027t want to mess with them.  We can clean that up in a follow on.","commit_id":"a0be147bc55f6689eb8b4cf1309e63a445f9425a"},{"author":{"_account_id":10608,"name":"Matthew Edmonds","email":"edmondsw@us.ibm.com","username":"edmondsw"},"change_message_id":"a6d4816f77ecfbad9f2ae8f18938869d2efea06c","unresolved":false,"context_lines":[{"line_number":20,"context_line":"RULE_ANY \u003d \u0027@\u0027"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"rules \u003d ["},{"line_number":23,"context_line":"    policy.RuleDefault(\u0027context_is_admin\u0027, \u0027role:admin or is_admin:True\u0027),"},{"line_number":24,"context_line":"    policy.RuleDefault("},{"line_number":25,"context_line":"        \u0027admin_api\u0027,"},{"line_number":26,"context_line":"        \u0027(role:admin and is_admin_project:True) or is_admin:True\u0027),"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa6399be_07c78e64","line":23,"in_reply_to":"fa6399be_8a3789af","updated":"2016-10-13 16:50:16.000000000","message":"+1. See https://github.com/openstack/nova/blob/master/nova/context.py#L129 and https://github.com/openstack/nova/blob/master/nova/policy.py#L181.","commit_id":"a0be147bc55f6689eb8b4cf1309e63a445f9425a"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"f81eadaef51a30d25b8aa7415779e111e78bd437","unresolved":false,"context_lines":[{"line_number":22,"context_line":"rules \u003d ["},{"line_number":23,"context_line":"    policy.RuleDefault(\u0027context_is_admin\u0027, \u0027role:admin or is_admin:True\u0027),"},{"line_number":24,"context_line":"    policy.RuleDefault("},{"line_number":25,"context_line":"        \u0027admin_api\u0027,"},{"line_number":26,"context_line":"        \u0027(role:admin and is_admin_project:True) or is_admin:True\u0027),"},{"line_number":27,"context_line":"    policy.RuleDefault("},{"line_number":28,"context_line":"        \u0027admin_or_owner\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa6399be_ad79c795","line":25,"updated":"2016-10-11 18:36:50.000000000","message":"Is there a reason you switched the ordering of the rules?\n\nThe issue with this is that you\u0027re changing the meaning of the admin_api rule, a meaning that deployers/users may rely on. There\u0027s no deprecation ability like with config but this new rule should be phased in. I would suggest adding a new rule like \u0027global_admin\u0027 or \u0027really_an_admin\" and check it wherever \u0027admin_api\u0027 is checked. Then add logging for the case where \u0027admin_api\u0027 passes and \u0027global_admin\u0027 doesn\u0027t indicating that \u0027admin_api\u0027 is being phased out. Then after a proper deprecation period remove \u0027admin_api\u0027 or convert it to match \u0027global_admin\u0027.","commit_id":"a0be147bc55f6689eb8b4cf1309e63a445f9425a"},{"author":{"_account_id":2218,"name":"Adam Young","email":"adam@younglogic.com","username":"ayoung"},"change_message_id":"a21a6710c7ab6248de9898d2f721f2c31ffa1a25","unresolved":false,"context_lines":[{"line_number":22,"context_line":"rules \u003d ["},{"line_number":23,"context_line":"    policy.RuleDefault(\u0027context_is_admin\u0027, \u0027role:admin or is_admin:True\u0027),"},{"line_number":24,"context_line":"    policy.RuleDefault("},{"line_number":25,"context_line":"        \u0027admin_api\u0027,"},{"line_number":26,"context_line":"        \u0027(role:admin and is_admin_project:True) or is_admin:True\u0027),"},{"line_number":27,"context_line":"    policy.RuleDefault("},{"line_number":28,"context_line":"        \u0027admin_or_owner\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa6399be_2c6aabb3","line":25,"in_reply_to":"fa6399be_684be887","updated":"2016-10-13 20:09:26.000000000","message":"Without this change, the unit tests all break.  \n\nIt is horrible, but not nearly as horrible as leaving this bug open for another release. Really, the default is what we are doing now. I had proposed a comparable hack inside the Keystone server, but the net effect is the same.\n\nIn two releases we can make this mandatory.","commit_id":"a0be147bc55f6689eb8b4cf1309e63a445f9425a"},{"author":{"_account_id":2218,"name":"Adam Young","email":"adam@younglogic.com","username":"ayoung"},"change_message_id":"500a6838b686a84409f315ea58245d5c9d0855e7","unresolved":false,"context_lines":[{"line_number":22,"context_line":"rules \u003d ["},{"line_number":23,"context_line":"    policy.RuleDefault(\u0027context_is_admin\u0027, \u0027role:admin or is_admin:True\u0027),"},{"line_number":24,"context_line":"    policy.RuleDefault("},{"line_number":25,"context_line":"        \u0027admin_api\u0027,"},{"line_number":26,"context_line":"        \u0027(role:admin and is_admin_project:True) or is_admin:True\u0027),"},{"line_number":27,"context_line":"    policy.RuleDefault("},{"line_number":28,"context_line":"        \u0027admin_or_owner\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa6399be_f9d1c1fc","line":25,"in_reply_to":"fa6399be_ad79c795","updated":"2016-10-11 20:47:22.000000000","message":"I switched the order to make things clearer.  I can switch them back, as it will make the review eaiser to rad.\n\nThere is no need for a deprecation here, and in fact, it would actively hurt things.\n\nThe change, as implemented, would have no impact.  is_admin_project needs to be actively set in the keystone server before it shows up as False in a token.  If nothing is set in Keystone, then all the rules still parse as they do today, as is shown by the fact that all of the existing policy tests still pass. I merely added tests for the new cases.","commit_id":"a0be147bc55f6689eb8b4cf1309e63a445f9425a"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"3ed876228174ef8760b9f181e41ea192a2d89fbb","unresolved":false,"context_lines":[{"line_number":22,"context_line":"rules \u003d ["},{"line_number":23,"context_line":"    policy.RuleDefault(\u0027context_is_admin\u0027, \u0027role:admin or is_admin:True\u0027),"},{"line_number":24,"context_line":"    policy.RuleDefault("},{"line_number":25,"context_line":"        \u0027admin_api\u0027,"},{"line_number":26,"context_line":"        \u0027(role:admin and is_admin_project:True) or is_admin:True\u0027),"},{"line_number":27,"context_line":"    policy.RuleDefault("},{"line_number":28,"context_line":"        \u0027admin_or_owner\u0027,"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa6399be_684be887","line":25,"in_reply_to":"fa6399be_f9d1c1fc","updated":"2016-10-12 20:10:26.000000000","message":"I see now. What I missed is that https://review.openstack.org/#/c/331916 adds is_admin_project to the context.to_policy_values() dict and that it defaults to True. That seems like a horrible change from a security standpoint, but means that this change is backwards compatible.","commit_id":"a0be147bc55f6689eb8b4cf1309e63a445f9425a"},{"author":{"_account_id":10608,"name":"Matthew Edmonds","email":"edmondsw@us.ibm.com","username":"edmondsw"},"change_message_id":"a6d4816f77ecfbad9f2ae8f18938869d2efea06c","unresolved":false,"context_lines":[{"line_number":23,"context_line":"    policy.RuleDefault(\u0027context_is_admin\u0027, \u0027role:admin or is_admin:True\u0027),"},{"line_number":24,"context_line":"    policy.RuleDefault("},{"line_number":25,"context_line":"        \u0027admin_api\u0027,"},{"line_number":26,"context_line":"        \u0027(role:admin and is_admin_project:True) or is_admin:True\u0027),"},{"line_number":27,"context_line":"    policy.RuleDefault("},{"line_number":28,"context_line":"        \u0027admin_or_owner\u0027,"},{"line_number":29,"context_line":"        \u0027rule:admin_api or project_id:%(project_id)s\u0027),"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa6399be_ed399bd8","line":26,"updated":"2016-10-13 16:50:16.000000000","message":"role:admin and is_admin:True are equivalent, since is_admin is set based on context_is_admin and context_is_admin is \"role:admin\", so the is_admin_project:True here really has no effect. At least by default. Are you trying to cover some case where someone customizes context_is_admin to check something other than role:admin? But I still don\u0027t think this would be right...\n\nI think what you really want here is \u0027is_admin:True and is_admin_project:True\u0027. Then you\u0027ll also need to change the defaults for all the nova/policies/*.py that were using base.RULE_ADMIN_API (except for https://github.com/openstack/nova/blob/master/nova/policies/servers.py#L25-L27 and https://github.com/openstack/nova/blob/master/nova/policies/fping.py#L26-L28 which really should be restricted to is_admin_project:True) to check \u0027is_admin:True and project_id:%(project_id)s\u0027... notice that is an \u0027and\u0027, not an \u0027or\u0027, so it\u0027s not the same as admin_or_owner.","commit_id":"a0be147bc55f6689eb8b4cf1309e63a445f9425a"},{"author":{"_account_id":2218,"name":"Adam Young","email":"adam@younglogic.com","username":"ayoung"},"change_message_id":"a21a6710c7ab6248de9898d2f721f2c31ffa1a25","unresolved":false,"context_lines":[{"line_number":23,"context_line":"    policy.RuleDefault(\u0027context_is_admin\u0027, \u0027role:admin or is_admin:True\u0027),"},{"line_number":24,"context_line":"    policy.RuleDefault("},{"line_number":25,"context_line":"        \u0027admin_api\u0027,"},{"line_number":26,"context_line":"        \u0027(role:admin and is_admin_project:True) or is_admin:True\u0027),"},{"line_number":27,"context_line":"    policy.RuleDefault("},{"line_number":28,"context_line":"        \u0027admin_or_owner\u0027,"},{"line_number":29,"context_line":"        \u0027rule:admin_api or project_id:%(project_id)s\u0027),"}],"source_content_type":"text/x-python","patch_set":2,"id":"fa6399be_ac4ebb6c","line":26,"in_reply_to":"fa6399be_ed399bd8","updated":"2016-10-13 20:09:26.000000000","message":"Looking at the APIs tagged as Admin API, they seem to be the ones that do not require the project check, which is why I chose this approach.","commit_id":"a0be147bc55f6689eb8b4cf1309e63a445f9425a"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"84674606888e2f49169f9dcc23ffa6574f74a28c","unresolved":false,"context_lines":[{"line_number":20,"context_line":"RULE_ANY \u003d \u0027@\u0027"},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"rules \u003d ["},{"line_number":23,"context_line":"    policy.RuleDefault(\u0027context_is_admin\u0027, \u0027role:admin or is_admin:True\u0027),"},{"line_number":24,"context_line":"    policy.RuleDefault("},{"line_number":25,"context_line":"        \u0027admin_or_owner\u0027,"},{"line_number":26,"context_line":"        \u0027rule:admin_api or project_id:%(project_id)s\u0027),"}],"source_content_type":"text/x-python","patch_set":3,"id":"fa6399be_8c7c97b6","line":23,"range":{"start_line":23,"start_character":55,"end_line":23,"end_character":71},"updated":"2016-10-13 20:44:12.000000000","message":"You mentioned that unit tests fail without this change but I think the reason for that needs to be tracked down and addressed. This check is redundant and it makes no sense that it would affect things. So if it is the reasoning should be traced.","commit_id":"ef2421208f46b809de7df33f1e5b5ffec63671c2"},{"author":{"_account_id":5441,"name":"Andrew Laski","email":"andrew@lascii.com","username":"alaski"},"change_message_id":"84674606888e2f49169f9dcc23ffa6574f74a28c","unresolved":false,"context_lines":[{"line_number":26,"context_line":"        \u0027rule:admin_api or project_id:%(project_id)s\u0027),"},{"line_number":27,"context_line":"    policy.RuleDefault("},{"line_number":28,"context_line":"        \u0027admin_api\u0027,"},{"line_number":29,"context_line":"        \u0027(role:admin and is_admin_project:True) or is_admin:True\u0027),"},{"line_number":30,"context_line":"    policy.RuleDefault(NETWORK_ATTACH_EXTERNAL, \u0027is_admin:True\u0027),"},{"line_number":31,"context_line":"]"},{"line_number":32,"context_line":""}],"source_content_type":"text/x-python","patch_set":3,"id":"fa6399be_4c0b5f2a","line":29,"range":{"start_line":29,"start_character":51,"end_line":29,"end_character":65},"updated":"2016-10-13 20:44:12.000000000","message":"To echo what Matthew was saying on patchset 2: the addition of this \u0027or is_admin:True\u0027 negates the \u0027and is_admin_project:True\u0027 addition. For example let\u0027s say we have a context with role:admin. Then the \u0027context_is_admin\u0027 check above is True so \u0027is_admin:True\u0027 passes which means the state of is_admin_project is meaningless.\n\nPut a different way, this policy check reduces to \u0027(role:admin and is_admin_project:True) or role:admin\u0027. There\u0027s no value in the is_admin_project check.\n\nYou seem hesitant to update the tests much but I think that\u0027s leading to changes here which don\u0027t make sense.","commit_id":"ef2421208f46b809de7df33f1e5b5ffec63671c2"},{"author":{"_account_id":2750,"name":"Sean Dague","email":"sean@dague.net","username":"sdague"},"change_message_id":"8d43775e8132b74777bc7dcc88423e360ea87377","unresolved":false,"context_lines":[{"line_number":23,"context_line":"rules \u003d ["},{"line_number":24,"context_line":"    policy.RuleDefault(\u0027context_is_admin\u0027, \u0027role:admin\u0027),"},{"line_number":25,"context_line":"    policy.RuleDefault(\u0027global_admin\u0027,"},{"line_number":26,"context_line":"        \u0027is_admin:True and is_admin_project:True\u0027),"},{"line_number":27,"context_line":"    policy.RuleDefault(\u0027admin_or_owner\u0027,"},{"line_number":28,"context_line":"        \u0027rule:global_admin or project_id:%(project_id)s\u0027),"},{"line_number":29,"context_line":"    policy.RuleDefault(\u0027admin_api\u0027,"}],"source_content_type":"text/x-python","patch_set":17,"id":"ff0f0b1f_853a61c9","line":26,"updated":"2017-05-22 16:38:14.000000000","message":"so, I don\u0027t really understand why we would leak through this implementation detail on is_admin_project to Nova instead of having keystonemiddleware and oslo.context process the abstraction for us.","commit_id":"39da426dbc975852971d03f16e058a897553cd3b"},{"author":{"_account_id":2218,"name":"Adam Young","email":"adam@younglogic.com","username":"ayoung"},"change_message_id":"1db2443c7ae1015695e54e6402f422b76b8b5172","unresolved":false,"context_lines":[{"line_number":23,"context_line":"rules \u003d ["},{"line_number":24,"context_line":"    policy.RuleDefault(\u0027context_is_admin\u0027, \u0027role:admin\u0027),"},{"line_number":25,"context_line":"    policy.RuleDefault(\u0027global_admin\u0027,"},{"line_number":26,"context_line":"        \u0027is_admin:True and is_admin_project:True\u0027),"},{"line_number":27,"context_line":"    policy.RuleDefault(\u0027admin_or_owner\u0027,"},{"line_number":28,"context_line":"        \u0027rule:global_admin or project_id:%(project_id)s\u0027),"},{"line_number":29,"context_line":"    policy.RuleDefault(\u0027admin_api\u0027,"}],"source_content_type":"text/x-python","patch_set":17,"id":"df140735_c2ae61e6","line":26,"in_reply_to":"ff0f0b1f_853a61c9","updated":"2017-05-30 18:35:06.000000000","message":"If we don\u0027t, Nova will reject all requests that don\u0027t match, providing no way to to a global override.\n\nIf nova does not know that role:admin  is_admin_project\u003dTrue means \"allow this through\" it will see that the project_id does not match that of the resource and reject it.","commit_id":"39da426dbc975852971d03f16e058a897553cd3b"}],"nova/tests/unit/test_policy.py":[{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"562689a12d5bce1f75a72e7b46a4457cae4c544e","unresolved":false,"context_lines":[{"line_number":270,"context_line":"        self.policy \u003d self.useFixture(policy_fixture.RealPolicyFixture())"},{"line_number":271,"context_line":"        self.non_admin_context \u003d context.RequestContext(\u0027fake\u0027, \u0027fake\u0027,"},{"line_number":272,"context_line":"                                                        roles\u003d[\u0027member\u0027])"},{"line_number":273,"context_line":"        self.elevated_context \u003d context.RequestContext(\u0027fake\u0027, \u0027fake\u0027, True,"},{"line_number":274,"context_line":"            roles\u003d[\u0027member\u0027], is_admin_project\u003dFalse)"},{"line_number":275,"context_line":"        self.global_admin_context \u003d context.RequestContext(\u0027fake\u0027, \u0027global\u0027,"},{"line_number":276,"context_line":"            roles\u003d[\u0027admin\u0027], is_admin_project\u003dTrue)"},{"line_number":277,"context_line":"        self.project_admin_context \u003d context.RequestContext(\u0027fake\u0027, \u0027fake\u0027,"}],"source_content_type":"text/x-python","patch_set":24,"id":"bf659307_bde42ee1","line":274,"range":{"start_line":273,"start_character":0,"end_line":274,"end_character":53},"updated":"2018-04-13 16:00:18.000000000","message":"It\u0027s going to get really confusing internally to refer to this \"it\u0027s admin but not really admin and isn\u0027t really a project admin\" thing as \"elevated context\".","commit_id":"c836c8481f80ac7a00031a74c119382e8694c66f"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"562689a12d5bce1f75a72e7b46a4457cae4c544e","unresolved":false,"context_lines":[{"line_number":274,"context_line":"            roles\u003d[\u0027member\u0027], is_admin_project\u003dFalse)"},{"line_number":275,"context_line":"        self.global_admin_context \u003d context.RequestContext(\u0027fake\u0027, \u0027global\u0027,"},{"line_number":276,"context_line":"            roles\u003d[\u0027admin\u0027], is_admin_project\u003dTrue)"},{"line_number":277,"context_line":"        self.project_admin_context \u003d context.RequestContext(\u0027fake\u0027, \u0027fake\u0027,"},{"line_number":278,"context_line":"            roles\u003d[\u0027admin\u0027], is_admin_project\u003dFalse)"},{"line_number":279,"context_line":"        self.target \u003d {}"},{"line_number":280,"context_line":"        self.fake_policy \u003d jsonutils.loads(fake_policy.policy_data)"},{"line_number":281,"context_line":""}],"source_content_type":"text/x-python","patch_set":24,"id":"bf659307_1d943a36","line":278,"range":{"start_line":277,"start_character":0,"end_line":278,"end_character":52},"updated":"2018-04-13 16:00:18.000000000","message":"So say that having a \"project_admin_context\" with is_admin_project\u003dFalse is confusing would be an understatement...","commit_id":"c836c8481f80ac7a00031a74c119382e8694c66f"}]}