From 36ae5d43345968e5dbeb02a375930cf5822f4d0e Mon Sep 17 00:00:00 2001 From: Niraj Adhikari <41701707+nrjadkry@users.noreply.github.com> Date: Tue, 23 Jan 2024 19:53:37 +0545 Subject: [PATCH] feat: organisation approval and admin endpoints (#1126) * dependencies to cehck if user_exists * endpoint to add organisation admin * added approved field in organisation model * fix: table name in migration file for organisations * feat: organisations list api updated according to role * feat: endpoint to approve organisations * update: get organisation endpoint for filtering approval * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * added docstrings * fix: pre-commit linting errors * added docstring in user_deps file * build: add default odk credentials to organisations * build: merge migrations to organisations table * refactor: fix linting errors * refactor: remove subscription_tier field for orgs * build: add public.organisation.approved field to base schema * refactor: remove extra url field from DbOrganisation * refactor: fix organizationModel dir --> organisation for import * fix: remove router get_db global dependency (on routes) * fix: use separate super_admin + check_super_admin deps * fix: update org_admin to also allow super_admin * refactor: remove missed log.warning from organisations endpoint * fix: separate Depends from logic, working org approval --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: spwoodcock --- src/backend/app/auth/roles.py | 70 ++++++++++++++----- src/backend/app/central/central_routes.py | 1 - src/backend/app/db/db_models.py | 2 +- .../app/organisations/organisation_crud.py | 51 ++++++++++++-- .../app/organisations/organisation_deps.py | 46 +++++++++--- .../app/organisations/organisation_routes.py | 42 +++++++++-- src/backend/app/projects/project_routes.py | 1 - .../app/submissions/submission_routes.py | 1 - src/backend/app/tasks/tasks_routes.py | 1 - src/backend/app/users/user_deps.py | 70 +++++++++++++++++++ src/backend/app/users/user_routes.py | 1 - .../migrations/002-add-profile-img.sql | 2 +- src/backend/migrations/003-project-roles.sql | 2 +- .../migrations/004-organisation-odk-creds.sql | 14 ++++ .../migrations/init/fmtm_base_schema.sql | 6 +- .../migrations/revert/002-add-profile-img.sql | 2 +- .../revert/004-organisation-odk-creds.sql | 13 ++++ .../createproject/createProjectModel.ts | 1 - .../organisationModel.ts | 3 - 19 files changed, 281 insertions(+), 48 deletions(-) create mode 100644 src/backend/app/users/user_deps.py create mode 100644 src/backend/migrations/004-organisation-odk-creds.sql create mode 100644 src/backend/migrations/revert/004-organisation-odk-creds.sql rename src/frontend/src/models/{organization => organisation}/organisationModel.ts (88%) diff --git a/src/backend/app/auth/roles.py b/src/backend/app/auth/roles.py index 96278f62be..17c16c2191 100644 --- a/src/backend/app/auth/roles.py +++ b/src/backend/app/auth/roles.py @@ -22,6 +22,8 @@ and always return an AuthUser object in a standard format. """ +from typing import Optional + from fastapi import Depends, HTTPException from loguru import logger as log from sqlalchemy.orm import Session @@ -30,6 +32,7 @@ from app.db.database import get_db from app.db.db_models import DbProject, DbUser, DbUserRoles, organisation_managers from app.models.enums import HTTPStatus, ProjectRole, UserRole +from app.organisations.organisation_deps import check_org_exists from app.projects.project_deps import get_project_by_id @@ -45,17 +48,30 @@ async def get_uid(user_data: AuthUser) -> int: ) +async def check_super_admin( + db: Session, + user: [AuthUser, int], +) -> DbUser: + """Database check to determine if super admin role.""" + if isinstance(user, int): + user_id = user + else: + user_id = await get_uid(user) + return db.query(DbUser).filter_by(id=user_id, role=UserRole.ADMIN).first() + + async def super_admin( - db: Session = Depends(get_db), user_data: AuthUser = Depends(login_required), + db: Session = Depends(get_db), ) -> AuthUser: """Super admin role, with access to all endpoints.""" - user_id = await get_uid(user_data) + super_admin = await check_super_admin(db, user_data) - match = db.query(DbUser).filter_by(id=user_id, role=UserRole.ADMIN).first() - - if not match: - log.error(f"User ID {user_id} requested an admin endpoint, but is not admin") + if not super_admin: + log.error( + f"User {user_data.get('username')} requested an admin endpoint, " + "but is not admin" + ) raise HTTPException( status_code=HTTPStatus.FORBIDDEN, detail="User must be an administrator" ) @@ -63,6 +79,35 @@ async def super_admin( return user_data +async def check_org_admin( + db: Session, + user: [AuthUser, int], + project: Optional[DbProject], + org_id: Optional[int], +) -> DbUser: + """Database check to determine if org admin role.""" + if isinstance(user, int): + user_id = user + else: + user_id = await get_uid(user) + + if project: + org_id = db.query(DbProject).filter_by(id=project.id).first().organisation_id + + # Check org exists + await check_org_exists(db, org_id) + + # If user is admin, skip checks + if await check_super_admin(db, user): + return user + + return ( + db.query(organisation_managers) + .filter_by(organisation_id=org_id, user_id=user_id) + .first() + ) + + async def org_admin( project: DbProject = Depends(get_project_by_id), org_id: int = None, @@ -77,19 +122,10 @@ async def org_admin( detail="Both org_id and project_id cannot be passed at the same time", ) - user_id = await get_uid(user_data) - - if project: - org_id = db.query(DbProject).filter_by(id=project.id).first().organisation_id - - org_admin = ( - db.query(organisation_managers) - .filter_by(organisation_id=org_id, user_id=user_id) - .first() - ) + org_admin = await check_org_admin(db, user_data, project, org_id) if not org_admin: - log.error(f"User ID {user_id} is not an admin for organisation {org_id}") + log.error(f"User {user_data} is not an admin for organisation {org_id}") raise HTTPException( status_code=HTTPStatus.FORBIDDEN, detail="User is not organisation admin", diff --git a/src/backend/app/central/central_routes.py b/src/backend/app/central/central_routes.py index bc4b74d26a..122842460f 100644 --- a/src/backend/app/central/central_routes.py +++ b/src/backend/app/central/central_routes.py @@ -38,7 +38,6 @@ router = APIRouter( prefix="/central", tags=["central"], - dependencies=[Depends(database.get_db)], responses={404: {"description": "Not found"}}, ) diff --git a/src/backend/app/db/db_models.py b/src/backend/app/db/db_models.py index cf14d49d2b..26494c594c 100644 --- a/src/backend/app/db/db_models.py +++ b/src/backend/app/db/db_models.py @@ -147,7 +147,7 @@ class DbOrganisation(Base): description = Column(String) url = Column(String) type = Column(Enum(OrganisationType), default=OrganisationType.FREE, nullable=False) - # subscription_tier = Column(Integer) + approved = Column(Boolean, default=False) managers = relationship( DbUser, diff --git a/src/backend/app/organisations/organisation_crud.py b/src/backend/app/organisations/organisation_crud.py index a058a70514..87d7a2f5e8 100644 --- a/src/backend/app/organisations/organisation_crud.py +++ b/src/backend/app/organisations/organisation_crud.py @@ -24,6 +24,8 @@ from sqlalchemy import update from sqlalchemy.orm import Session +from app.auth.osm import AuthUser +from app.auth.roles import check_super_admin from app.config import settings from app.db import db_models from app.models.enums import HTTPStatus @@ -34,11 +36,15 @@ from app.s3 import add_obj_to_bucket -def get_organisations( - db: Session, -): +async def get_organisations(db: Session, current_user: AuthUser, is_approved: bool): """Get all orgs.""" - return db.query(db_models.DbOrganisation).all() + super_admin = await check_super_admin(db, current_user) + + if super_admin: + return db.query(db_models.DbOrganisation).filter_by(approved=is_approved).all() + + # If user not admin, only show approved orgs + return db.query(db_models.DbOrganisation).filter_by(approved=True).all() async def upload_logo_to_s3( @@ -186,3 +192,40 @@ async def delete_organisation( db.commit() return Response(status_code=HTTPStatus.NO_CONTENT) + + +async def add_organisation_admin( + db: Session, user: db_models.DbUser, organisation: db_models.DbOrganisation +): + """Adds a user as an admin to the specified organisation. + + Args: + db (Session): The database session. + user (DbUser): The user model instance. + organisation (DbOrganisation): The organisation model instance. + + Returns: + Response: The HTTP response with status code 200. + """ + log.info(f"Adding user ({user.id}) as org ({organisation.id}) admin") + # add data to the managers field in organisation model + organisation.managers.append(user) + db.commit() + + return Response(status_code=HTTPStatus.OK) + + +async def approve_organisation(db, organisation): + """Approves an oranisation request made by the user . + + Args: + db: The database session. + organisation (DbOrganisation): The organisation model instance. + + Returns: + Response: An HTTP response with the status code 200. + """ + log.info(f"Approving organisation ID {organisation.id}") + organisation.approved = True + db.commit() + return Response(status_code=HTTPStatus.OK) diff --git a/src/backend/app/organisations/organisation_deps.py b/src/backend/app/organisations/organisation_deps.py index b4aad5f41b..4d26e504ed 100644 --- a/src/backend/app/organisations/organisation_deps.py +++ b/src/backend/app/organisations/organisation_deps.py @@ -31,39 +31,58 @@ from app.models.enums import HTTPStatus -async def get_organisation_by_name(db: Session, org_name: str) -> DbOrganisation: +async def get_organisation_by_name( + db: Session, org_name: str, check_approved: bool = True +) -> DbOrganisation: """Get an organisation from the db by name. Args: db (Session): database session org_name (int): id of the organisation + check_approved (bool): first check if the organisation is approved Returns: DbOrganisation: organisation with the given id """ - return ( + org_obj = ( db.query(DbOrganisation) .filter(func.lower(DbOrganisation.name).like(func.lower(f"%{org_name}%"))) .first() ) + if org_obj and check_approved and org_obj.approved is False: + raise HTTPException( + status_code=HTTPStatus.NOT_FOUND, + detail=f"Organisation ({org_obj.id}) is not approved yet", + ) + return org_obj -async def get_organisation_by_id(db: Session, org_id: int) -> DbOrganisation: +async def get_organisation_by_id( + db: Session, org_id: int, check_approved: bool = True +) -> DbOrganisation: """Get an organisation from the db by id. Args: db (Session): database session org_id (int): id of the organisation + check_approved (bool): first check if the organisation is approved Returns: DbOrganisation: organisation with the given id """ - return db.query(DbOrganisation).filter(DbOrganisation.id == org_id).first() + org_obj = db.query(DbOrganisation).filter_by(id=org_id).first() + if org_obj and check_approved and org_obj.approved is False: + raise HTTPException( + status_code=HTTPStatus.NOT_FOUND, + detail=f"Organisation {org_id} is not approved yet", + ) + return org_obj -async def org_exists( +async def check_org_exists( + db: Session, org_id: Union[str, int], - db: Session = Depends(get_db), + check_approved: bool = True, ) -> DbOrganisation: """Check if organisation name exists, else error. @@ -76,11 +95,11 @@ async def org_exists( if isinstance(org_id, int): log.debug(f"Getting organisation by id: {org_id}") - db_organisation = await get_organisation_by_id(db, org_id) + db_organisation = await get_organisation_by_id(db, org_id, check_approved) if isinstance(org_id, str): log.debug(f"Getting organisation by name: {org_id}") - db_organisation = await get_organisation_by_name(db, org_id) + db_organisation = await get_organisation_by_name(db, org_id, check_approved) if not db_organisation: raise HTTPException( @@ -90,3 +109,14 @@ async def org_exists( log.debug(f"Organisation match: {db_organisation}") return db_organisation + + +async def org_exists( + org_id: Union[str, int], + db: Session = Depends(get_db), +) -> DbOrganisation: + """Wrapper for check_org_exists to be used as a route dependency. + + Requires Depends from a route. + """ + return await check_org_exists(db, org_id) diff --git a/src/backend/app/organisations/organisation_routes.py b/src/backend/app/organisations/organisation_routes.py index b81dc8d763..6b5000f004 100644 --- a/src/backend/app/organisations/organisation_routes.py +++ b/src/backend/app/organisations/organisation_routes.py @@ -25,25 +25,29 @@ ) from sqlalchemy.orm import Session +from app.auth.osm import AuthUser, login_required +from app.auth.roles import org_admin, super_admin from app.db import database -from app.db.db_models import DbOrganisation +from app.db.db_models import DbOrganisation, DbUser from app.organisations import organisation_crud, organisation_schemas -from app.organisations.organisation_deps import org_exists +from app.organisations.organisation_deps import check_org_exists, org_exists +from app.users.user_deps import user_exists_in_db router = APIRouter( prefix="/organisation", tags=["organisation"], - dependencies=[Depends(database.get_db)], responses={404: {"description": "Not found"}}, ) @router.get("/", response_model=list[organisation_schemas.OrganisationOut]) -def get_organisations( +async def get_organisations( db: Session = Depends(database.get_db), + current_user: AuthUser = Depends(login_required), + approved: bool = True, ) -> list[organisation_schemas.OrganisationOut]: """Get a list of all organisations.""" - return organisation_crud.get_organisations(db) + return await organisation_crud.get_organisations(db, current_user, approved) @router.get("/{org_id}", response_model=organisation_schemas.OrganisationOut) @@ -85,3 +89,31 @@ async def delete_organisations( ): """Delete an organisation.""" return await organisation_crud.delete_organisation(db, organisation) + + +@router.post("/approve/") +async def approve_organisation( + org_id: int, + db: Session = Depends(database.get_db), + current_user: AuthUser = Depends(super_admin), +): + """Approve the organisation request made by the user. + + The logged in user must be super admin to perform this action . + """ + org_obj = await check_org_exists(db, org_id, check_approved=False) + return await organisation_crud.approve_organisation(db, org_obj) + + +@router.post("/add_admin/") +async def add_new_organisation_admin( + db: Session = Depends(database.get_db), + organisation: DbOrganisation = Depends(org_exists), + user: DbUser = Depends(user_exists_in_db), + current_user: AuthUser = Depends(org_admin), +): + """Add a new organisation admin. + + The logged in user must be either the owner of the organisation or a super admin. + """ + return await organisation_crud.add_organisation_admin(db, user, organisation) diff --git a/src/backend/app/projects/project_routes.py b/src/backend/app/projects/project_routes.py index f6df1ba6ec..0282a0bc92 100644 --- a/src/backend/app/projects/project_routes.py +++ b/src/backend/app/projects/project_routes.py @@ -55,7 +55,6 @@ router = APIRouter( prefix="/projects", tags=["projects"], - dependencies=[Depends(database.get_db)], responses={404: {"description": "Not found"}}, ) diff --git a/src/backend/app/submissions/submission_routes.py b/src/backend/app/submissions/submission_routes.py index 3687b2afb5..08c04536ec 100644 --- a/src/backend/app/submissions/submission_routes.py +++ b/src/backend/app/submissions/submission_routes.py @@ -38,7 +38,6 @@ router = APIRouter( prefix="/submission", tags=["submission"], - dependencies=[Depends(database.get_db)], responses={404: {"description": "Not found"}}, ) diff --git a/src/backend/app/tasks/tasks_routes.py b/src/backend/app/tasks/tasks_routes.py index 8e5d0b3d3f..6df1d6f367 100644 --- a/src/backend/app/tasks/tasks_routes.py +++ b/src/backend/app/tasks/tasks_routes.py @@ -34,7 +34,6 @@ router = APIRouter( prefix="/tasks", tags=["tasks"], - dependencies=[Depends(database.get_db)], responses={404: {"description": "Not found"}}, ) diff --git a/src/backend/app/users/user_deps.py b/src/backend/app/users/user_deps.py new file mode 100644 index 0000000000..3bf84e4363 --- /dev/null +++ b/src/backend/app/users/user_deps.py @@ -0,0 +1,70 @@ +# Copyright (c) 2022, 2023 Humanitarian OpenStreetMap Team +# +# 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 . +# + +"""User dependencies for use in Depends.""" + + +from typing import Union + +from fastapi import Depends +from fastapi.exceptions import HTTPException +from loguru import logger as log +from sqlalchemy.orm import Session + +from app.db.database import get_db +from app.db.db_models import DbUser +from app.models.enums import HTTPStatus +from app.users.user_crud import get_user, get_user_by_username + + +async def user_exists_in_db( + user_id: Union[str, int], + db: Session = Depends(get_db), +) -> DbUser: + """Check if a user exists, else Error. + + Args: + user_id (Union[str, int]): The user ID (integer) or username (string) to check. + db (Session, optional): The SQLAlchemy database session. + + Returns: + DbUser: The user if found. + + Raises: + HTTPException: Raised with a 404 status code if the user is not found. + """ + try: + user_id = int(user_id) + except ValueError: + pass + + if isinstance(user_id, int): + log.debug(f"Getting user by ID: {user_id}") + db_user = await get_user(db, user_id) + + if isinstance(user_id, str): + log.debug(f"Getting user by username: {user_id}") + db_user = await get_user_by_username(db, user_id) + + if not db_user: + raise HTTPException( + status_code=HTTPStatus.NOT_FOUND, + detail=f"User {user_id} does not exist", + ) + + return db_user diff --git a/src/backend/app/users/user_routes.py b/src/backend/app/users/user_routes.py index 9bfffd3fee..059b82a90a 100644 --- a/src/backend/app/users/user_routes.py +++ b/src/backend/app/users/user_routes.py @@ -29,7 +29,6 @@ router = APIRouter( prefix="/users", tags=["users"], - dependencies=[Depends(database.get_db)], responses={404: {"description": "Not found"}}, ) diff --git a/src/backend/migrations/002-add-profile-img.sql b/src/backend/migrations/002-add-profile-img.sql index ae7f620918..361fc76e1e 100644 --- a/src/backend/migrations/002-add-profile-img.sql +++ b/src/backend/migrations/002-add-profile-img.sql @@ -7,4 +7,4 @@ BEGIN; ALTER TABLE IF EXISTS public.users ADD COLUMN IF NOT EXISTS profile_img VARCHAR; -- Commit the transaction -COMMIT; \ No newline at end of file +COMMIT; diff --git a/src/backend/migrations/003-project-roles.sql b/src/backend/migrations/003-project-roles.sql index c4f75e5c5d..6c4bf50c69 100644 --- a/src/backend/migrations/003-project-roles.sql +++ b/src/backend/migrations/003-project-roles.sql @@ -17,4 +17,4 @@ ALTER TABLE public.user_roles ALTER COLUMN "role" TYPE public.projectrole USING ALTER TYPE public.projectrole OWNER TO fmtm; -- Commit the transaction -COMMIT; \ No newline at end of file +COMMIT; diff --git a/src/backend/migrations/004-organisation-odk-creds.sql b/src/backend/migrations/004-organisation-odk-creds.sql new file mode 100644 index 0000000000..d65b61a8a1 --- /dev/null +++ b/src/backend/migrations/004-organisation-odk-creds.sql @@ -0,0 +1,14 @@ +-- ## Migration to: +-- * Add odk central credentials (str) to organisations table. +-- * Add the approved (bool) field to organisations table. + +-- Start a transaction +BEGIN; + +ALTER TABLE IF EXISTS public.organisations + ADD COLUMN IF NOT EXISTS approved BOOLEAN DEFAULT false, + ADD COLUMN IF NOT EXISTS odk_central_url VARCHAR, + ADD COLUMN IF NOT EXISTS odk_central_user VARCHAR, + ADD COLUMN IF NOT EXISTS odk_central_password VARCHAR; +-- Commit the transaction +COMMIT; diff --git a/src/backend/migrations/init/fmtm_base_schema.sql b/src/backend/migrations/init/fmtm_base_schema.sql index 857ef6e430..d6c328aa47 100644 --- a/src/backend/migrations/init/fmtm_base_schema.sql +++ b/src/backend/migrations/init/fmtm_base_schema.sql @@ -276,7 +276,11 @@ CREATE TABLE public.organisations ( logo character varying, description character varying, url character varying, - type public.organisationtype NOT NULL + type public.organisationtype NOT NULL, + approved BOOLEAN DEFAULT false, + odk_central_url character varying, + odk_central_user character varying, + odk_central_password character varying ); ALTER TABLE public.organisations OWNER TO fmtm; CREATE SEQUENCE public.organisations_id_seq diff --git a/src/backend/migrations/revert/002-add-profile-img.sql b/src/backend/migrations/revert/002-add-profile-img.sql index 49b3e65f1d..5b02224dda 100644 --- a/src/backend/migrations/revert/002-add-profile-img.sql +++ b/src/backend/migrations/revert/002-add-profile-img.sql @@ -6,4 +6,4 @@ ALTER TABLE IF EXISTS public.users DROP COLUMN IF EXISTS profile_img; -- Commit the transaction -COMMIT; \ No newline at end of file +COMMIT; diff --git a/src/backend/migrations/revert/004-organisation-odk-creds.sql b/src/backend/migrations/revert/004-organisation-odk-creds.sql new file mode 100644 index 0000000000..263595afa4 --- /dev/null +++ b/src/backend/migrations/revert/004-organisation-odk-creds.sql @@ -0,0 +1,13 @@ +-- Start a transaction +BEGIN; + +-- Remove the odk central credentials columns and approved column +--- from the public.organisations table +ALTER TABLE IF EXISTS public.organisations + DROP COLUMN IF EXISTS approved, + DROP COLUMN IF EXISTS odk_central_url CASCADE, + DROP COLUMN IF EXISTS odk_central_user CASCADE, + DROP COLUMN IF EXISTS odk_central_password CASCADE; + +-- Commit the transaction +COMMIT; diff --git a/src/frontend/src/models/createproject/createProjectModel.ts b/src/frontend/src/models/createproject/createProjectModel.ts index cd89ed1893..1c78c11d86 100755 --- a/src/frontend/src/models/createproject/createProjectModel.ts +++ b/src/frontend/src/models/createproject/createProjectModel.ts @@ -74,7 +74,6 @@ export interface OrganisationListModel { slug: string; description: string; type: number; - subscription_tier: null | string; id: number; logo: string; url: string; diff --git a/src/frontend/src/models/organization/organisationModel.ts b/src/frontend/src/models/organisation/organisationModel.ts similarity index 88% rename from src/frontend/src/models/organization/organisationModel.ts rename to src/frontend/src/models/organisation/organisationModel.ts index 02278511d3..50e913c78e 100644 --- a/src/frontend/src/models/organization/organisationModel.ts +++ b/src/frontend/src/models/organisation/organisationModel.ts @@ -14,7 +14,6 @@ export interface OrganisationListModel { slug: string; description: string; type: number; - subscription_tier: null | string; id: number; logo: string; url: string; @@ -25,7 +24,6 @@ export interface GetOrganisationDataModel { slug: string; description: string; type: number; - subscription_tier: null; id: number; logo: string; url: string; @@ -35,7 +33,6 @@ export interface PostOrganisationDataModel { slug: string; description: string; type: number; - subscription_tier: null; id: number; logo: string; url: string;