From 1deac58d84469d3ce5e8d57041191dcdd206ba4b Mon Sep 17 00:00:00 2001 From: David Wallace Date: Wed, 18 Dec 2024 15:13:24 +0100 Subject: [PATCH 01/17] feat(accounts,ToU): add ToU update view, form and middleware #141 #161 Signed-off-by: David Wallace --- rdmo/accounts/forms.py | 36 ++++++++- rdmo/accounts/middleware.py | 74 +++++++++++++++++++ rdmo/accounts/signals.py | 13 ++++ .../account/terms_of_use_update_form.html | 33 +++++++++ rdmo/accounts/urls/__init__.py | 13 +++- rdmo/accounts/utils.py | 17 ++++- rdmo/accounts/views.py | 36 ++++++++- 7 files changed, 214 insertions(+), 8 deletions(-) create mode 100644 rdmo/accounts/middleware.py create mode 100644 rdmo/accounts/signals.py create mode 100644 rdmo/accounts/templates/account/terms_of_use_update_form.html diff --git a/rdmo/accounts/forms.py b/rdmo/accounts/forms.py index 52ad754417..353c2cf244 100644 --- a/rdmo/accounts/forms.py +++ b/rdmo/accounts/forms.py @@ -5,7 +5,7 @@ from django.contrib.auth import get_user_model from django.utils.translation import gettext_lazy as _ -from .models import AdditionalField, AdditionalFieldValue +from .models import AdditionalField, AdditionalFieldValue, ConsentFieldValue log = logging.getLogger(__name__) @@ -87,3 +87,37 @@ def __init__(self, *args, **kwargs): consent = forms.BooleanField(required=True) consent.label = _("I confirm that I want my profile to be completely removed. This can not be undone!") + + +class UpdateConsentForm(forms.Form): + + consent = forms.BooleanField( + label="I agree to the terms of use", + required=False, # not required because it won't be submitted during a delete + ) + + def __init__(self, *args, user=None, **kwargs): + self.user = user + super().__init__(*args, **kwargs) + + if not self.user: + raise ValueError("A user instance is required to initialize the form.") + + # pre-fill the 'consent' field based on the user's existing consent + self.fields["consent"].initial = ConsentFieldValue.objects.filter( + user=self.user + ).exists() + + def save(self) -> bool: + + if "delete" in self.data: + ConsentFieldValue.objects.filter(user=self.user).delete() + return False # consent was revoked + + if self.cleaned_data.get("consent"): + ConsentFieldValue.objects.update_or_create( + user=self.user, defaults={"consent": True} + ) + return True # consent was accepted + + return False diff --git a/rdmo/accounts/middleware.py b/rdmo/accounts/middleware.py new file mode 100644 index 0000000000..dbf654d9d1 --- /dev/null +++ b/rdmo/accounts/middleware.py @@ -0,0 +1,74 @@ +"""Terms and Conditions Middleware""" + +# ref: https://github.com/cyface/django-termsandconditions/blob/main/termsandconditions/middleware.py +import logging + +from django.conf import settings +from django.http import HttpResponseRedirect +from django.urls import reverse +from django.utils.deprecation import MiddlewareMixin + +from .utils import user_has_accepted_terms + +LOGGER = logging.getLogger(__name__) + + +ACCEPT_TERMS_PATH = getattr(settings, "ACCEPT_TERMS_PATH", reverse("terms_of_use_update")) +TERMS_EXCLUDE_URL_PREFIX_LIST = getattr( + settings, + "TERMS_EXCLUDE_URL_PREFIX_LIST", + {"/admin", "/i18n", "/static", "/account"}, +) +TERMS_EXCLUDE_URL_CONTAINS_LIST = getattr( + settings, "TERMS_EXCLUDE_URL_CONTAINS_LIST", {} +) +TERMS_EXCLUDE_URL_LIST = getattr( + settings, + "TERMS_EXCLUDE_URL_LIST", + {"/", settings.LOGOUT_URL}, +) + + +class TermsAndConditionsRedirectMiddleware(MiddlewareMixin): + + def process_request(self, request): + """Process each request to app to ensure terms have been accepted""" + + if not settings.ACCOUNT_TERMS_OF_USE: + return None # If terms are not enabled, consider them accepted. + + current_path = request.META["PATH_INFO"] + + if request.user.is_authenticated and is_path_protected(current_path): + if not user_has_accepted_terms(request.user, request.session): + # Redirect to update consent page if consent is missing + return HttpResponseRedirect(reverse("terms_of_use_update")) + + return None + + +def is_path_protected(path): + """ + returns True if given path is to be protected, otherwise False + + The path is not to be protected when it appears on: + TERMS_EXCLUDE_URL_PREFIX_LIST, TERMS_EXCLUDE_URL_LIST, TERMS_EXCLUDE_URL_CONTAINS_LIST or as + ACCEPT_TERMS_PATH + """ + protected = True + + for exclude_path in TERMS_EXCLUDE_URL_PREFIX_LIST: + if path.startswith(exclude_path): + protected = False + + for contains_path in TERMS_EXCLUDE_URL_CONTAINS_LIST: + if contains_path in path: + protected = False + + if path in TERMS_EXCLUDE_URL_LIST: + protected = False + + if path.startswith(ACCEPT_TERMS_PATH): + protected = False + + return protected diff --git a/rdmo/accounts/signals.py b/rdmo/accounts/signals.py new file mode 100644 index 0000000000..b08e28d09d --- /dev/null +++ b/rdmo/accounts/signals.py @@ -0,0 +1,13 @@ +from django.contrib.auth.signals import user_logged_in +from django.dispatch import receiver +from django.http import HttpResponseRedirect +from django.urls import reverse + +from .utils import user_has_accepted_terms + + +@receiver(user_logged_in) +def check_user_consent(sender, request, user, **kwargs): + # check consent and store it in the session + if not user_has_accepted_terms(user, request.session): + return HttpResponseRedirect(reverse("terms_of_use_update")) diff --git a/rdmo/accounts/templates/account/terms_of_use_update_form.html b/rdmo/accounts/templates/account/terms_of_use_update_form.html new file mode 100644 index 0000000000..ba0caa5e72 --- /dev/null +++ b/rdmo/accounts/templates/account/terms_of_use_update_form.html @@ -0,0 +1,33 @@ +{% extends 'core/page.html' %} +{% load i18n %} + +{% block page %} + +

{% trans 'Terms of use' %}

+ +

+ {% get_current_language as lang %} + {% if lang == 'en' %} + {% include 'account/terms_of_use_en.html' %} + {% elif lang == 'de' %} + {% include 'account/terms_of_use_de.html' %} + {% endif %} +

