Produits Entr'ouvert » Authentic 2

Je vois, dans le backend LDAP, la méthode _lookup_user_queryset :

return LDAPUser.objects.prefetch_related('groups').exclude(userexternalid__source=block['realm'])

Je pense que l’esprit du code de ce backend est que si des doublons existent dans l’annuaire, c’est de sa faute et qu’il ne faut pas chercher à corriger/réconcilier ça dans authentic. Je pense que le code est bon, mais que le test devrait créer un usager lambda, hors LDAP avec un mail bidon, et par ailleurs dans le LDAP un usager avec ce même mail BiDoN, et constater qu’aucun doublon n’est créé à l’import.

Dans 0002 le assert User.objects.count() == 7 : c'est 6 qu'on veut (pas de nouveau compte, on doit rester à 6).

Mais je suis super ultra frileux sur la modif LDAPUser/User, ça me semble très impactant.

Au final j'ai l'impression que les lookups sont bien prévus pour ne travailler que sur les comptes LDAP, que c'est fait à dessein. Donc s'il y a des doublons parce que d'autres comptes non-LDAP existaient, c'est "normal", ils n'auraient juste pas dû exister. Que la source soit une mauvaise manip ou défaut de configuration, ça doit être réglé autrement. Auquel cas je me serais trompé sur mon analyse et je rejetterai ce ticket.

Bref : je préfère que Benjamin vienne dire.

Thomas Noël a écrit :

Dans 0002 le assert User.objects.count() == 7 : c'est 6 qu'on veut (pas de nouveau compte, on doit rester à 6).

Cette fois-ci on crée bien un utilisateur hors LDAP dans le test (avec une adresse qui n’a rien à voir avec les "etienne.michu@…" créés jusque là), ça en fait un en plus de ceux de la synchro. On ajoute ce même usager avec une casse différente dans le ldif et on vérifie qu’on est bien à 7 (et non pas à 8 si doublon il y avait eu).

Mais je suis super ultra frileux sur la modif LDAPUser/User, ça me semble très impactant.

Oui, changement de paradigme, on cherche à réconcilier les usagers issus de l’annuaire sur une base de comptes déjà existante dans Publik.

Au final j'ai l'impression que les lookups sont bien prévus pour ne travailler que sur les comptes LDAP, que c'est fait à dessein. Donc s'il y a des doublons parce que d'autres comptes non-LDAP existaient, c'est "normal", ils n'auraient juste pas dû exister. Que la source soit une mauvaise manip ou défaut de configuration, ça doit être réglé autrement. Auquel cas je me serais trompé sur mon analyse et je rejetterai ce ticket.

Oui c’était l’esprit du code jusque là : un compartimentation des comptes LDAP sans chercher à résoudre les doublons avec des comptes hors-LDAP qui pré-existeraient à la synchro.

Est-ce que tu vois une autre solution à #67318 qui semble-t-il motivait la création de ce ticket ci ?

Paul Marillonnet a écrit :

Thomas Noël a écrit :

Dans 0002 le assert User.objects.count() 7 : c'est 6 qu'on veut (pas de nouveau compte, on doit rester à 6).

Cette fois-ci on crée bien un utilisateur hors LDAP dans le test (avec une adresse qui n’a rien à voir avec les "etienne.michu@…" créés jusque là), ça en fait un en plus de ceux de la synchro. On ajoute ce même usager avec une casse différente dans le ldif et on vérifie qu’on est bien à 7 (et non pas à 8 si doublon il y avait eu).

Je vais retourner apprendre à lire :)

Je verrais bien un assert User.objects.count() 7 avant le users = list(ldap_backend.LDAPBackend.get_users()), qu'on voit bien que ça n'a pas bougé. Et aussi un test que len(users) est bien aussi à 7, un utilisateur de plus.

Mais je suis super ultra frileux sur la modif LDAPUser/User, ça me semble très impactant.

Oui, changement de paradigme, on cherche à réconcilier les usagers issus de l’annuaire sur une base de comptes déjà existante dans Publik.

