Development #8627
Migrer le code SAML de récupération des attributs de auquotidien dans w.c.s.
0%
Description
Le code actuellement dans w.c.s. est incapable de créer un utilisateur qui n'existe pas, le système par double authentification ayant été retiré il faudrait complètement déplacer ce code d'auquotidien vers w.c.s.
Au passage il faudrait améliorer les logs comme:
fill_user_attributes: received attributes {'username': ['fpeters@entrouvert.com'], 'city': [], 'first_name': ['Fr\xc3\xa9d\xc3\xa9ric'], 'last_name': ['P\xc3\xa9ters'], 'is_superuser': ['true'], 'title': [], 'mobile': [], 'zipcode': [], 'phone': [], 'role-slug': ['33024ecaff154fb0ae39f6263f7e1d83', 'a2ccc25618464b1f8fd61c6e28c0f663'], 'address': [], 'email': ['fpeters@entrouvert.org']}
qui sont inutilement verbeux, on ne devrait signaler que les changements.
Aussi en cas de création l'utilisateur est rapporté comme étant None (valeur de user.id à ce moment là), il faudrait corriger cela.
Fichiers
Révisions associées
tests: add a saml auth test with nid format "unspecified" (#8627)
remove lookup_user from Saml2Directory (#8627)
it's now in w.c.s.
Historique
Mis à jour par Frédéric Péters il y a plus de 8 ans
C'est identification_token que tu appelles quoi le "système par double authentification" ?
Mis à jour par Benjamin Dauvergne il y a presque 8 ans
- Fichier 0001-import-Saml2Directory.lookup_user-from-auquotidien-8.patch 0001-import-Saml2Directory.lookup_user-from-auquotidien-8.patch ajouté
- Assigné à mis à Benjamin Dauvergne
- Patch proposed changé de Non à Oui
J'ai du modifier les tests qui ne passaient plus parce que maintenant le SSO ne passe que si il y a au moins un nom et un email dans les attributs. J'ai du modifier aussi le test avec de multiples IdP les pré-supposés étant faux (il n'y a pas de "premier IdP" qu'on puisse vraiment déterminer, ça dépend de la valeur des hashs, et avec tox ça change).
Mis à jour par Benjamin Dauvergne il y a presque 8 ans
Au passage ça corrige le possible double provisionning d'un utilisateur (c'est l'object principal du déterrage de ce ticket).
Mis à jour par Benjamin Dauvergne il y a presque 8 ans
- Fichier 0001-remove-custom-Saml2Directory-8627.patch 0001-remove-custom-Saml2Directory-8627.patch ajouté
Le nettoyage coté auquotidien.
Mis à jour par Benjamin Dauvergne il y a plus de 7 ans
- Fichier 0001-import-Saml2Directory.lookup_user-from-auquotidien-8.patch 0001-import-Saml2Directory.lookup_user-from-auquotidien-8.patch ajouté
Nouveau patch où je rends aussi déterministe l'ordre des IdPs en fonction de leur clé pour que les tests passent toujours (j'ai eu un ordre différent des clés entre le test et la vue de login :/ )
Mis à jour par Thomas Noël il y a plus de 7 ans
Ack pour moi mais je pense qu'il ne faut pas pousser cela maintenant, ie attendre au moins après la mise en prod du 15 septembre. Fred ?
Je ne vois pas d'impact majeur par rapport au code précédent. Il apparait plus clairement que user.name_identifiers sera toujours une liste avec un seul élément (potentiellement y'avait précédement une possiblité éventuelle d'avoir plusieurs idp, mais c'était un peu mal fichu et je pense même non fonctionnel).
Mis à jour par Frédéric Péters il y a plus de 7 ans
Non, à ne pas pousser.
Il manque des mots dans le message de commit.
On peut avoir plusieurs IdP; on peut avoir plusieurs éléments dans name_identifiers, ça a carrément été utile sur des migrations.
Je n'ai pas lu le diff.
Mis à jour par Frédéric Péters il y a plus de 7 ans
Ça m'ennuie d'avoir legacy_fill_user_attributes dont le nom marque bien le côté bagage historique qu'on laisserait bien dans auquo.
Mis à jour par Benjamin Dauvergne il y a plus de 7 ans
- Fichier 0001-import-Saml2Directory.lookup_user-from-auquotidien-8.patch 0001-import-Saml2Directory.lookup_user-from-auquotidien-8.patch ajouté
Frédéric Péters a écrit :
Il manque des mots dans le message de commit.
On peut avoir plusieurs IdP; on peut avoir plusieurs éléments dans name_identifiers, ça a carrément été utile sur des migrations.
Je n'ai pas supprimé cette possibilité, pas de panique :) Thomas interprète les choses de travers. On peut toujours manuellement avoir plusieurs name_id sur un utilisateur, même dans le passé via le SSO simple il n'était pas possible d'avoir plusieurs NameID sur un même utilisateur, c'était uniquement via la technique du token qui elle a été supprimée. Donc il ne reste que la technique manuelle.
Je n'ai pas lu le diff.
Message de commit corrigé.
Mis à jour par Benjamin Dauvergne il y a plus de 7 ans
Frédéric Péters a écrit :
Ça m'ennuie d'avoir legacy_fill_user_attributes dont le nom marque bien le côté bagage historique qu'on laisserait bien dans auquo.
Je veux bien le virer mais on est bien sûr de ne plus en avoir besoin nulle part ?
Mis à jour par Thomas Noël il y a plus de 7 ans
Benjamin Dauvergne a écrit :
Frédéric Péters a écrit :
Il manque des mots dans le message de commit.
On peut avoir plusieurs IdP; on peut avoir plusieurs éléments dans name_identifiers, ça a carrément été utile sur des migrations.
Je n'ai pas supprimé cette possibilité, pas de panique :) Thomas interprète les choses de travers.
Plusieurs IdP j'ai du mal à voir comment, mais soit.
Mis à jour par Frédéric Péters il y a plus de 7 ans
Je veux bien le virer mais on est bien sûr de ne plus en avoir besoin nulle part ?
Ce que j'imaginerais c'est le garder dans le module auquotidien, pas le déplacer ici.
Mis à jour par Benjamin Dauvergne il y a plus de 7 ans
- Fichier 0001-remove-lookup_user-from-Saml2Directory-8627.patch 0001-remove-lookup_user-from-Saml2Directory-8627.patch ajouté
- Fichier 0001-import-Saml2Directory.lookup_user-from-auquotidien-8.patch 0001-import-Saml2Directory.lookup_user-from-auquotidien-8.patch ajouté
Avec legacy_fill_user_attributes remis dans auquotidien et aussi j'ai du modifier les tests parce qu'ils dépendaient de legacy_fill_user_attributes, pour me simplifier la vie je suis parti d'un profil utilisateur défini via hobo_provision comme dans les tests sur hobo_notify.
Mis à jour par Frédéric Péters il y a plus de 7 ans
Je tourne en local avec une version manuellement rebasée depuis quelques semaines et c'est ok; ça m'irait bien de pousser ça maintenant, que ça vive sur la recette pendant une grosse semaine.
Mis à jour par Frédéric Péters il y a environ 7 ans
Oui, j'écrivais « Ça m'irait bien de pousser ça maintenant ». (pour profiter de la recette le plus longtemps possible et ce point est raté, mais bon).
Mis à jour par Frédéric Péters il y a environ 7 ans
C'est devenu trop tard pour ce cycle, pensons-y mardi prochain.
Mis à jour par Frédéric Péters il y a environ 7 ans
Surtout qu'à tester à neuf je me rends compte que sur un déploiement publik ça éclate le premier SSO des utilisateurs.
Il faudrait un test avec lasso.SAML2_NAME_IDENTIFIER_FORMAT_UNSPECIFIED, qui fera que login.identity sera None, ce qui montrera le patch suivant nécessaire :
--- a/wcs/qommon/saml2.py
+++ b/wcs/qommon/saml2.py
@@ -517,7 +517,8 @@ class Saml2Directory(Directory):
else:
user = get_publisher().user_class(ni)
user.name_identifiers = [ni]
- user.lasso_dump = login.identity.dump()
+ if login.identity:
+ user.lasso_dump = login.identity.dump()
user.store()
others = user_class.get_users_with_name_identifier(ni)
(patch pas testé plus que ça, manque peut-être d'autres éléments)
Mis à jour par Benjamin Dauvergne il y a environ 7 ans
J'ai mis dans mon calendrier de repasser sur ce ticket mardi prochain, à mardi donc :)
Mis à jour par Benjamin Dauvergne il y a environ 7 ans
- Fichier 0001-import-Saml2Directory.lookup_user-from-auquotidien-8.patch 0001-import-Saml2Directory.lookup_user-from-auquotidien-8.patch ajouté
- Fichier 0002-tests-add-a-saml-auth-test-with-nid-format-unspecifi.patch 0002-tests-add-a-saml-auth-test-with-nid-format-unspecifi.patch ajouté
- Priorité changé de Bas à Normal
- correction signalée ajoutée ainsi que le test en rapport
Je pousserai bien la chose telle quelle vu qu'on est en début de cycle si ça ne marche pas on le verra assez vite.
Mis à jour par Frédéric Péters il y a environ 7 ans
(pas oublier le patch côté auquotidien pour retirer le code)
Mis à jour par Thomas Noël il y a environ 7 ans
- Assigné à changé de Benjamin Dauvergne à Thomas Noël
Frédéric Péters a écrit :
(pas oublier le patch côté auquotidien pour retirer le code)
Je m'occupe d'adapter ça.
Mis à jour par Thomas Noël il y a environ 7 ans
- Statut changé de En cours à Résolu (à déployer)
Poussés par Benjamin entre temps (il manque un bout concernant les attributs vérifiés, je fais un autre ticket)
Mis à jour par Frédéric Péters il y a plus de 5 ans
- Statut changé de Résolu (à déployer) à Solution déployée
import Saml2Directory.lookup_user() from auquotidien (#8627)
SAML authentication tests had to be changed since with this new code display
name and an email are mandatory when creating an new user during SSO.
Also order idps by their key before using them.