+ +
+
+ {% csrf_token %} + {% if not has_consented %} + + + {% else %} + + {% endif %} +
+
+ +{% endblock %} diff --git a/rdmo/accounts/urls/__init__.py b/rdmo/accounts/urls/__init__.py index 7e04c88452..24782f47d5 100644 --- a/rdmo/accounts/urls/__init__.py +++ b/rdmo/accounts/urls/__init__.py @@ -2,7 +2,15 @@ from django.contrib.auth import views as auth_views from django.urls import include, re_path -from ..views import profile_update, remove_user, shibboleth_login, shibboleth_logout, terms_of_use, token +from ..views import ( + profile_update, + remove_user, + shibboleth_login, + shibboleth_logout, + terms_of_use, + terms_of_use_update, + token, +) urlpatterns = [ # edit own profile @@ -12,7 +20,8 @@ if settings.ACCOUNT_TERMS_OF_USE is True: urlpatterns += [ - re_path('^terms-of-use/', terms_of_use, name='terms_of_use') + re_path('^terms-of-use/$', terms_of_use, name='terms_of_use'), + re_path('^terms-of-use/update/$', terms_of_use_update, name='terms_of_use_update'), ] if settings.SHIBBOLETH: diff --git a/rdmo/accounts/utils.py b/rdmo/accounts/utils.py index 5bbb360496..43e07ecf69 100644 --- a/rdmo/accounts/utils.py +++ b/rdmo/accounts/utils.py @@ -6,11 +6,12 @@ from django.contrib.contenttypes.models import ContentType from django.core.exceptions import ObjectDoesNotExist -from .models import Role +from .models import ConsentFieldValue, Role from .settings import GROUPS log = logging.getLogger(__name__) +CONSENT_SESSION_KEY = "user_has_consented" def get_full_name(user): if user.first_name and user.last_name: @@ -93,3 +94,17 @@ def get_user_from_db_or_none(username: str, email: str): except ObjectDoesNotExist: log.error('Retrieval of user "%s" with email "%s" failed, user does not exist', username, email) return None + + +def user_has_accepted_terms(user, session) -> bool: + if not settings.ACCOUNT_TERMS_OF_USE: + return True # If terms are not enabled, consider them accepted. + + # check the session for cached consent status + if CONSENT_SESSION_KEY in session: + return session[CONSENT_SESSION_KEY] + + has_consented = ConsentFieldValue.objects.filter(user=user).exists() + session[CONSENT_SESSION_KEY] = has_consented # cache the result in the session + + return has_consented diff --git a/rdmo/accounts/views.py b/rdmo/accounts/views.py index 3300904cb3..e3086021b0 100644 --- a/rdmo/accounts/views.py +++ b/rdmo/accounts/views.py @@ -4,16 +4,17 @@ from django.conf import settings from django.contrib.auth import logout from django.contrib.auth.decorators import login_required -from django.http import HttpResponseRedirect -from django.shortcuts import render +from django.http import HttpResponseNotAllowed, HttpResponseRedirect +from django.shortcuts import redirect, render from django.urls import reverse from rest_framework.authtoken.models import Token from rdmo.core.utils import get_next, get_referer_path_info -from .forms import ProfileForm, RemoveForm -from .utils import delete_user +from .forms import ProfileForm, RemoveForm, UpdateConsentForm +from .models import ConsentFieldValue +from .utils import CONSENT_SESSION_KEY, delete_user log = logging.getLogger(__name__) @@ -109,3 +110,30 @@ def shibboleth_logout(request): or re.search(settings.SHIBBOLETH_USERNAME_PATTERN, request.user.username): logout_url += f'?next={settings.SHIBBOLETH_LOGOUT_URL}' return HttpResponseRedirect(logout_url) + + +def terms_of_use_update(request): + + if not request.user.is_authenticated: + return redirect("account_login") + + if request.method == "POST": + # Use the form to handle both update and delete actions + form = UpdateConsentForm(request.POST, user=request.user) + if form.is_valid(): + consent_status = form.save() + request.session[CONSENT_SESSION_KEY] = consent_status + # Update the session to reflect the new consent status + return redirect("home") + + elif request.method == "GET": + # Render the consent update form + form = UpdateConsentForm(user=request.user) + has_consented = ConsentFieldValue.objects.filter(user=request.user).exists() + return render( + request, + "account/terms_of_use_update_form.html", + {"form": form, "has_consented": has_consented}, + ) + + return HttpResponseNotAllowed(["GET", "POST"]) From 187c6303f476a305a90a573d14a0b70dbfe209fd Mon Sep 17 00:00:00 2001 From: David Wallace Date: Wed, 18 Dec 2024 15:14:11 +0100 Subject: [PATCH 02/17] tests(accounts,ToU): add tests for ToU update and middleware Signed-off-by: David Wallace --- rdmo/accounts/tests/test_views.py | 84 +++++++++++++++++++++++++++++++ 1 file changed, 84 insertions(+) diff --git a/rdmo/accounts/tests/test_views.py b/rdmo/accounts/tests/test_views.py index a76ce39052..cf4083f042 100644 --- a/rdmo/accounts/tests/test_views.py +++ b/rdmo/accounts/tests/test_views.py @@ -10,7 +10,9 @@ from pytest_django.asserts import assertContains, assertNotContains, assertRedirects, assertTemplateUsed, assertURLEqual +from rdmo.accounts.models import ConsentFieldValue from rdmo.accounts.tests.utils import reload_urls +from rdmo.accounts.utils import CONSENT_SESSION_KEY users = ( ('editor', 'editor'), @@ -795,3 +797,85 @@ def test_shibboleth_logout_username_pattern(db, client, settings, shibboleth, us assertURLEqual(response.url, reverse('account_logout') + f'?next={settings.SHIBBOLETH_LOGOUT_URL}') else: assertURLEqual(response.url, reverse('account_logout')) + + +@pytest.mark.parametrize('post_consent', [True, False]) +@pytest.mark.parametrize('username,password', users) +def test_terms_of_use_middleware_redirect_and_update( + db, client, settings, username, password, post_consent +): + """ + Test the behavior of TermsAndConditionsRedirectMiddleware and terms_of_use_update view: + - When the middleware is enabled, users are redirected to the terms update page if not consented. + - Valid POST saves consent to the session and database. + - Invalid POST does not save consent to the session or database. + """ + # Arrange + settings.ACCOUNT_TERMS_OF_USE = True + reload_urls('accounts') # needs to reload before the middleware + settings.MIDDLEWARE += [ + "rdmo.accounts.middleware.TermsAndConditionsRedirectMiddleware" + ] + + # Ensure there are no existing consent entries for the user + user = get_user_model().objects.get(username=username) if password else None + if user: + ConsentFieldValue.objects.filter(user=user).delete() + + # Act - Access the home page + client.login(username=username, password=password) + response = client.get(reverse('projects')) + + # Assert - Middleware should redirect to terms_of_use_update + if password is not None: + assert response.status_code == 302 + assert response.url == reverse('terms_of_use_update') + + # Session should not yet have consent + assert not client.session.get(CONSENT_SESSION_KEY, False) + else: + # Anonymous user is redirected to login + assert response.status_code == 302 + assert response.url == reverse('account_login') + '?next=' + reverse('projects') + return # Exit test for anonymous users + + # Act - Make a POST request to terms_of_use_update + terms_update_url = reverse('terms_of_use_update') + data = { + 'consent': post_consent, # Set based on parameterization + } + response = client.post(terms_update_url, data) + + assert response.status_code == 302 + assert response.url == reverse('home') + + # Assert POST behavior + if post_consent: # accepted you + # Consent should be stored in the session and database + assert client.session[CONSENT_SESSION_KEY] is True + assert ConsentFieldValue.objects.filter(user=user).exists() + + response = client.get(reverse('projects')) + assert response.status_code == 200 + + # also test the revoke consent + response = client.post(terms_update_url, {'delete': ''}) + assert response.status_code == 302 + assert response.url == reverse('home') + assert client.session.get(CONSENT_SESSION_KEY, False) is False + assert not ConsentFieldValue.objects.filter(user=user).exists() + + response = client.get(reverse('projects')) + assert response.status_code == 302 + assert response.url == reverse('terms_of_use_update') + + else: # did not accept you + # Invalid POST + # assertTemplateUsed(response, "account/terms_of_use_update_form.html") + # Consent should not be stored in the session or database + assert client.session.get(CONSENT_SESSION_KEY, False) is False + assert not ConsentFieldValue.objects.filter(user=user).exists() + + response = client.get(reverse('projects')) + assert response.status_code == 302 + assert response.url == reverse('terms_of_use_update') From 8c2a84b0b56500022bce531395ca2c6414ddc012 Mon Sep 17 00:00:00 2001 From: David Wallace Date: Wed, 18 Dec 2024 18:30:49 +0100 Subject: [PATCH 03/17] refactor(accounts, middleware): change ToU middleware to callable class Signed-off-by: David Wallace --- rdmo/accounts/middleware.py | 80 ++++++++++++++++++------------------- 1 file changed, 39 insertions(+), 41 deletions(-) diff --git a/rdmo/accounts/middleware.py b/rdmo/accounts/middleware.py index dbf654d9d1..9b04342c43 100644 --- a/rdmo/accounts/middleware.py +++ b/rdmo/accounts/middleware.py @@ -1,74 +1,72 @@ """Terms and Conditions Middleware""" - # ref: https://github.com/cyface/django-termsandconditions/blob/main/termsandconditions/middleware.py import logging from django.conf import settings from django.http import HttpResponseRedirect from django.urls import reverse -from django.utils.deprecation import MiddlewareMixin from .utils import user_has_accepted_terms LOGGER = logging.getLogger(__name__) - ACCEPT_TERMS_PATH = getattr(settings, "ACCEPT_TERMS_PATH", reverse("terms_of_use_update")) TERMS_EXCLUDE_URL_PREFIX_LIST = getattr( settings, "TERMS_EXCLUDE_URL_PREFIX_LIST", - {"/admin", "/i18n", "/static", "/account"}, -) -TERMS_EXCLUDE_URL_CONTAINS_LIST = getattr( - settings, "TERMS_EXCLUDE_URL_CONTAINS_LIST", {} + ["/admin", "/i18n", "/static", "/account"], ) +TERMS_EXCLUDE_URL_CONTAINS_LIST = getattr(settings, "TERMS_EXCLUDE_URL_CONTAINS_LIST", []) TERMS_EXCLUDE_URL_LIST = getattr( settings, "TERMS_EXCLUDE_URL_LIST", - {"/", settings.LOGOUT_URL}, + ["/", settings.LOGOUT_URL], ) -class TermsAndConditionsRedirectMiddleware(MiddlewareMixin): - - def process_request(self, request): - """Process each request to app to ensure terms have been accepted""" - - if not settings.ACCOUNT_TERMS_OF_USE: - return None # If terms are not enabled, consider them accepted. - - current_path = request.META["PATH_INFO"] +class TermsAndConditionsRedirectMiddleware: + """Middleware to ensure terms and conditions have been accepted.""" - if request.user.is_authenticated and is_path_protected(current_path): - if not user_has_accepted_terms(request.user, request.session): - # Redirect to update consent page if consent is missing - return HttpResponseRedirect(reverse("terms_of_use_update")) + def __init__(self, get_response): + self.get_response = get_response - return None + def __call__(self, request): + # Skip processing if ACCOUNT_TERMS_OF_USE is disabled + if not getattr(settings, "ACCOUNT_TERMS_OF_USE", False): + return self.get_response(request) + # check if the current path is protected + if ( + request.user.is_authenticated + and self.is_path_protected(request.path) + and not user_has_accepted_terms(request.user, request.session) + ): + return HttpResponseRedirect(ACCEPT_TERMS_PATH) -def is_path_protected(path): - """ - returns True if given path is to be protected, otherwise False + # Proceed with the response for non-protected paths or accepted terms + return self.get_response(request) - The path is not to be protected when it appears on: - TERMS_EXCLUDE_URL_PREFIX_LIST, TERMS_EXCLUDE_URL_LIST, TERMS_EXCLUDE_URL_CONTAINS_LIST or as - ACCEPT_TERMS_PATH - """ - protected = True + @staticmethod + def is_path_protected(path): + """ + Determine if a given path is protected by the middleware. - for exclude_path in TERMS_EXCLUDE_URL_PREFIX_LIST: - if path.startswith(exclude_path): - protected = False + Paths are excluded if they match any of the following: + - Start with a prefix in TERMS_EXCLUDE_URL_PREFIX_LIST + - Contain a substring in TERMS_EXCLUDE_URL_CONTAINS_LIST + - Are explicitly listed in TERMS_EXCLUDE_URL_LIST + - Start with the ACCEPT_TERMS_PATH + """ + if any(path.startswith(prefix) for prefix in TERMS_EXCLUDE_URL_PREFIX_LIST): + return False - for contains_path in TERMS_EXCLUDE_URL_CONTAINS_LIST: - if contains_path in path: - protected = False + if any(substring in path for substring in TERMS_EXCLUDE_URL_CONTAINS_LIST): + return False - if path in TERMS_EXCLUDE_URL_LIST: - protected = False + if path in TERMS_EXCLUDE_URL_LIST: + return False - if path.startswith(ACCEPT_TERMS_PATH): - protected = False + if path.startswith(ACCEPT_TERMS_PATH): + return False - return protected + return True From 3c2a3bc279f885eccaef3f2edd4f3b83fb41e760 Mon Sep 17 00:00:00 2001 From: David Wallace Date: Thu, 23 Jan 2025 17:14:06 +0100 Subject: [PATCH 04/17] feat(accounts,ToU): add updated and created to Consent Model Signed-off-by: David Wallace --- rdmo/accounts/admin.py | 2 +- .../0022_add_created_updated_to_consent.py | 26 +++++++++++++++++++ rdmo/accounts/models.py | 3 ++- 3 files changed, 29 insertions(+), 2 deletions(-) create mode 100644 rdmo/accounts/migrations/0022_add_created_updated_to_consent.py diff --git a/rdmo/accounts/admin.py b/rdmo/accounts/admin.py index 8d65fb1f57..17e825f40f 100644 --- a/rdmo/accounts/admin.py +++ b/rdmo/accounts/admin.py @@ -17,7 +17,7 @@ class AdditionalFieldValueAdmin(admin.ModelAdmin): @admin.register(ConsentFieldValue) class ConsentFieldValueAdmin(admin.ModelAdmin): - readonly_fields = ('user', 'consent') + readonly_fields = ('user', 'consent', 'updated', 'created') def has_add_permission(self, request, obj=None): return False diff --git a/rdmo/accounts/migrations/0022_add_created_updated_to_consent.py b/rdmo/accounts/migrations/0022_add_created_updated_to_consent.py new file mode 100644 index 0000000000..de41f3cab3 --- /dev/null +++ b/rdmo/accounts/migrations/0022_add_created_updated_to_consent.py @@ -0,0 +1,26 @@ +# Generated by Django 4.2.17 on 2025-01-23 16:06 + +from django.db import migrations, models +import django.utils.timezone + + +class Migration(migrations.Migration): + + dependencies = [ + ('accounts', '0021_alter_help_text'), + ] + + operations = [ + migrations.AddField( + model_name='consentfieldvalue', + name='created', + field=models.DateTimeField(default=django.utils.timezone.now, editable=False, verbose_name='created'), + preserve_default=False, + ), + migrations.AddField( + model_name='consentfieldvalue', + name='updated', + field=models.DateTimeField(default=django.utils.timezone.now, editable=False, verbose_name='updated'), + preserve_default=False, + ), + ] diff --git a/rdmo/accounts/models.py b/rdmo/accounts/models.py index d9e960f2d5..8b2ebfee9d 100644 --- a/rdmo/accounts/models.py +++ b/rdmo/accounts/models.py @@ -5,6 +5,7 @@ from django.dispatch import receiver from django.utils.translation import gettext_lazy as _ +from rdmo.core.models import Model as RDMOTimeStampedModel from rdmo.core.models import TranslationMixin @@ -106,7 +107,7 @@ def __str__(self): return self.user.username + '/' + self.field.key -class ConsentFieldValue(models.Model): +class ConsentFieldValue(RDMOTimeStampedModel): user = models.OneToOneField(settings.AUTH_USER_MODEL, on_delete=models.CASCADE) consent = models.BooleanField( From 0d694f7036829d55137050729ea944d4f6915ec5 Mon Sep 17 00:00:00 2001 From: David Wallace Date: Mon, 3 Feb 2025 11:00:13 +0100 Subject: [PATCH 05/17] feat(accounts,admin): update list display for ConsentFieldValue Signed-off-by: David Wallace --- rdmo/accounts/admin.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/rdmo/accounts/admin.py b/rdmo/accounts/admin.py index 17e825f40f..0edae0e7a5 100644 --- a/rdmo/accounts/admin.py +++ b/rdmo/accounts/admin.py @@ -19,6 +19,10 @@ class AdditionalFieldValueAdmin(admin.ModelAdmin): class ConsentFieldValueAdmin(admin.ModelAdmin): readonly_fields = ('user', 'consent', 'updated', 'created') + search_fields = ('user__username', 'user__email') + list_display = ('user', 'consent', 'updated') + list_filter = ('consent',) + def has_add_permission(self, request, obj=None): return False From e52f94085e2fedb8da7ccc7ee9f4909567e83334 Mon Sep 17 00:00:00 2001 From: David Wallace Date: Mon, 3 Feb 2025 12:50:15 +0100 Subject: [PATCH 06/17] feat(accounts,ToU): disable revocation and rename to accept Signed-off-by: David Wallace --- rdmo/accounts/forms.py | 29 +++++++------- rdmo/accounts/middleware.py | 11 ++---- .../account/terms_of_use_accept_form.html | 38 +++++++++++++++++++ .../account/terms_of_use_update_form.html | 33 ---------------- rdmo/accounts/urls/__init__.py | 6 +-- rdmo/accounts/views.py | 13 +++---- 6 files changed, 63 insertions(+), 67 deletions(-) create mode 100644 rdmo/accounts/templates/account/terms_of_use_accept_form.html delete mode 100644 rdmo/accounts/templates/account/terms_of_use_update_form.html diff --git a/rdmo/accounts/forms.py b/rdmo/accounts/forms.py index 353c2cf244..b48ab53bed 100644 --- a/rdmo/accounts/forms.py +++ b/rdmo/accounts/forms.py @@ -93,7 +93,7 @@ class UpdateConsentForm(forms.Form): consent = forms.BooleanField( label="I agree to the terms of use", - required=False, # not required because it won't be submitted during a delete + required=True, ) def __init__(self, *args, user=None, **kwargs): @@ -104,20 +104,19 @@ def __init__(self, *args, user=None, **kwargs): raise ValueError("A user instance is required to initialize the form.") # pre-fill the 'consent' field based on the user's existing consent - self.fields["consent"].initial = ConsentFieldValue.objects.filter( - user=self.user - ).exists() + # Ensure the user cannot uncheck the consent + has_consented = ConsentFieldValue.objects.filter(user=self.user).exists() + if has_consented: + self.fields["consent"].disabled = True # Disable field after acceptance + self.fields["consent"].initial = True + else: + self.fields["consent"].initial = False def save(self) -> bool: + if not ConsentFieldValue.objects.filter(user=self.user).exists(): + if self.cleaned_data.get("consent"): + ConsentFieldValue.objects.create(user=self.user, consent=True) + # session consent is updated in the view + return True # consent accepted - if "delete" in self.data: - ConsentFieldValue.objects.filter(user=self.user).delete() - return False # consent was revoked - - if self.cleaned_data.get("consent"): - ConsentFieldValue.objects.update_or_create( - user=self.user, defaults={"consent": True} - ) - return True # consent was accepted - - return False + return False # No changes made diff --git a/rdmo/accounts/middleware.py b/rdmo/accounts/middleware.py index 9b04342c43..5a2ebd22d1 100644 --- a/rdmo/accounts/middleware.py +++ b/rdmo/accounts/middleware.py @@ -1,6 +1,5 @@ """Terms and Conditions Middleware""" # ref: https://github.com/cyface/django-termsandconditions/blob/main/termsandconditions/middleware.py -import logging from django.conf import settings from django.http import HttpResponseRedirect @@ -8,9 +7,6 @@ from .utils import user_has_accepted_terms -LOGGER = logging.getLogger(__name__) - -ACCEPT_TERMS_PATH = getattr(settings, "ACCEPT_TERMS_PATH", reverse("terms_of_use_update")) TERMS_EXCLUDE_URL_PREFIX_LIST = getattr( settings, "TERMS_EXCLUDE_URL_PREFIX_LIST", @@ -36,12 +32,14 @@ def __call__(self, request): return self.get_response(request) # check if the current path is protected + accept_terms_path = reverse("terms_of_use_accept") if ( request.user.is_authenticated + and not request.path.startswith(accept_terms_path) and self.is_path_protected(request.path) and not user_has_accepted_terms(request.user, request.session) ): - return HttpResponseRedirect(ACCEPT_TERMS_PATH) + return HttpResponseRedirect(accept_terms_path) # Proceed with the response for non-protected paths or accepted terms return self.get_response(request) @@ -66,7 +64,4 @@ def is_path_protected(path): if path in TERMS_EXCLUDE_URL_LIST: return False - if path.startswith(ACCEPT_TERMS_PATH): - return False - return True diff --git a/rdmo/accounts/templates/account/terms_of_use_accept_form.html b/rdmo/accounts/templates/account/terms_of_use_accept_form.html new file mode 100644 index 0000000000..ede86a91e6 --- /dev/null +++ b/rdmo/accounts/templates/account/terms_of_use_accept_form.html @@ -0,0 +1,38 @@ +{% extends 'core/page.html' %} +{% load i18n %} + +{% block page %} + +

{% trans 'Terms of use' %}

+ +

+ {% get_current_language as lang %} + {% if lang == 'en' %} + {% include 'account/terms_of_use_en.html' %} + {% elif lang == 'de' %} + {% include 'account/terms_of_use_de.html' %} + {% endif %} +

+ +
+ {% if not has_consented %} +
+ {% csrf_token %} +
+ +
+ +
+ {% else %} +

+ {% trans "You have accepted the terms of use." %} +

+ {% endif %} +
+ +{% endblock %} diff --git a/rdmo/accounts/templates/account/terms_of_use_update_form.html b/rdmo/accounts/templates/account/terms_of_use_update_form.html deleted file mode 100644 index ba0caa5e72..0000000000 --- a/rdmo/accounts/templates/account/terms_of_use_update_form.html +++ /dev/null @@ -1,33 +0,0 @@ -{% extends 'core/page.html' %} -{% load i18n %} - -{% block page %} - -

{% trans 'Terms of use' %}

- -

- {% get_current_language as lang %} - {% if lang == 'en' %} - {% include 'account/terms_of_use_en.html' %} - {% elif lang == 'de' %} - {% include 'account/terms_of_use_de.html' %} - {% endif %} -

- -
-
- {% csrf_token %} - {% if not has_consented %} - - - {% else %} - - {% endif %} -
-
- -{% endblock %} diff --git a/rdmo/accounts/urls/__init__.py b/rdmo/accounts/urls/__init__.py index 24782f47d5..8e43759928 100644 --- a/rdmo/accounts/urls/__init__.py +++ b/rdmo/accounts/urls/__init__.py @@ -8,7 +8,7 @@ shibboleth_login, shibboleth_logout, terms_of_use, - terms_of_use_update, + terms_of_use_accept, token, ) @@ -18,10 +18,10 @@ re_path('^remove', remove_user, name='profile_remove'), ] -if settings.ACCOUNT_TERMS_OF_USE is True: +if settings.ACCOUNT_TERMS_OF_USE: urlpatterns += [ + re_path('^terms-of-use/accept/$', terms_of_use_accept, name='terms_of_use_accept'), re_path('^terms-of-use/$', terms_of_use, name='terms_of_use'), - re_path('^terms-of-use/update/$', terms_of_use_update, name='terms_of_use_update'), ] if settings.SHIBBOLETH: diff --git a/rdmo/accounts/views.py b/rdmo/accounts/views.py index e3086021b0..80d6dac3f8 100644 --- a/rdmo/accounts/views.py +++ b/rdmo/accounts/views.py @@ -112,7 +112,7 @@ def shibboleth_logout(request): return HttpResponseRedirect(logout_url) -def terms_of_use_update(request): +def terms_of_use_accept(request): if not request.user.is_authenticated: return redirect("account_login") @@ -121,19 +121,16 @@ def terms_of_use_update(request): # Use the form to handle both update and delete actions form = UpdateConsentForm(request.POST, user=request.user) if form.is_valid(): - consent_status = form.save() - request.session[CONSENT_SESSION_KEY] = consent_status - # Update the session to reflect the new consent status + # update the session to reflect the new consent status + request.session[CONSENT_SESSION_KEY] = form.save() return redirect("home") elif request.method == "GET": - # Render the consent update form - form = UpdateConsentForm(user=request.user) has_consented = ConsentFieldValue.objects.filter(user=request.user).exists() return render( request, - "account/terms_of_use_update_form.html", - {"form": form, "has_consented": has_consented}, + "account/terms_of_use_accept_form.html", + {"has_consented": has_consented}, ) return HttpResponseNotAllowed(["GET", "POST"]) From 22b63d480be9033575b461a98224fe87ce334167 Mon Sep 17 00:00:00 2001 From: David Wallace Date: Mon, 3 Feb 2025 19:12:30 +0100 Subject: [PATCH 07/17] feat(accounts,ToU): add version date check, refactor and fix tests Signed-off-by: David Wallace --- rdmo/accounts/adapter.py | 4 +- rdmo/accounts/forms.py | 26 ++-- rdmo/accounts/middleware.py | 42 ++----- rdmo/accounts/models.py | 64 ++++++++++ rdmo/accounts/signals.py | 13 -- .../account/terms_of_use_accept_form.html | 8 ++ rdmo/accounts/tests/test_views.py | 116 ++++++++++-------- rdmo/accounts/utils.py | 17 +-- rdmo/accounts/views.py | 18 ++- 9 files changed, 174 insertions(+), 134 deletions(-) delete mode 100644 rdmo/accounts/signals.py diff --git a/rdmo/accounts/adapter.py b/rdmo/accounts/adapter.py index 1d756001e5..772295f731 100644 --- a/rdmo/accounts/adapter.py +++ b/rdmo/accounts/adapter.py @@ -68,5 +68,5 @@ def signup(self, request, user): # store the consent field if settings.ACCOUNT_TERMS_OF_USE: - consent = ConsentFieldValue(user=user, consent=self.cleaned_data['consent']) - consent.save() + if self.cleaned_data['consent']: + ConsentFieldValue.create_consent(user=user, session=request.session) diff --git a/rdmo/accounts/forms.py b/rdmo/accounts/forms.py index b48ab53bed..f3f70475bb 100644 --- a/rdmo/accounts/forms.py +++ b/rdmo/accounts/forms.py @@ -89,7 +89,7 @@ def __init__(self, *args, **kwargs): consent.label = _("I confirm that I want my profile to be completely removed. This can not be undone!") -class UpdateConsentForm(forms.Form): +class AcceptConsentForm(forms.Form): consent = forms.BooleanField( label="I agree to the terms of use", @@ -103,20 +103,10 @@ def __init__(self, *args, user=None, **kwargs): if not self.user: raise ValueError("A user instance is required to initialize the form.") - # pre-fill the 'consent' field based on the user's existing consent - # Ensure the user cannot uncheck the consent - has_consented = ConsentFieldValue.objects.filter(user=self.user).exists() - if has_consented: - self.fields["consent"].disabled = True # Disable field after acceptance - self.fields["consent"].initial = True - else: - self.fields["consent"].initial = False - - def save(self) -> bool: - if not ConsentFieldValue.objects.filter(user=self.user).exists(): - if self.cleaned_data.get("consent"): - ConsentFieldValue.objects.create(user=self.user, consent=True) - # session consent is updated in the view - return True # consent accepted - - return False # No changes made + def save(self, session) -> bool: + if self.cleaned_data['consent']: + success = ConsentFieldValue.create_consent(user=self.user, session=session) + if not success: + self.add_error(None, "Consent could not be saved. Please try again.") # Add non-field error + return success + return False diff --git a/rdmo/accounts/middleware.py b/rdmo/accounts/middleware.py index 5a2ebd22d1..b2a90d0a08 100644 --- a/rdmo/accounts/middleware.py +++ b/rdmo/accounts/middleware.py @@ -5,8 +5,9 @@ from django.http import HttpResponseRedirect from django.urls import reverse -from .utils import user_has_accepted_terms +from .models import ConsentFieldValue +# these exclude url settings are optional TERMS_EXCLUDE_URL_PREFIX_LIST = getattr( settings, "TERMS_EXCLUDE_URL_PREFIX_LIST", @@ -27,41 +28,22 @@ def __init__(self, get_response): self.get_response = get_response def __call__(self, request): - # Skip processing if ACCOUNT_TERMS_OF_USE is disabled - if not getattr(settings, "ACCOUNT_TERMS_OF_USE", False): - return self.get_response(request) - - # check if the current path is protected - accept_terms_path = reverse("terms_of_use_accept") if ( - request.user.is_authenticated - and not request.path.startswith(accept_terms_path) + settings.ACCOUNT_TERMS_OF_USE # Terms enforcement enabled + and request.user.is_authenticated + and request.path != reverse("terms_of_use_accept") and self.is_path_protected(request.path) - and not user_has_accepted_terms(request.user, request.session) + and not ConsentFieldValue.has_accepted_terms(request.user, request.session) ): - return HttpResponseRedirect(accept_terms_path) + return HttpResponseRedirect(reverse("terms_of_use_accept")) # Proceed with the response for non-protected paths or accepted terms return self.get_response(request) @staticmethod def is_path_protected(path): - """ - Determine if a given path is protected by the middleware. - - Paths are excluded if they match any of the following: - - Start with a prefix in TERMS_EXCLUDE_URL_PREFIX_LIST - - Contain a substring in TERMS_EXCLUDE_URL_CONTAINS_LIST - - Are explicitly listed in TERMS_EXCLUDE_URL_LIST - - Start with the ACCEPT_TERMS_PATH - """ - if any(path.startswith(prefix) for prefix in TERMS_EXCLUDE_URL_PREFIX_LIST): - return False - - if any(substring in path for substring in TERMS_EXCLUDE_URL_CONTAINS_LIST): - return False - - if path in TERMS_EXCLUDE_URL_LIST: - return False - - return True + return not ( + any(path.startswith(prefix) for prefix in TERMS_EXCLUDE_URL_PREFIX_LIST) or + any(substring in path for substring in TERMS_EXCLUDE_URL_CONTAINS_LIST) or + path in TERMS_EXCLUDE_URL_LIST + ) diff --git a/rdmo/accounts/models.py b/rdmo/accounts/models.py index 8b2ebfee9d..1fb91115c0 100644 --- a/rdmo/accounts/models.py +++ b/rdmo/accounts/models.py @@ -1,13 +1,19 @@ +from datetime import datetime + from django.conf import settings from django.contrib.sites.models import Site from django.db import models from django.db.models.signals import post_save from django.dispatch import receiver +from django.utils.dateparse import parse_date +from django.utils.formats import get_format from django.utils.translation import gettext_lazy as _ from rdmo.core.models import Model as RDMOTimeStampedModel from rdmo.core.models import TranslationMixin +CONSENT_SESSION_KEY = "user_has_consented" + class AdditionalField(models.Model, TranslationMixin): @@ -124,6 +130,64 @@ class Meta: def __str__(self): return self.user.username + @classmethod + def create_consent(cls, user, session=None) -> bool: + obj, _created = cls.objects.update_or_create(user=user, defaults={"consent": True}) + + # Validate consent before storing in session + if obj.is_consent_valid(): + if session: + session[CONSENT_SESSION_KEY] = True + return True + + obj.delete() # Remove when consent is outdated + return False + + + @classmethod + def has_accepted_terms(cls, user, session) -> bool: + if not settings.ACCOUNT_TERMS_OF_USE: + return True # If terms are disabled, assume accepted. + + # Check session cache first + if CONSENT_SESSION_KEY in session: + return session[CONSENT_SESSION_KEY] + + # Query the database if not cached + consent = cls.objects.filter(user=user).first() + has_valid_consent = bool(consent and consent.is_consent_valid()) + + session[CONSENT_SESSION_KEY] = has_valid_consent # Cache result + return has_valid_consent + + def is_consent_valid(self) -> bool: + # optionally enable terms to be outdated + terms_version_date = getattr(settings, 'TERMS_VERSION_DATE', None) + + if terms_version_date is None: + return True + + # First, try standard ISO format (YYYY-MM-DD) + latest_terms_version_date = parse_date(terms_version_date) + + # If ISO parsing fails, try localized formats + if not latest_terms_version_date: + for fmt in get_format('DATE_INPUT_FORMATS'): + try: + latest_terms_version_date = datetime.strptime(terms_version_date, fmt).date() + break # Stop if parsing succeeds + except ValueError: + continue # Try the next format + + # If still not parsed, raise an error + if not latest_terms_version_date: + raise ValueError( + f"Invalid date format for TERMS_VERSION_DATE: {terms_version_date}. Valid formats {get_format('DATE_INPUT_FORMATS')}" # noqa: E501 + ) + + # Compare only dates (ignores time) + return self.updated and (self.updated.date() >= latest_terms_version_date) + class Role(models.Model): diff --git a/rdmo/accounts/signals.py b/rdmo/accounts/signals.py deleted file mode 100644 index b08e28d09d..0000000000 --- a/rdmo/accounts/signals.py +++ /dev/null @@ -1,13 +0,0 @@ -from django.contrib.auth.signals import user_logged_in -from django.dispatch import receiver -from django.http import HttpResponseRedirect -from django.urls import reverse - -from .utils import user_has_accepted_terms - - -@receiver(user_logged_in) -def check_user_consent(sender, request, user, **kwargs): - # check consent and store it in the session - if not user_has_accepted_terms(user, request.session): - return HttpResponseRedirect(reverse("terms_of_use_update")) diff --git a/rdmo/accounts/templates/account/terms_of_use_accept_form.html b/rdmo/accounts/templates/account/terms_of_use_accept_form.html index ede86a91e6..6354e0e206 100644 --- a/rdmo/accounts/templates/account/terms_of_use_accept_form.html +++ b/rdmo/accounts/templates/account/terms_of_use_accept_form.html @@ -33,6 +33,14 @@

