search

etalab / admin_api_entreprise

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

Problème d'ergonomie avec les jetons blaclisté #892

Closed Haelle closed 1 year ago

Haelle commented 1 year ago

Source du problème ce ticket : https://support.etalab.gouv.fr/#ticket/zoom/54019

@Samuelfaure a dû blacklister leur jeton, ça a été fait correctement.

Cependant l'interface n'est pas pratique dans ce cas là (les exemples sont sur staging avec user: user@yopmail.com mdp : user@yopmail.com) :

Je pense qu'il faut simplement revoir la liste des habilitations et renvoyer plutôt la liste des jetons (tout en gardant le même rendu) et les grouper par date d'habilitation et ainsi on verra tous les jetons d'une même habilitation

skelz0r commented 1 year ago

Le ticket parle d'API Particulier btw (et non API Entreprise).

Le bug ici est à mon sens seulement le /compte, le reste est logique. Dtf en vrai avoir N jetons par habilitation c'est juste stupide .. on devrait avoir un seul jeton et juste changer le hash. (ça sera résolu quand on aura une db sur siade)

Samuelfaure commented 1 year ago

Même avis que @skelz0r

skelz0r commented 1 year ago

@Un3x ici

Un3x commented 1 year ago

Du coup j'ai regardé un peu le code,

Dans le code, on a bien qu'un jeton par authorization request

class AuthorizationRequest < ApplicationRecord
   [...]
    has_one :token,
      foreign_key: 'authorization_request_model_id',
      required: false,
      inverse_of: :authorization_request,
      dependent: :nullify

et N authorization request par user 

class User < ApplicationRecord
  [...]
  has_many :authorization_requests, dependent: :destroy
  has_many :tokens, through: :authorization_requests

Par contre, en DB, on a une relation 1-n, donc rien n'empêche d'avoir n token (suffit qu'on ai une action de création de token pour une authorization_request)

Moralité en cas de N jetons, on est dans les choux quand on passe par authorization_request.

Du coup faut impacter dans le code, et gérer les cas => ptet des décisions fonctionnelle / ergo à prendre et aussi les impacts sur user.tokens si on change la relation de authorization_request

Samuelfaure commented 1 year ago

Du coup mon opinion c'est qu'on ferme juste ce sujet jusqu'à ce qu'on ait la DB sur Siade

Un3x commented 1 year ago

Faut voir les impacts.

La solution 'triviale' c'est de changer la relation authorizationRequest et token et les impacts dans le code.

class AuthorizationRequest < ApplicationRecord
   [...]
    has_many :tokens,

mais pas sur que ce soit smart si on se dit que dans le futur on aura qu'un seul token. Ca va complexifier la migration.

Les autres solutions que je vois sont d'écraser l'ancien token (mais on perd l'historique), soit créer une nouvelle authorization request en dupliquant la précédente (je sais pas si c'est pertinent)

skelz0r commented 1 year ago

Je pense qu'on devrait effectivement préciser la relation 1-N sur AuthorizationRequest MAIS qu'on devrait faire un has_one :valid_token + coller une validation soft/hard sur le fait qu'il peut y avoir qu'un seul jeton valide.

Un3x commented 1 year ago

FYI j'ai l'impression que la page /compte/jetons ne liste pas bien tous les jetons actifs.

Je vois pas le bug en staging, mais en local j'ai des jetons actifs qui apparaissent pas sur cette page. Je vais investiguer

Un3x commented 1 year ago

FYI j'ai l'impression que la page /compte/jetons ne liste pas bien tous les jetons actifs. Je vois pas le bug en staging, mais en local j'ai des jetons actifs qui apparaissent pas sur cette page. Je vais investiguer

Bon j'ai un peu digué sur cette partie là.

En fait, j'ai peut-être induit un misnaming dans le bouzin.

En gros on a des jetons :

Si j'ai bien pigé, la page /compte/jetons liste liste les jetons active && !expired?.

Moi j'ai rajouté le terme valid que j'ai calé sur la définition de active, ce qui d'une part :

Ca rajouterai de la consistency avec cet définition dans un token : 

  scope :valid_for, ->(api) { joins(:scopes).active.unexpired.where(scopes: { api: }).uniq }

=> Imo, pour que ce soit plus clair on devrait changer la def de valid_token pour que ce soit des token non expiré + valide, sinon ca n'a pas trop de sens.

@skelz0r, @Samuelfaure - J'veux bien vos inputs rapide

Samuelfaure commented 1 year ago

Je pense qu'il faut creuser le naming un poil plus, la différence entre "actif" et "valid" ne me parait pas assez claire.

En vrai, on a ptêtre même pas besoin de "active = blacklisted: false + archived: false" si ça se trouve ?

Le cas échéant je propose de potentiellement supprimer toute notion de "active" qui est un naming peu clair et de juste utiliser "Valid = Blacklisted: false + archived: false + unexpired"

Un3x commented 1 year ago

Le active est utilisé comme step intermédiaire en fait. Sous entendu, est actif tout token non blacklisté, non archivés, mais il est pas valid pour autant car il peut être expiré.

C'est utilisé a très peu d'endroit dans le code :

C'est pas forcément déconnant de le laisser, mais il faut que je change mon code à moi du coup

skelz0r commented 1 year ago

J'avoue que c'est un poil bordélique sur les scopes..

En réalité "active" devrait exclure les expirés je pense, et du coup on peut merger "active" et "valid"

Un3x commented 1 year ago

Ouais, bon du coup j'ai conservé les notions existantes et j'ai unifié mon nomage avec la définition de valid.

Du coup on a bien un distingo entre valid et active, valid prend en compte la notion d'expiration.

skelz0r commented 1 year ago

Donc pas de merge de active et valid ?

Un3x commented 1 year ago

Je viens de faire le merge.

J'ai choisi de conserver le terme active plutot que valid, vu que valide ca fait plus appel à une notion de validité au sens format (dans le code), et qu'en plus dans le wording du site pour les users on utilise le terme de jetons actifs. Ca me semblait plus cohérent.

J'ai mis ca dans un commit séparé du coup