From 32264f1251fb983272fac9224b12467f826c0cc1 Mon Sep 17 00:00:00 2001 From: F-G Fernandez <26927750+frgfm@users.noreply.github.com> Date: Tue, 7 Nov 2023 14:45:49 +0100 Subject: [PATCH 1/5] feat: Adds safeguard on guidelien reordering --- src/app/api/api_v1/endpoints/repos.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/app/api/api_v1/endpoints/repos.py b/src/app/api/api_v1/endpoints/repos.py index b3f2284..81815d0 100644 --- a/src/app/api/api_v1/endpoints/repos.py +++ b/src/app/api/api_v1/endpoints/repos.py @@ -60,13 +60,16 @@ async def fetch_repos( @router.put("/{repo_id}/guidelines/order", status_code=status.HTTP_200_OK) -async def reorder_guidelines( +async def reorder_repo_guidelines( payload: GuidelineOrder, repo_id: int = Path(..., gt=0), guidelines: GuidelineCRUD = Depends(get_guideline_crud), + repos: RepositoryCRUD = Depends(get_repo_crud), user=Security(get_current_user, scopes=[UserScope.USER, UserScope.ADMIN]), ) -> List[Guideline]: telemetry_client.capture(user.id, event="guideline-order", properties={"repo_id": repo_id}) + # Check the repo + await repos.get(repo_id, strict=True) # Ensure all IDs are unique if len(payload.guideline_ids) != len(set(payload.guideline_ids)): raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail="Duplicate IDs were passed.") From c2d9f1ec7e3450914dcfd2021c6761af00307aaf Mon Sep 17 00:00:00 2001 From: F-G Fernandez <26927750+frgfm@users.noreply.github.com> Date: Tue, 7 Nov 2023 14:46:40 +0100 Subject: [PATCH 2/5] refactor: Refactors github interactions --- src/app/api/api_v1/endpoints/login.py | 52 ++++++------------- src/app/core/config.py | 1 - src/app/schemas/login.py | 13 ----- src/app/schemas/services.py | 15 +++++- src/app/services/github.py | 73 ++++++++++++++++++--------- 5 files changed, 78 insertions(+), 76 deletions(-) diff --git a/src/app/api/api_v1/endpoints/login.py b/src/app/api/api_v1/endpoints/login.py index bc653d9..2deb7e3 100644 --- a/src/app/api/api_v1/endpoints/login.py +++ b/src/app/api/api_v1/endpoints/login.py @@ -5,7 +5,6 @@ import secrets -import requests from fastapi import APIRouter, Depends, HTTPException, status from fastapi.responses import RedirectResponse from fastapi.security import OAuth2PasswordRequestForm @@ -16,8 +15,10 @@ from app.core.security import create_access_token, hash_password, verify_password from app.crud import UserCRUD from app.models import UserScope -from app.schemas.login import GHAccessToken, GHToken, GHTokenRequest, Token, TokenRequest +from app.schemas.login import GHAccessToken, Token, TokenRequest +from app.schemas.services import GHToken from app.schemas.users import UserCreation +from app.services.github import gh_client from app.services.telemetry import telemetry_client router = APIRouter() @@ -35,23 +36,9 @@ async def authorize_github( @router.post("/github", status_code=status.HTTP_200_OK, summary="Request a GitHub token from authorization code") async def request_github_token_from_code( - payload: GHTokenRequest, + payload: TokenRequest, ) -> GHToken: - gh_payload = TokenRequest( - client_id=settings.GH_OAUTH_ID, - client_secret=settings.GH_OAUTH_SECRET, - redirect_uri=payload.redirect_uri, - code=payload.code, - ) - response = requests.post( - settings.GH_TOKEN_ENDPOINT, - json=gh_payload.dict(), - headers={"Accept": "application/json"}, - timeout=5, - ) - if response.status_code != status.HTTP_200_OK or isinstance(response.json().get("error"), str): - raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid authorization code.") - return GHToken(**response.json()) + return gh_client.get_token_from_code(payload.code, payload.redirect_uri) @router.post("/creds", status_code=status.HTTP_200_OK, summary="Request an access token using credentials") @@ -89,27 +76,22 @@ async def login_with_github_token( By default, the token expires after 1 hour. """ # Fetch GitHub - response = requests.get( - "https://api.github.com/user", - headers={"Content-Type": "application/json", "Authorization": f"Bearer {payload.github_token}"}, - timeout=5, + gh_user = gh_client.get_my_user(payload.github_token) + telemetry_client.capture(gh_user["id"], event="user-login", properties={"login": gh_user["login"]}) + telemetry_client.identify( + gh_user["id"], + properties={ + "login": gh_user["login"], + "name": gh_user["name"], + "email": gh_user["email"], + "twitter_username": gh_user["twitter_username"], + }, ) - if response.status_code != 200: - raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid GitHub token.") - gh_user = response.json() # Verify credentials user = await users.get(gh_user["id"], strict=False) # Register if non existing if user is None: - telemetry_client.identify( - gh_user["id"], - properties={ - "login": gh_user["login"], - "name": gh_user["name"], - "email": gh_user["email"], - "twitter_username": gh_user["twitter_username"], - }, - ) + telemetry_client.capture(gh_user["id"], event="user-creation", properties={"login": gh_user["login"]}) user = await users.create( UserCreation( id=gh_user["id"], @@ -118,11 +100,9 @@ async def login_with_github_token( scope=UserScope.USER, ) ) - telemetry_client.capture(user.id, event="user-creation", properties={"login": gh_user["login"]}) # create access token using user user_id/user_scopes token_data = {"sub": str(user.id), "scopes": user.scope.split()} token = await create_access_token(token_data, settings.ACCESS_TOKEN_UNLIMITED_MINUTES) - telemetry_client.capture(user.id, event="user-login", properties={"login": user.login}) return Token(access_token=token, token_type="bearer") # nosec B106 # noqa S106 diff --git a/src/app/core/config.py b/src/app/core/config.py index 44b35da..f97e888 100644 --- a/src/app/core/config.py +++ b/src/app/core/config.py @@ -23,7 +23,6 @@ class Settings(BaseSettings): CORS_ORIGIN: str = "*" # Ext API endpoints GH_AUTHORIZE_ENDPOINT: str = "https://github.com/login/oauth/authorize" - GH_TOKEN_ENDPOINT: str = "https://github.com/login/oauth/access_token" GH_OAUTH_ID: str = os.environ["GH_OAUTH_ID"] GH_OAUTH_SECRET: str = os.environ["GH_OAUTH_SECRET"] GH_TOKEN: Union[str, None] = os.environ.get("GH_TOKEN") diff --git a/src/app/schemas/login.py b/src/app/schemas/login.py index fe1b19c..76d281b 100644 --- a/src/app/schemas/login.py +++ b/src/app/schemas/login.py @@ -26,19 +26,6 @@ class TokenPayload(BaseModel): scopes: List[UserScope] = [] -class GHTokenRequest(BaseModel): - code: str - redirect_uri: HttpUrl - - class TokenRequest(BaseModel): - client_id: str - client_secret: str code: str redirect_uri: HttpUrl - - -class GHToken(BaseModel): - access_token: str - token_type: str - scope: str diff --git a/src/app/schemas/services.py b/src/app/schemas/services.py index 79c3938..60ab7e9 100644 --- a/src/app/schemas/services.py +++ b/src/app/schemas/services.py @@ -6,7 +6,7 @@ from enum import Enum from typing import Any, Dict, List -from pydantic import BaseModel +from pydantic import BaseModel, HttpUrl __all__ = ["ChatCompletion"] @@ -59,3 +59,16 @@ class ChatCompletion(BaseModel): function_call: Dict[str, str] temperature: float frequency_penalty: float + + +class GHTokenRequest(BaseModel): + client_id: str + client_secret: str + code: str + redirect_uri: HttpUrl + + +class GHToken(BaseModel): + access_token: str + token_type: str + scope: str diff --git a/src/app/services/github.py b/src/app/services/github.py index 392c77f..b053222 100644 --- a/src/app/services/github.py +++ b/src/app/services/github.py @@ -6,50 +6,73 @@ from typing import Any, Dict, Union import requests -from fastapi import HTTPException +from fastapi import HTTPException, status +from pydantic import HttpUrl from app.core.config import settings +from app.schemas.services import GHToken, GHTokenRequest __all__ = ["gh_client"] class GitHubClient: ENDPOINT: str = "https://api.github.com" + OAUTH_ENDPOINT: str = "https://github.com/login/oauth/access_token" def __init__(self, token: Union[str, None] = None) -> None: - self.headers = ( - {"Authorization": f"Bearer {token}", "Content-Type": "application/json"} - if token - else {"Content-Type": "application/json"} + self.headers = self._get_headers(token) + + def _get_headers(self, token: Union[str, None] = None) -> Dict[str, str]: + if isinstance(token, str): + return {"Authorization": f"Bearer {token}", "Content-Type": "application/json"} + return {"Content-Type": "application/json"} + + def _get(self, route: str, token: Union[str, None] = None, timeout: int = 5) -> Dict[str, Any]: + response = requests.get( + f"{self.ENDPOINT}/{route}", + headers=self._get_headers(token) if isinstance(token, str) else self.headers, + timeout=timeout, ) + json_response = response.json() + if response.status_code != status.HTTP_200_OK: + raise HTTPException( + status_code=response.status_code, detail=json_response.get("error", json_response["message"]) + ) + return json_response - def _get(self, endpoint: str, entry_id: int) -> Dict[str, Any]: - response = requests.get(f"{self.ENDPOINT}/{endpoint}{entry_id}", headers=self.headers, timeout=2) - if response.status_code != 200: - raise HTTPException(status_code=response.status_code, detail=response.json()["error"]["message"]) - return response.json() + def get_repo(self, repo_id: int, **kwargs: Any) -> Dict[str, Any]: + return self._get(f"repositories/{repo_id}", **kwargs) - def get_repo(self, repo_id: int) -> Dict[str, Any]: - return self._get("repositories/", repo_id) + def get_user(self, user_id: int, **kwargs: Any) -> Dict[str, Any]: + return self._get(f"user/{user_id}", **kwargs) - def get_user(self, user_id: int) -> Dict[str, Any]: - return self._get("user/", user_id) + def get_my_user(self, token: str) -> Dict[str, Any]: + return self._get("user", token) def get_permission(self, repo_name: str, user_name: str, github_token: str) -> str: - response = requests.get( - f"{self.ENDPOINT}/repos/{repo_name}/collaborators/{user_name}/permission", - headers={ - "Content-Type": "application/json", - "Authorization": f"Bearer {github_token}", - }, - timeout=5, - ) - if response.status_code != 200: - raise HTTPException(status_code=response.status_code, detail=response.json()["error"]["message"]) - return response.json()["role_name"] + return self._get(f"repos/{repo_name}/collaborators/{user_name}/permission", github_token)["role_name"] def has_valid_permission(self, repo_name: str, user_name: str, github_token: str) -> bool: return self.get_permission(repo_name, user_name, github_token) in ("maintain", "admin") + def get_token_from_code(self, code: str, redirect_uri: HttpUrl, timeout: int = 5) -> GHToken: + gh_payload = GHTokenRequest( + client_id=settings.GH_OAUTH_ID, + client_secret=settings.GH_OAUTH_SECRET, + redirect_uri=redirect_uri, + code=code, + ) + response = requests.post( + self.OAUTH_ENDPOINT, + json=gh_payload.dict(), + headers={"Accept": "application/json"}, + timeout=timeout, + ) + if response.status_code != status.HTTP_200_OK: + raise HTTPException(status_code=response.status_code, detail=response.json()["error"]) + if response.status_code == status.HTTP_200_OK and isinstance(response.json().get("error_description"), str): + raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=response.json()["error_description"]) + return GHToken(**response.json()) + gh_client = GitHubClient(settings.GH_TOKEN) From 1a65c164f8b00d813abd98c46544113571186dcc Mon Sep 17 00:00:00 2001 From: F-G Fernandez <26927750+frgfm@users.noreply.github.com> Date: Tue, 7 Nov 2023 14:47:04 +0100 Subject: [PATCH 3/5] test: Updates unittests --- src/tests/endpoints/test_login.py | 18 +++++++++------- src/tests/endpoints/test_repos.py | 36 +++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 8 deletions(-) diff --git a/src/tests/endpoints/test_login.py b/src/tests/endpoints/test_login.py index 84d0609..089ac9d 100644 --- a/src/tests/endpoints/test_login.py +++ b/src/tests/endpoints/test_login.py @@ -6,7 +6,7 @@ from httpx import AsyncClient from sqlmodel.ext.asyncio.session import AsyncSession -from app.core import security +from app.api.api_v1.endpoints import login from app.models import User USER_TABLE = [ @@ -20,7 +20,8 @@ async def login_session(async_session: AsyncSession, monkeypatch): for entry in USER_TABLE: async_session.add(User(**entry)) await async_session.commit() - monkeypatch.setattr(security, "verify_password", pytest.mock_verify_password) + monkeypatch.setattr(login, "verify_password", pytest.mock_verify_password) + monkeypatch.setattr(login, "hash_password", pytest.mock_hash_password) yield async_session @@ -28,7 +29,7 @@ async def login_session(async_session: AsyncSession, monkeypatch): ("payload", "status_code", "status_detail"), [ ({"username": "foo"}, 422, None), - ({"github_token": "foo"}, 401, None), + ({"github_token": "foo"}, 401, "Bad credentials"), ], ) @pytest.mark.asyncio() @@ -50,7 +51,8 @@ async def test_login_with_github_token( [ ({"username": "foo"}, 422, None), ({"username": "foo", "password": "bar"}, 401, None), - # ({"username": "first_login", "password": "first_pwd"}, 200, None), + ({"username": "first_login", "password": "pwd"}, 401, None), + ({"username": "first_login", "password": "first_pwd"}, 200, None), ], ) @pytest.mark.asyncio() @@ -67,9 +69,9 @@ async def test_login_with_creds( assert response.json()["detail"] == status_detail if response.status_code // 100 == 2: response_json = response.json() - assert response_json["token_type"] == "Bearer" # noqa: S105 + assert response_json["token_type"] == "bearer" # noqa: S105 assert isinstance(response_json["access_token"], str) - assert len(response_json["access_token"]) == 10 + assert len(response_json["access_token"]) == 143 @pytest.mark.parametrize( @@ -78,7 +80,7 @@ async def test_login_with_creds( ({"code": "foo", "redirect_uri": 0}, 422, None, None), # Github 422 ({"code": "foo", "redirect_uri": ""}, 422, None, None), - ({"code": "foo", "redirect_uri": "https://quackai.com"}, 401, None, None), + ({"code": "foo", "redirect_uri": "https://quackai.com"}, 400, None, None), ], ) @pytest.mark.asyncio() @@ -91,7 +93,7 @@ async def test_request_github_token_from_code( expected_response: Union[Dict[str, Any], None], ): response = await async_client.post("/login/github", json=payload) - assert response.status_code == status_code + assert response.status_code == status_code, print(response.json(), isinstance(response.json()["detail"], str)) if isinstance(status_detail, str): assert response.json()["detail"] == status_detail if isinstance(expected_response, dict): diff --git a/src/tests/endpoints/test_repos.py b/src/tests/endpoints/test_repos.py index 8974875..3ae9f53 100644 --- a/src/tests/endpoints/test_repos.py +++ b/src/tests/endpoints/test_repos.py @@ -251,3 +251,39 @@ async def test_fetch_guidelines_from_repo( assert response.json()["detail"] == status_detail if response.status_code // 100 == 2: assert response.json() == expected_response + + +@pytest.mark.parametrize( + ("user_idx", "repo_id", "payload", "status_code", "status_detail"), + [ + (None, 12345, {"guideline_ids": [2]}, 401, "Not authenticated"), + (0, 1, {"guideline_ids": [2]}, 404, ""), + (0, 12345, {"guideline_ids": [1, 2]}, 422, None), + (0, 12345, {"guideline_ids": [2, 2]}, 422, None), + (0, 12345, {"guideline_ids": [2]}, 200, None), + (0, 123456, {"guideline_ids": [2]}, 422, None), + (1, 12345, {"guideline_ids": [2]}, 200, None), + ], +) +@pytest.mark.asyncio() +async def test_reorder_repo_guidelines( + async_client: AsyncClient, + guideline_session: AsyncSession, + user_idx: Union[int, None], + repo_id: int, + payload: Dict[str, Any], + status_code: int, + status_detail: Union[str, None], +): + auth = None + if isinstance(user_idx, int): + auth = await pytest.get_token(USER_TABLE[user_idx]["id"], USER_TABLE[user_idx]["scope"].split()) + + response = await async_client.put(f"/repos/{repo_id}/guidelines/order", headers=auth) + assert response.status_code == status_code, print(response.__dict__) + if isinstance(status_detail, str): + assert response.json()["detail"] == status_detail + if response.status_code // 100 == 2: + assert response.json() == [ + {**entry, "order": order} for entry, order in zip(GUIDELINE_TABLE, payload["guideline_ids"]) + ] From 4ef7b4523aa02a5528509a69cd50eb028015c013 Mon Sep 17 00:00:00 2001 From: F-G Fernandez <26927750+frgfm@users.noreply.github.com> Date: Tue, 7 Nov 2023 15:41:48 +0100 Subject: [PATCH 4/5] test: Updates tests --- src/tests/endpoints/test_repos.py | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) diff --git a/src/tests/endpoints/test_repos.py b/src/tests/endpoints/test_repos.py index 3ae9f53..b312279 100644 --- a/src/tests/endpoints/test_repos.py +++ b/src/tests/endpoints/test_repos.py @@ -256,13 +256,13 @@ async def test_fetch_guidelines_from_repo( @pytest.mark.parametrize( ("user_idx", "repo_id", "payload", "status_code", "status_detail"), [ - (None, 12345, {"guideline_ids": [2]}, 401, "Not authenticated"), - (0, 1, {"guideline_ids": [2]}, 404, ""), + (None, 12345, {"guideline_ids": [1]}, 401, "Not authenticated"), + (0, 100, {"guideline_ids": [1]}, 404, "Table Repository has no corresponding entry."), (0, 12345, {"guideline_ids": [1, 2]}, 422, None), - (0, 12345, {"guideline_ids": [2, 2]}, 422, None), - (0, 12345, {"guideline_ids": [2]}, 200, None), - (0, 123456, {"guideline_ids": [2]}, 422, None), - (1, 12345, {"guideline_ids": [2]}, 200, None), + (0, 12345, {"guideline_ids": [1, 1]}, 422, None), + (0, 12345, {"guideline_ids": [1]}, 200, None), + (0, 123456, {"guideline_ids": [1]}, 422, None), + (1, 12345, {"guideline_ids": [1]}, 200, None), ], ) @pytest.mark.asyncio() @@ -279,11 +279,14 @@ async def test_reorder_repo_guidelines( if isinstance(user_idx, int): auth = await pytest.get_token(USER_TABLE[user_idx]["id"], USER_TABLE[user_idx]["scope"].split()) - response = await async_client.put(f"/repos/{repo_id}/guidelines/order", headers=auth) - assert response.status_code == status_code, print(response.__dict__) + response = await async_client.put(f"/repos/{repo_id}/guidelines/order", json=payload, headers=auth) + assert response.status_code == status_code, print(response.json()) if isinstance(status_detail, str): assert response.json()["detail"] == status_detail if response.status_code // 100 == 2: - assert response.json() == [ - {**entry, "order": order} for entry, order in zip(GUIDELINE_TABLE, payload["guideline_ids"]) + assert [{k: v for k, v in entry.items() if k not in {"updated_at", "order"}} for entry in response.json()] == [ + {k: v for k, v in entry.items() if k not in {"updated_at", "order"}} for entry in GUIDELINE_TABLE + ] + assert [entry["order"] for entry in response.json()] == [ + payload["guideline_ids"].index(entry["id"]) for entry in GUIDELINE_TABLE ] From cbc3cbf7a5ed5a53b9d78809f8bb914d6ef3c7b7 Mon Sep 17 00:00:00 2001 From: F-G Fernandez <26927750+frgfm@users.noreply.github.com> Date: Tue, 7 Nov 2023 15:41:59 +0100 Subject: [PATCH 5/5] feat: Improves request validation --- src/app/api/api_v1/endpoints/repos.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/api/api_v1/endpoints/repos.py b/src/app/api/api_v1/endpoints/repos.py index 81815d0..4db3176 100644 --- a/src/app/api/api_v1/endpoints/repos.py +++ b/src/app/api/api_v1/endpoints/repos.py @@ -68,11 +68,11 @@ async def reorder_repo_guidelines( user=Security(get_current_user, scopes=[UserScope.USER, UserScope.ADMIN]), ) -> List[Guideline]: telemetry_client.capture(user.id, event="guideline-order", properties={"repo_id": repo_id}) - # Check the repo - await repos.get(repo_id, strict=True) # Ensure all IDs are unique if len(payload.guideline_ids) != len(set(payload.guideline_ids)): raise HTTPException(status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, detail="Duplicate IDs were passed.") + # Check the repo + await repos.get(repo_id, strict=True) # Ensure all IDs are valid guideline_ids = [elt.id for elt in await guidelines.fetch_all(("repo_id", repo_id))] if set(payload.guideline_ids) != set(guideline_ids):