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

Refactor permissions #35

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Refactor permissions #35

wants to merge 18 commits into from

Conversation

TeddyRoncin
Copy link
Member

Checkez le README, si ce n'est pas clair dites-le moi :)

Copy link

codecov bot commented Nov 6, 2024

Codecov Report

Attention: Patch coverage is 88.98072% with 40 lines in your changes missing coverage. Please review.

Project coverage is 81.45%. Comparing base (9986481) to head (4496c4d).

Files with missing lines Patch % Lines
src/dependency_graph.ts 0.00% 16 Missing ⚠️
src/auth/auth.controller.ts 81.35% 11 Missing ⚠️
src/auth/guard/permission.guard.ts 0.00% 5 Missing ⚠️
src/auth/auth.service.ts 94.82% 3 Missing ⚠️
src/auth/guard/jwt.guard.ts 89.47% 2 Missing ⚠️
src/auth/dto/res/auth-redirection-res.dto.ts 0.00% 1 Missing ⚠️
src/auth/strategy/jwt.strategy.ts 93.33% 1 Missing ⚠️
src/ue/annals/annals.controller.ts 96.29% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #35      +/-   ##
==========================================
+ Coverage   80.73%   81.45%   +0.72%     
==========================================
  Files         110      123      +13     
  Lines        1775     2033     +258     
  Branches      352      387      +35     
==========================================
+ Hits         1433     1656     +223     
- Misses        337      372      +35     
  Partials        5        5              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@AlbanSdl AlbanSdl left a comment

Choose a reason for hiding this comment

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

Juste des commentaires sur la doc pour ce soir, comme dit je n'ai pas encore lu le code mais j'ai des questions sur la façon dont c'est fait


