Save date of password change and flag it as valid

hub/models.py

@@ -173,6 +173,8 @@ def save(
         using=None,
         update_fields=None,
     ):
+        created = self.pk is None
+
         if not update_fields or (update_fields and 'data' in update_fields):
             self.standardize_json_field('data', 'organization', str)
             self.standardize_json_field('data', 'name', str)
@@ -184,10 +186,17 @@ def save(
             update_fields=update_fields,
         )
 
-        # Sync validated_password field to KobocatUserProfile
-        if not settings.TESTING and (
-            not update_fields
-            or (update_fields and 'validated_password' in update_fields)
+        # Sync `validated_password` field to `KobocatUserProfile` only when
+        # this object is updated to avoid a race condition and an IntegrityError
+        # when trying to save `KobocatUserProfile` object whereas the related
+        # `KobocatUser` object has not been created yet.
+        if (
+            not settings.TESTING
+            and not created
+            and (
+                not update_fields
+                or (update_fields and 'validated_password' in update_fields)
+            )
         ):
             KobocatUserProfile.set_password_details(
                 self.user.id,

kobo/apps/accounts/adapter.py

@@ -6,6 +6,7 @@
 from django.db import transaction
 from django.shortcuts import resolve_url
 from django.template.response import TemplateResponse
+from django.utils import timezone
 from trench.utils import get_mfa_model, user_token_generator
 
 from .mfa.forms import MfaTokenForm
@@ -60,3 +61,13 @@ def save_user(self, request, user, form, commit=True):
             if commit:
                 user.extra_details.save()
         return user
+
+    def set_password(self, user, password):
+        with transaction.atomic():
+            user.extra_details.password_date_changed = timezone.now()
+            user.extra_details.validated_password = True
+            user.extra_details.save(
+                update_fields=['password_date_changed', 'validated_password']
+            )
+            user.set_password(password)
+            user.save()

kobo/apps/accounts/tests/test_current_user.py

@@ -1,22 +1,27 @@
 import json
+import re
+
 import constance
+from constance.test import override_config
+from django.core import mail
 from django.urls import reverse
+from django.utils import timezone
 from model_bakery import baker
 from rest_framework import status
 from rest_framework.test import APITestCase
 
 
 class CurrentUserAPITestCase(APITestCase):
     def setUp(self):
-        self.user = baker.make('auth.User')
+        self.user = baker.make('auth.User', username='spongebob', email='me@sponge.bob')
         self.client.force_login(self.user)
         self.url = reverse('currentuser-detail')
 
     def test_social_accounts(self):
         social_accounts = baker.make(
-            "socialaccount.SocialAccount", user=self.user, _quantity=2
+            'socialaccount.SocialAccount', user=self.user, _quantity=2
         )
-        other_social_account = baker.make("socialaccount.SocialAccount")
+        other_social_account = baker.make('socialaccount.SocialAccount')
         # This modifies the user account
         self.client.get(self.url)
         with self.assertNumQueries(4):
@@ -75,3 +80,45 @@ def test_validate_extra_detail(self):
         assert response.json() == {
             'extra_details': {'organization': 'This field may not be blank.'}
         }
+
+    # use `override_config` decorator to deactivate all password validators
+    # to let this test use a simple password.
+    @override_config(
+        ENABLE_PASSWORD_MINIMUM_LENGTH_VALIDATION=False,
+        ENABLE_PASSWORD_USER_ATTRIBUTE_SIMILARITY_VALIDATION=False,
+        ENABLE_MOST_RECENT_PASSWORD_VALIDATION=False,
+        ENABLE_COMMON_PASSWORD_VALIDATION=False,
+        ENABLE_PASSWORD_CUSTOM_CHARACTER_RULES_VALIDATION=False,
+    )
+    def test_validated_password_becomes_true_on_password_change(self):
+        self.user.extra_details.validated_password = False
+        self.user.extra_details.save(update_fields=['validated_password'])
+        assert self.user.extra_details.password_date_changed is None
+        data = {
+            'email': self.user.email,
+        }
+        self.client.post(reverse('account_reset_password'), data)
+        assert len(mail.outbox) == 1
+        assert mail.outbox[0].subject == 'KoboToolbox Password Reset'
+        reset_link = re.search(
+            r'(?P<url>http://testserver\S+)', mail.outbox[0].body
+        ).group('url')
+        response = self.client.get(reset_link, follow=True)
+        redirection, status_code = response.redirect_chain[0]
+        assert status_code == status.HTTP_302_FOUND
+
+        # Reset password
+        data = {
+            'password1': 'mypassword',
+            'password2': 'mypassword',
+        }
+        now = timezone.now()
+        response = self.client.post(redirection, data, follow=True)
+        redirection, status_code = response.redirect_chain[0]
+        assert status_code == status.HTTP_302_FOUND
+        self.user.refresh_from_db()
+
+        # Validate flag is back to True and password change has been tracked
+        assert self.user.extra_details.validated_password
+        assert self.user.extra_details.password_date_changed is not None
+        assert self.user.extra_details.password_date_changed >= now

kobo/apps/accounts/tests/test_social_account.py

@@ -1,5 +1,3 @@
-from allauth.account.models import EmailAddress
-from django.core import mail
 from django.urls import reverse
 from model_bakery import baker
 from rest_framework.test import APITestCase

kpi/deployment_backends/kc_access/shadow_models.py

@@ -130,20 +130,15 @@ def sync(cls, user):
         but updates `KobocatDigestPartial` in the KoBoCAT database instead of
         `PartialDigest` in the KPI database
         """
-        # Because of circular imports, we cannot decorate the method with
-        # `@kc_transaction_atomic`
-        from .utils import kc_transaction_atomic
-
-        with kc_transaction_atomic():
-            cls.objects.filter(user=user).delete()
-            # Query for `user_id` since user PKs are synchronized
-            for partial_digest in PartialDigest.objects.filter(user_id=user.pk):
-                cls.objects.create(
-                    user=user,
-                    login=partial_digest.login,
-                    confirmed=partial_digest.confirmed,
-                    partial_digest=partial_digest.partial_digest,
-                )
+        cls.objects.filter(user=user).delete()
+        # Query for `user_id` since user PKs are synchronized
+        for partial_digest in PartialDigest.objects.filter(user_id=user.pk):
+            cls.objects.create(
+                user=user,
+                login=partial_digest.login,
+                confirmed=partial_digest.confirmed,
+                partial_digest=partial_digest.partial_digest,
+            )
 
 
 class KobocatGenericForeignKey(GenericForeignKey):
@@ -384,7 +379,6 @@ class Meta(ShadowModel.Meta):
     # is using `LazyBooleanField` which is an integer behind the scene.
     # We do not want to port this class to KPI only for one line of code.
     is_mfa_active = models.PositiveSmallIntegerField(default=False)
-    password_date_changed = models.DateTimeField(null=True, blank=True)
     validated_password = models.BooleanField(default=False)
 
     @classmethod
@@ -396,8 +390,8 @@ def set_mfa_status(cls, user_id: int, is_active: bool):
 
     @classmethod
     def set_password_details(
-        cls, 
-        user_id: int, 
+        cls,
+        user_id: int,
         validated: bool,
     ):
         """

kpi/serializers/current_user.py

@@ -12,6 +12,8 @@
 from django.contrib.auth.password_validation import validate_password
 from django.conf import settings
 from django.core.exceptions import ValidationError as DjangoValidationError
+from django.db import transaction
+from django.utils import timezone
 from django.utils.translation import gettext as t
 from rest_framework import serializers
 
@@ -33,7 +35,7 @@ class CurrentUserSerializer(serializers.ModelSerializer):
     new_password = serializers.CharField(write_only=True, required=False)
     git_rev = serializers.SerializerMethodField()
     social_accounts = SocialAccountSerializer(
-        source="socialaccount_set", many=True, read_only=True
+        source='socialaccount_set', many=True, read_only=True
     )
 
     class Meta:
@@ -190,34 +192,47 @@ def update(self, instance, validated_data):
         # "The `.update()` method does not support writable dotted-source
         # fields by default." --DRF
         extra_details = validated_data.pop('extra_details', False)
-        if extra_details:
-            extra_details_obj, created = ExtraUserDetail.objects.get_or_create(
-                user=instance
-            )
-            if (
-                settings.KOBOCAT_URL
-                and settings.KOBOCAT_INTERNAL_URL
-                and 'require_auth' in extra_details['data']
-            ):
-                # `require_auth` needs to be written back to KC
-                set_kc_require_auth(
-                    instance.pk, extra_details['data']['require_auth']
+        new_password = validated_data.get('new_password', False)
+
+        extra_details_obj = None
+        with transaction.atomic():
+            if extra_details:
+                extra_details_obj, _ = ExtraUserDetail.objects.get_or_create(
+                    user=instance
                 )
+                if (
+                    settings.KOBOCAT_URL
+                    and settings.KOBOCAT_INTERNAL_URL
+                    and 'require_auth' in extra_details['data']
+                ):
+                    # `require_auth` needs to be written back to KC
+                    set_kc_require_auth(
+                        instance.pk, extra_details['data']['require_auth']
+                    )
 
-            # This is a PATCH, so retain existing values for keys that were not
-            # included in the request
-            extra_details_obj.data.update(extra_details['data'])
+                # This is a PATCH, so retain existing values for keys that were
+                # not included in the request
+                extra_details_obj.data.update(extra_details['data'])
+
+            if new_password:
+                instance.set_password(new_password)
+                instance.save()
+                request = self.context.get('request', False)
+                if request:
+                    update_session_auth_hash(request, instance)
+
+                # If `extra_details_obj` does not already exist, let's retrieve
+                # (or create) it to track password changes
+                if not extra_details_obj:
+                    extra_details_obj, _ = ExtraUserDetail.objects.get_or_create(
+                        user=instance
+                    )
+                extra_details_obj.password_date_changed = timezone.now()
+                extra_details_obj.validated_password = True
 
-            # Save to the database at last
-            extra_details_obj.save()
+            # if `extra_details_obj` exists, it needs to be saved to persist
+            # user's extra details changes.
+            if extra_details_obj:
+                extra_details_obj.save()
 
-        new_password = validated_data.get('new_password', False)
-        if new_password:
-            instance.set_password(new_password)
-            instance.save()
-            request = self.context.get('request', False)
-            if request:
-                update_session_auth_hash(request, instance)
-
-        return super().update(
-            instance, validated_data)
+            return super().update(instance, validated_data)

kpi/signals.py

@@ -10,8 +10,12 @@
 from kpi.deployment_backends.kc_access.shadow_models import (
     KobocatToken,
     KobocatUser,
+    KobocatUserProfile,
+)
+from kpi.deployment_backends.kc_access.utils import (
+    grant_kc_model_level_perms,
+    kc_transaction_atomic,
 )
-from kpi.deployment_backends.kc_access.utils import grant_kc_model_level_perms
 from kpi.models import Asset, TagUid
 from kpi.utils.permissions import grant_default_model_level_perms
 
@@ -42,16 +46,10 @@ def save_kobocat_user(sender, instance, created, raw, **kwargs):
     `settings.KOBOCAT_DEFAULT_PERMISSION_CONTENT_TYPES`
     """
     if not settings.TESTING:
-        KobocatUser.sync(instance)
-
-        if created:
-            # FIXME: If this fails, the next attempt results in
-            #   IntegrityError: duplicate key value violates unique constraint
-            #   "auth_user_username_key"
-            # and decorating this function with `transaction.atomic` doesn't
-            # seem to help. We should roll back the KC user creation if
-            # assigning model-level permissions fails
-            grant_kc_model_level_perms(instance)
+        with kc_transaction_atomic():
+            KobocatUser.sync(instance)
+            if created:
+                grant_kc_model_level_perms(instance)
 
 
 @receiver(post_save, sender=Token)

kpi/tests/api/test_api_current_user.py

@@ -3,6 +3,7 @@
 from constance.test import override_config
 from django.contrib.auth.models import User
 from django.urls import reverse
+from django.utils import timezone
 from rest_framework import status
 
 from kpi.tests.base_test_case import BaseTestCase
@@ -115,3 +116,30 @@ def test_password_is_validated_with_django(self):
             'You cannot use your last password.'
         }
         assert errors == {str(e) for e in response.data['new_password']}
+
+    @override_config(
+        ENABLE_PASSWORD_MINIMUM_LENGTH_VALIDATION=False,
+        ENABLE_PASSWORD_USER_ATTRIBUTE_SIMILARITY_VALIDATION=False,
+        ENABLE_MOST_RECENT_PASSWORD_VALIDATION=False,
+        ENABLE_COMMON_PASSWORD_VALIDATION=False,
+        ENABLE_PASSWORD_CUSTOM_CHARACTER_RULES_VALIDATION=False,
+    )
+    def test_validated_password_becomes_true_on_password_change(self):
+
+        password = 'delete_me'
+        self.user.extra_details.validated_password = False
+        self.user.extra_details.save(update_fields=['validated_password'])
+        assert self.user.extra_details.password_date_changed is None
+        now = timezone.now()
+        self.client.login(username='delete_me', password=password)
+        payload = {
+            'current_password': password,
+            'new_password': password,
+        }
+        response = self.client.patch(self.url, data=payload, format='json')
+        assert response.status_code == status.HTTP_200_OK
+
+        self.user.refresh_from_db()
+        assert self.user.extra_details.validated_password
+        assert self.user.extra_details.password_date_changed is not None
+        assert self.user.extra_details.password_date_changed >= now