Project

General

Profile

Development #61199

Maintenir une référence au dernier service et à la dernière URL de redirection

Added by Benjamin Dauvergne 4 months ago. Updated 4 months ago.

Status:
Fermé
Priority:
Normal
Category:
-
Target version:
-
Start date:
28 Jan 2022
Due date:
% Done:

0%

Estimated time:
Patch proposed:
Yes
Planning:
No

Description

Le système actuel est compliqué et ne donne pas le service demandé qui est sur les pages "Mon compte" de disposer en permanence d'une URL de retour et d'une référence au service et à l'OU d'origine. Le but de ce ticket est de simplifier tout ça:
  • maintenir en session une référence au dernier service et à la dernière homepage référencée,
  • sur tous les endpoints de SSO, poser la référence au service concerné en session,
  • sur toutes les pages mon compte, vérifier le paramètre ?next=, trouver le service correspondant, et conserver ce service et l'URL next en session
  • servir dans tous les templates des variables référençant ce service, son OU et la dernière URL next correspondante ou bien la "home_url" de l'OU correspondante

C'est basé sur #60349 (champ home_url dans OrganizationalUnit).


Files


Related issues

Related to Hobo - Development #61192: Introduire des variables pour URLs usuelles login_url, logout_url et registration_urlFermé27 Jan 2022

Actions

Associated revisions

Revision 051d27b0 (diff)
Added by Benjamin Dauvergne 4 months ago

