Connexion: Ajout de ProConnect pour les Prescripteurs et Employeurs by tonial · Pull Request #4080 · gip-inclusion/les-emplois

tonial · 2024-05-14T09:36:10Z

🤔 Pourquoi ?

Inclusion Connect va être remplacé par ProConnect au ~~1er octobre 2024~~ (un jour peut être)
la bascule se fera en renseignant le setting PRO_CONNECT_BASE_URL

Il reste quelques fixme (des liens de doc/aide à ajouter au niveau des boutons de connexion ProConnect) mais tout le reste est normalement bon.

🏝️ Comment tester

Aujourd'hui on n'a qu'un compte ProConnect de test :
le compte ProConnet est user@yopmail.com (mot de passe aussi user@yopmail.com)

les parcours sont

création de compte (il faut vérifier qu'il n'y a plus de compte user@yopmail.com avant via l'admin)
connexion sur compte existant
déconnexion
invitation (avec compte pré-existant ou non)
activation de compte (créer un compte dans l'admin avec le SSO DJANGO et l'email user@yopmail.com

github-actions · 2024-05-14T09:42:42Z

🥁 La recette jetable est prête ! 👉 Je veux tester cette PR !

notion-workspace · 2024-09-09T08:20:48Z

Préparer la bascule à ProConnect

rsebille

Revue partielle mais là j'ai plus de jus.

Remarques globales :

Pas super fan d'avoir des choses nommées IC et de faire IC+PC dedans car ça ne permet pas d'identifier simplement ce qui est spécifique ou partagé
Pratiquement pas relu les tests, un peu trop de bruit et le jeu des 7 différences est trop casse-pied 😁
Je me demande plutôt qu'avoir les deux en // si ça n'aurais pas été plus simple d'avoir un switch global qui indique le SSO par défaut et de gérer ça avec les variables existantes ou des templatetags/filter si on veux pas trop se faire chier. Ça aurais permis de lancer tout les tests avec l'un ou l'autre jusqu'au jour J, et la majorité du taf aurais été dans le renommage/refacto préalable plutôt que de dupliquer du code et de créer des branches de code qu'il faudra par la suite nettoyer.

rsebille · 2024-09-09T15:36:55Z

Pas super convaincu de l'indirection causée par la recopie des settings dans une constante.

C'est comme ça avec tous les SSO, et j'avoue que je trouve ça plus simple de laisser en place.
C'était parti d'une logique de victor de ne pas mettre dans les settings des trucs qui ne l'étaient pas, et ça reste dicsutable.
On pourrait (dans une autre PR) décider de tout remettre dans les settings, quitte à avoir une sorte de meta setting PRO_CONNECT qui contienne des attributs calculés ou juste configurés pour tout regrouper proprement.
Un peu comme pour la logique des settings des libs blue à l'époque.

Je suis plutôt team "séparer les constantes du code des paramètres de configuration" ;).

Et pour préciser mon commentaire, dans le fichier actuel ce n'est pas les URL qui me dérange, elles ont leur place ici plutôt que dans les settings vu qu'elle n'"active" rien ni ne sont des secrets ou configurable.
Par contre le schéma VAR = settings.VAR n'a strictement aucun intérêt, et je suppose que tout ça sert pour le reload_module() parce qu'on importe les symboles directement plutôt que les modules donc plus compliqué de mocké.

Euh, c'est possible 🤔
Je ne me rappelle plus pourquoi on avait fait comme ça.
Il me semble que ce n'est pas nécessaire parce que le reload_module est de toute façon lancé pour que le override_settings fasse effet.
Comme en plus les endpoints dépendent des settings, ça rend la frontière un peu floue et je crois qu'au final c'était une manière de tout avoir au même endroit.

rsebille · 2024-09-09T15:45:38Z

+# https://github.com/numerique-gouv/agentconnect-documentation/blob/main/doc_fs/donnees_fournies.md
+PRO_CONNECT_SCOPES = "openid email given_name usual_name"


Les scopes email given_name usual_name sont vraiment nécessaires et utiles ? Ou ça tombe en marche ? Ou alors c'est la doc qui n'est pas claire ?
Car de ce que je comprend de la doc d'PC ces informations sont obligatoirement renvoyées, et elles ne sont pas listées dans la doc OpenID pointée 😵.

Normalement il ne sont plus censé être nécessaires, mais sans, ça plante.... 🤷
j'ajoute un commentaire.

C'était parce que les scopes n'étaient pas cochés coté ProConnect.
normalement maintenant c'est bon.

rsebille · 2024-09-09T16:16:34Z

+    def asdict(self):
+        return dataclasses.asdict(self)


Haha, on se retrouve à devoir faire plus de code pour adhérer à la convention que d'utiliser le helper directement 😀. Même .bind_to_request()ne l'utilise pas 😅.

C'est utilisé dans OIDConnectUserData.create_or_update_user

Si tu parles d'ici alors ce n'est pas utilisé :).
https://github.com/gip-inclusion/les-emplois/blob/e69ba85ea3631a1d38d5c55083d64e6e6bdd324e/itou/openid_connect/models.py#L116C38-L116C44

