Skip to content

Commit

Permalink
feat: try to associate user by keycloak UUID
Browse files Browse the repository at this point in the history
User might have a social account with another Keycloak provider. Some fields
between Tunnistamo and Keycloak might not be in sync (e.g. email), so we'll use
UUID to attempt to find the user.

Refs: HP-2279
  • Loading branch information
charn committed Mar 18, 2024
1 parent 5d72c61 commit e779fb3
Show file tree
Hide file tree
Showing 3 changed files with 65 additions and 4 deletions.
5 changes: 5 additions & 0 deletions tunnistamo/settings.py
Original file line number Diff line number Diff line change
Expand Up @@ -482,6 +482,11 @@
# a similar email address.
# 'social_core.pipeline.social_auth.associate_by_email',

# User might have a social account with another Keycloak provider. Some fields
# between Tunnistamo and Keycloak might not be in sync (e.g. email), so we'll use
# UUID to attempt to find the user.
'users.pipeline.association_by_keycloak_uuid',

# Create a user account if we haven't found one yet.
'social_core.pipeline.user.create_user',

Expand Down
38 changes: 35 additions & 3 deletions users/pipeline.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
from auth_backends.adfs.base import BaseADFS
from auth_backends.adfs.helsinki import HelsinkiADFS
from auth_backends.helsinki_azure_ad import HelsinkiAzureADTenantOAuth2
from auth_backends.helsinki_tunnistus_suomifi import HelsinkiTunnistus
from auth_backends.helsinki_tunnus import HelsinkiTunnus
from auth_backends.tunnistamo import Tunnistamo
from users.models import LoginMethod, TunnistamoSession
from users.views import AuthenticationErrorView
Expand Down Expand Up @@ -155,6 +157,36 @@ def associate_by_email(strategy, details, user=None, *args, **kwargs):
return error_view.get(strategy.request)


def association_by_keycloak_uuid(strategy, backend, uuid, user=None, *args, **kwargs):
"""For Keycloak based authentication, associate user based on UUID.
User might have logged in with another keycloak based authentication method. If a
keycloak based authentication is used, we'll try to associate the user
based on the UUID.
Since Tunnistamo might not be in sync with Keycloak, a user might e.g. have a
different email address in Keycloak than in Tunnistamo. This would normally cause a
new user to be created, but since a user with the same UUID might already exist,
this could lead to integrity errors.
"""
if user or not isinstance(backend, (HelsinkiTunnus, HelsinkiTunnistus)):
return

user = strategy.get_user(uuid=uuid)

if (
user
and user.social_auth.filter(
provider__in=(HelsinkiTunnus.name, HelsinkiTunnistus.name)
).exists()
):
logger.debug(f"Keycloak user with the same UUID {user.uuid} exists.")
return {
"user": user,
}

return

def update_ad_groups(details, backend, user=None, *args, **kwargs):
"""Update users AD groups.
Expand All @@ -176,7 +208,7 @@ def check_existing_social_associations(backend, strategy, user=None, social=None
"""Deny adding additional social auths
social_core.pipeline.social_auth.associate_user would automatically
add additional social auths for the user, if they succesfully
add additional social auths for the user, if they successfully
authenticated again to an IdP while holding a session with Tunnistamo.
We don't want this to happen, as there is no interface for managing
additional IdPs.
Expand All @@ -195,8 +227,8 @@ def check_existing_social_associations(backend, strategy, user=None, social=None
# These are an exception to the only-one-social-auth -rule because we want to
# allow the user to use these provider pairs simultaneously.
allowed_exceptions = (
("helsinkiazuread", "helsinki_adfs"),
("helsinki_tunnus", "heltunnistussuomifi"),
(HelsinkiAzureADTenantOAuth2.name, HelsinkiADFS.name),
(HelsinkiTunnus.name, HelsinkiTunnistus.name),
)
for pair in allowed_exceptions:
if (
Expand Down
26 changes: 25 additions & 1 deletion users/tests/test_pipeline.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@
from django.urls import reverse
from django.utils.crypto import get_random_string
from social_core.exceptions import AuthFailed
from social_django.models import UserSocialAuth
from social_django.utils import load_backend, load_strategy

from tunnistamo.tests.conftest import create_rsa_key, reload_social_django_utils
from users.pipeline import update_ad_groups
from users.pipeline import association_by_keycloak_uuid, update_ad_groups
from users.tests.conftest import DummyFixedOidcBackend


Expand Down Expand Up @@ -78,3 +79,26 @@ def test_update_ad_groups_pipeline_part_should_work_with_variety_of_values_from_
update_ad_groups(details, backend, user)

assertCountEqual(user.ad_groups.all().values_list('name', flat=True), expected)

@pytest.mark.django_db
@pytest.mark.parametrize("login_backend,existing_backend,expected", (
("heltunnistussuomifi", "helsinki_tunnus", True),
("helsinki_tunnus", "heltunnistussuomifi", True),
("heltunnistussuomifi", None, False),
("helsinki_tunnus", None, False),
("helsinki_adfs", "helsinki_tunnus", False),
("helsinki_tunnus","helsinki_adfs", False),
), ids=repr)
def test_association_by_keycloak_uuid(login_backend, existing_backend, user, expected):
if existing_backend:
UserSocialAuth.objects.create(user=user, provider=existing_backend, uid=user.uuid)
strategy = load_strategy()
uri = reverse("social:complete", kwargs={"backend": login_backend})
backend = load_backend(strategy, login_backend, uri)

pipeline_data = association_by_keycloak_uuid(strategy, backend, user.uuid)

if expected:
assert pipeline_data == {"user": user}
else:
assert pipeline_data is None

0 comments on commit e779fb3

Please sign in to comment.