diff --git a/core/celery.py b/core/celery.py index 9047b74a..8495b8f4 100644 --- a/core/celery.py +++ b/core/celery.py @@ -37,6 +37,10 @@ 'task': 'core.common.tasks.rerun_indexing_job', 'schedule': timedelta(minutes=15), }, + 'sync-keycloak-groups': { + 'task': 'core.common.tasks.sync_keycloak_groups', + 'schedule': timedelta(minutes=15), + }, } app.config_from_object('django.conf:settings', namespace='CELERY') app.autodiscover_tasks(lambda: settings.INSTALLED_APPS) diff --git a/core/common/backends.py b/core/common/backends.py index 6f475866..de7c1201 100644 --- a/core/common/backends.py +++ b/core/common/backends.py @@ -8,7 +8,7 @@ from pydash import get from redis import Sentinel -from core.common.exceptions import DeactivatedAccountLoginRefused +from core.common.exceptions import DeactivatedAccountLoginRefused, SSOIdentityMismatchRefused logger = logging.getLogger('oclapi') @@ -62,6 +62,7 @@ def create_user(self, claims): from core.users.models import UserProfile user = UserProfile.objects.create_user( claims.get('preferred_username'), + sso_id=claims.get('sub'), email=claims.get('email'), first_name=claims.get('given_name'), last_name=claims.get('family_name'), @@ -78,6 +79,7 @@ def create_user(self, claims): def update_user(self, user, claims): user.first_name = claims.get('given_name') or user.first_name user.last_name = claims.get('family_name') or user.last_name + user.sso_id = user.sso_id or claims.get('sub') user.email = claims.get('email') or user.email user.company = claims.get('company', None) or user.company user.location = claims.get('location', None) or user.location @@ -85,7 +87,9 @@ def update_user(self, user, claims): user.set_groups(claims.get('groups', []), _save=False) if user.is_dirty(): + user._from_sso = True # pylint: disable=protected-access user.save() + user._from_sso = False # pylint: disable=protected-access user.set_checksums() return user @@ -93,7 +97,7 @@ def update_user(self, user, claims): def authenticate(self, request, **kwargs): try: return super().authenticate(request, **kwargs) - except DeactivatedAccountLoginRefused: + except (DeactivatedAccountLoginRefused, SSOIdentityMismatchRefused): # 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) @@ -101,7 +105,8 @@ def authenticate(self, request, **kwargs): def filter_users_by_claims(self, claims): """ - Matches the account with the claims' username, exactly as Keycloak sends it. A deactivated account is never + Matches the account linked to the claims' Keycloak id (`sub`), else the one with the claims' username exactly + as Keycloak sends it, unless that account is linked to a different Keycloak user. 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. @@ -113,8 +118,14 @@ def filter_users_by_claims(self, claims): if not username: return UserProfile.objects.none() + sub = claims.get('sub') # evaluated once, so the row checked here is the one the caller updates - users = UserProfile.objects.filter(username=username) + users = UserProfile.objects.filter(sso_id=sub) if sub else UserProfile.objects.none() + if not users: + users = UserProfile.objects.filter(username=username) + if users and sub and users[0].sso_id not in (None, sub): + logger.warning('OIDC sign-in refused: user %s is linked to another Keycloak user', users[0].id) + raise SSOIdentityMismatchRefused() user = users[0] if users else None if user and not user.is_active: if not self.has_verified_email_of(user, claims): diff --git a/core/common/exceptions.py b/core/common/exceptions.py index 48456232..0fc070e3 100644 --- a/core/common/exceptions.py +++ b/core/common/exceptions.py @@ -42,6 +42,17 @@ def __init__(self): ) +class SSOIdentityMismatchRefused(AuthenticationFailed): + """Raised when a Keycloak sign-in's username matches an account already linked to a different Keycloak user.""" + default_code = 'sso_identity_mismatch' + + def __init__(self): + super().__init__( + 'This username belongs to an OCL account linked to a different sign-in identity. ' + f'Contact {settings.COMMUNITY_EMAIL} for help.' + ) + + 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/management/commands/backfill_sso_ids.py b/core/common/management/commands/backfill_sso_ids.py new file mode 100644 index 00000000..060cdd45 --- /dev/null +++ b/core/common/management/commands/backfill_sso_ids.py @@ -0,0 +1,54 @@ +from django.core.management import BaseCommand +from django.db.models import Count +from django.db.models.functions import Lower + +from core.services.auth.openid import OpenIDAuthService +from core.users.models import UserProfile + + +class Command(BaseCommand): + help = ('Links active accounts without an sso_id to the Keycloak user with exactly their username. A mixed-case ' + 'account links to the Keycloak user with its lowercased username when no other account has that username ' + 'in another case and the emails match. Every link and every skip is listed.') + + def add_arguments(self, parser): + parser.add_argument('--dry-run', action='store_true') + + def handle(self, *args, **options): + keycloak_users = {user['username']: user for user in OpenIDAuthService.get_users()} + linked = set(UserProfile.objects.filter(sso_id__isnull=False).values_list('sso_id', flat=True)) + case_twins = set(UserProfile.objects.order_by().values(lower=Lower('username')).annotate( + count=Count('id')).filter(count__gt=1).values_list('lower', flat=True)) + links, skipped = [], [] + for user in UserProfile.objects.filter(is_active=True, sso_id__isnull=True).order_by('username'): + kc_user, reason = self.match(user, keycloak_users, case_twins) + if kc_user and kc_user['id'] in linked: + kc_user, reason = None, 'its Keycloak user is linked to another account' + if not kc_user: + skipped.append(f'{user.username} ({reason})') + continue + linked.add(kc_user['id']) + links.append(f"{user.username} -> {kc_user['username']}") + if not options['dry_run']: + UserProfile.objects.filter(id=user.id, sso_id__isnull=True).update(sso_id=kc_user['id']) + verb = 'would be linked' if options['dry_run'] else 'linked' + self.stdout.write(f'{len(links)} account(s) {verb}, {len(skipped)} skipped') + for link in links: + self.stdout.write(f' {verb}: {link}') + for username in skipped: + self.stdout.write(f' skipped: {username}') + + @staticmethod + def match(user, keycloak_users, case_twins): + if user.username in keycloak_users: + return keycloak_users[user.username], None + lower = user.username.lower() + if lower == user.username or lower not in keycloak_users: + return None, 'no Keycloak user has this username' + if lower in case_twins: + return None, 'another account has this username in another case' + kc_user = keycloak_users[lower] + email = (user.email or '').strip().lower() + if not email or email != (kc_user.get('email') or '').strip().lower(): + return None, 'emails differ' + return kc_user, None diff --git a/core/common/management/commands/sync_keycloak_groups.py b/core/common/management/commands/sync_keycloak_groups.py new file mode 100644 index 00000000..2c0cfa28 --- /dev/null +++ b/core/common/management/commands/sync_keycloak_groups.py @@ -0,0 +1,19 @@ +from django.core.management import BaseCommand + +from core.common.tasks import sync_groups_from_keycloak + + +class Command(BaseCommand): + help = 'Syncs Keycloak group memberships into Django, including for users who only use API tokens.' + + def add_arguments(self, parser): + parser.add_argument('--dry-run', action='store_true') + + def handle(self, *args, **options): + dry_run = options['dry_run'] + summary = sync_groups_from_keycloak(dry_run=dry_run) + self.stdout.write(f"{summary['changed']} user(s) {'would change' if dry_run else 'changed'}") + for name in sorted(set(summary['gained']) | set(summary['lost'])): + self.stdout.write(f" {name}: +{summary['gained'].get(name, 0)} -{summary['lost'].get(name, 0)}") + for change in summary['access_changes']: + self.stdout.write(f' access change: {change}') diff --git a/core/common/signals.py b/core/common/signals.py index aeead894..8c117de4 100644 --- a/core/common/signals.py +++ b/core/common/signals.py @@ -18,6 +18,8 @@ def propagate_owner_status(sender, instance=None, created=False, **kwargs): # p if created and instance.__class__ == Organization and instance.id != 1: instance.record_create_event() if not created and instance: + if isinstance(instance, UserProfile): + instance.push_profile_to_keycloak() updated_sources = 0 updated_collections = 0 updated_sources += instance.source_set.exclude( diff --git a/core/common/tasks.py b/core/common/tasks.py index adc1e6fd..c50bed7e 100644 --- a/core/common/tasks.py +++ b/core/common/tasks.py @@ -1220,3 +1220,87 @@ def collection_version_compare( def expansion_compare(expansion1_uri, expansion2_uri, is_changelog, verbosity, format_type='json'): from core.collections.models import Expansion return Expansion.run_diff(expansion1_uri, expansion2_uri, is_changelog, verbosity, format_type) + + +def _record_group_change(summary, user, current, expected): + from core.users.constants import STAFF_GROUP, SUPERADMIN_GROUP + + summary['changed'] += 1 + for name in expected - current: + summary['gained'][name] = summary['gained'].get(name, 0) + 1 + for name in current - expected: + summary['lost'][name] = summary['lost'].get(name, 0) + 1 + is_superuser = SUPERADMIN_GROUP in expected + is_staff = is_superuser or STAFF_GROUP in expected + if (is_staff, is_superuser) != (user.is_staff, user.is_superuser): + summary['access_changes'].append(f'{user.username}: staff={is_staff} superuser={is_superuser}') + logger.info('Keycloak group sync: user %s %s -> %s', user.id, sorted(current), sorted(expected)) + + +def _users_of_keycloak_groups(memberships, managed): + from django.db.models import Q + from core.users.models import UserProfile + + ids = {name: {member['id'] for member in memberships[name]} for name in managed} + # a Keycloak user already linked to an account is never matched to another one on username + linked = set(UserProfile.objects.filter( + sso_id__in=set().union(*ids.values())).values_list('sso_id', flat=True)) + usernames = { + name: {member['username'] for member in memberships[name] if member['id'] not in linked} for name in managed} + users = UserProfile.objects.filter( + Q(sso_id__in=set().union(*ids.values())) | + Q(sso_id__isnull=True, username__in=set().union(*usernames.values())) | + Q(groups__name__in=managed), is_active=True + ).distinct() + return users, ids, usernames + + +def sync_groups_from_keycloak(dry_run=False): + """Replaces users' Keycloak-managed groups with their Keycloak membership; no-op if Keycloak can't be read.""" + from django.contrib.auth.models import Group + from core.services.auth.core import AuthService + from core.services.auth.openid import OpenIDAuthService + + summary = {'changed': 0, 'gained': {}, 'lost': {}, 'access_changes': []} + if not (AuthService.is_sso_enabled() and OpenIDAuthService.is_admin_configured()): + return summary + + try: + memberships = OpenIDAuthService.get_group_memberships() + except Exception as ex: # pylint: disable=broad-except + logger.warning('Keycloak group sync aborted, could not read Keycloak: %s', ex) + return summary + + managed = set(Group.objects.filter(name__in=memberships).values_list('name', flat=True)) + if not managed: + logger.warning('Keycloak group sync aborted, no Keycloak group matches a Django group') + return summary + + users, ids, usernames = _users_of_keycloak_groups(memberships, managed) + + for user in users: + current = set(user.groups.values_list('name', flat=True)) + expected = (current - managed) | { + name for name in managed if (user.sso_id in ids[name] if user.sso_id else user.username in usernames[name])} + if current != expected: + _record_group_change(summary, user, current, expected) + if not dry_run: + user.set_groups(list(expected)) + return summary + + +@app.task(ignore_result=True) +def sync_keycloak_groups(): + sync_groups_from_keycloak() + + +@app.task(ignore_result=True) +def push_user_profile_to_keycloak(user_id): + from core.services.auth.openid import OpenIDAuthService + from core.users.models import UserProfile + + if not OpenIDAuthService.is_admin_configured(): + return + user = UserProfile.objects.filter(id=user_id).first() + if user: + OpenIDAuthService.update_user_profile(user) diff --git a/core/common/tests.py b/core/common/tests.py index 4194bf67..21b30a34 100644 --- a/core/common/tests.py +++ b/core/common/tests.py @@ -73,6 +73,7 @@ from core.users.models import UserProfile from core.users.tests.factories import UserProfileFactory from .backends import OCLOIDCAuthenticationBackend +from .exceptions import SSOIdentityMismatchRefused from .checksums import Checksum from .fhir_helpers import translate_fhir_query from .serializers import IdentifierSerializer @@ -2688,6 +2689,7 @@ def test_create_user(self, user_manager_mock): self.backend.create_user(self.claim) user_manager_mock.create_user.assert_called_once_with( 'batman', + sso_id=self.claim.get('sub'), email='batman@gotham.com', first_name='Bruce', last_name='Wayne', @@ -2736,6 +2738,38 @@ 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_the_account_linked_to_sub_even_after_a_username_change(self): + batman = UserProfileFactory(username='old-batman', sso_id='sub-1') + UserProfileFactory(username='batman') + + users = self.backend.filter_users_by_claims({**self.claim, 'sub': 'sub-1'}) + + self.assertEqual(list(users), [batman]) + + def test_filter_users_by_claims_refuses_account_linked_to_a_different_sub(self): + batman = UserProfileFactory(username='batman', sso_id='sub-1') + + with self.assertRaises(SSOIdentityMismatchRefused): + self.backend.filter_users_by_claims({**self.claim, 'sub': 'sub-2'}) + + batman.refresh_from_db() + self.assertEqual(batman.sso_id, 'sub-1') + + def test_filter_users_by_claims_links_unlinked_account_on_username(self): + batman = UserProfileFactory(username='batman') + + users = self.backend.filter_users_by_claims({**self.claim, 'sub': 'sub-1'}) + self.backend.update_user(users[0], {**self.claim, 'sub': 'sub-1'}) + + batman.refresh_from_db() + self.assertEqual(batman.sso_id, 'sub-1') + + def test_update_user_keeps_a_stored_sso_id(self): + user = Mock(sso_id='sub-1') + user.is_dirty.return_value = False + self.backend.update_user(user, {**self.claim, 'sub': 'sub-2'}) + self.assertEqual(user.sso_id, 'sub-1') + 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') diff --git a/core/middlewares/middlewares.py b/core/middlewares/middlewares.py index 4328c7c4..8c181cb8 100644 --- a/core/middlewares/middlewares.py +++ b/core/middlewares/middlewares.py @@ -15,7 +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.exceptions import DeactivatedAccountLoginRefused, SSOIdentityMismatchRefused 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 @@ -130,7 +130,7 @@ def __call__(self, request): """Allow exempt and approved anonymous traffic, otherwise return 403.""" try: is_allowed = self.is_request_allowed(request) - except DeactivatedAccountLoginRefused as ex: + except (DeactivatedAccountLoginRefused, SSOIdentityMismatchRefused) 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"' @@ -169,7 +169,7 @@ def get_authenticated_user(request): try: auth_result = OCLAuthentication().authenticate(Request(request)) - except DeactivatedAccountLoginRefused: + except (DeactivatedAccountLoginRefused, SSOIdentityMismatchRefused): raise except AuthenticationFailed: return user diff --git a/core/middlewares/tests.py b/core/middlewares/tests.py index 0df977df..a60b150d 100644 --- a/core/middlewares/tests.py +++ b/core/middlewares/tests.py @@ -7,7 +7,7 @@ from django.test import RequestFactory, SimpleTestCase, override_settings from rest_framework.exceptions import AuthenticationFailed -from core.common.exceptions import DeactivatedAccountLoginRefused +from core.common.exceptions import DeactivatedAccountLoginRefused, SSOIdentityMismatchRefused from core.middlewares.middlewares import RequireAuthenticationMiddleware @@ -82,6 +82,17 @@ def test_rejects_sign_in_refused_for_deactivated_account(self, authenticate_mock self.assertEqual(response['WWW-Authenticate'], 'Bearer realm="api"') self.assertIn('deactivated OCL account', json.loads(response.content)['detail']) + @patch('core.middlewares.middlewares.OCLAuthentication.authenticate') + def test_rejects_sign_in_refused_for_identity_mismatch(self, authenticate_mock): + """A sign-in refused for a username linked to another Keycloak user gets the same 401 that says why.""" + authenticate_mock.side_effect = SSOIdentityMismatchRefused() + + 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('different sign-in identity', 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/')) diff --git a/core/services/auth/openid.py b/core/services/auth/openid.py index f5f400c1..1073ed1d 100644 --- a/core/services/auth/openid.py +++ b/core/services/auth/openid.py @@ -1,4 +1,6 @@ import base64 +import logging +import time import requests from django.conf import settings @@ -8,6 +10,9 @@ from core.services.auth.core import AbstractAuthService +logger = logging.getLogger('oclapi') + + class OpenIDAuthService(AbstractAuthService): """ Service that interacts with OIDP for: @@ -18,6 +23,11 @@ class OpenIDAuthService(AbstractAuthService): authentication_class = OIDCAuthentication authentication_backend_class = OCLOIDCAuthenticationBackend USERS_URL = settings.OIDC_SERVER_INTERNAL_URL + f'/admin/realms/{settings.OIDC_REALM}/users' + GROUPS_URL = settings.OIDC_SERVER_INTERNAL_URL + f'/admin/realms/{settings.OIDC_REALM}/groups' + _service_token = None + _service_token_expires_at = 0 + ADMIN_TIMEOUT = 15 + PAGE_SIZE = 200 OIDP_ADMIN_TOKEN_URL = settings.OIDC_SERVER_INTERNAL_URL + '/realms/master/protocol/openid-connect/token' @staticmethod @@ -136,6 +146,84 @@ def exchange_code_for_token(code, redirect_uri, client_id, client_secret=None, c def get_admin_headers(**kwargs): return {'Authorization': f'Bearer {OpenIDAuthService.get_admin_token(**kwargs)}'} + @staticmethod + def is_admin_configured(): + return bool(settings.OIDC_ADMIN_CLIENT_ID and settings.OIDC_ADMIN_CLIENT_SECRET) + + @classmethod + def get_service_token(cls): + if cls._service_token and time.monotonic() < cls._service_token_expires_at: + return cls._service_token + response = requests.post( + settings.OIDC_OP_TOKEN_ENDPOINT, + data={ + 'grant_type': 'client_credentials', + 'client_id': settings.OIDC_ADMIN_CLIENT_ID, + 'client_secret': settings.OIDC_ADMIN_CLIENT_SECRET, + }, + timeout=cls.ADMIN_TIMEOUT, + ) + response.raise_for_status() + data = response.json() + cls._service_token = data['access_token'] + cls._service_token_expires_at = time.monotonic() + max(int(data.get('expires_in', 60)) - 30, 1) + return cls._service_token + + @classmethod + def _admin_request(cls, method, url, **kwargs): + response = requests.request( + method, url, headers={'Authorization': f'Bearer {cls.get_service_token()}'}, + timeout=cls.ADMIN_TIMEOUT, **kwargs) + response.raise_for_status() + return response + + @classmethod + def get_keycloak_user(cls, user): + if user.sso_id: + return cls._admin_request('GET', f'{cls.USERS_URL}/{user.sso_id}').json() + from core.users.models import UserProfile + users = cls._admin_request('GET', cls.USERS_URL, params={'username': user.username, 'exact': 'true'}).json() + kc_user = next((kc_user for kc_user in users if kc_user.get('username') == user.username), None) + # a Keycloak user already linked to another account is never matched on username + if kc_user and not UserProfile.objects.filter(sso_id=kc_user['id']).exists(): + return kc_user + return None + + @classmethod + def update_user_profile(cls, user): + kc_user = cls.get_keycloak_user(user) + if not kc_user: + raise LookupError(f'user {user.id} not found in Keycloak') + attributes = kc_user.get('attributes') or {} + for key in ('company', 'location'): + value = getattr(user, key) + attributes[key] = [value] if value else [] + kc_user.update({'firstName': user.first_name, 'lastName': user.last_name, 'attributes': attributes}) + cls._admin_request('PUT', f"{cls.USERS_URL}/{kc_user['id']}", json=kc_user) + + @classmethod + def _paginate(cls, url): + first, results = 0, [] + while True: + page = cls._admin_request('GET', url, params={'first': first, 'max': cls.PAGE_SIZE}).json() + results.extend(page) + if len(page) < cls.PAGE_SIZE: + return results + first += cls.PAGE_SIZE + + @classmethod + def get_users(cls): + return cls._paginate(cls.USERS_URL) + + @classmethod + def get_group_memberships(cls): + memberships = {} + for group in cls._paginate(cls.GROUPS_URL): + memberships[group['name']] = [ + {'id': member['id'], 'username': member['username']} + for member in cls._paginate(f"{cls.GROUPS_URL}/{group['id']}/members")] + return memberships + @staticmethod def create_user(_): """In OID auth, user signup needs to happen in OID first""" diff --git a/core/settings.py b/core/settings.py index cb57d6cb..f0f4c879 100644 --- a/core/settings.py +++ b/core/settings.py @@ -638,6 +638,8 @@ def get_set_from_env(name): OIDC_OP_TOKEN_ENDPOINT = f'{OIDC_SERVER_INTERNAL_URL}/realms/{OIDC_REALM}/protocol/openid-connect/token' OIDC_OP_USER_ENDPOINT = f'{OIDC_SERVER_INTERNAL_URL}/realms/{OIDC_REALM}/protocol/openid-connect/userinfo' OIDC_RP_SIGN_ALGO = 'RS256' +OIDC_ADMIN_CLIENT_ID = os.environ.get('OIDC_ADMIN_CLIENT_ID', '') +OIDC_ADMIN_CLIENT_SECRET = os.environ.get('OIDC_ADMIN_CLIENT_SECRET', '') OIDC_OP_JWKS_ENDPOINT = f'{OIDC_SERVER_INTERNAL_URL}/realms/{OIDC_REALM}/protocol/openid-connect/certs' OIDC_VERIFY_SSL = False OIDC_VERIFY_JWT = True diff --git a/core/users/migrations/0037_userprofile_sso_id.py b/core/users/migrations/0037_userprofile_sso_id.py new file mode 100644 index 00000000..5508f1b1 --- /dev/null +++ b/core/users/migrations/0037_userprofile_sso_id.py @@ -0,0 +1,16 @@ +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('users', '0036_retire_public_edit_access'), + ] + + operations = [ + migrations.AddField( + model_name='userprofile', + name='sso_id', + field=models.CharField(blank=True, max_length=64, null=True, unique=True), + ), + ] diff --git a/core/users/models.py b/core/users/models.py index f0933759..878c65ff 100644 --- a/core/users/models.py +++ b/core/users/models.py @@ -62,6 +62,7 @@ class Meta: last_name = models.CharField(max_length=100, blank=True) organizations = models.ManyToManyField('orgs.Organization', related_name='members') company = models.TextField(null=True, blank=True) + sso_id = models.CharField(max_length=64, null=True, blank=True, unique=True) location = models.TextField(null=True, blank=True) preferred_locale = models.TextField(null=True, blank=True) website = models.TextField(null=True, blank=True) @@ -456,6 +457,16 @@ def unfollow(self, following): referenced_object_url=following.url, ) + def push_profile_to_keycloak(self): + from core.services.auth.core import AuthService + dirty = self.get_dirty_fields() + if getattr(self, '_from_sso', False) or not any( + field in dirty for field in ('first_name', 'last_name', 'company', 'location')): + return + if AuthService.is_sso_enabled(): + from core.common.tasks import push_user_profile_to_keycloak + transaction.on_commit(lambda: push_user_profile_to_keycloak.delay(self.id)) + def set_groups(self, groups, verify=True, _save=True): if not verify or sorted(self.groups.values_list('name', flat=True)) != sorted(groups): self.groups.set(Group.objects.filter(name__in=groups)) diff --git a/core/users/tests/tests.py b/core/users/tests/tests.py index b0e51976..4fb54b8f 100644 --- a/core/users/tests/tests.py +++ b/core/users/tests/tests.py @@ -305,6 +305,237 @@ def test_user_without_preview_group_has_no_mapper_permission(self): self.assertFalse(user.has_perm(MAPPER_USE_PERMISSION)) +class KeycloakSyncTest(OCLTestCase): + SSO = 'core.services.auth.core.AuthService.is_sso_enabled' + CONFIGURED = 'core.services.auth.openid.OpenIDAuthService.is_admin_configured' + MEMBERSHIPS = 'core.services.auth.openid.OpenIDAuthService.get_group_memberships' + + @staticmethod + def member(username, sso_id=None): + return {'id': sso_id or f'id-{username}', 'username': username} + + def sync(self, memberships, **kwargs): + from core.common.tasks import sync_groups_from_keycloak + with patch(self.SSO, return_value=True), patch(self.CONFIGURED, return_value=True), \ + patch(self.MEMBERSHIPS, return_value=memberships): + return sync_groups_from_keycloak(**kwargs) + + def test_sync_adds_and_removes_groups_for_token_only_users_keeping_django_only_groups(self): + Group.objects.create(name='kc-a') + Group.objects.create(name='kc-b') + Group.objects.create(name='django-only') + added = UserProfileFactory(username='added') + removed = UserProfileFactory(username='removed') + removed.groups.set(Group.objects.filter(name__in=['kc-a', 'django-only'])) + + summary = self.sync({ + 'kc-a': [self.member('added')], 'kc-b': [self.member('added')], 'unknown': [self.member('removed')]}) + + self.assertEqual(summary['changed'], 2) + self.assertEqual(summary['gained'], {'kc-a': 1, 'kc-b': 1}) + self.assertEqual(summary['lost'], {'kc-a': 1}) + self.assertEqual(sorted(added.groups.values_list('name', flat=True)), ['kc-a', 'kc-b']) + self.assertEqual(list(removed.groups.values_list('name', flat=True)), ['django-only']) + + def test_sync_matches_on_sso_id_and_falls_back_to_username_only_without_one(self): + Group.objects.create(name='kc-a') + by_id = UserProfileFactory(username='Mixed-Case', sso_id='sub-1') + same_name_other_id = UserProfileFactory(username='someone', sso_id='sub-other') + never_signed_in = UserProfileFactory(username='legacy') + + self.sync({'kc-a': [self.member('mixed-case', 'sub-1'), self.member('someone', 'sub-2'), + self.member('legacy', 'sub-3')]}) + + self.assertEqual(list(by_id.groups.values_list('name', flat=True)), ['kc-a']) + self.assertFalse(same_name_other_id.groups.exists()) + self.assertEqual(list(never_signed_in.groups.values_list('name', flat=True)), ['kc-a']) + + def test_sync_dry_run_changes_nothing_and_reports_access_changes(self): + Group.objects.get_or_create(name=STAFF_GROUP) + user = UserProfileFactory(username='dry') + + summary = self.sync({STAFF_GROUP: [self.member('dry')]}, dry_run=True) + + self.assertEqual(summary['changed'], 1) + self.assertEqual(summary['access_changes'], ['dry: staff=True superuser=False']) + self.assertFalse(user.groups.exists()) + + def test_sync_aborts_when_keycloak_fails_or_has_no_matching_group(self): + from core.common.tasks import sync_groups_from_keycloak + group = Group.objects.create(name='kc-a') + user = UserProfileFactory(username='kept') + user.groups.add(group) + + with patch(self.SSO, return_value=True), patch(self.CONFIGURED, return_value=True): + with patch(self.MEMBERSHIPS, side_effect=Exception('down')): + self.assertEqual(sync_groups_from_keycloak()['changed'], 0) + with patch(self.MEMBERSHIPS, return_value={}): + self.assertEqual(sync_groups_from_keycloak()['changed'], 0) + + self.assertEqual(list(user.groups.values_list('name', flat=True)), ['kc-a']) + + def test_sync_is_noop_without_sso_or_admin_config(self): + from core.common.tasks import sync_groups_from_keycloak + with patch(self.MEMBERSHIPS) as memberships: + self.assertEqual(sync_groups_from_keycloak()['changed'], 0) + memberships.assert_not_called() + + def test_profile_save_enqueues_push_on_commit_for_changed_fields(self): + user = UserProfileFactory(first_name='Old') + + with patch(self.SSO, return_value=True), \ + patch('core.common.tasks.push_user_profile_to_keycloak.delay') as push: + with self.captureOnCommitCallbacks(execute=True): + user.first_name = 'New' + user.save() + push.assert_called_once_with(user.id) + push.reset_mock() + with self.captureOnCommitCallbacks(execute=True): + user.email = 'x@y.com' + user.save() + push.assert_not_called() + + def test_profile_save_from_sso_claims_is_not_pushed_back(self): + user = UserProfileFactory(first_name='Old') + + with patch(self.SSO, return_value=True), \ + patch('core.common.tasks.push_user_profile_to_keycloak.delay') as push: + with self.captureOnCommitCallbacks(execute=True): + user.first_name = 'New' + user._from_sso = True # pylint: disable=protected-access + user.save() + push.assert_not_called() + + def test_profile_save_not_pushed_without_sso(self): + user = UserProfileFactory(first_name='Old') + with patch('core.common.tasks.push_user_profile_to_keycloak.delay') as push: + with self.captureOnCommitCallbacks(execute=True): + user.first_name = 'New' + user.save() + push.assert_not_called() + + def test_push_task_updates_keycloak_from_the_current_row(self): + from core.common.tasks import push_user_profile_to_keycloak + user = UserProfileFactory(first_name='Latest') + + with patch(self.CONFIGURED, return_value=True), \ + patch('core.services.auth.openid.OpenIDAuthService.update_user_profile') as update: + push_user_profile_to_keycloak(user.id) + update.assert_called_once() + self.assertEqual(update.call_args[0][0].first_name, 'Latest') + with patch(self.CONFIGURED, return_value=False), \ + patch('core.services.auth.openid.OpenIDAuthService.update_user_profile') as update: + push_user_profile_to_keycloak(user.id) + update.assert_not_called() + + +class OpenIDAdminClientTest(OCLTestCase): + def test_paginate_fetches_until_a_short_page(self): + from core.services.auth.openid import OpenIDAuthService + size = OpenIDAuthService.PAGE_SIZE + pages = [[{'n': i} for i in range(size)], [{'n': 'last'}]] + with patch.object(OpenIDAuthService, '_admin_request') as request: + request.return_value.json.side_effect = pages + result = OpenIDAuthService._paginate('http://kc/groups') # pylint: disable=protected-access + self.assertEqual(len(result), size + 1) + self.assertEqual(request.call_args_list[1].kwargs['params'], {'first': size, 'max': size}) + + def test_paginate_stops_after_exact_full_page_followed_by_empty_page(self): + from core.services.auth.openid import OpenIDAuthService + size = OpenIDAuthService.PAGE_SIZE + with patch.object(OpenIDAuthService, '_admin_request') as request: + request.return_value.json.side_effect = [[{'n': i} for i in range(size)], []] + result = OpenIDAuthService._paginate('http://kc/groups') # pylint: disable=protected-access + self.assertEqual(len(result), size) + self.assertEqual(request.call_count, 2) + + def test_service_token_is_kept_in_process_memory(self): + from core.services.auth.openid import OpenIDAuthService + OpenIDAuthService._service_token = None # pylint: disable=protected-access + with patch('core.services.auth.openid.requests.post') as post: + post.return_value.json.return_value = {'access_token': 'abc', 'expires_in': 300} + self.assertEqual(OpenIDAuthService.get_service_token(), 'abc') + self.assertEqual(OpenIDAuthService.get_service_token(), 'abc') + post.assert_called_once() + OpenIDAuthService._service_token = None # pylint: disable=protected-access + + def test_group_memberships_carry_id_and_username(self): + from core.services.auth.openid import OpenIDAuthService + with patch.object(OpenIDAuthService, '_paginate', side_effect=[ + [{'id': 'g1', 'name': 'preview'}], [{'id': 'u1', 'username': 'a', 'email': 'x'}]]): + memberships = OpenIDAuthService.get_group_memberships() + self.assertEqual(memberships, {'preview': [{'id': 'u1', 'username': 'a'}]}) + + def test_update_user_profile_merges_into_existing_representation_by_sso_id(self): + from core.services.auth.openid import OpenIDAuthService + user = UserProfileFactory(first_name='New', last_name='Name', company='Co', location='', sso_id='sub-1') + kc_user = {'id': 'sub-1', 'username': 'u', 'attributes': {'other': ['x'], 'location': ['Old']}} + with patch.object(OpenIDAuthService, '_admin_request') as request: + request.return_value.json.return_value = kc_user + OpenIDAuthService.update_user_profile(user) + put = request.call_args_list[-1] + self.assertEqual(put.args[0], 'PUT') + self.assertEqual(put.kwargs['json']['firstName'], 'New') + self.assertEqual( + put.kwargs['json']['attributes'], {'other': ['x'], 'company': ['Co'], 'location': []}) + + +class BackfillSSOIdsTest(OCLTestCase): + def run_command(self, *args): + from io import StringIO + from django.core.management import call_command + out = StringIO() + with patch('core.services.auth.openid.OpenIDAuthService.get_users', return_value=[ + {'id': 'kc-1', 'username': 'exact'}, {'id': 'kc-2', 'username': 'mixed'}, + {'id': 'kc-3', 'username': 'taken'}]): + call_command('backfill_sso_ids', *args, stdout=out) + return out.getvalue() + + def test_links_exact_matches_and_skips_the_rest(self): + exact = UserProfileFactory(username='exact') + mixed = UserProfileFactory(username='Mixed') + UserProfileFactory(username='other-owner', sso_id='kc-3') + taken = UserProfileFactory(username='taken') + + output = self.run_command() + + exact.refresh_from_db() + self.assertEqual(exact.sso_id, 'kc-1') + mixed.refresh_from_db() + taken.refresh_from_db() + self.assertIsNone(mixed.sso_id) + self.assertIsNone(taken.sso_id) + self.assertIn('1 account(s) linked, 3 skipped', output) + self.assertIn('skipped: Mixed', output) + self.assertIn('skipped: taken', output) + + def test_dry_run_changes_nothing(self): + exact = UserProfileFactory(username='exact') + output = self.run_command('--dry-run') + exact.refresh_from_db() + self.assertIsNone(exact.sso_id) + self.assertIn('1 account(s) would be linked', output) + self.assertIn('would be linked: exact -> exact', output) + + def test_links_a_mixed_case_account_without_a_case_twin_when_the_emails_match(self): + from io import StringIO + from django.core.management import call_command + legacy = UserProfileFactory(username='Legacy', email='Legacy@Example.org') + twin = UserProfileFactory(username='Twin', email='twin@example.org') + UserProfileFactory(username='twin', email='twin@example.org') + other = UserProfileFactory(username='Other', email='someone@example.org') + with patch('core.services.auth.openid.OpenIDAuthService.get_users', return_value=[ + {'id': 'kc-l', 'username': 'legacy', 'email': 'legacy@example.org'}, + {'id': 'kc-t', 'username': 'twin', 'email': 'twin@example.org'}, + {'id': 'kc-o', 'username': 'other', 'email': 'other@example.org'}]): + call_command('backfill_sso_ids', stdout=StringIO()) + for user in (legacy, twin, other): + user.refresh_from_db() + self.assertEqual(legacy.sso_id, 'kc-l') + self.assertIsNone(twin.sso_id) + self.assertIsNone(other.sso_id) + + class TokenAuthenticationViewTest(OCLAPITestCase): def test_post(self): response = self.client.post('/users/login/', {})