ZEP-17 : Elaboration de l'API des membres

Ah merde :\ C'est surprenant parce que dans la plupart des APIs que j'ai utilisées ça prend plutôt bien en compte le cache normalement. D'ailleurs c'est bien documenté dans l'API Github par exemple, le client a tout intérêt à envoyer son If-Modified- Since puisque si une 304 lui est renvoyée il ne consomme pas son quota de requête.

En l'occurrence on parle d'un POST, donc d'une requête qui ne doit pas faire l'objet de caching et du coup consommer le quota de l'utilisateur a chaque requête. Dommage.

Honnêtement, si problème technique il y a, à mon sens il va falloir impérativement trouver un moyen de le contourner parce que sans, c'est le genre de problèmes qui peuvent foutre par terre le site et qui me semblent complètement bloquants.

Nous sommes bien d'accord Javier. En fait, il existe plusieurs niveaux de cache. Pour l'API, j'utilise la librairie DRF-extension pour activer l'ETag et le cache (donc uniquement pour l'API). Techniquement, il s'agit d'une annotation que je place sur les méthodes que je veux. Elles sont placées sur les méthodes GET. Jusque là, pas de problème.

Par contre, le site utilise le cache Memcache qui s'applique sur tout le site, l'API comprise … Et ça fout la merde. Alors soit j'ai mal compris la solution et, dans ce cas, ma question sur la mailing list aura une réponse en ce sens. Soit j'ai bien compris le problème et on est un peu dans la merde parce que c'est bloquant.

Juste pour l'explication, la génération de la nuit de samedi à dimanche venait d'un de mes serveurs. Le but était surtout de tester l'API en question dans les conditions réelles d'infrastructure. J'ai décidé de lancer les tests dans la nuit pour ne pas perturber ceux qui sont dessus en journée. Mais je me suis visiblement viandé dans ma commande. Résultat des courses au lieu de lancer check.sh dico.test.txt (d'environ 50 entrées) j'ai lancé check.sh dico.txt. Vous imaginez la souffrance du serveur par la suite.

Quoiqu'il en soit, autant j'avais pris en compte les paramètres suivants :