A priori les seules utilisations sont celles-ci :

❯ rg -tpy '\.asdict\(\)' tests/openid_connect/pro_connect/tests.py 678: pc_session_dict = pc_session.asdict() itou/openid_connect/inclusion_connect/views.py 141: data = _generate_inclusion_params_from_session(ic_data.asdict()) tests/openid_connect/inclusion_connect/tests.py 722: ic_session_dict = ic_session.asdict() itou/openid_connect/pro_connect/views.py 136: data = _generate_pro_params_from_session(pc_data.asdict())

Alors que l'utilisation direct du module (rg -tpy 'dataclasses\.asdict\(') en fait ressentir le quadruple, dont 4 occurrences sont en fait pour la convention/interface.

Donc je dirais que c'est assez peu utilisé et qu'en plus ça oblige à maintenir une interface dans chaque classe enfant pour un un gain nul.

Ah, mince j'ai confondu d'endroit.
C'est utilisé ligne 136
Mais en effet, on se retrouve à définir une fonction qui n'est utilisée qu'à un endroit, c'est pas super utile. Je vais remplacer.

rsebille · 2024-09-09T16:55:32Z

+@respx.mock
+@override_pro_connect_settings
+def test_prescriber_using_django_has_to_activate_pro_connect_account(client):
+    user = PrescriberFactory(identity_provider=IdentityProvider.DJANGO, email=OIDC_USERINFO["email"])
+    client.force_login(user)
+    url = reverse("dashboard:index")
+    response = client.get(url, follow=True)
+    activate_ic_account_url = reverse("dashboard:activate_ic_account")
+    assertRedirects(response, activate_ic_account_url)
+    params = {
+        "user_kind": UserKind.PRESCRIBER,
+        "previous_url": activate_ic_account_url,
+        "user_email": user.email,
+    }
+    url = escape(f"{reverse('pro_connect:authorize')}?{urlencode(params)}")
+    assertContains(response, url + '"')
+    response = mock_oauth_dance(
+        client,
+        UserKind.PRESCRIBER,
+        previous_url=activate_ic_account_url,
+    )
+    user.refresh_from_db()
+    assert user.identity_provider == IdentityProvider.PRO_CONNECT
+
+
+@respx.mock
+@override_pro_connect_settings
+def test_employer_using_django_has_to_activate_pro_connect_account(client):
+    user = EmployerFactory(with_company=True, identity_provider=IdentityProvider.DJANGO, email=OIDC_USERINFO["email"])
+    client.force_login(user)
+    url = reverse("dashboard:index")
+    response = client.get(url, follow=True)
+    activate_ic_account_url = reverse("dashboard:activate_ic_account")
+    assertRedirects(response, activate_ic_account_url)
+    params = {
+        "user_kind": UserKind.EMPLOYER,
+        "previous_url": activate_ic_account_url,
+        "user_email": user.email,
+    }
+    url = escape(f"{reverse('pro_connect:authorize')}?{urlencode(params)}")
+    assertContains(response, url + '"')
+    response = mock_oauth_dance(
+        client,
+        UserKind.EMPLOYER,
+        previous_url=activate_ic_account_url,
+    )
+    user.refresh_from_db()
+    assert user.identity_provider == IdentityProvider.PRO_CONNECT
+
+


Ces 2 tests ne devraient-il pas être dans leur propre fichier plutôt que celui d'IC ? En plus ça éviterais les renommages.
Et vu de la tête du fichier et que PC est globalement un drop-in de IC, je pense qu'une fixture paramétrique permettrais de tester les 2 fournisseurs sans avoir à dupliquer tout les tests.

Je dois avouer que j'ai fait au plus simple pour la suppression après coup.
J'aurais pu faire 2 vues différents, utiliser un switch encore plus élégant, un paramétrise des tests, etc, sauf que ça nécessite encore plus de travail : les InclusionConnectTestCase et ProConnectTestCase ne sont pas compatibles entre eux donc il faudrait les appliquer de manière paramétrique également sur les tests.

Pour les TestCase je suis d'accord que c'est pas le plus pratique, pour ça que j'ai pas fait la remarque ;).
Mais vu que ce fichier est en full pytest, que justement c'est les mêmes vues, et qu'on duplique des tests (mais pas tous) ça m'a fait penser à une fixture paramétrique histoire de tester les deux chemins de la même manière, mais si t'es serein alors y a pas de problème :).

