Produits Entr'ouvert » Authentic 2

• Virer LDAP_ENABLE ne sert pas, si pas d'entity_id pas d'authent LDAP
• Renommer entity_id en saml_entity_id
• Le ENABLE_CONDITION ne sert à rien
• Virer tout src/authentic2_auth_saml/app_ldap_saml_settings.py
• Je ne comprends pas pourquoi dans SAMLLdapBackend n'utilise pas get_config()
• Renommer get_block_users() en get_users_with_config().
• Ne pas utiliser sync_ldap_users_filter, en fait il faut factoriser la recherche d'un utilisateur avec le code classique
• Ne pas rechercher le nom de l'attribut SAML via une regexp sur le filtrer, à la place avoir un attribut explicite saml_username_attribute et passer ce username au code normal de recherche d'un utilisateur (il faut minimiser le code qui diffère), en gros le authenticate, il devrait faire :
  

     username = self.get_username_from_saml_attributes(saml_attributes)
     user = super(SAMLLdapBackend, self).authenticate(username=username, without_password=True)
  

et ajouter la gestion d'un mode "sans mot de passe" ou bien éclater authenticate en deux morceaux réutilisables, la recherche des enregistrements LDAP et la validation des credentials.

Benjamin Dauvergne a écrit :

• Virer LDAP_ENABLE ne sert pas, si pas d'entity_id pas d'authent LDAP
• Renommer entity_id en saml_entity_id
• Le ENABLE_CONDITION ne sert à rien
• Virer tout src/authentic2_auth_saml/app_ldap_saml_settings.py
• Je ne comprends pas pourquoi dans SAMLLdapBackend n'utilise pas get_config()
• Renommer get_block_users() en get_users_with_config().
• Ne pas utiliser sync_ldap_users_filter, en fait il faut factoriser la recherche d'un utilisateur avec le code classique
• Ne pas rechercher le nom de l'attribut SAML via une regexp sur le filtrer, à la place avoir un attribut explicite saml_username_attribute et passer ce username au code normal de recherche d'un utilisateur (il faut minimiser le code qui diffère), en gros le authenticate, il devrait faire :
  [...]

et ajouter la gestion d'un mode "sans mot de passe" ou bien éclater authenticate en deux morceaux réutilisables, la recherche des enregistrements LDAP et la validation des credentials.

Et donc ce que je veux dire c'est dans le authenticate du backend de base faire un truc comme :

def authenticate(username=None, password=None):
    for dn in self.get_dn_from_username(username):
        if self.try_bind(dn, password):
           return self.get_user_from_dn(dn, password, conn, block)

Benjamin Dauvergne a écrit :

et ajouter la gestion d'un mode "sans mot de passe" ou bien éclater authenticate en deux morceaux réutilisables, la recherche des enregistrements LDAP et la validation des credentials.

authenticate doit dans ce cas faire appel à un authenticate_with_block pour que la recherche/validation de l'usager se fasse uniquement dans l'annuaire qui correspond au fournisseur SAML, ok.

Au moment ou j'écris ça je me rends compte que authenticate_block existe déjà, donc certainement ça à refactoriser.

Serghei Mihai a écrit :

Benjamin Dauvergne a écrit :

et ajouter la gestion d'un mode "sans mot de passe" ou bien éclater authenticate en deux morceaux réutilisables, la recherche des enregistrements LDAP et la validation des credentials.

authenticate doit dans ce cas faire appel à un authenticate_with_block pour que la recherche/validation de l'usager se fasse uniquement dans l'annuaire qui correspond au fournisseur SAML, ok.

Oui effectivement j'ai volontairement écris du code qui ne parle pas des blocks mais l'idée c'est effectivement de faire un get_dn_from_username(block, username) idéalement faudrait abstraire le concept d'un serveur LDAP et arrêter de passer block à tous les appels mais bon.

Serghei Mihai a écrit :

Ok, un peu de séparation du code d'authentification. Tests sur jenkins passent.
Testé également en local avec un authentic qui tape dans un autre, it works.

Le code dans try_bind a complètement changé, l'ordre n'est pas bon (pas de validation du mot de passe usager si if not block['connect_with_user_credentials']: c'est pas ça. T'as gagné le droit de faire un test de cet option.

Je vais passer #30577 avant celui-ci par droit d'aînesse.

Serghei Mihai a écrit :

Effectivement j'avais trop simplifié.

Il y a pourtant un test pour l'option connect_with_credentials (tests/test_ldap.py::test_no_connect_with_user_credentials) mais qui ne détectait pas d'erreur.
Je vais en rajouter, mais voici le patch à jour gardant au maximum la logique précedente.

Oui le test existant n'est pas suffisant, il faudrait modifier les ACLs de la base LDAP pour voir le souci (qu'un bind user ne donne pas accès à suffisamment de choses genre ni nom, prénom ou email).

En relisant et re-relisant le code je constate que dans le test il y a déjà l'ACL qui interdit la récupération des attributs lors d'un user bind:

@pytest.fixture
def slapd_strict_acl(slapd):
...

qui donc fait que get_ldap_attributes ne retourne rien et _return_user ne retourne pas d'objet User.

Comme la nouvelle méthode try_bind utilise la même logique:

        try:
            conn.simple_bind_s(dn, utf8_password)
            if not block['connect_with_user_credentials']:
                try:
                    self.bind(block, conn)
                except Exception as e:
                    log.exception(u'rebind failure after login bind')
                    return False
            return True
        except ldap.INVALID_CREDENTIALS:

le test ne detecte pas de regression.

Serghei Mihai a écrit :

Patch rebasé sur master.

T'as ajouté un force_bytes et ça ne devrait pas marcher... Je regarde pourquoi ça marche quand même.

Je revois aussi de mon côté.
Les builds précedents échouaient quand je faisais un force_bytes(password).
Mais rien sur le username, par ex.

Benjamin, je pense qu'on peut rejeter car on a maintenant la possibilité de rechercher le compte via les attributs reçu de django-mellon.