Skip to content

Commit

Permalink
fix(FT OIDC): prevent overwriting duplicate emails
Browse files Browse the repository at this point in the history
fix(PoleEmploiConnectUserData): raise EmailInUseException on duplicate emails

France Travail allows users to overload the same email, and we allow only one

feat(pe_connect_callback): catch EmailInUseException

feat: render messages as modals

fix: quality

feat(france_travail_registration_failure.html): redirect to jobseeker registration

fix: tests

feat(users/tests): test_get_redacted_full_name

fix: solution should work for other identity providers

refactor(User.get_redacted_full_name): extend existing utility

fix(pe_connect/views.py): security best practice

fix: remove unused code
  • Loading branch information
calummackervoy committed Sep 4, 2024
1 parent 9d9fe75 commit 8a1532a
Show file tree
Hide file tree
Showing 16 changed files with 221 additions and 22 deletions.
12 changes: 10 additions & 2 deletions itou/openid_connect/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,12 @@
from .constants import OIDC_STATE_CLEANUP, OIDC_STATE_EXPIRATION


class EmailInUseException(Exception):
def __init__(self, user, *args):
self.user = user
super().__init__(*args)


class InvalidKindException(Exception):
def __init__(self, user, *args):
self.user = user
Expand Down Expand Up @@ -106,8 +112,8 @@ def create_or_update_user(self, is_login=False):
Create or update a user managed by another identity provider.
- If there is already a user with this username (user_info_dict["sub"])
and from the same identity provider, we update and return it.
- If there is already a user with the email, we return this user.
- otherwise, we create a new user based on the data we received.
- If there is already a user with the email, we return this user, provided
the email is not in use by another account (some providers allow email overloading)
"""
user_data_dict = dataclasses.asdict(self)
user_data_dict = {key: value for key, value in user_data_dict.items() if value}
Expand All @@ -124,6 +130,8 @@ def create_or_update_user(self, is_login=False):
self.check_valid_kind(user, user_data_dict, is_login)
# Don't update a user handled by another SSO provider.
return user, created
if user.identity_provider == self.identity_provider:
raise EmailInUseException(user)
except User.DoesNotExist:
# User.objects.create_user does the following:
# - set User.is_active to true,
Expand Down
31 changes: 24 additions & 7 deletions itou/openid_connect/pe_connect/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from django.shortcuts import render
from django.urls import reverse
from django.utils import crypto
from django.utils.html import format_html
from django.utils.http import urlencode

from itou.external_data.models import ExternalDataImport
Expand All @@ -19,15 +20,15 @@

from ..models import InvalidKindException, MultipleUsersFoundException
from . import constants
from .models import PoleEmploiConnectState, PoleEmploiConnectUserData
from .models import EmailInUseException, PoleEmploiConnectState, PoleEmploiConnectUserData


logger = logging.getLogger(__name__)


def _redirect_to_job_seeker_login_on_error(error_msg, request=None):
def _redirect_to_job_seeker_login_on_error(error_msg, request=None, extra_tags=""):
if request:
messages.error(request, error_msg)
messages.error(request, error_msg, extra_tags)
return HttpResponseRedirect(reverse("login:job_seeker"))


Expand Down Expand Up @@ -139,12 +140,28 @@ def pe_connect_callback(request):
return HttpResponseRedirect(url)
except MultipleUsersFoundException as e:
return _redirect_to_job_seeker_login_on_error(
"Vous avez deux comptes sur la plateforme et nous détectons un conflit d'email : "
f"{e.users[0].email} et {e.users[1].email}. "
"Veuillez vous rapprocher du support pour débloquer la situation en suivant "
f"<a href='{global_constants.ITOU_HELP_CENTER_URL}'>ce lien</a>.",
format_html(
"Vous avez deux comptes sur la plateforme et nous détectons un conflit d'email : "
f"{e.users[0].email} et {e.users[1].email}. "
"Veuillez vous rapprocher du support pour débloquer la situation en suivant "
f"<a href='{global_constants.ITOU_HELP_CENTER_URL}'>ce lien</a>."
),
request=request,
)
except EmailInUseException as e:
redacted_name = e.user.get_redacted_full_name()
msg_who = f" au nom de <strong>{redacted_name}</strong>" if redacted_name else ""

return _redirect_to_job_seeker_login_on_error(
format_html(
"Vous avez essayé de vous connecter avec un compte France Travail, mais un compte"
f"{msg_who} a déjà été créé avec cette adresse e-mail. Vous pouvez néanmoins"
" vous inscrire en utilisant une autre adresse e-mail et un mot de passe"
' en cliquant sur <strong>"Inscription"</strong>.'
),
request=request,
extra_tags="modal france_travail_registration_failure",
)

nir = request.session.get(global_constants.ITOU_SESSION_NIR_KEY)
if nir:
Expand Down
2 changes: 1 addition & 1 deletion itou/templates/django_bootstrap5/messages.html
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{% load i18n django_bootstrap5 %}
{% for message in messages %}
{% if "toast" not in message.tags %}
{% if "toast" not in message.tags and "modal" not in message.tags %}
<div class="alert alert-{{ message|bootstrap_message_alert_type }} alert-dismissible fade show" role="alert">
<button type="button" class="btn-close" data-bs-dismiss="alert" aria-label="{% trans 'close' %}"></button>
{{ message }}
Expand Down
1 change: 1 addition & 0 deletions itou/templates/layout/base.html
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@

<main id="main" role="main" class="s-main">
<div class="toast-container" aria-live="polite" aria-atomic="true">{% include "utils/toasts.html" %}</div>
{% include "utils/modals.html" %}

<section class="s-title-02">
<div class="s-title-02__container container">
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
{% load static %}
{% load theme_inclusion %}
{% load django_bootstrap5 %}

<div class="modal-dialog modal-dialog-centered">
<div class="modal-content">
<div class="modal-header">
<h3 class="modal-title text-danger" id="message-modal-{{ forloop.counter }}-label">
<i class="ri-xl ri-alert-line"></i>
L'inscription a échoué
</h3>
<button type="button" class="btn-close" data-bs-dismiss="modal" aria-label="Fermer"></button>
</div>
<div class="row modal-body">
<h2 class="h4">L'inscription via France Travail a échoué</h2>
<p>{{ message }}</p>
</div>
<div class="modal-footer">
<button type="button" class="btn" data-bs-dismiss="modal">Retour</button>
<a href="{% url 'signup:job_seeker' %}" class="btn btn-primary">Inscription</a>
</div>
</div>
</div>
19 changes: 19 additions & 0 deletions itou/templates/utils/modals.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
{% load static %}
{% load theme_inclusion %}
{% load django_bootstrap5 %}

{% for message in messages %}
{% if "modal" in message.extra_tags %}
<div class="modal fade" id="message-modal-{{ forloop.counter }}" data-bs-backdrop="static" tabindex="-1" role="dialog" aria-labelledby="message-modal-{{ forloop.counter }}-label" aria-modal="true">
{% if "france_travail_registration_failure" in message.extra_tags %}
{% include "utils/modal_includes/france_travail_registration_failure.html" %}
{% endif %}
</div>

<script nonce="{{ CSP_NONCE }}">
window.onload = function() {
new bootstrap.Modal(document.getElementById("message-modal-{{ forloop.counter }}")).show();
};
</script>
{% endif %}
{% endfor %}
13 changes: 13 additions & 0 deletions itou/users/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
from itou.companies.enums import CompanyKind
from itou.utils.db import or_queries
from itou.utils.models import UniqueConstraintWithErrorCode
from itou.utils.templatetags.str_filters import mask_unless
from itou.utils.validators import validate_birthdate, validate_nir, validate_pole_emploi_id

from .enums import IdentityProvider, LackOfNIRReason, LackOfPoleEmploiId, Title, UserKind
Expand Down Expand Up @@ -358,6 +359,18 @@ def get_full_name(self):
full_name = f"{self.first_name.strip().title()} {self.last_name.upper()}"
return full_name.strip()[:70]

def get_redacted_full_name(self):
"""
Return full name in redacted form for privacy, e.g. J** H***h
"""

def get_mask_for_part(part):
visible_chars = 1 if len(part) <= 3 else 2
last_char = part[-1] if visible_chars > 1 else ""
return part[0] + "*" * (min(len(part) - visible_chars, 10)) + last_char

return mask_unless(f"{self.first_name} {self.last_name}", False, get_mask_for_part)

@property
def is_job_seeker(self):
return self.kind == UserKind.JOB_SEEKER
Expand Down
4 changes: 2 additions & 2 deletions itou/utils/templatetags/str_filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ def pluralizefr(value, arg="s"):

@register.filter(is_safe=True)
@defaultfilters.stringfilter
def mask_unless(value, predicate):
def mask_unless(value, predicate, mask_function=(lambda x: x[0] + "…")):
if predicate:
return value

return " ".join(part[0] + "…" for part in re.split(f"[{re.escape(string.whitespace)}]+", value) if part)
return " ".join(mask_function(part) for part in re.split(f"[{re.escape(string.whitespace)}]+", value) if part)
19 changes: 18 additions & 1 deletion tests/openid_connect/pe_connect/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
from itou.external_data.apis import pe_connect
from itou.external_data.models import ExternalDataImport
from itou.openid_connect.constants import OIDC_STATE_CLEANUP
from itou.openid_connect.models import InvalidKindException
from itou.openid_connect.models import EmailInUseException, InvalidKindException
from itou.openid_connect.pe_connect import constants
from itou.openid_connect.pe_connect.models import PoleEmploiConnectState, PoleEmploiConnectUserData
from itou.users.enums import IdentityProvider, UserKind
Expand Down Expand Up @@ -181,6 +181,23 @@ def test_create_user_with_already_existing_peamu_id_but_from_other_sso(self):
with pytest.raises(ValidationError):
peamu_user_data.create_or_update_user()

def test_create_user_with_already_existing_peamu_email(self):
"""
If there already is an existing user from PE Connect with this email,
but under another username, we raise an error.
We do this because France Travail allows users to overload the same email
across multiple accounts, where we only allow one.
"""
peamu_user_data = PoleEmploiConnectUserData.from_user_info(PEAMU_USERINFO)
JobSeekerFactory(
username="another_username",
email=peamu_user_data.email,
identity_provider=IdentityProvider.PE_CONNECT,
)
with pytest.raises(EmailInUseException):
peamu_user_data.create_or_update_user()

def test_create_django_user_with_already_existing_peamu_email_django(self):
"""
If there already is an existing user from Django with email PEAMU sent us
Expand Down
23 changes: 23 additions & 0 deletions tests/users/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -647,6 +647,29 @@ def test_get_full_name(self):

