vgrange/peamu #126

dejafait · 2020-04-09T14:31:13Z

TODO Vermeer

matomo post MEP

La code review PE Connect est ouverte, merci de la faire commit par commit svp. Il y a 3 petits et 1 gros commit.

Vous pouvez directement tester en recette jetable https://review-vgrange-peamu.cleverapps.io/

Commentaire du principal commit dupliqué ci-dessous:

PE Connect aka PEAMU is based on OpenID Connect (OIDC).
Note that OpenID Connect (OIDC) is *not* the same as OpenID.

PEAMU acronym is used as it is less ambiguous than PEAM, which does not
explicitely state if we are talking about PEAM Users of PEAM Enterprise.

This code is heavily inspired by official Google provider, also using OIDC:
https://github.com/pennersr/django-allauth/tree/master/allauth/socialaccount/providers/google

PEAMU redirect_uri works with port 8080 but not 8000 nor 5000,
and works only with `localhost` and not `0.0.0.0`.
These new constraints were implemented by PEAMU during March 2020.
Thus we have to use localhost:8080 for our local dev
instead of the former 0.0.0.0:8000 sorry! ¯\_(ツ)_/¯

Disable annoying PIN code in local dev debugger.

Refactor frontend login page into 3 variants for our 3 user roles.
Job seeker can PE Connect either from login page or from signup page.

More pages have been implemented:
1) login_cancelled.html which happens when user clicks "Cancel" on the
"Mire de connexion PE".
2) signup.html used when PEAMU user account is not valid yet.
See explanation in the template for more information.
3) authentication_error.html which is never supposed to be visible.

8 extra API PE Connect are prepared but not used yet,
as our 8 contracts have not been validated yet.

Screenshots :

edge cases:

ikarius · 2020-04-15T14:47:49Z

Ça à l'air bien cool.
Une remarque, si on merge la PR 111 (new prescripteurs) avant, le path pour l'inscription d'un nouveau prescripteur ne sera plus signup/prescriber mais signup/select_prescriber_type

github-actions · 2020-04-22T12:48:22Z

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

github-actions · 2020-04-22T13:11:00Z

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

celine-m-s · 2020-05-18T15:14:15Z

itou/templates/socialaccount/signup.html

-	<a href="https://www.pole-emploi.fr">
-		<button class="btn btn-primary">
-            {% trans "S'identifier chez Pôle emploi pour valider son compte" %}
+    {% trans "Veuillez retrouver l'email &laquo; <i>Confirmation de votre adresse électronique</i> &raquo; que Pôle emploi vous a envoyé le jour de la création de votre compte et y cliquer sur le lien &laquo; <i>Je confirme mon adresse électronique</i> &raquo;." %}


Ici tu peux utiliser un {% blocktrans %} si tu veux éviter les «.

kemar

Presque LGTM mais j'aimerais comprendre certains changements de ports. Le reste est minime et ne pose pas de problème à court terme, à implémenter si tu le juges nécessaire. Chouette boulot !

kemar · 2020-05-26T08:56:21Z

config/settings/base.py

 # https://www.emploi-store-dev.fr/portail-developpeur/catalogueapi
+# Note that the `EMPLOI_STORE` naming below is incorrect,
+# as the Emploi Store is a totally different beast than the ESD.


Pour moi ce commentaire est source de confusion.

Si le nom te semble incorrect alors il faut : soit le changer, soit accepter de vivre avec :)

Ouais ça me faisait un peu peur de le changer car il fallait aller triturer dans la conf CC. Mais maintenant que je connais mieux je vais le faire.

C'est un peu tricky tout de même pour changer sans coupure. Dans la conf CC il faut dupliquer les variables, puis MEP, puis enlever les anciennes variables. Pour staging et pour prod.

Will do.

Je vais en profiter pour remplacer "Emploi Store Dev" par "ESD" partout. Shorter is nicer.

kemar · 2020-05-26T09:05:17Z

config/settings/base.py

 API_EMPLOI_STORE_KEY = os.environ["API_EMPLOI_STORE_KEY"]
 API_EMPLOI_STORE_SECRET = os.environ["API_EMPLOI_STORE_SECRET"]
 API_EMPLOI_STORE_AUTH_BASE_URL = "https://entreprise.pole-emploi.fr"
 API_EMPLOI_STORE_BASE_URL = "https://api.emploi-store.fr/partenaire"

+# PE Connect aka PEAMU - technically one of ESD's APIs.


En fait je vois PEAMU écrit partout dans la base de code mais je ne sais pas ce que ça signifie.

