-
Notifications
You must be signed in to change notification settings - Fork 24
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
[GEN-1937] Connexion: Empêcher l'écrasement des comptes France Travail (courriels dupliqués) #4619
[GEN-1937] Connexion: Empêcher l'écrasement des comptes France Travail (courriels dupliqués) #4619
Conversation
508064e
to
5930ce1
Compare
itou/templates/utils/modal_includes/france_travail_registration_failure.html
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Beau travail !
Il reste à peaufiner la fonction qui masque partiellement le prénom et le nom.
5930ce1
to
1aab8ac
Compare
🥁 La recette jetable est prête ! 👉 Je veux tester cette PR ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
En fait, j'ai pensé à une amélioration pour éviter que le problème se produise sur d'autres SSO dans le futur ^^
itou/openid_connect/models.py
Outdated
@@ -124,6 +133,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: | |||
self.handle_email_overloaded_on_new_identity(user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je te propose finalement de directement mettre ici le raise EmailInUseException
car si jamais un autre SSO s'amuse à faire pareil on voudra empêcher que le problème s'y produise aussi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ça va casser quelques comportements existants, comme par example :
If there already is an existing user from InclusionConnect with this email, but under another username, we overwrite the username of the existing account.
J'ai créé handle_email_overloaded_on_new_identity
pour que les fournisseurs peuvent choisir leur propre comportement quand il y a un e-mail surchargé, j'aime que cette proposition impacte au minimum les autres SSO dans le cadre de ce ticket ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there already is an existing user from InclusionConnect with this email, but under another username, we overwrite the username of the existing account.
Non, dans ce cas là, on a une erreur qui indique à l'utilisateur que l'email est déjà utilisé.
Il vient d'où cet exemple ?
Dans un SSO Openid Connect, la clé d'identification unique d'un utilisateur est le sub (qu'on stocke dans le username chez nous).
Donc si l'utilisateur qui a le même email, a le meme identity provider mais un sub différent, c'est un utilisateur différent.
C'est le problème qu'on a sur pe_connect, et c'est une erreur de notre code que l'on cherche à corriger ici, pour tous les sub.
Donc si tu regardes le code :
-
d'abord on cherche un utilisateur qui a le sub renvoyé par le SSO
user = User.objects.get(username=self.username, identity_provider=self.identity_provider)
S'il existe, on met à jour ses infos avec celles du SSO. -
Sinon, on cherche un utilisateur qui a le même email
Si c'est un utilisateur django, on le converti en utilisateur de self.identity_provider
si c'est un utilisateur d'un autre SSO, on accepte de le connecter, sans le changer
Si c'est un utilisateur du même SSO c'est un problème (c'est le bug que tu corriges)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK ! Merci pour le détail. J'ai modifié divers tests échouants sur le signup - je pense que ces tests passaient mais sans l'état préliminaire prévu
1aab8ac
to
4c132f8
Compare
4c132f8
to
8a1532a
Compare
itou/templates/utils/modal_includes/france_travail_registration_failure.html
Show resolved
Hide resolved
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn, j'avais oublié qu'on avait cette fonction.
Je pense que tu pourrais juste garder mask_unless sans la changer, tant pis pour la dernière lettre des prénoms.noms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pourquoi enlèver le paramètre mask_function
, juste pour impacter moins du code ?
Son ajout à ce fonction ne touche pas les utilisations existantes et il nous permet à rendre "P****m N*m"
en place de "P… N…"
, et dans cette utilisation et je pense que ce n'est pas seulement une question d'esthétique - on veut donner assez des indices au utilisateurs qui connaisent le nom de cette personne parmis plusieurs autres possibilités.
Dans les autres cas on pourrait vouloir rédacter tous qu'un <redacted>
, et j'avais pensé aussi que dans la prochaine on pourrait paramétrer la génération de liste (re.split
) afin de remplacer / simplifier le fonction de redact_email_address et fournir un outil plus évolué etc
itou/users/models.py
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tu peux mettre un commentaire pour expliquer pourquoi tu n'utilises pas self.get_full_name()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bonne rémarque sur le commentaire - du coup je voulais éviter les transformations appliqué par get_full_name
- le nom capitalisé et que Mary Sue
deviendra M*****e
(qui perturberait l'utilisateur dans ce cas)
J'aimais pas qu'on utilise pas le get_full_name
dans une méthode appelée get_redacted_full_name
... mais je voulais pas parametriser le comportement dans get_full_name
. Je pourrais rénommer la fonction, ou le déplacer du modèle à une fonction utilitaire plus spécialisée ?
8a1532a
to
9b4b69d
Compare
@tonial mes retours te semble rationnels ? |
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 fix: requested changes refactor: move modals messages to modals block
9b4b69d
to
ec6500a
Compare
Désolé, j'ai du manquer la notification, c'est bon pour moi |
🤔 Pourquoi ?
Le SSO permet l’existence de plusieurs comptes avec le même email, ce qui fait qu’à chaque connexion depuis un compte ou un autre attaché au même email, on va écraser les données.
🚨 À vérifier
💻 Captures d'écran