J'ai mis dans 2 fichiers séparés pour pouvoir supprimer directement celui d'ic à la fin et comparer les tests facilement avec un vim -d en attendant :)

tonial · 2024-09-10T05:12:22Z

Globalement je suis d'accord avec toi : il y aurait moyen de faire un truc super élégant qui permette de passer à la volée d'un SSO à l'autre dans les canaux de connexion, mais je pense que ça prendrait une grosse semaine de travail pour pas grand chose.

Résultat, c'est un peu un jeu des 7 différences avec tous ces tests dupliqués mais je n'ai pas réussi à faire mieux facilement, et je pense que ça ne vaut pas le coup d'y passer trop de temps.
J'aurais bien juste remplacé IC par PC, mais c'est plus risqué en cas de problème à la bascule donc on m'a convaincu de ne pas le faire :D

francoisfreitag · 2024-09-10T10:26:55Z

J’ai commencé par relire « inclusion_connect: Remove unused channel ». C’est possible de le sortir dans une autre PR pour éviter de polluer la discussion sur la migration à PC ?

Je pense qu’un peu de contexte dans le commit (pourquoi ça avait été ajouté/pourquoi ce n’est pas utilisé) serait très utile.

tonial · 2024-09-10T11:24:10Z

C'est fait, j'ai sorti dans une PR séparée

francoisfreitag

Super, très pratique d’avoir dupliqué les classes de tests juste en dessous, on voit bien ce qui change pour PC.
C’est chouette que ProConnect accepte le hint pour utiliser NEPTUNE.

J’ai vu un changement mineur, qui auraient été mieux à part (changer le niveau de logs de info vers error pour le message "Organization with SAFIR {safir} does not exist. Unable to add user {user.email}."). Là, il est juste perdu dans le flot.

Il reste quelques FIXME pour quand on aura les URLs, je présume.

tonial · 2024-09-11T05:05:33Z

Merci @rsebille et @francoisfreitag pour la relecture, j'ai pris en compte vos remarques (ou ajouté un commentaire en réponse) dans 2 commits de fixups séparé (un par personne)

rsebille · 2024-09-11T08:23:44Z

Je suis plutôt team "séparer les constantes du code des paramètres de configuration" ;).

Et pour préciser mon commentaire, dans le fichier actuel ce n'est pas les URL qui me dérange, elles ont leur place ici plutôt que dans les settings vu qu'elle n'"active" rien ni ne sont des secrets ou configurable.
Par contre le schéma VAR = settings.VAR n'a strictement aucun intérêt, et je suppose que tout ça sert pour le reload_module() parce qu'on importe les symboles directement plutôt que les modules donc plus compliqué de mocké.

rsebille · 2024-09-11T09:01:42Z

    identity_provider: IdentityProvider
    kind: str
-    login_allowed_user_kinds = [UserKind]
+    login_allowed_user_kinds: ClassVar[list[UserKind]]


Si login_allowed_user_kinds est une ClassVar alors identity_provider et kind le sont très probablement aussi ;).

C'est la seule façon que j'ai trouvé pour réussi à filer list[UserKind] :
Avec login_allowed_user_kinds: list[UserKind] = [UserKind.PRESCRIBER, UserKind.EMPLOYER]
Ça done on obtient une ValueError: mutable default <class 'list'> for field login_allowed_user_kinds is not allowed: use default_factory

Oui c'est ce que j'ai vu, au début je me suis dit qu'on avais pas spécifiquement besoin d'une list et qu'on pourrais avoir une structure immutable mais le ClassVar est tout aussi vrai vu qu'on veux la même valeur pour toutes les instances de classe, ce qui m'a fait penser que les 2 autres devraient aussi en être.
En soi c'est pas bloquant mais c'est plus pour éviter les Oo quand on arrive dessus et que un seul l'est alors que 3 sont utilisés de la même manière.

Ah, je comprends mieux.
On ne peut pas le mettre sur les 2 autres parce que sinon, on ne les récupère pas dans user_data_dict = dataclasses.asdict(self) et on il manque des champs pour créer l'utilisateur

