From 1f2c83426d8c297c0e90444bbd16691c97eabc44 Mon Sep 17 00:00:00 2001 From: sujan Date: Thu, 13 Jun 2024 18:25:37 +0545 Subject: [PATCH 01/23] feat: implemented jwt token authentication for login --- src/backend/app/auth/auth_routes.py | 62 +++++++++++++++-------------- src/backend/app/auth/osm.py | 44 ++++++++++++++++++-- src/backend/app/config.py | 33 ++++++++++++++- 3 files changed, 105 insertions(+), 34 deletions(-) diff --git a/src/backend/app/auth/auth_routes.py b/src/backend/app/auth/auth_routes.py index e3725a5f7c..5fed69048c 100644 --- a/src/backend/app/auth/auth_routes.py +++ b/src/backend/app/auth/auth_routes.py @@ -18,6 +18,7 @@ """Auth routes, to login, logout, and get user details.""" +import time from datetime import datetime, timezone from fastapi import APIRouter, Depends, HTTPException, Request, Response @@ -26,7 +27,7 @@ from sqlalchemy import text from sqlalchemy.orm import Session -from app.auth.osm import AuthUser, init_osm_auth, login_required +from app.auth.osm import AuthUser, create_access_token, init_osm_auth, login_required from app.config import settings from app.db import database from app.models.enums import HTTPStatus, UserRole @@ -83,19 +84,27 @@ async def callback(request: Request, osm_auth=Depends(init_osm_auth)): log.debug(f"Access token returned of length {len(access_token)}") response = Response(status_code=HTTPStatus.OK) + osm_user = osm_auth.deserialize_access_token(access_token) + user_data = { + "sub": f"fmtm|{osm_user['id']}", + "aud": "fmtm.localhost:8000", + "iat": int(time.time()), + "exp": int(time.time()) + 86400, + "username": osm_user["username"], + "email": osm_user.get("email"), + "img_url": osm_user.get("img_url"), + "role": UserRole.MAPPER, + } + jwt_token = create_access_token(user_data) + # Set cookie cookie_name = settings.FMTM_DOMAIN.replace(".", "_") - log.debug( - f"Setting cookie in response named '{cookie_name}' with params: " - f"max_age=31536000 | expires=31536000 | path='/' | " - f"domain={settings.FMTM_DOMAIN} | httponly=True | samesite='lax' | " - f"secure={False if settings.DEBUG else True}" - ) + response = Response(status_code=200) response.set_cookie( key=cookie_name, - value=access_token, - max_age=31536000, # OSM currently has no expiry - expires=31536000, # OSM currently has no expiry + value=jwt_token, + max_age=31536000, + expires=31536000, path="/", domain=settings.FMTM_DOMAIN, secure=False if settings.DEBUG else True, @@ -248,29 +257,24 @@ async def temp_login( Returns: Response: The response object containing the access token as a cookie. """ - access_token = settings.OSM_SVC_ACCOUNT_TOKEN - - if not access_token: - raise HTTPException( - status_code=HTTPStatus.FORBIDDEN, - detail=( - "OSM_SVC_ACCOUNT_TOKEN variable is not set. Temp login not possible." - ), - ) + username = "temp" + user_data = { + "sub": f"fmtm|{username}", + "aud": settings.FMTM_DOMAIN, + "iat": int(time.time()), + "exp": int(time.time()) + 86400, + "username": username, + "role": UserRole.MAPPER, + } + jwt_token = create_access_token(user_data) - response = Response(status_code=HTTPStatus.OK) + response = Response(status_code=200) cookie_name = settings.FMTM_DOMAIN.replace(".", "_") - log.debug( - f"Setting TEMP cookie in response named '{cookie_name}' with params: " - f"max_age=604800 | expires=604800 | path='/' | " - f"domain={settings.FMTM_DOMAIN} | httponly=True | samesite='lax' | " - f"secure={False if settings.DEBUG else True}" - ) response.set_cookie( key=cookie_name, - value=access_token, - max_age=604800, - expires=604800, # expiry set to 7 days, + value=jwt_token, + max_age=86400, + expires=86400, path="/", domain=settings.FMTM_DOMAIN, secure=False if settings.DEBUG else True, diff --git a/src/backend/app/auth/osm.py b/src/backend/app/auth/osm.py index ccd6771722..0c3037c044 100644 --- a/src/backend/app/auth/osm.py +++ b/src/backend/app/auth/osm.py @@ -21,6 +21,7 @@ import os from typing import Optional +import jwt from fastapi import Header, HTTPException, Request from loguru import logger as log from osm_login_python.core import Auth @@ -68,8 +69,6 @@ async def login_required( role=UserRole.ADMIN, ) - osm_auth = await init_osm_auth() - # Attempt extract from cookie if access token not passed if not access_token: cookie_name = settings.FMTM_DOMAIN.replace(".", "_") @@ -80,10 +79,47 @@ async def login_required( raise HTTPException(status_code=401, detail="No access token provided") try: - osm_user = osm_auth.deserialize_access_token(access_token) + token_data = verify_access_token(access_token) except ValueError as e: log.error(e) log.error("Failed to deserialise access token") raise HTTPException(status_code=401, detail="Access token not valid") from e - return AuthUser(**osm_user) + return AuthUser(**token_data) + + +def create_access_token(payload: dict) -> str: + """Generates an access token for the specified user. + + Args: + payload (dict): user data for which the access token is being generated. + + Returns: + str: The generated access token. + """ + private_key = settings.AUTH_PRIVATE_KEY + return jwt.encode(payload, str(private_key), algorithm=settings.ALGORITHM) + + +def verify_access_token(token: str): + """Verifies the access token and returns the payload if valid. + + Args: + token (str): The access token to be verified. + + Returns: + dict: The payload of the access token if verification is successful. + + Raises: + HTTPException: If the token has expired or credentials could not be validated. + """ + try: + public_key = settings.AUTH_PUBLIC_KEY + return jwt.decode(token, str(public_key), algorithms=[settings.ALGORITHM]) + except jwt.ExpiredSignatureError as e: + raise HTTPException(status_code=401, detail="Token has expired") from e + except Exception as e: + print(e) + raise HTTPException( + status_code=401, detail="Could not validate credentials" + ) from e diff --git a/src/backend/app/config.py b/src/backend/app/config.py index 19c1e814fc..7532416277 100644 --- a/src/backend/app/config.py +++ b/src/backend/app/config.py @@ -275,7 +275,38 @@ def set_raw_data_api_auth_none(cls, v: Optional[str]) -> Optional[str]: return None return v - # Used for temporary auth feature + ALGORITHM: str = "RS256" + AUTH_PRIVATE_KEY_PATH: str + AUTH_PUBLIC_KEY_PATH: str + AUTH_PRIVATE_KEY: Optional[str] = None + AUTH_PUBLIC_KEY: Optional[str] = None + + @field_validator("AUTH_PRIVATE_KEY", mode="before") + @classmethod + def load_private_key(cls, v: Optional[str], info: ValidationInfo) -> str: + """Loads the private key for authentication.""" + if v: + try: + with open(info.data.get("AUTH_PRIVATE_KEY_PATH"), "r") as f: + public_key = f.read() + return public_key + except Exception as e: + raise ValueError(f"Error reading public key: {e}") from e + return str(v) + + @field_validator("AUTH_PUBLIC_KEY", mode="before") + @classmethod + def load_public_key(cls, v: Optional[str], info: ValidationInfo) -> str: + """Loads the public key for authentication.""" + if v: + try: + with open(info.data.get("AUTH_PUBLIC_KEY_PATH"), "r") as f: + public_key = f.read() + return public_key + except Exception as e: + raise ValueError(f"Error reading public key: {e}") from e + return str(v) + OSM_SVC_ACCOUNT_TOKEN: Optional[str] = None @field_validator("OSM_SVC_ACCOUNT_TOKEN", mode="before") From 70e1eef891bc0b542fe29375bb72c1748f7dd179 Mon Sep 17 00:00:00 2001 From: sujan Date: Fri, 14 Jun 2024 11:50:24 +0545 Subject: [PATCH 02/23] fix: added audience while decoding --- src/backend/app/auth/auth_routes.py | 11 +++++++---- src/backend/app/auth/osm.py | 7 ++++++- src/backend/app/config.py | 4 ++-- 3 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/backend/app/auth/auth_routes.py b/src/backend/app/auth/auth_routes.py index 5fed69048c..e8915c32bb 100644 --- a/src/backend/app/auth/auth_routes.py +++ b/src/backend/app/auth/auth_routes.py @@ -86,10 +86,11 @@ async def callback(request: Request, osm_auth=Depends(init_osm_auth)): osm_user = osm_auth.deserialize_access_token(access_token) user_data = { + "id": osm_user["id"], "sub": f"fmtm|{osm_user['id']}", - "aud": "fmtm.localhost:8000", + "aud": settings.FMTM_DOMAIN, "iat": int(time.time()), - "exp": int(time.time()) + 86400, + "exp": int(time.time()) + 86400, # expiry set to 1 day "username": osm_user["username"], "email": osm_user.get("email"), "img_url": osm_user.get("img_url"), @@ -257,13 +258,15 @@ async def temp_login( Returns: Response: The response object containing the access token as a cookie. """ - username = "temp" + username = "Temp User" user_data = { + "id": 99, "sub": f"fmtm|{username}", "aud": settings.FMTM_DOMAIN, "iat": int(time.time()), - "exp": int(time.time()) + 86400, + "exp": int(time.time()) + 86400, # expiry set to 1 day "username": username, + "img_url": None, "role": UserRole.MAPPER, } jwt_token = create_access_token(user_data) diff --git a/src/backend/app/auth/osm.py b/src/backend/app/auth/osm.py index 0c3037c044..5c35b4dda9 100644 --- a/src/backend/app/auth/osm.py +++ b/src/backend/app/auth/osm.py @@ -115,7 +115,12 @@ def verify_access_token(token: str): """ try: public_key = settings.AUTH_PUBLIC_KEY - return jwt.decode(token, str(public_key), algorithms=[settings.ALGORITHM]) + return jwt.decode( + token, + str(public_key), + algorithms=[settings.ALGORITHM], + audience=settings.FMTM_DOMAIN, + ) except jwt.ExpiredSignatureError as e: raise HTTPException(status_code=401, detail="Token has expired") from e except Exception as e: diff --git a/src/backend/app/config.py b/src/backend/app/config.py index 7532416277..bd9ec6ee58 100644 --- a/src/backend/app/config.py +++ b/src/backend/app/config.py @@ -285,7 +285,7 @@ def set_raw_data_api_auth_none(cls, v: Optional[str]) -> Optional[str]: @classmethod def load_private_key(cls, v: Optional[str], info: ValidationInfo) -> str: """Loads the private key for authentication.""" - if v: + if not v: try: with open(info.data.get("AUTH_PRIVATE_KEY_PATH"), "r") as f: public_key = f.read() @@ -298,7 +298,7 @@ def load_private_key(cls, v: Optional[str], info: ValidationInfo) -> str: @classmethod def load_public_key(cls, v: Optional[str], info: ValidationInfo) -> str: """Loads the public key for authentication.""" - if v: + if not v: try: with open(info.data.get("AUTH_PUBLIC_KEY_PATH"), "r") as f: public_key = f.read() From 3febc7c24db0bc4d0f97a1724ff3554b47fe1b8d Mon Sep 17 00:00:00 2001 From: sujan Date: Fri, 14 Jun 2024 15:26:04 +0545 Subject: [PATCH 03/23] feat: added pyjwt , updated user id to existing svcfmtm osmid --- src/backend/app/auth/auth_routes.py | 2 +- src/backend/pdm.lock | 12 +++++++++++- src/backend/pyproject.toml | 1 + 3 files changed, 13 insertions(+), 2 deletions(-) diff --git a/src/backend/app/auth/auth_routes.py b/src/backend/app/auth/auth_routes.py index e8915c32bb..9fd26f65c7 100644 --- a/src/backend/app/auth/auth_routes.py +++ b/src/backend/app/auth/auth_routes.py @@ -260,7 +260,7 @@ async def temp_login( """ username = "Temp User" user_data = { - "id": 99, + "id": 20386219, "sub": f"fmtm|{username}", "aud": settings.FMTM_DOMAIN, "iat": int(time.time()), diff --git a/src/backend/pdm.lock b/src/backend/pdm.lock index 8b9ccf8c0c..ed8c3854e9 100644 --- a/src/backend/pdm.lock +++ b/src/backend/pdm.lock @@ -5,7 +5,7 @@ groups = ["default", "debug", "dev", "docs", "test", "monitoring"] strategy = ["cross_platform"] lock_version = "4.4.1" -content_hash = "sha256:3dfb8a9f5d6fb72d99be33d0576d5649f08da14be9108063f0dc1d1352e6bf51" +content_hash = "sha256:7e79ebd0cb22b648c1fbe41b91f1a2f0f9c8221f47eabe38d77f828e4ea19924" [[package]] name = "aiohttp" @@ -2154,6 +2154,16 @@ files = [ {file = "pyinstrument-4.6.1.tar.gz", hash = "sha256:f4731b27121350f5a983d358d2272fe3df2f538aed058f57217eef7801a89288"}, ] +[[package]] +name = "pyjwt" +version = "2.8.0" +requires_python = ">=3.7" +summary = "JSON Web Token implementation in Python" +files = [ + {file = "PyJWT-2.8.0-py3-none-any.whl", hash = "sha256:59127c392cc44c2da5bb3192169a91f429924e17aff6534d70fdc02ab3e04320"}, + {file = "PyJWT-2.8.0.tar.gz", hash = "sha256:57e28d156e3d5c10088e0c68abb90bfac3df82b40a71bd0daa20c65ccd5c23de"}, +] + [[package]] name = "pymdown-extensions" version = "10.7" diff --git a/src/backend/pyproject.toml b/src/backend/pyproject.toml index 675141d3c5..aae06b42eb 100644 --- a/src/backend/pyproject.toml +++ b/src/backend/pyproject.toml @@ -49,6 +49,7 @@ dependencies = [ "osm-fieldwork==0.11.2", "osm-rawdata==0.3.0", "fmtm-splitter==1.2.2", + "pyjwt>=2.8.0", ] requires-python = ">=3.10" readme = "../../README.md" From ff5859cfb8385af0f5bad8a71b666ebc61d539a1 Mon Sep 17 00:00:00 2001 From: sujan Date: Fri, 14 Jun 2024 17:28:04 +0545 Subject: [PATCH 04/23] feat: change username to svcfmtm --- src/backend/app/auth/auth_routes.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/app/auth/auth_routes.py b/src/backend/app/auth/auth_routes.py index 9fd26f65c7..484e590f9f 100644 --- a/src/backend/app/auth/auth_routes.py +++ b/src/backend/app/auth/auth_routes.py @@ -258,7 +258,7 @@ async def temp_login( Returns: Response: The response object containing the access token as a cookie. """ - username = "Temp User" + username = "svcfmtm" user_data = { "id": 20386219, "sub": f"fmtm|{username}", From 1d090e5a7b9f20f62ad1e80f4f499a87d9a5bbc3 Mon Sep 17 00:00:00 2001 From: sujan Date: Tue, 25 Jun 2024 22:27:55 +0545 Subject: [PATCH 05/23] feat: implemented refresh token and updated dependency lock file --- src/backend/app/auth/auth_routes.py | 81 ++++++++++++------------ src/backend/app/auth/osm.py | 97 ++++++++++++++++++++++++----- src/backend/pdm.lock | 2 +- 3 files changed, 124 insertions(+), 56 deletions(-) diff --git a/src/backend/app/auth/auth_routes.py b/src/backend/app/auth/auth_routes.py index 484e590f9f..b5b069293e 100644 --- a/src/backend/app/auth/auth_routes.py +++ b/src/backend/app/auth/auth_routes.py @@ -27,10 +27,19 @@ from sqlalchemy import text from sqlalchemy.orm import Session -from app.auth.osm import AuthUser, create_access_token, init_osm_auth, login_required +from app.auth.osm import ( + AuthUser, + create_tokens, + extract_refresh_token_from_cookie, + init_osm_auth, + login_required, + refresh_access_token, + set_cookies, + verify_token, +) from app.config import settings from app.db import database -from app.models.enums import HTTPStatus, UserRole +from app.models.enums import UserRole router = APIRouter( prefix="/auth", @@ -82,7 +91,6 @@ async def callback(request: Request, osm_auth=Depends(init_osm_auth)): # Get access token access_token = osm_auth.callback(callback_url).get("access_token") log.debug(f"Access token returned of length {len(access_token)}") - response = Response(status_code=HTTPStatus.OK) osm_user = osm_auth.deserialize_access_token(access_token) user_data = { @@ -96,23 +104,8 @@ async def callback(request: Request, osm_auth=Depends(init_osm_auth)): "img_url": osm_user.get("img_url"), "role": UserRole.MAPPER, } - jwt_token = create_access_token(user_data) - - # Set cookie - cookie_name = settings.FMTM_DOMAIN.replace(".", "_") - response = Response(status_code=200) - response.set_cookie( - key=cookie_name, - value=jwt_token, - max_age=31536000, - expires=31536000, - path="/", - domain=settings.FMTM_DOMAIN, - secure=False if settings.DEBUG else True, - httponly=True, - samesite="lax", - ) - return response + access_token, refresh_token = create_tokens(user_data) + return set_cookies(access_token, refresh_token) @router.get("/logout/") @@ -231,15 +224,35 @@ async def my_data( return await get_or_create_user(db, user_data) -@router.get("/introspect", response_model=AuthUser) -async def check_login( - user_data: AuthUser = Depends(login_required), +@router.get("/refresh") +async def refresh_token( + request: Request, ): """Verifies the validity of login cookies. Returns True if authenticated, False otherwise. """ - return user_data + refresh_token = extract_refresh_token_from_cookie(request) + if not refresh_token: + raise HTTPException(status_code=401, detail="No tokens provided") + + token_data = verify_token(refresh_token) + access_token = refresh_access_token(token_data) + + response = Response(status_code=200) + cookie_name = settings.FMTM_DOMAIN.replace(".", "_") + response.set_cookie( + key=cookie_name, + value=access_token, + max_age=86400, + expires=86400, + path="/", + domain=settings.FMTM_DOMAIN, + secure=False if settings.DEBUG else True, + httponly=True, + samesite="lax", + ) + return response @router.get("/temp-login") @@ -264,24 +277,10 @@ async def temp_login( "sub": f"fmtm|{username}", "aud": settings.FMTM_DOMAIN, "iat": int(time.time()), - "exp": int(time.time()) + 86400, # expiry set to 1 day + "exp": int(time.time()) + 86400 * 7, # expiry set to 7 days "username": username, "img_url": None, "role": UserRole.MAPPER, } - jwt_token = create_access_token(user_data) - - response = Response(status_code=200) - cookie_name = settings.FMTM_DOMAIN.replace(".", "_") - response.set_cookie( - key=cookie_name, - value=jwt_token, - max_age=86400, - expires=86400, - path="/", - domain=settings.FMTM_DOMAIN, - secure=False if settings.DEBUG else True, - httponly=True, - samesite="lax", - ) - return response + access_token, refresh_token = create_tokens(user_data) + return set_cookies(access_token, refresh_token) diff --git a/src/backend/app/auth/osm.py b/src/backend/app/auth/osm.py index 5c35b4dda9..7e26d09eb2 100644 --- a/src/backend/app/auth/osm.py +++ b/src/backend/app/auth/osm.py @@ -19,10 +19,11 @@ """Auth methods related to OSM OAuth2.""" import os +import time from typing import Optional import jwt -from fastapi import Header, HTTPException, Request +from fastapi import Header, HTTPException, Request, Response from loguru import logger as log from osm_login_python.core import Auth from pydantic import BaseModel, ConfigDict @@ -71,15 +72,13 @@ async def login_required( # Attempt extract from cookie if access token not passed if not access_token: - cookie_name = settings.FMTM_DOMAIN.replace(".", "_") - log.debug(f"Extracting token from cookie {cookie_name}") - access_token = request.cookies.get(cookie_name) + access_token = extract_token_from_cookie(request) if not access_token: raise HTTPException(status_code=401, detail="No access token provided") try: - token_data = verify_access_token(access_token) + token_data = verify_token(access_token) except ValueError as e: log.error(e) log.error("Failed to deserialise access token") @@ -88,20 +87,54 @@ async def login_required( return AuthUser(**token_data) -def create_access_token(payload: dict) -> str: - """Generates an access token for the specified user. +def extract_token_from_cookie(request: Request) -> str: + """Extract access token from cookies.""" + cookie_name = settings.FMTM_DOMAIN.replace(".", "_") + log.debug(f"Extracting token from cookie {cookie_name}") + return request.cookies.get(cookie_name) + + +def extract_refresh_token_from_cookie(request: Request) -> str: + """Extract refresh token from cookies.""" + cookie_name = settings.FMTM_DOMAIN.replace(".", "_") + return request.cookies.get(f"{cookie_name}_refresh") + + +def create_tokens(payload: dict) -> tuple[str, str]: + """Generates tokens for the specified user. Args: payload (dict): user data for which the access token is being generated. Returns: - str: The generated access token. + Tuple: The generated access tokens. """ + access_token_payload = payload + access_token_payload["exp"] = ( + int(time.time()) + 86400 + ) # set access token expiry to 1 day + private_key = settings.AUTH_PRIVATE_KEY + access_token = jwt.encode( + access_token_payload, str(private_key), algorithm=settings.ALGORITHM + ) + refresh_token = jwt.encode(payload, str(private_key), algorithm=settings.ALGORITHM) + return access_token, refresh_token + + +def refresh_access_token(payload: dict) -> str: + """Generate a new access token.""" + access_token_payload = payload + access_token_payload["exp"] = ( + int(time.time()) + 60 + ) # Access token valid for 15 minutes + private_key = settings.AUTH_PRIVATE_KEY - return jwt.encode(payload, str(private_key), algorithm=settings.ALGORITHM) + return jwt.encode( + access_token_payload, str(private_key), algorithm=settings.ALGORITHM + ) -def verify_access_token(token: str): +def verify_token(token: str): """Verifies the access token and returns the payload if valid. Args: @@ -113,8 +146,8 @@ def verify_access_token(token: str): Raises: HTTPException: If the token has expired or credentials could not be validated. """ + public_key = settings.AUTH_PUBLIC_KEY try: - public_key = settings.AUTH_PUBLIC_KEY return jwt.decode( token, str(public_key), @@ -122,9 +155,45 @@ def verify_access_token(token: str): audience=settings.FMTM_DOMAIN, ) except jwt.ExpiredSignatureError as e: - raise HTTPException(status_code=401, detail="Token has expired") from e + raise HTTPException(status_code=401, detail="Refresh token has expired") from e except Exception as e: - print(e) raise HTTPException( - status_code=401, detail="Could not validate credentials" + status_code=401, detail="Could not validate refresh token" ) from e + + +def set_cookies(access_token: str, refresh_token: str): + """Sets cookies for the access token and refresh token. + + Args: + access_token (str): The access token to be stored in the cookie. + refresh_token (str): The refresh token to be stored in the cookie. + + Returns: + Response: A response object with the cookies set. + """ + response = Response(status_code=200) + cookie_name = settings.FMTM_DOMAIN.replace(".", "_") + response.set_cookie( + key=cookie_name, + value=access_token, + max_age=86400, + expires=86400, # expiry set for 1 day + path="/", + domain=settings.FMTM_DOMAIN, + secure=False if settings.DEBUG else True, + httponly=True, + samesite="lax", + ) + response.set_cookie( + key=f"{cookie_name}_refresh", + value=refresh_token, + max_age=86400 * 7, + expires=86400 * 7, # expiry set for 7 days + path="/", + domain=settings.FMTM_DOMAIN, + secure=False if settings.DEBUG else True, + httponly=True, + samesite="lax", + ) + return response diff --git a/src/backend/pdm.lock b/src/backend/pdm.lock index 4b6fdbd0aa..c9cca61084 100644 --- a/src/backend/pdm.lock +++ b/src/backend/pdm.lock @@ -5,7 +5,7 @@ groups = ["default", "debug", "dev", "docs", "test", "monitoring"] strategy = ["cross_platform"] lock_version = "4.4.1" -content_hash = "sha256:7e79ebd0cb22b648c1fbe41b91f1a2f0f9c8221f47eabe38d77f828e4ea19924" +content_hash = "sha256:910d5a4b4d021a0aa3688ce07c91a51f4c7a636e3f257adde5f6eaeae83d0e9d" [[package]] name = "aiohttp" From a5ab004f9aaccb62c6a1435212d28e95719bf090 Mon Sep 17 00:00:00 2001 From: sujan Date: Tue, 25 Jun 2024 23:08:35 +0545 Subject: [PATCH 06/23] fix: return user data from refresh endpoint --- src/backend/app/auth/auth_routes.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/backend/app/auth/auth_routes.py b/src/backend/app/auth/auth_routes.py index b5b069293e..482c708029 100644 --- a/src/backend/app/auth/auth_routes.py +++ b/src/backend/app/auth/auth_routes.py @@ -226,8 +226,8 @@ async def my_data( @router.get("/refresh") async def refresh_token( - request: Request, -): + request: Request, user_data: AuthUser = Depends(login_required) +) -> AuthUser: """Verifies the validity of login cookies. Returns True if authenticated, False otherwise. @@ -238,8 +238,7 @@ async def refresh_token( token_data = verify_token(refresh_token) access_token = refresh_access_token(token_data) - - response = Response(status_code=200) + response = JSONResponse(content=user_data.dict(), status_code=200) cookie_name = settings.FMTM_DOMAIN.replace(".", "_") response.set_cookie( key=cookie_name, From 18af2e4710eaade78268245846cae06db8d30615 Mon Sep 17 00:00:00 2001 From: sujan Date: Thu, 27 Jun 2024 11:34:08 +0545 Subject: [PATCH 07/23] changed img_url -> picture, retrieve id from sub field --- src/backend/app/auth/auth_routes.py | 10 ++++------ src/backend/app/auth/osm.py | 21 +++++++++++++++++---- src/frontend/src/App.tsx | 2 +- src/frontend/src/store/types/ILogin.ts | 2 +- 4 files changed, 23 insertions(+), 12 deletions(-) diff --git a/src/backend/app/auth/auth_routes.py b/src/backend/app/auth/auth_routes.py index 482c708029..1403d97164 100644 --- a/src/backend/app/auth/auth_routes.py +++ b/src/backend/app/auth/auth_routes.py @@ -94,14 +94,13 @@ async def callback(request: Request, osm_auth=Depends(init_osm_auth)): osm_user = osm_auth.deserialize_access_token(access_token) user_data = { - "id": osm_user["id"], "sub": f"fmtm|{osm_user['id']}", "aud": settings.FMTM_DOMAIN, "iat": int(time.time()), "exp": int(time.time()) + 86400, # expiry set to 1 day "username": osm_user["username"], "email": osm_user.get("email"), - "img_url": osm_user.get("img_url"), + "picture": osm_user.get("img_url"), "role": UserRole.MAPPER, } access_token, refresh_token = create_tokens(user_data) @@ -157,7 +156,7 @@ async def get_or_create_user( { "user_id": user_data.id, "username": user_data.username, - "profile_img": user_data.img_url, + "profile_img": user_data.picture, "role": role, "mapping_level": "BEGINNER", "current_date": datetime.now(timezone.utc), @@ -272,13 +271,12 @@ async def temp_login( """ username = "svcfmtm" user_data = { - "id": 20386219, - "sub": f"fmtm|{username}", + "sub": "fmtm|20386219", "aud": settings.FMTM_DOMAIN, "iat": int(time.time()), "exp": int(time.time()) + 86400 * 7, # expiry set to 7 days "username": username, - "img_url": None, + "picture": None, "role": UserRole.MAPPER, } access_token, refresh_token = create_tokens(user_data) diff --git a/src/backend/app/auth/osm.py b/src/backend/app/auth/osm.py index 7e26d09eb2..5f15396378 100644 --- a/src/backend/app/auth/osm.py +++ b/src/backend/app/auth/osm.py @@ -26,7 +26,7 @@ from fastapi import Header, HTTPException, Request, Response from loguru import logger as log from osm_login_python.core import Auth -from pydantic import BaseModel, ConfigDict +from pydantic import BaseModel, ConfigDict, PrivateAttr, computed_field from app.config import settings from app.models.enums import UserRole @@ -41,11 +41,24 @@ class AuthUser(BaseModel): model_config = ConfigDict(use_enum_values=True) - id: int username: str - img_url: Optional[str] = None + picture: Optional[str] = None role: Optional[UserRole] = UserRole.MAPPER + _sub: str = PrivateAttr() # it won't return this field + + def __init__(self, sub: str, **data): + """Initializes the AuthUser class.""" + super().__init__(**data) + self._sub = sub + + @computed_field + @property + def id(self) -> Optional[str]: + """Compute id from sub field.""" + sub = self._sub + return (sub.split("|"))[1] + async def init_osm_auth(): """Initialise Auth object from osm-login-python.""" @@ -65,7 +78,7 @@ async def login_required( """Dependency to inject into endpoints requiring login.""" if settings.DEBUG: return AuthUser( - id=1, + sub="fmtm|1", username="localadmin", role=UserRole.ADMIN, ) diff --git a/src/frontend/src/App.tsx b/src/frontend/src/App.tsx index ea321910b5..083e51608c 100755 --- a/src/frontend/src/App.tsx +++ b/src/frontend/src/App.tsx @@ -19,7 +19,7 @@ const CheckLoginState = () => { const authDetails = CoreModules.useAppSelector((state) => state.login.authDetails); const checkIfUserLoginValid = () => { - fetch(`${import.meta.env.VITE_API_URL}/auth/introspect`, { credentials: 'include' }) + fetch(`${import.meta.env.VITE_API_URL}/auth/refresh`, { credentials: 'include' }) .then((resp) => { if (resp.status !== 200) { dispatch(LoginActions.signOut()); diff --git a/src/frontend/src/store/types/ILogin.ts b/src/frontend/src/store/types/ILogin.ts index cf8a0798aa..93f19d25a4 100644 --- a/src/frontend/src/store/types/ILogin.ts +++ b/src/frontend/src/store/types/ILogin.ts @@ -5,7 +5,7 @@ export type LoginStateTypes = { type authDetailsType = { id: string; - img_url: string; + picture: string; role: string; username: string; // sessionToken: string | null; From 39fe7c8de1fdb15576d132b211490b9d5244b8fa Mon Sep 17 00:00:00 2001 From: sujan Date: Thu, 27 Jun 2024 11:44:54 +0545 Subject: [PATCH 08/23] feat: exception handling if osm deserialization fails --- src/backend/app/auth/auth_routes.py | 96 ++++++++++++++++------------- 1 file changed, 53 insertions(+), 43 deletions(-) diff --git a/src/backend/app/auth/auth_routes.py b/src/backend/app/auth/auth_routes.py index 1403d97164..8dae35695b 100644 --- a/src/backend/app/auth/auth_routes.py +++ b/src/backend/app/auth/auth_routes.py @@ -39,7 +39,7 @@ ) from app.config import settings from app.db import database -from app.models.enums import UserRole +from app.models.enums import HTTPStatus, UserRole router = APIRouter( prefix="/auth", @@ -83,28 +83,32 @@ async def callback(request: Request, osm_auth=Depends(init_osm_auth)): Returns: access_token (string): The access token provided by the login URL request. """ - log.debug(f"Callback url requested: {request.url}") - - # Enforce https callback url for openstreetmap.org - callback_url = str(request.url).replace("http://", "https://") - - # Get access token - access_token = osm_auth.callback(callback_url).get("access_token") - log.debug(f"Access token returned of length {len(access_token)}") - - osm_user = osm_auth.deserialize_access_token(access_token) - user_data = { - "sub": f"fmtm|{osm_user['id']}", - "aud": settings.FMTM_DOMAIN, - "iat": int(time.time()), - "exp": int(time.time()) + 86400, # expiry set to 1 day - "username": osm_user["username"], - "email": osm_user.get("email"), - "picture": osm_user.get("img_url"), - "role": UserRole.MAPPER, - } - access_token, refresh_token = create_tokens(user_data) - return set_cookies(access_token, refresh_token) + try: + log.debug(f"Callback url requested: {request.url}") + + # Enforce https callback url for openstreetmap.org + callback_url = str(request.url).replace("http://", "https://") + + # Get access token + access_token = osm_auth.callback(callback_url).get("access_token") + log.debug(f"Access token returned of length {len(access_token)}") + osm_user = osm_auth.deserialize_access_token(access_token) + user_data = { + "sub": f"fmtm|{osm_user['id']}", + "aud": settings.FMTM_DOMAIN, + "iat": int(time.time()), + "exp": int(time.time()) + 86400, # expiry set to 1 day + "username": osm_user["username"], + "email": osm_user.get("email"), + "picture": osm_user.get("img_url"), + "role": UserRole.MAPPER, + } + access_token, refresh_token = create_tokens(user_data) + return set_cookies(access_token, refresh_token) + except Exception as e: + raise HTTPException( + status_code=HTTPStatus.UNAUTHORIZED, detail=f"Invalid OSM token: {e}" + ) from e @router.get("/logout/") @@ -231,26 +235,32 @@ async def refresh_token( Returns True if authenticated, False otherwise. """ - refresh_token = extract_refresh_token_from_cookie(request) - if not refresh_token: - raise HTTPException(status_code=401, detail="No tokens provided") - - token_data = verify_token(refresh_token) - access_token = refresh_access_token(token_data) - response = JSONResponse(content=user_data.dict(), status_code=200) - cookie_name = settings.FMTM_DOMAIN.replace(".", "_") - response.set_cookie( - key=cookie_name, - value=access_token, - max_age=86400, - expires=86400, - path="/", - domain=settings.FMTM_DOMAIN, - secure=False if settings.DEBUG else True, - httponly=True, - samesite="lax", - ) - return response + try: + refresh_token = extract_refresh_token_from_cookie(request) + if not refresh_token: + raise HTTPException(status_code=401, detail="No tokens provided") + + token_data = verify_token(refresh_token) + access_token = refresh_access_token(token_data) + response = JSONResponse(content=user_data.dict(), status_code=200) + cookie_name = settings.FMTM_DOMAIN.replace(".", "_") + response.set_cookie( + key=cookie_name, + value=access_token, + max_age=86400, + expires=86400, + path="/", + domain=settings.FMTM_DOMAIN, + secure=False if settings.DEBUG else True, + httponly=True, + samesite="lax", + ) + return response + except Exception as e: + raise HTTPException( + status_code=HTTPStatus.BAD_REQUEST, + detail=f"fail to refresh the access token: {e}", + ) from e @router.get("/temp-login") From 49f1e6da585d9225d2363977c2716efe5e6b5e34 Mon Sep 17 00:00:00 2001 From: spwoodcock Date: Mon, 1 Jul 2024 19:51:20 +0100 Subject: [PATCH 09/23] build: relock deps after pyjwt added --- src/backend/pdm.lock | 2 +- src/backend/pyproject.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/backend/pdm.lock b/src/backend/pdm.lock index c9cca61084..cf702f2b6a 100644 --- a/src/backend/pdm.lock +++ b/src/backend/pdm.lock @@ -5,7 +5,7 @@ groups = ["default", "debug", "dev", "docs", "test", "monitoring"] strategy = ["cross_platform"] lock_version = "4.4.1" -content_hash = "sha256:910d5a4b4d021a0aa3688ce07c91a51f4c7a636e3f257adde5f6eaeae83d0e9d" +content_hash = "sha256:dc843ea7774920cf4858affe48fec8310148cea089b2877e190e12732e754e90" [[package]] name = "aiohttp" diff --git a/src/backend/pyproject.toml b/src/backend/pyproject.toml index 3868443a00..ecff63e1d1 100644 --- a/src/backend/pyproject.toml +++ b/src/backend/pyproject.toml @@ -45,11 +45,11 @@ dependencies = [ "sozipfile==0.3.2", "cryptography>=42.0.1", "defusedxml>=0.7.1", + "pyjwt>=2.8.0", "osm-login-python==1.0.3", "osm-fieldwork==0.12.4", "osm-rawdata==0.3.0", "fmtm-splitter==1.2.2", - "pyjwt>=2.8.0", ] requires-python = ">=3.10" readme = "../../README.md" From e4e46c95def67b1f79e07b39f8d38d876524f637 Mon Sep 17 00:00:00 2001 From: spwoodcock Date: Mon, 1 Jul 2024 19:52:22 +0100 Subject: [PATCH 10/23] refactor: remove auth key path variables, require passing string values --- src/backend/app/config.py | 48 ++++++--------------------------------- 1 file changed, 7 insertions(+), 41 deletions(-) diff --git a/src/backend/app/config.py b/src/backend/app/config.py index bd9ec6ee58..3f0ca60df6 100644 --- a/src/backend/app/config.py +++ b/src/backend/app/config.py @@ -270,52 +270,18 @@ def configure_s3_download_root(cls, v: Optional[str], info: ValidationInfo) -> s @field_validator("RAW_DATA_API_AUTH_TOKEN", mode="before") @classmethod def set_raw_data_api_auth_none(cls, v: Optional[str]) -> Optional[str]: - """Set RAW_DATA_API_AUTH_TOKEN to None if set to empty string.""" + """Set RAW_DATA_API_AUTH_TOKEN to None if set to empty string. + + This variable is used by HOTOSM to track raw-data-api usage. + It is not required if running your own instance. + """ if v == "": return None return v ALGORITHM: str = "RS256" - AUTH_PRIVATE_KEY_PATH: str - AUTH_PUBLIC_KEY_PATH: str - AUTH_PRIVATE_KEY: Optional[str] = None - AUTH_PUBLIC_KEY: Optional[str] = None - - @field_validator("AUTH_PRIVATE_KEY", mode="before") - @classmethod - def load_private_key(cls, v: Optional[str], info: ValidationInfo) -> str: - """Loads the private key for authentication.""" - if not v: - try: - with open(info.data.get("AUTH_PRIVATE_KEY_PATH"), "r") as f: - public_key = f.read() - return public_key - except Exception as e: - raise ValueError(f"Error reading public key: {e}") from e - return str(v) - - @field_validator("AUTH_PUBLIC_KEY", mode="before") - @classmethod - def load_public_key(cls, v: Optional[str], info: ValidationInfo) -> str: - """Loads the public key for authentication.""" - if not v: - try: - with open(info.data.get("AUTH_PUBLIC_KEY_PATH"), "r") as f: - public_key = f.read() - return public_key - except Exception as e: - raise ValueError(f"Error reading public key: {e}") from e - return str(v) - - OSM_SVC_ACCOUNT_TOKEN: Optional[str] = None - - @field_validator("OSM_SVC_ACCOUNT_TOKEN", mode="before") - @classmethod - def set_osm_svc_account_none(cls, v: Optional[str]) -> Optional[str]: - """Set OSM_SVC_ACCOUNT_TOKEN to None if set to empty string.""" - if v == "": - return None - return v + AUTH_PRIVATE_KEY: str + AUTH_PUBLIC_KEY: str MONITORING: Optional[MonitoringTypes] = None From 0eaaf6f95f1b24770750d49c7aa5701100b92ba1 Mon Sep 17 00:00:00 2001 From: spwoodcock Date: Mon, 1 Jul 2024 19:57:00 +0100 Subject: [PATCH 11/23] docs: update install docs with info for gen priv/pub keys --- INSTALL.md | 4 +++ docs/dev/Setup.md | 89 ----------------------------------------------- 2 files changed, 4 insertions(+), 89 deletions(-) diff --git a/INSTALL.md b/INSTALL.md index f6e93cd5b0..b1eec18020 100644 --- a/INSTALL.md +++ b/INSTALL.md @@ -126,6 +126,10 @@ bash scripts/gen-env.sh > Note: If extra cors origins are required for testing, the variable > `EXTRA_CORS_ORIGINS` is a set of comma separated strings, e.g.: > +> +> Note: It is possible to generate the auth pub/priv key manually using: +> openssl genrsa -out fmtm-private.pem 4096 +> openssl rsa -in fmtm-private.pem -pubout -out fmtm-private.pem ### Start the API with Docker diff --git a/docs/dev/Setup.md b/docs/dev/Setup.md index fd4e22383f..d5295f6da5 100644 --- a/docs/dev/Setup.md +++ b/docs/dev/Setup.md @@ -13,8 +13,6 @@ - [FMTM frontend](#fmtm-frontend) - [FMTM backend](#fmtm-backend) - [Prerequisites for Contribution](#prerequisites-for-contribution) - - [Development: Setup Your Local Environment](#setup-your-local-environment) - - [Verify Setup](#verify-setup) - [Start Developing](#start-developing) ## Overview @@ -312,93 +310,6 @@ your changes and request that they be merged into the main codebase. That's it! You've now contributed to the Field Mapping Tasking Manager. -### Setup Your Local Environment - -These steps are essential to run and test your code! - -#### 1. Setup OSM OAUTH 2.0 - -The FMTM uses OAUTH2 with OSM to authenticate users. To properly configure your -FMTM project, you will need to create keys for OSM. - -1. [Login to OSM](https://www.openstreetmap.org/login) (_If you do not have an - account yet, click the signup button at the top navigation bar to create one_). - Click the drop down arrow on the extreme right of the navigation bar and - select My Settings. - -2. Register your FMTM instance to OAuth 2 applications. Put your login redirect - url as `http://127.0.0.1:7051/osmauth/`, For Production replace the URL as - production API Url - - > Note: `127.0.0.1` is required instead of `localhost` due to OSM restrictions. - - image - -3. Right now read user preferences permission is enough later on fmtm may need - permission to modify the map option which should be updated on OSM_SCOPE - variable on .env , Keep read_prefs for now. - -4. Now Copy your Client ID and Client Secret. Put them in the `OSM_CLIENT_ID` - and `OSM_CLIENT_SECRET` field of your `.env` file - -##### 2. Create an `.env` File - -Environmental variables are used throughout this project. -To get started, create `.env` file in the top level dir, -a sample is located at `.env.example`. - -This can be created interactively by running: - -```bash -bash scripts/gen-env.sh -``` - -> Note: If extra cors origins are required for testing, the variable -> `EXTRA_CORS_ORIGINS` is a set of comma separated strings, e.g.: -> - -### Verify Setup - -#### Check Deployment - -For details on how to run this project locally for development, please look at: -[Backend Docs](https://docs.fmtm.dev/dev/Backend) - -#### Check Authentication - -Once you have deployed, you will need to check that you can properly -authenticate. - -1. Navigate to `http://api.fmtm.localhost:7050/docs` - - Three endpoints are responsible for oauth - image - -2. Select the `/auth/osm-login/` endpoint, click `Try it out` and then - `Execute`. - This would give you the Login URL where you can supply your osm username - and password. - - Your response should look like this: - - ```json - { - "login_url": "https://www.openstreetmap.org/oauth2/authorize/?response_type=code&client_id=xxxx" - } - ``` - - Now copy and paste your login_url in a new tab. You would be redirected to - OSM for your LOGIN. Give FMTM the necessary permission. - - After a successful login, you will get your `access_token` for FMTM, Copy - it. Now, you can use it for rest of the endpoints that needs authorization. - -3. Check your access token: Select the `/auth/me/` endpoint and click - `Try it out`. - Pass in the `access_token` you copied in the previous step into the - `access-token` field and click `Execute`. You should get your osm id, - username and profile picture id. - ### Start Developing Don't forget to review the From 3493c27ce46bc4f77af077fdc22ad458504b6763 Mon Sep 17 00:00:00 2001 From: spwoodcock Date: Mon, 1 Jul 2024 19:57:24 +0100 Subject: [PATCH 12/23] docs: update all docs for AUTH_PUBLIC_KEY vars --- .env.example | 3 ++- .gitignore | 3 +++ chart/README.md | 6 +++++- docs/dev/Troubleshooting.md | 5 +++++ 4 files changed, 15 insertions(+), 2 deletions(-) diff --git a/.env.example b/.env.example index 0cf6d900fb..4bc5c33e91 100644 --- a/.env.example +++ b/.env.example @@ -11,6 +11,8 @@ ENCRYPTION_KEY=${ENCRYPTION_KEY:-"pIxxYIXe4oAVHI36lTveyc97FKK2O_l2VHeiuqU-K_4="} FMTM_DOMAIN=${FMTM_DOMAIN:-"fmtm.localhost"} FMTM_DEV_PORT=${FMTM_DEV_PORT:-7050} CERT_EMAIL=${CERT_EMAIL} +AUTH_PUBLIC_KEY=${AUTH_PUBLIC_KEY} +AUTH_PRIVATE_KEY=${AUTH_PRIVATE_KEY} # Use API_PREFIX if running behind a proxy subpath (e.g. /api) API_PREFIX=${API_PREFIX:-/} @@ -21,7 +23,6 @@ OSM_URL=${OSM_URL:-"https://www.openstreetmap.org"} OSM_SCOPE=${OSM_SCOPE:-"read_prefs"} OSM_LOGIN_REDIRECT_URI="http${FMTM_DOMAIN:+s}://${FMTM_DOMAIN:-127.0.0.1:7051}/osmauth/" OSM_SECRET_KEY=${OSM_SECRET_KEY} -OSM_SVC_ACCOUNT_TOKEN=${OSM_SVC_ACCOUNT_TOKEN} ### S3 File Storage ### S3_ENDPOINT=${S3_ENDPOINT:-"http://s3:9000"} diff --git a/.gitignore b/.gitignore index 07934d6068..6c83c3b1eb 100644 --- a/.gitignore +++ b/.gitignore @@ -93,3 +93,6 @@ envsubst # Chart dependencies chart/charts + +# Secrets / keys +**/**/*.pem \ No newline at end of file diff --git a/chart/README.md b/chart/README.md index 1615a50f8d..a4827300f7 100644 --- a/chart/README.md +++ b/chart/README.md @@ -50,6 +50,8 @@ kubectl - key: OSM_CLIENT_ID - key: OSM_CLIENT_SECRET - key: OSM_SECRET_KEY + - key: AUTH_PUBLIC_KEY + - key: AUTH_PRIVATE_KEY ```bash kubectl create secret generic api-fmtm-vars --namespace fmtm \ @@ -57,7 +59,9 @@ kubectl --from-literal=FMTM_DOMAIN=some.domain.com \ --from-literal=OSM_CLIENT_ID=xxxxxxx \ --from-literal=OSM_CLIENT_SECRET=xxxxxxx \ - --from-literal=OSM_SECRET_KEY=xxxxxxx + --from-literal=OSM_SECRET_KEY=xxxxxxx \ + --from-file=AUTH_PUBLIC_KEY=/path/to/pub/key.pem \ + --from-file=AUTH_PRIVATE_KEY=/path/to/priv/key.pem ``` ## Deployment diff --git a/docs/dev/Troubleshooting.md b/docs/dev/Troubleshooting.md index cf9488bd92..0c9810ee8f 100644 --- a/docs/dev/Troubleshooting.md +++ b/docs/dev/Troubleshooting.md @@ -34,6 +34,10 @@ OSM_SCOPE field required (type=value_error.missing) OSM_LOGIN_REDIRECT_URI field required (type=value_error.missing) +AUTH_PUBLIC_KEY + field required (type=value_error.missing) +AUTH_PRIVATE_KEY + field required (type=value_error.missing) ``` Then you need to set the env variables on your system. @@ -45,5 +49,6 @@ an alternative can be to feed them into the pdm command: FMTM_DOMAIN="" \ OSM_CLIENT_ID="" OSM_CLIENT_SECRET="" OSM_SECRET_KEY="" \ S3_ACCESS_KEY="" S3_SECRET_KEY="" \ +AUTH_PUBLIC_KEY="" AUTH_PRIVATE_KEY="" \ pdm run uvicorn app.main:api --host 0.0.0.0 --port 8000 ``` From 47759eee82ba2b18e87385087146e64e137880f4 Mon Sep 17 00:00:00 2001 From: spwoodcock Date: Mon, 1 Jul 2024 19:58:41 +0100 Subject: [PATCH 13/23] build: add AUTH_PUBLIC_KEY vars to gen-env.sh script --- scripts/gen-env.sh | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/scripts/gen-env.sh b/scripts/gen-env.sh index 1fbc35b874..1ee52c7468 100644 --- a/scripts/gen-env.sh +++ b/scripts/gen-env.sh @@ -342,6 +342,28 @@ check_change_port() { fi } +generate_auth_keys() { + pretty_echo "Generating Auth Keys" + + if ! AUTH_PRIVATE_KEY=$(openssl genrsa 4096 2>/dev/null); then + echo "Error generating private key. Aborting." + return 1 + fi + + if ! AUTH_PUBLIC_KEY=$(echo "$AUTH_PRIVATE_KEY" | openssl rsa -pubout 2>/dev/null); then + echo "Error generating public key. Aborting." + return 1 + fi + + # Quotes are required around key variables, else dotenv does not load + export AUTH_PRIVATE_KEY="\"$AUTH_PRIVATE_KEY\"" + export AUTH_PUBLIC_KEY="\"$AUTH_PUBLIC_KEY\"" + + echo + echo "Auth keys generated." + echo +} + generate_dotenv() { pretty_echo "Generating Dotenv File" @@ -390,6 +412,7 @@ prompt_user_gen_dotenv() { fi set_osm_credentials + generate_auth_keys generate_dotenv pretty_echo "Completed dotenv file generation" From fbb2f737ceefd24b632cda643e1e9478b5793d87 Mon Sep 17 00:00:00 2001 From: spwoodcock Date: Mon, 1 Jul 2024 20:10:39 +0100 Subject: [PATCH 14/23] refactor: exclude rsa keys from debug printed settings --- src/backend/app/config.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/backend/app/config.py b/src/backend/app/config.py index 3f0ca60df6..11ce2fe76b 100644 --- a/src/backend/app/config.py +++ b/src/backend/app/config.py @@ -302,7 +302,10 @@ def get_settings(): _settings = Settings() if _settings.DEBUG: - print("Loaded settings: " f"{_settings.model_dump()}") + print( + "Loaded settings: " + f"{_settings.model_dump(exclude=['AUTH_PRIVATE_KEY', 'AUTH_PUBLIC_KEY'])}" + ) return _settings From 5a4d9f8fc1ca2cfba9688cc22bcb1324482a5b7c Mon Sep 17 00:00:00 2001 From: spwoodcock Date: Tue, 2 Jul 2024 19:43:37 +0100 Subject: [PATCH 15/23] fix(backend): improve logic for /me endpoint upsert & return ProjectRoles OrgManagers --- src/backend/app/auth/auth_routes.py | 105 +++++++++++++-------------- src/backend/app/auth/auth_schemas.py | 64 ++++++++++++++++ 2 files changed, 114 insertions(+), 55 deletions(-) create mode 100644 src/backend/app/auth/auth_schemas.py diff --git a/src/backend/app/auth/auth_routes.py b/src/backend/app/auth/auth_routes.py index 8dae35695b..b10eb7e721 100644 --- a/src/backend/app/auth/auth_routes.py +++ b/src/backend/app/auth/auth_routes.py @@ -19,7 +19,6 @@ """Auth routes, to login, logout, and get user details.""" import time -from datetime import datetime, timezone from fastapi import APIRouter, Depends, HTTPException, Request, Response from fastapi.responses import JSONResponse @@ -27,6 +26,7 @@ from sqlalchemy import text from sqlalchemy.orm import Session +from app.auth.auth_schemas import FMTMUser from app.auth.osm import ( AuthUser, create_tokens, @@ -138,79 +138,74 @@ async def get_or_create_user( ): """Get user from User table if exists, else create.""" try: - update_sql = text( + upsert_sql = text( """ - INSERT INTO users ( + WITH upserted_user AS ( + INSERT INTO users ( id, username, profile_img, role, mapping_level, is_email_verified, is_expert, tasks_mapped, tasks_validated, tasks_invalidated, date_registered, last_validation_date - ) - VALUES ( - :user_id, :username, :profile_img, :role, - :mapping_level, FALSE, FALSE, 0, 0, 0, - :current_date, :current_date - ) - ON CONFLICT (id) - DO UPDATE SET profile_img = :profile_img; + ) VALUES ( + :user_id, :username, :profile_img, :role, :mapping_level, + FALSE, FALSE, 0, 0, 0, NOW(), NOW() + ) + ON CONFLICT (id) + DO UPDATE SET + profile_img = EXCLUDED.profile_img + RETURNING id, username, profile_img, role + ) + SELECT + u.id, u.username, u.profile_img, u.role, + array_agg( + DISTINCT om.organisation_id + ) FILTER (WHERE om.organisation_id IS NOT NULL) as orgs_managed, + jsonb_object_agg( + ur.project_id, + COALESCE(ur.role, 'MAPPER') + ) FILTER (WHERE ur.project_id IS NOT NULL) as project_roles + FROM upserted_user u + LEFT JOIN user_roles ur ON u.id = ur.user_id + LEFT JOIN organisation_managers om ON u.id = om.user_id + GROUP BY u.id, u.username, u.profile_img, u.role; """ ) - role = UserRole(user_data.role).name - db.execute( - update_sql, - { - "user_id": user_data.id, - "username": user_data.username, - "profile_img": user_data.picture, - "role": role, - "mapping_level": "BEGINNER", - "current_date": datetime.now(timezone.utc), - }, - ) - db.commit() - get_sql = text( - """ - SELECT users.*, - user_roles.project_id as project_id, - organisation_managers.organisation_id as created_org, - COALESCE(user_roles.role, 'MAPPER') as project_role - FROM users - LEFT JOIN user_roles ON users.id = user_roles.user_id - LEFT JOIN organisation_managers on users.id = organisation_managers.user_id - WHERE users.id = :user_id; - """ - ) - result = db.execute( - get_sql, - {"user_id": user_data.id}, - ) - db_user = result.first() - - user = { - "id": db_user.id, - "username": db_user.username, - "profile_img": db_user.profile_img, - "role": db_user.role, - "project_id": db_user.project_id, - "project_role": db_user.project_role, - "created_org": db_user.created_org, + parameters = { + "user_id": user_data.id, + "username": user_data.username, + "profile_img": user_data.picture or "", + "role": UserRole(user_data.role).name, + "mapping_level": "BEGINNER", } - return user + result = db.execute(upsert_sql, parameters) + db.commit() + + db_user_details = result.first() + if not db_user_details: + raise HTTPException( + status_code=HTTPStatus.NOT_FOUND, + detail=f"User ID ({user_data.id}) could not be inserted in db", + ) + + return db_user_details except Exception as e: - # Check if the exception is due to username already existing + db.rollback() + log.error(f"Exception occurred: {e}") if 'duplicate key value violates unique constraint "users_username_key"' in str( e ): raise HTTPException( - status_code=400, + status_code=HTTPStatus.BAD_REQUEST, detail=f"User with this username {user_data.username} already exists.", ) from e else: - raise HTTPException(status_code=400, detail=str(e)) from e + raise HTTPException( + status_code=HTTPStatus.BAD_REQUEST, detail=str(e) + ) from e -@router.get("/me/") +@router.get("/me/", response_model=FMTMUser) async def my_data( db: Session = Depends(database.get_db), user_data: AuthUser = Depends(login_required), diff --git a/src/backend/app/auth/auth_schemas.py b/src/backend/app/auth/auth_schemas.py new file mode 100644 index 0000000000..6368acc273 --- /dev/null +++ b/src/backend/app/auth/auth_schemas.py @@ -0,0 +1,64 @@ +# +# This file is part of FMTM. +# +# FMTM is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# FMTM is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with FMTM. If not, see . +# +"""Pydantic models for Auth.""" + +from typing import Any, Optional + +from pydantic import BaseModel +from pydantic.functional_validators import field_validator + +from app.models.enums import ProjectRole, UserRole + + +class FMTMUser(BaseModel): + """User details returned to the frontend. + + TODO this should inherit from AuthUser and extend. + TODO profile_img should be refactored to `picture`. + """ + + id: int + username: str + profile_img: str + role: UserRole + project_roles: Optional[dict[int, ProjectRole]] = {} + orgs_managed: Optional[list[int]] = [] + + @field_validator("role", mode="before") + @classmethod + def convert_user_role_str_to_ints(cls, role: Any) -> Optional[UserRole]: + """User role strings returned from db converted to enum integers.""" + if not role: + return None + if isinstance(role, str): + return UserRole[role] + return role + + @field_validator("project_roles", mode="before") + @classmethod + def convert_project_role_str_to_ints( + cls, roles: dict[int, Any] + ) -> Optional[dict[int, ProjectRole]]: + """User project strings returned from db converted to enum integers.""" + if not roles: + return {} + + first_value = next(iter(roles.values()), None) + if isinstance(first_value, str): + return {id: ProjectRole[role] for id, role in roles.items()} + + return roles From 070ced2ffd9a25e61aa6fbafe7e332215c8da3b4 Mon Sep 17 00:00:00 2001 From: spwoodcock Date: Tue, 2 Jul 2024 19:44:14 +0100 Subject: [PATCH 16/23] fix(backend): return int type for AuthUser.id --- src/backend/app/auth/osm.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/backend/app/auth/osm.py b/src/backend/app/auth/osm.py index 5f15396378..a24909d077 100644 --- a/src/backend/app/auth/osm.py +++ b/src/backend/app/auth/osm.py @@ -54,10 +54,10 @@ def __init__(self, sub: str, **data): @computed_field @property - def id(self) -> Optional[str]: + def id(self) -> int: """Compute id from sub field.""" sub = self._sub - return (sub.split("|"))[1] + return int(sub.split("|")[1]) async def init_osm_auth(): From 9bbfb6596253781a76e93052ae0e7e319b59bd9f Mon Sep 17 00:00:00 2001 From: spwoodcock Date: Tue, 2 Jul 2024 20:44:58 +0100 Subject: [PATCH 17/23] refactor: create auth_schemas and replace all imports --- src/backend/app/auth/auth_routes.py | 8 +- src/backend/app/auth/auth_schemas.py | 43 ++++++- src/backend/app/auth/osm.py | 27 +---- src/backend/app/auth/roles.py | 114 +++++++++++------- src/backend/app/helpers/helper_routes.py | 3 +- .../app/organisations/organisation_crud.py | 2 +- .../app/organisations/organisation_routes.py | 13 +- src/backend/app/projects/project_crud.py | 4 +- src/backend/app/projects/project_routes.py | 43 +++---- src/backend/app/projects/project_schemas.py | 4 +- .../app/submissions/submission_routes.py | 7 +- 11 files changed, 157 insertions(+), 111 deletions(-) diff --git a/src/backend/app/auth/auth_routes.py b/src/backend/app/auth/auth_routes.py index b10eb7e721..6ff85ca3c2 100644 --- a/src/backend/app/auth/auth_routes.py +++ b/src/backend/app/auth/auth_routes.py @@ -26,9 +26,8 @@ from sqlalchemy import text from sqlalchemy.orm import Session -from app.auth.auth_schemas import FMTMUser +from app.auth.auth_schemas import AuthUser, FMTMUser from app.auth.osm import ( - AuthUser, create_tokens, extract_refresh_token_from_cookie, init_osm_auth, @@ -146,8 +145,8 @@ async def get_or_create_user( is_email_verified, is_expert, tasks_mapped, tasks_validated, tasks_invalidated, date_registered, last_validation_date ) VALUES ( - :user_id, :username, :profile_img, :role, :mapping_level, - FALSE, FALSE, 0, 0, 0, NOW(), NOW() + :user_id, :username, :profile_img, :role, + 'BEGINNER', FALSE, FALSE, 0, 0, 0, NOW(), NOW() ) ON CONFLICT (id) DO UPDATE SET @@ -175,7 +174,6 @@ async def get_or_create_user( "username": user_data.username, "profile_img": user_data.picture or "", "role": UserRole(user_data.role).name, - "mapping_level": "BEGINNER", } result = db.execute(upsert_sql, parameters) db.commit() diff --git a/src/backend/app/auth/auth_schemas.py b/src/backend/app/auth/auth_schemas.py index 6368acc273..ed53b04406 100644 --- a/src/backend/app/auth/auth_schemas.py +++ b/src/backend/app/auth/auth_schemas.py @@ -16,14 +16,53 @@ # """Pydantic models for Auth.""" -from typing import Any, Optional +from typing import Any, Optional, TypedDict -from pydantic import BaseModel +from pydantic import BaseModel, ConfigDict, PrivateAttr, computed_field from pydantic.functional_validators import field_validator +from app.db.db_models import DbOrganisation, DbProject, DbUser from app.models.enums import ProjectRole, UserRole +class OrgUserDict(TypedDict): + """Dict of both DbOrganisation & DbUser.""" + + user: DbUser + org: DbOrganisation + + +class ProjectUserDict(TypedDict): + """Dict of both DbProject & DbUser.""" + + user: DbUser + project: DbProject + + +class AuthUser(BaseModel): + """The user model returned from OSM OAuth2.""" + + model_config = ConfigDict(use_enum_values=True) + + username: str + picture: Optional[str] = None + role: Optional[UserRole] = UserRole.MAPPER + + _sub: str = PrivateAttr() # it won't return this field + + def __init__(self, sub: str, **data): + """Initializes the AuthUser class.""" + super().__init__(**data) + self._sub = sub + + @computed_field + @property + def id(self) -> int: + """Compute id from sub field.""" + sub = self._sub + return int(sub.split("|")[1]) + + class FMTMUser(BaseModel): """User details returned to the frontend. diff --git a/src/backend/app/auth/osm.py b/src/backend/app/auth/osm.py index a24909d077..e3a954c30b 100644 --- a/src/backend/app/auth/osm.py +++ b/src/backend/app/auth/osm.py @@ -20,14 +20,13 @@ import os import time -from typing import Optional import jwt from fastapi import Header, HTTPException, Request, Response from loguru import logger as log from osm_login_python.core import Auth -from pydantic import BaseModel, ConfigDict, PrivateAttr, computed_field +from app.auth.auth_schemas import AuthUser from app.config import settings from app.models.enums import UserRole @@ -36,30 +35,6 @@ os.environ["OAUTHLIB_INSECURE_TRANSPORT"] = "1" -class AuthUser(BaseModel): - """The user model returned from OSM OAuth2.""" - - model_config = ConfigDict(use_enum_values=True) - - username: str - picture: Optional[str] = None - role: Optional[UserRole] = UserRole.MAPPER - - _sub: str = PrivateAttr() # it won't return this field - - def __init__(self, sub: str, **data): - """Initializes the AuthUser class.""" - super().__init__(**data) - self._sub = sub - - @computed_field - @property - def id(self) -> int: - """Compute id from sub field.""" - sub = self._sub - return int(sub.split("|")[1]) - - async def init_osm_auth(): """Initialise Auth object from osm-login-python.""" return Auth( diff --git a/src/backend/app/auth/roles.py b/src/backend/app/auth/roles.py index b5879b3f1d..84180d6e3f 100644 --- a/src/backend/app/auth/roles.py +++ b/src/backend/app/auth/roles.py @@ -29,7 +29,8 @@ from sqlalchemy import text from sqlalchemy.orm import Session -from app.auth.osm import AuthUser, login_required +from app.auth.auth_schemas import AuthUser, OrgUserDict, ProjectUserDict +from app.auth.osm import login_required from app.db.database import get_db from app.db.db_models import DbProject, DbUser from app.models.enums import HTTPStatus, ProjectRole, ProjectVisibility @@ -152,7 +153,7 @@ async def super_admin( return db_user log.error( - f"User {user_data.username} requested an admin endpoint, " "but is not admin" + f"User {user_data.username} requested an admin endpoint, but is not admin" ) raise HTTPException( status_code=HTTPStatus.FORBIDDEN, detail="User must be an administrator" @@ -163,7 +164,7 @@ async def check_org_admin( db: Session, user: Union[AuthUser, int], org_id: int, -) -> dict: +) -> OrgUserDict: """Database check to determine if org admin role. Returns: @@ -234,51 +235,85 @@ async def org_admin( return org_user_dict -async def project_admin( - project: DbProject = Depends(get_project_by_id), - db: Session = Depends(get_db), - user_data: AuthUser = Depends(login_required), -) -> dict: - """Project admin role.""" +async def wrap_check_access( + project: DbProject, + db: Session, + user_data: AuthUser, + role: ProjectRole, +) -> ProjectUserDict: + """Wrap check_access call with HTTPException.""" db_user = await check_access( user_data, db, project_id=project.id, - role=ProjectRole.PROJECT_MANAGER, + role=role, ) - if db_user: - project_user_dict = { - "user": db_user, - "project": project, - } - return project_user_dict + if not db_user: + raise HTTPException( + status_code=HTTPStatus.FORBIDDEN, + detail="User is not a project manager", + ) - raise HTTPException( - status_code=HTTPStatus.FORBIDDEN, - detail="User is not a project manager", + return { + "user": db_user, + "project": project, + } + + +async def project_manager( + project: DbProject = Depends(get_project_by_id), + db: Session = Depends(get_db), + user_data: AuthUser = Depends(login_required), +) -> ProjectUserDict: + """A project manager for a specific project.""" + return await wrap_check_access( + project, + db, + user_data, + ProjectRole.PROJECT_MANAGER, ) -async def validator( +async def associate_project_manager( project: DbProject = Depends(get_project_by_id), db: Session = Depends(get_db), user_data: AuthUser = Depends(login_required), -) -> DbUser: - """A validator for a specific project.""" - db_user = await check_access( +) -> ProjectUserDict: + """An associate project manager for a specific project.""" + return await wrap_check_access( + project, + db, user_data, + ProjectRole.ASSOCIATE_PROJECT_MANAGER, + ) + + +async def field_manager( + project: DbProject = Depends(get_project_by_id), + db: Session = Depends(get_db), + user_data: AuthUser = Depends(login_required), +) -> ProjectUserDict: + """A field manager for a specific project.""" + return await wrap_check_access( + project, db, - project_id=project.id, - role=ProjectRole.VALIDATOR, + user_data, + ProjectRole.FIELD_MANAGER, ) - if db_user: - return db_user - raise HTTPException( - status_code=HTTPStatus.FORBIDDEN, - detail="User does not have validator permission", +async def validator( + project: DbProject = Depends(get_project_by_id), + db: Session = Depends(get_db), + user_data: AuthUser = Depends(login_required), +) -> ProjectUserDict: + """A validator for a specific project.""" + return await wrap_check_access( + project, + db, + user_data, + ProjectRole.VALIDATOR, ) @@ -286,24 +321,17 @@ async def mapper( project: DbProject = Depends(get_project_by_id), db: Session = Depends(get_db), user_data: AuthUser = Depends(login_required), -) -> Optional[DbUser]: +) -> AuthUser: """A mapper for a specific project.""" # If project is public, skip permission check if project.visibility == ProjectVisibility.PUBLIC: - # FIXME this is a different type than DbUser return user_data - db_user = await check_access( - user_data, + await wrap_check_access( + project, db, - project_id=project.id, - role=ProjectRole.MAPPER, + user_data, + ProjectRole.MAPPER, ) - if db_user: - return db_user - - raise HTTPException( - status_code=HTTPStatus.FORBIDDEN, - detail="User does not have mapper permission", - ) + return user_data diff --git a/src/backend/app/helpers/helper_routes.py b/src/backend/app/helpers/helper_routes.py index 6475391054..b4dc907f49 100644 --- a/src/backend/app/helpers/helper_routes.py +++ b/src/backend/app/helpers/helper_routes.py @@ -34,7 +34,8 @@ from osm_fieldwork.xlsforms import xlsforms_path from requests import get -from app.auth.osm import AuthUser, login_required +from app.auth.auth_schemas import AuthUser +from app.auth.osm import login_required from app.central import central_deps from app.central.central_crud import ( convert_geojson_to_odk_csv, diff --git a/src/backend/app/organisations/organisation_crud.py b/src/backend/app/organisations/organisation_crud.py index bc80bb0178..3c07871776 100644 --- a/src/backend/app/organisations/organisation_crud.py +++ b/src/backend/app/organisations/organisation_crud.py @@ -25,7 +25,7 @@ from sqlalchemy import text, update from sqlalchemy.orm import Session -from app.auth.osm import AuthUser +from app.auth.auth_schemas import AuthUser from app.config import encrypt_value, settings from app.db import db_models from app.models.enums import HTTPStatus diff --git a/src/backend/app/organisations/organisation_routes.py b/src/backend/app/organisations/organisation_routes.py index 5b44b2f97f..436b56e598 100644 --- a/src/backend/app/organisations/organisation_routes.py +++ b/src/backend/app/organisations/organisation_routes.py @@ -27,7 +27,8 @@ ) from sqlalchemy.orm import Session -from app.auth.osm import AuthUser, login_required +from app.auth.auth_schemas import AuthUser, OrgUserDict +from app.auth.osm import login_required from app.auth.roles import org_admin, super_admin from app.db import database from app.db.db_models import DbOrganisation, DbUser @@ -65,7 +66,7 @@ async def get_my_organisations( @router.get("/unapproved/", response_model=list[organisation_schemas.OrganisationOut]) async def list_unapproved_organisations( db: Session = Depends(database.get_db), - current_user: AuthUser = Depends(super_admin), + current_user: DbUser = Depends(super_admin), ) -> list[DbOrganisation]: """Get a list of all organisations.""" return await organisation_crud.get_unapproved_organisations(db) @@ -75,7 +76,7 @@ async def list_unapproved_organisations( async def unapproved_org_detail( org_id: int, db: Session = Depends(database.get_db), - current_user: AuthUser = Depends(super_admin), + current_user: DbUser = Depends(super_admin), ): """Get a detail of an unapproved organisations.""" return await organisation_crud.get_unapproved_org_detail(db, org_id) @@ -112,7 +113,7 @@ async def update_organisation( logo: UploadFile = File(None), organisation: DbOrganisation = Depends(org_exists), db: Session = Depends(database.get_db), - org_user_dict: DbUser = Depends(org_admin), + org_user_dict: OrgUserDict = Depends(org_admin), ): """Partial update for an existing organisation.""" return await organisation_crud.update_organisation( @@ -123,7 +124,7 @@ async def update_organisation( @router.delete("/{org_id}") async def delete_org( db: Session = Depends(database.get_db), - org_user_dict: DbUser = Depends(org_admin), + org_user_dict: OrgUserDict = Depends(org_admin), ): """Delete an organisation.""" return await organisation_crud.delete_organisation(db, org_user_dict["org"]) @@ -169,7 +170,7 @@ async def add_new_organisation_admin( db: Session = Depends(database.get_db), user: DbUser = Depends(user_exists_in_db), org: DbOrganisation = Depends(org_exists), - org_user_dict: DbUser = Depends(org_admin), + org_user_dict: OrgUserDict = Depends(org_admin), ): """Add a new organisation admin. diff --git a/src/backend/app/projects/project_crud.py b/src/backend/app/projects/project_crud.py index 9cd14ef175..bac97d5ce2 100644 --- a/src/backend/app/projects/project_crud.py +++ b/src/backend/app/projects/project_crud.py @@ -1672,10 +1672,10 @@ def count_user_contributions(db: Session, user_id: int, project_id: int) -> int: return contributions_count -async def add_project_admin( +async def add_project_manager( db: Session, user: db_models.DbUser, project: db_models.DbProject ): - """Adds a user as an admin to the specified organisation. + """Adds a user as an manager to the specified project. Args: db (Session): The database session. diff --git a/src/backend/app/projects/project_routes.py b/src/backend/app/projects/project_routes.py index 5d9ae27efd..4966f6acba 100644 --- a/src/backend/app/projects/project_routes.py +++ b/src/backend/app/projects/project_routes.py @@ -45,8 +45,9 @@ from sqlalchemy.orm import Session from sqlalchemy.sql import text -from app.auth.osm import AuthUser, login_required -from app.auth.roles import mapper, org_admin, project_admin +from app.auth.auth_schemas import AuthUser, OrgUserDict, ProjectUserDict +from app.auth.osm import login_required +from app.auth.roles import mapper, org_admin, project_manager from app.central import central_crud, central_schemas from app.db import database, db_models from app.db.postgis_utils import ( @@ -441,11 +442,10 @@ async def read_project( @router.delete("/{project_id}") async def delete_project( db: Session = Depends(database.get_db), - org_user_dict: db_models.DbUser = Depends(org_admin), + project: db_models.DbProject = Depends(project_deps.get_project_by_id), + org_user_dict: OrgUserDict = Depends(org_admin), ): """Delete a project from both ODK Central and the local database.""" - project = org_user_dict.get("project") - log.info( f"User {org_user_dict.get('user').username} attempting " f"deletion of project {project.id}" @@ -463,7 +463,7 @@ async def delete_project( @router.post("/create_project", response_model=project_schemas.ProjectOut) async def create_project( project_info: project_schemas.ProjectUpload, - org_user_dict: db_models.DbUser = Depends(org_admin), + org_user_dict: OrgUserDict = Depends(org_admin), db: Session = Depends(database.get_db), ): """Create a project in ODK Central and the local database. @@ -476,6 +476,7 @@ async def create_project( TODO but first check doesn't break other endpoints """ db_user = org_user_dict.get("user") + db_org = org_user_dict.get("org") project_info.organisation_id = db_org.id @@ -510,7 +511,7 @@ async def create_project( """ ) result = db.execute(sql, {"project_name": project_info.project_info.name.lower()}) - project_exists = result.fetchone()[0] + project_exists = result.first() if project_exists: raise HTTPException( status_code=400, @@ -539,7 +540,7 @@ async def create_project( async def update_project( project_info: project_schemas.ProjectUpdate, db: Session = Depends(database.get_db), - project_user_dict: dict = Depends(project_admin), + project_user_dict: ProjectUserDict = Depends(project_manager), ): """Update an existing project by ID. @@ -549,7 +550,7 @@ async def update_project( Parameters: - id: ID of the project to update - project_info: Updated project information - - current_user (DbUser): Check if user is project_admin + - current_user (DbUser): Check if user is project_manager Returns: - Updated project information @@ -569,7 +570,7 @@ async def update_project( async def project_partial_update( project_info: project_schemas.ProjectPartialUpdate, db: Session = Depends(database.get_db), - project_user_dict: dict = Depends(project_admin), + project_user_dict: ProjectUserDict = Depends(project_manager), ): """Partial Update an existing project by ID. @@ -600,7 +601,7 @@ async def upload_project_task_boundaries( project_id: int, task_geojson: UploadFile = File(...), db: Session = Depends(database.get_db), - org_user_dict: db_models.DbUser = Depends(org_admin), + org_user_dict: OrgUserDict = Depends(org_admin), ): """Set project task boundaries using split GeoJSON from frontend. @@ -706,7 +707,7 @@ async def generate_files( background_tasks: BackgroundTasks, xls_form_upload: Optional[UploadFile] = File(None), db: Session = Depends(database.get_db), - org_user_dict: db_models.DbUser = Depends(org_admin), + project_user_dict: ProjectUserDict = Depends(project_manager), ): """Generate additional content to initialise the project. @@ -730,12 +731,12 @@ async def generate_files( xls_form_upload (UploadFile, optional): A custom XLSForm to use in the project. A file should be provided if user wants to upload a custom xls form. db (Session): Database session, provided automatically. - org_user_dict (AuthUser): Current logged in user. Must be org admin. + project_user_dict (ProjectUserDict): Project admin role. Returns: json (JSONResponse): A success message containing the project ID. """ - project = org_user_dict.get("project") + project = project_user_dict.get("project") log.debug(f"Generating media files tasks for project: {project.id}") @@ -860,7 +861,7 @@ async def get_or_set_data_extract( url: Optional[str] = None, project_id: int = Query(..., description="Project ID"), db: Session = Depends(database.get_db), - project_user_dict: dict = Depends(project_admin), + project_user_dict: ProjectUserDict = Depends(project_manager), ): """Get or set the data extract URL for a project.""" fgb_url = await project_crud.get_or_set_data_extract_url( @@ -877,7 +878,7 @@ async def upload_custom_extract( custom_extract_file: UploadFile = File(...), project_id: int = Query(..., description="Project ID"), db: Session = Depends(database.get_db), - project_user_dict: dict = Depends(project_admin), + project_user_dict: ProjectUserDict = Depends(project_manager), ): """Upload a custom data extract geojson for a project. @@ -951,7 +952,7 @@ async def update_project_form( category: XLSFormType = Form(...), upload: Optional[UploadFile] = File(None), db: Session = Depends(database.get_db), - project_user_dict: dict = Depends(project_admin), + project_user_dict: ProjectUserDict = Depends(project_manager), ) -> project_schemas.ProjectBase: """Update the XForm data in ODK Central. @@ -1256,15 +1257,15 @@ async def get_contributors( return project_users -@router.post("/add_admin/") -async def add_new_project_admin( +@router.post("/add-manager/") +async def add_new_project_manager( db: Session = Depends(database.get_db), - project_user_dict: dict = Depends(project_admin), + project_user_dict: ProjectUserDict = Depends(project_manager), ): """Add a new project manager. The logged in user must be either the admin of the organisation or a super admin. """ - return await project_crud.add_project_admin( + return await project_crud.add_project_manager( db, project_user_dict["user"], project_user_dict["project"] ) diff --git a/src/backend/app/projects/project_schemas.py b/src/backend/app/projects/project_schemas.py index 1005a9ad16..453bea4516 100644 --- a/src/backend/app/projects/project_schemas.py +++ b/src/backend/app/projects/project_schemas.py @@ -228,8 +228,10 @@ class ProjectPartialUpdate(BaseModel): @computed_field @property - def project_name_prefix(self) -> str: + def project_name_prefix(self) -> Optional[str]: """Compute project name prefix with underscores.""" + if not self.name: + return None return self.name.replace(" ", "_").lower() diff --git a/src/backend/app/submissions/submission_routes.py b/src/backend/app/submissions/submission_routes.py index 090997eb91..a69f9838f9 100644 --- a/src/backend/app/submissions/submission_routes.py +++ b/src/backend/app/submissions/submission_routes.py @@ -27,8 +27,9 @@ from fastapi.responses import FileResponse from sqlalchemy.orm import Session -from app.auth.osm import AuthUser, login_required -from app.auth.roles import mapper, project_admin +from app.auth.auth_schemas import AuthUser +from app.auth.osm import login_required +from app.auth.roles import mapper, project_manager from app.central import central_crud from app.db import database, db_models, postgis_utils from app.models.enums import HTTPStatus, ReviewStateEnum @@ -508,7 +509,7 @@ async def update_review_state( project_id: int, instance_id: str, review_state: ReviewStateEnum, - current_user: AuthUser = Depends(project_admin), + current_user: AuthUser = Depends(project_manager), db: Session = Depends(database.get_db), ): """Updates the review state of a project submission. From 0a1e4c5dea8c88e6372630b9e54d5b22f7e599e1 Mon Sep 17 00:00:00 2001 From: spwoodcock Date: Tue, 2 Jul 2024 20:45:34 +0100 Subject: [PATCH 18/23] refactor: pass user_id only to task_crud functions --- src/backend/app/tasks/tasks_crud.py | 7 +++---- src/backend/app/tasks/tasks_routes.py | 7 ++++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/backend/app/tasks/tasks_crud.py b/src/backend/app/tasks/tasks_crud.py index 5c77627e9c..67a02bc06a 100644 --- a/src/backend/app/tasks/tasks_crud.py +++ b/src/backend/app/tasks/tasks_crud.py @@ -24,7 +24,6 @@ from sqlalchemy.orm import Session from sqlalchemy.sql import text -from app.auth.osm import AuthUser from app.db import database, db_models from app.models.enums import ( TaskStatus, @@ -253,14 +252,14 @@ async def create_task_history_for_status_change( async def add_task_comments( - db: Session, comment: tasks_schemas.TaskCommentRequest, user_data: AuthUser + db: Session, comment: tasks_schemas.TaskCommentRequest, user_id: int ): """Add a comment to a task. Parameters: - db: SQLAlchemy database session - comment: TaskCommentBase instance containing the comment details - - user_data: AuthUser instance containing the user details + - user_id: OAuth user id. Returns: - Dictionary with the details of the added comment @@ -293,7 +292,7 @@ async def add_task_comments( "task_id": comment.task_id, "comment_text": comment.comment, "current_date": currentdate, - "user_id": user_data.id, + "user_id": user_id, } # Execute the query with the named parameters and commit the transaction diff --git a/src/backend/app/tasks/tasks_routes.py b/src/backend/app/tasks/tasks_routes.py index d0dcca3b51..54b37bec71 100644 --- a/src/backend/app/tasks/tasks_routes.py +++ b/src/backend/app/tasks/tasks_routes.py @@ -24,7 +24,7 @@ from fastapi import APIRouter, Depends, HTTPException from sqlalchemy.orm import Session -from app.auth.osm import AuthUser +from app.auth.auth_schemas import AuthUser from app.auth.roles import get_uid, mapper from app.db import database from app.models.enums import TaskStatus @@ -139,7 +139,7 @@ async def update_task_status( async def add_task_comments( comment: tasks_schemas.TaskCommentRequest, db: Session = Depends(database.get_db), - user_data: AuthUser = Depends(mapper), + current_user: AuthUser = Depends(mapper), ): """Create a new task comment. @@ -151,7 +151,8 @@ async def add_task_comments( Returns: TaskCommentResponse: The created task comment. """ - task_comment_list = await tasks_crud.add_task_comments(db, comment, user_data) + user_id = await get_uid(current_user) + task_comment_list = await tasks_crud.add_task_comments(db, comment, user_id) return task_comment_list From 553614be465ae2c2e9c240b8e955f7b2462987e1 Mon Sep 17 00:00:00 2001 From: spwoodcock Date: Tue, 2 Jul 2024 20:46:02 +0100 Subject: [PATCH 19/23] test: update auth user creation for fixtures --- src/backend/tests/conftest.py | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/backend/tests/conftest.py b/src/backend/tests/conftest.py index fd5299be55..be9c3d0820 100644 --- a/src/backend/tests/conftest.py +++ b/src/backend/tests/conftest.py @@ -31,7 +31,7 @@ from sqlalchemy_utils import create_database, database_exists from app.auth.auth_routes import get_or_create_user -from app.auth.osm import AuthUser +from app.auth.auth_schemas import AuthUser, FMTMUser from app.central import central_crud from app.config import settings from app.db.database import Base, get_db @@ -93,15 +93,18 @@ async def admin_user(db): db_user = await get_or_create_user( db, AuthUser( + sub="fmtm|1", username="localadmin", - id=1, role=UserRole.ADMIN, ), ) - # Upgrade role from default MAPPER (if user already exists) - db_user["role"] = UserRole.ADMIN - db.commit() - return db_user + + return FMTMUser( + id=db_user.id, + username=db_user.username, + role=UserRole[db_user.role], + profile_img=db_user.profile_img, + ) @pytest.fixture(scope="function") @@ -174,11 +177,7 @@ async def project(db, admin_user, organisation): db, project_metadata, odkproject["id"], - AuthUser( - username=admin_user["username"], - id=admin_user["id"], - role=UserRole.ADMIN, - ), + admin_user, ) log.debug(f"Project returned: {new_project.__dict__}") assert new_project is not None From a667e4d43aa25c5040ef2f4529080a83e30b167f Mon Sep 17 00:00:00 2001 From: spwoodcock Date: Tue, 2 Jul 2024 20:46:31 +0100 Subject: [PATCH 20/23] test: guarantee randomness with uuid4 over randint --- src/backend/tests/test_projects_routes.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/backend/tests/test_projects_routes.py b/src/backend/tests/test_projects_routes.py index 4e839fa1b6..8358e7c645 100644 --- a/src/backend/tests/test_projects_routes.py +++ b/src/backend/tests/test_projects_routes.py @@ -21,8 +21,8 @@ import os from io import BytesIO from pathlib import Path -from random import randint from unittest.mock import Mock, patch +from uuid import uuid4 import pytest import requests @@ -52,7 +52,7 @@ async def test_create_project(client, admin_user, organisation): project_data = { "project_info": { - "name": f"Test Project {randint(1, 1000000)}", + "name": f"Test Project {uuid4()}", "short_description": "test", "description": "test", }, @@ -87,7 +87,6 @@ async def test_create_project(client, admin_user, organisation): async def test_delete_project(client, admin_user, project): """Test deleting a FMTM project, plus ODK Central project.""" - log.warning(project) response = client.delete(f"/projects/{project.id}") assert response.status_code == 204 @@ -279,7 +278,7 @@ async def test_update_project(client, admin_user, project): """Test update project metadata.""" updated_project_data = { "project_info": { - "name": f"Updated Test Project {randint(1, 1000000)}", + "name": f"Updated Test Project {uuid4()}", "short_description": "updated short description", "description": "updated description", }, From 8f83eb5c05a26b95752766dc36a7b46093694b68 Mon Sep 17 00:00:00 2001 From: spwoodcock Date: Wed, 3 Jul 2024 13:45:04 +0100 Subject: [PATCH 21/23] fix(backend): create_project existing project check must be scalar --- src/backend/app/projects/project_routes.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/backend/app/projects/project_routes.py b/src/backend/app/projects/project_routes.py index 4966f6acba..ef312b59de 100644 --- a/src/backend/app/projects/project_routes.py +++ b/src/backend/app/projects/project_routes.py @@ -511,7 +511,7 @@ async def create_project( """ ) result = db.execute(sql, {"project_name": project_info.project_info.name.lower()}) - project_exists = result.first() + project_exists = result.scalar() if project_exists: raise HTTPException( status_code=400, From 6478217488977e20a309e404460980220cf197d0 Mon Sep 17 00:00:00 2001 From: spwoodcock Date: Wed, 3 Jul 2024 13:45:31 +0100 Subject: [PATCH 22/23] fix(backend): conform to nominatim usage policy referer header --- src/backend/app/db/postgis_utils.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/backend/app/db/postgis_utils.py b/src/backend/app/db/postgis_utils.py index 6c1cf55b1f..af2d9cc706 100644 --- a/src/backend/app/db/postgis_utils.py +++ b/src/backend/app/db/postgis_utils.py @@ -38,6 +38,8 @@ from sqlalchemy.exc import ProgrammingError from sqlalchemy.orm import Session +from app.config import settings + log = logging.getLogger(__name__) @@ -535,7 +537,13 @@ def get_address_from_lat_lon(latitude, longitude): "lon": longitude, "zoom": 18, } - headers = {"Accept-Language": "en"} # Set the language to English + headers = { + # Set the language to English + "Accept-Language": "en", + # Referer or User-Agent required as per usage policy: + # https://operations.osmfoundation.org/policies/nominatim + "Referer": settings.FMTM_DOMAIN, + } log.debug( f"Getting Nominatim address from project lat ({latitude}) lon ({longitude})" From 15da73b4003b8e9e8f2fe920791180b39ff82daa Mon Sep 17 00:00:00 2001 From: spwoodcock Date: Wed, 3 Jul 2024 13:45:47 +0100 Subject: [PATCH 23/23] test: rename var for clarity --- src/backend/tests/test_projects_routes.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/backend/tests/test_projects_routes.py b/src/backend/tests/test_projects_routes.py index 8358e7c645..95321bbe70 100644 --- a/src/backend/tests/test_projects_routes.py +++ b/src/backend/tests/test_projects_routes.py @@ -48,7 +48,7 @@ async def test_create_project(client, admin_user, organisation): "odk_central_user": odk_central_user, "odk_central_password": odk_central_password, } - odk_credentials = project_schemas.ODKCentralDecrypted(**odk_credentials) + odk_creds_models = project_schemas.ODKCentralDecrypted(**odk_credentials) project_data = { "project_info": { @@ -71,7 +71,7 @@ async def test_create_project(client, admin_user, organisation): "type": "Polygon", }, } - project_data.update(**odk_credentials.model_dump()) + project_data.update(**odk_creds_models.model_dump()) response = client.post( f"/projects/create_project?org_id={organisation.id}", json=project_data