From 5753b0967e1e88465888e4ebe0b8b2f04e47c079 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 | 53 ++++++++++++ tests/test_idp_oidc.py | 29 +++---- 8 files changed, 259 insertions(+), 70 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 10088737..8c7ef328 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 @@ -163,6 +164,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 @@ -332,6 +334,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): @@ -345,60 +354,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() @@ -408,6 +400,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() @@ -425,9 +419,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, @@ -447,9 +446,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 0e3e6558..dd25bf85 100644 --- a/tests/auth_fc/test_auth_fc.py +++ b/tests/auth_fc/test_auth_fc.py @@ -589,3 +589,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_0003(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, at_end=False) + + 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 58396a5d..9f1da46d 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 @@ -384,3 +388,52 @@ def service(db): ou=get_default_ou(), slug='service', name='Service') + + +@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, at_end=True): + """ Specify app and starting migration names as in: + before([('app', '0001_before')]) => app/migrations/0001_before.py + """ + executor = MigrationExecutor(connection) + executor.migrate(targets) + executor.loader.build_graph() + return executor._create_project_state(with_applied_migrations=True).apps + + def apply(self, targets): + """ Migrate forwards to the "targets" migration """ + executor = MigrationExecutor(connection) + executor.migrate(targets) + executor.loader.build_graph() + return executor._create_project_state(with_applied_migrations=True).apps + + yield Migrator() + + call_command('migrate', verbosity=0) diff --git a/tests/test_idp_oidc.py b/tests/test_idp_oidc.py index d6880db6..936a4daf 100644 --- a/tests/test_idp_oidc.py +++ b/tests/test_idp_oidc.py @@ -28,7 +28,6 @@ import utils from django.core.urlresolvers import reverse from django.core.files import File from django.db import connection -from django.db.migrations.executor import MigrationExecutor from django.utils.timezone import now from django.contrib.auth import get_user_model from django.utils.six.moves.urllib import parse as urlparse @@ -918,35 +917,33 @@ def test_registration_service_slug(oidc_settings, app, simple_oidc_client, simpl assert hooks.event[2]['kwargs']['service'] == 'client' -def test_oidclient_claims_data_migration(): - executor = MigrationExecutor(connection) +def test_oidclient_claims_data_migration(migration): app = 'authentic2_idp_oidc' migrate_from = [(app, '0009_auto_20180313_1156')] migrate_to = [(app, '0010_oidcclaim')] - executor.migrate(migrate_from) - executor.loader.build_graph() - old_apps = executor.loader.project_state(migrate_from).apps + old_apps = migration.before(migrate_from) OIDCClient = old_apps.get_model('authentic2_idp_oidc', 'OIDCClient') + client = OIDCClient(name='test', slug='test', redirect_uris='https://example.net/') client.save() - executor.migrate(migrate_to) - executor.loader.build_graph() + new_apps = migration.apply(migrate_to) + OIDCClient = new_apps.get_model('authentic2_idp_oidc', 'OIDCClient') + client = OIDCClient.objects.first() assert OIDCClaim.objects.filter(client=client.id).count() == 5 -def test_oidclient_preferred_username_as_identifier_data_migration(): - executor = MigrationExecutor(connection) +def test_oidclient_preferred_username_as_identifier_data_migration(migration): app = 'authentic2_idp_oidc' migrate_from = [(app, '0010_oidcclaim')] migrate_to = [(app, '0011_auto_20180808_1546')] - executor.migrate(migrate_from) - executor.loader.build_graph() - old_apps = executor.loader.project_state(migrate_from).apps + + old_apps = migration.before(migrate_from) OIDCClient = old_apps.get_model('authentic2_idp_oidc', 'OIDCClient') OIDCClaim = old_apps.get_model('authentic2_idp_oidc', 'OIDCClaim') + client1 = OIDCClient.objects.create(name='test', slug='test', redirect_uris='https://example.net/') client2 = OIDCClient.objects.create(name='test1', slug='test1', redirect_uris='https://example.net/') client3 = OIDCClient.objects.create(name='test2', slug='test2', redirect_uris='https://example.net/') @@ -964,8 +961,10 @@ def test_oidclient_preferred_username_as_identifier_data_migration(): continue OIDCClaim.objects.create(client=client, name='email', value='django_user_email', scopes='email') OIDCClaim.objects.create(client=client, name='email_verified', value='django_user_email_verified', scopes='email') - executor.migrate(migrate_to) - executor.loader.build_graph() + + new_apps = migration.apply(migrate_to) + OIDCClient = new_apps.get_model('authentic2_idp_oidc', 'OIDCClient') + client = OIDCClient.objects.first() for client in OIDCClient.objects.all(): claims = client.oidcclaim_set.all() -- 2.23.0