diff --git a/README.rst b/README.rst index 28914db..4e953c3 100644 --- a/README.rst +++ b/README.rst @@ -175,6 +175,10 @@ usernames won't be reused later. This adapter also handles getting basic information about the user from SPI's LDAP. +**Important:** If you are building on top of *allauth*, take care to preserve +the ``extra_data['ldap']`` of ``SocialAccount`` instances related to *Clipper* +(where ``provider_id`` is ``clipper`` or ``clipper_inactive``). + Configuration Set ``SOCIALACCOUNT_ADAPTER='allauth_ens.adapter.LongTermClipperAccountAdapter'`` in `settings.py` @@ -199,7 +203,6 @@ Customize You can customize the SocialAccountAdapter by inheriting ``allauth_ens.adapter.LongTermClipperAccountAdapter``. You might want to modify ``get_username(clipper, data)`` to change the default username format. - This function is used to disambiguate in the account deprecation process. By default, ``get_username`` raises a ``ValueError`` when the connexion to the LDAP failed or did not allow to retrieve the user's entrance year. Overriding ``get_username`` (as done in the example website) allows to get rid of that diff --git a/allauth_ens/adapter.py b/allauth_ens/adapter.py index e85d978..b9c5d48 100644 --- a/allauth_ens/adapter.py +++ b/allauth_ens/adapter.py @@ -25,8 +25,8 @@ class LongTermClipperAccountAdapter(DefaultSocialAccountAdapter): If a clipper connection has already existed with the uid, it checks that this connection still belongs to the user it was associated with. - This check is performed by comparing the generated username corresponding - to this connection with the old one. + This check is performed by comparing the entrance years provided by the + LDAP. If the check succeeds, it simply reactivates the clipper connection as belonging to the associated user. @@ -52,14 +52,19 @@ class LongTermClipperAccountAdapter(DefaultSocialAccountAdapter): ldap_data = get_ldap_infos(clipper_uid) sociallogin._ldap_data = ldap_data - if old_conn.user.username != self.get_username(clipper_uid, ldap_data): - # The admission year is different + if ldap_data is None or 'entrance_year' not in ldap_data: + raise ValueError("No entrance year in LDAP data") + + old_conn_entrance_year = ( + old_conn.extra_data.get('ldap', {}).get('entrance_year')) + + if old_conn_entrance_year != ldap_data['entrance_year']: # We cannot reuse this SocialAccount, so we need to invalidate # the email address of the previous user to prevent conflicts # if a new SocialAccount is created email = ldap_data.get('email', get_clipper_email(clipper_uid)) remove_email(old_conn.user, email) - + return # The admission year is the same, we can update the model and keep @@ -72,8 +77,7 @@ class LongTermClipperAccountAdapter(DefaultSocialAccountAdapter): def get_username(self, clipper_uid, data): """ - Util function to generate a unique username, by default 'clipper@promo' - This is used to disambiguate and recognize if the person is the same + Util function to generate a unique username, by default 'clipper@promo'. """ if data is None or 'entrance_year' not in data: raise ValueError("No entrance year in LDAP data") @@ -114,7 +118,7 @@ class LongTermClipperAccountAdapter(DefaultSocialAccountAdapter): get_account_adapter().populate_username(request, user) # Save extra data (only once) - sociallogin.account.extra_data = sociallogin.extra_data = ldap_data + sociallogin.account.extra_data['ldap'] = ldap_data sociallogin.save(request) sociallogin.account.save() @@ -175,15 +179,22 @@ def install_longterm_adapter(fake=False): else: user.save() cases.append(user.username) - if SocialAccount.objects.filter(provider='clipper', - uid=clipper_uid).exists(): - logs["updated"].append((clipper_uid, user.username)) - continue - sa = SocialAccount(user=user, provider='clipper', - uid=clipper_uid, extra_data=data) - if not fake: - sa.save() - logs["created"].append((clipper_uid, user.username)) + + try: + sa = SocialAccount.objects.get(provider='clipper', uid=clipper_uid) + if not sa.extra_data.get('ldap'): + sa.extra_data['ldap'] = data + if not fake: + sa.save(update_fields=['extra_data']) + logs["updated"].append((clipper_uid, user.username)) + except SocialAccount.DoesNotExist: + sa = SocialAccount( + provider='clipper', uid=clipper_uid, + user=user, extra_data={'ldap': data}, + ) + if not fake: + sa.save() + logs["created"].append((clipper_uid, user.username)) logs["unmodified"] = User.objects.exclude(username__in=cases)\ .values_list("username", flat=True) diff --git a/allauth_ens/providers/clipper/provider.py b/allauth_ens/providers/clipper/provider.py index 41bb56b..4ee6169 100644 --- a/allauth_ens/providers/clipper/provider.py +++ b/allauth_ens/providers/clipper/provider.py @@ -37,8 +37,23 @@ class ClipperProvider(CASProvider): ] def extract_extra_data(self, data): + """ + If LongTermClipperAccountAdapter is in use, keep the data retrieved + from the LDAP server. + """ + from allauth.socialaccount.models import SocialAccount # noqa extra_data = super(ClipperProvider, self).extract_extra_data(data) extra_data['email'] = self.extract_email(data) + + # Preserve LDAP data at all cost. + try: + clipper_account = SocialAccount.objects.get( + provider=self.id, uid=self.extract_uid(data)) + if 'ldap' in clipper_account.extra_data: + extra_data['ldap'] = clipper_account.extra_data['ldap'] + except SocialAccount.DoesNotExist: + pass + return extra_data def message_suggest_caslogout_on_logout(self, request): diff --git a/allauth_ens/providers/clipper/tests.py b/allauth_ens/providers/clipper/tests.py index 657982c..00bb3b1 100644 --- a/allauth_ens/providers/clipper/tests.py +++ b/allauth_ens/providers/clipper/tests.py @@ -17,6 +17,18 @@ class ClipperProviderTests(CASTestCase): u = User.objects.get(username='clipper_uid') self.assertEqual(u.email, 'clipper_uid@clipper.ens.fr') + def test_extra_data_keeps_ldap_data(self): + clipper_conn = self.u.socialaccount_set.create( + uid='user', provider='clipper', + extra_data={'ldap': {'aa': 'bb'}}, + ) + + self.client_cas_login( + self.client, provider_id='clipper', username='user') + + clipper_conn.refresh_from_db() + self.assertEqual(clipper_conn.extra_data['ldap'], {'aa': 'bb'}) + class ClipperViewsTests(CASViewTestCase): diff --git a/allauth_ens/tests.py b/allauth_ens/tests.py index d9c01e5..471f479 100644 --- a/allauth_ens/tests.py +++ b/allauth_ens/tests.py @@ -1,3 +1,5 @@ +from __future__ import unicode_literals + import re import django @@ -8,6 +10,7 @@ from django.test import TestCase, override_settings from allauth.socialaccount.models import SocialAccount +import six from allauth_cas.test.testcases import CASTestCase from fakeldap import MockLDAP from mock import patch @@ -161,8 +164,12 @@ class LongTermClipperTests(CASTestCase): _mock_ldap.reset() def _setup_ldap(self, promo=12, username="test"): + try: + buid = six.binary_type(username, 'utf-8') + except TypeError: + buid = six.binary_type(username) _mock_ldap.directory['dc=spi,dc=ens,dc=fr'] = { - 'uid': [username], + 'uid': [buid], 'cn': [b'John Smith'], 'mailRoutingAddress': [b'test@clipper.ens.fr'], 'homeDirectory': [b'/users/%d/phy/test/' % promo], @@ -188,6 +195,7 @@ class LongTermClipperTests(CASTestCase): sa = list(SocialAccount.objects.all())[-1] self.assertEqual(sa.user.id, u.id) + self.assertEqual(sa.extra_data['ldap']['entrance_year'], '12') def test_connect_disconnect(self): self._setup_ldap() @@ -312,9 +320,11 @@ class LongTermClipperTests(CASTestCase): username='test') user1 = r.context["user"] nsa1 = SocialAccount.objects.count() + conn = user1.socialaccount_set.get(provider='clipper') self.assertEqual(user1.id, user0.id) self.assertEqual(nsa1, nsa0) self.assertEqual(user1.username, "test@12") + self.assertEqual(conn.extra_data['ldap']['entrance_year'], '12') def test_longterm_installer_from_djangocas(self): with self.settings(SOCIALACCOUNT_ADAPTER= @@ -332,9 +342,11 @@ class LongTermClipperTests(CASTestCase): username='test') user1 = r.context["user"] nsa1 = SocialAccount.objects.count() + conn = user1.socialaccount_set.get(provider='clipper') self.assertEqual(user1.id, user0.id) self.assertEqual(nsa1, nsa0 + 1) self.assertEqual(user1.username, "test@12") + self.assertEqual(conn.extra_data['ldap']['entrance_year'], '12') def test_disconnect_ldap(self): nu0 = User.objects.count() diff --git a/setup.cfg b/setup.cfg index 179a83e..bfb5cf5 100644 --- a/setup.cfg +++ b/setup.cfg @@ -7,7 +7,6 @@ combine_as_imports = True default_section = THIRDPARTY include_trailing_comma = True known_allauth = allauth -known_future_library = future,six known_django = django known_first_party = allauth_ens multi_line_output = 5