Bug #31259
présenter les IDP OpenidConnect dans des blocks à part sur la page de login
0%
Description
Afin de pouvoir les filtrer en fonction de la provenance de l'usager.
Fichiers
Demandes liées
Révisions associées
Historique
Mis à jour par Serghei Mihai il y a environ 5 ans
- Lié à Development #30960: Permetttre de faire d'authentic un IdP proxy transparent ajouté
Mis à jour par Serghei Mihai il y a environ 5 ans
- Fichier 0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch 0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch ajouté
- Tracker changé de Support à Bug
- Statut changé de Nouveau à Solution proposée
- Patch proposed changé de Non à Oui
Tests en cours d'ajout.
Dépend du renommage des backends dans #14475.
Mis à jour par Serghei Mihai il y a environ 5 ans
- Fichier 0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch 0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch ajouté
Et avec un simple test.
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 deassert '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).
Mis à jour par Serghei Mihai il y a presque 5 ans
- Fichier 0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch 0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch ajouté
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.
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 ?
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).
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.
Mis à jour par Serghei Mihai il y a plus de 4 ans
- Fichier 0002-auth-separate-OIDC-providers-in-blocks-on-login-page.patch 0002-auth-separate-OIDC-providers-in-blocks-on-login-page.patch ajouté
- Fichier 0001-oidc-fix-next-url-encoding-on-login-page.patch 0001-oidc-fix-next-url-encoding-on-login-page.patch ajouté
- Statut changé de Information nécessaire à Solution proposée
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.
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.
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.)
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)
Mis à jour par Serghei Mihai il y a plus de 4 ans
- Fichier 0002-auth-separate-OIDC-providers-in-blocks-on-login-page.patch 0002-auth-separate-OIDC-providers-in-blocks-on-login-page.patch ajouté
- Fichier 0001-oidc-fix-next-url-encoding-on-login-page.patch 0001-oidc-fix-next-url-encoding-on-login-page.patch ajouté
- Statut changé de En cours à Solution proposée
Bien vu: next
rajouté.
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).
Mis à jour par Serghei Mihai il y a plus de 4 ans
- Fichier 0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch 0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch ajouté
Oké. En un seul patch.
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é
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.
Mis à jour par Serghei Mihai il y a plus de 4 ans
- Fichier 0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch 0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch ajouté
- Statut changé de Solution validée à Solution proposée
Yes, le nettoyage de ce is_hidden
et visible_blocks_count
à coordonner avec le nettoyage dans publik-base-theme.
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.
Mis à jour par Benjamin Dauvergne il y a plus de 4 ans
Effectivement le is_hidden va manquer aux blocks legacy.
Mis à jour par Serghei Mihai il y a plus de 4 ans
- Fichier 0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch 0001-auth-separate-OIDC-providers-in-blocks-on-login-page.patch ajouté
Prochaine étape est de se débarasser de ce "is_hidden"
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é
Mis à jour par Benjamin Dauvergne il y a plus de 4 ans
- Statut changé de Solution proposée à Solution validée
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)
Mis à jour par Serghei Mihai il y a plus de 4 ans
- Lié à Development #38125: oidc: simplifier la construction des urls de connexion ajouté
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
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)
Mis à jour par Frédéric Péters il y a plus de 4 ans
- Lié à Bug #38291: bloc de connexion ozwillo ajouté
auth: separate OIDC providers in blocks on login page (#31259)