From 05ea390c67da8056bd0cb4445f4f030d8181aaf6 Mon Sep 17 00:00:00 2001 From: Colleen Murphy Date: Tue, 17 Sep 2019 15:47:35 -0700 Subject: [PATCH] Allow system/domain scope for assignment tree list The comment regarding the scope_types setting for identity:list_role_assignments_for_tree was incorrect: the project ID for this request comes from a query parameter, not the token context, and therefore it makes sense to allow system users and domain users to call this API to get information about a project they have access to. This change updates the default policy for this API and adds tests for it. For project scope, the admin role is still required, as project members and project readers are typically not allowed rights to view the project hierarchy. Change-Id: If246298092940884a7b90e47cc9ce2f30da3e9e5 Closes-bug: #1844461 --- keystone/common/policies/role_assignment.py | 26 ++- .../tests/protection/v3/test_assignment.py | 214 +++++++++++++++++- .../notes/bug-1844461-08a8bdc5f613b88d.yaml | 31 +++ 3 files changed, 257 insertions(+), 14 deletions(-) create mode 100644 releasenotes/notes/bug-1844461-08a8bdc5f613b88d.yaml diff --git a/keystone/common/policies/role_assignment.py b/keystone/common/policies/role_assignment.py index 7a91e813d0..c70f292f38 100644 --- a/keystone/common/policies/role_assignment.py +++ b/keystone/common/policies/role_assignment.py @@ -19,11 +19,20 @@ SYSTEM_READER_OR_DOMAIN_READER = ( '(' + base.SYSTEM_READER + ') or ' '(role:reader and domain_id:%(target.domain_id)s)' ) +SYSTEM_READER_OR_PROJECT_DOMAIN_READER_OR_PROJECT_ADMIN = ( + '(' + base.SYSTEM_READER + ') or ' + '(role:reader and domain_id:%(target.project.domain_id)s) or ' + '(role:admin and project_id:%(target.project.id)s)' +) deprecated_list_role_assignments = policy.DeprecatedRule( name=base.IDENTITY % 'list_role_assignments', check_str=base.RULE_ADMIN_REQUIRED ) +deprecated_list_role_assignments_for_tree = policy.DeprecatedRule( + name=base.IDENTITY % 'list_role_assignments_for_tree', + check_str=base.RULE_ADMIN_REQUIRED +) DEPRECATED_REASON = ( "The assignment API is now aware of system scope and default roles." @@ -44,21 +53,18 @@ role_assignment_policies = [ deprecated_since=versionutils.deprecated.STEIN), policy.DocumentedRuleDefault( name=base.IDENTITY % 'list_role_assignments_for_tree', - check_str=base.RULE_ADMIN_REQUIRED, - # NOTE(lbragstad): This is purely a project-scoped operation. The - # project tree is calculated based on the project scope of the token - # used to make the request. System administrators would have to find a - # way to supply a project scope with a system-scoped token, which - # defeats the purpose. System administrators can list all role - # assignments anyway, so the usefulness of an API that returns a subset - # is negligible when they have access to the entire set. - scope_types=['project'], + check_str=SYSTEM_READER_OR_PROJECT_DOMAIN_READER_OR_PROJECT_ADMIN, + scope_types=['system', 'domain', 'project'], description=('List all role assignments for a given tree of ' 'hierarchical projects.'), operations=[{'path': '/v3/role_assignments?include_subtree', 'method': 'GET'}, {'path': '/v3/role_assignments?include_subtree', - 'method': 'HEAD'}]) + 'method': 'HEAD'}], + deprecated_rule=deprecated_list_role_assignments_for_tree, + deprecated_reason=DEPRECATED_REASON, + deprecated_since=versionutils.deprecated.TRAIN), + ] diff --git a/keystone/tests/protection/v3/test_assignment.py b/keystone/tests/protection/v3/test_assignment.py index a5bc13d44c..d275ba45ae 100644 --- a/keystone/tests/protection/v3/test_assignment.py +++ b/keystone/tests/protection/v3/test_assignment.py @@ -10,6 +10,7 @@ # License for the specific language governing permissions and limitations # under the License. +import copy from six.moves import http_client import uuid @@ -653,6 +654,49 @@ class _SystemUserTests(object): for assignment in actual: self.assertIn(assignment, expected) + def test_user_can_list_assignments_for_subtree(self): + assignments = self._setup_test_role_assignments() + user = PROVIDERS.identity_api.create_user( + unit.new_user_ref(domain_id=CONF.identity.default_domain_id) + ) + project = PROVIDERS.resource_api.create_project( + uuid.uuid4().hex, + unit.new_project_ref(domain_id=CONF.identity.default_domain_id, + parent_id=assignments['project_id']) + ) + PROVIDERS.assignment_api.create_grant( + assignments['role_id'], + user_id=user['id'], + project_id=project['id'] + ) + expected = [ + { + 'user_id': assignments['user_id'], + 'project_id': assignments['project_id'], + 'role_id': assignments['role_id'] + }, + { + 'group_id': assignments['group_id'], + 'project_id': assignments['project_id'], + 'role_id': assignments['role_id'] + }, + { + 'user_id': user['id'], + 'project_id': project['id'], + 'role_id': assignments['role_id'] + } + ] + with self.test_client() as c: + r = c.get( + ('/v3/role_assignments?scope.project.id=%s&include_subtree' % + assignments['project_id']), + headers=self.headers + ) + self.assertEqual(len(expected), len(r.json['role_assignments'])) + actual = self._extract_role_assignments_from_response_body(r) + for assignment in actual: + self.assertIn(assignment, expected) + class _DomainUserTests(object): """Common functionality for domain users.""" @@ -922,6 +966,60 @@ class _DomainUserTests(object): ) self.assertEqual(0, len(r.json['role_assignments'])) + def test_user_can_list_assignments_for_subtree_in_their_domain(self): + assignments = self._setup_test_role_assignments() + domain_assignments = self._setup_test_role_assignments_for_domain() + user = PROVIDERS.identity_api.create_user( + unit.new_user_ref(domain_id=self.domain_id) + ) + project = PROVIDERS.resource_api.create_project( + uuid.uuid4().hex, + unit.new_project_ref(domain_id=self.domain_id, + parent_id=domain_assignments['project_id']) + ) + PROVIDERS.assignment_api.create_grant( + assignments['role_id'], + user_id=user['id'], + project_id=project['id'] + ) + expected = [ + { + 'user_id': domain_assignments['user_id'], + 'project_id': domain_assignments['project_id'], + 'role_id': assignments['role_id'] + }, + { + 'group_id': domain_assignments['group_id'], + 'project_id': domain_assignments['project_id'], + 'role_id': assignments['role_id'] + }, + { + 'user_id': user['id'], + 'project_id': project['id'], + 'role_id': assignments['role_id'] + } + ] + with self.test_client() as c: + r = c.get( + ('/v3/role_assignments?scope.project.id=%s&include_subtree' % + domain_assignments['project_id']), + headers=self.headers + ) + self.assertEqual(len(expected), len(r.json['role_assignments'])) + actual = self._extract_role_assignments_from_response_body(r) + for assignment in actual: + self.assertIn(assignment, expected) + + def test_user_cannot_list_assignments_for_subtree_in_other_domain(self): + assignments = self._setup_test_role_assignments() + with self.test_client() as c: + c.get( + ('/v3/role_assignments?scope.project.id=%s&include_subtree' % + assignments['project_id']), + headers=self.headers, + expected_status_code=http_client.FORBIDDEN + ) + class _ProjectUserTests(object): @@ -1016,6 +1114,30 @@ class _ProjectUserTests(object): ) +class _ProjectReaderMemberTests(object): + def test_user_cannot_list_assignments_for_subtree(self): + user = PROVIDERS.identity_api.create_user( + unit.new_user_ref(domain_id=self.domain_id) + ) + project = PROVIDERS.resource_api.create_project( + uuid.uuid4().hex, + unit.new_project_ref(domain_id=self.domain_id, + parent_id=self.project_id) + ) + PROVIDERS.assignment_api.create_grant( + self.bootstrapper.reader_role_id, + user_id=user['id'], + project_id=project['id'] + ) + with self.test_client() as c: + c.get( + ('/v3/role_assignments?scope.project.id=%s&include_subtree' % + self.project_id), + headers=self.headers, + expected_status_code=http_client.FORBIDDEN + ) + + class SystemReaderTests(base_classes.TestCaseWithBootstrap, common_auth.AuthTestMixin, _AssignmentTestUtilities, @@ -1216,10 +1338,37 @@ class DomainAdminTests(base_classes.TestCaseWithBootstrap, _AssignmentTestUtilities, _DomainUserTests): + def _override_policy(self): + # TODO(lbragstad): Remove this once the deprecated policies in + # keystone.common.policies.role_assignment have been removed. This is + # only here to make sure we test the new policies instead of the + # deprecated ones. Oslo.policy will OR deprecated policies with new + # policies to maintain compatibility and give operators a chance to + # update permissions or update policies without breaking users. This + # will cause these specific tests to fail since we're trying to correct + # this broken behavior with better scope checking. + with open(self.policy_file_name, 'w') as f: + overridden_policies = { + 'identity:list_role_assignments': ( + rp.SYSTEM_READER_OR_DOMAIN_READER + ), + 'identity:list_role_assignments_for_tree': ( + rp.SYSTEM_READER_OR_PROJECT_DOMAIN_READER_OR_PROJECT_ADMIN + ) + } + f.write(jsonutils.dumps(overridden_policies)) + def setUp(self): super(DomainAdminTests, self).setUp() self.loadapp() - self.useFixture(ksfixtures.Policy(self.config_fixture)) + self.policy_file = self.useFixture(temporaryfile.SecureTempFile()) + self.policy_file_name = self.policy_file.file_name + self.useFixture( + ksfixtures.Policy( + self.config_fixture, policy_file=self.policy_file_name + ) + ) + self._override_policy() self.config_fixture.config(group='oslo_policy', enforce_scope=True) domain = PROVIDERS.resource_api.create_domain( @@ -1256,7 +1405,8 @@ class DomainAdminTests(base_classes.TestCaseWithBootstrap, class ProjectReaderTests(base_classes.TestCaseWithBootstrap, common_auth.AuthTestMixin, _AssignmentTestUtilities, - _ProjectUserTests): + _ProjectUserTests, + _ProjectReaderMemberTests): def setUp(self): super(ProjectReaderTests, self).setUp() @@ -1304,7 +1454,8 @@ class ProjectReaderTests(base_classes.TestCaseWithBootstrap, class ProjectMemberTests(base_classes.TestCaseWithBootstrap, common_auth.AuthTestMixin, _AssignmentTestUtilities, - _ProjectUserTests): + _ProjectUserTests, + _ProjectReaderMemberTests): def setUp(self): super(ProjectMemberTests, self).setUp() @@ -1394,7 +1545,7 @@ class ProjectAdminTests(base_classes.TestCaseWithBootstrap, auth = self.build_authentication_request( user_id=self.user_id, password=self.bootstrapper.admin_password, - project_id=self.bootstrapper.project_id + project_id=self.project_id ) # Grab a token using the persona we're testing and prepare headers @@ -1417,6 +1568,61 @@ class ProjectAdminTests(base_classes.TestCaseWithBootstrap, overridden_policies = { 'identity:list_role_assignments': ( rp.SYSTEM_READER_OR_DOMAIN_READER + ), + 'identity:list_role_assignments_for_tree': ( + rp.SYSTEM_READER_OR_PROJECT_DOMAIN_READER_OR_PROJECT_ADMIN ) } f.write(jsonutils.dumps(overridden_policies)) + + def test_user_can_list_assignments_for_subtree_on_own_project(self): + user = PROVIDERS.identity_api.create_user( + unit.new_user_ref(domain_id=self.domain_id) + ) + project = PROVIDERS.resource_api.create_project( + uuid.uuid4().hex, + unit.new_project_ref(domain_id=self.domain_id, + parent_id=self.project_id) + ) + PROVIDERS.assignment_api.create_grant( + self.bootstrapper.reader_role_id, + user_id=user['id'], + project_id=project['id'] + ) + expected = copy.copy(self.expected) + expected.append({ + 'project_id': project['id'], + 'user_id': user['id'], + 'role_id': self.bootstrapper.reader_role_id + }) + with self.test_client() as c: + r = c.get( + ('/v3/role_assignments?scope.project.id=%s&include_subtree' % + self.project_id), + headers=self.headers + ) + self.assertEqual(len(expected), len(r.json['role_assignments'])) + actual = self._extract_role_assignments_from_response_body(r) + for assignment in actual: + self.assertIn(assignment, expected) + + def test_user_cannot_list_assignments_for_subtree_on_other_project(self): + user = PROVIDERS.identity_api.create_user( + unit.new_user_ref(domain_id=self.domain_id) + ) + project = PROVIDERS.resource_api.create_project( + uuid.uuid4().hex, + unit.new_project_ref(domain_id=self.domain_id) + ) + PROVIDERS.assignment_api.create_grant( + self.bootstrapper.reader_role_id, + user_id=user['id'], + project_id=project['id'] + ) + with self.test_client() as c: + c.get( + ('/v3/role_assignments?scope.project.id=%s&include_subtree' % + project['id']), + headers=self.headers, + expected_status_code=http_client.FORBIDDEN + ) diff --git a/releasenotes/notes/bug-1844461-08a8bdc5f613b88d.yaml b/releasenotes/notes/bug-1844461-08a8bdc5f613b88d.yaml new file mode 100644 index 0000000000..4ea2904426 --- /dev/null +++ b/releasenotes/notes/bug-1844461-08a8bdc5f613b88d.yaml @@ -0,0 +1,31 @@ +--- +features: + - | + [`bug 1844461 `_] + Listing role assignments for a project subtree is now allowed by system + readers and domain readers in addition to project admins. +upgrade: + - | + [`bug 1844461 `_] + The ``identity:list_role_assignments_for_subtree`` policy now allows system + and domain readers to list role assignments for a project subtree and + deprecates the old ``rule:admin_required`` policy check string. Please + consider the new policies if your deployment overrides role + assignment policies. +deprecations: + - | + [`bug 1844461 `_] + The role assignment ``identity:list_role_assignments_for_subtree`` policy + now uses ``(role:reader and system_scope:all) or (role:reader and + domain_id:%(target.project.domain_id)s) or (role:admin and + project_id:%(target.project.id)s)`` instead of ``rule:admin_required``. + This new default automatically includes support for a read-only role + and allows for more granular access to the role assignment API. Please + consider this new default if your deployment overrides the role + assignment policies. +security: + - | + [`bug 1844461 `_] + Listing role assignments for a project subtree now uses system-scope, + domain-scope, project-scope, and default roles to provide better + accessbility to users in a secure way.