Project

General

Profile

Bug #31259

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

Added by Serghei Mihai 9 months ago. Updated 6 days ago.

Status:
Solution déployée
Priority:
Normal
Assignee:
Category:
-
Target version:
-
Start date:
11 Mar 2019
Due date:
% Done:

0%

Patch proposed:
Yes
Planning:
No

Description

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

0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch View (4.24 KB) Serghei Mihai, 12 Mar 2019 10:40 AM

0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch View (5.76 KB) Serghei Mihai, 12 Mar 2019 09:03 PM

0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch View (5.76 KB) Serghei Mihai, 01 Apr 2019 07:07 PM

0002-auth-separate-OIDC-providers-in-blocks-on-login-page.patch View (5.76 KB) Serghei Mihai, 03 Oct 2019 05:59 PM

0001-oidc-fix-next-url-encoding-on-login-page.patch View (1.08 KB) Serghei Mihai, 03 Oct 2019 05:59 PM

0002-auth-separate-OIDC-providers-in-blocks-on-login-page.patch View (5.8 KB) Serghei Mihai, 08 Oct 2019 11:55 AM

0001-oidc-fix-next-url-encoding-on-login-page.patch View (1.09 KB) Serghei Mihai, 08 Oct 2019 11:55 AM

0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch View (6.31 KB) Serghei Mihai, 17 Oct 2019 06:09 PM

0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch View (5.73 KB) Serghei Mihai, 28 Nov 2019 03:50 PM

0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch View (6.28 KB) Serghei Mihai, 28 Nov 2019 05:42 PM


Related issues

Related to Authentic 2 - Development #30960: Permetttre de faire d'authentic un IdP proxy transparent Nouveau 27 Feb 2019 31 May 2019
Related to Authentic 2 - Bug #28215: permettre le filtrage des frontends d'authentification à exposer à l'usager Solution proposée 21 Nov 2018
Related to Intégrations graphiques Publik - Bug #38063: ne plus utiliser la variable "visible_blocks_count" pour le nombre de blocks sur la page d'authentification Solution proposée 28 Nov 2019
Related to Authentic 2 - Development #38125: oidc: simplifier la construction des urls de connexion Solution déployée 02 Dec 2019
Related to Intégrations graphiques Publik - Bug #38291: bloc de connexion ozwillo Solution déployée 09 Dec 2019

Associated revisions

Revision 216323c7 (diff)
Added by Serghei Mihai 16 days ago

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

History

#1 Updated by Serghei Mihai 9 months ago

  • Related to Development #30960: Permetttre de faire d'authentic un IdP proxy transparent added

#2 Updated by Serghei Mihai 9 months ago

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

#4 Updated by Paul Marillonnet 9 months ago

  • 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 Updated by Serghei Mihai 9 months ago

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 Updated by Paul Marillonnet 4 months ago

  • Status changed from Solution proposée to 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 Updated by Serghei Mihai 2 months ago

Branche à jour par rapport au master.

#8 Updated by Paul Marillonnet 2 months ago

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 Updated by Serghei Mihai 2 months ago

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.

#10 Updated by Serghei Mihai 2 months ago

#11 Updated by Paul Marillonnet 2 months ago

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

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

#12 Updated by Benjamin Dauvergne 2 months ago

  • Status changed from Solution validée to En cours

Les tests ne passent pas.

#13 Updated by Paul Marillonnet 2 months ago

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

#14 Updated by Frédéric Péters 2 months ago

(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)

#15 Updated by Serghei Mihai 2 months ago

Bien vu: next rajouté.

#16 Updated by Benjamin Dauvergne 2 months ago

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 Updated by Benjamin Dauvergne about 2 months ago

Merci, je relis.

#19 Updated by Serghei Mihai 19 days ago

  • Related to Bug #28215: permettre le filtrage des frontends d'authentification à exposer à l'usager added

#20 Updated by Benjamin Dauvergne 17 days ago

  • Status changed from Solution proposée to 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 Updated by Serghei Mihai 17 days ago

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

#22 Updated by Thomas Noël 17 days ago

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 Updated by Benjamin Dauvergne 17 days ago

Effectivement le is_hidden va manquer aux blocks legacy.

#24 Updated by Serghei Mihai 17 days ago

Prochaine étape est de se débarasser de ce "is_hidden"

#25 Updated by Serghei Mihai 17 days ago

  • Related to Bug #38063: ne plus utiliser la variable "visible_blocks_count" pour le nombre de blocks sur la page d'authentification added

#26 Updated by Benjamin Dauvergne 17 days ago

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

#27 Updated by Serghei Mihai 16 days ago

  • Status changed from Solution validée to 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 Updated by Serghei Mihai 13 days ago

  • Related to Development #38125: oidc: simplifier la construction des urls de connexion added

#29 Updated by Frédéric Péters 11 days ago

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

#31 Updated by Frédéric Péters 6 days ago

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

#32 Updated by Frédéric Péters 6 days ago

  • Related to Bug #38291: bloc de connexion ozwillo added

Also available in: Atom PDF