assert len(JobSeekerFactory(first_name=too_long_name, last_name="maréchal").get_full_name()) == 70

def test_get_redacted_full_name(self):
user = JobSeekerFactory(first_name="", last_name="Bach")

def override_full_name(full_name):
names = full_name.split(" ")
user.first_name = " ".join(names[:-1]) if len(names) > 1 else names[0]
user.last_name = names[-1] if len(names) > 1 else ""

test_names = [
("Johan Sebastian Bach", "J***n S*******n B**h"),
("Max Weber", "M** W***r"),
("Charlemagne", "C*********e"),
("Salvador Felipe Jacinto Dalí y Domenech", "S******r F****e J*****o D**í y D******h"),
("Harald Blue-tooth", "H****d B********h"),
("Llanfairpwllgwyngyllgogerychwyrndrobwllllantysiliogogogoch", "L**********h"),
("Max", "M**"),
("", ""),
]

for name, expected_output in test_names:
override_full_name(name)
assert user.get_redacted_full_name() == expected_output


class JobSeekerProfileModelTest(TestCase):
def setUp(self):
Expand Down
6 changes: 6 additions & 0 deletions tests/www/companies_views/__snapshots__/test_card_views.ambr
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,12 @@
<div aria-atomic="true" aria-live="polite" class="toast-container">

