Pouvoir définir quels attributs de type téléphone sont à vocation identifiante pour l’usager (#78046) #75
vdeniaud
Loading…
Reference in New Issue
No description provided.
Delete Branch "wip/78046-password-authn-identifier-selection"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
db91642fbctoea4b59a9a8ea4b59a9a8tod5181a5dfa3dc3336db9to9372864bea9372864beatof5d2c390e4f5d2c390e4to957b00221f957b00221fto2e7276f5792e7276f579to7c61a5e9a57d3b8d4bb5toa2deb47587107f2cc0a5to43c8e37f3643c8e37f36to9d55ba27c99d55ba27c9to078ea11c44078ea11c44to5415bb6d915415bb6d91to913dc73bc7913dc73bc7to9099a2d2899099a2d289to09fbdcb83709fbdcb837to1b2a940a841b2a940a84to6cfaaa05216cfaaa0521tof9b33e2dd0f9b33e2dd0to9db24df86e9db24df86eto8a0fd753e48a0fd753e4toe82b35e423e82b35e423to7671e5a6d37671e5a6d3toa44bea2688a44bea2688to27aba4d69227aba4d692to66e637867c66e637867ctoec7b1caeb9WIP: Pouvoir définir quels attributs de type téléphone sont à vocation identifiante pour l’usager (#78046)to Pouvoir définir quels attributs de type téléphone sont à vocation identifiante pour l’usager (#78046)Voilà pour un premier tour, je n'ai rien de très intéressant à dire.
Par contre je suis assez opposé au dernier commit, c'est quel critère qui te fait dire qu'activer ou pas l'authentification par téléphone devrait être réservée aux super utilisateurs ? (par rapport à tout le reste qu'on peut faire dans ces vues, changer la politique du mot de passe, ouvrir les inscriptions, configurer un fournisseur SAML...)
@ -1959,3 +1957,3 @@if d not in block:block[d] = valueif d == 'user_filter' and app_settings.A2_ACCEPT_EMAIL_AUTHENTICATION:if d == 'user_filter' and get_password_authenticator().accept_email_authentication:Peut-être récupérer l'objet hors de la boucle, pour éviter une requête à chaque fois.
Oui complètement, c’est corrigé, merci.
@ -62,3 +63,3 @@)if not app_settings.A2_ACCEPT_PHONE_AUTHENTICATION or not get_user_model()._meta.get_field('phone'):if not utils_misc.get_password_authenticator().is_phone_authn_active:Tu peux directement utiliser
self.authenticator.C’est corrigé ici ainsi que tous les autres endroits où tu le mentionnes (+ quelques autres occurrences dans ce même formulaire).
@ -96,3 +103,3 @@def clean(self):if app_settings.A2_ACCEPT_PHONE_AUTHENTICATION and get_user_model()._meta.get_field('phone'):if utils_misc.get_password_authenticator().is_phone_authn_active:Pareil,
self.authenticator@ -125,0 +133,4 @@if not user.email and not (phone_authn_activeand models.AttributeValue.objects.filter(attribute=utils_misc.get_password_authenticator().phone_identifier_field,Pareil,
self.authenticator@ -1172,3 +1177,3 @@return self.perform_email_registration(form, email)if app_settings.A2_ACCEPT_PHONE_AUTHENTICATION:if utils_misc.get_password_authenticator().is_phone_authn_active:Pareil,
self.authenticator@ -1683,1 +1698,4 @@user = form.instanceif ((phone := getattr(self, 'phone', None))and (authn := utils_misc.get_password_authenticator())Pareil,
self.authenticator@ -1684,0 +1708,4 @@content=phone,attribute=authn.phone_identifier_field,)if authn.is_default_phone_authn_config:Ici on duplique donc la valeur de
User.phonedans un objet AttributeValue ? La raison c'est de ne pas complexifier les endroits où on filtre sur cet attribut ?Je ne mesure pas la probabilité des situations où les deux pourraient se trouver désynchronisés, ni leur impact.
Non, bonne remarque, il n’y a plus de raison à chercher à continuer à gérer une prétendue configuration par défaut qui serait plus simple. Je retire cela ainsi que la propriété is_default_phone_authn_config sur l’authentificateur, qui n’a plus lieu d’être.
Pour prévenir la désynchronisation il y avait eu
d3e64bd82au départ (qui s’assure que le contenu de l’attribut étendu reprend toujours le contenu du champ de modèle utilisateur, si existant), qui n’est maintenant plus pertinent et qu’on pourra virer dans une prochaine PR.@ -324,3 +324,2 @@def test_views_sms_ratelimit(app, db, simple_user, settings, freezer, view_name):def test_views_sms_ratelimit(app, db, simple_user, settings, freezer, view_name, phone_activated_authn):freezer.move_to('2020-01-01')LoginPasswordAuthenticator.objects.update(sms_ip_ratelimit='10/h', sms_number_ratelimit='3/d')Cosmétique, cette ligne n'aurait pas dû bouger
Oui en effet, j’ai rétabli.
Merci pour la relecture.
C’est une restriction temporaire pour refléter ce qui se fait actuellement avec les settings. L’authn, la récupération de mot de passe et l’enregistrement fonctionnent mais il manque encore la partie parcours de modification de numéro de téléphone similaire à l’email, pour que ça puisse arriver en prod sans tout casser (sans parler des instances dont les démarches présupposent que l’usager a un courriel, lesquelles seront à revoir). Il manque aussi le parcours de suppression où l’on re-vérifie la détention du numéro par l’usager connecté.
Pour l’instant la fonctionnalité peut donc être activée seulement au cas par cas en recette. Si l’on expose ces champs de config d’authentificateur à quiconque détient le rôle “Administrateur des moyens d’authentification” (ce qui est le cas de pas mal de clients, via héritage du rôle générique “Administrateur”), c’est sûr que la fonctionnalité va arriver par mégarde en prod et qu’il faudra ensuite rétablir le bon fonctionnement de l’instance depuis un état de données incohérent :/
ec7b1caeb9tob332ad2abdb332ad2abdtob13b68aaa1b13b68aaa1to6da76f6c6fD'accord, c'est donc temporaire, dans ce cas j'aimerais bien qu'on colle à ce qui est fait ailleurs, c'est à dire ajouter un feature flag qui contrôlerait l'affichage de ces options. L'avantage c'est que le changement à effectuer le moment venu est trivial, gérable par n'importe qui, vs ici où il y a quand même pas mal de code à comprendre et bouger.
Sinon, au moins splitter le commit pour vraiment en avoir un avec juste le code à revert le moment venu.
Oui c’est vrai, ce sera mieux. Le changement à faire dans le code en l’état actuel de la PR, pour ouvrir la fonctionnalité le moment venu, me paraissait assez simple, mais c’est encore plus direct avec un feature flag.
Je fais la correction. Merci.
Voilà, le commit en haut de branche est corrigé et le message adapté en conséquence.
70ec268580to049c4a4f82Arf j’ai oublié de découper en deux commits, je recommence.
Edit: Et on me dit de vive-voix que ça va très bien comme ça, ça m’arrange :)
049c4a4f82tofc9cc67c04@ -135,0 +148,4 @@resp = app.get('/manage/authenticators/%s/edit/' % authenticator.pk)assert list(resp.form.fields) == [Dernière remarque tatillonne, ça serait bien ici de vérifier que les champs sont bien apparus avec des « in » (ces listes explicites sont pénibles à maintenir (ma faute c'est moi qui ai ajouté celle plus haut))
Plutôt en manipulant des ensembles (et donc sans ce soucier d’un ordre quel qu’il soit), par exemple en testant que l’ensemble des champs écrit en dur à droite est bien inclus dans l’ensemble champs présentés par le formulaire ?
Pour moi c'est superflu de vérifier que les autres champs sont toujours là, mais comme tu veux.
À relire le code, comme plus bas on a
on vérifie déjà que les champs sont apparus, dont contrairement à ce que j'écrivais en parlant de « in », on pourrait juste supprimer ce assert sans remplacer.
Oui, fair enough, je retire ce assert et je merge.
0ccef49b72toe4cb69c1cb