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

@@ -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:
+        try:
-            idp_ref = self._get_idp(session, idp_id)
+            with sql.session_for_write() as session:
-            old_idp = idp_ref.to_dict()
+                idp_ref = self._get_idp(session, idp_id)
-            old_idp.update(idp)
+                old_idp = idp_ref.to_dict()
-            new_idp = IdentityProviderModel.from_dict(old_idp)
+                old_idp.update(idp)
-            for attr in IdentityProviderModel.mutable_attributes:
+                new_idp = IdentityProviderModel.from_dict(old_idp)
-                setattr(idp_ref, attr, getattr(new_idp, attr))
+                for attr in IdentityProviderModel.mutable_attributes:
-            return idp_ref.to_dict()
+                    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):

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'
+            self._handle_idp_conflict(e)
-            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)
 
     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:
+        try:
-            idp_ref = self._get_idp(session, idp_id)
+            with sql.session_for_write() as session:
-            old_idp = idp_ref.to_dict()
+                idp_ref = self._get_idp(session, idp_id)
-            old_idp.update(idp)
+                old_idp = idp_ref.to_dict()
-            new_idp = IdentityProviderModel.from_dict(old_idp)
+                old_idp.update(idp)
-            for attr in IdentityProviderModel.mutable_attributes:
+                new_idp = IdentityProviderModel.from_dict(old_idp)
-                setattr(idp_ref, attr, getattr(new_idp, attr))
+                for attr in IdentityProviderModel.mutable_attributes:
-            return idp_ref.to_dict()
+                    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):

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.