OpenConceptLab/ocl_online#307 | User profile sync to django | job to sync groups from Keycloak to django

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)

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,30 +79,34 @@ 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
 
         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
 
     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)
             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
+        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):

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).

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

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}')

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(

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)

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')

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

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/'))