diff --git a/keystone/api/users.py b/keystone/api/users.py index f31b5bf32f..7009c87146 100644 --- a/keystone/api/users.py +++ b/keystone/api/users.py @@ -181,8 +181,11 @@ class UserResource(ks_flask.ResourceBase): POST /v3/users """ - ENFORCER.enforce_call(action='identity:create_user') user_data = self.request_body_json.get('user', {}) + target = {'user': user_data} + ENFORCER.enforce_call( + action='identity:create_user', target_attr=target + ) validation.lazy_validate(schema.user_create, user_data) user_data = self._normalize_dict(user_data) user_data = self._normalize_domain_id(user_data) @@ -196,7 +199,11 @@ class UserResource(ks_flask.ResourceBase): PATCH /v3/users/{user_id} """ - ENFORCER.enforce_call(action='identity:update_user') + ENFORCER.enforce_call( + action='identity:update_user', + build_target=_build_user_target_enforcement + ) + PROVIDERS.identity_api.get_user(user_id) user_data = self.request_body_json.get('user', {}) validation.lazy_validate(schema.user_update, user_data) self._require_matching_id(user_data) diff --git a/keystone/common/policies/user.py b/keystone/common/policies/user.py index fd289bdc4b..7d1743f776 100644 --- a/keystone/common/policies/user.py +++ b/keystone/common/policies/user.py @@ -25,6 +25,11 @@ SYSTEM_READER_OR_DOMAIN_READER = ( '(' + base.SYSTEM_READER + ') or (' + base.DOMAIN_READER + ')' ) +SYSTEM_ADMIN_OR_DOMAIN_ADMIN = ( + '(role:admin and system_scope:all) or ' + '(role:admin and token.domain.id:%(target.user.domain_id)s)' +) + DEPRECATED_REASON = """ As of the Stein release, the user API understands how to handle system-scoped tokens in addition to project and domain tokens, making the API more accessible @@ -104,15 +109,8 @@ user_policies = [ 'method': 'GET'}]), policy.DocumentedRuleDefault( name=base.IDENTITY % 'create_user', - check_str=base.SYSTEM_ADMIN, - # FIXME(lbragstad): This can be considered either a system-level policy - # or a project-level policy. System administrator should have the - # ability to create users in any domain. Domain (or project) - # administrators should have the ability to create users in the domain - # they administer. The second case is going to require a policy check - # in code. Until that happens, we will leave this as a system-level - # policy. - scope_types=['system'], + check_str=SYSTEM_ADMIN_OR_DOMAIN_ADMIN, + scope_types=['system', 'domain'], description='Create a user.', operations=[{'path': '/v3/users', 'method': 'POST'}], @@ -121,10 +119,8 @@ user_policies = [ deprecated_since=versionutils.deprecated.STEIN), policy.DocumentedRuleDefault( name=base.IDENTITY % 'update_user', - check_str=base.SYSTEM_ADMIN, - # FIXME(lbragstad): See the above comment about adding support for - # project scope_types in the future. - scope_types=['system'], + check_str=SYSTEM_ADMIN_OR_DOMAIN_ADMIN, + scope_types=['system', 'domain'], description='Update a user, including administrative password resets.', operations=[{'path': '/v3/users/{user_id}', 'method': 'PATCH'}], @@ -133,10 +129,8 @@ user_policies = [ deprecated_since=versionutils.deprecated.STEIN), policy.DocumentedRuleDefault( name=base.IDENTITY % 'delete_user', - check_str=base.SYSTEM_ADMIN, - # FIXME(lbragstad): See the above comment about adding support for - # project scope_types in the future. - scope_types=['system'], + check_str=SYSTEM_ADMIN_OR_DOMAIN_ADMIN, + scope_types=['system', 'domain'], description='Delete a user.', operations=[{'path': '/v3/users/{user_id}', 'method': 'DELETE'}], diff --git a/keystone/tests/unit/protection/v3/test_users.py b/keystone/tests/unit/protection/v3/test_users.py index 89839bee40..c8da2fa210 100644 --- a/keystone/tests/unit/protection/v3/test_users.py +++ b/keystone/tests/unit/protection/v3/test_users.py @@ -12,14 +12,17 @@ import uuid +from oslo_serialization import jsonutils from six.moves import http_client +from keystone.common.policies import user as up from keystone.common import provider_api import keystone.conf from keystone.tests.common import auth as common_auth from keystone.tests import unit from keystone.tests.unit import base_classes from keystone.tests.unit import ksfixtures +from keystone.tests.unit.ksfixtures import temporaryfile CONF = keystone.conf.CONF PROVIDERS = provider_api.ProviderAPIs @@ -132,8 +135,8 @@ class _SystemMemberAndReaderUserTests(object): ) -class _DomainMemberAndReaderUserTests(object): - """Functionality for all domain members and domain readers.""" +class _DomainUserTests(object): + """Commont default functionality for all domain users.""" def test_user_can_get_user_within_domain(self): user = PROVIDERS.identity_api.create_user( @@ -189,6 +192,10 @@ class _DomainMemberAndReaderUserTests(object): user_ids.append(u['id']) self.assertNotIn(user['id'], user_ids) + +class _DomainMemberAndReaderUserTests(object): + """Functionality for all domain members and domain readers.""" + def test_user_cannot_create_users_within_domain(self): create = { 'user': { @@ -438,6 +445,7 @@ class SystemAdminTests(base_classes.TestCaseWithBootstrap, class DomainReaderTests(base_classes.TestCaseWithBootstrap, common_auth.AuthTestMixin, _CommonUserTests, + _DomainUserTests, _DomainMemberAndReaderUserTests): def setUp(self): @@ -473,6 +481,7 @@ class DomainReaderTests(base_classes.TestCaseWithBootstrap, class DomainMemberTests(base_classes.TestCaseWithBootstrap, common_auth.AuthTestMixin, _CommonUserTests, + _DomainUserTests, _DomainMemberAndReaderUserTests): def setUp(self): @@ -503,3 +512,161 @@ class DomainMemberTests(base_classes.TestCaseWithBootstrap, r = c.post('/v3/auth/tokens', json=auth) self.token_id = r.headers['X-Subject-Token'] self.headers = {'X-Auth-Token': self.token_id} + + +class DomainAdminTests(base_classes.TestCaseWithBootstrap, + common_auth.AuthTestMixin, + _CommonUserTests, + _DomainUserTests): + + def setUp(self): + super(DomainAdminTests, self).setUp() + self.loadapp() + + 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( + uuid.uuid4().hex, unit.new_domain_ref() + ) + self.domain_id = domain['id'] + domain_admin = unit.new_user_ref(domain_id=self.domain_id) + self.user_id = PROVIDERS.identity_api.create_user(domain_admin)['id'] + PROVIDERS.assignment_api.create_grant( + self.bootstrapper.admin_role_id, user_id=self.user_id, + domain_id=self.domain_id + ) + + auth = self.build_authentication_request( + user_id=self.user_id, password=domain_admin['password'], + domain_id=self.domain_id, + ) + + # Grab a token using the persona we're testing and prepare headers + # for requests we'll be making in the tests. + with self.test_client() as c: + r = c.post('/v3/auth/tokens', json=auth) + self.token_id = r.headers['X-Subject-Token'] + self.headers = {'X-Auth-Token': self.token_id} + + def _override_policy(self): + # TODO(lbragstad): Remove this once the deprecated policies in + # keystone.common.policies.users have been removed. This is only + # here to make sure we test the new policies instead of the deprecated + # ones. Oslo.policy will apply a logical OR to 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:get_user': up.SYSTEM_READER_OR_DOMAIN_READER_OR_USER, + 'identity:list_users': up.SYSTEM_READER_OR_DOMAIN_READER, + 'identity:create_user': up.SYSTEM_ADMIN_OR_DOMAIN_ADMIN, + 'identity:update_user': up.SYSTEM_ADMIN_OR_DOMAIN_ADMIN, + 'identity:delete_user': up.SYSTEM_ADMIN_OR_DOMAIN_ADMIN + } + f.write(jsonutils.dumps(overridden_policies)) + + def test_user_can_create_users_within_domain(self): + create = { + 'user': { + 'domain_id': self.domain_id, + 'name': uuid.uuid4().hex + } + } + + with self.test_client() as c: + c.post('/v3/users', json=create, headers=self.headers) + + def test_user_cannot_create_users_in_other_domain(self): + 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_can_update_users_within_domain(self): + user = PROVIDERS.identity_api.create_user( + unit.new_user_ref(domain_id=self.domain_id) + ) + + update = {'user': {'email': uuid.uuid4().hex}} + with self.test_client() as c: + c.patch( + '/v3/users/%s' % user['id'], json=update, headers=self.headers + ) + + def test_user_cannot_update_users_in_other_domain(self): + 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=domain['id']) + ) + + update = {'user': {'email': uuid.uuid4().hex}} + with self.test_client() as c: + c.patch( + '/v3/users/%s' % user['id'], json=update, headers=self.headers, + expected_status_code=http_client.FORBIDDEN + ) + + def test_user_cannot_update_non_existent_user_forbidden(self): + update = {'user': {'email': uuid.uuid4().hex}} + with self.test_client() as c: + c.patch( + '/v3/users/%s' % uuid.uuid4().hex, json=update, + headers=self.headers, + expected_status_code=http_client.FORBIDDEN + ) + + def test_user_can_delete_users_within_domain(self): + user = PROVIDERS.identity_api.create_user( + unit.new_user_ref(domain_id=self.domain_id) + ) + + with self.test_client() as c: + c.delete( + '/v3/users/%s' % user['id'], headers=self.headers + ) + + def test_user_cannot_delete_users_in_other_domain(self): + 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=domain['id']) + ) + + with self.test_client() as c: + c.delete( + '/v3/users/%s' % user['id'], headers=self.headers, + expected_status_code=http_client.FORBIDDEN + ) + + def test_user_cannot_delete_non_existent_user_forbidden(self): + with self.test_client() as c: + c.delete( + '/v3/users/%s' % uuid.uuid4().hex, headers=self.headers, + expected_status_code=http_client.FORBIDDEN + ) diff --git a/releasenotes/notes/bug-1805406-252b45d443af20b3.yaml b/releasenotes/notes/bug-1805406-252b45d443af20b3.yaml index 61131dd2a5..4311c4efb8 100644 --- a/releasenotes/notes/bug-1805406-252b45d443af20b3.yaml +++ b/releasenotes/notes/bug-1805406-252b45d443af20b3.yaml @@ -27,12 +27,13 @@ deprecations: and domain_id:%(target.domain_id)s)`` instead of ``rule:admin_required``. The ``identity:create_user``, ``identity:update_user``, and ``identity:delete_user`` policies now use - ``role:admin and system_scope:all`` instead of ``rule:admin_required``. - These new defaults automatically account - for system-scope, domain-scope, and support a read-only role, making it easier - for system and domain administrators to delegate subsets of responsibility - without compromising security. Please consider these new defaults - if your deployment overrides the user policies. + ``(role:admin and system_scope:all) or (role:admin and + token.domain.id:%(target.user.domain_id)s)`` instead of ``rule:admin_required``. + These new defaults automatically account for system-scope, domain-scope, + and support a read-only role, making it easier for system and domain + administrators to delegate subsets of responsibility without compromising + security. Please consider these new defaults if your deployment overrides + the user policies. security: - | [`bug 1805406 `_]