From 800c3788aa89666abce6e582099755c2c794d3d1 Mon Sep 17 00:00:00 2001
From: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date: Fri, 12 Feb 2016 17:19:34 +0100
Subject: [PATCH] fix concurrency error when creating new users (fixes #9965)

UserSAMLIdentifier is retrieved using get_or_create() first, and if is new
we proceed with the creation of the new user, otherwise we delete the temporaru
user we created use the one attached to the existing UserSAMLIdentifier.
---
 mellon/adapters.py            | 23 +++++++++++++----------
 tests/conftest.py             | 12 ++++++++++++
 tests/test_default_adapter.py | 21 +++++++++++++++++++++
 testsettings.py               |  3 +++
 4 files changed, 49 insertions(+), 10 deletions(-)
 create mode 100644 tests/conftest.py

diff --git a/mellon/adapters.py b/mellon/adapters.py
index 5f59dc2..475f7e6 100644
--- a/mellon/adapters.py
+++ b/mellon/adapters.py
@@ -1,4 +1,5 @@
 import logging
+import uuid
 
 from django.core.exceptions import PermissionDenied
 from django.contrib import auth
@@ -53,16 +54,24 @@ class DefaultAdapter(object):
         issuer = saml_attributes['issuer']
         try:
             return User.objects.get(saml_identifiers__name_id=name_id,
-                    saml_identifiers__issuer=issuer)
+                                    saml_identifiers__issuer=issuer)
         except User.DoesNotExist:
             if not utils.get_setting(idp, 'PROVISION'):
+                self.logger.warning('provisionning disabled, login refused')
                 return None
             username = self.format_username(idp, saml_attributes)
             if not username:
+                self.logger.warning('could not build a username, login refused')
                 return None
-            user = User(username=username)
-            user.save()
-            self.provision_name_id(user, idp, saml_attributes)
+            user = User.objects.create(username=uuid.uuid4().hex[:30])
+            saml_id, created = models.UserSAMLIdentifier.objects.get_or_create(
+                name_id=name_id, issuer=issuer, defaults={'user': user})
+            if created:
+                user.username = username
+                user.save()
+            else:
+                user.delete()
+                user = saml_id.user
         return user
 
     def provision(self, user, idp, saml_attributes):
@@ -70,12 +79,6 @@ class DefaultAdapter(object):
         self.provision_superuser(user, idp, saml_attributes)
         self.provision_groups(user, idp, saml_attributes)
 
-    def provision_name_id(self, user, idp, saml_attributes):
-        models.UserSAMLIdentifier.objects.get_or_create(
-                user=user,
-                issuer=saml_attributes['issuer'],
-                name_id=saml_attributes['name_id_content'])
-
     def provision_attribute(self, user, idp, saml_attributes):
         realm = utils.get_setting(idp, 'REALM')
         attribute_mapping = utils.get_setting(idp, 'ATTRIBUTE_MAPPING')
diff --git a/tests/conftest.py b/tests/conftest.py
new file mode 100644
index 0000000..ed76201
--- /dev/null
+++ b/tests/conftest.py
@@ -0,0 +1,12 @@
+import pytest
+
+
+@pytest.fixture
+def concurrency(settings):
+    '''Select a level of concurrency based on the db, sqlite3 is less robust
+       thant postgres due to its transaction lock timeout of 5 seconds.
+    '''
+    if 'sqlite' in settings.DATABASES['default']['ENGINE']:
+        return 20
+    else:
+        return 100
diff --git a/tests/test_default_adapter.py b/tests/test_default_adapter.py
index d84f8f0..906fbf1 100644
--- a/tests/test_default_adapter.py
+++ b/tests/test_default_adapter.py
@@ -1,3 +1,4 @@
+import threading
 import pytest
 
 from django.conf import settings
@@ -45,6 +46,26 @@ def test_lookup_user(settings):
     assert user is None
     assert User.objects.count() == 0
 
+
+def test_lookup_user_transaction(transactional_db, concurrency):
+    adapter = DefaultAdapter()
+    N = 30
+    def map_threads(f, l):
+        threads = []
+        for i in l:
+            threads.append(threading.Thread(target=f, args=(i,)))
+            threads[-1].start()
+        for thread in threads:
+            thread.join()
+    users = []
+
+    def f(i):
+        users.append(adapter.lookup_user(idp, saml_attributes))
+    map_threads(f, range(concurrency))
+    assert len(users) == concurrency
+    assert len(set(user.pk for user in users)) == 1
+
+
 def test_provision(settings):
     settings.MELLON_GROUP_ATTRIBUTE = 'group'
     User = auth.get_user_model()
diff --git a/testsettings.py b/testsettings.py
index 752a9b1..5f7bb5c 100644
--- a/testsettings.py
+++ b/testsettings.py
@@ -4,6 +4,9 @@ DATABASES = {
     'default': {
         'ENGINE': 'django.db.backends.sqlite3',
         'NAME': 'mellon.sqlite3',
+        'TEST': {
+            'NAME': 'mellon-test.sqlite',
+        },
     }
 }
 DEBUG = True
-- 
2.1.4