Skip to content

Remplacer l'authentification par mot de passe par le code de connexion pour les usagers#6204

Open
Holist wants to merge 31 commits intoproductionfrom
replace_user_password_with_login_code
Open

Remplacer l'authentification par mot de passe par le code de connexion pour les usagers#6204
Holist wants to merge 31 commits intoproductionfrom
replace_user_password_with_login_code

Conversation

@Holist
Copy link
Collaborator

@Holist Holist commented Feb 27, 2026

En lien avec #5481

Suite de #6203

Supprime l'authentification par mot de passe pour les usagers

Les usagers passent désormais exclusivement par le code de connexion par email (déjà en place). Cette PR retire tout le code lié au mot de passe côté usager.

Modules Devise supprimés du modèle User :

:confirmable, :recoverable, :validatable, :database_authenticatable, :registerable

Fichiers supprimés :

  • users/passwords_controller.rb + vues (réinitialisation de mot de passe)
  • users/confirmations_controller.rb (confirmation de compte par email)
  • users/registrations/new.html.slim + Users::RegistrationForm (inscription)
  • Specs correspondantes

Choix d'architecture :

La colonne confirmed_at est remplacée par latest_login_at plus juste sémantiquement (colonne mise en place dans cette pr).

Adaptations :

  • Les routes user ne pouvant plus être générées implicitement par Devise, elles sont déclarées explicitement via devise_scope
  • Les seeds et la factory ne définissent plus de password pour les usagers
  • Correction d'un effet de bord sur les vues agents : le helper générique registration_path(resource_name) de Devise disparaît quand un seul scope a :registerable → remplacé par agent_registration_path

Non traité dans cette PR (prévu séparément) :

  • Suppression des colonnes devises sur la table users

Prochaines étapes :

  • gérer la connexion usager lorsqu'il y a plusieurs comptes usagers avec le même email (email et notification_email)

Base automatically changed from remove_user_invitation_to_register to production March 3, 2026 10:21
@@ -1,112 +1,4 @@
RSpec.describe CustomDeviseMailer, "#domain" do
subject(:sent_email) { described_class.reset_password_instructions(user, "t0k3n") }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tout ça n'avait plus grand chose à faire la et a été déplacé dans spec/models/user_spec.rb

Copy link
Collaborator

Choose a reason for hiding this comment

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

En effet, ça teste User#domain, bien vu !

@Holist Holist marked this pull request as ready for review March 3, 2026 15:30
@Holist Holist self-assigned this Mar 3, 2026
Copy link
Collaborator

@francois-ferrandis francois-ferrandis left a comment

Choose a reason for hiding this comment

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

J'ai tout lu ligne par ligne et je suis tellement heureux de voir tout ce que ça simplifie ! J'ai deux ou trois suggestions qu'on peut discuter, mais sinon ça me paraît bon à partir en prod. 🥹

@@ -1,112 +1,4 @@
RSpec.describe CustomDeviseMailer, "#domain" do
subject(:sent_email) { described_class.reset_password_instructions(user, "t0k3n") }
Copy link
Collaborator

Choose a reason for hiding this comment

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

En effet, ça teste User#domain, bien vu !

Copy link
Collaborator

@aminedhobb aminedhobb left a comment

Choose a reason for hiding this comment

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

Merci @Holist , trop cool de voir tout ce code enlevé 👏 🫶 !!
J'ai juste quelques questions que je t'ai posé ici.

Aussi qu'est-ce qu'il se passe quand des usagers vont cliquer sur les anciens liens d'invitation à se créer un compte après cette PR ?


def self.should_send_sms_to_user?(user:, rdv:, author:)
author != user || !user.confirmed? || rdv.starts_at < 2.days.from_now
author != user || user.confirmed_at.nil? || rdv.starts_at < 2.days.from_now
Copy link
Collaborator

Choose a reason for hiding this comment

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

gros nitpick: personnellement j'aurais garder l'alias de confirmed? à confirmed_at?, c'est plus beau sémantiquement et si on doit rajouter de la logique de confirmation ce serait plus facile

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A ce sujet cette PR de @francois-ferrandis qu'on va merge avant : #6221

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Après discussions avec @francois-ferrandis , on a donc remplacé confirmed? et le champ confirmed_at par already_logged_in? et latest_login_at

if matching_login_code&.usable?
@user = upsert_user
user.confirm
user.update!(confirmed_at: Time.zone.now) if user.confirmed_at.nil?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Je trouve ça peut-être dommage que tous les types de connexions mettent à jour la même colonne confirmed_at de la même façon.
On ne saura pas distinguer les connexions via login code, fc et password

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A ce sujet cette PR de @francois-ferrandis qu'on va merge avant : #6221

Copy link
Collaborator Author

@Holist Holist Mar 4, 2026

Choose a reason for hiding this comment

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

On a remplacé cette ligne par user.update!(latest_login_at: Time.zone.now) dans #6221

Holist added 2 commits March 4, 2026 15:05
…tagouv/rdv-service-public into replace_user_password_with_login_code
@Holist
Copy link
Collaborator Author

Holist commented Mar 4, 2026

Aussi qu'est-ce qu'il se passe quand des usagers vont cliquer sur les anciens liens d'invitation à se créer un compte après cette PR ?

Comme je l'avais précisé dans la description de la PR précédente les anciens liens d'invitations donneront une 404. On peut toujours mettre en place un système de redirection vers le sign_in pour les anciens liens si on estime que ca vaut le coup @francois-ferrandis

@@ -0,0 +1,14 @@
class BackfillLatestLoginAtFromConfirmedAt < ActiveRecord::Migration[7.2]
def up
safety_assured { execute(<<~SQL.squish) }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@francois-ferrandis pour information. Je récupère les infos du confirmed_at qui passe dans ignored_columns

user = User.find_or_initialize_by(pro_connect_openid_sub: callback_client.openid_sub)

user.confirmed_at ||= Time.zone.now # Nécessaire pour que Devise autorise la connexion
user.latest_login_at ||= Time.zone.now
Copy link
Collaborator

Choose a reason for hiding this comment

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

Est-ce que c'est pas plutôt un update write à tous les coups ?

    user.latest_login_at = Time.zone.now

Et d'ailleurs ça pourrait être fusionné avec le update! juste en dessous.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

évidemment !

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@aminedhobb aminedhobb left a comment

Choose a reason for hiding this comment

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

Vu avec @Holist en live, ça m'a l'air top 🚀 !

def create
prepare_create
authorize(@user, policy_class: Agent::UserPolicy)
@user.skip_confirmation_notification!
Copy link
Collaborator

Choose a reason for hiding this comment

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

Quel kiff d'enlever tous ces skip_* 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: 🚀 To deploy

Development

Successfully merging this pull request may close these issues.

3 participants