diff --git a/api/tests/test_mixed_auth.py b/api/tests/test_mixed_auth.py new file mode 100644 index 00000000..52144bb7 --- /dev/null +++ b/api/tests/test_mixed_auth.py @@ -0,0 +1,48 @@ +import pytest +from django.test import Client +from django.urls import path +from model_bakery import baker +from ninja import NinjaAPI +from ninja.security import SessionAuth + +from api.auth import ApiKeyAuth +from api.hashers import generate_key +from api.models import ApiClient, ApiKey + +api = NinjaAPI() + + +@api.post("", auth=[ApiKeyAuth(), SessionAuth()]) +def post_method(*args, **kwargs) -> None: + """Dummy POST route authenticated by either api key or session cookie.""" + pass + + +urlpatterns = [path("", api.urls)] + + +@pytest.mark.django_db +@pytest.mark.urls(__name__) +@pytest.mark.parametrize("user_logged_in", [False, True]) +def test_csrf_token(user_logged_in): + """Test that CSRF check happens only when no api key is used.""" + client = Client(enforce_csrf_checks=True) + key, hashed = generate_key() + api_client = baker.make(ApiClient) + baker.make(ApiKey, client=api_client, hashed_key=hashed) + if user_logged_in: + client.force_login(api_client.owner) + + response = client.post("") + assert response.status_code == 403 + assert response.json()["detail"] == "CSRF check Failed" + + # if using a valid API key, CSRF check should not occur + response = client.post("", headers={"X-APIKey": key}) + assert response.status_code == 200 + + # if using a wrong API key, ApiKeyAuth should fail, + # leading to a fallback into SessionAuth and a CSRF check + response = client.post("", headers={"X-APIKey": generate_key()[0]}) + assert response.status_code == 403 + assert response.json()["detail"] == "CSRF check Failed" diff --git a/club/api.py b/club/api.py index 9bf7bcad..1479ee5c 100644 --- a/club/api.py +++ b/club/api.py @@ -16,7 +16,7 @@ class ClubController(ControllerBase): @route.get( "/search", response=PaginatedResponseSchema[SimpleClubSchema], - auth=[SessionAuth(), ApiKeyAuth()], + auth=[ApiKeyAuth(), SessionAuth()], permissions=[CanAccessLookup], url_name="search_club", ) @@ -27,7 +27,7 @@ class ClubController(ControllerBase): @route.get( "/{int:club_id}", response=ClubSchema, - auth=[SessionAuth(), ApiKeyAuth()], + auth=[ApiKeyAuth(), SessionAuth()], permissions=[HasPerm("club.view_club")], url_name="fetch_club", ) diff --git a/core/api.py b/core/api.py index af4daff5..ab69f86e 100644 --- a/core/api.py +++ b/core/api.py @@ -99,7 +99,7 @@ class SithFileController(ControllerBase): @route.get( "/search", response=PaginatedResponseSchema[SithFileSchema], - auth=[SessionAuth(), ApiKeyAuth()], + auth=[ApiKeyAuth(), SessionAuth()], permissions=[CanAccessLookup], ) @paginate(PageNumberPaginationExtra, page_size=50) @@ -112,7 +112,7 @@ class GroupController(ControllerBase): @route.get( "/search", response=PaginatedResponseSchema[GroupSchema], - auth=[SessionAuth(), ApiKeyAuth()], + auth=[ApiKeyAuth(), SessionAuth()], permissions=[CanAccessLookup], ) @paginate(PageNumberPaginationExtra, page_size=50) diff --git a/counter/api.py b/counter/api.py index b7995d74..72da74e5 100644 --- a/counter/api.py +++ b/counter/api.py @@ -64,7 +64,7 @@ class CounterController(ControllerBase): @route.get( "/search", response=PaginatedResponseSchema[SimplifiedCounterSchema], - auth=[SessionAuth(), ApiKeyAuth()], + auth=[ApiKeyAuth(), SessionAuth()], permissions=[CanAccessLookup], ) @paginate(PageNumberPaginationExtra, page_size=50) @@ -77,7 +77,7 @@ class ProductController(ControllerBase): @route.get( "/search", response=PaginatedResponseSchema[SimpleProductSchema], - auth=[SessionAuth(), ApiKeyAuth()], + auth=[ApiKeyAuth(), SessionAuth()], permissions=[CanAccessLookup], ) @paginate(PageNumberPaginationExtra, page_size=50) diff --git a/docs/tutorial/api/dev.md b/docs/tutorial/api/dev.md index c3e00709..7d3e8fb9 100644 --- a/docs/tutorial/api/dev.md +++ b/docs/tutorial/api/dev.md @@ -1,4 +1,3 @@ - Pour l'API, nous utilisons `django-ninja` et sa surcouche `django-ninja-extra`. Ce sont des librairies relativement simples et qui présentent l'immense avantage d'offrir des mécanismes de validation et de sérialisation @@ -65,8 +64,8 @@ utilisez l'attribut `auth` et les classes `SessionAuth` et @route.get("", auth=[ApiKeyAuth()]) def fetch_foo(self, club_id: int): ... - # Celle-ci sera accessible peu importe la méthode d'authentification - @route.get("/bar", auth=[SessionAuth(), ApiKeyAuth()]) + # Celle-ci sera accessible avec les deux méthodes d'authentification + @route.get("/bar", auth=[ApiKeyAuth(), SessionAuth()]) def fetch_bar(self, club_id: int): ... # Et celle-ci sera accessible aussi aux utilisateurs non-connectés @@ -84,9 +83,7 @@ par-dessus `django-ninja`, le système de permissions de django et notre propre système. Cette dernière est documentée [ici](../perms.md). -### Limites des clefs d'API - -#### Incompatibilité avec certaines permissions +### Incompatibilité avec certaines permissions Le système des clefs d'API est apparu très tard dans l'histoire du site (en P25, 10 ans après le début du développement). @@ -117,10 +114,33 @@ Les principaux points de friction sont : - `IsLoggedInCounter`, qui utilise encore un autre système d'authentification maison et qui n'est pas fait pour être utilisé en dehors du site. -#### Incompatibilité avec les tokens csrf +### CSRF -Le [CSRF (*cross-site request forgery*)](https://fr.wikipedia.org/wiki/Cross-site_request_forgery) -est un des multiples facteurs d'attaque sur le web. +!!!info "A propos du csrf" + + Le [CSRF (*cross-site request forgery*)](https://fr.wikipedia.org/wiki/Cross-site_request_forgery) + est un vecteur d'attaque sur le web consistant + à soumettre des données au serveur à l'insu + de l'utilisateur, en profitant de sa session. + + C'est une attaque qui peut se produire lorsque l'utilisateur + est authentifié par cookie de session. + En effet, les cookies sont joints automatiquement à + toutes les requêtes ; + en l'absence de protection contre le CSRF, + un attaquant parvenant à insérer un formulaire + dans la page de l'utilisateur serait en mesure + de faire presque n'importe quoi en son nom, + et ce sans même que l'utilisateur ni les administrateurs + ne s'en rendent compte avant qu'il ne soit largement trop tard ! + + Sur le CSRF et les moyens de s'en prémunir, voir : + + - [https://owasp.org/www-community/attacks/csrf]() + - [https://security.stackexchange.com/questions/166724/should-i-use-csrf-protection-on-rest-api-endpoints]() + - [https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html]() + +Le CSRF, c'est dangereux. Heureusement, Django vient encore une fois à notre aide, avec des mécanismes intégrés pour s'en protéger. Ceux-ci incluent notamment un système de @@ -131,13 +151,36 @@ Ceux-ci sont bien adaptés au cycle requêtes/réponses typiques de l'expérience utilisateur sur un navigateur, où les requêtes POST sont toujours effectuées après une requête GET au cours de laquelle on a pu récupérer un token csrf. -Cependant, le flux des requêtes sur une API est bien différent ; -de ce fait, il est à attendre que les requêtes POST envoyées à l'API -par un client externe n'aient pas de token CSRF et se retrouvent -donc bloquées. +Cependant, ils sont également gênants et moins utiles +dans le cadre d'une API REST, étant donné +que l'authentification cesse d'être implicite : +la clef d'API doit être explicitement jointe aux headers, +pour chaque requête. -Pour ces raisons, l'accès aux requêtes POST/PUT/PATCH de l'API -par un client externe ne marche pas. +Pour ces raisons, la vérification CSRF ne prend place +que pour la vérification de l'authentification +par cookie de session. + +!!!warning "L'ordre est important" + + Si vous écrivez le code suivant, l'authentification par clef d'API + ne marchera plus : + + ```python + @api_controller("/foo") + class FooController(ControllerBase): + @route.post("/bar", auth=[SessionAuth(), ApiKeyAuth()]) + def post_bar(self, club_id: int): ... + ``` + + En effet, la vérification du cookie de session intègrera + toujours la vérification CSRF. + Or, un échec de cette dernière est traduit par django en un code HTTP 403 + au lieu d'un HTTP 401. + L'authentification se retrouve alors court-circuitée, + faisant que la vérification de la clef d'API ne sera jamais appelée. + + `SessionAuth` doit donc être déclaré **après** `ApiKeyAuth`. ## Créer un client et une clef d'API @@ -176,5 +219,3 @@ qui en a besoin. Dites-lui bien de garder cette clef en lieu sûr ! Si la clef est perdue, il n'y a pas moyen de la récupérer, vous devrez en recréer une. - - diff --git a/pedagogy/api.py b/pedagogy/api.py index ae8b7ea8..5bd68b8c 100644 --- a/pedagogy/api.py +++ b/pedagogy/api.py @@ -19,7 +19,7 @@ from pedagogy.utbm_api import UtbmApiClient class UvController(ControllerBase): @route.get( "/{code}", - auth=[SessionAuth(), ApiKeyAuth()], + auth=[ApiKeyAuth(), SessionAuth()], permissions=[ # this route will almost always be called in the context # of a UV creation/edition @@ -45,7 +45,7 @@ class UvController(ControllerBase): "", response=PaginatedResponseSchema[SimpleUvSchema], url_name="fetch_uvs", - auth=[SessionAuth(), ApiKeyAuth()], + auth=[ApiKeyAuth(), SessionAuth()], permissions=[HasPerm("pedagogy.view_uv")], ) @paginate(PageNumberPaginationExtra, page_size=100) diff --git a/sas/api.py b/sas/api.py index 298c4d06..3b87325f 100644 --- a/sas/api.py +++ b/sas/api.py @@ -52,7 +52,7 @@ class AlbumController(ControllerBase): @route.get( "/autocomplete-search", response=PaginatedResponseSchema[AlbumAutocompleteSchema], - auth=[SessionAuth(), ApiKeyAuth()], + auth=[ApiKeyAuth(), SessionAuth()], permissions=[CanAccessLookup], ) @paginate(PageNumberPaginationExtra, page_size=50)