Projet

Général

Profil

Bug #31259

présenter les IDP OpenidConnect dans des blocks à part sur la page de login

Ajouté par Serghei Mihai il y a environ 5 ans. Mis à jour il y a plus de 4 ans.

Statut:
Fermé
Priorité:
Normal
Assigné à:
Catégorie:
-
Version cible:
-
Début:
11 mars 2019
Echéance:
% réalisé:

0%

Temps estimé:
Patch proposed:
Oui
Planning:
Non

Description

Afin de pouvoir les filtrer en fonction de la provenance de l'usager.


Fichiers

0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch (4,24 ko) 0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch Serghei Mihai, 12 mars 2019 10:40
0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch (5,76 ko) 0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch Serghei Mihai, 12 mars 2019 21:03
0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch (5,76 ko) 0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch Serghei Mihai, 01 avril 2019 19:07
0002-auth-separate-OIDC-providers-in-blocks-on-login-page.patch (5,76 ko) 0002-auth-separate-OIDC-providers-in-blocks-on-login-page.patch Serghei Mihai, 03 octobre 2019 17:59
0001-oidc-fix-next-url-encoding-on-login-page.patch (1,08 ko) 0001-oidc-fix-next-url-encoding-on-login-page.patch Serghei Mihai, 03 octobre 2019 17:59
0002-auth-separate-OIDC-providers-in-blocks-on-login-page.patch (5,8 ko) 0002-auth-separate-OIDC-providers-in-blocks-on-login-page.patch Serghei Mihai, 08 octobre 2019 11:55
0001-oidc-fix-next-url-encoding-on-login-page.patch (1,09 ko) 0001-oidc-fix-next-url-encoding-on-login-page.patch Serghei Mihai, 08 octobre 2019 11:55
0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch (6,31 ko) 0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch Serghei Mihai, 17 octobre 2019 18:09
0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch (5,73 ko) 0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch Serghei Mihai, 28 novembre 2019 15:50
0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch (6,28 ko) 0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch Serghei Mihai, 28 novembre 2019 17:42

Demandes liées

Lié à Authentic 2 - Development #30960: Permetttre de faire d'authentic un IdP proxy transparentFermé27 février 201931 mai 2019

Actions
Lié à Authentic 2 - Development #28215: permettre le filtrage des frontends d'authentification à exposer à l'usagerFermé21 novembre 2018

Actions
Lié à Intégrations graphiques Publik - Development #38063: ne plus utiliser la variable "visible_blocks_count" pour le nombre de blocks sur la page d'authentificationFermé28 novembre 2019

Actions
Lié à Authentic 2 - Development #38125: oidc: simplifier la construction des urls de connexionFermé02 décembre 2019

Actions
Lié à Intégrations graphiques Publik - Bug #38291: bloc de connexion ozwilloFermé09 décembre 2019

Actions

Révisions associées

Révision 216323c7 (diff)
Ajouté par Serghei Mihai il y a plus de 4 ans

auth: separate OIDC providers in blocks on login page (#31259)

Historique

#1

Mis à jour par Serghei Mihai il y a environ 5 ans

  • Lié à Development #30960: Permetttre de faire d'authentic un IdP proxy transparent ajouté
#2

Mis à jour par Serghei Mihai il y a environ 5 ans

Tests en cours d'ajout.
Dépend du renommage des backends dans #14475.

#4

Mis à jour par Paul Marillonnet il y a presque 5 ans

  • Le tests ne vérifie explicitement pas la présence des deux blocs en même temps.
    J'apprécierais bien un truc un peu plus costaud et plus évocateur, genre un peu de pyquery, au lieu de assert 'machin' in response.
  • Un saut à la ligne en trop en à la fin de src/authentic2_auth_oidc/templates/authentic2_auth_oidc/login.html
  • Un autre en trop à la fin de la définition de OIDCAuthenticator.instances.
  • À la lecture du code, on s'attendrait à ce que cette fonction OIDCAuthenticator.instances soit une propriété de classe, avec un générateur, ce qui n'est pas le cas.
    Je pense que tu peux t'en passer complètement, ça ne changerait pas grand chose de reconstruire le tuple (instance_id, instance) directement dans la vue.
    Ça t'éviterait d'avoir un dictionnaire qui donne l'impression d'être modifié en cours d'itération pendant que chaque itération dépend de son contenu (même si dans les faits ce n'est pas le cas, ce n'est que l'impression que le code donne).
#5

Mis à jour par Serghei Mihai il y a presque 5 ans

Paul Marillonnet a écrit :

  • Le tests ne vérifie explicitement pas la présence des deux blocs en même temps.

test_providers_on_login_page vérifie d'abord qu'il n'y qu'un seul bloc OIDC, puis crée un deuxième IDP OIDC et vérifie que l'onglet associé est créé.
Fait avec pyquery.

  • Un saut à la ligne en trop en à la fin de src/authentic2_auth_oidc/templates/authentic2_auth_oidc/login.html

