OAuth Provider #4749
OAuth Provider #4749
Conversation
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
victormours
force-pushed
the
oauth-provider
branch
3 times, most recently
from
4457543
to
5d084e2
Compare
victormours
force-pushed
the
oauth-provider
branch
from
0c91a46
to
531860d
Compare
There was a problem hiding this comment.
commentaire edité car posté trop vite, woups!
félicitations pour cette PR monstre @victormours 👍
la description est limpide et c’est parfait de pouvoir se projeter aussi loin que tu l’as déjà fait dans les étapes à venir.
Je n’ai pas formellement accepté la PR puisqu’elle n’est pas vouée à être mergée mais plutôt qu’on va en extraire des PRs successives.
Je n’ai aucun retour bloquant.
Je n’ai pas testé en local, je n’ai pas cherché à le faire je me dis que ça pourrait être plus efficace de le faire tous les deux en synchrone et éventuellement le documenter.
Dans la description de la PR je n’ai pas compris cette phrase :
L'introspection de tokens ne fonctionne pas parce que le token qui authentifie la requête doit être différent de celui qui est introspecté)
| @@ -90,6 +97,9 @@ | |||
| sessions: "agents/sessions", | |||
| passwords: "agents/passwords", | |||
| } | |||
| devise_scope :agent do | |||
| get "agents/sign_out", to: "agents/sessions#destroy" # Utilisé par les clients Oauth pour se déconnecter | |||
There was a problem hiding this comment.
y a-t-il une raison de dédoubler le bloc devise_scope :agent ? il est déjà ouvert ligne 104
| @@ -41,7 +41,7 @@ | |||
| end | |||
|
|
|||
| RSpec.configure do |config| | |||
| config.after(:each, js: true) do | |||
| config.after(:each, ignore_js_errors: nil, js: true) do | |||
There was a problem hiding this comment.
Je vois que quand on le désactive la spec spec/features/agents/oauth_provider_spec.rb échoue à cause d’une 404 sur le favicon, j’imagine que c’est la raison pour rajouter cette option ?
ça me va tout à fait mais je me demande si ce n’est pas plus robuste pour l’avenir de réactiver la remontée des erreurs JS et de faire une route favicon.ico sur le serveur sinatra.
| @@ -0,0 +1,8 @@ | |||
| class ValidateDoorkeeperTables < ActiveRecord::Migration[7.1] | |||
| def change | |||
| validate_foreign_key :oauth_access_grants, :oauth_applications | |||
There was a problem hiding this comment.
Ce commentaire est purement pour ma compréhension et pas une remise en question de ce code :)
J’ai lu https://blog.saeloun.com/2024/05/08/rails-validate-foreign-key/ mais je ne suis pas sûr de comprendre pourquoi il faut faire l’ajout de FK en deux étapes.
Pourquoi est-ce que lorsqu’on créé une nouvelle colonne avec une FK pointant vers une grande table qui change beaucoup ça prend du temps ? 🤔 ma compréhension naïve est que c’est instantané, que PostgreSQL ajoute simplement de la métadonnée sur cette colonne vide disant « lorsque de la donnée sera insérée dans cette colonne il faudra vérifier telle contrainte »
Je comprends pourquoi ça peut prendre du temps de rajouter une contrainte FK sur une colonne existante mais sur une nouvelle colonne je ne comprends pas. J’imagine que ma compréhension de comment marche une contrainte FK est incorrecte ! merci d’avance si vous avez des infos ou des articles vers lesquels me pointer :)
| @@ -0,0 +1,53 @@ | |||
| class CommentDoorkeeperTables < ActiveRecord::Migration[7.1] | |||
| def change | |||
| change_column_comment(:oauth_applications, :uid, from: nil, to: <<~COMMENT | |||
There was a problem hiding this comment.
| change_column_comment(:oauth_applications, :uid, from: nil, to: <<~COMMENT | |
| change_column_comment(:oauth_applications, :uid, from: nil, to: <<~COMMENT.strip |
le .strip permet d’éviter le \n qui traîne à la fin de ces commentaires.
Et très cools ces commentaires sur les colonnes de la db, j’allais dire que j’avais un doute je n’avais pas vu qu’ils apparaissent aussi dans le schema.rb ce qui est super 👍
| @@ -0,0 +1,30 @@ | |||
| # RDV-SP comme provider OAuth | |||
|
|
|||
| Cette application peut s'interconnecter avec des clients externes via le protocol Oauth 2.0. Ce mécanisme permet à une application tierce de proposer à ses utilisateurs d'autoriser l'application tierce à faire des appels à l'API RDV-SP en son nom. Par exemple, un agent qui a un compte sur demarches-simplifiees.fr peut autoriser la plateforme à créer des RDVs en son nom sur RDV-SP. | |||
There was a problem hiding this comment.
| Cette application peut s'interconnecter avec des clients externes via le protocol Oauth 2.0. Ce mécanisme permet à une application tierce de proposer à ses utilisateurs d'autoriser l'application tierce à faire des appels à l'API RDV-SP en son nom. Par exemple, un agent qui a un compte sur demarches-simplifiees.fr peut autoriser la plateforme à créer des RDVs en son nom sur RDV-SP. | |
| Cette application peut s'interconnecter avec des clients externes via le protocole Oauth 2.0. Ce mécanisme permet à une application tierce de proposer à ses utilisateurs de l’autoriser à faire des appels à l'API RDV Service Public en son nom. Par exemple, un agent qui a un compte sur demarches-simplifiees.fr peut autoriser la plateforme à créer des RDV en son nom sur RDV Service Public. |
|
|
||
| Ci-dessous le processus d'autorisation par lequel un agent de demarches-simplifiees.fr lie son compte à RDV-SP : | ||
|
|
||
| ```mermaid |
|
|
||
| # On change les options passées en dernier argument par rapport à la classe mère | ||
| def client | ||
| ::OAuth2::Client.new(options.client_id, options.client_secret, client_options) |
There was a problem hiding this comment.
nitpick complètement ignorable : j’aurais tendance à inliner la méthode client_options ici. Ça permettrait d’éviter la confusion avec options.client_options qui existe dans la classe mère OmniAuth::Strategies::OAuth2.
encore plus profondément dans le détail négligeable, je trouve que le commentaire n’explicite pas que le but de cet override est de permettre aux utilisateurs de ce provider de n’avoir à passer qu’une option base_url plutôt que site, authorize_url et base_url (si j’ai bien compris)
| # Enforce token request content type to application/x-www-form-urlencoded. | ||
| # It is not enabled by default to not break prior versions of the gem. | ||
| # | ||
| # enforce_content_type |
There was a problem hiding this comment.
on peut peut-être l’activer celle-ci ? je suis un peu perdu dans la terminologie des token, access, authorization codes dans ce fichier mais à la lecture du commentaire au dessus j’ai l’impression que les devs de doorkeeper auraient aimé l’activer par défaut s’il n’y avait pas les anciennes versions de la gem
| = render "agents/preferences_menu" | ||
| - else | ||
| h4.mb-3 Prenez RDV en ligne avec votre département ! | ||
| p.lead.mb-3 Terminé l'agenda papier, moins de temps perdu. |
There was a problem hiding this comment.
ce n’est pas très important, mais je ne suis pas sûr de comprendre ce changement dans cette PR ? ça vient peut-être d’un merge raté ?
Contexte : l'intégration à Démarches Simplifiées
Dans le cadre de notre financement par le FTAP, on a prévu de faire une intégration avec Démarches Simplifiées pour permettre à l'instructeur d'un dossier sur DS de proposer un rendez-vous à l'usager qui a envoyé le dossier.
Cette intégration nécessite que Démarches Simplifiées appelle notre api, par exemple vérifier l'existence de plages d'ouverture, ou pour envoyer l'invitation (une fois qu'on aura un endpoint qui permettra de faire cette action).
Voir la maquette de l'intégration : https://www.figma.com/board/Vuy7UCLaLWpoY1u30AEaKG/Piste-d'int%C3%A9gration-RDV-%26-DS?node-id=0-1&node-type=canvas&t=eQKAZwYgYYkh8ERT-0
Piste d'intégration RDV & DS.pdf
Le problème, c'est que l'authentification à notre api actuelle ne fonctionne pas très bien pour ce cas d'usage. On a deux modes d'authentification :
devise_token_auth: l'endpoint/api/v1/auth/sign_inpermet d'envoyer un email/mot de passe et d'obtenir un token d'api en échange. Si on utilisait ce mécanisme pour l'intégration à DS, il faudrait demander aux agents d'entrer leur email/mot de passe de RDV Service Public sur le front de DS, ce qui n'est pas acceptable (c'est la pire dé-sensibilitation au risque de phishing qu'on puisse imaginer). Cela ne permet pas non plus d'authentifier les agents qui utilisent ProConnect)app/controllers/api/v1/agent_auth_base_controller.rb:95), et ajoute ce hash en header des requêtes. C'est ce qu'utilise RDV Insertion pour les agents qui utilisent ProConnect. Le problème de cette approche est qu'il n'y a pas vraiment de vérification que l'agent utilisé a bien donné la permission à ce qu'on utilise l'api en son nom, et ça permet à l'application cliente de lire et modifier des données pour tous les agents de l'application. C'est déjà discutable pour RDV Insertion, et pas généralisable à d'autres applications (impossible à justifier d'un point de vue RGPD).On a donc besoin d'un autre mode d'authentification des agents à notre api, et on a la chance d'être dans un cas très classique pour lequel un standard a émergé : l'OAuth.
La place de l'oauth dans cette intégration
En proposant aux agents qui utilisent Démarches Simplifiées et RDV Service Public de connecter les deux applications par Oauth, on permet à la fois de fournir un mode d'authentification viable, et d'expliciter l'intégration aux agents.
Le fait de rendre notre application visible à l'agent permet aussi d'éviter les demandes qu'on a eu de la part de certains partenaires potentiel de cacher toute notre application derrière leur front et de recoder toute notre application en api.
Le parcours d'OAuth permettra à Démarches Simplifiées de récupérer un token d'api au nom de l'agent.
Le plan d'ouverture de l'oauth
C'est un gros projet, donc on va le découper :
MVP : Mise en place du parcours d'OAuth pour RDV Insertion
Pour cette première étape, on ne change pas l'authentification à l'api, mais on met en place le parcours d'oauth pour les agents.
Ça nous permet directement de remplacer la manière dont certains agents se connectent à RDV Insertion en entrant leur email/mot de passe RDV Solidarités sur le front de RDV Insertion. Cette PR chez RDV Insertion permettra de faire le changement une fois qu'on aura déployé l'oauth sur RDV Service Public gip-inclusion/rdv-insertion#2426
Critère d'acceptation (aka "definition of done") :
V1 : Authentification à l'api par token récupérés pendant l'Oauth par RDV Insertion
Une fois que les agents seront authentifiés par Oauth, on pourra commencer à utiliser les token pour les appels faits par RDV Insertion, et à voir comment on gère les refresh_token.
A partir de ce moment, on pourra aussi désactiver le bouton ProConnect sur RDV Insertion, puisque la connexion à ProConnect pourra se faire directement sur RDV Solidarités.
Réutilisation pour Démarches Simplifiées
Une fois que ça marchera pour RDV Insertion, ça devrait être réutilisable à l'identique pour commencer à travailler sur l'intégration avec DS
Le périmètre de cette PR dans ce plan
Le but de cette PR est de permettre des premiers tests utilisateurs du parcours d'OAuth avec RDV Insertion. Elle se concentre plutôt sur les aspects techniques, et le but est donc de valider les différents choix qui sont faits ici.
NB : Pour faire les strong migrations correctement, on ne peut pas merger directement cette PR. On va d'abord extraire la migration de création des tables dans une autre PR avant de merger celle-ci
La mise en prod du code de cette PR
Plan de mise en ligne :
forcedans l'urlSolution :
Rappel sur l'oauth https://auth0.com/docs/get-started/authentication-and-authorization-flow/authorization-code-flow
Choix d'implémentation majeurs
Doorkeeper
On utilise la gem doorkeeper pour l'implémentation. Ça semble être un standard, et gitlab s'en sert aussi. On a essayé d'en faire l'usage le plus simple possible.
gem omniauth dans lib
endpoint de
/meDe manière similaire à ce que fait Github, on propose un endpoint
/api/v1/agents/mepour les infos de l'agent connecté. C'est la manière la plus standard que j'ai trouvé pour faire ça. L'introspection de tokens ne fonctionne pas parce que le token qui authentifie la requête doit être différent de celui qui est introspecté)Choix de configuration de doorkeeper
Choix d'implémentation mineurs
Tester
En local, on peut directement tester avec RDV Insertion en étant sur la bonne branche et en relançant les seeds.
Scénarios à tester dans le navigateur
Prochaines étapes après cette PR
Tests utilisateurs et amélioration de la page pour accepter l'autorisation
skip_authorizationdans l'initializerHors scope