Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tech : Correction d'un comportement étrange sur la connexion par email / mot de passe #11156

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

mmagn
Copy link
Contributor

@mmagn mmagn commented Dec 19, 2024

Comportement étrange trouvé avec Sim sur la fonctionnalité qui bloque l'authentication par email/mdp si l'email est sur un domaine dont la connexion doit être gérée exclusivement par AgentConnect (ex @beta.gouv.fr).
L'utilisateur ne devrait pas pouvoir se connecter par email/mdp (bloqué par un redirect dans le before_action sur sessions#create) or c'est le cas (la redirection se passe bien, mais l'utilisateur est quand même connecté).

Après investigation nous avons remarqué que si aucun utilisateur n'est connecté, mais que dans le code on appelle current_user, Devise par Warden va essayer d'authentifier l'utilisateur en utilisant le contexte de la requête en cours.

Cela pose problème chez nous dans le cas d'une requête de connexion POST /users/sign_in car si on appelle dans un before_action la méthode current_user, Devise va connecter l'utilisateur avant de passer dans le code de l'action sessions#create.

Issues : heartcombo/devise#4951 heartcombo/devise#5602

Réponse officielle de Devise

Since the code is out there for almost 10 years we can't know the impact a change like that would have, that's why we rather not change this behavior, even in a major version (we don't want to make it a pain for people to update it).

Cette PR surchage donc current_user pour éviter l'authentification non intentionnelle

@mmagn mmagn changed the title Override current_user by checking if a user is already authenticated … Tech : Correction d'un comportement étrange sur la connexion par email / mot de passe Dec 19, 2024
@mmagn mmagn marked this pull request as ready for review December 19, 2024 11:24
Copy link

codecov bot commented Dec 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.45%. Comparing base (ef4e680) to head (cfb7dcb).
Report is 48 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11156      +/-   ##
==========================================
- Coverage   84.48%   84.45%   -0.04%     
==========================================
  Files        1187     1187              
  Lines       26265    26236      -29     
  Branches     4948     4940       -8     
==========================================
- Hits        22190    22157      -33     
- Misses       4075     4079       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@LeSim LeSim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

incroyable. Ma confiance en devise croit de jour en jour.

@mmagn mmagn added this pull request to the merge queue Dec 19, 2024
Merged via the queue into main with commit 5c45754 Dec 19, 2024
18 checks passed
@mmagn mmagn deleted the fix-current-user-triggers-authentication branch December 19, 2024 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants