Produits Entr'ouvert » Authentic 2

Et aussi il faut retourner 400: The authorization server responds with an HTTP 400 (Bad Request) status code

Serghei Mihai a écrit :

Et aussi il faut retourner 400: The authorization server responds with an HTTP 400 (Bad Request) status code

unless specified otherwise.
(Non sur l'échec d'authn c'est bien 401.)

Paul Marillonnet a écrit :

Voilà, en corrigeant aussi la réponse sur l'endpoint UserInfo.

Cf. https://openid.net/specs/openid-connect-core-1_0.html#UserInfoError

Ah bein oui HttpResponse ne fait pas ça, c'est vrai :)
Dis-moi, je refais mon patch sinon, comme tu veux.

• J'ai vu au moins une interpolation positionnelle parmi les chaînes internationalisées :
  

  _('OpenIDConnect Error "%s": %s') % (self.error_code, self.error_description)

  
  ça va pas marcher avec makemessages.

• pas de possessif sur « client's identifier », « client's secret », je dirais plutôt « client identifier », « client secret », dans les description d'erreur.

• Est-ce qu’on ne pourrait pas plutôt utiliser une property sur le modèle plutôt que de s'encombrer de ce genre de fonctions :
  

  +def access_token_duration(client):
  +    return client.access_token_duration or datetime.timedelta(seconds=app_settings.IDTOKEN_DURATION)
  

  
  (même si oui certes c'était déjà là avant.)

• des litérales unicodes u'' qui traînent encore ici et là dans le code des vues et qu'on pourrait virer, tant qu'à faire un patch de 2000 lignes :)

• pourquoi est-ce qu'on supprime à certains moments des appels aux hooks, mais pas tous (un grep de 'call_hooks' montre qu'il en reste dans views et utils) ?

• « User consent refused » -> « User denied consent » (? — pas sûr)

• pourquoi zéro log dans la nouvelle fonction parse_http_basic ?

• fonction check_ratelimited -> c'est un nom générique mais en fait restreinte au groupe par cession ro-cred.
  • Je pense qu'il fautajuster en changeant le nom de fonction ou en laissant le groupe passable en paramètre de fonction.

• if check_ratelimited(request, key=lambda group, request: client.client_id): -> pourquoi une fonction lambda ici, pas simplement la chaîne d'id de client puisqu'on la connaît au moment de l'appel et que le paramètre key accepte une chaîne, pas nécessairement un callable ?

• params={} au lieu de params=None dans la signature de make_client t'éviterait de faire un params or {} ensuite, non ?

• 'Token is expired or user is disconnected' -> Je pense que ce serait plus clair avec une forme active, genre 'Token expired or user disconnected'

• Pas de 'Bearer' dans l'entête WWW-Authenticate en cas d'erreur sur l'endpoint UserInfo, cf https://openid.net/specs/openid-connect-core-1_0.html#UserInfoError

Mes réponses, j'ai intégré toutes les corrections sur la branche.

Paul Marillonnet a écrit :

Dans 0001 :

• J'ai vu au moins une interpolation positionnelle parmi les chaînes internationalisées :
  [...]
  ça va pas marcher avec makemessages.

Corrigé.

• pas de possessif sur « client's identifier », « client's secret », je dirais plutôt « client identifier », « client secret », dans les description d'erreur.

Corrigé.

• Est-ce qu’on ne pourrait pas plutôt utiliser une property sur le modèle plutôt que de s'encombrer de ce genre de fonctions :
  [...]
  (même si oui certes c'était déjà là avant.)

Ça disparaît dans #48889 donc non je ne m'en occupe pas ici.

• des litérales unicodes u'' qui traînent encore ici et là dans le code des vues et qu'on pourrait virer, tant qu'à faire un patch de 2000 lignes :)

Ok fait dans models.py et views.py

• pourquoi est-ce qu'on supprime à certains moments des appels aux hooks, mais pas tous (un grep de 'call_hooks' montre qu'il en reste dans views et utils) ?

Je ne vois pas, dans le patch j'en retire un et j'en rajoute, je l'ai juste déplacé donc (sso-request en début de authorize_for_client).

• « User consent refused » -> « User denied consent » (? — pas sûr)

J'ai mis "User did not consent".

• pourquoi zéro log dans la nouvelle fonction parse_http_basic ?

Je ne vois pas, il n'y en avait déjà pas avant, c'est authenticate_client qui lève des exceptions qui seront loggées par OIDCException.redirect_response(), j'ai essayé de rassembler la gestion d'erreur dans ces exceptions.

• fonction check_ratelimited -> c'est un nom générique mais en fait restreinte au groupe par cession ro-cred.
  • Je pense qu'il fautajuster en changeant le nom de fonction ou en laissant le groupe passable en paramètre de fonction.

Ok renommé en is_ro_cred_grant_ratelimited.

• if check_ratelimited(request, key=lambda group, request: client.client_id): -> pourquoi une fonction lambda ici, pas simplement la chaîne d'id de client puisqu'on la connaît au moment de l'appel et que le paramètre key accepte une chaîne, pas nécessairement un callable ?

Parce que ça ne marche pas comme ça cf. la doc de django-ratelimit si c'est une chaîne ça doit être un truc connu (user, ip, etc..) sinon ça doit être un callable ou un chemin vers un callable¹.

¹ https://django-ratelimit.readthedocs.io/en/stable/keys.html#string-values

Dans 0003 :

• params={} au lieu de params=None dans la signature de make_client t'éviterait de faire un params or {} ensuite, non ?

C'est un code smell en python: c'est malvenu d'utiliser un objet mutable comme valeur par défaut d'un paramètre de fonction (idem pour la valeur par défaut d'un attribut de classe), s'il est modifié ça modifie la valeur pour tous les appelants.

Dans 0004 :

• 'Token is expired or user is disconnected' -> Je pense que ce serait plus clair avec une forme active, genre 'Token expired or user disconnected'

Ok, j'ai corrigé 'Token is unknown' aussi juste avant.

• Pas de 'Bearer' dans l'entête WWW-Authenticate en cas d'erreur sur l'endpoint UserInfo, cf https://openid.net/specs/openid-connect-core-1_0.html#UserInfoError

C'est la spéc qui se trompe, c'est bien comme ça que c'est décrit dans la RFC sur Bearer authentication (fin de section https://tools.ietf.org/html/rfc6750#section-3). Je pense même qu'il doit y avoir un errata, https://bitbucket.org/openid/connect/issues/990/userinfo-error-response-example-missing