Project

General

Profile

Development #55557

Ajouter une vue de debug de l'authentification

Added by Valentin Deniaud 3 months ago. Updated about 2 months ago.

Status:
Solution déployée
Priority:
Normal
Target version:
-
Start date:
13 Jul 2021
Due date:
% Done:

0%

Estimated time:
Patch proposed:
Yes
Planning:
No

Description

Benjamin Dauvergne a écrit (#55124) :

une vue de debug accessible quand on est superadmin, ça reprendrait globalement le code de la vu de login sauf la partie qui pose l'utilisateu ne session, ça ferait un rendu de tout ce qui est obtenu (attributs / assertions) dans une page HTML


Files

0001-wip.patch (3.3 KB) 0001-wip.patch Valentin Deniaud, 13 Jul 2021 11:37 AM
0001-wip.patch (6.97 KB) 0001-wip.patch Valentin Deniaud, 19 Jul 2021 05:04 PM
0001-views-move-login-code-to-separate-method-55557.patch (2.98 KB) 0001-views-move-login-code-to-separate-method-55557.patch Valentin Deniaud, 03 Aug 2021 12:05 PM
0002-views-add-debug-login-view-55557.patch (6.9 KB) 0002-views-add-debug-login-view-55557.patch Valentin Deniaud, 03 Aug 2021 12:05 PM

Associated revisions

Revision dde8fa5d (diff)
Added by Valentin Deniaud about 2 months ago

views: move login code to separate method (#55557)

Revision dbdd6fd7 (diff)
Added by Valentin Deniaud about 2 months ago

views: add debug login view (#55557)

History

#1

Updated by Valentin Deniaud 3 months ago

Ça fonctionne mais c'est pas le plus beau patch du monde, je veux bien un avis avant ajout de tests et compagnie.

En todo il y aurait aussi à ajouter un peu de code pour afficher du joli XML, là ça fait une page de 20km de large.

#2

Updated by Frédéric Péters 3 months ago

Je me pose la question de l'utilité, souvent on a à débugguer un accès par un tiers, pas son propre compte admin. Dernier exemple en date, #55353#note-3, où on reste à devoir deviner "Il doit manquer une des clés qui sont indiqués comme mandatory", et ce patch n'aidera pas.

On ne pourrait pas imaginer, authentic, journaliser toutes les assertions (valides/signées) reçues ?

Ainsi on aurait au max à demander à quelle heure la tentative, filtrer le journal vers ce moment, et lire l'assertion.

#3

Updated by Benjamin Dauvergne 3 months ago

Frédéric Péters a écrit :

Je me pose la question de l'utilité, souvent on a à débugguer un accès par un tiers, pas son propre compte admin. Dernier exemple en date, #55353#note-3, où on reste à devoir deviner "Il doit manquer une des clés qui sont indiqués comme mandatory", et ce patch n'aidera pas.

Il y a déjà un message qui s'affiche dans ce cas; là il n'est pas là, donc ce n'est peut-être pas le problème. L'assertion aurait été poussé dans les logs que ce serait pareil, vu qu'on a plus les logs.

On ne pourrait pas imaginer, authentic, journaliser toutes les assertions (valides/signées) reçues ?

Oui, mais avec des logs de deux semaines, et deux semaines pour que quelqu'un regarde dans les logs ça n'aidera pas.

#4

Updated by Frédéric Péters 3 months ago

Quand j'écris "journaliser" je pensais au journal, /manage/journal/, il n'y a pas cette expiration à deux semaines là.

#5

Updated by Benjamin Dauvergne 3 months ago

Frédéric Péters a écrit :

Quand j'écris "journaliser" je pensais au journal, /manage/journal/, il n'y a pas cette expiration à deux semaines là.

Alors là ça devient un autre ticket, il faudrait un logging.Handler à poser sur les domaines authentic2_auth_saml et mellon qui enverrait les messages de auth_saml/mellon dans le journal, je suppose qu'il faudrait aussi ajouter au journal la possibilité d'afficher des popups dans le style des logs passerelle pour les objets un peu gros ne rentrant pas sur une ligne (style un message SAML). Ceci pour éviter d'avoir douzes façons de logger dans le code.

#6

Updated by Valentin Deniaud 3 months ago

Benjamin Dauvergne a écrit :

Alors là ça devient un autre ticket

Moi je pensais que c'était exclu parce que ce message SAML est gros : encodé en ASCII chez moi ça fait 6Ko, si on se dit qu'on a 5000 connexions par jour par SAML, ça fait 1Go de données par mois. Voilà j'ai aucune idée de si c'est signifiant ou si c'est rien du tout.

il faudrait un logging.Handler à poser sur les domaines authentic2_auth_saml et mellon qui enverrait les messages de auth_saml/mellon dans le journal

En moins compliqué mais peut-être plus schlag, request.session['mellon_saml_response'] = saml_response et libre à qui utilise mellon d'accéder à ça et d'en faire ce qu'il veut ? C'est deux patches d'une ligne pour arriver à ce qu'on veut.

je suppose qu'il faudrait aussi ajouter au journal la possibilité d'afficher des popups dans le style des logs passerelle pour les objets un peu gros ne rentrant pas sur une ligne (style un message SAML)

Ça oui mais ça me paraît une bonne évolution.

#7

Updated by Benjamin Dauvergne 3 months ago

Valentin Deniaud a écrit :

Benjamin Dauvergne a écrit :

Alors là ça devient un autre ticket

Moi je pensais que c'était exclu parce que ce message SAML est gros : encodé en ASCII chez moi ça fait 6Ko, si on se dit qu'on a 5000 connexions par jour par SAML, ça fait 1Go de données par mois. Voilà j'ai aucune idée de si c'est signifiant ou si c'est rien du tout.

Je n'avais pas fait le calcul en tête, mais oui ça doit être ça, c'est pour ça que je suggérai une vue de debug parce que le cas nominal ce sont les soucis au premier raccordement, où un développeur/CPT est en train de faire le raccordement et donc peut utiliser cette vue de debug.

Le cas lié ici d'une personne pour laquelle ça ne marche pas, alors que ça marche pour les autres, ça devrait être réglé par des logs précis que normalement on a déjà, la résolution du ticket nous éclairera sur ce point. Je ne suis pas certain que dumper l'assertion nous aide plus.

il faudrait un logging.Handler à poser sur les domaines authentic2_auth_saml et mellon qui enverrait les messages de auth_saml/mellon dans le journal

En moins compliqué mais peut-être plus schlag, request.session['mellon_saml_response'] = saml_response et libre à qui utilise mellon d'accéder à ça et d'en faire ce qu'il veut ? C'est deux patches d'une ligne pour arriver à ce qu'on veut.

Oui aussi.

je suppose qu'il faudrait aussi ajouter au journal la possibilité d'afficher des popups dans le style des logs passerelle pour les objets un peu gros ne rentrant pas sur une ligne (style un message SAML)

Ça oui mais ça me paraît une bonne évolution.

Oui mais avant de se disperser je me dis qu'il faut comprendre pourquoi dans le cas lié on a pas le message d'erreur en front avec l'erreur de mapping qu'on aurait du avoir si c'est vraiment une erreur de mapping (manque un attribut) qui a bloqué le SSO.

#8

Updated by Benjamin Dauvergne 3 months ago

Je reviens au patch, plutôt que d'utiliser lasso_node_dump() utiliser lasso_node_debug(), via response_dump = login.response and login.response.debug(4) (4 c'est l'indentation), je dumperai login.assertion aussi (c'est dans la réponse, mais c'est plus court à analyser).

Et les choses dans cet ordre, de plus utile au moins utile :
  • attributs
  • assertion
  • response
  • artifact/POST (parce que msgBody ça peut aussi être le contenu du POST en cas de binding POST, en fait ce sera la même chose que dans login.response mais encodé).

Est-ce qu'on ne laisserait pas la vue active pour tout le monde aussi si DEBUG = True ?


Et si le cas DEBUG = True est implémenté: j'ai l'impression qu'une fois mellon_debug_login posé il ne se ré-initialise jamais, il faudrait le virer dans DebugLoginView.authenticate, et ajouter un lien "Try Again" au template (<a class="button" href="?">Try again</a>).

#9

Updated by Benjamin Dauvergne 3 months ago

Et dernière remarque : je laissera l'authentification se faire (via django.contrib.auth.authenticate()), juste je ne loggerai pas l'utilisateur (on shunte l'appel à utils.login(...)), et j'afficherai l'utilisateur qui serait authentifié ainsi que tous les logs entre temps (via https://docs.python.org/3/library/logging.handlers.html#memoryhandler, posé sur le logger mellon et authentic2_auth_saml temporairement). Comme ça on sait tout.

PS: aussi exemple, https://docs.python.org/3/howto/logging-cookbook.html#buffering-logging-messages-and-outputting-them-conditionally

#10

Updated by Valentin Deniaud 2 months ago

Voilà j'ai implémenté tes idées, j'attends quand même une dernière approbation avant de fignoler et d'écrire les tests.

Benjamin Dauvergne a écrit :

tous les logs entre temps (via https://docs.python.org/3/library/logging.handlers.html#memoryhandler, posé sur le logger mellon et authentic2_auth_saml temporairement). Comme ça on sait tout.

Je pense qu'on a pas le droit de faire référence à authentic2_auth_saml mais poser le handler sur le root logger ça fait ce qu'on veut j'ai l'impression. Aussi pas compris l'histoire de MemoryHandler, j'ai fait sans.

#11

Updated by Thomas Noël (congés → 11 octobre) 2 months ago

Selon moi "if not request.user.is_superuser" est en trop, ça va compliquer le debug quand on n'aura pas un accès à l'idp (pas de compte test ou pas d'accès à l'idp du tout : mon idée est est dans ce cas on inviterait le client à tester par lui même et nous envoyer la page de résultat en copier-coller).

Aussi, y'a un self.log.root.addHandler, mais pas de suppression ensuite : est-ce qu'on ne risque pas ici d'activer un log permanent par la suite ? (Si c'est trop compliqué, je pense qu'on peut retirer les logs dans un premier temps, histoire d'avoir au moins un debug SAML de base)

#12

Updated by Benjamin Dauvergne 2 months ago

Valentin Deniaud:

Je pense qu'on a pas le droit de faire référence à authentic2_auth_saml mais poser le handler sur le root logger ça fait ce qu'on veut j'ai l'impression. Aussi pas compris l'histoire de MemoryHandler, j'ai fait sans.

Parce qu'à la lecture de la doc de logging je me suis laissé à penser qu'il fallait faire absolument comme ça, mais ton StreamHandler sur un io.StreamIO semble effectivement encore plus simple et naturel.

Thomas Noël a écrit :

Selon moi "if not request.user.is_superuser" est en trop, ça va compliquer le debug quand on n'aura pas un accès à l'idp (pas de compte test ou pas d'accès à l'idp du tout : mon idée est est dans ce cas on inviterait le client à tester par lui même et nous envoyer la page de résultat en copier-coller).

Ça me gêne vraiment que ce soit ouvert tout le temps (les audits à la con tout ça), mais si superuser est jugé trop dur, on peut se baser sur settings.DEBUG; pour l'utiliser en prod pour nous ou un client il suffit de mettre le ou les IPs concernée dans les IPs de debug dans hobo (/debug/) sur hobo.

Aussi, y'a un self.log.root.addHandler, mais pas de suppression ensuite : est-ce qu'on ne risque pas ici d'activer un log permanent par la suite ? (Si c'est trop compliqué, je pense qu'on peut retirer les logs dans un premier temps, histoire d'avoir au moins un debug SAML de base)

Oui pareil pour addHandler j'ai fait une remarque équivalente sur un addFilter sur un ticket passerelle, faut faire attention quand on touche aux loggers, sinon on laisse du bordel derrière, j'en ferai un contextmanager comme je l'ai dit dans l'autre ticket, c'est le code pythonesque normal pour allouer/désallouer une ressource.

#13

Updated by Thomas Noël (congés → 11 octobre) 2 months ago

Benjamin Dauvergne a écrit :

Thomas Noël a écrit :

Selon moi "if not request.user.is_superuser" est en trop, ça va compliquer le debug quand on n'aura pas un accès à l'idp (pas de compte test ou pas d'accès à l'idp du tout : mon idée est est dans ce cas on inviterait le client à tester par lui même et nous envoyer la page de résultat en copier-coller).

Ça me gêne vraiment que ce soit ouvert tout le temps (les audits à la con tout ça), mais si superuser est jugé trop dur, on peut se baser sur settings.DEBUG; pour l'utiliser en prod pour nous ou un client il suffit de mettre le ou les IPs concernée dans les IPs de debug dans hobo (/debug/) sur hobo.

Ouaip mais bon, DEBUG=True ça pourrait suffire, selon moi (ça s'expliquer lors d'un audit)... Dans le wip proposé c'est « if not request.user.is_superuser or not settings.DEBUG: » et je proposais de remplacer par « if not settings.DEBUG: »...

Ceci dit, ça ferme le debug en prod (on n'y fait jamais DEBUG=True), et donc, pourquoi pas : « if not (settings.DEBUG or request.user.is_superuser): » ?

#14

Updated by Benjamin Dauvergne 2 months ago

Thomas Noël a écrit :

Ceci dit, ça ferme le debug en prod (on n'y fait jamais DEBUG=True), et donc, pourquoi pas : « if not (settings.DEBUG or request.user.is_superuser): » ?

On peut toujours activer le DEBUG en prod via https://hobo.client.fr/debug/ pour des adresses IPs données.

#15

Updated by Valentin Deniaud about 2 months ago

Version relisable, vous avez l'air d'accord pour se baser uniquement sur DEBUG alors j'ai fait ça.

J'ai complètement revu la logique de l'ensemble, maintenant DebugLoginView c'est une vue simple qui pose un cookie et LoginView affiche les infos de debug en fonction.

#16

Updated by Benjamin Dauvergne about 2 months ago

  • Status changed from Solution proposée to Solution validée
#17

Updated by Valentin Deniaud about 2 months ago

  • Status changed from Solution validée to Résolu (à déployer)
commit dbdd6fd70b26bc32cdfbd4917f5a0bf4fafaa276
Author: Valentin Deniaud <vdeniaud@entrouvert.com>
Date:   Tue Aug 3 11:15:57 2021 +0200

    views: add debug login view (#55557)

commit dde8fa5d026f69a0bd1ae41468741412f901816b
Author: Valentin Deniaud <vdeniaud@entrouvert.com>
Date:   Tue Aug 3 11:12:18 2021 +0200

    views: move login code to separate method (#55557)
#18

Updated by Frédéric Péters about 2 months ago

  • Status changed from Résolu (à déployer) to Solution déployée

Also available in: Atom PDF