• on était pas en phase de release officielle, et la preprod est le seul moyen de faire des tests ISO prod
• les données insérées via une chaîne de curl, sont simplement supprimable en BD (je l'ai fait en rentrant dimanche soir sur la preprod)

Autant le gros problème réel ici est que des mails sont visiblement parti vers des adresses potentiellement existantes. Et ça, c'est grave.

Ceci dit, étant donné que j'avais fait la remarque sur cet aspect sur la PR de la ZEP, et que celle-ci à été mergée, je m'attendais à ce que le problème soit corrigé (via code ou infrastructure). Je reste donc persuadé que cette fonctionnalité est beaucoup trop dangereuse pour être exposée via l'API en l'état.

Je ne sais quoi penser firm1. Faire des tests sur la bêta, je peux le comprendre tout à fait mais tu as un accès au serveur de la bêta, tu sais comment il est configuré (les mails ont été envoyés "signé" par le serveur de la prod), tu semblais être conscient de la faille … alors pourquoi ? Sincèrement, cela m'échappe.

Rien ne t'empêchait de contribuer à l'API ou simplement de créer un ticket sur le GitHub. Tiens, la prochaine fois que je vais découvrir une faille importante, je vais l'exploiter de manière abusive puis je vais en informer la communauté. Comme ça, je serais certain que la faille est belle et bien présente …

Bref, je crois que j'ai tué un bisounous dans le tas.

le gros problème réel ici est que des mails sont visiblement parti vers des adresses potentiellement existantes. Et ça, c'est grave.

Non, le gros problème réel c'est que tu aies utilisé des adresses email potentiellement existantes. Et qu'il ne faille donner les accès à la préprod qu'à des gens de confiance.

Dans tous les projets dans lesquels j'ai bossé la préprod est iso-prod, sinon on ne s'en sort pas. Ca veut dire que les emails doivent partir et être envoyés tels qu'ils le sont en prod. Sinon comment la personne lambda qui fait la QA (pas un dev, donc) va vérifier que son email a bien été envoyé, bien reçu, qu'il n'apparaît pas dans ses spams, qu'il est bien lisible avec son client ?

Quand on veut faire un test de charge (ou un test potentiellement dangereux, encadré par des devs) :

• soit on le fait avec des alias mail @trash.monserveur.com

• soit on installe un environnement dédié, avec un serveur mail dédié

Pour moi le problème ne vient pas de la préprod là. Si on vire la fonctionnalité d'envoi d'email c'est cuit pour la QA après.

Par contre ça soulève le problème de qui a accès à la préprod d'une part et de cette faille qui aurait pu être exploitée en prod d'autre part (mais ça on était déjà en train d'en discuter).

Je reste donc persuadé que cette fonctionnalité est beaucoup trop dangereuse pour être exposée via l'API en l'état.

Oui, et on était très exactement en train de chercher des solutions.

Mets toi à ma place. J'ai posé une alerte sur cette aspect, le code a quand même été mergé en l'état, donc je me dis qu'il y a une configuration serveur qui a été mise en place pour parer au problème. Pour préciser la chose, je m'attendais clairement à ce que dans ma pile de 50 créations, au bout de 10, une règle serveur me rejette automatiquement. Et dans le pire des cas, si mes 50 membres passent, une requête en base suffit à nettoyer l'action. Donc en théorie il n'y avait pas de risque, on est pas sur la prod. Le seul point auquel je n'ai pas pensé était le départ des mails, et donc je m'en excuse.

Mais je ne sais pas s'il faut le dramatiser autant, étant donné que le problème n'est pas en production, il reste fixable. Si la faille arrive en production par contre, ça devient trop tard.

C'est bien ce que je dis, j'avais complètement zappé l'étape du mail. Et le problème est bien là.

J'ai bien vu mais je tenais quand même a éclaircir mon point de vue.

L'erreur est humaine. Cela aurait pu arriver à n'importe qui. Fait juste gaffe à l'avenir. Je pense que nous sommes face à une mauvaise manipulation de ta part. Tu peux réintégrer les bisounours et chercher une solution avec nous !

Sinon, je n'ai toujours pas reçu une réponse sur la mailing list de DRF et ça m'inquiète un peu. A court terme, nous pourrons supprimer la route pour la création d'un compte mais à long terme, il faudra vraiment trouver une solution. Et cette solution, je n'en ai aucune idée pour l'instant. Même pas une piste …

+1 Andr0. Je rajouterais que la prochaine fois, évite de laisser traîner tes scripts tous seuls la nuit, sans surveillance et surtout sans prévenir personne. Là par exemple, le fait que les 50 premiers utilisateurs ont été créés en moins de 10 secondes aurait du te mettre la puce à l'oreille.

Pour en revenir au sujet, vu l'état des solutions, je pense que la première version de cette API ne va tout simplement pas permettre de créer des utilisateurs. C'est dommage, mais c'est toujours mieux que rien – et ce serait stupide d'interdire toute l'API parce qu'une seule fonctionnalité déconne.

Néanmoins, peut-être qu'il y a aussi des tests à faire pour le reste des URLs, en particulier celles qui ne sont pas un simple GET.

Y'a une contrainte de temps ? (genre ça bloque une release de la 1.6 attendue pour des corrections de bugs majeurs ? ou un autre truc auquel j'ai pas pensé ?)

Parce qu'au delà du cas de la création de compte, ça me paraît quand même assez chaud de passer l'API en prod sans garde-fou.

J'ai pas fait le test mais si j'envoie un GET /api/membres?perPage=2000 toutes les 100ms, bien entendu sans aucun header de mise en cache (If-Modified-Since etc.) y'a pas un danger que je finisse par écrouler la BDD (ou au moins ralentir pas mal le site) ?

C'est vite fait hein, le cas d'école :

• un client décide de récupérer la liste des membres en polling, pour être sûr de ne pas louper les nouvelles inscriptions

• "pfff j'ai pas envie de m'emmerder avec la pagination, je prends tout ça va y'a que 2000 membres"

• un setTimeout fixé avec une mauvaise valeur ("ah c'est en millisecondes ? merde…")

• "allo ? ah vous m'attendez pour partir en week-end, j'arrive de suite"

Après les clefs API distribuées avec parcimonie ça peut aider mais on n'est pas à l'abris de ce genre de blagues.

Autant s'en prémunir de façon pérenne non ? A moins effectivement que y'ait un gros impératif de release bloquée.

Javier, le cas que tu exposes est géré. Il existe une limite de 100 pour le nombre d'item par page et un cache est mis en place avec la pagination en clé. Tu pourras bourriner l'appel autant que tu veux, dès le deuxième appel tu ne feras plus d'appel à la base de données mais tu taperas dans le cache.

Cela dit, on peut le valider en préprod, elle est là pour ça. Il suffit juste de prévenir et de s'assurer que quelqu'un soit là pour couper le truc en cas de problème.

OK.

Dans ce cas peut-être que la solution de SpaceFox est la bonne.

Mais honnêtement, je suis pas du tout emballé par une release en prod sans limitation, je vais pas passer ma journée à inventer des use cases farfelus mais ça me semble vachement dangereux, et c'est bien pour ça que toutes les APIs en disposent.

Même "à la main" ça me paraît pas très compliqué à mettre en place (faut juste bien prendre en compte qu'on compte sur une fenêtre de temps glissante) donc stocker les timestamps d'appel par IP, c'est super dommage de s'en priver.

Mais soit.

Edit : surtout que c'est une mécanique générique cross-API et que quand on va commencer à toucher à des trucs sensibles comme envoyer un MP, poster dans les forums, … Le besoin va devenir urgentissime.

Le perpage est documenté où ? Sur ce message il n'est pas documenté et sur la doc de l'API en ligne, c'est la requete pour lister les membres qui n'est pas documenté.

Javier : Je crois que je rafraîchis plus de 5 fois toutes les minutes le sujet sur la mailing list de DRF pour voir si je n'ai pas une réponse pour mettre le throttling en place. Je suis bien conscient de sa nécessité mais, techniquement, je n'ai aucune idée de sa mise en place avec le cache de notre projet.

Kje : Tu es au taquet sur la documentation ! En fait, sur la doc de l'API en ligne, la requête pour lister les membres est documentée mais non visible pour une obscure raison. Si tu tires le même tag que la bêta en local, tu verras apparaitre la route. L'origine de ce problème n'a pas encore été trouvé non plus mais il est moins critique que le throttling.

Ceci étant dit, afin de t'épargner de lancer ZdS en local pour voir comment faire, il faut rajouter le paramètre page_size dans ta requête.

Note (pour info) : j'ai tiré le paramètre de la première page de ce topic, je croyais me rappeler que tu l'avais plusieurs fois mis à jour Andr0 mais j'ai dû extrapoler, désolé pour l'erreur

Je m'efforce de le faire pour les deux autres ZEPs API (2 et 23) mais j'avoue ne pas mettre à jour ce sujet. La ZEP est encore en développement et la documentation swagger et du projet regroupent toutes les mises à jour par rapport à la spec écrite sur ce sujet. Après, si vous voulez, je peux m'efforcer à mettre à jour le premier message de ce sujet aussi.

Nan nan t'en fais pas, comme je te disais c'est moi qui ait extrapolé (parce que justement tu mets les autres ZEP à jour).

J'aurais du regarder la date d'édition, nevermind.

Oui je suis au taquet car j'ai cru comprendre qu'il y avait peu de testeurs donc pour apporter ma pierre a l'édifice, et aider les membres à exploiter cette killer-feature, je suis en train de finir un petit package python pour exploiter l'API. Histoire de pouvoir facilement lancer des requettes et fournir un client testable.

Bon je regarderais chez moi du coup

Enfin des réponses sur la mailing list, dont une par l'auteur de la bibliothèque. Notre problème semble connu et corrigé dans une nouvelle version. Je test ça ce soir et je vous tiens au jus. Croisons les doigts mais d'après leurs réponses, cela devrait être possible de toute façon.

Je propose donc que nous commençons à discuter du rate limit que nous appliquons sur l'API pour les utilisateurs déconnectés et les utilisateurs connectés. Personnellement, j'avais pensé à 60/heure pour les non connectés et 2000/heure pour les connectés.

Ces chiffres ne sont pas totalement pris par hasard. Ils sont fortement inspiré du rate limit de GitHub avec une légère adaptation à notre plateforme puisque la notre est bien plus modeste que celle de GitHub.

Ces chiffres ne sont pas totalement pris par hasard. Ils sont fortement inspiré du rate limit de GitHub avec une légère adaptation à notre plateforme puisque la notre est bien plus modeste que celle de GitHub.

Et ils seront bien sur changeable en deux temps trois mouvements dans le settings.py ?

Exactement, ce sont des valeurs spécifiées dans le fichier settings.py.