Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connexion : Empêcher les utilisateurs de se connecter à un autre compte du SSO partageant un courriel existant #4810

Merged
merged 1 commit into from
Sep 24, 2024

Conversation

calummackervoy
Copy link
Contributor

@calummackervoy calummackervoy commented Sep 23, 2024

🤔 Pourquoi ?

Sur OIDConnect, les nom d’utilisateurs (sub) sont uniques, mais non les e-mails. Les SSOs permettent donc la réutilisation des emails entre comptes, qui est invalide sur notre service. Désormais on a permis la modification de nom d'utilisateur quand il arrive du même SSO avec un autre sub. Cela cause des problèmes intransients pour nos utilisateurs, et ce PR remplace ce comportement avec les erreurs explicite sur le login. C'est une étape intermediare, où après on va enforcer qu'un email peut utiliser seulement un SSO.

🚨 À vérifier

  • Mettre à jour le CHANGELOG_breaking_changes.md ?

Captures d'écran

Screenshot 2024-09-24 at 15 04 37

FranceConnect

Screenshot 2024-09-24 at 15 29 39

InclusionConnect

@calummackervoy calummackervoy added the no-changelog Ne doit pas figurer dans le journal des changements. label Sep 23, 2024
@calummackervoy calummackervoy self-assigned this Sep 23, 2024
@tonial
Copy link
Contributor

tonial commented Sep 24, 2024

Quel est le contexte ?

Encore une fois, le Sub OpenID Connect est le seul identifiant unique d'un utilisateur : on ne doit pas le changer même si on trouve un autre utilisateur avec le même email.

@calummackervoy
Copy link
Contributor Author

Yes bien sûr. Je vois le problème qu'on a raté sur le premier PR alors, voilà l'explication :

La test que j'ai ajouté echoue sans les autres changements proposés (un EmailInUseException est lancée).

Dans notre base de données, username et email sont les deux uniques. Dans l'ancien PR, on a changé le comportement (que le compte, inclusif du nom d'utilisateur, soit écrasé) à la lancement de cette EmailInUseException.

Le ticket impliqué était assez clair que uniquement les comptes PEConnect sont impliqués par cette EmailInUseException, et que sinon le comportement attendu était d'écraser le nom d'utilisateur.

@tonial
Copy link
Contributor

tonial commented Sep 24, 2024

Vu en semble en réunion : cette solution n'est pas souhaitable car le sub est l'identifiant unique pour un SSO donné.

Afin de bloquer la connexion via un autre mode de connexion je propose de modifier le bloc

user = User.objects.get(email=self.email)
created = False
if user.identity_provider not in [IdentityProvider.DJANGO, self.identity_provider]:
    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:
    self.handle_email_conflict_on_sso(user)

en un truc du genre

if user: = User.objects.get(email=self.email)
    self.handle_email_conflict_on_sso(user)
    return user, False

self.handle_email_conflict_on_sso(user) par défaut va lever une erreur EmailInUseException
et ou dans InclusionConnect on accepte si le user.identity_provider est Django
(car on veut permettre de mettre à jour le SSO de Django vers Inclusion Connect à partir de l'email)

@calummackervoy
Copy link
Contributor Author

@tonial je propose de merger ce PR dans l'état actuel pour corriger le bug et puis que j'ouvre un autre PR pour bloquer la mise à jour des comptes ? C'est pour déplacer le pression donné par un erreur 500 et nous laisser le temps à faire le processus complet sur le changement (la recette jetable etc)

@calummackervoy
Copy link
Contributor Author

Autre solution rapide / temporaire : envoyer ces utilisateurs de FranceConnect (ce sub n'existe pas, mais l'email est utilisé par un autre compte), à la même page d'erreur que j'ai mis en place pour le SSO PEConnect ?

@calummackervoy calummackervoy changed the title Connexion : corrige l'exception EmailInUseException lancée sur compte FranceConnect Connexion : Empêcher les utilisateurs de se connecter à un autre compte du SSO partageant un courriel existant Sep 24, 2024
@calummackervoy calummackervoy added modifié and removed no-changelog Ne doit pas figurer dans le journal des changements. labels Sep 24, 2024
@calummackervoy calummackervoy added no-changelog Ne doit pas figurer dans le journal des changements. and removed modifié labels Sep 24, 2024
Copy link
Contributor

@tonial tonial left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super !
J'ai ajouté quelques remarques sur les commentaires, mais sinon, c'est parfait !

itou/openid_connect/models.py Outdated Show resolved Hide resolved
itou/openid_connect/models.py Outdated Show resolved Hide resolved
itou/openid_connect/models.py Outdated Show resolved Hide resolved
… overloading

fix(openid_connect): catch EmailInUseException in all SSO providers

fix: pipeline failures

fix: comments
@calummackervoy calummackervoy added this pull request to the merge queue Sep 24, 2024
Merged via the queue into master with commit da62785 Sep 24, 2024
11 checks passed
@calummackervoy calummackervoy deleted the calum/fix-exception-email-in-use branch September 24, 2024 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Ne doit pas figurer dans le journal des changements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants