Require domain_id when registering Identity Providers

An Identity Provider (IdP) should be related (1:1) to a domain so that
federated users properly belong to a domain and can be uniquely
identified by their domain + unique_id. This patch makes it so that a
domain_id is required when registering a new IdP. If not explicitly set
via the API, the IdP will be mapped to a newly created domain. The docs
and release notes will be added in a subsequent patch.

Partial-Bug: #1642687
Partially-Implements: bp support-federated-attr
Change-Id: Id18b8b2fe853b97631bc990df8188ed64a6e1275

keystone/common/sql/contract_repo/versions/012_contract_add_domain_id_to_idp.py

@@ -0,0 +1,38 @@
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+import sqlalchemy as sql
+
+from keystone.common.sql import upgrades
+
+
+def upgrade(migrate_engine):
+    meta = sql.MetaData()
+    meta.bind = migrate_engine
+
+    idp_table = sql.Table('identity_provider', meta, autoload=True)
+    idp_table.c.domain_id.alter(nullable=False, unique=True)
+
+    if upgrades.USE_TRIGGERS:
+        if migrate_engine.name == 'postgresql':
+            drop_idp_insert_trigger = (
+                'DROP TRIGGER idp_insert_read_only on identity_provider;'
+            )
+        elif migrate_engine.name == 'mysql':
+            drop_idp_insert_trigger = (
+                'DROP TRIGGER idp_insert_read_only;'
+            )
+        else:
+            drop_idp_insert_trigger = (
+                'DROP TRIGGER IF EXISTS idp_insert_read_only;'
+            )
+        migrate_engine.execute(drop_idp_insert_trigger)

keystone/common/sql/data_migration_repo/versions/012_migrate_add_domain_id_to_idp.py

@@ -0,0 +1,55 @@
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+import uuid
+
+import sqlalchemy as sql
+from sqlalchemy.orm import sessionmaker
+
+from keystone.resource.backends import base
+
+
+def upgrade(migrate_engine):
+    meta = sql.MetaData()
+    meta.bind = migrate_engine
+    maker = sessionmaker(bind=migrate_engine)
+    session = maker()
+
+    idp_table = sql.Table('identity_provider', meta, autoload=True)
+
+    for idp_row in idp_table.select().execute():
+        domain_id = _create_federated_domain(meta, session, idp_row['id'])
+        # update idp with the new federated domain_id
+        values = {'domain_id': domain_id}
+        stmt = idp_table.update().where(
+            idp_table.c.id == idp_row['id']).values(values)
+        stmt.execute()
+
+
+def _create_federated_domain(meta, session, idp_id):
+    domain_id = uuid.uuid4().hex
+    desc = 'Auto generated federated domain for Identity Provider: ' + idp_id
+    federated_domain = {
+        'id': domain_id,
+        'name': domain_id,
+        'enabled': True,
+        'description': desc,
+        'domain_id': base.NULL_DOMAIN_ID,
+        'is_domain': True,
+        'parent_id': None,
+        'extra': '{}'
+    }
+    project_table = sql.Table('project', meta, autoload=True)
+    new_row = project_table.insert().values(**federated_domain)
+    session.execute(new_row)
+    session.commit()
+    return domain_id

keystone/common/sql/expand_repo/versions/012_expand_add_domain_id_to_idp.py