</div>







<section class="s-title-02">
<div class="s-title-02__container container">
Expand Down
6 changes: 5 additions & 1 deletion tests/www/invitations_views/test_company_accept.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,11 @@ def test_accept_invitation_signup_bad_email_case(self):
@respx.mock
def test_accept_existing_user_not_logged_in_using_IC(self):
invitation = SentEmployerInvitationFactory(email=OIDC_USERINFO["email"])
user = EmployerFactory(email=OIDC_USERINFO["email"], has_completed_welcoming_tour=True)
user = EmployerFactory(
username=OIDC_USERINFO["sub"],
email=OIDC_USERINFO["email"],
has_completed_welcoming_tour=True,
)
response = self.client.get(invitation.acceptance_link, follow=True)
assert reverse("login:employer") in response.wsgi_request.get_full_path()
assert not invitation.accepted
Expand Down
6 changes: 5 additions & 1 deletion tests/www/invitations_views/test_prescriber_organization.py
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,11 @@ def test_accept_existing_user_belongs_to_another_organization(self):
@respx.mock
def test_accept_existing_user_not_logged_in_using_IC(self):
invitation = PrescriberWithOrgSentInvitationFactory(sender=self.sender, organization=self.organization)
user = PrescriberFactory(email=OIDC_USERINFO["email"], has_completed_welcoming_tour=True)
user = PrescriberFactory(
username=OIDC_USERINFO["sub"],
email=OIDC_USERINFO["email"],
has_completed_welcoming_tour=True,
)
# The user verified its email
EmailAddress(user_id=user.pk, email=user.email, verified=True, primary=True).save()
invitation = PrescriberWithOrgSentInvitationFactory(
Expand Down
Loading

0 comments on commit 8a1532a

Please sign in to comment.