Merge "Implement domain admin functionality for user API"

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)

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,
+        check_str=SYSTEM_ADMIN_OR_DOMAIN_ADMIN,
-        # FIXME(lbragstad): This can be considered either a system-level policy
+        scope_types=['system', 'domain'],
-        # 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'],
         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,
+        check_str=SYSTEM_ADMIN_OR_DOMAIN_ADMIN,
-        # FIXME(lbragstad): See the above comment about adding support for
+        scope_types=['system', 'domain'],
-        # project scope_types in the future.
-        scope_types=['system'],
         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,
+        check_str=SYSTEM_ADMIN_OR_DOMAIN_ADMIN,
-        # FIXME(lbragstad): See the above comment about adding support for
+        scope_types=['system', 'domain'],
-        # project scope_types in the future.
-        scope_types=['system'],
         description='Delete a user.',
         operations=[{'path': '/v3/users/{user_id}',
                      'method': 'DELETE'}],

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):
+class _DomainUserTests(object):
-    """Functionality for all domain members and domain readers."""
+    """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
+            )

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``.
+    ``(role:admin and system_scope:all) or (role:admin and
-    These new defaults automatically account
+    token.domain.id:%(target.user.domain_id)s)`` instead of ``rule:admin_required``.
-    for system-scope, domain-scope, and support a read-only role, making it easier
+    These new defaults automatically account for system-scope, domain-scope,
-    for system and domain administrators to delegate subsets of responsibility
+    and support a read-only role, making it easier for system and domain
-    without compromising security. Please consider these new defaults
+    administrators to delegate subsets of responsibility without compromising
-    if your deployment overrides the user policies.
+    security. Please consider these new defaults if your deployment overrides
+    the user policies.
 security:
   - |
     [`bug 1805406 <https://bugs.launchpad.net/keystone/+bug/1805406>`_]