Merge pull request #4789 from kobotoolbox/remove-duplicate-django-allauth-app

Refactor django-allauth migration

Author: bufke

kobo/apps/accounts/admin.py

@@ -1,5 +1,9 @@
-from django.contrib import admin
 from allauth.account.models import EmailAddress
+from allauth.socialaccount.admin import SocialAppForm, SocialAppAdmin
+from allauth.socialaccount.models import SocialApp
+from django.contrib import admin
+from django.core.exceptions import ValidationError
+from django.db.models import Q
 
 from .models import EmailAddressAdmin, SocialAppCustomData
 from kobo.apps.accounts.models import EmailContent
@@ -13,4 +17,50 @@ class EmailContentView(admin.ModelAdmin):
 admin.site.unregister(EmailAddress)
 admin.site.register(EmailAddress, EmailAddressAdmin)
 
+
+class RequireProviderIdSocialAppForm(SocialAppForm):
+    def __init__(self, *args, **kwargs):
+        super(SocialAppForm, self).__init__(*args, **kwargs)
+        # require the provider_id in the admin, since we can't make it required on allauth's model
+        self.fields['provider_id'].required = True
+
+    def clean_provider_id(self):
+        reserved_keywords = ['kobo']
+        provider_id = self.cleaned_data.get('provider_id')
+        """
+        Don't allow `kobo` to be set as the `provider_id` value in `SOCIALACCOUNT_PROVIDERS`
+        settings because it breaks the login page redirect when language is changed.
+        """
+        if provider_id in reserved_keywords:
+            raise ValidationError(
+                f'`{provider_id}` is not a valid value for the `provider_id` setting.'
+            )
+
+        """
+        By default, django-allauth only supports showing one provider on the login screen.
+        But OIDC providers allow multiple subproviders, so kpi has some additional code to display multiple providers.
+        Because of that, we need to make sure that the `provider` and `provider_id` fields are unique.
+        django-allauth (as of 0.57.0) technically enforces this on the model level, but in practice it's flawed.
+        """
+        if SocialApp.objects.filter(
+            Q(provider_id=provider_id) |
+            Q(provider=provider_id)
+        ).exclude(pk=self.instance.pk).exists():
+            raise ValidationError(
+                """The Provider ID value must be unique and cannot match an existing Provider name.
+                Please use a different value."""
+            )
+        return provider_id
+
+
+class RequireProviderIdSocialAppAdmin(SocialAppAdmin):
+    form = RequireProviderIdSocialAppForm
+
+    class Meta:
+        proxy = True
+
+
+admin.site.unregister(SocialApp)
+admin.site.register(SocialApp, RequireProviderIdSocialAppAdmin)
+
 admin.site.register(SocialAppCustomData)

kobo/apps/django_allauth/migrations/0001_initial.py renamed to kobo/apps/accounts/migrations/0007_add_providers_from_environment_to_db.py

@@ -1,15 +1,17 @@
 import os
 import re
 from django.db import migrations
+from django.db.models import Q
+
 
-"""
-Add any OIDC providers from the environment variables to the database. The upgrade to
-[email protected] removed code from /settings/base.py that parsed OIDC settings from
-environment variables, so this migration is needed to avoid manually setting up OIDC
-providers from the admin. Adapted from:
-https://gitlab.com/glitchtip/glitchtip-backend/-/blob/master/users/migrations/0010_allauth_oidc_from_env_var.py?ref_type=heads
-"""
 def add_OIDC_settings_from_env(apps, schema_editor):
+    """
+    Add any OIDC providers from the environment variables to the database. The upgrade to
+    [email protected] removed code from /settings/base.py that parsed OIDC settings from
+    environment variables, so this migration is needed to avoid manually setting up OIDC
+    providers from the admin. Adapted from:
+    https://gitlab.com/glitchtip/glitchtip-backend/-/blob/master/users/migrations/0010_allauth_oidc_from_env_var.py?ref_type=heads
+    """
     SocialApp = apps.get_model('socialaccount', 'SocialApp')
     SocialAppCustomData = apps.get_model('accounts', 'SocialAppCustomData')
 
