Merge "Consolidate user protection tests"
This commit is contained in:
Zuul
Gerrit Code Review
commit
947e0a2e39
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -0,0 +1,15 @@
|
|||
---
|
||||
upgrade:
|
||||
- |
|
||||
[`bug 1806762 <https://bugs.launchpad.net/keystone/+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 <https://bugs.launchpad.net/keystone/+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.
|
||||
Loading…
Reference in New Issue