diff --git a/core/common/backends.py b/core/common/backends.py index 0a7fae8b..6f475866 100644 --- a/core/common/backends.py +++ b/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,7 +90,22 @@ 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') @@ -92,7 +113,25 @@ def filter_users_by_claims(self, claims): 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): diff --git a/core/common/exceptions.py b/core/common/exceptions.py index f7ae38c7..48456232 100644 --- a/core/common/exceptions.py +++ b/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). diff --git a/core/common/tests.py b/core/common/tests.py index b5975357..f676098a 100644 --- a/core/common/tests.py +++ b/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): diff --git a/core/middlewares/middlewares.py b/core/middlewares/middlewares.py index b7d72a76..4328c7c4 100644 --- a/core/middlewares/middlewares.py +++ b/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 diff --git a/core/middlewares/tests.py b/core/middlewares/tests.py index 827f4894..0df977df 100644 --- a/core/middlewares/tests.py +++ b/core/middlewares/tests.py @@ -5,7 +5,9 @@ from django.contrib.auth.models import AnonymousUser from django.http import HttpResponse from django.test import RequestFactory, SimpleTestCase, override_settings +from rest_framework.exceptions import AuthenticationFailed +from core.common.exceptions import DeactivatedAccountLoginRefused from core.middlewares.middlewares import RequireAuthenticationMiddleware @@ -60,6 +62,26 @@ def test_blocks_request_when_drf_header_auth_fails(self, authenticate_mock): self.assertEqual(response.status_code, 403) + @patch('core.middlewares.middlewares.OCLAuthentication.authenticate') + def test_blocks_request_when_drf_header_auth_raises(self, authenticate_mock): + """An invalid header credential reads as anonymous access.""" + authenticate_mock.side_effect = AuthenticationFailed('Login failed') + + response = self.middleware(self.make_request('/orgs/OCL/', HTTP_AUTHORIZATION='Bearer bad-token')) + + self.assertEqual(response.status_code, 403) + + @patch('core.middlewares.middlewares.OCLAuthentication.authenticate') + def test_rejects_sign_in_refused_for_deactivated_account(self, authenticate_mock): + """A sign-in refused for a deactivated account gets a 401 that says why, not the anonymous 403.""" + authenticate_mock.side_effect = DeactivatedAccountLoginRefused() + + response = self.middleware(self.make_request('/user/', HTTP_AUTHORIZATION='Bearer sso-token')) + + self.assertEqual(response.status_code, 401) + self.assertEqual(response['WWW-Authenticate'], 'Bearer realm="api"') + self.assertIn('deactivated OCL account', json.loads(response.content)['detail']) + def test_blocks_anonymous_request_for_protected_path(self): """Anonymous traffic to protected API paths should receive a 403 response.""" response = self.middleware(self.make_request('/orgs/OCL/'))