Produits Entr'ouvert » Authentic 2

La mécanique pour bloquer la navigation sur un critère et rediriger sur une autre page est déjà là (authentic2.middleware.ViewRestrictionMiddleware).

Actuellement chaque plugin a2 peut proposer via une méthode check_view_restriction() qui doit retourner le nom d'une vue ou une URL. Vu que le concept de plugin est en cours de déprécation pour utiliser simplement des applications Django et leur objet AppConfig à la place il faudrait ici plutôt passer par un hook (donc modifier ViewRestrictionMiddleware pour appeler call_hooks('check_view_restriction')).

Ensuite il suffit de renvoyer l'URL de la vue de profil et de poser un message sur la requête du genre "Vous devez valider les CGUs"[1].

Ce qui m'embête c'est comment générer ce message de manière automatique et intelligible, on génèrera facilement un message du genre "Veuillez compléter les champs requis suivants: date de naissance, validation des CGUs" mais est-ce que c'est suffisant ?

¹

def a2_hook_check_view_restriction(self, request):
     if 'required_fields' in request.session:
         return
     # on vérifie si tous les champs requis sont là
     if unfilled_required_fields:
        message = _('You must complete required fields: %s') % ', '.join([field.label for field in unfilled_required_fields])
        messages.info(request, message)
        return 'profile_edit'
     request.session['required_fields'] = True
     return None

C'est bien si on ne valide une restriction qu'une fois par session (sinon ça fait des requêtes intempestives en base pour rien), i.e. si une restriction de vue passe, il faut le noter dans request.session pour ne pas le refaire.

Ce qui m'embête c'est comment générer ce message de manière automatique et intelligible, on génèrera facilement un message du genre "Veuillez compléter les champs requis suivants: date de naissance, validation des CGUs" mais est-ce que c'est suffisant ?

Dans le contexte du cas d'usage "acceptation des CGU" pour un compte créé avant obligation de celui-ci, cela me semble OK

• j'ai fait un tour (à coup de find -exec grep) des vues concernées par

  if 'logout' in url_name or '-slo' in url_name:

  Ok pour le logout.
  Le SLO c’est les vues de authentic2.idp.saml qui vont être concernées. En renommant a2-idp-saml-finish-slo en a2-idp-saml-slo-finish on peut avoir un test un peu plus carré à base de .startswith('a2-idp-saml-slo').
  Autre chose dans authentic2.idp.saml.saml2_enpdoints il me semble qu’il y a plein d’autres vues frontchannel qui ne contiennent pas logout ni -slo et qui pourtant doivent être exemptées de cette redirection vers la page de complétion des attributs manquants par l’usager.

• avec la nouvelle méthode de classe EditRequired.get_fields on perd la logique de vérification des scopes des attributs. L’usager peut donc se voir présenter des attributs auxquels il n’a pas accès dans sa page standard d’édition de profil (EditProfile). Étrange.

De façon plus générale, à la généralisation de ces attributs requis au login, je me pose la question des perfs lorsqu’on tape cette vérification dans le middleware plutôt que d’isoler les vues candidates une à une. Mais bon la valeur de 60 secondes minimum entre deux vérifications en succès deux paraît raisonnable.

(Rien à voir, mais un petit s/succesfull/successful/ dans une ligne de commentaire de 0002.)

Paul Marillonnet a écrit :

Dans 0002 :

• j'ai fait un tour (à coup de find -exec grep) des vues concernées par [...] Ok pour le logout.
  Le SLO c’est les vues de authentic2.idp.saml qui vont être concernées. En renommant a2-idp-saml-finish-slo en a2-idp-saml-slo-finish on peut avoir un test un peu plus carré à base de .startswith('a2-idp-saml-slo').

La vue finish n'est pas concernée, c'est uniquement les vues de SLO initiée par le SP, pas toutes les vues frontchannel, disons que que ça pourrait arriver entre le moment d'émettre un SLO et de recevoir sa réponse, mais je préfère ignorer le problème.

Dans 0003 :

• avec la nouvelle méthode de classe EditRequired.get_fields on perd la logique de vérification des scopes des attributs. L’usager peut donc se voir présenter des attributs auxquels il n’a pas accès dans sa page standard d’édition de profil (EditProfile). Étrange.

Les scopes ne servent pas à cacher des attributs, donc non ça ne sert à rien ici, on peut l'ignorer sans souci.

De façon plus générale, à la généralisation de ces attributs requis au login, je me pose la question des perfs lorsqu’on tape cette vérification dans le middleware plutôt que d’isoler les vues candidates une à une. Mais bon la valeur de 60 secondes minimum entre deux vérifications en succès deux paraît raisonnable.

