diff --git a/etc/policy.v3cloudsample.json b/etc/policy.v3cloudsample.json index 2366de76c6..fc912bcbb2 100644 --- a/etc/policy.v3cloudsample.json +++ b/etc/policy.v3cloudsample.json @@ -25,13 +25,6 @@ "identity:update_project_tags": "rule:admin_required", "admin_and_matching_target_user_domain_id": "rule:admin_required and domain_id:%(target.user.domain_id)s", - "admin_and_matching_user_domain_id": "rule:admin_required and domain_id:%(user.domain_id)s", - "identity:get_user": "rule:cloud_admin or rule:admin_and_matching_target_user_domain_id or rule:owner", - "identity:list_users": "rule:cloud_admin or rule:admin_and_matching_domain_id", - "identity:create_user": "rule:cloud_admin or rule:admin_and_matching_user_domain_id", - "identity:update_user": "rule:cloud_admin or rule:admin_and_matching_target_user_domain_id", - "identity:delete_user": "rule:cloud_admin or rule:admin_and_matching_target_user_domain_id", - "admin_and_matching_target_group_domain_id": "rule:admin_required and domain_id:%(target.group.domain_id)s", "admin_and_matching_group_domain_id": "rule:admin_required and domain_id:%(group.domain_id)s", "identity:get_group": "rule:cloud_admin or rule:admin_and_matching_target_group_domain_id", diff --git a/keystone/api/users.py b/keystone/api/users.py index 7009c87146..cf2bf7a405 100644 --- a/keystone/api/users.py +++ b/keystone/api/users.py @@ -174,7 +174,20 @@ class UserResource(ks_flask.ResourceBase): domain = self.oslo_context.domain_id refs = PROVIDERS.identity_api.list_users( domain_scope=domain, hints=hints) - return self.wrap_collection(refs, hints=hints) + + # If the user making the request used a domain-scoped token, let's make + # sure we filter out users that are not in that domain. Otherwise, we'd + # be exposing users in other domains. This if statement is needed in + # case _get_domain_id_for_list_request() short-circuits due to + # configuration and protects against information from other domains + # leaking to people who shouldn't see it. + if self.oslo_context.domain_id: + domain_id = self.oslo_context.domain_id + users = [user for user in refs if user['domain_id'] == domain_id] + else: + users = refs + + return self.wrap_collection(users, hints=hints) def post(self): """Create a user. diff --git a/keystone/tests/unit/protection/v3/test_users.py b/keystone/tests/unit/protection/v3/test_users.py index ae5317efce..f73b2d57a0 100644 --- a/keystone/tests/unit/protection/v3/test_users.py +++ b/keystone/tests/unit/protection/v3/test_users.py @@ -746,6 +746,28 @@ class DomainAdminTests(base_classes.TestCaseWithBootstrap, with self.test_client() as c: c.post('/v3/users', json=create, headers=self.headers) + def test_user_cannot_create_users_within_domain_hyphened_domain_id(self): + # Finally, show that we can create a new user without any surprises. + # But if we specify a 'domain-id' instead of a 'domain_id', we get a + # Forbidden response because we fail a policy check before + # normalization occurs. + domain = PROVIDERS.resource_api.create_domain( + uuid.uuid4().hex, unit.new_domain_ref() + ) + + create = { + 'user': { + 'domain-id': domain['id'], + 'name': uuid.uuid4().hex + } + } + + with self.test_client() as c: + c.post( + '/v3/users', json=create, headers=self.headers, + expected_status_code=http_client.FORBIDDEN + ) + def test_user_cannot_create_users_in_other_domain(self): domain = PROVIDERS.resource_api.create_domain( uuid.uuid4().hex, unit.new_domain_ref() @@ -775,6 +797,26 @@ class DomainAdminTests(base_classes.TestCaseWithBootstrap, '/v3/users/%s' % user['id'], json=update, headers=self.headers ) + def test_user_can_update_users_within_domain_hyphened_domain_id(self): + # If we try updating the user's 'domain_id' by specifying a + # 'domain-id', then it'll be stored into extras rather than normalized, + # and the user's actual 'domain_id' is not affected. + domain = PROVIDERS.resource_api.create_domain( + uuid.uuid4().hex, unit.new_domain_ref() + ) + + user = PROVIDERS.identity_api.create_user( + unit.new_user_ref(domain_id=self.domain_id) + ) + + update = {'user': {'domain-id': domain['id']}} + with self.test_client() as c: + r = c.patch( + '/v3/users/%s' % user['id'], json=update, headers=self.headers + ) + self.assertEqual(domain['id'], r.json['user']['domain-id']) + self.assertEqual(self.domain_id, r.json['user']['domain_id']) + def test_user_cannot_update_users_in_other_domain(self): domain = PROVIDERS.resource_api.create_domain( uuid.uuid4().hex, unit.new_domain_ref() diff --git a/keystone/tests/unit/test_policy.py b/keystone/tests/unit/test_policy.py index a7d7259fa5..cc0a897492 100644 --- a/keystone/tests/unit/test_policy.py +++ b/keystone/tests/unit/test_policy.py @@ -196,6 +196,7 @@ class PolicyJsonTestCase(unit.TestCase): 'identity:list_service_providers', 'identity:update_service_provider', 'identity:delete_service_provider', + 'identity:list_role_assignments', 'identity:create_role', 'identity:get_role', 'identity:list_roles', @@ -250,7 +251,11 @@ class PolicyJsonTestCase(unit.TestCase): 'identity:list_services', 'identity:update_service', 'identity:delete_service', - 'identity:list_role_assignments' + 'identity:create_user', + 'identity:get_user', + 'identity:list_users', + 'identity:update_user', + 'identity:delete_user' ] policy_keys = self._get_default_policy_rules() for p in removed_policies: diff --git a/keystone/tests/unit/test_v3_protection.py b/keystone/tests/unit/test_v3_protection.py index 5e430683cd..0ee91c7144 100644 --- a/keystone/tests/unit/test_v3_protection.py +++ b/keystone/tests/unit/test_v3_protection.py @@ -793,25 +793,6 @@ class IdentityTestv3CloudPolicySample(test_v3.RestfulTestCase, else: return (expected_status, expected_status, expected_status) - def _test_user_management(self, user_id, domain_id, expected=None): - status_OK, status_created, status_no_data = self._stati(expected) - entity_url = '/users/%s' % user_id - list_url = '/users?domain_id=%s' % domain_id - - self.get(entity_url, auth=self.auth, - expected_status=status_OK) - self.get(list_url, auth=self.auth, - expected_status=status_OK) - user = {'description': 'Updated'} - self.patch(entity_url, auth=self.auth, body={'user': user}, - expected_status=status_OK) - self.delete(entity_url, auth=self.auth, - expected_status=status_no_data) - - user_ref = unit.new_user_ref(domain_id=domain_id) - self.post('/users', auth=self.auth, body={'user': user_ref}, - expected_status=status_created) - def _test_group_management(self, group, expected=None): status_OK, status_created, status_no_data = self._stati(expected) entity_url = '/groups/%s' % group['id'] @@ -938,113 +919,6 @@ class IdentityTestv3CloudPolicySample(test_v3.RestfulTestCase, self.post('/roles', auth=self.auth, body={'role': role_ref}, expected_status=status_created) - def test_user_management(self): - # First, authenticate with a user that does not have the domain - # admin role - shouldn't be able to do much. - self.auth = self.build_authentication_request( - user_id=self.just_a_user['id'], - password=self.just_a_user['password'], - domain_id=self.domainA['id']) - - self._test_user_management( - self.domain_admin_user['id'], self.domainA['id'], - expected=exception.ForbiddenAction.code) - - # Now, authenticate with a user that does have the domain admin role - self.auth = self.build_authentication_request( - user_id=self.domain_admin_user['id'], - password=self.domain_admin_user['password'], - domain_id=self.domainA['id']) - - self._test_user_management(self.just_a_user['id'], self.domainA['id']) - - def test_user_management_normalized_keys(self): - """Illustrate the inconsistent handling of hyphens in keys. - - To quote Morgan in bug 1526244: - - the reason this is converted from "domain-id" to "domain_id" is - because of how we process/normalize data. The way we have to handle - specific data types for known columns requires avoiding "-" in the - actual python code since "-" is not valid for attributes in python - w/o significant use of "getattr" etc. - - In short, historically we handle some things in conversions. The - use of "extras" has long been a poor design choice that leads to - odd/strange inconsistent behaviors because of other choices made in - handling data from within the body. (In many cases we convert from - "-" to "_" throughout openstack) - - Source: https://bugs.launchpad.net/keystone/+bug/1526244/comments/9 - - """ - # Authenticate with a user that has the domain admin role - self.auth = self.build_authentication_request( - user_id=self.domain_admin_user['id'], - password=self.domain_admin_user['password'], - domain_id=self.domainA['id']) - - # Show that we can read a normal user without any surprises. - r = self.get( - '/users/%s' % self.just_a_user['id'], - auth=self.auth, - expected_status=http_client.OK) - self.assertValidUserResponse(r) - - # We don't normalize query string keys, so both of these result in a - # 403, because we didn't specify a domain_id query string in either - # case, and we explicitly require one (it doesn't matter what - # 'domain-id' value you use). - self.get( - '/users?domain-id=%s' % self.domainA['id'], - auth=self.auth, - expected_status=exception.ForbiddenAction.code) - self.get( - '/users?domain-id=%s' % self.domainB['id'], - auth=self.auth, - expected_status=exception.ForbiddenAction.code) - - # If we try updating the user's 'domain_id' by specifying a - # 'domain-id', then it'll be stored into extras rather than normalized, - # and the user's actual 'domain_id' is not affected. - r = self.patch( - '/users/%s' % self.just_a_user['id'], - auth=self.auth, - body={'user': {'domain-id': self.domainB['id']}}, - expected_status=http_client.OK) - self.assertEqual(self.domainB['id'], r.json['user']['domain-id']) - self.assertEqual(self.domainA['id'], r.json['user']['domain_id']) - self.assertNotEqual(self.domainB['id'], self.just_a_user['domain_id']) - self.assertValidUserResponse(r, self.just_a_user) - - # Finally, show that we can create a new user without any surprises. - # But if we specify a 'domain-id' instead of a 'domain_id', we get a - # Forbidden response because we fail a policy check before - # normalization occurs. - user_ref = unit.new_user_ref(domain_id=self.domainA['id']) - r = self.post( - '/users', - auth=self.auth, - body={'user': user_ref}, - expected_status=http_client.CREATED) - self.assertValidUserResponse(r, ref=user_ref) - user_ref['domain-id'] = user_ref.pop('domain_id') - self.post( - '/users', - auth=self.auth, - body={'user': user_ref}, - expected_status=exception.ForbiddenAction.code) - - def test_user_management_by_cloud_admin(self): - # Test users management with a cloud admin. This user should - # be able to manage users in any domain. - self.auth = self.build_authentication_request( - user_id=self.cloud_admin_user['id'], - password=self.cloud_admin_user['password'], - project_id=self.admin_project['id']) - - self._test_user_management(self.just_a_user['id'], self.domainA['id']) - def test_group_management(self): # First, authenticate with a user that does not have the domain # admin role - shouldn't be able to do much. diff --git a/releasenotes/notes/bug-1806762-09f414995924db23.yaml b/releasenotes/notes/bug-1806762-09f414995924db23.yaml new file mode 100644 index 0000000000..891c9baec9 --- /dev/null +++ b/releasenotes/notes/bug-1806762-09f414995924db23.yaml @@ -0,0 +1,15 @@ +--- +upgrade: + - | + [`bug 1806762 `_] + The user policies defined in ``policy.v3cloudsample.json`` have + been removed. These policies are now obsolete after incorporating + system-scope, domain-scope, and project-scope into the user API + and implementing default roles. +fixes: + - | + [`bug 1806762 `_] + The user policies in ``policy.v3cloudsample.json`` policy file + have been removed in favor of better defaults in code. These + policies weren't tested exhaustively and were misleading to users + and operators.