{% trans 'Terms of use' %}

{% trans "You have accepted the terms of use." %}

{% endif %} + + {% if form.non_field_errors %} +
    + {% for error in form.non_field_errors %} +
  • {{ error }}
  • + {% endfor %} +
+ {% endif %} {% endblock %} diff --git a/rdmo/accounts/tests/test_views.py b/rdmo/accounts/tests/test_views.py index cf4083f042..039d324955 100644 --- a/rdmo/accounts/tests/test_views.py +++ b/rdmo/accounts/tests/test_views.py @@ -1,4 +1,5 @@ import re +from datetime import datetime, timedelta import pytest @@ -10,9 +11,8 @@ from pytest_django.asserts import assertContains, assertNotContains, assertRedirects, assertTemplateUsed, assertURLEqual -from rdmo.accounts.models import ConsentFieldValue +from rdmo.accounts.models import CONSENT_SESSION_KEY, ConsentFieldValue from rdmo.accounts.tests.utils import reload_urls -from rdmo.accounts.utils import CONSENT_SESSION_KEY users = ( ('editor', 'editor'), @@ -799,17 +799,10 @@ def test_shibboleth_logout_username_pattern(db, client, settings, shibboleth, us assertURLEqual(response.url, reverse('account_logout')) -@pytest.mark.parametrize('post_consent', [True, False]) @pytest.mark.parametrize('username,password', users) -def test_terms_of_use_middleware_redirect_and_update( - db, client, settings, username, password, post_consent +def test_terms_of_use_middleware_redirect_and_accept( + db, client, settings, username, password ): - """ - Test the behavior of TermsAndConditionsRedirectMiddleware and terms_of_use_update view: - - When the middleware is enabled, users are redirected to the terms update page if not consented. - - Valid POST saves consent to the session and database. - - Invalid POST does not save consent to the session or database. - """ # Arrange settings.ACCOUNT_TERMS_OF_USE = True reload_urls('accounts') # needs to reload before the middleware @@ -826,56 +819,81 @@ def test_terms_of_use_middleware_redirect_and_update( client.login(username=username, password=password) response = client.get(reverse('projects')) - # Assert - Middleware should redirect to terms_of_use_update + # Assert - Middleware should redirect to terms_of_use_accept if password is not None: - assert response.status_code == 302 - assert response.url == reverse('terms_of_use_update') + assertRedirects(response, reverse('terms_of_use_accept')) # Session should not yet have consent assert not client.session.get(CONSENT_SESSION_KEY, False) else: # Anonymous user is redirected to login - assert response.status_code == 302 - assert response.url == reverse('account_login') + '?next=' + reverse('projects') + assertRedirects(response, reverse('account_login') + '?next=' + reverse('projects')) return # Exit test for anonymous users - # Act - Make a POST request to terms_of_use_update - terms_update_url = reverse('terms_of_use_update') - data = { - 'consent': post_consent, # Set based on parameterization - } - response = client.post(terms_update_url, data) + # Act - Make a POST request to terms_of_use_accept + response = client.post(reverse('terms_of_use_accept'), {'consent': True}, follow=True) + assertRedirects(response, reverse('projects')) - assert response.status_code == 302 - assert response.url == reverse('home') + # Assert POST behavior, ToU accepted + # Consent should be stored in the session and database + assert client.session[CONSENT_SESSION_KEY] is True + assert ConsentFieldValue.objects.filter(user=user).exists() - # Assert POST behavior - if post_consent: # accepted you - # Consent should be stored in the session and database - assert client.session[CONSENT_SESSION_KEY] is True - assert ConsentFieldValue.objects.filter(user=user).exists() + response = client.get(reverse('projects')) + assert response.status_code == 200 - response = client.get(reverse('projects')) - assert response.status_code == 200 - # also test the revoke consent - response = client.post(terms_update_url, {'delete': ''}) - assert response.status_code == 302 - assert response.url == reverse('home') - assert client.session.get(CONSENT_SESSION_KEY, False) is False - assert not ConsentFieldValue.objects.filter(user=user).exists() +def test_terms_of_use_middleware_invalidate_terms_version( + db, client, settings +): + # Arrange constants, settings and user + past_datetime = (datetime.now() - timedelta(days=10)).strftime(format="%Y-%m-%d") + future_datetime = (datetime.now() + timedelta(days=10)).strftime(format="%Y-%m-%d") - response = client.get(reverse('projects')) - assert response.status_code == 302 - assert response.url == reverse('terms_of_use_update') + settings.ACCOUNT_TERMS_OF_USE = True + reload_urls('accounts') # needs to reload before the middleware + settings.MIDDLEWARE += [ + "rdmo.accounts.middleware.TermsAndConditionsRedirectMiddleware" + ] + terms_accept_url = reverse('terms_of_use_accept') + # Arrange user object + username = password = 'user' + user = get_user_model().objects.get(username=username) + _consent = ConsentFieldValue.objects.create(user=user, consent=True) + + # Assert - Access the home page, user has a valid consent + # settings.TERMS_VERSION_DATE is not set + client.login(username=username, password=password) + response = client.get(reverse('projects')) + assert response.status_code == 200 + client.logout() - else: # did not accept you - # Invalid POST - # assertTemplateUsed(response, "account/terms_of_use_update_form.html") - # Consent should not be stored in the session or database - assert client.session.get(CONSENT_SESSION_KEY, False) is False - assert not ConsentFieldValue.objects.filter(user=user).exists() + # Act - change the version date setting to a distant future + settings.TERMS_VERSION_DATE = future_datetime + # Arrange - new login + client.login(username=username, password=password) + response = client.get(reverse('projects')) - response = client.get(reverse('projects')) - assert response.status_code == 302 - assert response.url == reverse('terms_of_use_update') + # Assert - consent is now invalid and should redirect to terms_of_use_accept + assertRedirects(response, terms_accept_url) + + # Act - Try to make a POST request to terms_of_use_accept + response = client.post(terms_accept_url, {'consent': True}) + # Assert - consent was not saved because version date is in the future + assert not ConsentFieldValue.objects.filter(user=user).exists() + assertContains(response, 'could not be saved') + client.logout() + + # Act - change the version date setting to a past datetime + settings.TERMS_VERSION_DATE = past_datetime + # Arrange - new login without consent + client.login(username=username, password=password) + response = client.get(reverse('projects')) + assertRedirects(response, terms_accept_url) + + # Act - post the consent + response = client.post(terms_accept_url, {'consent': True}, follow=True) + # Assert - the consent should now be updated since the version date is valid + assert ConsentFieldValue.objects.filter(user=user).exists() + assert client.session[CONSENT_SESSION_KEY] is True + assertRedirects(response, reverse('projects')) diff --git a/rdmo/accounts/utils.py b/rdmo/accounts/utils.py index 43e07ecf69..5bbb360496 100644 --- a/rdmo/accounts/utils.py +++ b/rdmo/accounts/utils.py @@ -6,12 +6,11 @@ from django.contrib.contenttypes.models import ContentType from django.core.exceptions import ObjectDoesNotExist -from .models import ConsentFieldValue, Role +from .models import Role from .settings import GROUPS log = logging.getLogger(__name__) -CONSENT_SESSION_KEY = "user_has_consented" def get_full_name(user): if user.first_name and user.last_name: @@ -94,17 +93,3 @@ def get_user_from_db_or_none(username: str, email: str): except ObjectDoesNotExist: log.error('Retrieval of user "%s" with email "%s" failed, user does not exist', username, email) return None - - -def user_has_accepted_terms(user, session) -> bool: - if not settings.ACCOUNT_TERMS_OF_USE: - return True # If terms are not enabled, consider them accepted. - - # check the session for cached consent status - if CONSENT_SESSION_KEY in session: - return session[CONSENT_SESSION_KEY] - - has_consented = ConsentFieldValue.objects.filter(user=user).exists() - session[CONSENT_SESSION_KEY] = has_consented # cache the result in the session - - return has_consented diff --git a/rdmo/accounts/views.py b/rdmo/accounts/views.py index 80d6dac3f8..e785155826 100644 --- a/rdmo/accounts/views.py +++ b/rdmo/accounts/views.py @@ -12,9 +12,9 @@ from rdmo.core.utils import get_next, get_referer_path_info -from .forms import ProfileForm, RemoveForm, UpdateConsentForm +from .forms import AcceptConsentForm, ProfileForm, RemoveForm from .models import ConsentFieldValue -from .utils import CONSENT_SESSION_KEY, delete_user +from .utils import delete_user log = logging.getLogger(__name__) @@ -119,11 +119,17 @@ def terms_of_use_accept(request): if request.method == "POST": # Use the form to handle both update and delete actions - form = UpdateConsentForm(request.POST, user=request.user) + form = AcceptConsentForm(request.POST, user=request.user) if form.is_valid(): - # update the session to reflect the new consent status - request.session[CONSENT_SESSION_KEY] = form.save() - return redirect("home") + consent_saved = form.save(request.session) # saves the consent and sets the session key + if consent_saved: + return redirect("home") + + # If consent was not saved, re-render the form with an error + return render(request, + "account/terms_of_use_accept_form.html", + {"form": form}, + ) elif request.method == "GET": has_consented = ConsentFieldValue.objects.filter(user=request.user).exists() From fa8b7d150e2ead9960983723757f0a76e27b25eb Mon Sep 17 00:00:00 2001 From: David Wallace Date: Fri, 7 Feb 2025 16:29:34 +0100 Subject: [PATCH 08/17] refactor(core,accounts): move ToU settings and date parser to core and add checks Signed-off-by: David Wallace --- rdmo/accounts/middleware.py | 22 ++++---------- rdmo/accounts/models.py | 49 ++++++++++--------------------- rdmo/accounts/tests/test_views.py | 10 +++---- rdmo/core/apps.py | 10 +++++++ rdmo/core/checks.py | 34 +++++++++++++++++++++ rdmo/core/settings.py | 7 +++++ rdmo/core/utils.py | 27 +++++++++++++++++ 7 files changed, 104 insertions(+), 55 deletions(-) create mode 100644 rdmo/core/apps.py create mode 100644 rdmo/core/checks.py diff --git a/rdmo/accounts/middleware.py b/rdmo/accounts/middleware.py index b2a90d0a08..c806d76c03 100644 --- a/rdmo/accounts/middleware.py +++ b/rdmo/accounts/middleware.py @@ -7,19 +7,6 @@ from .models import ConsentFieldValue -# these exclude url settings are optional -TERMS_EXCLUDE_URL_PREFIX_LIST = getattr( - settings, - "TERMS_EXCLUDE_URL_PREFIX_LIST", - ["/admin", "/i18n", "/static", "/account"], -) -TERMS_EXCLUDE_URL_CONTAINS_LIST = getattr(settings, "TERMS_EXCLUDE_URL_CONTAINS_LIST", []) -TERMS_EXCLUDE_URL_LIST = getattr( - settings, - "TERMS_EXCLUDE_URL_LIST", - ["/", settings.LOGOUT_URL], -) - class TermsAndConditionsRedirectMiddleware: """Middleware to ensure terms and conditions have been accepted.""" @@ -31,7 +18,6 @@ def __call__(self, request): if ( settings.ACCOUNT_TERMS_OF_USE # Terms enforcement enabled and request.user.is_authenticated - and request.path != reverse("terms_of_use_accept") and self.is_path_protected(request.path) and not ConsentFieldValue.has_accepted_terms(request.user, request.session) ): @@ -42,8 +28,10 @@ def __call__(self, request): @staticmethod def is_path_protected(path): + # all paths should be protected, except what is excluded here return not ( - any(path.startswith(prefix) for prefix in TERMS_EXCLUDE_URL_PREFIX_LIST) or - any(substring in path for substring in TERMS_EXCLUDE_URL_CONTAINS_LIST) or - path in TERMS_EXCLUDE_URL_LIST + path == reverse("terms_of_use_accept") or + any(path.startswith(prefix) for prefix in settings.ACCOUNT_TERMS_OF_USE_EXCLUDE_URL_PREFIXES) or + any(substring in path for substring in settings.ACCOUNT_TERMS_OF_USE_EXCLUDE_URL_CONTAINS) or + path in settings.ACCOUNT_TERMS_OF_USE_EXCLUDE_URLS ) diff --git a/rdmo/accounts/models.py b/rdmo/accounts/models.py index 1fb91115c0..634a809d02 100644 --- a/rdmo/accounts/models.py +++ b/rdmo/accounts/models.py @@ -1,16 +1,13 @@ -from datetime import datetime - from django.conf import settings from django.contrib.sites.models import Site from django.db import models from django.db.models.signals import post_save from django.dispatch import receiver -from django.utils.dateparse import parse_date -from django.utils.formats import get_format from django.utils.translation import gettext_lazy as _ from rdmo.core.models import Model as RDMOTimeStampedModel from rdmo.core.models import TranslationMixin +from rdmo.core.utils import parse_date_from_string CONSENT_SESSION_KEY = "user_has_consented" @@ -135,7 +132,13 @@ def create_consent(cls, user, session=None) -> bool: obj, _created = cls.objects.update_or_create(user=user, defaults={"consent": True}) # Validate consent before storing in session - if obj.is_consent_valid(): + has_valid_consent = ( + obj.is_consent_valid() + if settings.ACCOUNT_TERMS_OF_USE_DATE is not None + else True # If terms update date is not enforced, any consent is valid + ) + + if has_valid_consent: if session: session[CONSENT_SESSION_KEY] = True return True @@ -143,7 +146,6 @@ def create_consent(cls, user, session=None) -> bool: obj.delete() # Remove when consent is outdated return False - @classmethod def has_accepted_terms(cls, user, session) -> bool: if not settings.ACCOUNT_TERMS_OF_USE: @@ -153,38 +155,19 @@ def has_accepted_terms(cls, user, session) -> bool: if CONSENT_SESSION_KEY in session: return session[CONSENT_SESSION_KEY] - # Query the database if not cached - consent = cls.objects.filter(user=user).first() - has_valid_consent = bool(consent and consent.is_consent_valid()) + consent = cls.objects.filter(user=user).only("updated").first() + has_valid_consent = ( + consent.is_consent_valid() + if consent and settings.ACCOUNT_TERMS_OF_USE_DATE is not None + else bool(consent) + ) session[CONSENT_SESSION_KEY] = has_valid_consent # Cache result return has_valid_consent def is_consent_valid(self) -> bool: - # optionally enable terms to be outdated - terms_version_date = getattr(settings, 'TERMS_VERSION_DATE', None) - - if terms_version_date is None: - return True - - # First, try standard ISO format (YYYY-MM-DD) - latest_terms_version_date = parse_date(terms_version_date) - - # If ISO parsing fails, try localized formats - if not latest_terms_version_date: - for fmt in get_format('DATE_INPUT_FORMATS'): - try: - latest_terms_version_date = datetime.strptime(terms_version_date, fmt).date() - break # Stop if parsing succeeds - except ValueError: - continue # Try the next format - - # If still not parsed, raise an error - if not latest_terms_version_date: - raise ValueError( - f"Invalid date format for TERMS_VERSION_DATE: {terms_version_date}. Valid formats {get_format('DATE_INPUT_FORMATS')}" # noqa: E501 - ) - + # optionally check if the terms are outdated + latest_terms_version_date = parse_date_from_string(settings.ACCOUNT_TERMS_OF_USE_DATE) # Compare only dates (ignores time) return self.updated and (self.updated.date() >= latest_terms_version_date) diff --git a/rdmo/accounts/tests/test_views.py b/rdmo/accounts/tests/test_views.py index 039d324955..8f5f986a7c 100644 --- a/rdmo/accounts/tests/test_views.py +++ b/rdmo/accounts/tests/test_views.py @@ -807,7 +807,7 @@ def test_terms_of_use_middleware_redirect_and_accept( settings.ACCOUNT_TERMS_OF_USE = True reload_urls('accounts') # needs to reload before the middleware settings.MIDDLEWARE += [ - "rdmo.accounts.middleware.TermsAndConditionsRedirectMiddleware" + settings.ACCOUNT_TERMS_OF_USE_MIDDLEWARE ] # Ensure there are no existing consent entries for the user @@ -853,7 +853,7 @@ def test_terms_of_use_middleware_invalidate_terms_version( settings.ACCOUNT_TERMS_OF_USE = True reload_urls('accounts') # needs to reload before the middleware settings.MIDDLEWARE += [ - "rdmo.accounts.middleware.TermsAndConditionsRedirectMiddleware" + settings.ACCOUNT_TERMS_OF_USE_MIDDLEWARE ] terms_accept_url = reverse('terms_of_use_accept') # Arrange user object @@ -862,14 +862,14 @@ def test_terms_of_use_middleware_invalidate_terms_version( _consent = ConsentFieldValue.objects.create(user=user, consent=True) # Assert - Access the home page, user has a valid consent - # settings.TERMS_VERSION_DATE is not set + # settings.ACCOUNT_TERMS_OF_USE_DATE is not set client.login(username=username, password=password) response = client.get(reverse('projects')) assert response.status_code == 200 client.logout() # Act - change the version date setting to a distant future - settings.TERMS_VERSION_DATE = future_datetime + settings.ACCOUNT_TERMS_OF_USE_DATE = future_datetime # Arrange - new login client.login(username=username, password=password) response = client.get(reverse('projects')) @@ -885,7 +885,7 @@ def test_terms_of_use_middleware_invalidate_terms_version( client.logout() # Act - change the version date setting to a past datetime - settings.TERMS_VERSION_DATE = past_datetime + settings.ACCOUNT_TERMS_OF_USE_DATE = past_datetime # Arrange - new login without consent client.login(username=username, password=password) response = client.get(reverse('projects')) diff --git a/rdmo/core/apps.py b/rdmo/core/apps.py new file mode 100644 index 0000000000..cf8d9c956f --- /dev/null +++ b/rdmo/core/apps.py @@ -0,0 +1,10 @@ +from django.apps import AppConfig +from django.utils.translation import gettext_lazy as _ + + +class AccountsConfig(AppConfig): + name = 'rdmo.core' + verbose_name = _('Core') + + def ready(self): + pass diff --git a/rdmo/core/checks.py b/rdmo/core/checks.py new file mode 100644 index 0000000000..fef36bc2b5 --- /dev/null +++ b/rdmo/core/checks.py @@ -0,0 +1,34 @@ +from django.conf import settings +from django.core.checks import Error, register + + +@register() +def check_account_terms_of_use_date_setting(app_configs, **kwargs): + errors = [] + + if settings.ACCOUNT_TERMS_OF_USE: + if settings.ACCOUNT_TERMS_OF_USE_MIDDLEWARE not in settings.MIDDLEWARE: + errors.append( + Error( + "When ACCOUNT_TERMS_OF_USE is enabled, " + "ACCOUNT_TERMS_OF_USE_MIDDLEWARE needs to be added to the middlewares.", + hint=f"add '{settings.ACCOUNT_TERMS_OF_USE_MIDDLEWARE}' to MIDDLEWARE", + id="core.E001", + ) + ) + + if settings.ACCOUNT_TERMS_OF_USE_DATE is not None: + # Ensure that ACCOUNT_TERMS_OF_USE_DATE is a valid date string + from .utils import parse_date_from_string + try: + parse_date_from_string(settings.ACCOUNT_TERMS_OF_USE_DATE) + except ValueError as exc: + errors.append( + Error( + f"ACCOUNT_TERMS_OF_USE_DATE = {settings.ACCOUNT_TERMS_OF_USE_DATE} is not a valid date string.", + hint=f"{exc}", + id="core.E002", + ) + ) + + return errors diff --git a/rdmo/core/settings.py b/rdmo/core/settings.py index 63d3b17459..35b975d9c1 100644 --- a/rdmo/core/settings.py +++ b/rdmo/core/settings.py @@ -99,7 +99,14 @@ ACCOUNT = False ACCOUNT_SIGNUP = False ACCOUNT_GROUPS = [] + ACCOUNT_TERMS_OF_USE = False +ACCOUNT_TERMS_OF_USE_MIDDLEWARE = 'rdmo.accounts.middleware.TermsAndConditionsRedirectMiddleware' +ACCOUNT_TERMS_OF_USE_DATE = None # None or a valid date string +ACCOUNT_TERMS_OF_USE_EXCLUDE_URL_PREFIXES = ("/admin", "/i18n", "/static", "/account") +ACCOUNT_TERMS_OF_USE_EXCLUDE_URLS = ("/",) # is LOGOUT_URL needed here? +ACCOUNT_TERMS_OF_USE_EXCLUDE_URL_CONTAINS = [] + ACCOUNT_ADAPTER = 'rdmo.accounts.adapter.AccountAdapter' ACCOUNT_FORMS = { 'login': 'rdmo.accounts.adapter.LoginForm', diff --git a/rdmo/core/utils.py b/rdmo/core/utils.py index 0813b05c74..769f560b8c 100644 --- a/rdmo/core/utils.py +++ b/rdmo/core/utils.py @@ -3,6 +3,7 @@ import logging import os import re +from datetime import datetime from pathlib import Path from tempfile import mkstemp from urllib.parse import urlparse @@ -11,7 +12,9 @@ from django.conf import settings from django.http import Http404, HttpResponse, HttpResponseBadRequest from django.template.loader import get_template +from django.utils.dateparse import parse_date from django.utils.encoding import force_str +from django.utils.formats import get_format from django.utils.translation import gettext_lazy as _ import pypandoc @@ -422,3 +425,27 @@ def save_metadata(metadata): def remove_double_newlines(string): return re.sub(r'[\n]{2,}', '\n\n', string) + + +def parse_date_from_string(date: str) -> datetime.date: + try: + # First, try standard ISO format (YYYY-MM-DD) + parsed_date = parse_date(settings.ACCOUNT_TERMS_OF_USE_DATE) + except ValueError as exc: + raise exc from exc + + # If ISO parsing fails, try localized DATE_INPUT_FORMATS formats + if parsed_date is None: + for fmt in get_format('DATE_INPUT_FORMATS'): + try: + parsed_date = datetime.strptime(date, fmt).date() + break # Stop if parsing succeeds + except ValueError: + continue # Try the next format + + # If still not parsed, raise an error + if not parsed_date: + raise ValueError( + f"Invalid date format for: {date}. Valid formats {get_format('DATE_INPUT_FORMATS')}" + ) + return parsed_date From c5100273e88a5439426ef8ac5261a2d435888a76 Mon Sep 17 00:00:00 2001 From: David Wallace Date: Mon, 10 Feb 2025 18:33:47 +0100 Subject: [PATCH 09/17] feat(sociallaccount): add ToU to social signup form Signed-off-by: David Wallace --- rdmo/accounts/adapter.py | 16 ---- rdmo/accounts/forms.py | 2 +- rdmo/accounts/socialaccount.py | 45 ++++++++++ rdmo/accounts/tests/helpers.py | 140 ++++++++++++++++++++++++++++++ rdmo/accounts/tests/test_views.py | 96 +++++++++++++++++--- rdmo/accounts/tests/utils.py | 33 ------- rdmo/core/settings.py | 5 +- 7 files changed, 275 insertions(+), 62 deletions(-) create mode 100644 rdmo/accounts/socialaccount.py create mode 100644 rdmo/accounts/tests/helpers.py delete mode 100644 rdmo/accounts/tests/utils.py diff --git a/rdmo/accounts/adapter.py b/rdmo/accounts/adapter.py index 772295f731..b803635e64 100644 --- a/rdmo/accounts/adapter.py +++ b/rdmo/accounts/adapter.py @@ -5,7 +5,6 @@ from allauth.account.adapter import DefaultAccountAdapter from allauth.account.forms import LoginForm as AllauthLoginForm from allauth.account.forms import SignupForm as AllauthSignupForm -from allauth.socialaccount.adapter import DefaultSocialAccountAdapter from .forms import ProfileForm from .models import ConsentFieldValue @@ -25,21 +24,6 @@ def save_user(self, request, user, form, commit=True): return user -class SocialAccountAdapter(DefaultSocialAccountAdapter): - - def is_open_for_signup(self, request, sociallogin): - return settings.SOCIALACCOUNT_SIGNUP - - def save_user(self, request, sociallogin, form=None): - user = super().save_user(request, sociallogin, form) - - if settings.SOCIALACCOUNT_GROUPS: - provider = str(sociallogin.account.provider) - groups = Group.objects.filter(name__in=settings.SOCIALACCOUNT_GROUPS.get(provider, [])) - user.groups.set(groups) - - return user - class LoginForm(AllauthLoginForm): diff --git a/rdmo/accounts/forms.py b/rdmo/accounts/forms.py index f3f70475bb..e71127c2ab 100644 --- a/rdmo/accounts/forms.py +++ b/rdmo/accounts/forms.py @@ -107,6 +107,6 @@ def save(self, session) -> bool: if self.cleaned_data['consent']: success = ConsentFieldValue.create_consent(user=self.user, session=session) if not success: - self.add_error(None, "Consent could not be saved. Please try again.") # Add non-field error + self.add_error(None, _("Consent could not be saved. Please try again.")) # Add non-field error return success return False diff --git a/rdmo/accounts/socialaccount.py b/rdmo/accounts/socialaccount.py new file mode 100644 index 0000000000..e9eafb6d88 --- /dev/null +++ b/rdmo/accounts/socialaccount.py @@ -0,0 +1,45 @@ +from django.conf import settings +from django.contrib.auth.models import Group +from django.forms import BooleanField + +from allauth.socialaccount.adapter import DefaultSocialAccountAdapter +from allauth.socialaccount.forms import SignupForm as AllauthSocialSignupForm + +from rdmo.accounts.forms import ProfileForm +from rdmo.accounts.models import ConsentFieldValue + + +class SocialAccountAdapter(DefaultSocialAccountAdapter): + + def is_open_for_signup(self, request, sociallogin): + return settings.SOCIALACCOUNT_SIGNUP + + def save_user(self, request, sociallogin, form=None): + user = super().save_user(request, sociallogin, form) + + if settings.SOCIALACCOUNT_GROUPS: + provider = str(sociallogin.account.provider) + groups = Group.objects.filter(name__in=settings.SOCIALACCOUNT_GROUPS.get(provider, [])) + user.groups.set(groups) + + return user + + +class SocialSignupForm(AllauthSocialSignupForm, ProfileForm): + + use_required_attribute = False + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + + # add a consent field, the label is added in the template + if settings.ACCOUNT_TERMS_OF_USE: + self.fields['consent'] = BooleanField(required=True) + + def signup(self, request, user): + self._save_additional_values(user) + + # store the consent field + if settings.ACCOUNT_TERMS_OF_USE: + if self.cleaned_data['consent']: + ConsentFieldValue.create_consent(user=user, session=request.session) diff --git a/rdmo/accounts/tests/helpers.py b/rdmo/accounts/tests/helpers.py new file mode 100644 index 0000000000..75f2d5c49e --- /dev/null +++ b/rdmo/accounts/tests/helpers.py @@ -0,0 +1,140 @@ +import sys +from importlib import import_module, reload +from typing import Optional + +import pytest + +from django.core.management import call_command +from django.db import connection +from django.urls import clear_url_caches, get_resolver, reverse + + +def reload_urlconf(urlconf=None, settings=None): + clear_url_caches() + + if urlconf is None and settings is None: + from django.conf import settings + urlconf = settings.ROOT_URLCONF + elif urlconf is None and settings is not None: + # take the settings during pytest run + urlconf = settings.ROOT_URLCONF + + if urlconf in sys.modules: + reload(sys.modules[urlconf]) + else: + import_module(urlconf) + + +def reload_urls(app_name: Optional[str] = None, + settings=None, + other_apps: list[str] | None = None ) -> None: + # reload the urlconf of the app + if app_name is not None: + reload_urlconf(urlconf=f'rdmo.{app_name}.urls', settings=settings) + + # reload the core urlconf + reload_urlconf(urlconf='rdmo.core.urls', settings=settings) + + # reload the testcase settings urlconf + reload_urlconf(settings=settings) + + if other_apps: + for _app in other_apps: + reload_urlconf(urlconf=f"{_app}.urls", settings=settings) + + get_resolver()._populate() + + +@pytest.fixture(autouse=False) +def enable_terms_of_use(settings): # noqa: PT004 + settings.ACCOUNT_TERMS_OF_USE = True + settings.MIDDLEWARE += [ + settings.ACCOUNT_TERMS_OF_USE_MIDDLEWARE + ] + reload_urls('accounts') # Reloads URLs to apply new settings + + yield + + # revert settings to initial state + settings.ACCOUNT_TERMS_OF_USE = False + settings.MIDDLEWARE.remove(settings.ACCOUNT_TERMS_OF_USE_MIDDLEWARE) + # 🔹 Reload URLs to reflect the changes + reload_urls("accounts") + + +@pytest.fixture(autouse=False) +def enable_socialaccount(db, client, settings, django_db_setup, django_db_blocker, django_db_use_migrations): # noqa: PT004 + # Arrange: this fixture enable and initializes the allauth.sociallaccounts + # use this fixture with the decorator @pytest.mark.django_db(transaction=True) on your tests + + # Detect if --no-migrations is being used + if not django_db_use_migrations and connection.vendor == 'sqlite': + pytest.xfail("This test is expected to fail when --no-migrations is enabled.") + + settings.SOCIALACCOUNT = True + settings.SOCIALACCOUNT_SIGNUP = True + # 🔹 Add required apps + added_apps = [ + "allauth.socialaccount", + "allauth.socialaccount.providers.dummy", + ] + new_apps = [app for app in added_apps if app not in settings.INSTALLED_APPS] + + if new_apps: + settings.INSTALLED_APPS += new_apps + + # 🔹 Force Django to reload apps + import django + from django.apps import apps + apps.clear_cache() + django.setup() + + with django_db_blocker.unblock(): + call_command("migrate") + + # 🔹 Reload URL patterns manually + clear_url_caches() + reload_urls("accounts",settings=settings, other_apps=['allauth']) + # 🔹 Verify that the route now exists + assert reverse("dummy_login") # Ensure the route exists + + # Check database backend + db_backend = connection.vendor # 'sqlite', 'mysql', or 'postgresql' + + if db_backend == "sqlite": + with connection.cursor() as cursor: + cursor.execute("PRAGMA foreign_keys=OFF;") # Disable FKs in SQLite + + from allauth.socialaccount.models import SocialAccount + assert SocialAccount.objects.exists() is not None # Ensure table exists before using it + + yield # Run test + + # 🔹 Cleanup: Remove added apps and reset settings + settings.INSTALLED_APPS.remove("allauth.socialaccount") + settings.INSTALLED_APPS.remove("allauth.socialaccount.providers.dummy") + + settings.SOCIALACCOUNT = False + settings.SOCIALACCOUNT_SIGNUP = False + + # 🔹 Force Django to reload apps again + import django + from django.apps import apps + apps.clear_cache() + django.setup() + + reload_urls("accounts",settings=settings, other_apps=['allauth']) + + # **Teardown: Apply only for SQLite** + if db_backend == "sqlite": + with django_db_blocker.unblock(): + with connection.cursor() as cursor: + cursor.execute("PRAGMA foreign_keys=OFF;") # Disable FKs before flush + try: + call_command("flush", "--no-input") # Reset DB + finally: + with connection.cursor() as cursor: + cursor.execute("PRAGMA foreign_keys=ON;") # Re-enable FKs + else: + # For MySQL & PostgreSQL, just run normal flush + call_command("flush", "--no-input", allow_cascade=True) diff --git a/rdmo/accounts/tests/test_views.py b/rdmo/accounts/tests/test_views.py index 8f5f986a7c..6fdc3bd5aa 100644 --- a/rdmo/accounts/tests/test_views.py +++ b/rdmo/accounts/tests/test_views.py @@ -8,11 +8,13 @@ from django.db.models import ObjectDoesNotExist from django.urls import reverse from django.urls.exceptions import NoReverseMatch +from django.utils.http import urlencode from pytest_django.asserts import assertContains, assertNotContains, assertRedirects, assertTemplateUsed, assertURLEqual from rdmo.accounts.models import CONSENT_SESSION_KEY, ConsentFieldValue -from rdmo.accounts.tests.utils import reload_urls + +from .helpers import reload_urls users = ( ('editor', 'editor'), @@ -603,10 +605,45 @@ def test_signup_next(db, client, settings, account_signup): else: assert response.status_code == 200 +@pytest.mark.django_db(transaction=True) +def test_social_signup(db, client, settings, enable_socialaccount): + # Arrange socialaccount enabled by fixture enable_socialaccount + # Ensure migrations are applied before the test runs + # Step 1: Initiate Dummy Login + login_url = reverse("dummy_login") + "?" + urlencode({"process": "login"}) + login_response = client.post(login_url) + assert login_response.status_code == 302 # Ensure login form loads correctly + assert reverse("dummy_authenticate") in login_response.url + + # Step 2: POST request to Dummy Authenticate with test user data + user_data = { + "id": 99, + "email": "newuser@example.com", # New user + "username": "new_user", + "first_name": "New", + "last_name": "User", + } + # The authentication state is included in the redirected URL + auth_response = client.post(login_response.url, user_data) + + # Step 3: Ensure we are redirected to the signup form (if new user) + assert auth_response.status_code == 302 + assert auth_response.url.startswith(reverse("socialaccount_signup")) # Redirect to signup form + + # step 4: post to signup + signup_response = client.post(auth_response.url, user_data) + assert signup_response.status_code == 302 + assert signup_response.url == reverse('home') + + # Assert login was successful and redirected to /projects/ + response = client.get(signup_response.url,follow=True) + assert response.status_code == 200 + assert (reverse('projects'),302) in response.redirect_chain + @pytest.mark.parametrize('account_terms_of_use', boolean_toggle) @pytest.mark.parametrize('username,password', users) -def test_terms_of_use(db, client, settings, username, password, account_terms_of_use): +def test_view_terms_of_use(db, client, settings, username, password, account_terms_of_use): settings.ACCOUNT_TERMS_OF_USE = account_terms_of_use reload_urls('accounts') @@ -801,14 +838,10 @@ def test_shibboleth_logout_username_pattern(db, client, settings, shibboleth, us @pytest.mark.parametrize('username,password', users) def test_terms_of_use_middleware_redirect_and_accept( - db, client, settings, username, password -): - # Arrange - settings.ACCOUNT_TERMS_OF_USE = True - reload_urls('accounts') # needs to reload before the middleware - settings.MIDDLEWARE += [ - settings.ACCOUNT_TERMS_OF_USE_MIDDLEWARE - ] + db, client, settings, username, password, + enable_terms_of_use + ): + # Arrange with enable_terms_of_use # Ensure there are no existing consent entries for the user user = get_user_model().objects.get(username=username) if password else None @@ -843,9 +876,50 @@ def test_terms_of_use_middleware_redirect_and_accept( assert response.status_code == 200 +@pytest.mark.django_db(transaction=True) +def test_social_signup_with_terms_of_use( + db, client, settings,enable_socialaccount, enable_terms_of_use + ): + # Arrange with enable_socialaccount and enable_terms_of_use + reload_urls("accounts") + + # Step 1: Initiate Dummy Login + login_response = client.post(reverse("dummy_login") + "?" + urlencode({"process": "login"})) + + user_data = { + "id": 199, + "email": "newuser@example.com", + "username": "new_user", + "first_name": "New", + "last_name": "User", + } + # The authentication state is included in the redirected URL + auth_response = client.post(login_response.url, user_data) + # Assert, redirected to the signup form (if new user) + assert auth_response.status_code == 302 + assert auth_response.url == reverse("socialaccount_signup") # Redirect to signup form + + # Arrange, post to signup without consent + failed_signup_response = client.post(auth_response.url, user_data) + + # Assert: without consent, signup failed + content_str = failed_signup_response.content.decode() + expected_error = 'You need to agree to the terms of use to proceed' + assert expected_error in content_str, f"Expected error message not found. Response content:\n{content_str}" + + # Arrange step 4 : successful post to signup with consent + user_data['consent'] = True + signup_response = client.post(auth_response.url, user_data) + + # Assert login was successful and redirected to /projects/ + response = client.get(signup_response.url,follow=True) + assert response.status_code == 200 + assert (reverse('projects'),302) in response.redirect_chain + + def test_terms_of_use_middleware_invalidate_terms_version( db, client, settings -): + ): # Arrange constants, settings and user past_datetime = (datetime.now() - timedelta(days=10)).strftime(format="%Y-%m-%d") future_datetime = (datetime.now() + timedelta(days=10)).strftime(format="%Y-%m-%d") diff --git a/rdmo/accounts/tests/utils.py b/rdmo/accounts/tests/utils.py deleted file mode 100644 index b9f796e2d7..0000000000 --- a/rdmo/accounts/tests/utils.py +++ /dev/null @@ -1,33 +0,0 @@ -import sys -from importlib import import_module, reload -from typing import Optional - -from django.urls import clear_url_caches - - -def reload_urlconf(urlconf=None, settings=None): - clear_url_caches() - - if urlconf is None and settings is None: - from django.conf import settings - urlconf = settings.ROOT_URLCONF - elif urlconf is None and settings is not None: - # take the settings during pytest run - urlconf = settings.ROOT_URLCONF - - if urlconf in sys.modules: - reload(sys.modules[urlconf]) - else: - import_module(urlconf) - - -def reload_urls(app_name: Optional[str] = None, settings=None) -> None: - # reload the urlconf of the app - if app_name is not None: - reload_urlconf(urlconf=f'rdmo.{app_name}.urls', settings=settings) - - # reload the core urlconf - reload_urlconf(urlconf='rdmo.core.urls', settings=settings) - - # reload the testcase settings urlconf - reload_urlconf(settings=settings) diff --git a/rdmo/core/settings.py b/rdmo/core/settings.py index 35b975d9c1..a857fd647f 100644 --- a/rdmo/core/settings.py +++ b/rdmo/core/settings.py @@ -128,7 +128,10 @@ SOCIALACCOUNT_SIGNUP = False SOCIALACCOUNT_GROUPS = [] SOCIALACCOUNT_AUTO_SIGNUP = False -SOCIALACCOUNT_ADAPTER = 'rdmo.accounts.adapter.SocialAccountAdapter' +SOCIALACCOUNT_ADAPTER = 'rdmo.accounts.socialaccount.SocialAccountAdapter' +SOCIALACCOUNT_FORMS = { + 'signup': 'rdmo.accounts.socialaccount.SocialSignupForm' +} SOCIALACCOUNT_OPENID_CONNECT_URL_PREFIX = "" # required since 0.60.0 else default is "oidc" SHIBBOLETH = False From 151213578e5422ad0ee7368aabbb973e03c1554a Mon Sep 17 00:00:00 2001 From: David Wallace Date: Tue, 11 Feb 2025 14:03:42 +0100 Subject: [PATCH 10/17] tests(accounts): fix helpers and code style Signed-off-by: David Wallace --- rdmo/accounts/tests/helpers.py | 25 ++++----- rdmo/accounts/tests/test_views.py | 87 ++++++++++++++----------------- 2 files changed, 49 insertions(+), 63 deletions(-) diff --git a/rdmo/accounts/tests/helpers.py b/rdmo/accounts/tests/helpers.py index 75f2d5c49e..b401236077 100644 --- a/rdmo/accounts/tests/helpers.py +++ b/rdmo/accounts/tests/helpers.py @@ -1,6 +1,5 @@ import sys from importlib import import_module, reload -from typing import Optional import pytest @@ -25,12 +24,10 @@ def reload_urlconf(urlconf=None, settings=None): import_module(urlconf) -def reload_urls(app_name: Optional[str] = None, - settings=None, - other_apps: list[str] | None = None ) -> None: +def reload_urls(*app_names: str, settings = None) -> None: # reload the urlconf of the app - if app_name is not None: - reload_urlconf(urlconf=f'rdmo.{app_name}.urls', settings=settings) + for _app in app_names: + reload_urlconf(urlconf=_app, settings=settings) # reload the core urlconf reload_urlconf(urlconf='rdmo.core.urls', settings=settings) @@ -38,20 +35,16 @@ def reload_urls(app_name: Optional[str] = None, # reload the testcase settings urlconf reload_urlconf(settings=settings) - if other_apps: - for _app in other_apps: - reload_urlconf(urlconf=f"{_app}.urls", settings=settings) - get_resolver()._populate() @pytest.fixture(autouse=False) -def enable_terms_of_use(settings): # noqa: PT004 +def enable_terms_of_use(settings): # noqa: PT004 settings.ACCOUNT_TERMS_OF_USE = True settings.MIDDLEWARE += [ settings.ACCOUNT_TERMS_OF_USE_MIDDLEWARE ] - reload_urls('accounts') # Reloads URLs to apply new settings + reload_urls('rdmo.accounts.urls') yield @@ -59,11 +52,11 @@ def enable_terms_of_use(settings): # noqa: PT004 settings.ACCOUNT_TERMS_OF_USE = False settings.MIDDLEWARE.remove(settings.ACCOUNT_TERMS_OF_USE_MIDDLEWARE) # 🔹 Reload URLs to reflect the changes - reload_urls("accounts") + reload_urls('rdmo.accounts.urls') @pytest.fixture(autouse=False) -def enable_socialaccount(db, client, settings, django_db_setup, django_db_blocker, django_db_use_migrations): # noqa: PT004 +def enable_socialaccount(db, client, settings, django_db_setup, django_db_blocker, django_db_use_migrations): # noqa: PT004 # Arrange: this fixture enable and initializes the allauth.sociallaccounts # use this fixture with the decorator @pytest.mark.django_db(transaction=True) on your tests @@ -94,7 +87,7 @@ def enable_socialaccount(db, client, settings, django_db_setup, django_db_blocke # 🔹 Reload URL patterns manually clear_url_caches() - reload_urls("accounts",settings=settings, other_apps=['allauth']) + reload_urls('allauth.urls', 'rdmo.accounts.urls',settings=settings) # 🔹 Verify that the route now exists assert reverse("dummy_login") # Ensure the route exists @@ -123,7 +116,7 @@ def enable_socialaccount(db, client, settings, django_db_setup, django_db_blocke apps.clear_cache() django.setup() - reload_urls("accounts",settings=settings, other_apps=['allauth']) + reload_urls('allauth.urls', 'rdmo.accounts.urls', settings=settings) # **Teardown: Apply only for SQLite** if db_backend == "sqlite": diff --git a/rdmo/accounts/tests/test_views.py b/rdmo/accounts/tests/test_views.py index 6fdc3bd5aa..8f5d08e702 100644 --- a/rdmo/accounts/tests/test_views.py +++ b/rdmo/accounts/tests/test_views.py @@ -14,7 +14,7 @@ from rdmo.accounts.models import CONSENT_SESSION_KEY, ConsentFieldValue -from .helpers import reload_urls +from .helpers import enable_socialaccount, enable_terms_of_use, reload_urls # noqa: F401 users = ( ('editor', 'editor'), @@ -45,7 +45,7 @@ def _reload_urls_at_teardown(): '''Clear the url cache after the test function.''' yield - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') @pytest.mark.parametrize('profile_update', boolean_toggle) @@ -54,7 +54,7 @@ def test_get_profile_update(db, client, settings, profile_update): An authorized GET request to the profile update form returns the form. """ settings.PROFILE_UPDATE = profile_update - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') client.login(username='user', password='user') @@ -74,7 +74,7 @@ def test_get_profile_update_redirect(db, client, settings, profile_update): redirected to login. """ settings.PROFILE_UPDATE = profile_update - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') client.logout() # anynoumous user will be redirected to login in any case @@ -91,7 +91,7 @@ def test_post_profile_update(db, client, settings, profile_update): user and redirects to home. """ settings.PROFILE_UPDATE = profile_update - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') client.login(username='user', password='user') @@ -119,7 +119,7 @@ def test_post_profile_update_cancel(db, client, settings, profile_update): cancel redirects to home. """ settings.PROFILE_UPDATE = profile_update - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') client.login(username='user', password='user') @@ -146,7 +146,7 @@ def test_post_profile_update_cancel2(db, client, settings, profile_update): cancel and the next field redirects to the given url. """ settings.PROFILE_UPDATE = profile_update - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') client.login(username='user', password='user') @@ -174,7 +174,7 @@ def test_post_profile_update_next(db, client, settings, profile_update): updates the user and redirects to the given url. """ settings.PROFILE_UPDATE = profile_update - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') client.login(username='user', password='user') @@ -203,7 +203,7 @@ def test_post_profile_update_next2(db, client, settings, profile_update): field set to profile_update updates the user and redirects to home. """ settings.PROFILE_UPDATE = profile_update - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') client.login(username='user', password='user') @@ -231,7 +231,7 @@ def test_password_change_get(db, client, settings, account): An authorized GET request to the password change form returns the form. """ settings.ACCOUNT = account - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') # independent of the setting, the reverse url exists client.login(username='user', password='user') @@ -251,7 +251,7 @@ def test_password_change_post(db, client, settings, account): password and redirects to home. """ settings.ACCOUNT = account - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') client.login(username='user', password='user') if settings.ACCOUNT: @@ -273,7 +273,7 @@ def test_password_reset_get(db, client, settings, account): A GET request to the password reset form returns the form. """ settings.ACCOUNT = account - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') if settings.ACCOUNT: url = reverse('account_reset_password') @@ -290,7 +290,7 @@ def test_password_reset_post_invalid(db, client, settings, account): sends no mail. """ settings.ACCOUNT = account - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') if settings.ACCOUNT: url = reverse('account_reset_password') @@ -310,7 +310,7 @@ def test_password_reset_post_valid(db, client, settings, account): sends a mail with a correct link. """ settings.ACCOUNT = account - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') if settings.ACCOUNT: url = reverse('account_reset_password') @@ -336,7 +336,7 @@ def test_password_reset_post_valid(db, client, settings, account): @pytest.mark.parametrize('profile_delete', boolean_toggle) def test_remove_user_get(db, client, settings, profile_delete): settings.PROFILE_DELETE = profile_delete - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') client.login(username='user', password='user') url = reverse('profile_remove') @@ -352,7 +352,7 @@ def test_remove_user_get(db, client, settings, profile_delete): @pytest.mark.parametrize('profile_delete', boolean_toggle) def test_remove_user_post(db, client, settings, profile_delete): settings.PROFILE_DELETE = profile_delete - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') client.login(username='user', password='user') url = reverse('profile_remove') @@ -392,7 +392,7 @@ def test_remove_user_post_cancelled(db, client, settings, profile_update): @pytest.mark.parametrize('profile_delete', boolean_toggle) def test_remove_user_post_invalid_email(db, client, settings, profile_delete): settings.PROFILE_DELETE = profile_delete - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') client.login(username='user', password='user') url = reverse('profile_remove') @@ -414,7 +414,7 @@ def test_remove_user_post_invalid_email(db, client, settings, profile_delete): @pytest.mark.parametrize('profile_delete', boolean_toggle) def test_remove_user_post_invalid_password(db, client, settings, profile_delete): settings.PROFILE_DELETE = profile_delete - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') client.login(username='user', password='user') url = reverse('profile_remove') @@ -436,7 +436,7 @@ def test_remove_user_post_invalid_password(db, client, settings, profile_delete) @pytest.mark.parametrize('profile_delete', boolean_toggle) def test_remove_user_post_invalid_consent(db, client, settings, profile_delete): settings.PROFILE_DELETE = profile_delete - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') client.login(username='user', password='user') url = reverse('profile_remove') @@ -458,7 +458,7 @@ def test_remove_user_post_invalid_consent(db, client, settings, profile_delete): @pytest.mark.parametrize('profile_delete', boolean_toggle) def test_remove_a_user_without_usable_password_post(db, client, settings, profile_delete): settings.PROFILE_DELETE = profile_delete - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') user = get_user_model().objects.get(username='user', email='user@example.com') user.set_unusable_password() @@ -484,7 +484,7 @@ def test_remove_a_user_without_usable_password_post(db, client, settings, profil @pytest.mark.parametrize('profile_delete', boolean_toggle) def test_remove_a_user_without_usable_password_post_invalid_email(db, client, settings, profile_delete): settings.PROFILE_DELETE = profile_delete - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') user = get_user_model().objects.get(username='user', email='user@example.com') user.set_unusable_password() @@ -510,7 +510,7 @@ def test_remove_a_user_without_usable_password_post_invalid_email(db, client, se @pytest.mark.parametrize('profile_delete', boolean_toggle) def test_remove_a_user_without_usable_password_post_empty_email(db, client, settings, profile_delete): settings.PROFILE_DELETE = profile_delete - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') user = get_user_model().objects.get(username='user', email='user@example.com') user.set_unusable_password() @@ -537,7 +537,7 @@ def test_remove_a_user_without_usable_password_post_empty_email(db, client, sett @pytest.mark.parametrize('profile_delete', boolean_toggle) def test_remove_a_user_without_usable_password_post_invalid_consent(db, client, settings, profile_delete): settings.PROFILE_DELETE = profile_delete - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') user = get_user_model().objects.get(username='user', email='user@example.com') user.set_unusable_password() @@ -564,7 +564,7 @@ def test_remove_a_user_without_usable_password_post_invalid_consent(db, client, @pytest.mark.parametrize('account_signup', boolean_toggle) def test_signup(db, client, settings, account_signup): settings.ACCOUNT_SIGNUP = account_signup - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') url = reverse('account_signup') @@ -587,7 +587,7 @@ def test_signup(db, client, settings, account_signup): @pytest.mark.parametrize('account_signup', boolean_toggle) def test_signup_next(db, client, settings, account_signup): settings.ACCOUNT_SIGNUP = account_signup - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') url = reverse('account_signup') + '?next=/about/' response = client.post(url, { @@ -606,7 +606,7 @@ def test_signup_next(db, client, settings, account_signup): assert response.status_code == 200 @pytest.mark.django_db(transaction=True) -def test_social_signup(db, client, settings, enable_socialaccount): +def test_social_signup(db, client, settings, enable_socialaccount): # noqa: F811 # Arrange socialaccount enabled by fixture enable_socialaccount # Ensure migrations are applied before the test runs # Step 1: Initiate Dummy Login @@ -645,7 +645,7 @@ def test_social_signup(db, client, settings, enable_socialaccount): @pytest.mark.parametrize('username,password', users) def test_view_terms_of_use(db, client, settings, username, password, account_terms_of_use): settings.ACCOUNT_TERMS_OF_USE = account_terms_of_use - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') client.login(username=username, password=password) if settings.ACCOUNT_TERMS_OF_USE: @@ -661,7 +661,7 @@ def test_view_terms_of_use(db, client, settings, username, password, account_ter @pytest.mark.parametrize('account_allow_user_token', boolean_toggle) def test_token_get_for_user(db, client, settings, account_allow_user_token): settings.ACCOUNT_ALLOW_USER_TOKEN = account_allow_user_token - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') client.login(username='user', password='user') @@ -677,7 +677,7 @@ def test_token_get_for_user(db, client, settings, account_allow_user_token): @pytest.mark.parametrize('account_allow_user_token', boolean_toggle) def test_token_get_for_anonymous(db, client, settings, account_allow_user_token): settings.ACCOUNT_ALLOW_USER_TOKEN = account_allow_user_token - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') client.login(username='anonymous', password=None) if settings.ACCOUNT_ALLOW_USER_TOKEN: @@ -693,7 +693,7 @@ def test_token_get_for_anonymous(db, client, settings, account_allow_user_token) @pytest.mark.parametrize('account_allow_user_token', boolean_toggle) def test_token_post_for_user(db, client, settings, account_allow_user_token): settings.ACCOUNT_ALLOW_USER_TOKEN = account_allow_user_token - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') client.login(username='user', password='user') if settings.ACCOUNT_ALLOW_USER_TOKEN: @@ -708,7 +708,7 @@ def test_token_post_for_user(db, client, settings, account_allow_user_token): @pytest.mark.parametrize('account_allow_user_token', boolean_toggle) def test_token_post_for_anonymous(db, client, settings, account_allow_user_token): settings.ACCOUNT_ALLOW_USER_TOKEN = account_allow_user_token - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') client.login(username='anonymous', password=None) if settings.ACCOUNT_ALLOW_USER_TOKEN: @@ -725,7 +725,7 @@ def test_token_post_for_anonymous(db, client, settings, account_allow_user_token @pytest.mark.parametrize('username,password', users) def test_home_login_form(db, client, settings, login_form, username, password): settings.LOGIN_FORM = login_form - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') # Anonymous user lands on home client.login(username='anonymous', password=None) url = reverse('home') @@ -743,7 +743,7 @@ def test_home_login_form(db, client, settings, login_form, username, password): def test_shibboleth_for_home_url(db, client, settings, shibboleth, username, password): settings.SHIBBOLETH = shibboleth settings.ACCOUNT = False - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') # Anonymous user lands on home client.login(username='anonymous', password=None) url = reverse('home') @@ -759,7 +759,7 @@ def test_shibboleth_for_home_url(db, client, settings, shibboleth, username, pas def test_shibboleth_login_view(db, client, settings, username, password): settings.SHIBBOLETH = True settings.SHIBBOLETH_LOGIN_URL = '/shibboleth/login' - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') # Anonymous user lands on home client.login(username='anonymous', password=None) @@ -786,7 +786,7 @@ def test_shibboleth_login_view(db, client, settings, username, password): # settings.SHIBBOLETH = shibboleth # settings.SHIBBOLETH_LOGIN_URL = shibboleth_login_url # settings.ACCOUNT = False -# reload_urls('accounts') +# reload_urls('rdmo.accounts.urls') # client.login(username='anonymous', password=None) @@ -824,7 +824,7 @@ def test_shibboleth_login_view(db, client, settings, username, password): def test_shibboleth_logout_username_pattern(db, client, settings, shibboleth, username, password): settings.SHIBBOLETH = shibboleth settings.SHIBBOLETH_USERNAME_PATTERN = username - reload_urls('accounts') + reload_urls('rdmo.accounts.urls') client.login(username=username, password=password) if settings.SHIBBOLETH: @@ -838,8 +838,7 @@ def test_shibboleth_logout_username_pattern(db, client, settings, shibboleth, us @pytest.mark.parametrize('username,password', users) def test_terms_of_use_middleware_redirect_and_accept( - db, client, settings, username, password, - enable_terms_of_use + db, client, settings, username, password, enable_terms_of_use # noqa: F811 ): # Arrange with enable_terms_of_use @@ -878,10 +877,9 @@ def test_terms_of_use_middleware_redirect_and_accept( @pytest.mark.django_db(transaction=True) def test_social_signup_with_terms_of_use( - db, client, settings,enable_socialaccount, enable_terms_of_use + db, client, settings, enable_socialaccount, enable_terms_of_use # noqa: F811 ): # Arrange with enable_socialaccount and enable_terms_of_use - reload_urls("accounts") # Step 1: Initiate Dummy Login login_response = client.post(reverse("dummy_login") + "?" + urlencode({"process": "login"})) @@ -918,18 +916,12 @@ def test_social_signup_with_terms_of_use( def test_terms_of_use_middleware_invalidate_terms_version( - db, client, settings + db, client, settings, enable_terms_of_use # noqa: F811 ): # Arrange constants, settings and user past_datetime = (datetime.now() - timedelta(days=10)).strftime(format="%Y-%m-%d") future_datetime = (datetime.now() + timedelta(days=10)).strftime(format="%Y-%m-%d") - settings.ACCOUNT_TERMS_OF_USE = True - reload_urls('accounts') # needs to reload before the middleware - settings.MIDDLEWARE += [ - settings.ACCOUNT_TERMS_OF_USE_MIDDLEWARE - ] - terms_accept_url = reverse('terms_of_use_accept') # Arrange user object username = password = 'user' user = get_user_model().objects.get(username=username) @@ -949,6 +941,7 @@ def test_terms_of_use_middleware_invalidate_terms_version( response = client.get(reverse('projects')) # Assert - consent is now invalid and should redirect to terms_of_use_accept + terms_accept_url = reverse('terms_of_use_accept') assertRedirects(response, terms_accept_url) # Act - Try to make a POST request to terms_of_use_accept From e86843c87a733aa2cc9f1e9e04900c26d2aa1959 Mon Sep 17 00:00:00 2001 From: David Wallace Date: Thu, 13 Feb 2025 15:34:13 +0100 Subject: [PATCH 11/17] tests(accounts): add allauth.socialaccount to base config and fix social tests Signed-off-by: David Wallace --- rdmo/accounts/tests/helpers.py | 93 ++--------- rdmo/accounts/tests/test_views.py | 150 +++++------------- .../tests/test_views_socialaccount.py | 68 ++++++++ testing/config/settings/base.py | 4 +- 4 files changed, 123 insertions(+), 192 deletions(-) create mode 100644 rdmo/accounts/tests/test_views_socialaccount.py diff --git a/rdmo/accounts/tests/helpers.py b/rdmo/accounts/tests/helpers.py index b401236077..d8df21c177 100644 --- a/rdmo/accounts/tests/helpers.py +++ b/rdmo/accounts/tests/helpers.py @@ -3,20 +3,17 @@ import pytest -from django.core.management import call_command -from django.db import connection from django.urls import clear_url_caches, get_resolver, reverse -def reload_urlconf(urlconf=None, settings=None): +def reload_urlconf(urlconf=None, root_urlconf=None): clear_url_caches() - - if urlconf is None and settings is None: + if urlconf is None and root_urlconf is None: from django.conf import settings urlconf = settings.ROOT_URLCONF - elif urlconf is None and settings is not None: + elif urlconf is None and root_urlconf is not None: # take the settings during pytest run - urlconf = settings.ROOT_URLCONF + urlconf = root_urlconf if urlconf in sys.modules: reload(sys.modules[urlconf]) @@ -24,21 +21,21 @@ def reload_urlconf(urlconf=None, settings=None): import_module(urlconf) -def reload_urls(*app_names: str, settings = None) -> None: +def reload_urls(*app_names: str, root_urlconf = None) -> None: # reload the urlconf of the app for _app in app_names: - reload_urlconf(urlconf=_app, settings=settings) + reload_urlconf(urlconf=_app) # reload the core urlconf - reload_urlconf(urlconf='rdmo.core.urls', settings=settings) + reload_urlconf(urlconf='rdmo.core.urls') # reload the testcase settings urlconf - reload_urlconf(settings=settings) + reload_urlconf(root_urlconf=root_urlconf) get_resolver()._populate() -@pytest.fixture(autouse=False) +@pytest.fixture def enable_terms_of_use(settings): # noqa: PT004 settings.ACCOUNT_TERMS_OF_USE = True settings.MIDDLEWARE += [ @@ -55,79 +52,17 @@ def enable_terms_of_use(settings): # noqa: PT004 reload_urls('rdmo.accounts.urls') -@pytest.fixture(autouse=False) -def enable_socialaccount(db, client, settings, django_db_setup, django_db_blocker, django_db_use_migrations): # noqa: PT004 - # Arrange: this fixture enable and initializes the allauth.sociallaccounts - # use this fixture with the decorator @pytest.mark.django_db(transaction=True) on your tests - - # Detect if --no-migrations is being used - if not django_db_use_migrations and connection.vendor == 'sqlite': - pytest.xfail("This test is expected to fail when --no-migrations is enabled.") - +@pytest.fixture +def enable_socialaccount(settings): # noqa: PT004 + # Arrange: this fixture enable and initializes the allauth.sociallaccount + # INSTALLED_APPS already has "allauth.socialaccount","allauth.socialaccount.providers.dummy" settings.SOCIALACCOUNT = True settings.SOCIALACCOUNT_SIGNUP = True - # 🔹 Add required apps - added_apps = [ - "allauth.socialaccount", - "allauth.socialaccount.providers.dummy", - ] - new_apps = [app for app in added_apps if app not in settings.INSTALLED_APPS] - - if new_apps: - settings.INSTALLED_APPS += new_apps - - # 🔹 Force Django to reload apps - import django - from django.apps import apps - apps.clear_cache() - django.setup() - - with django_db_blocker.unblock(): - call_command("migrate") - # 🔹 Reload URL patterns manually - clear_url_caches() - reload_urls('allauth.urls', 'rdmo.accounts.urls',settings=settings) - # 🔹 Verify that the route now exists assert reverse("dummy_login") # Ensure the route exists - # Check database backend - db_backend = connection.vendor # 'sqlite', 'mysql', or 'postgresql' - - if db_backend == "sqlite": - with connection.cursor() as cursor: - cursor.execute("PRAGMA foreign_keys=OFF;") # Disable FKs in SQLite - - from allauth.socialaccount.models import SocialAccount - assert SocialAccount.objects.exists() is not None # Ensure table exists before using it - yield # Run test - # 🔹 Cleanup: Remove added apps and reset settings - settings.INSTALLED_APPS.remove("allauth.socialaccount") - settings.INSTALLED_APPS.remove("allauth.socialaccount.providers.dummy") - + # 🔹 Cleanup: reset settings settings.SOCIALACCOUNT = False settings.SOCIALACCOUNT_SIGNUP = False - - # 🔹 Force Django to reload apps again - import django - from django.apps import apps - apps.clear_cache() - django.setup() - - reload_urls('allauth.urls', 'rdmo.accounts.urls', settings=settings) - - # **Teardown: Apply only for SQLite** - if db_backend == "sqlite": - with django_db_blocker.unblock(): - with connection.cursor() as cursor: - cursor.execute("PRAGMA foreign_keys=OFF;") # Disable FKs before flush - try: - call_command("flush", "--no-input") # Reset DB - finally: - with connection.cursor() as cursor: - cursor.execute("PRAGMA foreign_keys=ON;") # Re-enable FKs - else: - # For MySQL & PostgreSQL, just run normal flush - call_command("flush", "--no-input", allow_cascade=True) diff --git a/rdmo/accounts/tests/test_views.py b/rdmo/accounts/tests/test_views.py index 8f5d08e702..4916a136c7 100644 --- a/rdmo/accounts/tests/test_views.py +++ b/rdmo/accounts/tests/test_views.py @@ -3,18 +3,16 @@ import pytest -from django.contrib.auth import get_user_model from django.core import mail from django.db.models import ObjectDoesNotExist from django.urls import reverse from django.urls.exceptions import NoReverseMatch -from django.utils.http import urlencode from pytest_django.asserts import assertContains, assertNotContains, assertRedirects, assertTemplateUsed, assertURLEqual from rdmo.accounts.models import CONSENT_SESSION_KEY, ConsentFieldValue -from .helpers import enable_socialaccount, enable_terms_of_use, reload_urls # noqa: F401 +from .helpers import enable_terms_of_use, reload_urls # noqa: F401 users = ( ('editor', 'editor'), @@ -49,7 +47,7 @@ def _reload_urls_at_teardown(): @pytest.mark.parametrize('profile_update', boolean_toggle) -def test_get_profile_update(db, client, settings, profile_update): +def test_get_profile_update(db, client, settings, django_user_model, profile_update): """ An authorized GET request to the profile update form returns the form. """ @@ -68,7 +66,7 @@ def test_get_profile_update(db, client, settings, profile_update): @pytest.mark.parametrize('profile_update', boolean_toggle) -def test_get_profile_update_redirect(db, client, settings, profile_update): +def test_get_profile_update_redirect(db, client, settings, django_user_model, profile_update): """ An unauthorized GET request to the profile update form gets redirected to login. @@ -85,7 +83,7 @@ def test_get_profile_update_redirect(db, client, settings, profile_update): @pytest.mark.parametrize('profile_update', boolean_toggle) -def test_post_profile_update(db, client, settings, profile_update): +def test_post_profile_update(db, client, settings, django_user_model, profile_update): """ An authorized POST request to the profile update form updates the user and redirects to home. @@ -113,7 +111,7 @@ def test_post_profile_update(db, client, settings, profile_update): @pytest.mark.parametrize('profile_update', boolean_toggle) -def test_post_profile_update_cancel(db, client, settings, profile_update): +def test_post_profile_update_cancel(db, client, settings, django_user_model, profile_update): """ An authorized POST request to the profile update form updates with cancel redirects to home. @@ -140,7 +138,7 @@ def test_post_profile_update_cancel(db, client, settings, profile_update): @pytest.mark.parametrize('profile_update', boolean_toggle) -def test_post_profile_update_cancel2(db, client, settings, profile_update): +def test_post_profile_update_cancel2(db, client, settings, django_user_model, profile_update): """ An authorized POST request to the profile update form updates with cancel and the next field redirects to the given url. @@ -168,7 +166,7 @@ def test_post_profile_update_cancel2(db, client, settings, profile_update): @pytest.mark.parametrize('profile_update', boolean_toggle) -def test_post_profile_update_next(db, client, settings, profile_update): +def test_post_profile_update_next(db, client, settings, django_user_model, profile_update): """ An authorized POST request to the profile update form with next field updates the user and redirects to the given url. @@ -197,7 +195,7 @@ def test_post_profile_update_next(db, client, settings, profile_update): @pytest.mark.parametrize('profile_update', boolean_toggle) -def test_post_profile_update_next2(db, client, settings, profile_update): +def test_post_profile_update_next2(db, client, settings, django_user_model, profile_update): """ An authorized POST request to the profile update form with next field set to profile_update updates the user and redirects to home. @@ -334,7 +332,7 @@ def test_password_reset_post_valid(db, client, settings, account): @pytest.mark.parametrize('profile_delete', boolean_toggle) -def test_remove_user_get(db, client, settings, profile_delete): +def test_remove_user_get(db, client, settings, django_user_model, profile_delete): settings.PROFILE_DELETE = profile_delete reload_urls('rdmo.accounts.urls') @@ -350,7 +348,7 @@ def test_remove_user_get(db, client, settings, profile_delete): @pytest.mark.parametrize('profile_delete', boolean_toggle) -def test_remove_user_post(db, client, settings, profile_delete): +def test_remove_user_post(db, client, settings, django_user_model, profile_delete): settings.PROFILE_DELETE = profile_delete reload_urls('rdmo.accounts.urls') @@ -366,14 +364,14 @@ def test_remove_user_post(db, client, settings, profile_delete): assert response.status_code == 200 if settings.PROFILE_DELETE: assertTemplateUsed(response, 'profile/profile_remove_success.html') - pytest.raises(ObjectDoesNotExist, get_user_model().objects.get, username='user') + pytest.raises(ObjectDoesNotExist, django_user_model.objects.get, username='user') else: assertTemplateUsed(response, 'profile/profile_remove_closed.html') - assert get_user_model().objects.get(username='user') + assert django_user_model.objects.get(username='user') @pytest.mark.parametrize('profile_update', boolean_toggle) -def test_remove_user_post_cancelled(db, client, settings, profile_update): +def test_remove_user_post_cancelled(db, client, settings, django_user_model, profile_update): settings.PROFILE_UPDATE = profile_update settings.PROFILE_DELETE = True @@ -382,7 +380,7 @@ def test_remove_user_post_cancelled(db, client, settings, profile_update): response = client.post(url, {'cancel': 'cancel'}) assert response.status_code == 302 - assert get_user_model().objects.filter(username='user').exists() + assert django_user_model.objects.filter(username='user').exists() if settings.PROFILE_UPDATE: assert response.url == '/account' else: @@ -390,7 +388,7 @@ def test_remove_user_post_cancelled(db, client, settings, profile_update): @pytest.mark.parametrize('profile_delete', boolean_toggle) -def test_remove_user_post_invalid_email(db, client, settings, profile_delete): +def test_remove_user_post_invalid_email(db, client, settings, django_user_model, profile_delete): settings.PROFILE_DELETE = profile_delete reload_urls('rdmo.accounts.urls') @@ -404,7 +402,7 @@ def test_remove_user_post_invalid_email(db, client, settings, profile_delete): response = client.post(url, data) assert response.status_code == 200 - assert get_user_model().objects.get(username='user') + assert django_user_model.objects.get(username='user') if settings.PROFILE_DELETE: assertTemplateUsed(response, 'profile/profile_remove_failed.html') else: @@ -412,7 +410,7 @@ def test_remove_user_post_invalid_email(db, client, settings, profile_delete): @pytest.mark.parametrize('profile_delete', boolean_toggle) -def test_remove_user_post_invalid_password(db, client, settings, profile_delete): +def test_remove_user_post_invalid_password(db, client, settings, django_user_model, profile_delete): settings.PROFILE_DELETE = profile_delete reload_urls('rdmo.accounts.urls') @@ -426,7 +424,7 @@ def test_remove_user_post_invalid_password(db, client, settings, profile_delete) response = client.post(url, data) assert response.status_code == 200 - assert get_user_model().objects.get(username='user') + assert django_user_model.objects.get(username='user') if settings.PROFILE_DELETE: assertTemplateUsed(response, 'profile/profile_remove_failed.html') else: @@ -434,7 +432,7 @@ def test_remove_user_post_invalid_password(db, client, settings, profile_delete) @pytest.mark.parametrize('profile_delete', boolean_toggle) -def test_remove_user_post_invalid_consent(db, client, settings, profile_delete): +def test_remove_user_post_invalid_consent(db, client, settings, django_user_model, profile_delete): settings.PROFILE_DELETE = profile_delete reload_urls('rdmo.accounts.urls') @@ -456,11 +454,11 @@ def test_remove_user_post_invalid_consent(db, client, settings, profile_delete): @pytest.mark.parametrize('profile_delete', boolean_toggle) -def test_remove_a_user_without_usable_password_post(db, client, settings, profile_delete): +def test_remove_a_user_without_usable_password_post(db, client, settings, django_user_model, profile_delete): settings.PROFILE_DELETE = profile_delete reload_urls('rdmo.accounts.urls') - user = get_user_model().objects.get(username='user', email='user@example.com') + user = django_user_model.objects.get(username='user', email='user@example.com') user.set_unusable_password() user.save() @@ -475,18 +473,19 @@ def test_remove_a_user_without_usable_password_post(db, client, settings, profil assert response.status_code == 200 if settings.PROFILE_DELETE: assertTemplateUsed(response, 'profile/profile_remove_success.html') - pytest.raises(ObjectDoesNotExist, get_user_model().objects.get, pk=user.pk) + pytest.raises(ObjectDoesNotExist, django_user_model.objects.get, pk=user.pk) else: assertTemplateUsed(response, 'profile/profile_remove_closed.html') client.logout() @pytest.mark.parametrize('profile_delete', boolean_toggle) -def test_remove_a_user_without_usable_password_post_invalid_email(db, client, settings, profile_delete): +def test_remove_a_user_without_usable_password_post_invalid_email(db, client, settings, + django_user_model, profile_delete): settings.PROFILE_DELETE = profile_delete reload_urls('rdmo.accounts.urls') - user = get_user_model().objects.get(username='user', email='user@example.com') + user = django_user_model.objects.get(username='user', email='user@example.com') user.set_unusable_password() user.save() @@ -499,7 +498,7 @@ def test_remove_a_user_without_usable_password_post_invalid_email(db, client, se response = client.post(url, data) assert response.status_code == 200 - get_user_model().objects.get(pk=user.pk) + django_user_model.objects.get(pk=user.pk) if settings.PROFILE_DELETE: assertTemplateUsed(response, 'profile/profile_remove_failed.html') else: @@ -508,11 +507,12 @@ def test_remove_a_user_without_usable_password_post_invalid_email(db, client, se @pytest.mark.parametrize('profile_delete', boolean_toggle) -def test_remove_a_user_without_usable_password_post_empty_email(db, client, settings, profile_delete): +def test_remove_a_user_without_usable_password_post_empty_email(db, client, settings, + django_user_model, profile_delete): settings.PROFILE_DELETE = profile_delete reload_urls('rdmo.accounts.urls') - user = get_user_model().objects.get(username='user', email='user@example.com') + user = django_user_model.objects.get(username='user', email='user@example.com') user.set_unusable_password() user.save() @@ -525,7 +525,7 @@ def test_remove_a_user_without_usable_password_post_empty_email(db, client, sett response = client.post(url, data) assert response.status_code == 200 - assert get_user_model().objects.get(pk=user.pk) + assert django_user_model.objects.get(pk=user.pk) if settings.PROFILE_DELETE: assertTemplateUsed(response, 'profile/profile_remove_form.html') assert response.context['form'].errors['email'] == ['This field is required.'] # check if email is required @@ -535,11 +535,12 @@ def test_remove_a_user_without_usable_password_post_empty_email(db, client, sett @pytest.mark.parametrize('profile_delete', boolean_toggle) -def test_remove_a_user_without_usable_password_post_invalid_consent(db, client, settings, profile_delete): +def test_remove_a_user_without_usable_password_post_invalid_consent(db, client, settings, + django_user_model, profile_delete): settings.PROFILE_DELETE = profile_delete reload_urls('rdmo.accounts.urls') - user = get_user_model().objects.get(username='user', email='user@example.com') + user = django_user_model.objects.get(username='user', email='user@example.com') user.set_unusable_password() user.save() @@ -552,7 +553,7 @@ def test_remove_a_user_without_usable_password_post_invalid_consent(db, client, response = client.post(url, data) assert response.status_code == 200 - assert get_user_model().objects.get(pk=user.pk) + assert django_user_model.objects.get(pk=user.pk) if settings.PROFILE_DELETE: assertTemplateUsed(response, 'profile/profile_remove_form.html') assert response.context['form'].errors['consent'] == ['This field is required.'] # check if consent is required @@ -605,41 +606,6 @@ def test_signup_next(db, client, settings, account_signup): else: assert response.status_code == 200 -@pytest.mark.django_db(transaction=True) -def test_social_signup(db, client, settings, enable_socialaccount): # noqa: F811 - # Arrange socialaccount enabled by fixture enable_socialaccount - # Ensure migrations are applied before the test runs - # Step 1: Initiate Dummy Login - login_url = reverse("dummy_login") + "?" + urlencode({"process": "login"}) - login_response = client.post(login_url) - assert login_response.status_code == 302 # Ensure login form loads correctly - assert reverse("dummy_authenticate") in login_response.url - - # Step 2: POST request to Dummy Authenticate with test user data - user_data = { - "id": 99, - "email": "newuser@example.com", # New user - "username": "new_user", - "first_name": "New", - "last_name": "User", - } - # The authentication state is included in the redirected URL - auth_response = client.post(login_response.url, user_data) - - # Step 3: Ensure we are redirected to the signup form (if new user) - assert auth_response.status_code == 302 - assert auth_response.url.startswith(reverse("socialaccount_signup")) # Redirect to signup form - - # step 4: post to signup - signup_response = client.post(auth_response.url, user_data) - assert signup_response.status_code == 302 - assert signup_response.url == reverse('home') - - # Assert login was successful and redirected to /projects/ - response = client.get(signup_response.url,follow=True) - assert response.status_code == 200 - assert (reverse('projects'),302) in response.redirect_chain - @pytest.mark.parametrize('account_terms_of_use', boolean_toggle) @pytest.mark.parametrize('username,password', users) @@ -838,12 +804,12 @@ def test_shibboleth_logout_username_pattern(db, client, settings, shibboleth, us @pytest.mark.parametrize('username,password', users) def test_terms_of_use_middleware_redirect_and_accept( - db, client, settings, username, password, enable_terms_of_use # noqa: F811 + db, client, settings, django_user_model, username, password, enable_terms_of_use # noqa: F811 ): # Arrange with enable_terms_of_use # Ensure there are no existing consent entries for the user - user = get_user_model().objects.get(username=username) if password else None + user = django_user_model.objects.get(username=username) if password else None if user: ConsentFieldValue.objects.filter(user=user).delete() @@ -875,48 +841,8 @@ def test_terms_of_use_middleware_redirect_and_accept( assert response.status_code == 200 -@pytest.mark.django_db(transaction=True) -def test_social_signup_with_terms_of_use( - db, client, settings, enable_socialaccount, enable_terms_of_use # noqa: F811 - ): - # Arrange with enable_socialaccount and enable_terms_of_use - - # Step 1: Initiate Dummy Login - login_response = client.post(reverse("dummy_login") + "?" + urlencode({"process": "login"})) - - user_data = { - "id": 199, - "email": "newuser@example.com", - "username": "new_user", - "first_name": "New", - "last_name": "User", - } - # The authentication state is included in the redirected URL - auth_response = client.post(login_response.url, user_data) - # Assert, redirected to the signup form (if new user) - assert auth_response.status_code == 302 - assert auth_response.url == reverse("socialaccount_signup") # Redirect to signup form - - # Arrange, post to signup without consent - failed_signup_response = client.post(auth_response.url, user_data) - - # Assert: without consent, signup failed - content_str = failed_signup_response.content.decode() - expected_error = 'You need to agree to the terms of use to proceed' - assert expected_error in content_str, f"Expected error message not found. Response content:\n{content_str}" - - # Arrange step 4 : successful post to signup with consent - user_data['consent'] = True - signup_response = client.post(auth_response.url, user_data) - - # Assert login was successful and redirected to /projects/ - response = client.get(signup_response.url,follow=True) - assert response.status_code == 200 - assert (reverse('projects'),302) in response.redirect_chain - - def test_terms_of_use_middleware_invalidate_terms_version( - db, client, settings, enable_terms_of_use # noqa: F811 + db, client, settings, django_user_model, enable_terms_of_use # noqa: F811 ): # Arrange constants, settings and user past_datetime = (datetime.now() - timedelta(days=10)).strftime(format="%Y-%m-%d") @@ -924,7 +850,7 @@ def test_terms_of_use_middleware_invalidate_terms_version( # Arrange user object username = password = 'user' - user = get_user_model().objects.get(username=username) + user = django_user_model.objects.get(username=username) _consent = ConsentFieldValue.objects.create(user=user, consent=True) # Assert - Access the home page, user has a valid consent diff --git a/rdmo/accounts/tests/test_views_socialaccount.py b/rdmo/accounts/tests/test_views_socialaccount.py new file mode 100644 index 0000000000..e8cc2e55eb --- /dev/null +++ b/rdmo/accounts/tests/test_views_socialaccount.py @@ -0,0 +1,68 @@ + +from django.urls import reverse +from django.utils.http import urlencode + + +def test_social_signup(db, client, enable_socialaccount): + # Arrange with enable_socialaccount + login_url = reverse("dummy_login") + "?" + urlencode({"process": "login"}) + login_response = client.post(login_url) + assert login_response.status_code == 302 + assert reverse("dummy_authenticate") in login_response.url + + user_data = { + "id": 99, + "email": "newuser@example.com", + "username": "new_user", + "first_name": "New", + "last_name": "User", + } + auth_response = client.post(login_response.url, user_data) + assert auth_response.status_code == 302 + assert auth_response.url.startswith(reverse("socialaccount_signup")) + + signup_response = client.post(auth_response.url, user_data) + assert signup_response.status_code == 302 + assert signup_response.url == reverse('home') + + response = client.get(signup_response.url, follow=True) + assert response.status_code == 200 + assert (reverse('projects'), 302) in response.redirect_chain + + +def test_social_signup_with_terms_of_use( + db, client, enable_terms_of_use, enable_socialaccount + ): + # Arrange with enable_socialaccount and enable_terms_of_use + # Arrange: initiate dummy Login + login_response = client.post(reverse("dummy_login") + "?" + urlencode({"process": "login"})) + + user_data = { + "id": 199, + "email": "newuser@example.com", + "username": "new_user", + "first_name": "New", + "last_name": "User", + } + # The authentication state is included in the redirected URL + auth_response = client.post(login_response.url, user_data) + # Assert, redirected to the signup form (if new user) + assert auth_response.status_code == 302 + assert auth_response.url == reverse("socialaccount_signup") # Redirect to signup form + + # Arrange, post to signup without consent + failed_signup_response = client.post(auth_response.url, user_data) + + # Assert: without consent, signup failed + content_str = failed_signup_response.content.decode() + expected_error = 'You need to agree to the terms of use to proceed' + assert expected_error in content_str, f"Expected error message not found. Response content:\n{content_str}" + + # Arrange: successful post to signup with consent + user_data['consent'] = True + signup_response = client.post(auth_response.url, user_data) + + # Assert login was successful and redirected to /projects/ + response = client.get(signup_response.url,follow=True) + assert response.status_code == 200 + assert (reverse('projects'),302) in response.redirect_chain diff --git a/testing/config/settings/base.py b/testing/config/settings/base.py index dad68a9871..bbf4a333d4 100644 --- a/testing/config/settings/base.py +++ b/testing/config/settings/base.py @@ -52,7 +52,9 @@ INSTALLED_APPS += [ 'allauth', - 'allauth.account' + 'allauth.account', + "allauth.socialaccount", + "allauth.socialaccount.providers.dummy", ] MIDDLEWARE += [ From f2c4ffb83b2c77dddb551f874eea0879f0c734e9 Mon Sep 17 00:00:00 2001 From: David Wallace Date: Thu, 13 Feb 2025 16:44:57 +0100 Subject: [PATCH 12/17] tests(accounts): fix import and code style Signed-off-by: David Wallace --- rdmo/accounts/tests/test_views_socialaccount.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/rdmo/accounts/tests/test_views_socialaccount.py b/rdmo/accounts/tests/test_views_socialaccount.py index e8cc2e55eb..7686e4704d 100644 --- a/rdmo/accounts/tests/test_views_socialaccount.py +++ b/rdmo/accounts/tests/test_views_socialaccount.py @@ -2,8 +2,10 @@ from django.urls import reverse from django.utils.http import urlencode +from .helpers import enable_socialaccount, enable_terms_of_use # noqa: F401 -def test_social_signup(db, client, enable_socialaccount): + +def test_social_signup(db, client, enable_socialaccount): # noqa: F811 # Arrange with enable_socialaccount login_url = reverse("dummy_login") + "?" + urlencode({"process": "login"}) login_response = client.post(login_url) @@ -31,7 +33,7 @@ def test_social_signup(db, client, enable_socialaccount): def test_social_signup_with_terms_of_use( - db, client, enable_terms_of_use, enable_socialaccount + db, client, enable_terms_of_use, enable_socialaccount # noqa: F811 ): # Arrange with enable_socialaccount and enable_terms_of_use # Arrange: initiate dummy Login From 3aa760a1bc57f4826cffd8a3eb175d53cc7e61c0 Mon Sep 17 00:00:00 2001 From: David Wallace Date: Mon, 17 Feb 2025 15:16:47 +0100 Subject: [PATCH 13/17] refactor(core): rename to CoreConfig and remove ready method Signed-off-by: David Wallace --- rdmo/core/apps.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/rdmo/core/apps.py b/rdmo/core/apps.py index cf8d9c956f..cd76489364 100644 --- a/rdmo/core/apps.py +++ b/rdmo/core/apps.py @@ -2,9 +2,6 @@ from django.utils.translation import gettext_lazy as _ -class AccountsConfig(AppConfig): +class CoreConfig(AppConfig): name = 'rdmo.core' verbose_name = _('Core') - - def ready(self): - pass From ead1f71112b708aafee03f0bc2e3d27598dc8c39 Mon Sep 17 00:00:00 2001 From: David Wallace Date: Mon, 17 Feb 2025 15:41:59 +0100 Subject: [PATCH 14/17] refactor(core,ToU): add ToU middleware to MIDDLEWARE by default Signed-off-by: David Wallace --- rdmo/accounts/tests/helpers.py | 4 ---- rdmo/core/checks.py | 6 +++--- rdmo/core/settings.py | 4 ++-- 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/rdmo/accounts/tests/helpers.py b/rdmo/accounts/tests/helpers.py index d8df21c177..5881d5ae68 100644 --- a/rdmo/accounts/tests/helpers.py +++ b/rdmo/accounts/tests/helpers.py @@ -38,16 +38,12 @@ def reload_urls(*app_names: str, root_urlconf = None) -> None: @pytest.fixture def enable_terms_of_use(settings): # noqa: PT004 settings.ACCOUNT_TERMS_OF_USE = True - settings.MIDDLEWARE += [ - settings.ACCOUNT_TERMS_OF_USE_MIDDLEWARE - ] reload_urls('rdmo.accounts.urls') yield # revert settings to initial state settings.ACCOUNT_TERMS_OF_USE = False - settings.MIDDLEWARE.remove(settings.ACCOUNT_TERMS_OF_USE_MIDDLEWARE) # 🔹 Reload URLs to reflect the changes reload_urls('rdmo.accounts.urls') diff --git a/rdmo/core/checks.py b/rdmo/core/checks.py index fef36bc2b5..13e6f80970 100644 --- a/rdmo/core/checks.py +++ b/rdmo/core/checks.py @@ -7,12 +7,12 @@ def check_account_terms_of_use_date_setting(app_configs, **kwargs): errors = [] if settings.ACCOUNT_TERMS_OF_USE: - if settings.ACCOUNT_TERMS_OF_USE_MIDDLEWARE not in settings.MIDDLEWARE: + if 'rdmo.accounts.middleware.TermsAndConditionsRedirectMiddleware' not in settings.MIDDLEWARE: errors.append( Error( "When ACCOUNT_TERMS_OF_USE is enabled, " - "ACCOUNT_TERMS_OF_USE_MIDDLEWARE needs to be added to the middlewares.", - hint=f"add '{settings.ACCOUNT_TERMS_OF_USE_MIDDLEWARE}' to MIDDLEWARE", + "The TermsAndConditionsRedirectMiddleware is missing from the middlewares.", + hint="add rdmo.accounts.middleware.TermsAndConditionsRedirectMiddleware to MIDDLEWARE", id="core.E001", ) ) diff --git a/rdmo/core/settings.py b/rdmo/core/settings.py index a857fd647f..ba969f27a9 100644 --- a/rdmo/core/settings.py +++ b/rdmo/core/settings.py @@ -54,7 +54,8 @@ 'django.contrib.auth.middleware.AuthenticationMiddleware', 'django.contrib.messages.middleware.MessageMiddleware', 'django.middleware.clickjacking.XFrameOptionsMiddleware', - 'django.contrib.sites.middleware.CurrentSiteMiddleware' + 'django.contrib.sites.middleware.CurrentSiteMiddleware', + 'rdmo.accounts.middleware.TermsAndConditionsRedirectMiddleware' ] ROOT_URLCONF = 'config.urls' @@ -101,7 +102,6 @@ ACCOUNT_GROUPS = [] ACCOUNT_TERMS_OF_USE = False -ACCOUNT_TERMS_OF_USE_MIDDLEWARE = 'rdmo.accounts.middleware.TermsAndConditionsRedirectMiddleware' ACCOUNT_TERMS_OF_USE_DATE = None # None or a valid date string ACCOUNT_TERMS_OF_USE_EXCLUDE_URL_PREFIXES = ("/admin", "/i18n", "/static", "/account") ACCOUNT_TERMS_OF_USE_EXCLUDE_URLS = ("/",) # is LOGOUT_URL needed here? From 28ecef09f7e0cebedd440bbac220d61e05925b92 Mon Sep 17 00:00:00 2001 From: David Wallace Date: Mon, 17 Feb 2025 16:46:33 +0100 Subject: [PATCH 15/17] fix(core,utils): use input date arg Signed-off-by: David Wallace --- rdmo/core/utils.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/rdmo/core/utils.py b/rdmo/core/utils.py index 769f560b8c..c3c4d2d4aa 100644 --- a/rdmo/core/utils.py +++ b/rdmo/core/utils.py @@ -428,9 +428,12 @@ def remove_double_newlines(string): def parse_date_from_string(date: str) -> datetime.date: + if not isinstance(date, str): + raise TypeError("date must be provided as string") + try: # First, try standard ISO format (YYYY-MM-DD) - parsed_date = parse_date(settings.ACCOUNT_TERMS_OF_USE_DATE) + parsed_date = parse_date(date) except ValueError as exc: raise exc from exc From 356f92e9479d78655f3a9980a7d71a9de57047c5 Mon Sep 17 00:00:00 2001 From: David Wallace Date: Mon, 17 Feb 2025 17:06:04 +0100 Subject: [PATCH 16/17] tests(core,utils): add tests for the date parser Signed-off-by: David Wallace --- rdmo/core/tests/test_utils.py | 40 ++++++++++++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/rdmo/core/tests/test_utils.py b/rdmo/core/tests/test_utils.py index 05fed48bbb..cb8c71bb39 100644 --- a/rdmo/core/tests/test_utils.py +++ b/rdmo/core/tests/test_utils.py @@ -1,8 +1,11 @@ +import datetime from typing import Optional import pytest -from rdmo.core.utils import human2bytes, join_url, sanitize_url +from django.utils.translation import activate + +from rdmo.core.utils import human2bytes, join_url, parse_date_from_string, sanitize_url urls = ( ('', ''), @@ -23,6 +26,25 @@ ("0", 0), ) +valid_date_strings = [ + ("en", "2025-02-17", datetime.date(2025, 2, 17)), + ("en", "Feb 17, 2025", datetime.date(2025, 2, 17)), + ("de", "17.02.2025", datetime.date(2025, 2, 17)), + ("fr", "17/02/2025", datetime.date(2025, 2, 17)), + ("nl", "17-02-2025", datetime.date(2025, 2, 17)), + ("es", "17/02/25", datetime.date(2025, 2, 17)), +] + +invalid_date_strings = [ + ("2025-02-31","day is out of range for month"), + ("2025-17-02", "month must be in 1..12"), + ("99/99/9999", "Invalid date format"), + ("abcd-ef-gh", "Invalid date format"), + ("32-13-2024", "Invalid date format"), + ("", "Invalid date format"), + (None, "date must be provided as string"), + (datetime.date(2025, 2, 17), "date must be provided as string") +] @pytest.mark.parametrize('url,sanitized_url', urls) def test_sanitize_url(url, sanitized_url): @@ -36,3 +58,19 @@ def test_join_url(): @pytest.mark.parametrize('human,bytes', human2bytes_test_values) def test_human2bytes(human: Optional[str], bytes: float): assert human2bytes(human) == bytes + + +@pytest.mark.parametrize("locale, date_string, expected_date",valid_date_strings) +def test_parse_date_from_string_valid_formats(settings, locale, date_string, expected_date): + activate(locale) + assert parse_date_from_string(date_string) == expected_date + + +@pytest.mark.parametrize("invalid_date, error_msg",invalid_date_strings) +def test_parse_date_from_string_invalid_formats(settings, invalid_date, error_msg): + if not isinstance(invalid_date,str): + with pytest.raises(TypeError, match=error_msg): + parse_date_from_string(invalid_date) + else: + with pytest.raises(ValueError,match=error_msg): + parse_date_from_string(invalid_date) From b9cdafc236195f7ce28e40a63868bc6199a33a2d Mon Sep 17 00:00:00 2001 From: David Wallace Date: Mon, 17 Feb 2025 17:21:11 +0100 Subject: [PATCH 17/17] refactor(accounts): rename adapter.py to account.py Signed-off-by: David Wallace --- rdmo/accounts/{adapter.py => account.py} | 0 rdmo/core/settings.py | 6 +++--- rdmo/core/views.py | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) rename rdmo/accounts/{adapter.py => account.py} (100%) diff --git a/rdmo/accounts/adapter.py b/rdmo/accounts/account.py similarity index 100% rename from rdmo/accounts/adapter.py rename to rdmo/accounts/account.py diff --git a/rdmo/core/settings.py b/rdmo/core/settings.py index ba969f27a9..ef5534716a 100644 --- a/rdmo/core/settings.py +++ b/rdmo/core/settings.py @@ -107,10 +107,10 @@ ACCOUNT_TERMS_OF_USE_EXCLUDE_URLS = ("/",) # is LOGOUT_URL needed here? ACCOUNT_TERMS_OF_USE_EXCLUDE_URL_CONTAINS = [] -ACCOUNT_ADAPTER = 'rdmo.accounts.adapter.AccountAdapter' +ACCOUNT_ADAPTER = 'rdmo.accounts.account.AccountAdapter' ACCOUNT_FORMS = { - 'login': 'rdmo.accounts.adapter.LoginForm', - 'signup': 'rdmo.accounts.adapter.SignupForm' + 'login': 'rdmo.accounts.account.LoginForm', + 'signup': 'rdmo.accounts.account.SignupForm' } ACCOUNT_USER_DISPLAY = 'rdmo.accounts.utils.get_full_name' ACCOUNT_EMAIL_REQUIRED = True diff --git a/rdmo/core/views.py b/rdmo/core/views.py index ca2fad62b0..401eaa2702 100644 --- a/rdmo/core/views.py +++ b/rdmo/core/views.py @@ -32,7 +32,7 @@ def home(request): else: if settings.LOGIN_FORM: if settings.ACCOUNT or settings.SOCIALACCOUNT: - from rdmo.accounts.adapter import LoginForm + from rdmo.accounts.account import LoginForm return render(request, 'core/home.html', { 'form': LoginForm(), 'signup_url': reverse("account_signup")