Je ne pense pas que ce soit un problème.

Benjamin Dauvergne a écrit :

Paul Marillonnet a écrit :

Dans 0002 :

• j'ai fait un tour (à coup de find -exec grep) des vues concernées par [...] Ok pour le logout.
  Le SLO c’est les vues de authentic2.idp.saml qui vont être concernées. En renommant a2-idp-saml-finish-slo en a2-idp-saml-slo-finish on peut avoir un test un peu plus carré à base de .startswith('a2-idp-saml-slo').

La vue finish n'est pas concernée, c'est uniquement les vues de SLO initiée par le SP, pas toutes les vues frontchannel, disons que que ça pourrait arriver entre le moment d'émettre un SLO et de recevoir sa réponse, mais je préfère ignorer le problème.

Où est-ce que tu vois que toutes les vues frontchannel ne vont pas tomber dans cette logique de vérification/redirection ? J’ai loupé ce passage dans le patch.

Dans 0003 :

• avec la nouvelle méthode de classe EditRequired.get_fields on perd la logique de vérification des scopes des attributs. L’usager peut donc se voir présenter des attributs auxquels il n’a pas accès dans sa page standard d’édition de profil (EditProfile). Étrange.

Les scopes ne servent pas à cacher des attributs, donc non ça ne sert à rien ici, on peut l'ignorer sans souci.

Et pourtant c’est ce que je comprends à la lecture de EditProfile.get_fields :

        # […]
        if scopes:
            scopes = set(scopes)
            default_fields = [
                attribute.name for attribute in attributes if scopes & set(attribute.scopes.split())
            ]
        # […]
        if scopes:
            # restrict fields to those in the scopes
            fields = [field for field in fields if field in default_fields]
        # […]

De façon plus générale, à la généralisation de ces attributs requis au login, je me pose la question des perfs lorsqu’on tape cette vérification dans le middleware plutôt que d’isoler les vues candidates une à une. Mais bon la valeur de 60 secondes minimum entre deux vérifications en succès deux paraît raisonnable.

Je ne pense pas que ce soit un problème.

Ok, fair enough.

Paul Marillonnet a écrit :

Et pourtant c’est ce que je comprends à la lecture de EditProfile.get_fields :
[...] lesquels fields sont ensuite servis au modelform_factory. Je loupe un truc ?

Le scope est un truc passé en paramètre de la vue, c'est pour limiter le formulaire à un certain ensemble de champs pour des raisons d'organisation, à Strasbourg je crois (genre séparer les champs d'adresse des champs d'état civil). Je ne vois pas de lien avec la fonctionnalité ici implémentée.

Benjamin Dauvergne a écrit :

Le scope est un truc passé en paramètre de la vue, c'est pour limiter le formulaire à un certain ensemble de champs pour des raisons d'organisation, à Strasbourg je crois (genre séparer les champs d'adresse des champs d'état civil). Je ne vois pas de lien avec la fonctionnalité ici implémentée.

D’ac je n’avais pas connaissance de cet usage.

Benjamin Dauvergne a écrit :

Après réflexion j'ai changé mon fusil d'épaule, je passe plutôt par un flag directement sur les vues liées au logout. Le logout SOAP est hors session donc n'est pas concerné.

Oui sur cette épaule ça vise plus juste :)
Juste une remarque de détail avant le ack : du code churn entre 0002 et 0003 qu’on pourrait s'éviter, genre dans 0002 on introduit

+        # prevent blocking people when they logout
+        if 'logout' in url_name or '-slo' in url_name:

J'ai corrigé le bug dans l'IdP SAML sur make_url(continue_sso, ...).

Benjamin Dauvergne a écrit :

(J'ai pris en compte ta remarque sur le code churn, dans la refactorisation je ne garde que le comportement existant, i.e. view == 'auth_logout', donc juste de la refactorisation sans changement de comportement).

Encore un changement, j'ai préféré une approche opt-in, désormais les vues qui sont des points sûrs pour arrêter le parcours de l'utilisateur sont explicitement indiquées, ça concerne :
[…]

Oui en mode opt-in c’est bien aussi.
Je ne comprends pas l’idée du décorateur cependant. Pour moi ce flag view_restriction c’est inhérent à la vue, et non quelque chose qui s’applique à une ou plusieurs de ses occurrences dans les urls.py.
Du coup avec le décorateur ça donne du code de copie qui il me semble n’a pas lieu d'être : on ne se retrouve pas dans le cas où deux versions (l’une originale, l’autre copiée-décorée) de la vue coexistent.