Corrigé.

  • À la lecture du code, on s'attendrait à ce que cette fonction OIDCAuthenticator.instances soit une propriété de classe, avec un générateur, ce qui n'est pas le cas.

Tant que c'est un itérable: ok pour le générateur.

Je pense que tu peux t'en passer complètement, ça ne changerait pas grand chose de reconstruire le tuple (instance_id, instance) directement dans la vue.

Tu parles de

block['id'] = instance_id
?

Je suis bien obligé car sinon le code de la vue ne fait pas de distinction entre les blocs "oidc". Branche à jour.

#6

Mis à jour par Paul Marillonnet il y a plus de 4 ans

  • Statut changé de Solution proposée à Information nécessaire

Désolé, j'ai laissé passer ça. Cette partie du code ayant un peu bougé entre temps, peux-tu proposer un patch rebasé sur master stp ?

#7

Mis à jour par Serghei Mihai il y a plus de 4 ans

Branche à jour par rapport au master.

#8

Mis à jour par Paul Marillonnet il y a plus de 4 ans

Quelques modifs qui me semblent pertinentes (dont un bogue déjà présent sur l'argument de querystring next dans le gabarit du bloc d'authentification) :

diff --git a/src/authentic2/views.py b/src/authentic2/views.py
index eb1fb8a4f..063a42c7b 100644
--- a/src/authentic2/views.py
+++ b/src/authentic2/views.py
@@ -325,7 +325,7 @@ def login(request, template_name='authentic2/login.html',
                           'context': context}
             # check if the authenticator has multiple instances
             if hasattr(authenticator, 'instances'):
-                for instance_id, instance in authenticator.instances(**parameters):
+                for instance_id, instance in authenticator.instances:
                     parameters['instance'] = instance
                     block = utils.get_authenticator_method(authenticator, 'login', parameters)
                     # update block id in order to separate instances
diff --git a/src/authentic2_auth_oidc/authenticators.py b/src/authentic2_auth_oidc/authenticators.py
index 84ee27a87..47b106336 100644
--- a/src/authentic2_auth_oidc/authenticators.py
+++ b/src/authentic2_auth_oidc/authenticators.py
@@ -30,7 +30,8 @@ class OIDCAuthenticator(object):
     def id(self):
         return 'oidc'

-    def instances(self, request, *args, **kwargs):
+    @property
+    def instances(self):
         for p in utils.get_providers(shown=True):
             yield (p.slug, p)

diff --git a/src/authentic2_auth_oidc/templates/authentic2_auth_oidc/login.html b/src/authentic2_auth_oidc/templates/authentic2_auth_oidc/login.html
index bd3056883..c2aeab906 100644
--- a/src/authentic2_auth_oidc/templates/authentic2_auth_oidc/login.html
+++ b/src/authentic2_auth_oidc/templates/authentic2_auth_oidc/login.html
@@ -1,4 +1,4 @@
 <p id="oidc-p-{% firstof provider.slug provider.name|slugify %}">
   <a id="oidc-a-{% firstof provider.slug  provider.name|slugify %}" 
-     href="{% url "oidc-login" pk=provider.pk %}?{{ request.GET.urlencode }}">{{ provider.name }}</a>
+     href="{% url "oidc-login" pk=provider.pk %}?{{ request.GET.next|urlencode }}">{{ provider.name }}</a>
 </p>
diff --git a/tests/test_auth_oidc.py b/tests/test_auth_oidc.py
index 5910cf275..00a92204d 100644
--- a/tests/test_auth_oidc.py
+++ b/tests/test_auth_oidc.py
@@ -309,8 +309,6 @@ def test_providers_on_login_page(oidc_provider, app):
     assert response.pyquery('p#oidc-p-oidcidp-2')

-
-
 def test_sso(app, caplog, code, oidc_provider, oidc_provider_jwkset, login_url, login_callback_url, hooks):
     OU = get_ou_model()
     cassis = OU.objects.create(name='Cassis', slug='cassis')

Une dernière chose encore, qui sera peut-être l'objet d'un autre ticket, et qui est le comportement de ce bout de code dans authentic2.views.login :

            # If a login frontend method returns an HttpResponse with a status code != 200
            # this response is returned.
            for block in auth_blocks:
                if block:
                    if block['status_code'] != 200:
                        return block['response']

-> Ce n'est peut-être plus ce que l'on veut, maintenant qu'on a potentiellement des blocs issus d'instances multiples d'un même authentificateur (cas dans lequel on pourrait peut-être, au lieu de remonter l'erreur, détecter si au moins un bloc pour une des instances d'un même authentificateur renvoie bien un 200, auquel cas on ne retiendrait pas les autres en erreur).

#9

Mis à jour par Serghei Mihai il y a plus de 4 ans

Tes remarques prises en compte.