**Exemple :** prenons l'exemple de l'intégration : ils auront :
* Une _clé API_ pour le pour se connecter au front de EtuUTT avec le compte `[email protected]` (reliée à l'_application_ `EtuUTT-front`)
* Une _clé API_ pour le back de leur site web (reliée à `Integration-website`)
Copy link
Member

Choose a reason for hiding this comment

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

ça dépend non ? pourquoi il ne pourrait pas y avoir une key/user si on veut accéder à des données privées des utilisateurs par exemple ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ça serait le cas sur une application, sur un site web autant juste donner des permissions à la clé

Une _clé API_ (ou _Api Key_) est une relation entre un utilisateur et une _application_. Un utilisateur ne peut avoir qu'une _clé API_ par _application_.

```{note}
Une _clé API_ **n'est pas** un token, c'est plutôt un objet qui servira à générer un token et authentifier les requêtes.
Copy link
Member

Choose a reason for hiding this comment

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

Peut être rappeler que les applis agissent "on the behalf of the user" et que l'utilisateur doit avoir les permissions afin que la clé puisse accéder à la dite permission. (eg. modérer les commentaires, etc)

```{note}
Une _clé API_ **n'est pas** un token, c'est plutôt un objet qui servira à générer un token et authentifier les requêtes.

Un utilisateur n'a pas nécessairement les mêmes droits sur les différentes _applications_. Il est tout de même important de noter que rien ne l'empêchera d'utiliser une _clé API_ sur une _application_ qui n'est pas liée à cette _clé API_. Il est donc important **d'avoir confiance** en l'utilisateur, et pas uniquement en l'application.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Un utilisateur n'a pas nécessairement les mêmes droits sur les différentes _applications_. Il est tout de même important de noter que rien ne l'empêchera d'utiliser une _clé API_ sur une _application_ qui n'est pas liée à cette _clé API_. Il est donc important **d'avoir confiance** en l'utilisateur, et pas uniquement en l'application.
Un utilisateur peut ne pas accorder les mêmes droits aux différentes _applications_. Il est important de noter que rien ne l'empêchera d'utiliser une _clé API_ sur une _application_ qui n'est pas liée à cette _clé API_. Il est donc important **d'avoir confiance** en l'utilisateur, et pas uniquement en l'application.

Mais sinon, on peut imaginer un travail de déchiffrement de la clé renvoyée par l'api du site étu. Mais ça veut dire que les requêtes doivent passer par un backend intermédiaire

Copy link
Member Author

Choose a reason for hiding this comment

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

Par rapport au requested change, nope, c'est pas ça, c'est pas l'user qui donne des perms a sa clé api, c'est les gens concernés (admins, autres utilisateurs, ou lui dans le cas spécifique du front-end)


La méthode de connexion "utilisateur" permettra donc de générer un _bearer token_ temporaire, avec une connexion standard (décentralisée, nom d'utilisateur / mot de passe).

La méthode de connexion "application" permettra de générer un _bearer token_ avec une durée de vie possiblement infinie (en fonction de ce que veut l'utilisateur). On passe ici par une autre application (le site EtuUTT) pour générer un _bearer token_.
Copy link
Member

Choose a reason for hiding this comment

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

J'ai un problème avec cette durée de vie de token potentiellement infinie... C'est contraire a plein de règles de sécurité. Alors oui ça permet d'éviter d'avoir à se reconnecter mais il serait plus sécuritaire de renouveler régulièrement le jwt - automatiquement de façon complètement transparente, par exemple en renvoyant un header spécifique avec la réponse ou de trouver une façon de le rotate - plutôt que de pouvoir lui donner une durée de vie infinie...

Si un token arrive à expiration, l'application refait la demande de connexion à l'application dont la clé émane. Si l'utilisateur est connecté, le token est regen instant sinon il doit rentrer ses identifiants, seems legit

Copy link
Member Author

Choose a reason for hiding this comment

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

C'était pas vraiment pour ce cas là, c'était plus pour les applis ou on a la flm de modifier les env vars tous les 4 matins


### Pour une application

Pour une application, on génère un token pour la _clé API_ demandée, puis on retourne le _bearer token_ associé. Il faut aussi bien sauvegarder la date de dernière mise à jour (`tokenUpdatedAt`), et utiliser cette date pour toujours retourner la même version du token (champ `iat` dans l'objet à encoder avec JWT).
Copy link
Member

Choose a reason for hiding this comment

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

Rajouter le nom de la table avec tokenUpdatedAt

Maintenant (je pense que j'aurai la réponse quand je lirai le code) mais j'imagine que c'est pour mitiger le fait que la durée de vie puisse être infinie:

  • soit en pouvant "supprimer" des tokens qui auraient fuité. Sauf que du coup c'est pas préventif et ça présuppose que des données ont déjà fuité
  • soit en regénérant les tokens à chaque connexion de l'utilisateur. Mais ça, ça part du principe que l'app est bien faite et que le dev n'était pas un flemmard qui aurait juste mis une durée de token infinie sans jamais faire se reconnecter les utilisateurs

Copy link
Member Author

Choose a reason for hiding this comment

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

Sinon sur tout ce qui est route login, on empêche le token d'être infini ? Et sur les routes de gestion des api keys, on leur permet ?


Le _bearer token_ est une chaîne de caractère encodant une certaine _clé API_, en utilisant le standard JWT.

### Soft grant
Copy link
Member

Choose a reason for hiding this comment

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

J'ai définitivement pas compris l'idée d'avoir une clé unique pour une application avec des soft grants et hard grants au lieu d'avoir une clé pour chaque utilisateur. De cette façon. Les fois où l'appli n'a pas besoin de faire une opération en tant qu'un utilisateur particulier (ex. accéder au profil de Jean-Patrick en étant connecté en tant que le dit Jean-Patrick), et bien ça n'arrive pas puisque ces données là sont publiques ? Et si jamais une appli devait accéder à une section privée du site étu (les commentaires des UEs par exemple) et à y donner accès sans login dans ce cas le developpeur n'a cas générer une key et faire accéder son site/app on his behalf, même s'il ne devrait théoriquement pas avoir le droit de le faire (puisque si une appli fait fuiter les commentaires des UEs qui ne sont pas censés être visibles des enseignants, c'est gênant)

Si c'est au niveau du stockage du token, ce token peut rester sur le front en soit et même ça pourrait être le front lui même qui effectuerait ces requêtes ? (je suis pas sûr de l'idée) ou sinon stocké dans la session côté serveur (et transmis lors de la connexion via le site étu) ou sinon mis dans une db (ce qui ne serait pas une guideline)

Copy link
Member Author

Choose a reason for hiding this comment

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

Les données des utilisateurs sont pas publiques 😅

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.

2 participants