OpenConceptLab/ocl_online#339 | Sign-in: don't map a Keycloak identity onto a deactivated account unless the verified email matches

core/common/backends.py

@@ -1,3 +1,5 @@
+import logging
+
 from celery_once.backends import Redis
 from django.conf import settings
 from django.contrib.auth.backends import ModelBackend
@@ -6,6 +8,10 @@
 from pydash import get
 from redis import Sentinel
 
+from core.common.exceptions import DeactivatedAccountLoginRefused
+
+logger = logging.getLogger('oclapi')
+
 
 class QueueOnceRedisSentinelBackend(Redis):
     def __init__(self, backend_settings):
@@ -84,15 +90,48 @@ def update_user(self, user, claims):
 
         return user
 
+    def authenticate(self, request, **kwargs):
+        try:
+            return super().authenticate(request, **kwargs)
+        except DeactivatedAccountLoginRefused:
+            # the refused identity's tokens were stored in the session before its account was looked up
+            for key in ('oidc_access_token', 'oidc_id_token'):
+                request.session.pop(key, None)
+            return None
+
     def filter_users_by_claims(self, claims):
+        """
+        Matches the account with the claims' username, exactly as Keycloak sends it. A deactivated account is never
+        matched on its username alone (ocl_online#339): it's reactivated when the claims carry its email, verified by
+        Keycloak, and otherwise the sign-in is refused and the account left untouched. The username can't go to a new
+        account either, since usernames are unique.
+        """
         from core.users.models import UserProfile
 
         username = claims.get('preferred_username')
 
         if not username:
             return UserProfile.objects.none()
 
-        return UserProfile.objects.filter(username=username)
+        # evaluated once, so the row checked here is the one the caller updates
+        users = UserProfile.objects.filter(username=username)
+        user = users[0] if users else None
+        if user and not user.is_active:
+            if not self.has_verified_email_of(user, claims):
+                logger.warning(
+                    'OIDC sign-in refused: user %s is deactivated and the claims lack its verified email', user.id)
+                raise DeactivatedAccountLoginRefused()
+            user.undelete()
+            logger.warning('OIDC sign-in reactivated deactivated user %s on its verified email', user.id)
+
+        return users
+
+    @staticmethod
+    def has_verified_email_of(user, claims):
+        email = claims.get('email')
+        if claims.get('email_verified') is not True or not isinstance(email, str) or not email.strip():
+            return False
+        return email.strip().lower() == (user.email or '').strip().lower()
 
 
 class OCLAuthenticationBackend(ModelBackend):

core/common/exceptions.py

@@ -1,6 +1,7 @@
-from rest_framework import status
-from rest_framework.exceptions import APIException
+from django.conf import settings
 from django.utils.translation import gettext_lazy as _
+from rest_framework import status
+from rest_framework.exceptions import APIException, AuthenticationFailed
 
 
 class Http409(APIException):
@@ -27,6 +28,20 @@ class Http403(APIException):
     default_code = 'forbidden'
 
 
