search

etalab / admin_api_entreprise

Site vitrine / backoffice de API Entreprise
https://entreprise.api.gouv.fr
MIT License
9 stars 5 forks source link

Intégration nouveau statut Datapass (revoked - webhook) #382

Closed Haelle closed 1 year ago

Haelle commented 2 years ago

un nouvel état a été ajouté dans Datapass qui n'est actuellement pas encore renvoyé à API Entreprise ; le statut revoked.

Il pourrait être utilisé pour :

Attention ; si on blacklist automatiquement il faut que ça soit reporté en production ou au moins qu'on reçoive dans un premier temps un email etc... Car aujourd'hui c'est à faire dans le backoffice et nécessite une PR et un redéploiement Ansible

PRIORITÉ FAIBLE

skelz0r commented 2 years ago

je pense qu'il faut aussi gérer à minima l'event, même si il fait rien (je pense qu'en l'état ça raise une erreur).

skelz0r commented 1 year ago

@Un3x ici

Un3x commented 1 year ago

Bon du coup, j'ai fais le tour du code et j'ai regardé un peu comment ca marche, ALLES KLAR so far.

J'ai quand même plusieurs question sur le sujet :

Est ce que c'est un event api-entreprise, api-particulier, ou les deux ? Est ce qu'on a qqpart la payload qui va avec ce nouvel event, ou une doc, ou les deux (ce qui répondrait peut être à la 1ère question) ? - Je ne trouve pas l'event dans https://github.com/betagouv/signup-back/blob/master/docs/webhooks.md Plus généralement est ce qu'on a une documentation sur le sujet (les webhook datapass, pas le webhook.md dans le projet) ?

Aujourd'hui ca fonctionne comment ? On recoit l'event mais on le traite pas / on renvoie une erreur. Mais y a quand même une demande derrière ca qui doit être traitée. Pour revoke les webhooks, c'est un mec qui fait une demande et nous on blackliste à la main (j'ai pigé la partie manuelle, c'est le trigger pour faire la manipulation qui m'intéresse)

Quitte à gérer l'event et ne rien faire, on pourrait aussi nous envoyer un email pour nous prévenir d'une demande, au moins on serait pro-actif sur le blacklisting du token. Ca ferait légèrement évoluer le process (on peut le faire dans un 2nd temps aussi).

Un3x commented 1 year ago

Ah, le dépot a été migré et la doc des webhooks est ici : https://github.com/betagouv/datapass/blob/master/backend/docs/webhooks.md

skelz0r commented 1 year ago

Est ce que c'est un event api-entreprise, api-particulier, ou les deux ?

Les 2.

Est ce qu'on a qqpart la payload qui va avec ce nouvel event, ou une doc, ou les deux (ce qui répondrait peut être à la 1ère question) ? - Je ne trouve pas l'event dans https://github.com/betagouv/signup-back/blob/master/docs/webhooks.md Plus généralement est ce qu'on a une documentation sur le sujet (les webhook datapass, pas le webhook.md dans le projet) ?

La doc est ici: https://github.com/betagouv/datapass/blob/master/backend/docs/webhooks.md

Aujourd'hui ca fonctionne comment ?

Techniquement je pense que ça update la demande sans rien faire. Mais à vérifier. D'un point de vue métier c'est un instructeur de la demande qui blacklist la demande, à priori on est au courant.

Quitte à gérer l'event et ne rien faire, on pourrait aussi nous envoyer un email pour nous prévenir d'une demande, au moins on serait pro-actif sur le blacklisting du token. Ca ferait légèrement évoluer le process (on peut le faire dans un 2nd temps aussi).

Je pense que déjà dans un premier temps on peut checker voir ce que ça fait (avec un test). En fonction, on peut décider de voir ce qu'on fait. Je pense que c'est un poil "tricky" ici car notre organizer ne s'occupe que de create/update, ici il peut y avoir des actions "destructives". Je pense que le minimal est de gérer l'affichage déjà.

