From c73fb0a6b16a1d8fea739ed54acd21166fe06acd Mon Sep 17 00:00:00 2001 From: Wilfried BARADAT Date: Thu, 6 Jun 2024 09:19:34 +0200 Subject: [PATCH 1/2] =?UTF-8?q?=E2=AC=86=EF=B8=8F(project)=20upgrade=20bas?= =?UTF-8?q?e=20warren=20images=20to=200.3.0?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `warren:api-core-0.3.0` and `warren:app-0.3.0` are released. Upgrading base images to these last revision. --- CHANGELOG.md | 4 ++++ src/api/Dockerfile | 2 +- src/app/Dockerfile | 2 +- 3 files changed, 6 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2986d30..80b1f2d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ and this project adheres to ## Unreleased +### Changed + +- Upgrade base Warren images to 0.3.0 + ## [0.2.0] - 2024-05-23 ### Changed diff --git a/src/api/Dockerfile b/src/api/Dockerfile index a24cb04..6894427 100644 --- a/src/api/Dockerfile +++ b/src/api/Dockerfile @@ -1,5 +1,5 @@ # -- Base image -- -FROM fundocker/warren:api-core-0.2.0 as base +FROM fundocker/warren:api-core-0.3.0 as base # -- Builder -- FROM base as builder diff --git a/src/app/Dockerfile b/src/app/Dockerfile index 31951f5..3a31e3a 100644 --- a/src/app/Dockerfile +++ b/src/app/Dockerfile @@ -1,5 +1,5 @@ # -- Base image -- -FROM fundocker/warren:app-0.2.0 +FROM fundocker/warren:app-0.3.0 # Privileged user USER root:root From ff75a700cae6bcfebc67e512e47e9501d6e95e30 Mon Sep 17 00:00:00 2001 From: Wilfried BARADAT Date: Wed, 29 May 2024 19:54:10 +0200 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=94=92=EF=B8=8F(api)=20securing=20API?= =?UTF-8?q?=20with=20LTI=20token?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit API was unsecure and anyone could make request to it from a browser. Securing it with a LTI token verification on each endpoint, and to get the `course_id`, the `roles` of the user and its `user_id`. --- CHANGELOG.md | 1 + src/api/plugins/tdbp/tests/conftest.py | 1 + src/api/plugins/tdbp/tests/test_api.py | 301 +++++++++++++++++------- src/api/plugins/tdbp/warren_tdbp/api.py | 109 +++++---- 4 files changed, 284 insertions(+), 128 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 80b1f2d..8d90646 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to ### Changed - Upgrade base Warren images to 0.3.0 +- Secure API endpoints with LTI token ## [0.2.0] - 2024-05-23 diff --git a/src/api/plugins/tdbp/tests/conftest.py b/src/api/plugins/tdbp/tests/conftest.py index ac9da74..36160e5 100644 --- a/src/api/plugins/tdbp/tests/conftest.py +++ b/src/api/plugins/tdbp/tests/conftest.py @@ -6,6 +6,7 @@ import pytest from warren.tests.fixtures.app import http_client from warren.tests.fixtures.asynchronous import anyio_backend +from warren.tests.fixtures.auth import auth_headers from warren.tests.fixtures.db import ( db_engine, db_session, diff --git a/src/api/plugins/tdbp/tests/test_api.py b/src/api/plugins/tdbp/tests/test_api.py index b5a6686..2559a1b 100644 --- a/src/api/plugins/tdbp/tests/test_api.py +++ b/src/api/plugins/tdbp/tests/test_api.py @@ -1,46 +1,44 @@ """Tests for the TdBP Warren plugin.""" -from datetime import date, datetime + +from datetime import datetime import httpx import pytest from pydantic import ValidationError +from warren.utils import LTIUser, forge_lti_token from warren_tdbp.models import Grades, Scores, SlidingWindow @pytest.mark.anyio -@pytest.mark.parametrize( - "query_params", - [ - {}, # Missing parameters - {"course_id": 1234}, # Wrong "course_id" parameter data type - { - "course_id": "course_id_101", - "until": "today", - }, # Wrong "until" parameter data type - {"until": date.today()}, # Missing "course_id" parameter - {"foo": "fake"}, # Unsupported parameter - ], -) async def test_api_sliding_window_with_invalid_params( - query_params, http_client: httpx.AsyncClient + http_client: httpx.AsyncClient, auth_headers: dict ): - """Test '/window' indicator with invalid query parameters returns 422 HTTP code.""" - response = await http_client.get("/api/v1/tdbp/window", params=query_params) + """Test `/window` endpoint with invalid query parameters returns 422 HTTP code.""" + query_params = {"until": "today"} + response = await http_client.get( + "/api/v1/tdbp/window", + params=query_params, + headers=auth_headers, + ) assert response.status_code == 422 @pytest.mark.anyio async def test_api_sliding_window_with_valid_params( - http_client: httpx.AsyncClient, db_session, sliding_window_fake_dataset + http_client: httpx.AsyncClient, + db_session, + sliding_window_fake_dataset, ): - """Test '/window' indicator with query parameters returns correct output.""" - course_id = "https://fake-lms.com/course/tdbp_101" + """Test `/window` endpoint with query parameters returns correct output.""" + token = forge_lti_token(course_id="https://fake-lms.com/course/tdbp_101") date_until = datetime.now().date() response = await http_client.get( - "/api/v1/tdbp/window", params={"course_id": course_id, "until": date_until} + "/api/v1/tdbp/window", + params={"until": date_until}, + headers={"Authorization": f"Bearer {token}"}, ) assert response.status_code == 200 @@ -52,38 +50,50 @@ async def test_api_sliding_window_with_valid_params( @pytest.mark.anyio -@pytest.mark.parametrize( - "query_params", - [ - {}, # Missing parameters - {"course_id": 1234}, # Wrong "course_id" parameter data type - { - "course_id": "course_id_101", - "until": "today", - }, # Wrong "until" parameter data type - {"until": date.today()}, # Missing "course_id" parameter - {"foo": "fake"}, # Unsupported parameter - ], -) +async def test_api_sliding_window_with_invalid_auth_headers( + http_client: httpx.AsyncClient, +): + """Test `/window` endpoint with an invalid `auth_headers`.""" + date_until = datetime.now().date() + response = await http_client.get( + "/api/v1/tdbp/window", + params={"until": date_until}, + headers={"Authorization": "Bearer Wrong_Token"}, + ) + + assert response.status_code == 401 + assert response.json().get("detail") == "Could not validate credentials" + + +@pytest.mark.anyio async def test_api_cohort_with_invalid_params( - query_params, http_client: httpx.AsyncClient + http_client: httpx.AsyncClient, auth_headers: dict ): - """Test '/cohort' indicator with invalid query parameters returns 422 HTTP code.""" - response = await http_client.get("/api/v1/tdbp/cohort", params=query_params) + """Test `/cohort` endpoint with invalid query parameters returns 422 HTTP code.""" + query_params = {"until": "today"} + response = await http_client.get( + "/api/v1/tdbp/cohort", + params=query_params, + headers=auth_headers, + ) assert response.status_code == 422 @pytest.mark.anyio async def test_api_cohort_with_valid_params( - http_client: httpx.AsyncClient, db_session, sliding_window_fake_dataset + http_client: httpx.AsyncClient, + db_session, + sliding_window_fake_dataset, ): - """Test '/window' indicator with query parameters returns correct output.""" - course_id = "https://fake-lms.com/course/tdbp_101" + """Test `/window` endpoint with query parameters returns correct output.""" + token = forge_lti_token(course_id="https://fake-lms.com/course/tdbp_101") date_until = datetime.now().date() response = await http_client.get( - "/api/v1/tdbp/cohort", params={"course_id": course_id, "until": date_until} + "/api/v1/tdbp/cohort", + params={"until": date_until}, + headers={"Authorization": f"Bearer {token}"}, ) assert response.status_code == 200 @@ -91,107 +101,232 @@ async def test_api_cohort_with_valid_params( assert len(response.json()) > 1 +@pytest.mark.anyio +async def test_api_cohort_with_invalid_auth_headers( + http_client: httpx.AsyncClient, +): + """Test `/cohort` endpoint with an invalid `auth_headers`.""" + date_until = datetime.now().date() + response = await http_client.get( + "/api/v1/tdbp/cohort", + params={"until": date_until}, + headers={"Authorization": "Bearer Wrong_Token"}, + ) + + assert response.status_code == 401 + assert response.json().get("detail") == "Could not validate credentials" + + @pytest.mark.anyio @pytest.mark.parametrize( "query_params", [ - {}, # Missing parameters - {"course_id": 1234}, # Wrong "course_id" parameter data type { - "course_id": "course_id_101", "until": "today", }, # Wrong "until" parameter data type { - "course_id": "course_id_101", - "student_id": 12345, - }, # Wrong "student_id" parameter data type - { - "course_id": "course_id_101", - "totals": 1, + "totals": 2, }, # Wrong "totals" parameter data type { - "course_id": "course_id_101", - "average": 1, + "average": 2, }, # Wrong "average" parameter data type - {"until": date.today()}, # Missing required "course_id" parameter - {"foo": "fake"}, # Unsupported parameter ], ) async def test_api_scores_with_invalid_params( - query_params, http_client: httpx.AsyncClient + query_params, http_client: httpx.AsyncClient, auth_headers ): - """Test '/scores' indicator with invalid query parameters returns 422 HTTP code.""" - response = await http_client.get("/api/v1/tdbp/scores", params=query_params) + """Test `/scores` endpoint with invalid query parameters returns 422 HTTP code.""" + response = await http_client.get( + "/api/v1/tdbp/scores", + params=query_params, + headers=auth_headers, + ) assert response.status_code == 422 @pytest.mark.anyio -async def test_api_scores_with_valid_params( - http_client: httpx.AsyncClient, db_session, sliding_window_fake_dataset +async def test_api_scores_with_valid_params_instructor( + http_client: httpx.AsyncClient, + db_session, + sliding_window_fake_dataset, ): - """Test '/scores' indicator with query parameters returns correct output.""" - course_id = "https://fake-lms.com/course/tdbp_101" + """Test `/scores` endpoint for an instructor returns correct output.""" + token = forge_lti_token( + roles=("instructor",), + course_id="https://fake-lms.com/course/tdbp_101", + ) date_until = datetime.now().date() response = await http_client.get( - "/api/v1/tdbp/scores", params={"course_id": course_id, "until": date_until} + "/api/v1/tdbp/scores", + params={"until": date_until, "average": True, "totals": True}, + headers={"Authorization": f"Bearer {token}"}, ) assert response.status_code == 200 try: - Scores.parse_obj(response.json()) + scores = Scores.parse_obj(response.json()) except ValidationError as err: pytest.fail(f"Scores indicator is invalid: {err}") + # Instructors can see average and totals scores + assert len(scores.scores) > 1 + assert scores.average + assert scores.total + + +@pytest.mark.anyio +async def test_api_scores_with_valid_params_student( + http_client: httpx.AsyncClient, + db_session, + sliding_window_fake_dataset, +): + """Test `/scores` endpoint for a student returns correct output.""" + token = forge_lti_token( + user=LTIUser( + id="johndoe", + email="johndoe@example.com", + ), + roles=("student",), + course_id="https://fake-lms.com/course/tdbp_101", + ) + date_until = datetime.now().date() + + response = await http_client.get( + "/api/v1/tdbp/scores", + params={"until": date_until, "average": 1, "total": 1}, + headers={"Authorization": f"Bearer {token}"}, + ) + + assert response.status_code == 200 + + try: + scores = Scores.parse_obj(response.json()) + except ValidationError as err: + pytest.fail(f"Scores indicator is invalid: {err}") + + # Student can only see its scores, and cannot see total and average scores + scores.scores.pop("johndoe") + assert not scores.scores + assert not scores.average + assert not scores.total + + +@pytest.mark.anyio +async def test_api_scores_with_invalid_auth_headers( + http_client: httpx.AsyncClient, +): + """Test `/scores` endpoint with an invalid `auth_headers`.""" + date_until = datetime.now().date() + response = await http_client.get( + "/api/v1/tdbp/scores", + params={"until": date_until}, + headers={"Authorization": "Bearer Wrong_Token"}, + ) + + assert response.status_code == 401 + assert response.json().get("detail") == "Could not validate credentials" + @pytest.mark.anyio @pytest.mark.parametrize( "query_params", [ - {}, # Missing parameters - {"course_id": 1234}, # Wrong "course_id" parameter data type { - "course_id": "course_id_101", "until": "today", }, # Wrong "until" parameter data type { - "course_id": "course_id_101", - "student_id": 12345, - }, # Wrong "student_id" parameter data type - { - "course_id": "course_id_101", - "average": 1, + "average": 2, }, # Wrong "average" parameter data type - {"until": date.today()}, # Missing required "course_id" parameter - {"foo": "fake"}, # Unsupported parameter ], ) async def test_api_grades_with_invalid_params( - query_params, http_client: httpx.AsyncClient + query_params, http_client: httpx.AsyncClient, auth_headers: dict ): - """Test '/grades' indicator with invalid query parameters returns 422 HTTP code.""" - response = await http_client.get("/api/v1/tdbp/grades", params=query_params) + """Test `/grades` endpoint with invalid query parameters returns 422 HTTP code.""" + response = await http_client.get( + "/api/v1/tdbp/grades", + params=query_params, + headers=auth_headers, + ) assert response.status_code == 422 @pytest.mark.anyio -async def test_api_grades_with_valid_params( - http_client: httpx.AsyncClient, db_session, sliding_window_fake_dataset +async def test_api_grades_with_valid_params_instructor( + http_client: httpx.AsyncClient, + db_session, + sliding_window_fake_dataset, ): - """Test '/grades' indicator with query parameters returns correct output.""" - course_id = "https://fake-lms.com/course/tdbp_101" + """Test `/grades` endpoint for an instructor returns correct output.""" + token = forge_lti_token( + roles=("instructor",), + course_id="https://fake-lms.com/course/tdbp_101", + ) date_until = datetime.now().date() response = await http_client.get( - "/api/v1/tdbp/grades", params={"course_id": course_id, "until": date_until} + "/api/v1/tdbp/grades", + params={"until": date_until, "average": True}, + headers={"Authorization": f"Bearer {token}"}, ) assert response.status_code == 200 try: - Grades.parse_obj(response.json()) + grades = Grades.parse_obj(response.json()) except ValidationError as err: pytest.fail(f"Grades indicator is invalid: {err}") + + assert len(grades.grades) > 1 + assert grades.average + + +@pytest.mark.anyio +async def test_api_grades_with_valid_params_student( + http_client: httpx.AsyncClient, + db_session, + sliding_window_fake_dataset, +): + """Test `/grades` endpoint for a student returns correct output.""" + token = forge_lti_token( + user=LTIUser(id="student_1", email="student_1@example.com"), + roles=("student",), + course_id="https://fake-lms.com/course/tdbp_101", + ) + date_until = datetime.now().date() + + response = await http_client.get( + "/api/v1/tdbp/grades", + params={"until": date_until}, + headers={"Authorization": f"Bearer {token}"}, + ) + + assert response.status_code == 200 + + try: + grades = Grades.parse_obj(response.json()) + except ValidationError as err: + pytest.fail(f"Grades indicator is invalid: {err}") + + # Student can only see its grades + assert len(grades.grades) == 1 + + +@pytest.mark.anyio +async def test_api_grades_with_invalid_auth_headers( + http_client: httpx.AsyncClient, +): + """Test `/grades` endpoint with an invalid `auth_headers`.""" + date_until = datetime.now().date() + response = await http_client.get( + "/api/v1/tdbp/grades", + params={"until": date_until}, + headers={"Authorization": "Bearer Wrong_Token"}, + ) + + assert response.status_code == 401 + assert response.json().get("detail") == "Could not validate credentials" diff --git a/src/api/plugins/tdbp/warren_tdbp/api.py b/src/api/plugins/tdbp/warren_tdbp/api.py index f9a32ee..a2b8cbd 100644 --- a/src/api/plugins/tdbp/warren_tdbp/api.py +++ b/src/api/plugins/tdbp/warren_tdbp/api.py @@ -2,10 +2,12 @@ import logging from datetime import date -from typing import Optional +from typing import Annotated, List, Optional -from fastapi import APIRouter, HTTPException, Query, status +from fastapi import APIRouter, Depends, HTTPException, Query, status +from lti_toolbox.launch_params import LTIRole from warren.exceptions import LrsClientException +from warren.utils import get_lti_course_id, get_lti_roles, get_lti_user_id from .indicators import ( CohortIndicator, @@ -13,6 +15,7 @@ ScoresIndicator, SlidingWindowIndicator, ) +from .models import Grades, Scores, SlidingWindow router = APIRouter( prefix="/tdbp", @@ -23,13 +26,14 @@ @router.get("/window") async def get_sliding_window( - course_id: str = Query( - description="The course identifier on Moodle", - ), - until: Optional[date] = Query( - description="End date until when to compute the sliding window", - ), -): + course_id: Annotated[str, Depends(get_lti_course_id)], + until: Annotated[ + Optional[date], + Query( + description="End date until when to compute the sliding window", + ), + ] = None, +) -> SlidingWindow: """Return course sliding window indicator. Args: @@ -61,12 +65,11 @@ async def get_sliding_window( @router.get("/cohort") async def get_cohort( - course_id: str = Query( - description="The course identifier on Moodle", - ), - until: Optional[date] = Query( - description="End date until when to compute the sliding window", - ), + course_id: Annotated[str, Depends(get_lti_course_id)], + until: Annotated[ + Optional[date], + Query(description="End date until when to compute the sliding window"), + ] = None, ): """Return course (static) cohort information. @@ -95,27 +98,30 @@ async def get_cohort( @router.get("/scores") -async def get_scores( - course_id: str = Query( - description="The course identifier on Moodle", - ), - until: Optional[date] = Query( - description="End date until when to compute the sliding window", - ), - student_id: Optional[str] = Query( - None, description="The student identifier on Moodle" - ), - totals: bool = Query(False, description="Flag to activate to compute total scores"), - average: bool = Query( - False, description="Flag to activate to compute average scores" - ), -): +async def get_scores( # noqa: PLR0913 + course_id: Annotated[str, Depends(get_lti_course_id)], + roles: Annotated[List[LTIRole], Depends(get_lti_roles)], + user_id: Annotated[str, Depends(get_lti_user_id)], + until: Annotated[ + Optional[date], + Query( + description="End date until when to compute the sliding window", + ), + ] = None, + totals: Annotated[ + bool, Query(description="Flag to activate to compute total scores") + ] = False, + average: Annotated[ + bool, Query(description="Flag to activate to compute average scores") + ] = False, +) -> Scores: """Return student or cohort scores on active actions. Args: course_id (str): The course identifier on Moodle. + roles (LTIRole): The roles of the user. + user_id (str): The user identifier on Moodle. until (datetime): End date until when to compute the sliding window. - student_id (str): The student identifier on Moodle. totals (bool): Flag to activate cohort totals scores computing on active actions. average (bool): Flag to activate cohort average scores for computing on active @@ -129,6 +135,12 @@ async def get_scores( - average (list): Average students' score per active action. """ logger.debug("Start computing 'scores' indicator") + # Instructors can see scores of all students, the totals and average + if any(role in ["instructor", "teacher", "staff"] for role in roles): + student_id = None + # Students can only see their scores + else: + student_id = user_id indicator = ScoresIndicator( course_id=course_id, until=until, @@ -152,25 +164,26 @@ async def get_scores( @router.get("/grades") async def get_grades( - course_id: str = Query( - description="The course identifier on Moodle", - ), - until: Optional[date] = Query( - description="End date until when to compute the sliding window", - ), - student_id: Optional[str] = Query( - None, description="The student identifier on Moodle" - ), - average: bool = Query( - False, description="Flag to activate to compute average grades" - ), -): + course_id: Annotated[str, Depends(get_lti_course_id)], + roles: Annotated[List[LTIRole], Depends(get_lti_roles)], + user_id: Annotated[str, Depends(get_lti_user_id)], + until: Annotated[ + Optional[date], + Query( + description="End date until when to compute the sliding window", + ), + ] = None, + average: Annotated[ + bool, Query(description="Flag to activate to compute average grades") + ] = False, +) -> Grades: """Return average mark for graded active activities. Args: course_id (str): The course identifier on Moodle. + roles (list): The roles of the user. + user_id (str): The user identifier on Moodle. until (datetime): End date until when to compute the sliding window. - student_id (str): The student identifier on Moodle. average (bool): Flag to activate average grade computing on each graded active activity. @@ -181,6 +194,12 @@ async def get_grades( - average (list): Average students' grades per active activity. """ logger.debug("Start computing 'grades' indicator") + # Instructors can see scores of all students, the totals and average + if any(role in ["instructor", "teacher", "staff"] for role in roles): + student_id = None + # Students can only see their scores + else: + student_id = user_id indicator = GradesIndicator( course_id=course_id, until=until, student_id=student_id, average=average )