entrouvert
/
authentic
16
0
Fork 0
Code Issues Pull Requests 33 Releases Activity

auth_oidc: avoid user messages with prompt=none related errors (#72538) #38

Merged
pmarillonnet merged 1 commits from wip/72538-auth-oidc-prompt-none-error into main 2023-04-19 12:59:12 +02:00
Conversation 2 Commits 1 Files Changed 5 +85 -30
pmarillonnet commented 2023-04-17 09:35:43 +02:00
Owner
Copy Link

Le ticket est déjà validé mais à la relecture on m’a demandé une PR avec la version rebasée de la branche. La voici.

Le ticket est déjà validé mais à la relecture on m’a demandé une PR avec la version rebasée de la branche. La voici.
bdauvergne commented 2023-04-17 11:48:14 +02:00
Owner
Copy Link

Je déplace ici ma remarque du ticket #73669 parce qu'en fait c'est ici que ça se joue.

En refaisant l'état des lieux entre ce ticket, #72538 et le fait de faire marcher is_passive sur GLC (#24195) je me dis que qu'on doit varier le comportement selon qu'on a envoyé prompt=none ou pas:

si prompt=none on log rien, on ne s'attendait pas à ce que le sso fonctionne, ça foire, tant pis,
si on a pas prompt=none alors bien sur on peut logger via logging mais surtout il faudrait enregistrer dans le journal une user.login.failure :

     request.journal.record('user.login.failure', ...) # ici on ne sait pas trop quoi mettre comme contexte


on ne peut pas vraiment y indiquer d'attributs de contexte vu qu'on a rien à part une erreur mais donc il faudrait modifier la définition du type d'èvènement login.failure pour que username ne soit plus requis mais qu'on y enregistre les références au mode d'authentification ainsi que le message d'erreur précis remonté. Ça permettra une éventuelle analyse à posteriori par un admin.

src/authentic2/journal_event_types.py
class UserLoginFailure(EventTypeWithService):
name = 'user.login.failure'
label = _('login failure')

@classmethod
def record(cls, *, service, username, user, reason=None):
    super().record(user=user, service=service, data={'username': username, 'reason': reason})

@classmethod
def get_message(cls, event, context):
    username = event.get_data('username')
    reason = event.get_data('reason')
    msg = _('login failure with username "{username}"').format(username=username)
    if reason:
        msg.append(_(' (reason: {reason})').format(reason=reason))
    return msg

Et je rajoute le patch fourni sur la PR-39 initialement qui permet de conserver le paramètre prompt au cours du processus de SSO.

diff --git a/src/authentic2_auth_oidc/views.py b/src/authentic2_auth_oidc/views.py
index 602eb512..a8732e8d 100644
--- a/src/authentic2_auth_oidc/views.py
+++ b/src/authentic2_auth_oidc/views.py
@@ -63,6 +63,7 @@ def oidc_login(request, pk, next_url=None, passive=None, *args, **kwargs):
             prompt.add('none')
         else:
             prompt.add('login')
+        state_content['prompt'] = prompt
     params = {
         'client_id': provider.client_id,
         'scope': ' '.join(scopes),
@@ -168,7 +169,7 @@ class LoginCallback(View):
             return response
 
         if 'error' in request.GET:  # error code path
-            return self.handle_error(request, provider)
+            return self.handle_error(request, provider, prompt=state_content.get('prompt'))
         elif not code:
             messages.warning(request, _('Missing code, report %s to an administrator') % request.request_id)
             logger.warning('auth_oidc: missing code, %r', request.GET)
@@ -309,7 +310,7 @@ class LoginCallback(View):
         }
     }
 
-    def handle_error(self, request, provider):
+    def handle_error(self, request, provider, prompt):
         error = request.GET['error']
         error_dict = self.errors.get(error, {})
         level = error_dict.get('level', logging.WARNING)