@@ -31,24 +33,36 @@ def add_OIDC_settings_from_env(apps, schema_editor):
 
     for index, app in enumerate(oidc_apps):
         app_id = app.get('id')
+
         # the app needs a name to be editable in the admin - give it a default name if necessary
         app_name = app.get('name', f'OIDC {index}')
         app_settings = {'server_url': app.get('server_url', None)}
+
         # parse the tenant variable for microsoft OIDC providers - check for uppercase (old way) and lowercase (new way)
         if app_tenant := app.get('TENANT', app.get('tenant', None)):
             app_settings['tenant'] = app_tenant
-        if app_id and app_settings['server_url'] and not (
-            # if the app already exists, let's assume it's been configured
-            SocialApp.objects.filter(provider_id=app_id).exists()
-        ):
-            db_social_app, created = SocialApp.objects.get_or_create(provider_id=app_id)
-            db_social_app.provider = 'openid_connect'
-            db_social_app.name = app_name
-            db_social_app.client_id = app.get('APP_client_id', '')
-            db_social_app.secret = app.get('APP_secret', '')
-            db_social_app.key = app.get('APP_key', '')
+
+        if app_id and app_settings['server_url']:
+            db_social_app = SocialApp.objects.filter(
+                Q(provider=app_id) |
+                Q(provider__iexact='openid_connect', provider_id=app_id) |
+                Q(provider='', name__iexact=app_name)
+            ).first()
+            if not db_social_app:
+                db_social_app = SocialApp.objects.create()
             db_social_app.settings = app_settings
+            if not db_social_app.settings.get('previous_provider'):
+                db_social_app.settings['previous_provider'] = app_id
+            db_social_app.provider = 'openid_connect'
+            db_social_app.provider_id = app_id
+            # we don't want to overwrite the following settings if they're already defined
+            # on the social app we grabbed
+            db_social_app.name = db_social_app.name or app_name
+            db_social_app.client_id = db_social_app.client_id or app.get('APP_client_id', '')
+            db_social_app.secret = db_social_app.secret or app.get('APP_secret', '')
+            db_social_app.key = db_social_app.key or app.get('APP_key', '')
             db_social_app.save()
+
             # hide the OIDC provider from the login page, since it was already hidden
             SocialAppCustomData.objects.get_or_create(social_app=db_social_app)
 
@@ -65,14 +79,24 @@ def add_OIDC_settings_from_env(apps, schema_editor):
             ms_app.save()
 
 
+def revert_OIDC_provider_id(apps, schema_editor):
+    SocialApp = apps.get_model('socialaccount', 'SocialApp')
+
+    changed_apps = SocialApp.objects.filter(settings__has_key='previous_provider')
+    for app in list(changed_apps):
+        app.provider = app.settings['previous_provider']
+        app.save()
+
+
 class Migration(migrations.Migration):
     dependencies = [
         ('socialaccount', '0005_socialtoken_nullable_app'),
-        ('accounts', '0005_do_nothing'),
+        ('accounts', '0006_alter_emailcontent_unique_together'),
     ]
 
     operations = [
         migrations.RunPython(
-            code=add_OIDC_settings_from_env, reverse_code=migrations.RunPython.noop
+            code=add_OIDC_settings_from_env,
+            reverse_code=revert_OIDC_provider_id,
         )
     ]

kobo/apps/django_allauth/__init__.py

@@ -128,7 +128,6 @@
     'kobo.apps.trash_bin.TrashBinAppConfig',
     'kobo.apps.markdownx_uploader.MarkdownxUploaderAppConfig',
     'kobo.apps.form_disclaimer.FormDisclaimerAppConfig',
-    'kobo.apps.django_allauth',
 )
 
 MIDDLEWARE = [