Perso je connais PEAM pour "Pôle Emploi Access Management" tel que mentionné dans la doc.

Je suppose que le U vient pour "User" ? Ça serait pas mal de le mentionner une fois en commentaire dans les settings.

Nouveau commentaire :

# PE Connect aka PEAMU - technically one of ESD's APIs. # PEAM stands for Pôle Emploi Access Management. # Technically there are two PEAM distinct systems: # - PEAM "Entreprise", PEAM-E or PEAME for short. # - PEAM "Utilisateur", PEAM-U or PEAMU for short. # To avoid confusion between the two when contacting ESD support, # we get the habit to always explicitely state that we are using PEAM*U*.

Pour mémoire j'ai pris cette habitude suite à une grosse galère avec le support ESD pour un souci PE Connect LBB. Après m'avoir baladé d'interlocuteur en interlocuteur, je me suis pris un "Ah bon c'est pas PEAM Entreprise que vous utilisez?". Never again. PEAMU.

kemar · 2020-05-26T09:11:57Z

docker-compose-dev.yml

@@ -34,7 +34,7 @@ services:
      - .:/app
    restart: always
    ports:
-      - "${DJANGO_PORT_ON_DOCKER_HOST:-8000}:8000"
+      - "${DJANGO_PORT_ON_DOCKER_HOST:-8080}:8080"


Je ne comprends pas cette modif et du coup j'ai l'impression que tu n'as pas bien compris non plus.

J'explique : la première valeur est configurable et opère sur la machine hôte (ta machine de dev) via une variable d'environnement à la convenance du développeur avec DJANGO_PORT_ON_DOCKER_HOST. La seconde valeur n'a pas besoin d'être modifiée car elle concerne le port à l'intérieur du conteneur Docker.

Est-ce que c'était une incompréhension ou il y a une vraie raison pour ce changement ?

C'est pas plus simple de garder le même numéro de port pour les deux? Sinon on va se mélanger entre 8000 et 8080. Avant PEAMU c'était le même port 8000 pour les deux, on se posait pas la question.

Alors laisse 8000 car c'est le port par défaut.

Si tu as besoin d'un 8080 sur ta machine alors c'est un réglage spécifique pour toi et tu dois utiliser le mapping de ports grâce à DJANGO_PORT_ON_DOCKER_HOST.

Ce n'est pas un réglage/tuning perso, c'est nécéssaire pour faire fonctionner PEAMU en local dev, qui plante pour une redirect_uri en localhost 8000 mais passe avec un localhost 8080. Me demande pas pourquoi, c'est une maj cryptique de sécu côté PEAMU qui date de mars 2020 et qui a été bien pénible à investiguer et solutionner.

Du coup en forcant et passant tout le monde à 8080, j'évite une galère monumentale aux autres devs qui veulent faire tourner PEAMU en local.

Ah ok alors je comprends qu'il y a un motif légitime. C'est bizarre que ça te force aussi à modifier le port à l'intérieur du Docker cela dit… Bref, compris !

Je ne suis pas obligé de modifier le port à l'intérieur du docker, j'aurai pu le laisser à 8000. Mais imagine que tu es un nouveau dev qui arrive sur le projet, tu vas être "wtf pourquoi c'est deux ports différents à l'extérieur et à l'intérieur du docker, pourquoi pas juste 8080 partout?".

Oui ok j'ai compris mais je peux pas t'expliquer Docker plus que ce que j'ai déjà fait. S'il y a un WTF c'est juste que tu n'as pas compris ton archi de dev !

Mmmm apparemment il n'y a que moi que ça pertube d'utiliser deux numéros de port différents là où on pourrait utiliser le même pour plus de simplicité, je rollbacke donc le port interne du container à la valeur par défaut de 8000. Garder la valeur par défaut là où c'est possible est plus important que mon pinaillage.

kemar · 2020-05-26T09:26:51Z

docker/dev/django/entrypoint.sh

+#
+# Disable for you the annoying PIN security prompt on the web
+# debugger. For local dev only of course!
+django-admin runserver_plus 0.0.0.0:8080 --nopin


Attention car là tu es dans ton Docker. Donc pour parler au monde extérieur tu dois bien utiliser 0.0.0.0 pour écouter sur toutes les interfaces réseau disponibles. En laissant 127.0.0.1 ou localhost tu te cantonnes à communiquer avec le même hôte, donc ici ton conteneur Docker.

Idem, pas besoin de ce changement de port à l'intérieur du conteneur Docker qui ne concerne pas le monde extérieur.