Voilà. Comme on a de la synchro LDAP à plein d'endroit, c'est bien ce changement de paradigme qu'il faut qu'on valide bien ensemble.

Aussi, si je comprends le code 0001, en cas de découverte d'un User qui a le même mail, alors on ne va pas mettre à jour cet utilisateur existant avec nom/prénom/etc venant du LDAP. Ce n'est pas vraiment ce qui est attendu/imaginé je pense : on préférerait que le compte User trouvé "devienne" un LDAPUser et qu'il soit dès lors synchronisé.

Est-ce que tu vois une autre solution à #67318 qui semble-t-il motivait la création de ce ticket ci ?

Pour l'instant je propose un nettoyage du compte qui a été créé manuellement et qui "plante" la compréhension et le bon fonctionnement.

Le but ici c'est la réconciliation entre des comptes LDAP nouveaux et des comptes autocréés OIDCs existant (pour éviter la problème de poule et d'oeuf pour les villes raccordées en OIDC et LDAP), la recherche doit se faire dans l'OU visée par le LDAP sans considération pour l'état du raccordement au LDAP ou pas et bien sûr insensiblement à la casse (après on peut avoir des soucis de casse différente entre OIDC et LDAP, ben c'est nul, ça se mettra à jour à chaque fois, ce n'est pas l'objet du ticket).

Benjamin Dauvergne a écrit :

Le but ici c'est la réconciliation entre des comptes LDAP nouveaux et des comptes autocréés OIDCs existant (pour éviter la problème de poule et d'oeuf pour les villes raccordées en OIDC et LDAP), la recherche doit se faire dans l'OU visée par le LDAP sans considération pour l'état du raccordement au LDAP ou pas et bien sûr insensiblement à la casse (après on peut avoir des soucis de casse différente entre OIDC et LDAP, ben c'est nul, ça se mettra à jour à chaque fois, ce n'est pas l'objet du ticket).

On dirait qu'on est dans le même feeling donc.

Le problème que j'ai eu chez le client c'est la réconciliation avec des comptes créés manuellement avant la mise en place du LDAP : on peut considérer que c'est pareil qu'un compte créé précédemment via OIDC et donc, selon moi, on est dans un cas de figure équivalent.

Ce que je voudrais juste qu'on vérifie c'est que les comptes déjà créés (par OIDC ou autre) deviennent bien, lorsqu'il sont retrouvés selon le mail à l'occasion du sync-ldap, des comptes identifiables comme synchronisés en LDAP. Ca me semblerait logique et donc facile à comprendre.

(On s'écarte un poil un peu de l'objet du ticket qui parle de "sensible à la casse du mail", mais l'objectif reste bien de ne jamais provoquer la création de doublon)

Thomas Noël a écrit :

Ce que je voudrais juste qu'on vérifie c'est que les comptes déjà créés (par OIDC ou autre) deviennent bien, lorsqu'il sont retrouvés selon le mail à l'occasion du sync-ldap, des comptes identifiables comme synchronisés en LDAP. Ca me semblerait logique et donc facile à comprendre.

Oui c'est exactement ce qui se passe déjà, modulo le fait que ça n'ignore pas la casse et ça devrait (ma faute).

Benjamin Dauvergne a écrit :

Thomas Noël a écrit :

Ce que je voudrais juste qu'on vérifie c'est que les comptes déjà créés (par OIDC ou autre) deviennent bien, lorsqu'il sont retrouvés selon le mail à l'occasion du sync-ldap, des comptes identifiables comme synchronisés en LDAP. Ca me semblerait logique et donc facile à comprendre.

Oui c'est exactement ce qui se passe déjà, modulo le fait que ça n'ignore pas la casse et ça devrait (ma faute).

Sauf erreur, même avec un mail totalement identique, il y a création d'un nouvel utilisateur. Ca semble lié (comme dit Paul) au fait qu'on recherche le lookup dans les LDAPUser et pas dans les User...?

Thomas Noël a écrit :

Benjamin Dauvergne a écrit :

Thomas Noël a écrit :

Ce que je voudrais juste qu'on vérifie c'est que les comptes déjà créés (par OIDC ou autre) deviennent bien, lorsqu'il sont retrouvés selon le mail à l'occasion du sync-ldap, des comptes identifiables comme synchronisés en LDAP. Ca me semblerait logique et donc facile à comprendre.

Oui c'est exactement ce qui se passe déjà, modulo le fait que ça n'ignore pas la casse et ça devrait (ma faute).

Sauf erreur, même avec un mail totalement identique, il y a création d'un nouvel utilisateur. Ca semble lié (comme dit Paul) au fait qu'on recherche le lookup dans les LDAPUser et pas dans les User...?

Je ne met pas en doute ce qui se passe, mais normalement LDAPUser est un modèle "proxy" donc il se comporte exactement comme User, sur mon instance locale publik.dev :

In [7]: User.objects.count()
django.db.backends DEBUG (0.001) SET search_path = authentic_dev_publik_love,public; args=None
django.db.backends DEBUG (0.001) SET application_name = authentic_dev_publik_love; args=None
django.db.backends DEBUG (0.004) SELECT COUNT(*) AS "__count" FROM "custom_user_user"; args=()
Out[7]: 10008

In [8]: LDAPUser.objects.count()
django.db.backends DEBUG (0.000) SET search_path = authentic_dev_publik_love,public; args=None
django.db.backends DEBUG (0.000) SET application_name = authentic_dev_publik_love; args=None
django.db.backends DEBUG (0.003) SELECT COUNT(*) AS "__count" FROM "custom_user_user"; args=()
Out[8]: 10008

C'est pareil, il y a certainement autre chose. J'ai regardé le ticket Nantes lié mais je n'y vois pas la configuration complète, donc je ne sais pas si ils ont joué avec les lookups, normalement la valeur actuelle fait tout bien comme il faut ('guid', 'external_id', 'username', 'email') mais si c'est modifié ça peut poser problème. Le mieux ce serait d'avoir un test qui reproduit le/les problèmes a minima (sans passer par OIDC, juste en créant d'autres utilisateurs avec le même mail, 1, plusieurs, dans différentes OUs).

Thomas Noël a écrit :

Ci-joint une tentative qui ne fonctionne pas, le tox :
[...]

Donc : je n'arrive pas à faire ou correctif, ou le test, ou les deux...

Ton patch était bon et le comportement que vous avec compris aussi: ça ne cherche que dans les comptes pas encore reliés au même LDAP, effectivement si il y a un doublon du coté de l'annuaire LDAP on ne le résout pas, le premier compte qui tentera la liaison (par synchro ou au login) gagnera la liaison avec le compte existant.

Ton test ne marche pas car tu ne rajoutes pas de nouveau compte pour créer de liaison avec, il faut mettre une email quelconque avec des majuscules dans ton LDIF est créé un compte local avec la même email capitalisée autrement. Pas tenté de réutiliser l'EMAIL déjà présent, qui est déjà relié à un autre compte.

Benjamin Dauvergne (bdauvergne) a ouvert une pull request sur Gitea concernant cette demande :

• URL : https://git.entrouvert.org/entrouvert/authentic/pulls/258
• Titre : WIP: ignorer la casse lors de la recherche d'un nouvel utilisateur LDAP (#67600)
• Modifications : https://git.entrouvert.org/entrouvert/authentic/pulls/258/files

Je suis reparti du patch de Thomas ne voyant pas bien l'utilité du 0001 de Paul (et le changement LDAPUser -> User cassait pas mal de trucs). J'en ai profité pour faire du ménage dans test_get_users, pour bien séparer les cas testés, le cas générique et le cas casse de l'adresse de courriel utilisent maintenant la configuration par défaut de "lookups".

Au passage ça a changé l'ordre de remonté des utilisateurs et donc je l'ai rendu déterministe lorsqu'on cherche l'uuid du premier utilisateur dans test_sync_ldap_users.