Ah bah oui 🤦 👍

Pour info : j'ai extrait cette partie pour la mettre dans cette PR : #4822

rsebille · 2024-09-11T09:14:10Z

+    def asdict(self):
+        return dataclasses.asdict(self)


Si tu parles d'ici alors ce n'est pas utilisé :).
https://github.com/gip-inclusion/les-emplois/blob/e69ba85ea3631a1d38d5c55083d64e6e6bdd324e/itou/openid_connect/models.py#L116C38-L116C44

A priori les seules utilisations sont celles-ci :

❯ rg -tpy '\.asdict\(\)' tests/openid_connect/pro_connect/tests.py 678: pc_session_dict = pc_session.asdict() itou/openid_connect/inclusion_connect/views.py 141: data = _generate_inclusion_params_from_session(ic_data.asdict()) tests/openid_connect/inclusion_connect/tests.py 722: ic_session_dict = ic_session.asdict() itou/openid_connect/pro_connect/views.py 136: data = _generate_pro_params_from_session(pc_data.asdict())

Alors que l'utilisation direct du module (rg -tpy 'dataclasses\.asdict\(') en fait ressentir le quadruple, dont 4 occurrences sont en fait pour la convention/interface.

Donc je dirais que c'est assez peu utilisé et qu'en plus ça oblige à maintenir une interface dans chaque classe enfant pour un un gain nul.

rsebille · 2024-09-11T09:30:14Z

+@respx.mock
+@override_pro_connect_settings
+def test_prescriber_using_django_has_to_activate_pro_connect_account(client):
+    user = PrescriberFactory(identity_provider=IdentityProvider.DJANGO, email=OIDC_USERINFO["email"])
+    client.force_login(user)
+    url = reverse("dashboard:index")
+    response = client.get(url, follow=True)
+    activate_ic_account_url = reverse("dashboard:activate_ic_account")
+    assertRedirects(response, activate_ic_account_url)
+    params = {
+        "user_kind": UserKind.PRESCRIBER,
+        "previous_url": activate_ic_account_url,
+        "user_email": user.email,
+    }
+    url = escape(f"{reverse('pro_connect:authorize')}?{urlencode(params)}")
+    assertContains(response, url + '"')
+    response = mock_oauth_dance(
+        client,
+        UserKind.PRESCRIBER,
+        previous_url=activate_ic_account_url,
+    )
+    user.refresh_from_db()
+    assert user.identity_provider == IdentityProvider.PRO_CONNECT
+
+
+@respx.mock
+@override_pro_connect_settings
+def test_employer_using_django_has_to_activate_pro_connect_account(client):
+    user = EmployerFactory(with_company=True, identity_provider=IdentityProvider.DJANGO, email=OIDC_USERINFO["email"])
+    client.force_login(user)
+    url = reverse("dashboard:index")
+    response = client.get(url, follow=True)
+    activate_ic_account_url = reverse("dashboard:activate_ic_account")
+    assertRedirects(response, activate_ic_account_url)
+    params = {
+        "user_kind": UserKind.EMPLOYER,
+        "previous_url": activate_ic_account_url,
+        "user_email": user.email,
+    }
+    url = escape(f"{reverse('pro_connect:authorize')}?{urlencode(params)}")
+    assertContains(response, url + '"')
+    response = mock_oauth_dance(
+        client,
+        UserKind.EMPLOYER,
+        previous_url=activate_ic_account_url,
+    )
+    user.refresh_from_db()
+    assert user.identity_provider == IdentityProvider.PRO_CONNECT
+
+


Pour les TestCase je suis d'accord que c'est pas le plus pratique, pour ça que j'ai pas fait la remarque ;).
Mais vu que ce fichier est en full pytest, que justement c'est les mêmes vues, et qu'on duplique des tests (mais pas tous) ça m'a fait penser à une fixture paramétrique histoire de tester les deux chemins de la même manière, mais si t'es serein alors y a pas de problème :).

rsebille · 2024-09-12T07:38:27Z

    identity_provider: IdentityProvider
    kind: str
-    login_allowed_user_kinds = [UserKind]
+    login_allowed_user_kinds: ClassVar[list[UserKind]]


Oui c'est ce que j'ai vu, au début je me suis dit qu'on avais pas spécifiquement besoin d'une list et qu'on pourrais avoir une structure immutable mais le ClassVar est tout aussi vrai vu qu'on veux la même valeur pour toutes les instances de classe, ce qui m'a fait penser que les 2 autres devraient aussi en être.
En soi c'est pas bloquant mais c'est plus pour éviter les Oo quand on arrive dessus et que un seul l'est alors que 3 sont utilisés de la même manière.