Ok. C'est juste dommage de pas avoir trouvé une solution pour que l'URL ici :

soit la bonne.

En fait l'URL est bonne puisque c'est l'IP 0.0.0.0 du conteneur Docker qui est exposée. Il faut juste comprendre que c'est celle du Docker et que tu l'attaques depuis l'hôte via un mapping de port.

kemar · 2020-05-26T09:54:59Z

itou/allauth/peamu/client.py

+        if self.access_token_method == "GET":
+            params = data
+            data = None
+        # TODO: Proper exception handling


Tu peux mettre un resp.raise_for_status() ici peut-être ?

Bof. L'idée en surchargeant get_access_token est juste d'injecter le strict nécéssaire params = {"realm": "/individu"} et de ne surtout pas toucher au code de la dépendance.

Je dis ça en rapport au commentaire :

TODO: Proper exception handling

Je me suis peut-être mal exprimé. Ce commentaire vient de la dépendance : https://github.com/pennersr/django-allauth/blob/6a6d3c618ab018234dde8701173093274710ee0a/allauth/socialaccount/providers/oauth2/client.py#L65

Ta suggestion a plutôt sa place comme une PR sur django-allauth non?

Ah compris, du coup est-ce qu'il faut vraiment conserver verbatim le code de la dépendance quitte à créer la confusion à la lecture ?

Je vais juste virer le commentaire confusant, ça mange pas de pain.

kemar · 2020-05-26T09:57:09Z

README.md

@@ -12,7 +12,7 @@

 Vous pouvez personnaliser la configuration Compose en créant [un fichier `.env`](https://docs.docker.com/compose/env-file/) au même niveau que le fichier `README.md`, puis y configurer les variables d'environnement suivantes :

-    DJANGO_PORT_ON_DOCKER_HOST=8000
+    DJANGO_PORT_ON_DOCKER_HOST=8080


Ok, pas de problème avec la modification de ce port car il concerne un port sur la machine de dev et pas dans le conteneur Docker.

kemar · 2020-05-26T12:47:23Z

itou/users/models.py

@@ -119,6 +119,16 @@ def get_eligibility_diagnosis(self):
            "author", "author_siae", "author_prescriber_organization"
        ).latest("-created_at")

+    @property
+    def is_peamu(self):
+        return self.socialaccount_set.exists()


Ici la fonction ne fait pas ce qu'elle annonce, tu devrais pouvoir filtrer sur provider ou quelque chose comme ça pour être certain de taper dans du PE Connect et ne pas remonter autre chose si un jour on rajoute Facebook connect ou autre.

Astuce bonus : tu peux utiliser cached_property.

kemar · 2020-05-26T12:48:01Z

itou/users/models.py

+    def peamu_id_token(self):
+        if not self.is_peamu:
+            return None
+        return self.socialaccount_set.get().extra_data["id_token"]


Idem pour le filtre PE Connect.

PE Connect aka PEAMU is based on OpenID Connect (OIDC). Note that OpenID Connect (OIDC) is *not* the same as OpenID. PEAMU acronym is used as it is less ambiguous than PEAM, which does not explicitely state if we are talking about PEAM Users of PEAM Enterprise. This code is heavily inspired by official Google provider, also using OIDC: https://github.com/pennersr/django-allauth/tree/master/allauth/socialaccount/providers/google PEAMU redirect_uri works with port 8080 but not 8000 nor 5000, and works only with `localhost` and not `0.0.0.0`. These new constraints were implemented by PEAMU during March 2020. Thus we have to use localhost:8080 for our local dev instead of the former 0.0.0.0:8000 sorry! ¯\_(ツ)_/¯ Disable annoying PIN code in local dev debugger. Refactor frontend login page into 3 variants for our 3 user roles. Job seeker can PE Connect either from login page or from signup page. More pages have been implemented: 1) login_cancelled.html which happens when user clicks "Cancel" on the "Mire de connexion PE". 2) signup.html used when PEAMU user account is not valid yet. See explanation in the template for more information. 3) authentication_error.html which is never supposed to be visible. 8 extra API PE Connect are prepared but not used yet, as our 8 contracts have not been validated yet.

And clarify the local dev URL in the README

github-actions · 2020-05-27T12:34:10Z

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

We use 8080 since PR #126, because of PEAMU. But we can now use PEAMU with the port 8000, so there's no need to use a different port from django's default port. Also : PEAMU does not allow localhost as base url for the callback uri, but 127.0.0.1 is fine. Therefore I changed the dev FQDN to allow us to use PEAMU without having to update the dev settings.

