From 8c190a1a29dbb17fee044827fc9b651918dfc51e Mon Sep 17 00:00:00 2001 From: Ronald De Rose Date: Fri, 18 Nov 2016 16:41:08 +0000 Subject: [PATCH] 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 --- .../012_contract_add_domain_id_to_idp.py | 38 ++++++++ .../012_migrate_add_domain_id_to_idp.py | 55 +++++++++++ .../012_expand_add_domain_id_to_idp.py | 73 ++++++++++++++ keystone/federation/backends/sql.py | 3 +- keystone/federation/controllers.py | 2 +- keystone/federation/core.py | 26 +++++ keystone/federation/schema.py | 19 +++- .../tests/unit/test_backend_federation_sql.py | 1 + keystone/tests/unit/test_sql_upgrade.py | 67 +++++++++++++ keystone/tests/unit/test_v3_federation.py | 96 ++++++++++++++++++- 10 files changed, 372 insertions(+), 8 deletions(-) create mode 100644 keystone/common/sql/contract_repo/versions/012_contract_add_domain_id_to_idp.py create mode 100644 keystone/common/sql/data_migration_repo/versions/012_migrate_add_domain_id_to_idp.py create mode 100644 keystone/common/sql/expand_repo/versions/012_expand_add_domain_id_to_idp.py diff --git a/keystone/common/sql/contract_repo/versions/012_contract_add_domain_id_to_idp.py b/keystone/common/sql/contract_repo/versions/012_contract_add_domain_id_to_idp.py new file mode 100644 index 0000000000..b919f93dcc --- /dev/null +++ b/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) diff --git a/keystone/common/sql/data_migration_repo/versions/012_migrate_add_domain_id_to_idp.py b/keystone/common/sql/data_migration_repo/versions/012_migrate_add_domain_id_to_idp.py new file mode 100644 index 0000000000..d8b931aed8 --- /dev/null +++ b/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 diff --git a/keystone/common/sql/expand_repo/versions/012_expand_add_domain_id_to_idp.py b/keystone/common/sql/expand_repo/versions/012_expand_add_domain_id_to_idp.py new file mode 100644 index 0000000000..ef4522f598 --- /dev/null +++ b/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) diff --git a/keystone/federation/backends/sql.py b/keystone/federation/backends/sql.py index c994574576..85990e21ff 100644 --- a/keystone/federation/backends/sql.py +++ b/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', diff --git a/keystone/federation/controllers.py b/keystone/federation/controllers.py index b6b1faa0f2..6748ba1a1f 100644 --- a/keystone/federation/controllers.py +++ b/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 diff --git a/keystone/federation/core.py b/keystone/federation/core.py index eb2ea997a3..ea39f807ff 100644 --- a/keystone/federation/core.py +++ b/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. diff --git a/keystone/federation/schema.py b/keystone/federation/schema.py index 0159d39780..ca2951592f 100644 --- a/keystone/federation/schema.py +++ b/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 diff --git a/keystone/tests/unit/test_backend_federation_sql.py b/keystone/tests/unit/test_backend_federation_sql.py index 995c564db3..ca8043b49e 100644 --- a/keystone/tests/unit/test_backend_federation_sql.py +++ b/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) diff --git a/keystone/tests/unit/test_sql_upgrade.py b/keystone/tests/unit/test_sql_upgrade.py index 47d013eedd..7c70d5c2a8 100644 --- a/keystone/tests/unit/test_sql_upgrade.py +++ b/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 diff --git a/keystone/tests/unit/test_v3_federation.py b/keystone/tests/unit/test_v3_federation.py index b20cf1d65e..1ea2042143 100644 --- a/keystone/tests/unit/test_v3_federation.py +++ b/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')