Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remplace la vue de login par la LoginView de Django #6273

Merged
merged 9 commits into from
Jul 19, 2022

Conversation

Arnaud-D
Copy link
Contributor

@Arnaud-D Arnaud-D commented Mar 28, 2022

Un des items de #6246.
Fix #4861 .

Refactorisation des tests du login :

Refactorisation du login lui-même :

  • utilise la LoginView de Django
  • change le backend standard pour afficher une erreur spécifique pour les comptes inactifs (nécessaire en utilisant la LoginView de Django, par défaut les comptes inactifs ratent le test d'authentification, ce qui fait directement une erreur login/password et pas celle de compte inactif) ;
  • simplifie en fusionnant les erreurs pour mauvais nom d'utilisateur ou mot de passe (standard partout pour éviter d'informer un attaquant éventuel ; l'avantage est limité chez nous puisque qu'on se logue par pseudo, rapidement public et pas par email) ;
  • conserve le comportement de redirection originel

Quelques tests supplémentaires :

  • rajouter un test pour le "remember me"
  • rajouter un test pour le bon enregistrement de l'IP

Contrôle qualité

Ces choses-là sont faites aussi dans les tests unitaires, mais on peut tester à la main aussi :

  • Se loguer normalement avec un compte actif (n'importe lequel des fixtures fait l'affaire).
  • Tenter de se loguer avec un mauvais utilisateur, puis mauvais mot de passe et constater la même erreur dans les deux cas.
  • Bannir compte et constater une erreur spécifique quand on tente de se loguer avec
  • créer un compte inactif (admin, puis créer un user, puis créer un profile associé, bien mettre le user inactif), tenter de se loguer et constater l'erreur adéquate
  • jouer avec l'option "se souvenir de moi" et constater que le cookie de session est bien réglé avec les outils de dév
  • constater les bonnes redirections avec le paramètres 'next' (notamment : la page indiquée si elle existe, la homepage au lieu de retourner sur le login/inscription/déconnexion, ou la homepage de manière générale en cas de soucis)

En plus, il serait sûrement souhaitable de vérifier que l'authentification par Google et Facebook marche, mais ça ne fonctionne pas du tout en local chez moi, que ce soit cette PR ou même avant ça.

@Arnaud-D Arnaud-D added the C-Back Concerne le back-end Django label Mar 28, 2022
@coveralls
Copy link

coveralls commented Mar 28, 2022

Coverage Status

Coverage increased (+0.008%) to 88.087% when pulling 4f2a440 on Arnaud-D:django_loginview into d585f02 on zestedesavoir:dev.

@Arnaud-D Arnaud-D force-pushed the django_loginview branch 2 times, most recently from ba909b3 to 2f38ef4 Compare March 28, 2022 14:01
@Arnaud-D Arnaud-D changed the title Remplace notre vue de login par le LoginView de Django Remplace la vue de login par la LoginView de Django Mar 28, 2022
zds/member/views/login.py Outdated Show resolved Hide resolved
zds/member/views/login.py Outdated Show resolved Hide resolved
zds/member/views/login.py Outdated Show resolved Hide resolved
@Arnaud-D Arnaud-D force-pushed the django_loginview branch 2 times, most recently from 054137f to 9e89f58 Compare April 3, 2022 20:22
@Situphen Situphen self-assigned this Apr 3, 2022
@Situphen Situphen self-requested a review April 3, 2022 20:59
@Arnaud-D Arnaud-D marked this pull request as ready for review April 4, 2022 08:02
@Arnaud-D
Copy link
Contributor Author

Arnaud-D commented Apr 4, 2022

C'est prêt à être QA !

@Arnaud-D Arnaud-D added the S-Refactorisation Améliore le code existant sans forcément ajouter de nouvelle fonctionnalité label Apr 8, 2022
@Arnaud-D
Copy link
Contributor Author

Arnaud-D commented Jun 2, 2022

D'après le commentaire de Migwel dans l'issue ci-dessus : fix #4861.

@philippemilink
Copy link
Member

Tu peux mettre à jour la PR et corriger le conflit, stp ?

Copy link
Member

@philippemilink philippemilink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quelques petits détails à corriger et ça devrait être bon.

zds/member/forms.py Outdated Show resolved Hide resolved
zds/member/forms.py Outdated Show resolved Hide resolved
zds/member/tests/views/tests_login.py Show resolved Hide resolved
zds/member/forms.py Show resolved Hide resolved
* réorganise les tests existants
* ajoute un test pour vérifier l'erreur obtenue pour un utilisateur inactif
* corrige un bug dans la vue pour passer le test
* utilise la LoginView de Django
* change le backend standard pour afficher une erreur spécifique pour les comptes inactifs (nécessaire en utilisant la LoginView de Django)
* simplifie en fusionnant les erreurs pour mauvais nom d'utilisateur ou mot de passe
* conserve le comportement de redirection originel
* ne teste plus le formulaire seul (tout est couvert par les tests de la vue)
* corrige des tests annexes qui ne testaient pas la bonne chose
Copy link
Member

@philippemilink philippemilink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tout fonctionne bien, sauf la redirection vers l'URL quand on se connecte.

Pour reproduire :

  1. Se connecter
  2. Aller sur une page à laquelle on ne peut accéder qu'en étant connecté (éditer un message sur le forum, les paramètres de son compte, etc)
  3. Copier l'URL
  4. Se déconnecter
  5. Coller l'URL. On est redirigé vers la page de connexion, avec le paramètre next bien positionné.
  6. Se connecter
  7. On est redirigé vers l'accueil au lieu de l'URL collée initialement.

J'arrive à causer le bug avec l'URL http://127.0.0.1:8000/membres/parametres/profil/.

Il faudrait aussi trouver pourquoi les tests ne montrent pas le problème...

@Arnaud-D
Copy link
Contributor Author

Arnaud-D commented Jul 19, 2022

C'est un bug intéressant. Les tests que j'avais écrits interrogeaient directement la vue en POST, en envoyant correctement le paramètre next dans l'URL. Sauf que quand on GET la page, l'attribut action du formulaire n'était pas correct : le next n'y était pas. Quand on soumet le formulaire comme un humain, l'info n'était alors pas envoyée et on retombait sur la redirection par défaut, la page d'accueil. J'ai ajouté un test qui regarde si on a bien un action={next} dans la page.

C'est le genre de choses qui bénéficieraient de tests plus englobants, par exemple avec selenium.

Copy link
Member

@philippemilink philippemilink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA OK ✔️

@philippemilink philippemilink enabled auto-merge (squash) July 19, 2022 17:27
@philippemilink philippemilink merged commit 029e2e9 into zestedesavoir:dev Jul 19, 2022
@Arnaud-D Arnaud-D deleted the django_loginview branch July 19, 2022 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Back Concerne le back-end Django S-Refactorisation Améliore le code existant sans forcément ajouter de nouvelle fonctionnalité
Projects
Archived in project
4 participants