misc: maintain home url, service and ou (#61199)

Home service is defined on SSO requests.

Home URL is the OU home URL or the default homepage url, but on
account's pages if you pass a ?next= to an /accounts/ or /register/ view
and this URL can be linked ton an existing service, the home service is
set and the URL is take as the home URL.

History

#1

Updated by Benjamin Dauvergne 4 months ago

Pour l'utiliser il suffit d'ajouter dans n'importe quel entête de page :

   <a href="{{ home_url }}">Retour au portail</a>

Par ailleurs pour les liens "Mon compte", il suffit d'ajouter ?next=https://mon-service/ma-page/ à https://idp/accounts/ pour que ce soit maintenu sur toutes les pages (uniquement pour un service SAML, OIDC ou CAS).

#2

Updated by Benjamin Dauvergne 4 months ago

  • Related to Development #61192: Introduire des variables pour URLs usuelles login_url, logout_url et registration_url added
#3

Updated by Benjamin Dauvergne 4 months ago

  • Status changed from Solution proposée to En cours
#5

Updated by Benjamin Dauvergne 4 months ago

Ajout d'un test sur le header 'Sec-Fetch-Dest' pour éviter de définir le service sur des chargements JSONP ou autre, seul la navigation nous intéresse.

#6

Updated by Emmanuel Cazenave 4 months ago

C'est cool ça va dans le sens d'une simplification. Par contre c'est quand même bien massif, la relecture est difficile, je serai pour attendre le prochain cycle et pousser ça un vendredi.

Ci dessous deux remarques formulées avant que mon cerveau ne s'embourbe.

Il y a une nécessité à ce que set_service puisse nettoyer la session (session.pop('sevice_type', None) ...) ?
En tous cas à la lecture je ne vois pas les moments où on passerait dans ce chemin de code, et si je les loupe, ça me parait être une bonne raison de passer par une autre méthode genre clean_service qui faciliterait la compréhension.

Tout à fait mineur, return getattr(request, '_service', None) overkill, tu es sûr d'avoir une attribut _service.

#7

Updated by Benjamin Dauvergne 4 months ago

Voilà remarques intégrés concernant le nettoyage de service_*, avoir une méthode clean_service et le getattr() inutile.

#8

Updated by Benjamin Dauvergne 4 months ago

Emmanuel Cazenave a écrit :

Il y a une nécessité à ce que set_service puisse nettoyer la session (session.pop('sevice_type', None) ...) ?

Non c'est vrai, mais je pense que je devrait le faire dans set_home_url() si on ne trouve pas de service associé à l'URL, revenir dans un état neutre me paraît la bonne pratique. Je n'ai pas écrit le code dans l'ordre le plus logique pendant que je le faisais et donc j'ai du oublier de le faire.

En tous cas à la lecture je ne vois pas les moments où on passerait dans ce chemin de code, et si je les loupe, ça me parait être une bonne raison de passer par une autre méthode genre clean_service qui faciliterait la compréhension.

Éventuellement, mais toujours dans set_home_url() je pense.

Tout à fait mineur, return getattr(request, '_service', None) overkill, tu es sûr d'avoir une attribut _service.

Yep.

#10

Updated by Paul Marillonnet 4 months ago

  • Status changed from Solution proposée to Solution validée

Dans l’esprit ok, rien à redire, juste quelques petites remarques de détail ici et là.

––––

  • Dans auth_fc :
    Un espace en trop dans encode_state entre la next_url et la signature, on va se retrouver avec une chaîne vide dans le split ensuite.
             encoded_state = state + ' ' + self.next_url + ' '                                   
             encoded_state += ' ' + hmac_url(settings.SECRET_KEY, encoded_state)

    D’ailleurs dans decode_state j’ai l’impression que la vérification du nombre d’éléments dans le payload n’est plus cruciale, je pense qu’on peut virer le try … except  par simplement :
    state, next_url, *dummy = payload.split(' ')

    (ou du moins si on veut s’assurer qu’il y a au moins trois éléments
    state, next_url, dummy = payload.split(' ', maxsplit=2)
    )
  • Du détail, et par simple curiosité, dans authentic2.utils.misc.select_next_url, je comprends pas pourquoi on instancie un EMPTY = object() et faire la distinction entre None et cet objet ?
    Est-ce qu’on ne peut pas simplement faire la même distinction entre None et '', genre
    def select_next_url(request, default=None, ):
        if default is None:
            if request.user.is_authenticated and request.user.ou and request.user.ou.home_url:
                default = request.user.ou.home_url
            else:
                default = settings.LOGIN_REDIRECT_URL
    

    et auquel cas dans set_home_url avec on appellerait select_next_url avec default='' ?
  • Pour l’usage de set_service j’ai fait quelques git-grep ici et là, ça m’a l’air bon.
    Il y aurait aussi des modifs pertinentes dans les modules tiers, là à vue de nez je vois par exemple authentic2-cut où on place le slug de service dans la session (dans CUTMiddleware), pour l’utiliser notamment pour les entrées de journal. Je pense qu’on peut faire un ticket pour virer ça, on n’a plus besoin de ce slug dans la session dorénavant.
#11

Updated by Benjamin Dauvergne 4 months ago

  • Status changed from Solution validée to Résolu (à déployer)
commit 051d27b06805898a115f857a26e50946009414c0
Author: Benjamin Dauvergne <bdauvergne@entrouvert.com>
Date:   Fri Jan 28 00:31:04 2022 +0100

    misc: maintain home url, service and ou (#61199)

    Home service is defined on SSO requests.

    Home URL is the OU home URL or the default homepage url, but on
    account's pages if you pass a ?next= to an /accounts/ or /register/ view
    and this URL can be linked ton an existing service, the home service is
    set and the URL is take as the home URL.
#12

Updated by Benjamin Dauvergne 4 months ago

Paul Marillonnet a écrit :

  • Dans auth_fc :
    Un espace en trop dans encode_state entre la next_url et la signature, on va se retrouver avec une chaîne vide dans le split ensuite.
    [...]
    D’ailleurs dans decode_state j’ai l’impression que la vérification du nombre d’éléments dans le payload n’est plus cruciale, je pense qu’on peut virer le try … except  par simplement :
    [...]
    (ou du moins si on veut s’assurer qu’il y a au moins trois éléments [...])

Ok, fait avec *dummy.

  • Du détail, et par simple curiosité, dans authentic2.utils.misc.select_next_url, je comprends pas pourquoi on instancie un EMPTY = object() et faire la distinction entre None et cet objet ?

Au cas où l'appelant voudrait utiliser None comme valeur par défaut et pas une des valeurs pré-définies ailleurs (home_url, etc...).

Est-ce qu’on ne peut pas simplement faire la même distinction entre None et '', genre
[...]
et auquel cas dans set_home_url avec on appellerait select_next_url avec default='' ?

Mouais, je préfère le EMPTY, c'est impossible de se tromper. Avec ta solution il faut lire le code pour savoir si c'est '' ou None ou que sais-je. C'est un usage assez classique en pyhon pour le paramètre default.

  • Pour l’usage de set_service j’ai fait quelques git-grep ici et là, ça m’a l’air bon.
    Il y aurait aussi des modifs pertinentes dans les modules tiers, là à vue de nez je vois par exemple authentic2-cut où on place le slug de service dans la session (dans CUTMiddleware), pour l’utiliser notamment pour les entrées de journal. Je pense qu’on peut faire un ticket pour virer ça, on n’a plus besoin de ce slug dans la session dorénavant.

Oui c'est un peu le but, je vais ouvrir le ticket.

#13

Updated by Paul Marillonnet 4 months ago

Benjamin Dauvergne a écrit :

Mouais, je préfère le EMPTY, c'est impossible de se tromper. Avec ta solution il faut lire le code pour savoir si c'est '' ou None ou que sais-je. C'est un usage assez classique en pyhon pour le paramètre default.

Ok, je ne connaissais pas.

Oui c'est un peu le but, je vais ouvrir le ticket.

D’ac.

#15

Updated by Transition automatique 4 months ago

  • Status changed from Résolu (à déployer) to Solution déployée
#16

Updated by Paul Marillonnet 4 months ago

Paul Marillonnet a écrit :

Benjamin Dauvergne a écrit :

Mouais, je préfère le EMPTY, c'est impossible de se tromper. Avec ta solution il faut lire le code pour savoir si c'est '' ou None ou que sais-je. C'est un usage assez classique en pyhon pour le paramètre default.

Ok, je ne connaissais pas.

Oui c'est un peu le but, je vais ouvrir le ticket.

D’ac.

#61508.

#18

Updated by Transition automatique about 2 months ago

Automatic expiration

Also available in: Atom PDF