github-actions · 2024-09-12T09:24:14Z

🥁 La recette jetable est prête ! 👉 Je veux tester cette PR !

tonial added 1-recette-jetable [Payé à l’heure] Crée une recette jetable sur CC ajouté Ajouté dans le changelog. labels May 14, 2024

tonial self-assigned this May 14, 2024

tonial marked this pull request as draft May 14, 2024 09:36

tonial force-pushed the alaurent/add_pro_connect branch from ab0a277 to e6bb867 Compare May 16, 2024 08:41

github-advanced-security AI found potential problems May 16, 2024

View reviewed changes

Comment thread itou/openid_connect/pro_connect/views.py Fixed

Comment thread itou/openid_connect/pro_connect/views.py Fixed

Comment thread itou/openid_connect/pro_connect/views.py Fixed

tonial force-pushed the alaurent/add_pro_connect branch 12 times, most recently from 822f01f to 14ba96f Compare May 22, 2024 06:17

tonial force-pushed the alaurent/add_pro_connect branch 3 times, most recently from 461731a to d0d1396 Compare May 24, 2024 08:05

tonial force-pushed the alaurent/add_pro_connect branch 3 times, most recently from b9d84a5 to 559aef0 Compare June 6, 2024 13:50

tonial force-pushed the alaurent/add_pro_connect branch 5 times, most recently from c5bffd3 to 6053bfb Compare June 18, 2024 03:54

tonial requested review from celine-m-s, francoisfreitag and rsebille September 7, 2024 04:37

tonial changed the title ~~Connexion: Ajout de ProConnect pour les Prescripteurs et Employeurs~~ Connexion: Ajout de ProConnect pour les Prescripteurs et Employeurs [GEN-2013] Sep 9, 2024

rsebille reviewed Sep 9, 2024

View reviewed changes

tonial force-pushed the alaurent/add_pro_connect branch 5 times, most recently from 31d734d to 9909ebb Compare September 10, 2024 08:01

tonial requested a review from rsebille September 10, 2024 10:39

tonial force-pushed the alaurent/add_pro_connect branch from 9909ebb to 6186311 Compare September 10, 2024 10:45

francoisfreitag approved these changes Sep 10, 2024

View reviewed changes

tonial force-pushed the alaurent/add_pro_connect branch from 6186311 to 7b2b414 Compare September 11, 2024 05:04

tonial requested a review from francoisfreitag September 11, 2024 05:04

tonial force-pushed the alaurent/add_pro_connect branch from 7b2b414 to e69ba85 Compare September 11, 2024 05:18

rsebille reviewed Sep 11, 2024

View reviewed changes

tonial force-pushed the alaurent/add_pro_connect branch from e69ba85 to 55fd4c1 Compare September 12, 2024 04:11

rsebille approved these changes Sep 12, 2024

View reviewed changes

tonial force-pushed the alaurent/add_pro_connect branch 2 times, most recently from 0a95fe5 to 016dbb1 Compare September 12, 2024 08:15

tonial mentioned this pull request Sep 26, 2024

Connexion: Ajout d'un vérification du paramètre next_url lors d'une connexion via un SSO #4824

Merged

1 task

xavfernandez reviewed Oct 1, 2024

View reviewed changes

Comment thread itou/templates/dashboard/edit_user_info.html Outdated

pro_connect: Add a new SSO

7e283db

		# https://github.com/numerique-gouv/agentconnect-documentation/blob/main/doc_fs/donnees_fournies.md
		PRO_CONNECT_SCOPES = "openid email given_name usual_name"

Conversation

tonial commented May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🤔 Pourquoi ?

🏝️ Comment tester

Uh oh!

github-actions Bot commented May 14, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

notion-workspace Bot commented Sep 9, 2024

Uh oh!

rsebille left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonial Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonial Sep 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonial Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonial Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonial commented Sep 10, 2024

Uh oh!

francoisfreitag commented Sep 10, 2024

Uh oh!

tonial commented Sep 10, 2024

Uh oh!

francoisfreitag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tonial commented Sep 11, 2024

Uh oh!

tonial commented May 14, 2024 •

edited

Loading

tonial Sep 10, 2024 •

edited

Loading

tonial Sep 12, 2024 •

edited

Loading

tonial Sep 10, 2024 •

edited

Loading

tonial Sep 10, 2024 •

edited

Loading

tonial Sep 12, 2024 •

edited

Loading