Au passage, je me rends compte qu'il y a maintenant archive et deleted, faudrait peut-être penser à gérer ces status là aussi (faut ouvrir des issues à part).

skelz0r commented 1 year ago

Pour info, les triggers de webhooks se passent ici: https://github.com/betagouv/datapass/tree/a5bf53d47e6f7d43a7aa91a248891d3683963a2f/backend/app/notifiers

Un3x commented 1 year ago

Tu m'as un peu devancé sur ton comment :D.

Ok donc si je comprends bien l'ensemble.

Nous tous les event ne nous intéressent pas (même si on fait des majs de data), ce qui a vraiment un impact pour nous c'est le validate et potentiellement tous les events qui arrivent post validate pour altérer l'état de l'habilitation (qu'on ne gère pas).

revoke : Bon là, je pense que la doc est fausse coté datapass. C'est le même commentaire que pour le refuse. J'imagine que ca arrive bien après une validation du token et qu'il faudrait le blacklister. archive : Ok, ca correspond à un usecase plus ou moins similaire chez nous. Je pense qu'à minima faudrait gérer cet event de la même manière que le revoke. delete : J'ai pas l'impression que ca nous concerne vraiment. J'imagine qu'on ferait plutot un revoke/archive sur une habilitation existante qu'un delete (à confirmer, parce qu'apparement un admin peut delete post validate).

Je pense que le minimal est de gérer l'affichage déjà.

Là j'avoue que je pige pas ce que tu attends. Je comprends pas l'impact de la gestion de ces webhooks avec l'affichage. Ok on gère pas les webhooks ajd, mais même si on le faisait ce ne serait que l'adaptation d'un process qui est aujourd'hui manuel. Du coup j'imagine que la gestion des infos post traitement est déjà ok.

skelz0r commented 1 year ago

Nous tous les event ne nous intéressent pas (même si on fait des majs de data), ce qui a vraiment un impact pour nous c'est le validate et potentiellement tous les events qui arrivent post validate pour altérer l'état de l'habilitation (qu'on ne gère pas).

revoke : Bon là, je pense que la doc est fausse coté datapass. C'est le même commentaire que pour le refuse. J'imagine que ca arrive bien après une validation du token et qu'il faudrait le blacklister. archive : Ok, ca correspond à un usecase plus ou moins similaire chez nous. Je pense qu'à minima faudrait gérer cet event de la même manière que le revoke. delete : J'ai pas l'impression que ca nous concerne vraiment. J'imagine qu'on ferait plutot un revoke/archive sur une habilitation existante qu'un delete (à confirmer, parce qu'apparement un admin peut delete post validate).

Pour les 2 derniers imo go faire des issues spécifiques sur le traitement potentiellement attendu. J'ai demandé à avoir des précisions sur Mattermost. A chaud je pense que si la demande n'a pas été validée on peut carrément supprimer les infos sur ces 2 events (quasiment sûr pour delete, archive très certainement vu le code de la state machine (référencé ci-dessous)).

Pour revoke c'est que depuis une demande qui a été validée ( https://github.com/betagouv/datapass/blob/8cb0c6372d78e03411359a24e7ab0348bc4b44e1/backend/app/models/enrollment.rb#L58 ). Dans le meilleur des mondes il faudrait le blacklister oui, mais notre process est pain as fuck car c'est fait avec du ansible and stuff...

Je pense que le minimal est de gérer l'affichage déjà.

Là j'avoue que je pige pas ce que tu attends. Je comprends pas l'impact de la gestion de ces webhooks avec l'affichage. Ok on gère pas les webhooks ajd, mais même si on le faisait ce ne serait que l'adaptation d'un process qui est aujourd'hui manuel. Du coup j'imagine que la gestion des infos post traitement est déjà ok.

Gérer l'affichage des demandes, i.e. afficher "Demande révoquée". Je pense que là ça affiche une erreur i18n (car non implémenté ici: https://github.com/etalab/admin_api_entreprise/blob/7bf8435f154afffacba57a176aa34ea0ceec4aae/config/locales/api_entreprise/fr.yml#L168 )