Je déplace ici ma remarque du ticket #73669 parce qu'en fait c'est ici que ça se joue. > En refaisant l'état des lieux entre ce ticket, #72538 et le fait de faire marcher is_passive sur GLC (#24195) je me dis que qu'on doit varier le comportement selon qu'on a envoyé prompt=none ou pas: > > si prompt=none on log rien, on ne s'attendait pas à ce que le sso fonctionne, ça foire, tant pis, > si on a pas prompt=none alors bien sur on peut logger via logging mais surtout il faudrait enregistrer dans le journal une user.login.failure : > > request.journal.record('user.login.failure', ...) # ici on ne sait pas trop quoi mettre comme contexte > > > on ne peut pas vraiment y indiquer d'attributs de contexte vu qu'on a rien à part une erreur mais donc il faudrait modifier la définition du type d'èvènement login.failure pour que username ne soit plus requis mais qu'on y enregistre les références au mode d'authentification ainsi que le message d'erreur précis remonté. Ça permettra une éventuelle analyse à posteriori par un admin. > > src/authentic2/journal_event_types.py > class UserLoginFailure(EventTypeWithService): > name = 'user.login.failure' > label = _('login failure') > > @classmethod > def record(cls, *, service, username, user, reason=None): > super().record(user=user, service=service, data={'username': username, 'reason': reason}) > > @classmethod > def get_message(cls, event, context): > username = event.get_data('username') > reason = event.get_data('reason') > msg = _('login failure with username "{username}"').format(username=username) > if reason: > msg.append(_(' (reason: {reason})').format(reason=reason)) > return msg > Et je rajoute le patch fourni sur la PR-39 initialement qui permet de conserver le paramètre prompt au cours du processus de SSO. ``` diff --git a/src/authentic2_auth_oidc/views.py b/src/authentic2_auth_oidc/views.py index 602eb512..a8732e8d 100644 --- a/src/authentic2_auth_oidc/views.py +++ b/src/authentic2_auth_oidc/views.py @@ -63,6 +63,7 @@ def oidc_login(request, pk, next_url=None, passive=None, *args, **kwargs): prompt.add('none') else: prompt.add('login') + state_content['prompt'] = prompt params = { 'client_id': provider.client_id, 'scope': ' '.join(scopes), @@ -168,7 +169,7 @@ class LoginCallback(View): return response if 'error' in request.GET: # error code path - return self.handle_error(request, provider) + return self.handle_error(request, provider, prompt=state_content.get('prompt')) elif not code: messages.warning(request, _('Missing code, report %s to an administrator') % request.request_id) logger.warning('auth_oidc: missing code, %r', request.GET) @@ -309,7 +310,7 @@ class LoginCallback(View): } } - def handle_error(self, request, provider): + def handle_error(self, request, provider, prompt): error = request.GET['error'] error_dict = self.errors.get(error, {}) level = error_dict.get('level', logging.WARNING) ```
bdauvergne requested changes 2023-04-17 11:49:44 +02:00
src/authentic2_auth_oidc/views.py Outdated
@ -343,3 +328,1 @@
)
else:
message = _('Login with OpenID Connect failed, report %s to an administrator. (%(error)s)')
if error not in (
bdauvergne commented 2023-04-17 11:49:40 +02:00
Owner
Copy Link

Remplace ça par un check sur prompt=none, plus général et plus pertinent dans le sens où on se décide par rapport à ce que le RP a demandé et pas uniquement par rapport

Remplace ça par un check sur prompt=none, plus général et plus pertinent dans le sens où on se décide par rapport à ce que le RP a demandé et pas uniquement par rapport
pmarillonnet marked this conversation as resolved
pmarillonnet force-pushed wip/72538-auth-oidc-prompt-none-error from 85fa18ebd8 to 03f6f9e711 2023-04-17 17:17:20 +02:00 Compare
pmarillonnet force-pushed wip/72538-auth-oidc-prompt-none-error from 03f6f9e711 to fea0568936 2023-04-18 10:25:55 +02:00 Compare
pmarillonnet force-pushed wip/72538-auth-oidc-prompt-none-error from fea0568936 to 568a2bd8ac 2023-04-18 14:35:21 +02:00 Compare
pmarillonnet force-pushed wip/72538-auth-oidc-prompt-none-error from 568a2bd8ac to 5a93ac1739 2023-04-18 14:47:07 +02:00 Compare
pmarillonnet force-pushed wip/72538-auth-oidc-prompt-none-error from 5a93ac1739 to 94345c6be9 2023-04-18 14:59:48 +02:00 Compare
pmarillonnet commented 2023-04-18 15:15:33 +02:00
Author
Owner
Copy Link

Merci pour la relecture, j’ai mis à jour la branche en conséquence.

Merci pour la relecture, j’ai mis à jour la branche en conséquence.
pmarillonnet requested review from bdauvergne 2023-04-18 15:15:37 +02:00
bdauvergne requested changes 2023-04-18 15:19:24 +02:00
src/authentic2/journal_event_types.py Outdated
@ -162,3 +162,2 @@
@classmethod
def record(cls, *, service, username, user, reason=None):
super().record(user=user, service=service, data={'username': username, 'reason': reason})
def record(cls, *, service, username=None, user=None, reason=None, authn_type=None, errmsg=None):
bdauvergne commented 2023-04-18 15:19:20 +02:00
Owner
Copy Link

Vu qu'on a des objets Authenticator , plutôt que authn_type j'aurai bien mis un authenticator_pk, ça permettra d'avoir une vue sur les erreurs d'un authenticateur dans l'admin, de les compter, etc...

À voir ensuite dans get_message pour produire un truc intelligible juste avec le type de l'authenticateur (si les messages ne sont pas magnifiques ça n'est pas bien grave).