Pour le status code, comme c'est général pour tous les authentificateurs on devrait gérer cela séparément.

#11

Mis à jour par Paul Marillonnet il y a plus de 4 ans

  • Statut changé de Solution proposée à Solution validée

(Il reste encore un quadruple saut de ligne à la fin du test, à corriger) sinon c'est bon pour moi. Ack.

#12

Mis à jour par Benjamin Dauvergne il y a plus de 4 ans

  • Statut changé de Solution validée à En cours

Les tests ne passent pas.

#13

Mis à jour par Paul Marillonnet il y a plus de 4 ans

(Trop fatigué pour comparer avec le build précédent et voir pourquoi ça ne passe pas maintenant. Je regarde demain.)

#14

Mis à jour par Frédéric Péters il y a plus de 4 ans

(Trop fatigué pour comparer avec le build précédent et voir pourquoi ça ne passe pas maintenant. Je regarde demain.)

Ce ne serait pas la suggestion de faire :

-     href="{% url "oidc-login" pk=provider.pk %}?{{ request.GET.urlencode }}">{{ provider.name }}</a>
+     href="{% url "oidc-login" pk=provider.pk %}?{{ request.GET.next|urlencode }}">{{ provider.name }}</a>

qui va faire ?whatever plutôt que ?next=whatever ?

(je ne comprends pas de quel bug tu pouvais parler dans le commentaire précédent)

#16

Mis à jour par Benjamin Dauvergne il y a plus de 4 ans

Tant qu'à bosser sur une fonctionnalité pas super passionnante j'aimerai beaucoup qu'on en profite pour ne plus construire d'URL dans un template (je plaide coupable); tout ticket doit être l'occasion d'améliorer un peu le code quand on passe dessus (ici utiliser authentic2.utils.make_url qui est fait pour gérer toutes les manipulations d'URL).

#18

Mis à jour par Benjamin Dauvergne il y a plus de 4 ans

Merci, je relis.

#19

Mis à jour par Serghei Mihai il y a plus de 4 ans

  • Lié à Development #28215: permettre le filtrage des frontends d'authentification à exposer à l'usager ajouté
#20

Mis à jour par Benjamin Dauvergne il y a plus de 4 ans

  • Statut changé de Solution proposée à Solution validée

Benjamin Dauvergne a écrit :

Merci, je relis.

Garder l'id initial dans les blocks, genre block['id'] = block['id'] + '_' + instance_id sinon on a rien qui permette d'éviter des collisions.

J'ai cherché il n'y a rien qui utilise is_hidden tu peux virer ce code, i.e. mettre is_hidden à False tout le temps et commentaire FIXME pour le virer des templates plus tard.

#21

Mis à jour par Serghei Mihai il y a plus de 4 ans

Yes, le nettoyage de ce is_hidden et visible_blocks_count à coordonner avec le nettoyage dans publik-base-theme.

#22

Mis à jour par Thomas Noël il y a plus de 4 ans

Moi je suis surpris par l'identation qui change pour ce qui concerne l'ancien code

        if hasattr(authenticator, 'is_hidden'):
            blocks[-1]['is_hidden'] = authenticator.is_hidden(request)
        else:
            blocks[-1]['is_hidden'] = False

qui n'était pas compris dans le "else: # New frontends API" comme il semble l'être dans ce patch.

#23

Mis à jour par Benjamin Dauvergne il y a plus de 4 ans

Effectivement le is_hidden va manquer aux blocks legacy.

#25

Mis à jour par Serghei Mihai il y a plus de 4 ans

  • Lié à Development #38063: ne plus utiliser la variable "visible_blocks_count" pour le nombre de blocks sur la page d'authentification ajouté
#26

Mis à jour par Benjamin Dauvergne il y a plus de 4 ans

  • Statut changé de Solution proposée à Solution validée
#27

Mis à jour par Serghei Mihai il y a plus de 4 ans

  • Statut changé de Solution validée à Résolu (à déployer)
commit 216323c7ad560b8417535dcc5fdca694b3713d7e (origin/master, origin/HEAD)
Author: Serghei Mihai <smihai@entrouvert.com>
Date:   Fri Mar 8 14:57:45 2019 +0100

    auth: separate OIDC providers in blocks on login page (#31259)
#28

Mis à jour par Serghei Mihai il y a plus de 4 ans

  • Lié à Development #38125: oidc: simplifier la construction des urls de connexion ajouté
#29

Mis à jour par Frédéric Péters il y a plus de 4 ans

  • Statut changé de Résolu (à déployer) à Solution déployée
#31

Mis à jour par Frédéric Péters il y a plus de 4 ans

(pour info ça a cassé toodego) (je m'en occupe, pour continuer à contourner)

#32

Mis à jour par Frédéric Péters il y a plus de 4 ans

  • Lié à Bug #38291: bloc de connexion ozwillo ajouté

Formats disponibles : Atom PDF