Un3x commented 1 year ago

Pour les 2 derniers imo go faire des issues spécifiques sur le traitement potentiellement attendu. J'ai demandé à avoir des précisions sur Mattermost.

https://github.com/etalab/admin_api_entreprise/issues/947, https://github.com/etalab/admin_api_entreprise/issues/948

Je pense que j'ai de quoi avancer sur ce sujet du coup.

Un3x commented 1 year ago

RAF :

Un3x commented 1 year ago

J'ai réouvert le ticket parce qu'il a été close auto avec la PR mais c'est pas fini !

Cf la discussion sur mattersmost où il y a une discussion sur l'impact que devrait avoir les event revoke et archive

Haelle commented 1 year ago

Conclusion des échanges avec @SchweisguthN :

Il va faire un diagramme de leur machine à état.

Questions

Quand doit-on blacklister tous les jetons d'une demande ?

On blackliste automatiquement tous les jetons d'une demande quand on reçoit le statut revoked et uniquement dans ce cas là.

Que doit-on faire quand on reçoit le statut archive ?

Le statut archive peut arriver de tous les état sauf validated qui est obligé de passer par revoke avant d'être archive.

Donc on ne fait rien d'autre que mettre à jour le modèle authorisation_requests de notre côté.

Que doit-on faire quand on reçoit le statut delete ?

Lever une alerte, ce statut existe encore pour une durée indéterminée, mais il est voué à disparaître.

Quels sont les edges cases ?

Les demandes archive peuvent revenir à leur état précédent et donc revenir à tous les états possible sauf validated. Ça n'a pas de sens métier ça ne peut être que la réparation d'une erreur humaine.

Là je pense que la solution KISS c'est de lever une alerte Sentry et de traiter le cas à la main ; ça ne vaut pas le coup de développer une solution pour des cas qui ne devront jamais arriver.

@skelz0r @Un3x avez-vous tout ce qu'il vous faut pour reprendre le dev ?

skelz0r commented 1 year ago

A la lumière de tes échanges pour moi:

Au passage ça me fait dire qu'on devrait flip les demandes refusées aussi.

Notre système sert principalement à gérer des demandes validées, ou en passe d'être validées, le reste balec

Haelle commented 1 year ago

on ne peut pas supprimer comme ça les données, notamment les jetons il faut qu'on en garde la trace et à qui ils appartenaient.

on avait avant la fonctionnalité de suppression automatique des jetons et on l'a supprimée, on garde tout pour le moment aucun changement de comportement là dessus.

On pourra changer d'avis plus tard, l'inverse n'est pas vrai...

Ne serait-ce que pour le bizdev avoir un historique des anciennes demandes dans notre Metabase, ou même pouvoir voir à qui appartient un jeton expiré/archivé c'est utile. Je vois de nombreux cas où on aimerait pouvoir avoir ces infos (qui ne seront pas souvent ok, mais je ne souhaite pas aller dans ce sens sans qu'on développe le sujet tranquillement et ça peut être fait sur une autre PR/issue pour ne pas bloquer ce sujet)

skelz0r commented 1 year ago

on ne peut pas supprimer comme ça les données, notamment les jetons il faut qu'on en garde la trace et à qui ils appartenaient.

On ne supprime pas de jetons, je propose de ne plus persister dans le temps les demandes qui n'aboutissent pas à un jeton.

Les 2 seules raisons pour laquelle je persistais les demandes avant leurs validations:

  1. Les relances via Mailjet ;
  2. Faire des stats ( funnels de biz dev typiquement )

Pour 1., c'est un effet de bord de 2. Pour 2., c'est clairement le rôle de DataPass de faire ça 🤷

De plus, je suis sûr que ça fausse pas mal de stats chez nous d'avoir ces relicats avec N demandes qui sont des brouillons 🤷

En fait tout tient en une phrase:

Notre système sert principalement à gérer des demandes validées, ou en passe d'être validées, le reste balec.