Vu qu'on a des objets Authenticator , plutôt que authn_type j'aurai bien mis un authenticator_pk, ça permettra d'avoir une vue sur les erreurs d'un authenticateur dans l'admin, de les compter, etc... À voir ensuite dans get_message pour produire un truc intelligible juste avec le type de l'authenticateur (si les messages ne sont pas magnifiques ça n'est pas bien grave).
bdauvergne commented 2023-04-18 15:22:02 +02:00
Owner
Copy Link

Et vu que c'est un objet en base tu peux juste le passer comme objet en référence. Aussi reason n'est pas utilisé ici, ne pas l'introduire, dans data on doit juste avoir username et/ou error.

Et vu que c'est un objet en base tu peux juste le passer comme objet en référence. Aussi reason n'est pas utilisé ici, ne pas l'introduire, dans data on doit juste avoir username et/ou error.
bdauvergne commented 2023-04-18 15:29:53 +02:00
Owner
Copy Link

La signature finale ce serait record(cls, *, service, username=None, authenticator=None, error=None).

La signature finale ce serait `record(cls, *, service, username=None, authenticator=None, error=None)`.
pmarillonnet commented 2023-04-19 09:46:04 +02:00
Author
Owner
Copy Link

Merci, c’est corrigé dans la branche.
J’ai laissé reason dans la signature, parce que cet argument pré-existe au patch (et que d’ailleurs il est utilisé pour la journalisation des erreurs de login dans l’authentification par mot de passe local).

Merci, c’est corrigé dans la branche. J’ai laissé `reason` dans la signature, parce que cet argument pré-existe au patch (et que d’ailleurs il est utilisé pour la journalisation des erreurs de login dans l’authentification par mot de passe local).
bdauvergne commented 2023-04-19 11:16:25 +02:00
Owner
Copy Link

Ok pour reason, alors c'est error qui pourrait dégager, y en a un en trop de toute façon.

Ok pour reason, alors c'est error qui pourrait dégager, y en a un en trop de toute façon.
pmarillonnet force-pushed wip/72538-auth-oidc-prompt-none-error from 94345c6be9 to bc947852b5 2023-04-19 09:20:43 +02:00 Compare
pmarillonnet force-pushed wip/72538-auth-oidc-prompt-none-error from bc947852b5 to 478bfee3fa 2023-04-19 09:30:59 +02:00 Compare
pmarillonnet requested review from bdauvergne 2023-04-19 09:46:20 +02:00
bdauvergne requested changes 2023-04-19 11:22:16 +02:00
src/authentic2/journal_event_types.py Outdated
@ -162,3 +162,2 @@
@classmethod
def record(cls, *, service, username, user, reason=None):
super().record(user=user, service=service, data={'username': username, 'reason': reason})
def record(cls, *, service, username=None, user=None, reason=None, authenticator=None, error=None):
bdauvergne commented 2023-04-19 11:21:40 +02:00
Owner
Copy Link

Virer error ne garder que reason, ça fait doublon.

Virer error ne garder que reason, ça fait doublon.
src/authentic2/journal_event_types.py Outdated
@ -165,0 +167,4 @@
data={
'username': username,
'reason': reason,
'authenticator': authenticator.name if authenticator else None,
bdauvergne commented 2023-04-19 11:20:18 +02:00
Owner
Copy Link

Ne pas mettre authenticator dans data mais dans references, sinon on ne peut pas facilement faire de requête en base.

Ne pas mettre authenticator dans data mais dans references, sinon on ne peut pas facilement faire de requête en base.
src/authentic2/journal_event_types.py
@ -165,3 +173,4 @@
)
@classmethod
def get_message(cls, event, context):
bdauvergne commented 2023-04-19 11:21:13 +02:00
Owner
Copy Link

et là 👍

(authenticator,) = self.get_typed_references(Authenticator)

au lieu de regarder dans data.

et là 👍 (authenticator,) = self.get_typed_references(Authenticator) au lieu de regarder dans data.
pmarillonnet commented 2023-04-19 11:51:23 +02:00
Author
Owner
Copy Link

C’est fait dans la branche, mais visiblement le prefetch des références lors de l’affichage du journal se passe mal : https://jenkins.entrouvert.org/job/gitea/job/authentic/job/wip%252F72538-auth-oidc-prompt-none-error/11/console

Je ne connais pas ce code, je ne sais pas si c’est du au fait que dans le type d’événement on sait seulement qu’on a un BaseAuthenticator, mais que dans les appels à la journalisation c’est une de ses classes filles, et que par conséquent les ContentTypes ne concordent pas.

Bref ça marche pas en l’état, et j’ai un peu la flemme de déboguer EventQuerySet.prefetch_references pour comprendre ce qui ne va pas, alors que cette PR au départ se limite aux messages affichés à l’usager sur les retours d’erreur de connexion OIDC…

C’est fait dans la branche, mais visiblement le prefetch des références lors de l’affichage du journal se passe mal : https://jenkins.entrouvert.org/job/gitea/job/authentic/job/wip%252F72538-auth-oidc-prompt-none-error/11/console Je ne connais pas ce code, je ne sais pas si c’est du au fait que dans le type d’événement on sait seulement qu’on a un BaseAuthenticator, mais que dans les appels à la journalisation c’est une de ses classes filles, et que par conséquent les ContentTypes ne concordent pas. Bref ça marche pas en l’état, et j’ai un peu la flemme de déboguer `EventQuerySet.prefetch_references` pour comprendre ce qui ne va pas, alors que cette PR au départ se limite aux messages affichés à l’usager sur les retours d’erreur de connexion OIDC…
bdauvergne commented 2023-04-19 11:56:47 +02:00
Owner
Copy Link

C’est fait dans la branche, mais visiblement le prefetch des références lors de l’affichage du journal se passe mal : https://jenkins.entrouvert.org/job/gitea/job/authentic/job/wip%252F72538-auth-oidc-prompt-none-error/11/console

Je ne connais pas ce code, je ne sais pas si c’est du au fait que dans le type d’événement on sait seulement qu’on a un BaseAuthenticator, mais que dans les appels à la journalisation c’est une de ses classes filles, et que par conséquent les ContentTypes ne concordent pas.

Il faut juste caster l'authenticateur pour avoir le bon content-type, donc references=[authenticator.baseauthenticator_ptr].

> C’est fait dans la branche, mais visiblement le prefetch des références lors de l’affichage du journal se passe mal : https://jenkins.entrouvert.org/job/gitea/job/authentic/job/wip%252F72538-auth-oidc-prompt-none-error/11/console > > Je ne connais pas ce code, je ne sais pas si c’est du au fait que dans le type d’événement on sait seulement qu’on a un BaseAuthenticator, mais que dans les appels à la journalisation c’est une de ses classes filles, et que par conséquent les ContentTypes ne concordent pas. Il faut juste caster l'authenticateur pour avoir le bon content-type, donc `references=[authenticator.baseauthenticator_ptr]`.
pmarillonnet commented 2023-04-19 12:34:56 +02:00
Author
Owner
Copy Link

Ok oui en effet, bien vu. C’était ça et la nécessité d’ajouter l’authentificateur à chaque enregistrement d’un événement de type échec de connexion partout dans le code a2. J’en ai profité pour modifier la signature de la fonction pour ne pas donner de valeur nulle par défaut à l’authentificateur.

Le seul truc qui m’embête, c’est que sur les instances il y a déjà des événements de type échec de connexion, sauf référence vers un authentificateur. Il faut que je corrige le truc qui fait que ça crashe dans le manage lorsqu’on essaie d’afficher un tel événement.

Ok oui en effet, bien vu. C’était ça et la nécessité d’ajouter l’authentificateur à chaque enregistrement d’un événement de type échec de connexion partout dans le code a2. J’en ai profité pour modifier la signature de la fonction pour ne pas donner de valeur nulle par défaut à l’authentificateur. Le seul truc qui m’embête, c’est que sur les instances il y a déjà des événements de type échec de connexion, sauf référence vers un authentificateur. Il faut que je corrige le truc qui fait que ça crashe dans le manage lorsqu’on essaie d’afficher un tel événement.
pmarillonnet commented 2023-04-19 12:47:41 +02:00
Author
Owner
Copy Link

Le seul truc qui m’embête, c’est que sur les instances il y a déjà des événements de type échec de connexion, sauf référence vers un authentificateur. Il faut que je corrige le truc qui fait que ça crashe dans le manage lorsqu’on essaie d’afficher un tel événement.

Et en fait non, mauvaise connaissance de l’interface de gestion des événements et du passage d’objet en référence, je cherchais par erreur à en créer un avec references=[None].

La PR est bonne pour relecture.

> Le seul truc qui m’embête, c’est que sur les instances il y a déjà des événements de type échec de connexion, sauf référence vers un authentificateur. Il faut que je corrige le truc qui fait que ça crashe dans le manage lorsqu’on essaie d’afficher un tel événement. Et en fait non, mauvaise connaissance de l’interface de gestion des événements et du passage d’objet en référence, je cherchais par erreur à en créer un avec `references=[None]`. La PR est bonne pour relecture.
pmarillonnet force-pushed wip/72538-auth-oidc-prompt-none-error from 8d758775a5 to 261c5647e9 2023-04-19 12:25:27 +02:00 Compare
pmarillonnet requested review from bdauvergne 2023-04-19 12:47:48 +02:00
bdauvergne approved these changes 2023-04-19 12:50:08 +02:00
pmarillonnet merged commit 261c5647e9 into main 2023-04-19 12:59:12 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
bdauvergne
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: entrouvert/authentic#38
No description provided.
Delete Branch "wip/72538-auth-oidc-prompt-none-error"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?