From bfcbb3cd7679dd13d5ededd2f3b765d40e0bca7d Mon Sep 17 00:00:00 2001 From: Rodrigo Duarte Date: Thu, 17 Mar 2016 14:28:55 -0300 Subject: [PATCH] Add conflict validation for idp update Remote IDs conflicts can happen during an identity provider update (similar to what happens during create). This patch adds the same conflict handling, so a 500 is not returned by keystone. Change-Id: I1f093dad0b9427027edf4dc1a9f563e99aedad0c Closes-Bug: 1558670 --- keystone/federation/V8_backends/sql.py | 39 ++++++++++++++++----- keystone/federation/backends/sql.py | 42 +++++++++++++---------- keystone/tests/unit/test_v3_federation.py | 31 +++++++++++++++++ 3 files changed, 86 insertions(+), 26 deletions(-) diff --git a/keystone/federation/V8_backends/sql.py b/keystone/federation/V8_backends/sql.py index c95e9dbd2b..d6b42aa0c7 100644 --- a/keystone/federation/V8_backends/sql.py +++ b/keystone/federation/V8_backends/sql.py @@ -12,12 +12,18 @@ # License for the specific language governing permissions and limitations # under the License. +from oslo_log import log from oslo_serialization import jsonutils +import six from sqlalchemy import orm from keystone.common import sql from keystone import exception from keystone.federation import core +from keystone.i18n import _ + + +LOG = log.getLogger(__name__) class FederationProtocolModel(sql.ModelBase, sql.DictBase): @@ -157,6 +163,20 @@ class ServiceProviderModel(sql.ModelBase, sql.DictBase): class Federation(core.FederationDriverV8): + _CONFLICT_LOG_MSG = 'Conflict %(conflict_type)s: %(details)s' + + def _handle_idp_conflict(self, e): + conflict_type = 'identity_provider' + details = six.text_type(e) + LOG.debug(self._CONFLICT_LOG_MSG, {'conflict_type': conflict_type, + 'details': details}) + if 'remote_id' in details: + msg = _('Duplicate remote ID: %s') + else: + msg = _('Duplicate entry: %s') + msg = msg % e.value + raise exception.Conflict(type=conflict_type, details=msg) + # Identity Provider CRUD @sql.handle_conflicts(conflict_type='identity_provider') def create_idp(self, idp_id, idp): @@ -203,14 +223,17 @@ class Federation(core.FederationDriverV8): return ref.to_dict() def update_idp(self, idp_id, idp): - with sql.session_for_write() as session: - idp_ref = self._get_idp(session, idp_id) - old_idp = idp_ref.to_dict() - old_idp.update(idp) - new_idp = IdentityProviderModel.from_dict(old_idp) - for attr in IdentityProviderModel.mutable_attributes: - setattr(idp_ref, attr, getattr(new_idp, attr)) - return idp_ref.to_dict() + try: + with sql.session_for_write() as session: + idp_ref = self._get_idp(session, idp_id) + old_idp = idp_ref.to_dict() + old_idp.update(idp) + new_idp = IdentityProviderModel.from_dict(old_idp) + for attr in IdentityProviderModel.mutable_attributes: + setattr(idp_ref, attr, getattr(new_idp, attr)) + return idp_ref.to_dict() + except sql.DBDuplicateEntry as e: + self._handle_idp_conflict(e) # Protocol CRUD def _get_protocol(self, session, idp_id, protocol_id): diff --git a/keystone/federation/backends/sql.py b/keystone/federation/backends/sql.py index 78a7831ba1..add409e60c 100644 --- a/keystone/federation/backends/sql.py +++ b/keystone/federation/backends/sql.py @@ -165,6 +165,18 @@ class Federation(core.FederationDriverV9): _CONFLICT_LOG_MSG = 'Conflict %(conflict_type)s: %(details)s' + def _handle_idp_conflict(self, e): + conflict_type = 'identity_provider' + details = six.text_type(e) + LOG.debug(self._CONFLICT_LOG_MSG, {'conflict_type': conflict_type, + 'details': details}) + if 'remote_id' in details: + msg = _('Duplicate remote ID: %s') + else: + msg = _('Duplicate entry: %s') + msg = msg % e.value + raise exception.Conflict(type=conflict_type, details=msg) + # Identity Provider CRUD def create_idp(self, idp_id, idp): idp['id'] = idp_id @@ -174,16 +186,7 @@ class Federation(core.FederationDriverV9): session.add(idp_ref) return idp_ref.to_dict() except sql.DBDuplicateEntry as e: - conflict_type = 'identity_provider' - details = six.text_type(e) - LOG.debug(self._CONFLICT_LOG_MSG, {'conflict_type': conflict_type, - 'details': details}) - if 'remote_id' in details: - msg = _('Duplicate remote ID: %s') - else: - msg = _('Duplicate entry: %s') - msg = msg % e.value - raise exception.Conflict(type=conflict_type, details=msg) + self._handle_idp_conflict(e) def delete_idp(self, idp_id): with sql.session_for_write() as session: @@ -223,14 +226,17 @@ class Federation(core.FederationDriverV9): return ref.to_dict() def update_idp(self, idp_id, idp): - with sql.session_for_write() as session: - idp_ref = self._get_idp(session, idp_id) - old_idp = idp_ref.to_dict() - old_idp.update(idp) - new_idp = IdentityProviderModel.from_dict(old_idp) - for attr in IdentityProviderModel.mutable_attributes: - setattr(idp_ref, attr, getattr(new_idp, attr)) - return idp_ref.to_dict() + try: + with sql.session_for_write() as session: + idp_ref = self._get_idp(session, idp_id) + old_idp = idp_ref.to_dict() + old_idp.update(idp) + new_idp = IdentityProviderModel.from_dict(old_idp) + for attr in IdentityProviderModel.mutable_attributes: + setattr(idp_ref, attr, getattr(new_idp, attr)) + return idp_ref.to_dict() + except sql.DBDuplicateEntry as e: + self._handle_idp_conflict(e) # Protocol CRUD def _get_protocol(self, session, idp_id, protocol_id): diff --git a/keystone/tests/unit/test_v3_federation.py b/keystone/tests/unit/test_v3_federation.py index 8f1a54e42a..56c95115d6 100644 --- a/keystone/tests/unit/test_v3_federation.py +++ b/keystone/tests/unit/test_v3_federation.py @@ -985,6 +985,37 @@ class FederatedIdentityProviderTests(test_v3.RestfulTestCase): self.assertEqual(sorted(body['remote_ids']), sorted(returned_idp.get('remote_ids'))) + def test_update_idp_remote_repeated(self): + """Update an IdentityProvider entity reusing a remote_id. + + A remote_id is the same for both so the second IdP is not + updated because of the uniqueness of the remote_ids. + + Expect HTTP 409 Conflict code for the latter call. + + """ + # Create first identity provider + body = self.default_body.copy() + repeated_remote_id = uuid.uuid4().hex + body['remote_ids'] = [uuid.uuid4().hex, + repeated_remote_id] + self._create_default_idp(body=body) + + # Create second identity provider (without remote_ids) + 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') + url = self.base_url(suffix=idp_id) + + body['remote_ids'] = [repeated_remote_id] + resp = self.patch(url, body={'identity_provider': body}, + expected_status=http_client.CONFLICT) + resp_data = jsonutils.loads(resp.body) + self.assertIn('Duplicate remote ID', + resp_data['error']['message']) + def test_list_idps(self, iterations=5): """List all available IdentityProviders.