From 22b63d480be9033575b461a98224fe87ce334167 Mon Sep 17 00:00:00 2001 From: David Wallace Date: Mon, 3 Feb 2025 19:12:30 +0100 Subject: [PATCH] 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 %} + + {% 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()