From 3c51ae6008104903823006667f2441758fd2ffa5 Mon Sep 17 00:00:00 2001
From: Andreas Ellewsen <ae@uio.no>
Date: Mon, 14 Feb 2022 16:27:15 +0100
Subject: [PATCH] Handle wrong person following invitation link
If someone that already exists in greg follows an invite, we are kind
enough to give the new role to the existing person. This introduced a
security risk if the invitation was actually meant for someone else.
Because of this situation, we introduce a security mechanism where we
disable the invitationlink if the name of the existing person is too
different from the name used in the invitation.
Resolves: GREG-166
---
gregui/authentication/auth_backends.py | 67 ++++++++++++++++----------
gregui/tests/test_utils.py | 8 ++-
gregui/utils.py | 4 ++
3 files changed, 53 insertions(+), 26 deletions(-)
diff --git a/gregui/authentication/auth_backends.py b/gregui/authentication/auth_backends.py
index 63754aae..c3ff9ef0 100644
--- a/gregui/authentication/auth_backends.py
+++ b/gregui/authentication/auth_backends.py
@@ -10,13 +10,13 @@ from django.conf import settings
from django.contrib.auth import get_user_model
from django.contrib.auth.backends import BaseBackend, UserModel
from django.core.exceptions import SuspiciousOperation
-from django.utils.timezone import make_aware
+from django.utils import timezone
from mozilla_django_oidc.auth import OIDCAuthenticationBackend
from greg.models import Identity, Person, Sponsor, InvitationLink
from gregui.models import GregUserProfile
-from gregui.utils import name_diff
+from gregui.utils import name_diff_too_large
logger = structlog.getLogger(__name__)
@@ -339,7 +339,7 @@ class GregOIDCBackend(ValidatingOIDCBackend):
existing_nin.source = source
existing_nin.value = new_nin
existing_nin.verified = Identity.Verified.AUTOMATIC
- existing_nin.verified_at = make_aware(datetime.datetime.now())
+ existing_nin.verified_at = timezone.make_aware(datetime.datetime.now())
existing_nin.save()
logger.info("identity_update", identity_id=existing_nin.id)
@@ -357,7 +357,7 @@ class GregOIDCBackend(ValidatingOIDCBackend):
type=Identity.IdentityType.NORWEGIAN_NATIONAL_ID_NUMBER,
value=new_nin,
verified=Identity.Verified.AUTOMATIC,
- verified_at=make_aware(datetime.datetime.now()),
+ verified_at=timezone.make_aware(datetime.datetime.now()),
)
nin_id.save()
logger.info("identity_add", identity_id=nin_id.id)
@@ -399,7 +399,7 @@ class GregOIDCBackend(ValidatingOIDCBackend):
existing_feide_id.source = source
existing_feide_id.value = new_feide_id
existing_feide_id.verified = Identity.Verified.AUTOMATIC
- existing_feide_id.verified_at = make_aware(datetime.datetime.now())
+ existing_feide_id.verified_at = timezone.make_aware(datetime.datetime.now())
existing_feide_id.save()
else:
@@ -421,7 +421,7 @@ class GregOIDCBackend(ValidatingOIDCBackend):
type=Identity.IdentityType.FEIDE_ID,
value=new_feide_id,
verified=Identity.Verified.AUTOMATIC,
- verified_at=make_aware(datetime.datetime.now()),
+ verified_at=timezone.make_aware(datetime.datetime.now()),
)
feide_id.save()
logger.info("identity_add", identity_id=feide_id.id, feide_id=new_feide_id)
@@ -445,7 +445,7 @@ class GregOIDCBackend(ValidatingOIDCBackend):
old_email = existing_email.value
existing_email.value = new_email
existing_email.verified = Identity.Verified.AUTOMATIC
- existing_email.verified_at = make_aware(datetime.datetime.now())
+ existing_email.verified_at = timezone.make_aware(datetime.datetime.now())
existing_email.save()
logger.info(
"identity_update",
@@ -472,19 +472,19 @@ class GregOIDCBackend(ValidatingOIDCBackend):
type=Identity.IdentityType.FEIDE_EMAIL,
value=new_email,
verified=Identity.Verified.AUTOMATIC,
- verified_at=make_aware(datetime.datetime.now()),
+ verified_at=timezone.make_aware(datetime.datetime.now()),
)
email_id.save()
logger.info("identity_add", identity_id=email_id.id, value=new_email)
def _update_person_name(
- self, person: Person, first_name: str, last_name: str, source: str
+ self, person: Person, first_name: str, last_name: str
) -> None:
new_combined_name = f"{first_name} {last_name}"
existing_combined_name = f"{person.first_name} {person.last_name}"
- if name_diff(existing_combined_name, new_combined_name) > 4:
+ if name_diff_too_large(existing_combined_name, new_combined_name, 4):
logger.warning(
"name_diff_to_large",
existing_name=existing_combined_name,
@@ -518,9 +518,7 @@ class GregOIDCBackend(ValidatingOIDCBackend):
self._update_person_feide_id(person, userinfo["userid_feide"], "feide")
if "email" in userinfo:
self._update_person_email(person, userinfo["email"], "feide")
- self._update_person_name(
- person, userinfo["first_name"], userinfo["last_name"], "feide"
- )
+ self._update_person_name(person, userinfo["first_name"], userinfo["last_name"])
def _update_sponsor_from_userinfo(self, userinfo: dict, sponsor: Sponsor) -> None:
"""Updates a sponsor object with data from OIDC."""
@@ -578,26 +576,19 @@ class GregOIDCBackend(ValidatingOIDCBackend):
def _get_or_create_greg_user_profile(self, userinfo: dict, user: UserModel):
"""Get or create a GregUserProfile."""
- def _get_invite_person(
- session, old_person: Optional[Person]
- ) -> Optional[Person]:
+ def _get_invitation_link(session) -> Optional[InvitationLink]:
try:
- invitation_link = InvitationLink.objects.get(uuid=session["invite_id"])
+ return InvitationLink.objects.get(uuid=session["invite_id"])
except InvitationLink.DoesNotExist:
logger.error("invite_not_found", invite_id=session["invite_id"])
logger.bind(authenticating_invite=False)
- return old_person
+ return None
+ def _get_invite_person(invitation_link) -> Person:
logger.bind(authenticating_invite=True)
role = invitation_link.invitation.role
invite_person = role.person
logger.info("invite_found", invite_person=invite_person.id)
-
- if old_person and not old_person == invite_person:
- role.person = old_person
- role.save()
- invite_person.delete()
- return old_person
return invite_person
logger.bind(user=user)
@@ -618,7 +609,33 @@ class GregOIDCBackend(ValidatingOIDCBackend):
old_person = None
if user_profile and user_profile.person:
old_person = self._get_person_from_userinfo(userinfo)
- person = _get_invite_person(session, old_person)
+ person = old_person
+ invitation_link = _get_invitation_link(session)
+ if invitation_link:
+ inv_person = _get_invite_person(invitation_link)
+ if old_person and old_person != inv_person:
+ # Logged in person has a person object in greg but has followed an
+ # invite.
+ inv_name = f"{inv_person.first_name} {inv_person.last_name}"
+ old_name = f"{old_person.first_name} {old_person.last_name}"
+ if (
+ not inv_person.registration_completed_date
+ and not name_diff_too_large(old_name, inv_name, 4)
+ ):
+ # The name is close and the invited person has not completed
+ # registration. Give the role to the existing person, and
+ # delete the invited one.
+ role = invitation_link.invitation.role
+ role.person = old_person
+ role.save()
+ inv_person.delete()
+ else:
+ # The logged in user has gotten someone else's invitation, and
+ # the invitation should be disabled.
+ invitation_link.expire = timezone.now()
+ invitation_link.save()
+ else:
+ person = inv_person
else:
person = self._get_person_from_userinfo(userinfo)
logger.bind(authenticating_invite=False)
diff --git a/gregui/tests/test_utils.py b/gregui/tests/test_utils.py
index f759f306..d8cfe531 100644
--- a/gregui/tests/test_utils.py
+++ b/gregui/tests/test_utils.py
@@ -1,4 +1,4 @@
-from gregui.utils import name_diff, restricted_damarau_levenshtein
+from gregui.utils import name_diff, name_diff_too_large, restricted_damarau_levenshtein
def test_rdm_replace():
@@ -21,3 +21,9 @@ def test_name_longer():
def test_name_threshold():
"""Verify continues with raised threshold"""
assert name_diff("Abcdefgh Ijklmnop", "Qrstuvw Xyz", 100) == 10
+
+
+def test_name_too_large():
+ """Verify boolean version responds with expected boolean"""
+ assert not name_diff_too_large("Test Name", "Testing Name", 3)
+ assert name_diff_too_large("Test Name", "Testing Name", 2)
diff --git a/gregui/utils.py b/gregui/utils.py
index b08012f7..0b19c993 100644
--- a/gregui/utils.py
+++ b/gregui/utils.py
@@ -60,3 +60,7 @@ def name_diff(full_name1: str, full_name2: str, threshold: int = 2) -> int:
if total_difference > threshold:
break
return total_difference
+
+
+def name_diff_too_large(full_name1: str, full_name2: str, threshold: int) -> bool:
+ return name_diff(full_name1, full_name2, threshold) > threshold
--
GitLab