@@ -0,0 +1,73 @@
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+import sqlalchemy as sql
+
+from keystone.common.sql import upgrades
+
+
+MYSQL_INSERT_TRIGGER = """
+CREATE TRIGGER idp_insert_read_only BEFORE INSERT ON identity_provider
+FOR EACH ROW
+BEGIN
+  SIGNAL SQLSTATE '45000'
+    SET MESSAGE_TEXT = '%s';
+END;
+"""
+
+SQLITE_INSERT_TRIGGER = """
+CREATE TRIGGER idp_insert_read_only BEFORE INSERT ON identity_provider
+BEGIN
+  SELECT RAISE (ABORT, '%s');
+END;
+"""
+
+POSTGRESQL_INSERT_TRIGGER = """
+CREATE OR REPLACE FUNCTION keystone_read_only_insert()
+  RETURNS trigger AS
+$BODY$
+BEGIN
+  RAISE EXCEPTION '%s';
+END
+$BODY$ LANGUAGE plpgsql;
+
+CREATE TRIGGER idp_insert_read_only BEFORE INSERT ON identity_provider
+FOR EACH ROW
+EXECUTE PROCEDURE keystone_read_only_insert();
+"""
+
+
+def upgrade(migrate_engine):
+    meta = sql.MetaData()
+    meta.bind = migrate_engine
+
+    idp = sql.Table('identity_provider', meta, autoload=True)
+    project = sql.Table('project', meta, autoload=True)
+    domain_id = sql.Column('domain_id', sql.String(64),
+                           sql.ForeignKey(project.c.id), nullable=True)
+    idp.create_column(domain_id)
+
+    if upgrades.USE_TRIGGERS:
+        # Setting idp to be read-only to prevent old code from creating an idp
+        # without a domain_id during an upgrade. This should be okay as it is
+        # highly unlikely that an idp would be created during the migration and
+        # the impact from preventing creations is minor.
+        error_message = ('Identity provider migration in progress. Cannot '
+                         'insert new rows into the identity_provider table at '
+                         'this time.')
+        if migrate_engine.name == 'postgresql':
+            idp_insert_trigger = POSTGRESQL_INSERT_TRIGGER % error_message
+        elif migrate_engine.name == 'sqlite':
+            idp_insert_trigger = SQLITE_INSERT_TRIGGER % error_message
+        else:
+            idp_insert_trigger = MYSQL_INSERT_TRIGGER % error_message
+        migrate_engine.execute(idp_insert_trigger)

keystone/federation/backends/sql.py

@@ -51,10 +51,11 @@ class FederationProtocolModel(sql.ModelBase, sql.DictBase):
 
 class IdentityProviderModel(sql.ModelBase, sql.DictBase):
     __tablename__ = 'identity_provider'
-    attributes = ['id', 'enabled', 'description', 'remote_ids']
+    attributes = ['id', 'domain_id', 'enabled', 'description', 'remote_ids']
     mutable_attributes = frozenset(['description', 'enabled', 'remote_ids'])
 
     id = sql.Column(sql.String(64), primary_key=True)