dejafait force-pushed the vgrange/peamu branch 4 times, most recently from dc1208f to 8bd6546 Compare April 10, 2020 16:05

dejafait added review-app and removed review-app labels Apr 13, 2020

dejafait force-pushed the vgrange/peamu branch 2 times, most recently from c4bb5f7 to 94c885e Compare April 15, 2020 13:09

dejafait added review-app and removed review-app labels Apr 15, 2020

dejafait force-pushed the vgrange/peamu branch from b4e14f9 to f993f4a Compare April 22, 2020 12:20

dejafait added recette-jetable and removed review-app labels Apr 22, 2020

dejafait added recette-jetable and removed recette-jetable labels Apr 22, 2020

dejafait force-pushed the vgrange/peamu branch 8 times, most recently from a106a36 to c6e8a78 Compare April 29, 2020 12:31

dejafait removed the recette-jetable label Apr 29, 2020

dejafait force-pushed the vgrange/peamu branch from 42c0f24 to 00bd137 Compare April 29, 2020 12:54

dejafait added the recette-jetable label Apr 29, 2020

dejafait force-pushed the vgrange/peamu branch 3 times, most recently from 4632629 to 96bd03b Compare May 15, 2020 12:09

celine-m-s self-requested a review May 18, 2020 15:11

celine-m-s approved these changes May 18, 2020

View reviewed changes

celine-m-s reviewed May 18, 2020

View reviewed changes

kemar suggested changes May 26, 2020

View reviewed changes

dejafait added 9 commits May 26, 2020 17:27

Cosmetic reindentations

ac72b7a

Fix confusing module name: ESD != PE Connect

1ea0ec5

Add matomo custom variables for all account types

3bc3bfa

s/is_peamu_user/is_peamu/

d8a84d1

Move peamu in a new allauth folder

4db8954

Fix instructions about PE account verification

26bc8a3

Show user.is_peamu in admin

7aa010d

Make matomo custom variables dynamic

3f46157

dejafait force-pushed the vgrange/peamu branch from 229bf08 to 3f46157 Compare May 26, 2020 14:27

dejafait added 4 commits May 26, 2020 19:00

Implement kemar's suggestions.

828a12f

s/Emploi Store/ESD/

4ea0d45

Keep using default 8000 port inside the container

efb9ed1

And clarify the local dev URL in the README

Add link for job seekers without email

6d90746

dejafait added recette-jetable and removed recette-jetable labels May 27, 2020

dejafait merged commit e8555c9 into master May 27, 2020

dejafait deleted the vgrange/peamu branch May 27, 2020 12:54

tonial mentioned this pull request Sep 2, 2022

config: Update default FQDN #1489

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vgrange/peamu #126

vgrange/peamu #126

dejafait commented Apr 9, 2020 •

edited

ikarius commented Apr 15, 2020

github-actions bot commented Apr 22, 2020

github-actions bot commented Apr 22, 2020

celine-m-s May 18, 2020

kemar left a comment

kemar May 26, 2020

dejafait May 26, 2020 •

edited

dejafait May 26, 2020

kemar May 26, 2020

dejafait May 26, 2020

dejafait May 26, 2020

kemar May 26, 2020

dejafait May 26, 2020 •

edited

kemar May 26, 2020

dejafait May 26, 2020 •

edited

kemar May 26, 2020

dejafait May 26, 2020 •

edited

kemar May 26, 2020

dejafait May 27, 2020

kemar May 26, 2020

dejafait May 26, 2020

kemar May 26, 2020

kemar May 26, 2020

dejafait May 26, 2020

kemar May 26, 2020

dejafait May 26, 2020

kemar May 26, 2020

dejafait May 26, 2020

kemar May 26, 2020

kemar May 26, 2020

kemar May 26, 2020

github-actions bot commented May 27, 2020

vgrange/peamu #126

vgrange/peamu #126

Conversation

dejafait commented Apr 9, 2020 • edited

ikarius commented Apr 15, 2020

github-actions bot commented Apr 22, 2020

github-actions bot commented Apr 22, 2020

Choose a reason for hiding this comment

kemar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dejafait May 26, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dejafait May 26, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dejafait May 26, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dejafait May 26, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TODO: Proper exception handling

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 27, 2020

dejafait commented Apr 9, 2020 •

edited

dejafait May 26, 2020 •

edited

dejafait May 26, 2020 •

edited

dejafait May 26, 2020 •

edited

dejafait May 26, 2020 •

edited