+class DeactivatedAccountLoginRefused(AuthenticationFailed):
+    """
+    Raised when a Keycloak sign-in's username matches a deactivated account but the claims don't carry that
+    account's email, verified by Keycloak (ocl_online#339). The account is left as it was; staff can reactivate it.
+    """
+    default_code = 'deactivated_account'
+
+    def __init__(self):
+        super().__init__(
+            'This username belongs to a deactivated OCL account. If it is yours, contact '
+            f'{settings.COMMUNITY_EMAIL} and we will restore it.'
+        )
+
+
 class BatchIndexingError(Exception):
     """
     Raised once a batched ES indexing run has attempted every batch and one or more failed (ocl_online#241).

core/common/tests.py

@@ -14,7 +14,8 @@
 from django.contrib.auth.models import AnonymousUser, Group
 from django.core.files.base import File
 from django.core.management import call_command
-from django.test import TestCase, override_settings
+from django.http import HttpResponse
+from django.test import TestCase, override_settings, RequestFactory
 from django.test.runner import DiscoverRunner
 from django.utils import timezone
 from mock.mock import call
@@ -24,11 +25,12 @@
 from rest_framework.test import APITestCase, APITransactionTestCase
 
 from core.collections.models import CollectionReference, Expansion
-from core.collections.tests.factories import ExpansionFactory, OrganizationCollectionFactory
+from core.collections.tests.factories import ExpansionFactory, OrganizationCollectionFactory, \
+    UserCollectionFactory
 from core.common.checksums import VersionCompareMixin, ChecksumDiff
 from core.common.constants import HEAD
 from core.common.es import ESScript
-from core.common.exceptions import BatchIndexingError
+from core.common.exceptions import BatchIndexingError, DeactivatedAccountLoginRefused
 from core.common.models import BaseModel
 from core.common.tasks import delete_s3_objects, bulk_import_parallel_inline, resources_report, calculate_checksums, \
     delete_organization, delete_source, delete_collection, add_references, handle_m2m_changed, handle_pre_delete, \
@@ -64,6 +66,7 @@
 from core.concepts.documents import ConceptDocument
 from core.concepts.models import Concept
 from core.mappings.documents import MappingDocument
+from core.middlewares.middlewares import RequireAuthenticationMiddleware
 from core.orgs.models import Organization
 from core.sources.models import Source
 from core.users.constants import CORE_USER_GROUP, GUEST_GROUP
@@ -79,7 +82,7 @@
 from ..concepts.tests.factories import ConceptFactory, ConceptNameFactory
 from ..mappings.serializers import MappingDetailSerializer
 from ..mappings.tests.factories import MappingFactory
-from ..sources.tests.factories import OrganizationSourceFactory
+from ..sources.tests.factories import OrganizationSourceFactory, UserSourceFactory
 
 PREVIEW_GROUP_NAME = 'preview'
 TEST_GROUPS_CONFIG_FILE = 'core/capabilities/tests/groups.test.yaml'
@@ -2565,6 +2568,199 @@ def test_filter_users_by_claims(self):
 
         self.assertEqual(self.backend.filter_users_by_claims({**self.claim, 'preferred_username': None}).count(), 0)
 
+    def test_filter_users_by_claims_matches_username_exactly(self):
+        """Keycloak sends usernames in lowercase, and a mixed-case account is never matched, active or not"""
+        UserProfileFactory(username='Batman', email='batman@gotham.com')
+        robin = UserProfileFactory(username='Robin', email='robin@gotham.com')
+        robin.deactivate()
+
+        self.assertEqual(self.backend.filter_users_by_claims(self.claim).count(), 0)
+        self.assertEqual(
+            self.backend.filter_users_by_claims(
+                {**self.claim, 'preferred_username': 'robin', 'email': 'robin@gotham.com'}).count(),
+            0
+        )
+        robin.refresh_from_db()
+        self.assertFalse(robin.is_active)
+
+    def test_filter_users_by_claims_reactivates_deactivated_user_with_its_verified_email(self):
+        batman = UserProfileFactory(username='batman', email='Batman@Gotham.com ')
+        source = UserSourceFactory(user=batman)
+        collection = UserCollectionFactory(user=batman)
+        batman.deactivate()
+        source.refresh_from_db()
+        collection.refresh_from_db()
+        self.assertFalse(source.is_active)
+        self.assertFalse(collection.is_active)
+
+        users = self.backend.filter_users_by_claims(self.claim)
+
+        self.assertEqual(list(users), [batman])
+        self.assertTrue(users[0].is_active)
+        batman.refresh_from_db()
+        self.assertTrue(batman.is_active)
+        self.assertTrue(batman.verified)
+        self.assertIsNone(batman.deactivated_at)
+        self.assertEqual(batman.status, 'verified')
+        source.refresh_from_db()
+        collection.refresh_from_db()
+        self.assertTrue(source.is_active)
+        self.assertTrue(collection.is_active)
+
+    def test_filter_users_by_claims_refuses_deactivated_user_without_its_verified_email(self):
+        batman = UserProfileFactory(username='batman', email='batman@gotham.com', first_name='Bat')
+        source = UserSourceFactory(user=batman)
+        collection = UserCollectionFactory(user=batman)
+        batman.deactivate()
+        batman.refresh_from_db()
+        deactivated_at = batman.deactivated_at
+        updated_at = batman.updated_at
+        without_email_verified = {key: value for key, value in self.claim.items() if key != 'email_verified'}
+
+        for claims in [
+                {**self.claim, 'email_verified': False},
+                {**self.claim, 'email_verified': 'true'},
+                without_email_verified,
+                {**self.claim, 'email': 'joker@gotham.com'},
+                {**self.claim, 'email': None},
+                {**self.claim, 'email': 42},
+                {**self.claim, 'email': ['batman@gotham.com']},
+        ]:
+            with self.subTest(claims=claims):
+                with patch('core.users.models.UserProfile.save') as save_mock:
+                    with self.assertRaises(DeactivatedAccountLoginRefused):
+                        self.backend.filter_users_by_claims(claims)
+                save_mock.assert_not_called()
+
+                batman.refresh_from_db()
+                self.assertFalse(batman.is_active)
+                self.assertFalse(batman.verified)
+                self.assertEqual(batman.email, 'batman@gotham.com')
+                self.assertEqual(batman.first_name, 'Bat')
+                self.assertEqual(batman.deactivated_at, deactivated_at)
+                self.assertEqual(batman.updated_at, updated_at)
+                source.refresh_from_db()
+                collection.refresh_from_db()
+                self.assertFalse(source.is_active)
+                self.assertFalse(collection.is_active)
+
+    def test_filter_users_by_claims_looks_up_the_user_once(self):
+        UserProfileFactory(username='batman')
+
+        with self.assertNumQueries(1):
+            users = self.backend.filter_users_by_claims(self.claim)
+            self.assertEqual(len(users), 1)
+            self.assertEqual(users[0].username, 'batman')
+
+    def test_filter_users_by_claims_refuses_deactivated_user_without_email(self):
+        batman = UserProfileFactory(username='batman', email='')
+        batman.deactivate()
+
+        with self.assertRaises(DeactivatedAccountLoginRefused):
+            self.backend.filter_users_by_claims({**self.claim, 'email': ''})
+
+        batman.refresh_from_db()
+        self.assertFalse(batman.is_active)
+
+    @patch('core.common.backends.OCLOIDCAuthenticationBackend.get_userinfo')
+    def test_get_or_create_user_for_active_user(self, get_userinfo_mock):
+        get_userinfo_mock.return_value = self.claim
+        batman = UserProfileFactory(username='batman', email='old@gotham.com', first_name='Bat')
+
+        user = self.backend.get_or_create_user('access-token', None, None)
+
+        self.assertEqual(user, batman)
+        batman.refresh_from_db()
+        self.assertTrue(batman.is_active)
+        self.assertEqual(batman.email, 'batman@gotham.com')
+        self.assertEqual(batman.first_name, 'Bruce')
+
+    @patch('core.common.backends.OCLOIDCAuthenticationBackend.get_userinfo')
+    def test_get_or_create_user_for_deactivated_user(self, get_userinfo_mock):
+        batman = UserProfileFactory(username='batman', email='batman@gotham.com', first_name='Bat')
+        batman.deactivate()
+
+        get_userinfo_mock.return_value = {**self.claim, 'email': 'joker@gotham.com'}
+        with patch.object(self.backend, 'update_user') as update_user_mock, \
+                patch.object(self.backend, 'create_user') as create_user_mock:
+            with self.assertRaises(DeactivatedAccountLoginRefused):
+                self.backend.get_or_create_user('access-token', None, None)
+        update_user_mock.assert_not_called()
+        create_user_mock.assert_not_called()
+        self.assertEqual(UserProfile.objects.filter(username__iexact='batman').count(), 1)
+        batman.refresh_from_db()
+        self.assertFalse(batman.is_active)
+        self.assertEqual(batman.email, 'batman@gotham.com')
+        self.assertEqual(batman.first_name, 'Bat')
+
+        get_userinfo_mock.return_value = self.claim
+        user = self.backend.get_or_create_user('access-token', None, None)
+
+        self.assertEqual(user, batman)
+        batman.refresh_from_db()
+        self.assertTrue(batman.is_active)
+        self.assertEqual(batman.first_name, 'Bruce')
+
+    @patch('core.common.backends.OCLOIDCAuthenticationBackend.get_userinfo')
+    def test_get_or_create_user_leaves_mixed_case_deactivated_user(self, get_userinfo_mock):
+        get_userinfo_mock.return_value = self.claim
+        old_batman = UserProfileFactory(username='Batman', email='batman@gotham.com', first_name='Bat')
+        old_batman.deactivate()
+
+        user = self.backend.get_or_create_user('access-token', None, None)
+
+        self.assertNotEqual(user, old_batman)
+        self.assertEqual(user.username, 'batman')
+        self.assertTrue(user.is_active)
+        old_batman.refresh_from_db()
+        self.assertFalse(old_batman.is_active)
+        self.assertEqual(old_batman.first_name, 'Bat')
+
+    @patch('mozilla_django_oidc.auth.OIDCAuthenticationBackend.authenticate')
+    def test_authenticate_fails_and_drops_stored_tokens_when_sign_in_is_refused(self, authenticate_mock):
+        def store_tokens_then_refuse(request, **kwargs):  # pylint: disable=unused-argument
+            request.session['oidc_access_token'] = 'access-token'
+            request.session['oidc_id_token'] = 'id-token'
+            raise DeactivatedAccountLoginRefused()
+        authenticate_mock.side_effect = store_tokens_then_refuse
+        request = Mock(session={'other': 'kept'})
+
+        self.assertIsNone(self.backend.authenticate(request, nonce='nonce'))
+
+        authenticate_mock.assert_called_once()
+        self.assertEqual(request.session, {'other': 'kept'})
+
+    @override_settings(TEST_MODE=False, ES_SYNC=False, OIDC_SERVER_URL='https://sso.example.org')
+    @patch('core.common.backends.OCLOIDCAuthenticationBackend.get_userinfo')
+    def test_bearer_request_for_deactivated_user(self, get_userinfo_mock):
+        """Through the real middleware, DRF OIDC authentication and backend"""
+        batman = UserProfileFactory(username='batman', email='batman@gotham.com', first_name='Bat')
+        batman.deactivate()
+        middleware = RequireAuthenticationMiddleware(lambda request: HttpResponse('ok'))
+
+        def get(claims):
+            get_userinfo_mock.return_value = claims
+            request = RequestFactory().get('/user/', HTTP_AUTHORIZATION='Bearer sso-token')
+            request.user = AnonymousUser()
+            return request, middleware(request)
+
+        _, response = get({**self.claim, 'email': 'joker@gotham.com'})
+
+        self.assertEqual(response.status_code, 401)
+        self.assertIn('deactivated OCL account', json.loads(response.content)['detail'])
+        batman.refresh_from_db()
+        self.assertFalse(batman.is_active)
+        self.assertEqual(batman.email, 'batman@gotham.com')
+        self.assertEqual(batman.first_name, 'Bat')
+
+        request, response = get(self.claim)
+
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(request.user, batman)
+        batman.refresh_from_db()
+        self.assertTrue(batman.is_active)
+        self.assertEqual(batman.first_name, 'Bruce')
+
 
 class ChecksumTest(OCLTestCase):
     def test_generate(self):

core/middlewares/middlewares.py

@@ -15,6 +15,7 @@
 from core.common.authentication import OCLAuthentication
 from core.common.constants import VERSION_HEADER, REQUEST_USER_HEADER, RESPONSE_TIME_HEADER, REQUEST_URL_HEADER, \
     REQUEST_METHOD_HEADER
+from core.common.exceptions import DeactivatedAccountLoginRefused
 from core.common.throttling import ThrottleUtil
 from core.common.utils import set_current_user, set_request_url
 from core.services.analytics_event_emitter import AnalyticsEventEmitter
@@ -127,7 +128,15 @@ class RequireAuthenticationMiddleware(BaseMiddleware):
 
     def __call__(self, request):
         """Allow exempt and approved anonymous traffic, otherwise return 403."""
-        if self.is_request_allowed(request):
+        try:
+            is_allowed = self.is_request_allowed(request)
+        except DeactivatedAccountLoginRefused as ex:
+            # a refused sign-in says why, rather than reading as anonymous access (ocl_online#339)
+            response = JsonResponse({'detail': str(ex.detail)}, status=ex.status_code)
+            response['WWW-Authenticate'] = 'Bearer realm="api"'
+            return response
+
+        if is_allowed:
             return self.get_response(request)
 
         return JsonResponse(self.forbidden_response, status=403)
@@ -160,6 +169,8 @@ def get_authenticated_user(request):
 
         try:
             auth_result = OCLAuthentication().authenticate(Request(request))
+        except DeactivatedAccountLoginRefused:
+            raise
         except AuthenticationFailed:
             return user