+    domain_id = sql.Column(sql.String(64), nullable=False, unique=True)
     enabled = sql.Column(sql.Boolean, nullable=False)
     description = sql.Column(sql.Text(), nullable=True)
     remote_ids = orm.relationship('IdPRemoteIdsModel',

keystone/federation/controllers.py

@@ -56,7 +56,7 @@ class IdentityProvider(_ControllerBase):
     member_name = 'identity_provider'
 
     _public_parameters = frozenset(['id', 'enabled', 'description',
-                                    'remote_ids', 'links'
+                                    'remote_ids', 'links', 'domain_id'
                                     ])
 
     @classmethod

keystone/federation/core.py

@@ -12,6 +12,8 @@
 
 """Main entry point into the Federation service."""
 
+import uuid
+
 from keystone.common import cache
 from keystone.common import dependency
 from keystone.common import extension
@@ -43,6 +45,7 @@ extension.register_public_extension(EXTENSION_DATA['alias'], EXTENSION_DATA)
 
 
 @dependency.provider('federation_api')
+@dependency.requires('resource_api')
 class Manager(manager.Manager):
     """Default pivot point for the Federation backend.
 
@@ -56,6 +59,29 @@ class Manager(manager.Manager):
     def __init__(self):
         super(Manager, self).__init__(CONF.federation.driver)
 
+    def create_idp(self, idp_id, idp):
+        if not idp.get('domain_id'):
+            idp['domain_id'] = self._create_idp_domain(idp_id)
+        else:
+            self._assert_valid_domain_id(idp['domain_id'])
+        return self.driver.create_idp(idp_id, idp)
+
+    def _create_idp_domain(self, idp_id):
+        domain_id = uuid.uuid4().hex
+        desc = 'Auto generated federated domain for Identity Provider: '
+        desc += idp_id
+        domain = {
+            'id': domain_id,
+            'name': domain_id,
+            'description': desc,
+            'enabled': True
+        }
+        self.resource_api.create_domain(domain['id'], domain)
+        return domain_id
+
+    def _assert_valid_domain_id(self, domain_id):
+        self.resource_api.get_domain(domain_id)
+
     @MEMOIZE
     def get_enabled_service_providers(self):
         """List enabled service providers for Service Catalog.

keystone/federation/schema.py

@@ -78,7 +78,20 @@ service_provider_update = {
     'additionalProperties': False
 }
 
-_identity_provider_properties = {
+_identity_provider_properties_create = {
+    'enabled': parameter_types.boolean,
+    'description': validation.nullable(parameter_types.description),
+    'domain_id': validation.nullable(parameter_types.id_string),
+    'remote_ids': {
+        'type': ['array', 'null'],
+        'items': {
+            'type': 'string'
+        },
+        'uniqueItems': True
+    }
+}
+
+_identity_provider_properties_update = {
     'enabled': parameter_types.boolean,
     'description': validation.nullable(parameter_types.description),
     'remote_ids': {
@@ -92,13 +105,13 @@ _identity_provider_properties = {
 
 identity_provider_create = {
     'type': 'object',
-    'properties': _identity_provider_properties,
+    'properties': _identity_provider_properties_create,
     'additionalProperties': False
 }
 
 identity_provider_update = {
     'type': 'object',
-    'properties': _identity_provider_properties,
+    'properties': _identity_provider_properties_update,
     # Make sure at least one property is being updated
     'minProperties': 1,
     'additionalProperties': False

keystone/tests/unit/test_backend_federation_sql.py

@@ -21,6 +21,7 @@ class SqlFederation(test_backend_sql.SqlModels):
 
     def test_identity_provider(self):
         cols = (('id', sql.String, 64),
+                ('domain_id', sql.String, 64),
                 ('enabled', sql.Boolean, None),
                 ('description', sql.Text, None))
         self.assertExpectedSchema('identity_provider', cols)

keystone/tests/unit/test_sql_upgrade.py

@@ -48,6 +48,7 @@ from keystone.common import sql
 from keystone.common.sql import upgrades
 import keystone.conf
 from keystone.credential.providers import fernet as credential_fernet
+from keystone.resource.backends import base as resource_base
 from keystone.tests import unit
 from keystone.tests.unit import default_fixtures
 from keystone.tests.unit import ksfixtures
@@ -1837,6 +1838,72 @@ class FullMigration(SqlMigrateBase, unit.TestCase):
         self.contract(11)
         self.assertTrue(self.does_unique_constraint_exist(table_name, column))
 
+    def test_migration_012_add_domain_id_to_idp(self):
+        def _create_domain():
+            domain_id = uuid.uuid4().hex
+            domain = {
+                'id': domain_id,
+                'name': domain_id,
+                'enabled': True,
+                'description': uuid.uuid4().hex,
+                'domain_id': resource_base.NULL_DOMAIN_ID,
+                'is_domain': True,
+                'parent_id': None,
+                'extra': '{}'
+            }
+            self.insert_dict(session, 'project', domain)
+            return domain_id
+
+        def _get_new_idp(domain_id):
+            new_idp = {'id': uuid.uuid4().hex,
+                       'domain_id': domain_id,
+                       'enabled': True,
+                       'description': uuid.uuid4().hex}
+            return new_idp
+
+        session = self.sessionmaker()
+        idp_name = 'identity_provider'
+        self.expand(11)
+        self.migrate(11)
+        self.contract(11)
+        self.assertTableColumns(idp_name,
+                                ['id',
+                                 'enabled',
+                                 'description'])
+        # add some data
+        for i in range(5):
+            idp = {'id': uuid.uuid4().hex,
+                   'enabled': True,
+                   'description': uuid.uuid4().hex}
+            self.insert_dict(session, idp_name, idp)
+
+        # upgrade
+        self.expand(12)
+        self.assertTableColumns(idp_name,
+                                ['id',
+                                 'domain_id',
+                                 'enabled',
+                                 'description'])
+
+        # confirm we cannot insert an idp during expand
+        domain_id = _create_domain()
+        new_idp = _get_new_idp(domain_id)
+        self.assertRaises(db_exception.DBError, self.insert_dict, session,
+                          idp_name, new_idp)
+
+        # confirm we cannot insert an idp during migrate
+        self.migrate(12)
+        self.assertRaises(db_exception.DBError, self.insert_dict, session,
+                          idp_name, new_idp)
+
+        # confirm we can insert a new idp after contract
+        self.contract(12)
+        self.insert_dict(session, idp_name, new_idp)
+
+        # confirm domain_id column is not null
+        idp_table = sqlalchemy.Table(idp_name, self.metadata, autoload=True)
+        self.assertFalse(idp_table.c.domain_id.nullable)
+
 
 class MySQLOpportunisticFullMigration(FullMigration):
     FIXTURE = test_base.MySQLOpportunisticFixture

keystone/tests/unit/test_v3_federation.py

@@ -823,13 +823,14 @@ class FederatedIdentityProviderTests(test_v3.RestfulTestCase):
         resp = self.get(url)
         return resp
 
-    def _create_default_idp(self, body=None):
+    def _create_default_idp(self, body=None,
+                            expected_status=http_client.CREATED):
         """Create default IdP."""
         url = self.base_url(suffix=uuid.uuid4().hex)
         if body is None:
             body = self._http_idp_input()
         resp = self.put(url, body={'identity_provider': body},
-                        expected_status=http_client.CREATED)
+                        expected_status=expected_status)
         return resp
 
     def _http_idp_input(self, **kwargs):
@@ -877,7 +878,12 @@ class FederatedIdentityProviderTests(test_v3.RestfulTestCase):
                  body={'mapping': mapping},
                  expected_status=http_client.CREATED)
 
-    def test_create_idp(self):
+    def assertIdpDomainCreated(self, idp_id, domain_id):
+        domain = self.resource_api.get_domain(domain_id)
+        self.assertEqual(domain_id, domain['name'])
+        self.assertIn(idp_id, domain['description'])
+
+    def test_create_idp_without_domain_id(self):
         """Create the IdentityProvider entity associated to remote_ids."""
         keys_to_check = list(self.idp_keys)
         body = self.default_body.copy()
@@ -886,6 +892,81 @@ class FederatedIdentityProviderTests(test_v3.RestfulTestCase):
         self.assertValidResponse(resp, 'identity_provider', dummy_validator,
                                  keys_to_check=keys_to_check,
                                  ref=body)
+        attr = self._fetch_attribute_from_response(resp, 'identity_provider')
+        self.assertIdpDomainCreated(attr['id'], attr['domain_id'])
+
+    def test_create_idp_with_domain_id(self):
+        keys_to_check = list(self.idp_keys)
+        keys_to_check.append('domain_id')
+        body = self.default_body.copy()
+        body['description'] = uuid.uuid4().hex
+        domain = unit.new_domain_ref()
+        self.resource_api.create_domain(domain['id'], domain)
+        body['domain_id'] = domain['id']
+        resp = self._create_default_idp(body=body)
+        self.assertValidResponse(resp, 'identity_provider', dummy_validator,
+                                 keys_to_check=keys_to_check,
+                                 ref=body)
+
+    def test_create_idp_domain_id_none(self):
+        keys_to_check = list(self.idp_keys)
+        body = self.default_body.copy()
+        body['description'] = uuid.uuid4().hex
+        body['domain_id'] = None
+        resp = self._create_default_idp(body=body)
+        self.assertValidResponse(resp, 'identity_provider', dummy_validator,
+                                 keys_to_check=keys_to_check,
+                                 ref=body)
+        attr = self._fetch_attribute_from_response(resp, 'identity_provider')
+        self.assertIdpDomainCreated(attr['id'], attr['domain_id'])
+
+    def test_create_idp_domain_id_unique_constraint(self):
+        # create domain and add domain_id to keys to check
+        domain = unit.new_domain_ref()
+        self.resource_api.create_domain(domain['id'], domain)
+        keys_to_check = list(self.idp_keys)
+        keys_to_check.append('domain_id')
+        # create idp with the domain_id
+        body = self.default_body.copy()
+        body['description'] = uuid.uuid4().hex
+        body['domain_id'] = domain['id']
+        resp = self._create_default_idp(body=body)
+        self.assertValidResponse(resp, 'identity_provider', dummy_validator,
+                                 keys_to_check=keys_to_check,
+                                 ref=body)
+        # create a 2nd idp with the same domain_id
+        url = self.base_url(suffix=uuid.uuid4().hex)
+        body = self.default_body.copy()
+        body['description'] = uuid.uuid4().hex
+        body['domain_id'] = domain['id']
+        resp = self.put(url, body={'identity_provider': body},
+                        expected_status=http_client.CONFLICT)
+        resp_data = jsonutils.loads(resp.body)
+        self.assertIn('Duplicate entry',
+                      resp_data.get('error', {}).get('message'))
+
+    def test_cannot_update_idp_domain(self):
+        # create new idp
+        body = self.default_body.copy()
+        default_resp = self._create_default_idp(body=body)
+        default_idp = self._fetch_attribute_from_response(default_resp,
+                                                          'identity_provider')
+        idp_id = default_idp.get('id')
+        self.assertIsNotNone(idp_id)
+        # create domain and try to update the idp's domain
+        domain = unit.new_domain_ref()
+        self.resource_api.create_domain(domain['id'], domain)
+        body['domain_id'] = domain['id']
+        body = {'identity_provider': body}
+        url = self.base_url(suffix=idp_id)
+        self.patch(url, body=body, expected_status=http_client.BAD_REQUEST)
+
+    def test_create_idp_with_nonexistent_domain_id_fails(self):
+        body = self.default_body.copy()
+        body['description'] = uuid.uuid4().hex
+        body['domain_id'] = uuid.uuid4().hex
+        self._create_default_idp(body=body,
+                                 expected_status=http_client.NOT_FOUND)
 
     def test_create_idp_remote(self):
         """Create the IdentityProvider entity associated to remote_ids."""
@@ -900,6 +981,8 @@ class FederatedIdentityProviderTests(test_v3.RestfulTestCase):
         self.assertValidResponse(resp, 'identity_provider', dummy_validator,
                                  keys_to_check=keys_to_check,
                                  ref=body)
+        attr = self._fetch_attribute_from_response(resp, 'identity_provider')
+        self.assertIdpDomainCreated(attr['id'], attr['domain_id'])
 
     def test_create_idp_remote_repeated(self):
         """Create two IdentityProvider entities with some remote_ids.
@@ -1060,6 +1143,7 @@ class FederatedIdentityProviderTests(test_v3.RestfulTestCase):
         ids = set(ids)
 
         keys_to_check = self.idp_keys
+        keys_to_check.append('domain_id')
         url = self.base_url()
         resp = self.get(url)
         self.assertValidListResponse(resp, 'identity_providers',
@@ -1130,6 +1214,9 @@ class FederatedIdentityProviderTests(test_v3.RestfulTestCase):
         """
         url = self.base_url(suffix=uuid.uuid4().hex)
         body = self._http_idp_input()
+        domain = unit.new_domain_ref()
+        self.resource_api.create_domain(domain['id'], domain)
+        body['domain_id'] = domain['id']
         self.put(url, body={'identity_provider': body},
                  expected_status=http_client.CREATED)
         resp = self.put(url, body={'identity_provider': body},
@@ -1142,6 +1229,9 @@ class FederatedIdentityProviderTests(test_v3.RestfulTestCase):
     def test_get_idp(self):
         """Create and later fetch IdP."""
         body = self._http_idp_input()
+        domain = unit.new_domain_ref()
+        self.resource_api.create_domain(domain['id'], domain)
+        body['domain_id'] = domain['id']
         default_resp = self._create_default_idp(body=body)
         default_idp = self._fetch_attribute_from_response(default_resp,
                                                           'identity_provider')