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.