Si on sort de ce paradigme, on marche sur les platebandes de DataPass, le separation of concern n'est plus respecté, et je pense que c'est une mauvaise approche.

Ne serait-ce que pour le bizdev avoir un historique des anciennes demandes dans notre Metabase, ou même pouvoir voir à qui appartient un jeton expiré/archivé c'est utile.

ça tombe bien, je ne propose pas de supprimer ces données

on avait avant la fonctionnalité de suppression automatique des jetons et on l'a supprimée, on garde tout pour le moment aucun changement de comportement là dessus.

C'était clairement idiot, c'est notre responsabilité de gérer les jetons, pas DataPass.

tldr: ce que je propose n'a aucune incidence sur tes objections

Un3x commented 1 year ago

Je confirme que ce qu'il faut faire avec le status revoked est archi clair pour moi.

Par contre le status archive un peu moins. On ne traite plus cet évènement si je suis la proposition de @skelz0r.

Je vais reprendre la PR pour adapter. Pour le status archive je suis quand même d'avis de catch l'évènement et de ne faire aucun traitement dessus, histoire de s'assurer qu'il n'y ai pas d'erreur renvoyée lors de cet event (ajd les events qu'on ne connait pas ne renvoie pas d'erreur mais bon).

skelz0r commented 1 year ago

Par contre le status archive un peu moins. On ne traite plus cet évènement si je suis la proposition de @skelz0r.

Je propose carrément de supprimer les données associée à une demande archivée non révoquée.

Archivée non révoquée n'arrive que pour des demandes qui n'ont pas été validé ou révoqué, et à priori ces demandes ne seront jamais terminées. (d'ailleurs je proposais de faire la même chose pour les refusés, car aucune chance que ça termine aussi).

Un3x commented 1 year ago

Ok donc faut statuer et valider avec @Haelle sur ce point.

SchweisguthN commented 1 year ago

"a priori ces demandes ne seront jamais terminées" : dans 99% des cas oui, mais il y a une possibilité métier qu'elles le soient. Le remplacement de la suppression par l'archive a notamment été pensée pour "repêcher" des habilitations archivées par erreur, les remettre à leur statut initial et permettre au demandeur de la reprendre. Ca sera un cas très rare bien sûr, mais ça existe.

skelz0r commented 1 year ago

"a priori ces demandes ne seront jamais terminées" : dans 99% des cas oui, mais il y a une possibilité métier qu'elles le soient. Le remplacement de la suppression par l'archive a notamment été pensée pour "repêcher" des habilitations archivées par erreur, les remettre à leur statut initial et permettre au demandeur de la reprendre. Ca sera un cas très rare bien sûr, mais ça existe.

Dans ce cas là, un nouvel événement est envoyé chez nous, et le cycle de création de la demande/jeton/contacts et re-trigger chez nous. Il n'y a donc aucun problème ici. Merci pour la précision en tout cas.

Un3x commented 1 year ago

Ok donc la suppression de l'authorization_request associée à un évènement archive est pertinent. Dans le cas où l'authorization_request est en status blacklisted alors on ne traite pas l'évèment ou alors on le flag en archivé de notre coté aussi ?

skelz0r commented 1 year ago

Dans le cas où l'authorization_request est en status blacklisted alors on ne traite pas l'évèment ou alors on le flag en archivé de notre coté aussi ?

Il n'y a pas d'authorization request blacklisté (ou alors j'ai loupé un truc 🤔).

Un3x commented 1 year ago

Quand doit-on blacklister tous les jetons d'une demande ?

On blackliste automatiquement tous les jetons d'une demande quand on reçoit le statut revoked et uniquement dans ce cas là.

Oui ok j'ai dis authorization_request et c'´était jeton pardon. faut s/authorization_request/jeton/g dans mon message précédent du coup.

skelz0r commented 1 year ago

Ok. Alors, si une demande a des jetons, elle est passé par le status "validé". Si elle est validée, il faut qu'elle soit révoquée avant d'être archivée. On ne la supprime donc pas.