From 6e60948c20e9a8fe5469bb7735641026a1f36cd5 Mon Sep 17 00:00:00 2001
From: yangweiwei <yangweiwei@cmss.chinamobile.com>
Date: Thu, 29 Jun 2017 18:49:33 +0800
Subject: [PATCH] Handle auto-generated domains when creating IdPs

When creating an IdP, if a domain was generated for it and a conflict
was raised while effectively creating the IdP in the database, the
auto-generated domain is now cleaned up.

Change-Id: I9b7c3c1fae32b9412f75323a75d9ebe4ad756729
Closes-Bug: #1688188
---
 keystone/federation/core.py                   | 18 +++++++++-
 keystone/tests/unit/test_v3_federation.py     | 36 ++++++++++++++++---
 .../notes/bug_1688188-256e3572295231a1.yaml   |  7 ++++
 3 files changed, 56 insertions(+), 5 deletions(-)
 create mode 100644 releasenotes/notes/bug_1688188-256e3572295231a1.yaml

diff --git a/keystone/federation/core.py b/keystone/federation/core.py
index 26a8c29a41..03a408d916 100644
--- a/keystone/federation/core.py
+++ b/keystone/federation/core.py
@@ -60,11 +60,27 @@ class Manager(manager.Manager):
         super(Manager, self).__init__(CONF.federation.driver)
 
     def create_idp(self, idp_id, idp):
+        auto_created_domain = False
         if not idp.get('domain_id'):
             idp['domain_id'] = self._create_idp_domain(idp_id)
+            auto_created_domain = True
         else:
             self._assert_valid_domain_id(idp['domain_id'])
-        return self.driver.create_idp(idp_id, idp)
+
+        try:
+            return self.driver.create_idp(idp_id, idp)
+        except exception.Conflict:
+            # If there is a conflict storing the Identity Provider in the
+            # backend, then we need to make sure we clean up the domain we just
+            # created for it and raise the Conflict exception afterwards.
+            if auto_created_domain:
+                self._cleanup_idp_domain(idp['domain_id'])
+            raise
+
+    def _cleanup_idp_domain(self, domain_id):
+        domain = {'enabled': False}
+        self.resource_api.update_domain(domain_id, domain)
+        self.resource_api.delete_domain(domain_id)
 
     def _create_idp_domain(self, idp_id):
         domain_id = uuid.uuid4().hex
diff --git a/keystone/tests/unit/test_v3_federation.py b/keystone/tests/unit/test_v3_federation.py
index 06ee8a701e..4cb4130f0b 100644
--- a/keystone/tests/unit/test_v3_federation.py
+++ b/keystone/tests/unit/test_v3_federation.py
@@ -912,8 +912,7 @@ class FederatedIdentityProviderTests(test_v3.RestfulTestCase):
         attr = self._fetch_attribute_from_response(resp, 'identity_provider')
         self.assertIdpDomainCreated(attr['id'], attr['domain_id'])
 
-    @utils.wip('This will fail because of bug #1688188')
-    def test_conflicting_idp_results_in_unhandled_domain_cleanup(self):
+    def test_conflicting_idp_cleans_up_auto_generated_domain(self):
         # NOTE(lbragstad): Create an identity provider, save its ID, and count
         # the number of domains.
         resp = self._create_default_idp()
@@ -925,8 +924,8 @@ class FederatedIdentityProviderTests(test_v3.RestfulTestCase):
         # conflict, this is going to result in a domain getting created for the
         # new identity provider. The domain for the new identity provider is
         # going to be created before the conflict is raised from the database
-        # layer. The resulting domain is never cleaned up but it should be
-        # since the identity provider was never created.
+        # layer. This makes sure the domain is cleaned up after a Conflict is
+        # detected.
         resp = self.put(
             self.base_url(suffix=idp_id),
             body={'identity_provider': self.default_body.copy()},
@@ -935,6 +934,35 @@ class FederatedIdentityProviderTests(test_v3.RestfulTestCase):
         domains = self.resource_api.list_domains()
         self.assertEqual(number_of_domains, len(domains))
 
+    def test_conflicting_idp_does_not_delete_existing_domain(self):
+        # Create a new domain
+        domain = unit.new_domain_ref()
+        self.resource_api.create_domain(domain['id'], domain)
+
+        # Create an identity provider and specify the domain
+        body = self.default_body.copy()
+        body['description'] = uuid.uuid4().hex
+        body['domain_id'] = domain['id']
+        resp = self._create_default_idp(body=body)
+        idp = resp.json_body['identity_provider']
+        idp_id = idp['id']
+        self.assertEqual(idp['domain_id'], domain['id'])
+
+        # Create an identity provider with the same domain and ID to ensure a
+        # Conflict is raised and then to verify the existing domain not deleted
+        # below
+        body = self.default_body.copy()
+        body['domain_id'] = domain['id']
+        resp = self.put(
+            self.base_url(suffix=idp_id),
+            body={'identity_provider': body},
+            expected_status=http_client.CONFLICT
+        )
+
+        # Make sure the domain specified in the second request was not deleted,
+        # since it wasn't auto-generated
+        self.assertIsNotNone(self.resource_api.get_domain(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()
diff --git a/releasenotes/notes/bug_1688188-256e3572295231a1.yaml b/releasenotes/notes/bug_1688188-256e3572295231a1.yaml
new file mode 100644
index 0000000000..1418f6eb19
--- /dev/null
+++ b/releasenotes/notes/bug_1688188-256e3572295231a1.yaml
@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    [`bug 1688188 <https://bugs.launchpad.net/keystone/+bug/1688188>`_]
+    When creating an IdP, if a domain was generated for it and a conflict
+    was raised while effectively creating the IdP in the database, the
+    auto-generated domain is now cleaned up.