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

FranceConnect for users #752

Merged
merged 4 commits into from Feb 17, 2021
Merged

FranceConnect for users #752

merged 4 commits into from Feb 17, 2021

Conversation

adipasquale
Copy link
Contributor

@adipasquale adipasquale commented Jun 25, 2020

1. migration to add FranceConnect fields to users

I'm adding two fields to users:

  • franceconnect_openid_sub : this is a unique identifier for accounts from FC. not used yet but could be useful in the future.
  • created_through : describes how the user was created. possible values: agent_creation user_sign_up franceconnect_sign_up user_relative_creation unknown. I'm relying on a 'bug' in the migration: we were setting invited_by for all users created by agents, regardless of whether they were indeed invited or not. For the other ones we cannot always know if they were created by the user herself or by an agent so I'm using unknown.

2. eject from devise for SuperAdmin omniauth with GH

Devise unfortunately does not support using omniauth with multiple models so we need to eject from devise for SuperAdmins.

I've isolated the 'ejection' from Devise iso-feature with the current GH OAuth apps for SuperAdmins. This shouldn't change anything except that we'll need to change the oauth apps callback urls.

3. Implement FranceConnect omniauth sign in for users

I'm adding the gems + the buttons + the callback handler + the service to upsert users with FC data.

Tested paths

  • signup with FC from rdv tunnel
  • login with FC from rdv tunnel
  • login with FC for existing account created with password
  • reset password from account created with FC
  • login to super admin

Prod migration simulation results

users.created_through
 => {"unknown"=>2346, "user_sign_up"=>6372, "agent_creation"=>52046}

Testing on the review app

there is a list of demo users for the FC demo fournisseur here: https://github.com/france-connect/identity-provider-example/blob/master/database.csv

Tech discussion:

I'm not happy at all to introduce these new gems dependencies, especially since they have very low usage. BUT I think I prefer this than to having to implement OpenID protocol ourselves.

Namely, I'm adding a dependency on omniauth_openid_connect (~62 stars) which itself depends on openid_connect (~300 GH stars)

Local setup

add this to your .env

HOST=http://localhost:5000
FRANCECONNECT_HOST=fcp.integ01.dev-franceconnect.fr
FRANCECONNECT_APP_ID=xxxx
FRANCECONNECT_APP_SECRET=xxxx

you can find these credentials in the FCP interface https://partenaires.franceconnect.gouv.fr/ using the password in nextcloud, or copy them from the scalingo review app env. make sure that the callback url on the recette environment is set to localhost

not done in this PR, notes for later :

  • fetch more data (address, phone number) from a specific "FI" ?
  • for FC-created users, improve message when trying to login/re-register with existing email address
  • should we store extra identity data (birthplace, birthcountry..)?

and maaaaaybe someday extract france_connect strategy to separate gem

@guillett
Copy link
Member

guillett commented Jul 1, 2020

As a first iteration, we can insist on relying on a single token.

I agree with you that extracting to a dedicated gem is optional so far.

@adipasquale adipasquale changed the title ⚙️ FranceConnect PoC 3 with omniauth + openid_connect ⚙️ [users] sign in and sign up with France Connect Jul 1, 2020
@adipasquale adipasquale changed the base branch from master to refactor/sessions-controllers July 1, 2020 16:25
@adipasquale adipasquale marked this pull request as ready for review July 1, 2020 16:41
@adipasquale adipasquale changed the title ⚙️ [users] sign in and sign up with France Connect [users] sign in and sign up with France Connect Jul 1, 2020
Base automatically changed from refactor/sessions-controllers to master July 6, 2020 10:31
@adipasquale
Copy link
Contributor Author

closing because we're still waiting to get token keys from France Connect or something

@adipasquale adipasquale closed this Sep 7, 2020
@yaf yaf deleted the feature/franceconnect-poc-3 branch November 12, 2020 16:43
@adipasquale adipasquale restored the feature/franceconnect-poc-3 branch January 12, 2021 15:25
@adipasquale adipasquale reopened this Jan 12, 2021
@adipasquale adipasquale marked this pull request as draft January 12, 2021 15:25
@adipasquale
Copy link
Contributor Author

back from the dead 🌈

@adipasquale adipasquale mentioned this pull request Jan 13, 2021
@adipasquale adipasquale force-pushed the feature/franceconnect-poc-3 branch 3 times, most recently from 79023d8 to 804a8a9 Compare January 14, 2021 16:06
@adipasquale adipasquale changed the title [users] sign in and sign up with France Connect France Connect for users Jan 14, 2021
@guillett
Copy link
Member

J'ai fait un test exploratoire, peut-être incomplet.
En cas de création de compte, et hors d'un parcours de prise de RDV (ça doit être assez rare en fait) ça serait peut-être pas mal d'arriver sur la page « Vos informations » plutôt que la « home » usager connecté.

En l'écrivant je me dis que c'est une situation qui ne doit pas arriver souvent donc pourquoi s'embêter. Je pose ça quand même là mais c'est pas une très bonne idée. 😅

@adipasquale adipasquale force-pushed the feature/franceconnect-poc-3 branch 2 times, most recently from 798b47a to d0772c8 Compare February 1, 2021 12:57
@adipasquale adipasquale force-pushed the feature/franceconnect-poc-3 branch 4 times, most recently from 9905aa7 to f702dcf Compare February 10, 2021 15:33
@yaf yaf linked an issue Feb 15, 2021 that may be closed by this pull request
@adipasquale adipasquale marked this pull request as ready for review February 16, 2021 07:31
@adipasquale adipasquale changed the title France Connect for users FranceConnect for users Feb 16, 2021
Copy link
Contributor

@yaf yaf 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 fait un ou deux tests de rapprochement, tout semble bien se passer. La déconnexion aussi.

@adipasquale adipasquale merged commit d251cf3 into master Feb 17, 2021
@adipasquale adipasquale deleted the feature/franceconnect-poc-3 branch February 17, 2021 07:00
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.

France Connect
3 participants