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

Amélioration de la connexion via les réseaux sociaux #6429

Merged
merged 4 commits into from
Mar 11, 2024

Conversation

Situphen
Copy link
Member

@Situphen Situphen commented Dec 1, 2022

Amélioration de la connexion via les réseaux sociaux en deux points.

Fixes #6274

  • (On n'enlève plus les lettres accentuées des pseudos)
    Lors de la première connexion depuis un réseau social et donc de la création du membre en base de données, une fonction de nettoyage des pseudos est appliquée par notre dépendance social-auth-app-django. Celle-ci consiste en l'application de deux expressions régulières ci-dessous et a notamment pour conséquence la suppression des lettres accentuées et des espaces.

    NO_ASCII_REGEX = re.compile(r"[^\x00-\x7F]+")
    NO_SPECIAL_REGEX = re.compile(r"[^\w.@+_-]+", re.UNICODE)
    
    def clean_username(cls, value):
        """Clean username removing any unsupported character"""
        value = NO_ASCII_REGEX.sub("", value)
        value = NO_SPECIAL_REGEX.sub("", value)
    return value

    Heureusement, j'ai remarqué qu'il est possible de remplacer cette fonction par une autre fonction. Il faut alors définir la variable SOCIAL_AUTH_CLEAN_USERNAME_FUNCTION avec le chemin vers cette nouvelle fonction. La fonction de nettoyage des pseudos que je propose supprime les virgules et les barres obliques comme c'est le cas dans le module member (cf. ce bout de code), mais ne remplace pas les caractères utf8mb4.

    J'ai besoin d'un avis sur ce changement. Sur la fonctionnalité en elle-même, je pense que garder les lettres accentuées est une bonne chose mais je ne sais pas si garder les espaces est souhaitable, notamment car ça complique le ping dans les messages. Sur l'implémentation, est-ce que j'oublie quelque chose ? Normalement, on ne devrait pas avoir de soucis avec les comptes Google car les pseudos sont créés à partir de l'adresse de courriel, mais avec les comptes Facebook il doit être possible d'avoir des caractères assez exotiques.

  • (On récupère l'adresse de courriel des comptes Facebook)
    Figurez-vous que demander le scope de suffit pas et qu'il faut explicitement demander à récupérer ce champ à l'aide de la variable SOCIAL_AUTH_FACEBOOK_PROFILE_EXTRA_PARAMS. Malheureusement, il me semble que ce n'est pas rétroactif donc seuls les nouveaux comptes en bénéficieront.

QA : Vérifier le bon fonctionnement de la connexion via les réseaux sociaux, notamment Facebook. (Cela se fera directement sur la bêta.)

@Situphen Situphen added Feedback Ticket ou PR en attente de retours S-Évolution Ajoute de nouvelles fonctionnalités labels Dec 1, 2022
@coveralls
Copy link

coveralls commented Dec 1, 2022

Coverage Status

coverage: 88.671% (-0.003%) from 88.674%
when pulling 028602c on Situphen:facebook
into 2910ebf on zestedesavoir:dev.

@Situphen Situphen force-pushed the facebook branch 3 times, most recently from 9647946 to 887def3 Compare December 6, 2022 11:29
@philippemilink
Copy link
Member

Hmmm, je ne comprends pas tout au premier point...

  • ce que tu proposes pour le premier point n'est pas committé. Tu attends qu'on se mette d'accord ici sur ce qu'il faut précisément implémenter pour le commiter ?
  • tu parles de nettoyage de mot de passe, je suppose que tu veux dire nettoyage de login ?
  • je suis d'accord pour empêcher d'avoir des espaces dans le login, mais en fait ce n'est pas une restriction qui est déjà en place...
  • pourquoi ne pas tout simplement reprendre ce qui est fait dans le morceau de code que cites (+ quelques lignes suivantes) ? Ça interdit les caractères qu'on ne veut pas (notamment le utf8mb4 avec ses caractères fourbes) et devrait autoriser les caractères accentués...

@Situphen
Copy link
Member Author

Situphen commented Dec 6, 2022

Je viens de relire et effectivement ce n'était pas très clair, j'ai probablement écrit ces paragraphes dans un état de fatigue avancé ! J'ai réécrit quelques morceaux pour que cela soit mieux compréhensible (enfin j'espère).

ce que tu proposes pour le premier point n'est pas committé. Tu attends qu'on se mette d'accord ici sur ce qu'il faut précisément implémenter pour le commiter ?

Si si, le premier point est commité. Il y a un commit par point ! J'attends surtout un retour sur le premier point car je ne suis pas sûr que ce que je propose soit souhaitable en fait.

tu parles de nettoyage de mot de passe, je suppose que tu veux dire nettoyage de login ?

Oups, il s'agit en effet du nettoyage du pseudo et non pas du mot de passe.

je suis d'accord pour empêcher d'avoir des espaces dans le login, mais en fait ce n'est pas une restriction qui est déjà en place...

Il n'y a pas de restriction sur les espaces dans les pseudos lors d'une modification de pseudo ou d'une inscription classique, mais il y a en a une de facto pour l'inscription via les réseaux sociaux. Dans son état actuel, le premier commit enlèverai cette restriction pour l'inscription via les réseaux sociaux. Au début je pensais que c'était une bonne idée mais je ne suis finalement pas sûr, car pour les comptes Facebook les pseudos sont créés à partir du nom du compte. On peut donc penser qu'une grande proportion de ces pseudos contiendront des espaces. Et la conséquence que j'imagine c'est que ça augmenterait le nombre de fois où on doit utiliser @**pseudo avec espace** au lieu de @pseudo, ce qui n'est pas très souhaitable je trouve. Il est possible que ce soit une tempête dans un verre d'eau, à vous de me le dire !

pourquoi ne pas tout simplement reprendre ce qui est fait dans le morceau de code que cites (+ quelques lignes suivantes) ? Ça interdit les caractères qu'on ne veut pas (notamment le utf8mb4 avec ses caractères fourbes) et devrait autoriser les caractères accentués...

Même réponse qu'au-dessus.

@Arnaud-D
Copy link
Contributor

Arnaud-D commented Aug 27, 2023

À mon avis, on devrait partir sur les mêmes restrictions que quand on s'inscrit sans passer par Facebook. Autrement dit, la suggestion de Philippe de reprendre les validations en place actuellement, avec utf8mb4.

Pour les nombreux pseudos avec espaces, je n'ai pas de craintes particulières. Le ping est une fonctionnalité non-centrale pour l'usage du site. Ensuite, on a fait le choix d'avoir un raccourci pour le ping qui ne fonctionne pas dans 100% des cas et la syntaxe qui marche à tous les coups se fait oublier. Faut vivre avec ! :D

@Situphen Situphen added the S-Zombie Ticket ou PR oubliée label Mar 9, 2024
@Situphen Situphen force-pushed the facebook branch 2 times, most recently from 58510de to ea0fdfe Compare March 10, 2024 13:29
@Situphen Situphen removed the S-Zombie Ticket ou PR oubliée label Mar 10, 2024
@Situphen
Copy link
Member Author

Nouvel élan de motivation donc j'ai mis à jour cette PR !

J'ai suivi vos retours et j'ai implémenté les même filtres pour les comptes créés à partir des réseaux sociaux que pour ceux créés de façon classique : pas de virgule, pas de barre oblique et pas de caractères utf8mb4. Pour ce dernier point, pas sûr que ce soit nécessaire car les réseaux sociaux ne doivent pas l'autoriser je suppose, mais ça ne mange pas de pain. J'ai du créer une fonction à part de la fonction existante car la bibliothèque attend une fonction qui nettoie le pseudo et non pas une fonction qui lève une exception. Si cela vous semble bon, alors on peut tester la PR sur la bêta.

zds/member/validators.py Outdated Show resolved Hide resolved
zds/utils/misc.py Outdated Show resolved Hide resolved
@philippemilink
Copy link
Member

Je viens de déployer sur la bêta, je te laisse tester @Situphen avec le compte Facebook de test (je pense que ça va aussi être l'occasion de faire un tour dans les réglages de Facebook pour utiliser leur API).

@Situphen
Copy link
Member Author

Ça fonctionne ! J'ai créé un compte ZDS avec un utilisateur test de Facebook et j'ai bien un pseudo avec un espace entre le nom et prénom, ainsi qu'une une adresse de courriel.

image

@philippemilink philippemilink merged commit 2da324a into zestedesavoir:dev Mar 11, 2024
12 checks passed
@Situphen Situphen deleted the facebook branch March 11, 2024 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feedback Ticket ou PR en attente de retours S-Évolution Ajoute de nouvelles fonctionnalités
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

S'inscrire avec Facebook crée un pseudo sans les lettres accentuées
4 participants