From 394db5dcb0ce97bc1bc4d97178447e7afa54951a Mon Sep 17 00:00:00 2001 From: Benjamin Dauvergne Date: Thu, 13 Jun 2019 11:47:49 +0200 Subject: [PATCH] auth_fc: make user and sub relatively unique (#19959) As we are not sure they are unique in all deployments, we make them unique relative to a new order integer field. New federations with FranceConnect with be forced to have the column order to be 0, making them unique. --- src/authentic2_auth_fc/backends.py | 39 ++++++--- .../migrations/0002_fcaccount_order1.py | 59 +++++++++++++ .../migrations/0003_fcaccount_order2.py | 26 ++++++ src/authentic2_auth_fc/models.py | 9 ++ src/authentic2_auth_fc/views.py | 86 +++++++++---------- tests/auth_fc/test_auth_fc.py | 28 ++++++ tests/conftest.py | 55 ++++++++++++ 7 files changed, 247 insertions(+), 55 deletions(-) create mode 100644 src/authentic2_auth_fc/migrations/0002_fcaccount_order1.py create mode 100644 src/authentic2_auth_fc/migrations/0003_fcaccount_order2.py diff --git a/src/authentic2_auth_fc/backends.py b/src/authentic2_auth_fc/backends.py index bf142bea..f873555a 100644 --- a/src/authentic2_auth_fc/backends.py +++ b/src/authentic2_auth_fc/backends.py @@ -19,6 +19,8 @@ import logging from django.contrib.auth import get_user_model from django.contrib.auth.backends import ModelBackend +from django.core.exceptions import PermissionDenied, MultipleObjectsReturned +from django.db import IntegrityError from authentic2.a2_rbac.utils import get_default_ou from authentic2 import hooks @@ -33,24 +35,41 @@ class FcBackend(ModelBackend): user_info = kwargs.get('user_info') user = None try: - fc_account = models.FcAccount.objects.get(sub=sub, user__is_active=True) - logger.debug(u'existing user %s using sub %s', fc_account.user, sub) - user = fc_account.user + try: + account = models.FcAccount.objects.select_related().get(sub=sub) + except MultipleObjectsReturned: + account = models.FcAccount.objects.select_related().get(sub=sub, order=0) except models.FcAccount.DoesNotExist: logger.debug(u'user with the sub %s does not exist.', sub) + else: + user = account.user + logger.debug(u'found user %s with sub %s', user, sub) + if not user.is_active: + logger.info(u'user %s login refused, it is inactive', user) + raise PermissionDenied if user_info: if not user and app_settings.create: User = get_user_model() user = User.objects.create(ou=get_default_ou()) - fc_account = models.FcAccount.objects.create( - user=user, - sub=sub, - token=json.dumps(kwargs['token'])) - logger.debug(u'user creation enabled with fc_account (sub : %s - token : %s)', - sub, json.dumps(kwargs['token'])) - hooks.call_hooks('event', name='fc-create', user=user, sub=sub) + try: + models.FcAccount.objects.create( + user=user, + sub=sub, + order=0, + token=json.dumps(kwargs['token'])) + except IntegrityError: + # uniqueness check failed, as the user is new, it can only means that the sub is not unique + # let's try again + user.delete() + return self.authenticate(sub, **kwargs) + else: + logger.debug(u'user creation enabled with fc_account (sub : %s - token : %s)', + sub, json.dumps(kwargs['token'])) + hooks.call_hooks('event', name='fc-create', user=user, sub=sub) + if not user: return None + logger.debug(u'updated (given_name : %s - family_name : %s)', user_info['given_name'], user_info['family_name']) user.first_name = user_info['given_name'] diff --git a/src/authentic2_auth_fc/migrations/0002_fcaccount_order1.py b/src/authentic2_auth_fc/migrations/0002_fcaccount_order1.py new file mode 100644 index 00000000..35475737 --- /dev/null +++ b/src/authentic2_auth_fc/migrations/0002_fcaccount_order1.py @@ -0,0 +1,59 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.20 on 2019-06-12 21:27 +from __future__ import unicode_literals + +from collections import Counter + +from django.db import migrations, models + + +def set_fcaccount_order(apps, schema_editor): + FcAccount = apps.get_model('authentic2_auth_fc', 'FcAccount') + c = Counter() + FcAccount.objects.update(order=0) + user_ids = ( + FcAccount.objects + .values('user_id') + .annotate(total=models.Count('id')) + .filter(total__gt=1) + .values_list('user_id', flat=True) + ) + subs = ( + FcAccount.objects + .values_list('sub') + .annotate(total=models.Count('id')) + .filter(total__gt=1) + .values_list('sub', flat=True) + ) + + for account in FcAccount.objects.filter(models.Q(user_id__in=user_ids) | models.Q(sub__in=subs)): + order = max(c[account.user_id], c[account.sub]) + if account.order != order: + account.order = order + account.save() + c[account.user_id] = order + 1 + c[account.sub] = order + 1 + assert FcAccount.objects.filter(order__isnull=True).count() == 0 + + +def noop(apps, schema_editor): + pass + + +class Migration(migrations.Migration): + # Disembiguate sub and user_id columns using an integer order column, then + # force sub/order and user_id/order pairs unique to be unique in order + # to move to a model where user and sub are unique without breaking on existing data + + dependencies = [ + ('authentic2_auth_fc', '0001_initial'), + ] + + operations = [ + migrations.AddField( + model_name='fcaccount', + name='order', + field=models.IntegerField(null=True, verbose_name='order'), + ), + migrations.RunPython(set_fcaccount_order, noop), + ] diff --git a/src/authentic2_auth_fc/migrations/0003_fcaccount_order2.py b/src/authentic2_auth_fc/migrations/0003_fcaccount_order2.py new file mode 100644 index 00000000..a8bd1d3c --- /dev/null +++ b/src/authentic2_auth_fc/migrations/0003_fcaccount_order2.py @@ -0,0 +1,26 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.20 on 2019-06-13 09:44 +from __future__ import unicode_literals + +from django.conf import settings +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('authentic2_auth_fc', '0002_fcaccount_order1'), + ] + + operations = [ + migrations.AlterField( + model_name='fcaccount', + name='order', + field=models.PositiveIntegerField(default=0, verbose_name='order'), + ), + migrations.AlterUniqueTogether( + name='fcaccount', + unique_together=set([('sub', 'order'), ('user', 'order')]), + ), + ] diff --git a/src/authentic2_auth_fc/models.py b/src/authentic2_auth_fc/models.py index 698e7e0a..e34a8b07 100644 --- a/src/authentic2_auth_fc/models.py +++ b/src/authentic2_auth_fc/models.py @@ -87,6 +87,9 @@ class FcAccount(models.Model): sub = models.TextField( verbose_name=_('sub'), db_index=True) + order = models.PositiveIntegerField( + verbose_name=_('order'), + default=0) token = models.TextField(verbose_name=_('access token')) user_info = models.TextField(verbose_name=_('access token'), blank=True, null=True) @@ -108,3 +111,9 @@ class FcAccount(models.Model): if 'family_name' in user_info: display_name.append(user_info['family_name']) return ' '.join(display_name) + + class Meta: + unique_together = [ + ('sub', 'order'), + ('user', 'order'), + ] diff --git a/src/authentic2_auth_fc/views.py b/src/authentic2_auth_fc/views.py index 6e3b4f54..595bb211 100644 --- a/src/authentic2_auth_fc/views.py +++ b/src/authentic2_auth_fc/views.py @@ -24,6 +24,7 @@ from requests_oauthlib import OAuth2Session import django +from django.db import IntegrityError from django.views.generic import View, FormView from django.http import HttpResponseRedirect, Http404 from django.contrib.auth import REDIRECT_FIELD_NAME, get_user_model @@ -162,6 +163,7 @@ class FcOAuthSessionViewMixin(LoggerMixin): redirect_field_name = REDIRECT_FIELD_NAME in_popup = False token = None + user_info = None def get_in_popup(self): return self.in_popup @@ -331,6 +333,13 @@ class FcOAuthSessionViewMixin(LoggerMixin): self.scopes.extend(scopes) return ask_authorization(request, self.get_scopes(), self.logger) + @property + def fc_display_name(self): + '''Human representation of the current FC account''' + if not self.user_info: + return u'' + return u'{0} {1}'.format(self.user_info['family_name'], self.user_info['given_name']) + class PopupViewMixin(object): def get_in_popup(self): @@ -344,60 +353,43 @@ class LoginOrLinkView(PopupViewMixin, FcOAuthSessionViewMixin, View): ''' def update_user_info(self): + self.fc_account.token = json.dumps(self.token) self.fc_account.user_info = json.dumps(self.user_info) - self.fc_account.save() + self.fc_account.save(update_fields=['token', 'user_info']) utils.apply_user_info_mappings(self.fc_account.user, self.user_info) self.logger.debug('updating user_info %s', self.fc_account.user_info) + def uniqueness_check_failed(self, request): + if models.FcAccount.objects.filter(user=request.user, order=0).count(): + messages.error(request, + _('Your account is already linked to a FranceConnect account')) + else: + messages.error(request, + _('The FranceConnect account {} is already' + ' linked with another account.').format(self.fc_display_name)) + return self.redirect(request) + def get(self, request, *args, **kwargs): registration = True if 'registration' in request.GET else False '''Request an access grant code and associate it to the current user''' self.service_slug = request.GET.get(constants.SERVICE_FIELD_NAME) if request.user.is_authenticated(): - # Prevent to add a link with an FC account already linked with another user. try: - fc_account = models.FcAccount.objects.get(sub=self.sub, user__is_active=True) - if fc_account.user is not request.user: - msg = 'Attempt to link FC account {} already linked with user {}' - self.logger.info(msg.format(self.sub, fc_account.user)) - messages.error(request, - _('The FranceConnect account {} is already' - ' linked with another account.').format(fc_account)) - return self.redirect(request) - except models.FcAccount.DoesNotExist: - pass - - # Prevent to add a link to an user which is already linked to an FC account - if request.user.fc_accounts.exists(): - self.logger.warning(u'cannot link to sub %s, account is already linked to an ' - u'FC account', self.sub) - messages.error(request, - _('Your account is already linked to a FranceConnect account')) - return self.redirect(request) + self.fc_account, created = models.FcAccount.objects.get_or_create( + sub=self.sub, user=request.user, order=0, + defaults={'token': json.dumps(self.token)}) + except IntegrityError: + # unique index check failed, find why. + return self.uniqueness_check_failed(request) - json_token = json.dumps(self.token) - self.fc_account, created = models.FcAccount.objects.get_or_create( - defaults={'token': json_token}, - sub=self.sub, user=request.user) if created: self.logger.info('fc link created sub %s', self.sub) - self.update_user_info() - data = utils.get_mapped_attributes_flat(request) - if 'email' in data: - messages.info(request, - _('Your FranceConnect account {} with ' - 'email {} has been linked.').format(self.fc_account, - data['email'])) - else: - messages.info(request, _('Your FranceConnect account {} ' - 'has been linked.').format(self.fc_account)) - hooks.call_hooks('event', name='fc-link', user=request.user, sub=self.sub, - request=request) + messages.info(request, + _('Your FranceConnect account {} has been linked.').format(self.fc_display_name)) + hooks.call_hooks('event', name='fc-link', user=request.user, sub=self.sub, request=request) else: - self.fc_account.token = json_token - self.fc_account.save() - self.update_user_info() messages.info(request, _('Your local account has been updated.')) + self.update_user_info() return self.redirect(request) default_ou = get_default_ou() @@ -407,6 +399,8 @@ class LoginOrLinkView(PopupViewMixin, FcOAuthSessionViewMixin, View): sub=self.sub, user_info=self.user_info, token=self.token) + if user: + self.fc_account = user.fc_accounts.get(order=0) if not user and self.user_info.get('email') and email_is_unique: email = self.user_info['email'] User = get_user_model() @@ -424,9 +418,14 @@ class LoginOrLinkView(PopupViewMixin, FcOAuthSessionViewMixin, View): user = qs[0] # but does he have already a link to an FC account ? if not user.fc_accounts.exists(): - fc_account, created = models.FcAccount.objects.get_or_create( - defaults={'token': json.dumps(self.token)}, - sub=self.sub, user=user) + try: + self.fc_account, created = models.FcAccount.objects.get_or_create( + defaults={'token': json.dumps(self.token)}, + sub=self.sub, user=user, order=0) + except IntegrityError: + # unique index check failed, find why. + return self.uniqueness_check_failed(request) + if created: self.logger.info(u'fc link created sub %s user %s', self.sub, user) hooks.call_hooks('event', name='fc-link', user=user, sub=self.sub, @@ -446,9 +445,6 @@ class LoginOrLinkView(PopupViewMixin, FcOAuthSessionViewMixin, View): return self.redirect(request) if user: a2_utils.login(request, user, 'france-connect', service_slug=self.service_slug) - self.fc_account = models.FcAccount.objects.get(sub=self.sub, user=user) - self.fc_account.token = json.dumps(self.token) - self.fc_account.save(update_fields=['token']) self.update_user_info() self.logger.info('logged in using fc sub %s', self.sub) return self.redirect(request) diff --git a/tests/auth_fc/test_auth_fc.py b/tests/auth_fc/test_auth_fc.py index 3c02d768..b7404d22 100644 --- a/tests/auth_fc/test_auth_fc.py +++ b/tests/auth_fc/test_auth_fc.py @@ -588,3 +588,31 @@ def test_can_change_password(app, fc_settings, caplog, hooks): assert path(response['Location']) == '/accounts/' response = response.follow() assert len(response.pyquery('[href*="password/change"]')) > 0 + + +def test_migration_0002(migration): + migrate_from = [('authentic2_auth_fc', '0001_initial')] + # it's a two-parts migration, as it contains data and schema changes. + migrate_to = [('authentic2_auth_fc', '0003_fcaccount_order2')] + + old_apps = migration.before(migrate_from) + + User = old_apps.get_model('custom_user', 'User') + FcAccount = old_apps.get_model('authentic2_auth_fc', 'FcAccount') + user1 = User.objects.create(username='user1') + user2 = User.objects.create(username='user2') + user3 = User.objects.create(username='user3') + FcAccount.objects.create(user=user1, sub='sub1') + FcAccount.objects.create(user=user1, sub='sub2') + FcAccount.objects.create(user=user2, sub='sub2') + FcAccount.objects.create(user=user2, sub='sub3') + FcAccount.objects.create(user=user3, sub='sub3') + assert len(set(FcAccount.objects.values_list('user_id', flat=True))) == 3 + assert len(set(FcAccount.objects.values_list('sub', flat=True))) == 3 + assert FcAccount.objects.count() == 5 + + # execute migration + new_apps = migration.apply(migrate_to) + FcAccount = new_apps.get_model('authentic2_auth_fc', 'FcAccount') + assert len(set(FcAccount.objects.values_list('user_id', 'order'))) == 5 + assert len(set(FcAccount.objects.values_list('sub', 'order'))) == 5 diff --git a/tests/conftest.py b/tests/conftest.py index c5ecca05..c6d5bcbd 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -21,10 +21,14 @@ import mock import django_webtest +import django from django.contrib.auth import get_user_model from django_rbac.utils import get_ou_model, get_role_model from django.conf import settings from django.utils.six.moves.urllib import parse as urlparse +from django.db.migrations.executor import MigrationExecutor +from django.db import connection +from django.core.management import call_command from pytest_django.migrations import DisableMigrations @@ -360,3 +364,54 @@ def french_translation(): @pytest.fixture def media(settings, tmpdir): settings.MEDIA_ROOT = str(tmpdir.mkdir('media')) + + +@pytest.fixture() +def migration(request, transactional_db): + # see https://gist.github.com/asfaltboy/b3e6f9b5d95af8ba2cc46f2ba6eae5e2 + if django.VERSION < (1, 9): + pytest.skip('migration fixture only works with Django 1.9') + """ + This fixture returns a helper object to test Django data migrations. + The fixture returns an object with two methods; + - `before` to initialize db to the state before the migration under test + - `after` to execute the migration and bring db to the state after the migration + The methods return `old_apps` and `new_apps` respectively; these can + be used to initiate the ORM models as in the migrations themselves. + For example: + def test_foo_set_to_bar(migration): + old_apps = migration.before([('my_app', '0001_inital')]) + Foo = old_apps.get_model('my_app', 'foo') + Foo.objects.create(bar=False) + assert Foo.objects.count() == 1 + assert Foo.objects.filter(bar=False).count() == Foo.objects.count() + # executing migration + new_apps = migration.apply([('my_app', '0002_set_foo_bar')]) + Foo = new_apps.get_model('my_app', 'foo') + + assert Foo.objects.filter(bar=False).count() == 0 + assert Foo.objects.filter(bar=True).count() == Foo.objects.count() + Based on: https://gist.github.com/blueyed/4fb0a807104551f103e6 + """ + class Migrator(object): + def before(self, targets): + """ Specify app and starting migration names as in: + before([('app', '0001_before')]) => app/migrations/0001_before.py + """ + self.executor = MigrationExecutor(connection) + # prepare state of db to before the migration ("migrate_from" state) + self._old_apps = self.executor.migrate(targets).apps + return self._old_apps + + def apply(self, targets): + """ Migrate forwards to the "targets" migration """ + self.executor.loader.build_graph() # reload. + self._new_apps = self.executor.migrate(targets).apps + return self._new_apps + + # ensure to migrate forward migrated apps all the way after test + def migrate_to_end(): + call_command('migrate', verbosity=0) + request.addfinalizer(migrate_to_end) + + return Migrator() -- 2.20.1