diff --git a/ooniapi/common/src/common/alembic/versions/981d92cf8790_init_tables.py b/ooniapi/common/src/common/alembic/versions/981d92cf8790_init_tables.py index 147afeb4..c7e46941 100644 --- a/ooniapi/common/src/common/alembic/versions/981d92cf8790_init_tables.py +++ b/ooniapi/common/src/common/alembic/versions/981d92cf8790_init_tables.py @@ -84,4 +84,4 @@ def upgrade() -> None: def downgrade() -> None: op.drop_table("oonirun_nettest") - op.drop_table("oonirun") \ No newline at end of file + op.drop_table("oonirun") diff --git a/ooniapi/common/src/common/alembic/versions/a037e908f3a0_init_oonifindings.py b/ooniapi/common/src/common/alembic/versions/a037e908f3a0_init_oonifindings.py new file mode 100644 index 00000000..462f7099 --- /dev/null +++ b/ooniapi/common/src/common/alembic/versions/a037e908f3a0_init_oonifindings.py @@ -0,0 +1,54 @@ +"""init oonifindings + +Revision ID: a037e908f3a0 +Revises: c9119c05cf42 +Create Date: 2024-07-17 16:45:25.752551 + +""" +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision: str = 'a037e908f3a0' +down_revision: Union[str, None] = "c9119c05cf42" +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + op.create_table( + "oonifinding", + sa.Column( + "finding_id", + sa.String(), + nullable=False, + primary_key=True, + ), + sa.Column("finding_slug", sa.String(), nullable=True), + sa.Column("create_time", sa.DateTime(timezone=True), nullable=False), + sa.Column("update_time", sa.DateTime(timezone=True), nullable=False), + sa.Column("start_time", sa.DateTime(timezone=True), nullable=True), + sa.Column("end_time", sa.DateTime(timezone=True), nullable=True), + sa.Column("creator_account_id", sa.String(), nullable=False), + sa.Column("title", sa.String(), nullable=False), + sa.Column("short_description", sa.String(), nullable=False), + sa.Column("text", sa.String(), nullable=False), + sa.Column("reported_by", sa.String(), nullable=False), + sa.Column("email_address", sa.String(), nullable=False), + sa.Column("event_type", sa.String(), nullable=False), + sa.Column("published", sa.Integer(), nullable=False), + sa.Column("deleted", sa.Integer(), nullable=False, default=0), + sa.Column("country_codes", sa.JSON(), nullable=True), + sa.Column("asns", sa.JSON(), nullable=True), + sa.Column("domains", sa.JSON(), nullable=True), + sa.Column("tags", sa.JSON(), nullable=True), + sa.Column("links", sa.JSON(), nullable=True), + sa.Column("test_names", sa.JSON(), nullable=True), + ) + + +def downgrade() -> None: + op.drop_table("oonifinding") diff --git a/ooniapi/common/src/common/models.py b/ooniapi/common/src/common/models.py index 2f74f40b..fb346b86 100644 --- a/ooniapi/common/src/common/models.py +++ b/ooniapi/common/src/common/models.py @@ -1,5 +1,4 @@ from datetime import datetime, timezone -from typing import List, Dict, Any from sqlalchemy.types import DateTime, TypeDecorator diff --git a/ooniapi/services/oonifindings/migrations/clickhouse_init_tables.sql b/ooniapi/services/oonifindings/migrations/clickhouse_init_tables.sql deleted file mode 100644 index 8216cc7f..00000000 --- a/ooniapi/services/oonifindings/migrations/clickhouse_init_tables.sql +++ /dev/null @@ -1,28 +0,0 @@ --- Create tables for integration tests - -CREATE TABLE default.incidents -( - `id` String, - `title` String, - `short_description` String, - `text` String, - `start_time` Datetime DEFAULT now(), - `end_time` Nullable(Datetime), - `create_time` Datetime, - `update_time` Datetime DEFAULT now(), - `creator_account_id` FixedString(32), - `reported_by` String, - `email_address` String, - `event_type` LowCardinality(String), - `published` UInt8, - `deleted` UInt8 DEFAULT 0, - `CCs` Array(FixedString(2)), - `ASNs` Array(String), - `domains` Array(String), - `tags` Array(String), - `links` Array(String), - `test_names` Array(String), -) -ENGINE=ReplacingMergeTree -ORDER BY (create_time, creator_account_id, id) -SETTINGS index_granularity = 8192; diff --git a/ooniapi/services/oonifindings/pyproject.toml b/ooniapi/services/oonifindings/pyproject.toml index ccfeff33..ac444d23 100644 --- a/ooniapi/services/oonifindings/pyproject.toml +++ b/ooniapi/services/oonifindings/pyproject.toml @@ -16,7 +16,9 @@ dependencies = [ "statsd ~= 4.0.1", "uvicorn ~= 0.25.0", "httpx ~= 0.26.0", + "psycopg2 ~= 2.9.9", "pyjwt ~= 2.8.0", + "alembic ~= 1.13.1", "prometheus-fastapi-instrumentator ~= 6.1.0", "prometheus-client", ] @@ -55,9 +57,6 @@ include = ["BUILD_LABEL"] packages = ["src/oonifindings"] artifacts = ["BUILD_LABEL"] -[tool.hatch.metadata] -allow-direct-references = true - [tool.hatch.envs.default] dependencies = [ "pytest", @@ -65,7 +64,7 @@ dependencies = [ "click", "black", "pytest-asyncio", - "pytest-docker", + "pytest-postgresql", ] path = ".venv/" @@ -78,12 +77,8 @@ cov = ["test-cov", "cov-report"] [[tool.hatch.envs.all.matrix]] python = ["3.8", "3.9", "3.10", "3.11", "3.12"] -[tool.hatch.envs.types] -dependencies = [ - "mypy>=1.0.0", -] -[tool.hatch.envs.types.scripts] -check = "mypy --install-types --non-interactive {args:src/oonifindings tests}" +[tool.pytest.ini_options] +addopts = ["--import-mode=importlib"] [tool.coverage.run] source_pkgs = ["oonifindings", "tests"] diff --git a/ooniapi/services/oonifindings/src/oonifindings/dependencies.py b/ooniapi/services/oonifindings/src/oonifindings/dependencies.py index f3d86738..8e23f796 100644 --- a/ooniapi/services/oonifindings/src/oonifindings/dependencies.py +++ b/ooniapi/services/oonifindings/src/oonifindings/dependencies.py @@ -2,14 +2,19 @@ from fastapi import Depends -from clickhouse_driver import Client as Clickhouse +from sqlalchemy import create_engine +from sqlalchemy.orm import sessionmaker from .common.config import Settings from .common.dependencies import get_settings -def get_clickhouse_session(settings: Annotated[Settings, Depends(get_settings)]): - db = Clickhouse.from_url(settings.clickhouse_url) + +def get_postgresql_session(settings: Annotated[Settings, Depends(get_settings)]): + engine = create_engine(settings.postgresql_url) + SessionLocal = sessionmaker(autocommit=False, autoflush=False, bind=engine) + + db = SessionLocal() try: yield db - finally: - db.disconnect() + finally: + db.close() diff --git a/ooniapi/services/oonifindings/src/oonifindings/main.py b/ooniapi/services/oonifindings/src/oonifindings/main.py index 3f003c8a..91d29c64 100644 --- a/ooniapi/services/oonifindings/src/oonifindings/main.py +++ b/ooniapi/services/oonifindings/src/oonifindings/main.py @@ -8,14 +8,13 @@ from prometheus_fastapi_instrumentator import Instrumentator +from . import models from .routers import v1 -from .dependencies import get_settings, get_clickhouse_session +from .dependencies import get_settings, get_postgresql_session from .common.version import get_build_label, get_pkg_version -from .common.clickhouse_utils import query_click from .common.metrics import mount_metrics - pkg_name = "oonifindings" pkg_version = get_pkg_version(pkg_name) @@ -67,17 +66,12 @@ class HealthStatus(BaseModel): @app.get("/health") async def health( settings=Depends(get_settings), - db=Depends(get_clickhouse_session), + db=Depends(get_postgresql_session), ): errors = [] try: - query = f"""SELECT id, update_time, start_time, end_time, reported_by, - title, event_type, published, CCs, ASNs, domains, tags, test_names, - links, short_description, email_address, create_time, creator_account_id - FROM incidents FINAL - """ - query_click(db=db, query=query, query_params={}) + db.query(models.OONIFinding).limit(1).all() except Exception as exc: log.error(exc) errors.append("db_error") @@ -86,7 +80,7 @@ async def health( err = "bad_jwt_secret" log.error(err) errors.append(err) - + if settings.prometheus_metrics_password == "CHANGEME": err = "bad_prometheus_password" log.error(err) @@ -94,9 +88,9 @@ async def health( if len(errors) > 0: raise HTTPException(status_code=400, detail="health check failed") - + status = "ok" - + return { "status": status, "errors": errors, diff --git a/ooniapi/services/oonifindings/src/oonifindings/models.py b/ooniapi/services/oonifindings/src/oonifindings/models.py new file mode 100644 index 00000000..c832274b --- /dev/null +++ b/ooniapi/services/oonifindings/src/oonifindings/models.py @@ -0,0 +1,40 @@ +from datetime import datetime +from typing import List +from sqlalchemy import String +from sqlalchemy.orm import Mapped +from sqlalchemy.orm import mapped_column + +from .common.models import UtcDateTime +from .common.postgresql import Base + + +class OONIFinding(Base): + __tablename__ = "oonifinding" + + finding_id: Mapped[str] = mapped_column(String, primary_key=True) + # TODO(decfox): this is nullable for now. We should + # make this a non-nullable field eventually and have an endpoint + # where we can query findings using the finding_slug. + finding_slug: Mapped[str] = mapped_column(String, nullable=True) + + create_time: Mapped[datetime] = mapped_column(UtcDateTime()) + update_time: Mapped[datetime] = mapped_column(UtcDateTime()) + start_time: Mapped[datetime] = mapped_column(UtcDateTime(), nullable=True) + end_time: Mapped[datetime] = mapped_column(UtcDateTime(), nullable=True) + creator_account_id: Mapped[str] = mapped_column(String(32)) + + title: Mapped[str] = mapped_column() + short_description: Mapped[str] = mapped_column() + text: Mapped[str] = mapped_column() + reported_by: Mapped[str] = mapped_column() + email_address: Mapped[str] = mapped_column() + event_type: Mapped[str] = mapped_column() + published: Mapped[int] = mapped_column() + deleted: Mapped[int] = mapped_column(default=0) + + country_codes: Mapped[List[str]] = mapped_column(nullable=True) + asns: Mapped[List[str]] = mapped_column(nullable=True) + domains: Mapped[List[str]] = mapped_column(nullable=True) + tags: Mapped[List[str]] = mapped_column(nullable=True) + links: Mapped[List[str]] = mapped_column(nullable=True) + test_names: Mapped[List[str]] = mapped_column(nullable=True) diff --git a/ooniapi/services/oonifindings/src/oonifindings/routers/v1.py b/ooniapi/services/oonifindings/src/oonifindings/routers/v1.py index 301af35c..415ff7ce 100644 --- a/ooniapi/services/oonifindings/src/oonifindings/routers/v1.py +++ b/ooniapi/services/oonifindings/src/oonifindings/routers/v1.py @@ -3,26 +3,26 @@ """ from datetime import datetime, timezone -from typing import List, Dict, Optional, Union, Tuple, Any, Annotated +from typing import List, Optional, Union, Tuple, Any, Annotated import logging -from clickhouse_driver import Client as Clickhouse +import sqlalchemy as sa from fastapi import APIRouter, Depends, Header, Response, HTTPException, Query -from pydantic import Field +from pydantic import Field, ValidationInfo from pydantic.functional_validators import field_validator -from ..common.routers import BaseModel, ISO_FORMAT_DATE +from .. import models + +from ..common.routers import BaseModel from ..common.dependencies import get_settings, role_required from ..common.auth import ( - check_email_address, get_account_id_or_raise, - get_account_id_or_none, + get_account_id_or_none, get_client_role ) from ..common.utils import setnocacheresponse, generate_random_intuid -from ..common.clickhouse_utils import query_click, raw_query, insert_click, optimize_table -from ..dependencies import get_clickhouse_session +from ..dependencies import get_postgresql_session log = logging.getLogger(__name__) @@ -36,7 +36,8 @@ def utcnow_seconds(): class OONIFindingId(BaseModel): incident_id: str = Field( alias="id" - ) + ) + class OONIFindingWithMail(OONIFindingId): email_address: str = Field( @@ -70,7 +71,7 @@ class OONIFinding(OONIFindingWithMail): default="", title="account id of the oonifinding report creator" ) published: bool = Field( - default=False, title="binary check if event is published" + default=False, title="binary check if event is published" ) event_type: str = Field( default="", title="type of oonifinding event" @@ -97,6 +98,17 @@ class OONIFinding(OONIFindingWithMail): default=False, title="check if creator account id matches user" ) + @field_validator("end_time") + @classmethod + def check_time_difference(cls, end_time: datetime, info: ValidationInfo): + start_time = info.data.get('start_time') + if end_time and start_time: + start_time = start_time.replace(microsecond=0) + end_time = end_time.replace(microsecond=0) + delta = end_time - start_time + if delta.total_seconds() < 0: + raise ValueError("invalid start and end time") + class OONIFindingWithText(OONIFinding): text: str = Field( @@ -122,7 +134,7 @@ class OONIFindingIncidents(BaseModel): @router.get( "/v1/incidents/search", tags=["oonifindings"], - response_model = OONIFindingIncidents + response_model=OONIFindingIncidents ) def list_oonifindings( only_mine: Annotated[ @@ -131,173 +143,153 @@ def list_oonifindings( ], response: Response, authorization: str = Header("authorization"), - db=Depends(get_clickhouse_session), + db=Depends(get_postgresql_session), settings=Depends(get_settings), ): """ Search and list incidents """ log.debug("listing incidents") - where = "WHERE deleted != 1" - query_params = {} + client_role = get_client_role(authorization, jwt_encryption_key=settings.jwt_encryption_key) account_id = get_account_id_or_none( authorization, jwt_encryption_key=settings.jwt_encryption_key ) - if only_mine: - if account_id is None: - return OONIFindingIncidents(incidents=[]) - where += "\nAND creator_account_id = %(account_id)s" + + q = db.query(models.OONIFinding).filter( + models.OONIFinding.deleted != 1, + sa.or_(not only_mine, models.OONIFinding.creator_account_id == account_id) + ) if account_id is None: # non-published incidents are not exposed to anon users - where += "\nAND published = 1" - query_params["account_id"] = "never-match" - else: - query_params["account_id"] = account_id - - query = f"""SELECT id, update_time, start_time, end_time, reported_by, - title, event_type, published, CCs, ASNs, domains, tags, test_names, - links, short_description, email_address, create_time, - creator_account_id = %(account_id)s as mine - FROM incidents FINAL - {where} - ORDER BY title - """ - q = query_click(db=db, query=query, query_params=query_params) - - incidents = list(q) - client_role = get_client_role(authorization, jwt_encryption_key=settings.jwt_encryption_key) - for incident in incidents: - incident["published"] = bool(incident["published"]) + q = q.filter(models.OONIFinding.published == 1) + account_id = "never-match" + + findings = [] + for row in q.all(): + oonifinding = OONIFinding( + id=row.finding_id, + update_time=row.update_time, + start_time=row.start_time, + end_time=row.end_time, + reported_by=row.reported_by, + title=row.title, + event_type=row.event_type, + published=bool(row.published), + CCs=row.country_codes, + ASNs=row.asns, + domains=row.domains, + tags=row.tags, + test_names=row.test_names, + links=row.links, + short_description=row.short_description, + email_address=row.email_address, + create_time=row.create_time, + mine=(row.creator_account_id == account_id) + ) + if account_id is None or client_role != "admin": - incident["email_address"] = "" - + oonifinding.email_address = "" + + findings.append(oonifinding) + setnocacheresponse(response) - incident_models = [] - for i in range(len(incidents)): - incident = incidents[i] - incident_model = OONIFinding.model_validate(incident) - incident_models.append(incident_model) - return OONIFindingIncidents(incidents=incident_models) + return OONIFindingIncidents(incidents=findings) @router.get( - "/v1/incidents/show/{incident_id}", + "/v1/incidents/show/{finding_id}", tags=["oonifindings"], response_model=OONIFindingIncident ) def get_oonifinding_by_id( - incident_id: str, + finding_id: str, response: Response, authorization: str = Header("authorization"), - db=Depends(get_clickhouse_session), + db=Depends(get_postgresql_session), settings=Depends(get_settings) ): """ Returns an incident """ log.debug("showing incident") - where = "WHERE id = %(id)s AND deleted != 1" + + client_role = get_client_role(authorization, jwt_encryption_key=settings.jwt_encryption_key) account_id = get_account_id_or_none( authorization, jwt_encryption_key=settings.jwt_encryption_key ) + + q = db.query(models.OONIFinding).filter( + models.OONIFinding.finding_id == finding_id, + models.OONIFinding.deleted != 1, + ) + + # non-published incidents are not exposed to anon users if account_id is None: - # non-published incidents are not exposed to anon users - where += "\nAND published = 1" - query_params = {"id": incident_id, "account_id": "never-match"} - else: - query_params = {"id": incident_id, "account_id": account_id} - - query = f"""SELECT id, update_time, start_time, end_time, reported_by, - title, text, event_type, published, CCs, ASNs, domains, tags, test_names, - links, short_description, email_address, create_time, - creator_account_id = %(account_id)s AS mine - FROM incidents FINAL - {where} - LIMIT 1 - """ - q = query_click(db=db, query=query, query_params=query_params) - if len(q) < 1: - raise HTTPException(status_code=404, detail="Incident not found") + q = q.filter(models.OONIFinding.published == 1) + account_id = "never-match" + + try: + finding = q.one() + except sa.exc.NoResultFound: + raise HTTPException(status_code=404, detail="OONI Finding not found") + + oonifinding = OONIFindingWithText( + id=finding.finding_id, + update_time=finding.update_time, + start_time=finding.start_time, + end_time=finding.end_time, + reported_by=finding.reported_by, + title=finding.title, + text=finding.text, + event_type=finding.event_type, + published=bool(finding.published), + CCs=finding.country_codes, + ASNs=finding.asns, + domains=finding.domains, + tags=finding.tags, + test_names=finding.test_names, + links=finding.links, + short_description=finding.short_description, + email_address=finding.email_address, + create_time=finding.create_time, + mine=(finding.creator_account_id == account_id) + ) - incident = q[0] - incident["published"] = bool(incident["published"]) - client_role = get_client_role(authorization, jwt_encryption_key=settings.jwt_encryption_key) if account_id is None or client_role != "admin": - incident["email_address"] = "" # hide email - + oonifinding.email_address = "" # hide email + # TODO: cache if possible setnocacheresponse(response) - incident_model = OONIFindingWithText.model_validate(incident) - return OONIFindingIncident(incident=incident_model) - - -def prepare_incident_dict(incident: OONIFinding) -> Dict: - incident.start_time = incident.start_time.replace(microsecond=0) - if incident.end_time is not None: - incident.end_time = incident.end_time.replace(microsecond=0) - delta = incident.end_time - incident.start_time - if delta.total_seconds() < 0: - raise HTTPException(status_code=400, detail="invalid query paramters") - incident_dict = incident.model_dump(by_alias=True) - return incident_dict - - -def user_cannot_update( - db: Clickhouse, - authorization: str, - jwt_encryption_key: str, - incident_id: str, -) -> bool: - # Check if there is already an incident and belongs to a different user - query = """SELECT count() AS cnt - FROM incidents FINAL - WHERE deleted != 1 - AND id = %(incident_id)s - AND creator_account_id != %(account_id)s - """ - account_id = get_account_id_or_raise(authorization, jwt_encryption_key=jwt_encryption_key) - query_params = dict(incident_id=incident_id, account_id=account_id) - q = query_click(db, query, query_params) - return q[0]["cnt"] > 0 - - -def verify_user( - db: Clickhouse, - authorization: str, - jwt_encryption_key: str, - incident_id: str, - email_address: str, - key: str -): - if user_cannot_update( - db, authorization, jwt_encryption_key=jwt_encryption_key, incident_id=incident_id - ): - raise HTTPException(status_code=400, detail="Attempted to create, update or delete an item belonging to another user") - - if not check_email_address( - authorization=authorization, - jwt_encryption_key=jwt_encryption_key, - email_address=email_address, - key=key - ): - raise HTTPException(status_code=400, detail="Invalid email address for owner account") + return OONIFindingIncident(incident=oonifinding) class OONIFindingCreateUpdate(OONIFindingWithText): pass +# TODO(decfox): we maintain this pydantic model to ensure client response +# does not change. Eventually, we should get rid of this and simply +# return the updated sqlalchemy model. class OONIFindingsUpdateResponse(OONIFindingId): r: Union[int, Tuple[List[Any]]] = Field( default=0, title="result of the update operation" ) +def validate_time(incident: OONIFinding) -> bool: + incident.start_time = incident.start_time.replace(microsecond=0) + if incident.end_time is not None: + incident.end_time = incident.end_time.replace(microsecond=0) + delta = incident.end_time - incident.start_time + if delta.total_seconds() < 0: + raise HTTPException(status_code=400, detail="invalid query paramters") + return True + + @router.post( "/v1/incidents/create", - dependencies=[Depends(role_required(["admin"]))], tags=["oonifindings"], response_model=OONIFindingsUpdateResponse ) @@ -305,98 +297,115 @@ def create_oonifinding( create_request: OONIFindingCreateUpdate, response: Response, authorization: str = Header("authorization"), - db=Depends(get_clickhouse_session), + token=Depends(role_required(["admin"])), + db=Depends(get_postgresql_session), settings=Depends(get_settings) ): """ Create an incident """ - if not check_email_address( - authorization=authorization, - jwt_encryption_key=settings.jwt_encryption_key, - email_address=create_request.email_address, - key=settings.account_id_hashing_key - ): + if create_request.email_address != token["email_address"]: raise HTTPException(status_code=400, detail="Invalid email address for creator account") - + # assert create_request if create_request.published: raise HTTPException(status_code=400, detail="Invalid publish parameter on create request") - - incident_id = str(generate_random_intuid(collector_id=settings.collector_id)) - create_request.incident_id = incident_id - create_request.create_time = utcnow_seconds() - create_request.creator_account_id = get_account_id_or_raise( + # TODO(decfox): evaluate if we can replace this with a simple getter + account_id = get_account_id_or_raise( authorization, jwt_encryption_key=settings.jwt_encryption_key - ) - incident_dict = prepare_incident_dict(incident=create_request) + ) + now = utcnow_seconds() + finding_id = str(generate_random_intuid(collector_id=settings.collector_id)) + + log.info(f"Creating incident {finding_id}") + + db_oonifinding = models.OONIFinding( + finding_id=finding_id, + create_time=now, + update_time=now, + start_time=create_request.start_time, + end_time=create_request.end_time, + creator_account_id=account_id, + title=create_request.title, + short_description=create_request.short_description, + text=create_request.text, + reported_by=create_request.reported_by, + email_address=create_request.email_address, + event_type=create_request.event_type, + published=int(create_request.published), + country_codes=create_request.CCs, + asns=create_request.ASNs, + domains=create_request.domains, + tags=create_request.tags, + links=create_request.links, + test_names=create_request.test_names, + ) - log.info(f"Creating incident {incident_id}") + db.add(db_oonifinding) + db.commit() - query = """INSERT INTO incidents - (id, start_time, end_time, creator_account_id, reported_by, title, - text, event_type, published, CCs, ASNs, domains, tags, links, - test_names, short_description, email_address, create_time) - VALUES - """ - r = insert_click(db, query, [incident_dict]) - optimize_table(db, tblname="incidents") - setnocacheresponse(response) - return OONIFindingsUpdateResponse(r=r, id=incident_id) + return OONIFindingsUpdateResponse(r=1, id=finding_id) @router.post( "/v1/incidents/update", - dependencies=[Depends(role_required(["admin", "user"]))], tags=["oonifindings"], - response_model=OONIFindingsUpdateResponse + response_model=OONIFindingsUpdateResponse ) def update_oonifinding( update_request: OONIFindingCreateUpdate, response: Response, - authorization: str = Header("authorization"), - db=Depends(get_clickhouse_session), + db=Depends(get_postgresql_session), token=Depends(role_required(["admin", "user"])), - settings=Depends(get_settings) ): """ Update an incident - """ - incident_id = update_request.incident_id - if token["role"] != "admin": - verify_user( - db, - authorization=authorization, - jwt_encryption_key=settings.jwt_encryption_key, - incident_id=incident_id, - email_address=update_request.email_address, - key=settings.account_id_hashing_key, - ) - - if update_request.published is True: - raise HTTPException(status_code=400, detail="Not enough permissions to publish") - - update_request.creator_account_id = get_account_id_or_raise( - authorization, jwt_encryption_key=settings.jwt_encryption_key - ) - incident_dict = prepare_incident_dict(update_request) + """ + finding_id = update_request.incident_id + account_id = token["account_id"] + + if token["role"] == "user": + if update_request.email_address != token["email_address"]: + raise HTTPException(status_code=403, detail="You are not allowed to set the email address to something other than your email address") + if update_request.published: + raise HTTPException(status_code=403, detail="You are not allowed to publish") + else: + assert token["role"] == "admin" - log.info(f"Updating incident {incident_id}") + q = db.query(models.OONIFinding).filter( + models.OONIFinding.finding_id == finding_id, + sa.or_(token["role"] != "user", models.OONIFinding.creator_account_id == account_id) + ) + try: + oonifinding = q.one() + except sa.exc.NoResultFound: + raise HTTPException(status_code=404, detail="OONI Finding not found") + + log.info(f"Updating incident {finding_id}") + + now = utcnow_seconds() + oonifinding.update_time = now + oonifinding.start_time = update_request.start_time + oonifinding.end_time = update_request.end_time + oonifinding.reported_by = update_request.reported_by + oonifinding.title = update_request.title + oonifinding.text = update_request.text + oonifinding.event_type = update_request.event_type + oonifinding.published = int(update_request.published) + oonifinding.country_codes = update_request.CCs + oonifinding.asns = update_request.ASNs + oonifinding.domains = update_request.domains + oonifinding.tags = update_request.tags + oonifinding.links = update_request.links + oonifinding.test_names = update_request.test_names + oonifinding.short_description = update_request.short_description + oonifinding.email_address = update_request.email_address + db.commit() - insert_query = """INSERT INTO incidents - (id, start_time, end_time, creator_account_id, reported_by, title, - text, event_type, published, CCs, ASNs, domains, tags, links, - test_names, short_description, email_address, create_time) - VALUES - """ - r = insert_click(db, insert_query, [incident_dict]) - log.debug(f"Result: {r}") - optimize_table(db, tblname="incidents") - setnocacheresponse(response) - return OONIFindingsUpdateResponse(r=r, id=incident_id) + return OONIFindingsUpdateResponse(r=1, id=finding_id) @router.post( @@ -406,37 +415,38 @@ def update_oonifinding( def delete_oonifinding( delete_request: OONIFindingWithMail, response: Response, - authorization: str = Header("authorization"), token=Depends(role_required(["admin", "user"])), - db=Depends(get_clickhouse_session), - settings=Depends(get_settings) + db=Depends(get_postgresql_session), ): """ Delete an incident """ assert delete_request - incident_id = delete_request.incident_id - if token["role"] != "admin": - try: - verify_user( - db, - authorization=authorization, - jwt_encryption_key=settings.jwt_encryption_key, - incident_id=incident_id, - email_address=delete_request.email_address, - key=settings.account_id_hashing_key, - ) - except: - raise - - query = "ALTER TABLE incidents DELETE WHERE id = %(incident_id)s" - r = raw_query(db, query, {"incident_id": incident_id}) - optimize_table(db, "incidents") + account_id = token["account_id"] + finding_id = delete_request.incident_id + + if token["role"] == "user": + if delete_request.email_address != token["email_address"]: + raise HTTPException(status_code=403, detail="You are not allowed to delete the incident") + else: + assert token["role"] == "admin" + + q = db.query(models.OONIFinding).filter( + models.OONIFinding.finding_id == finding_id, + sa.or_(token["role"] != "user", models.OONIFinding.creator_account_id == account_id) + ) + try: + q.one() + except sa.exc.NoResultFound: + raise HTTPException(status_code=404, detail="OONI Finding not found") + + q.delete() + db.commit() + setnocacheresponse(response) return {} - @router.post( "/v1/incidents/{action}", tags=["oonifindings"], @@ -447,7 +457,7 @@ def update_oonifinding_publish_status( action: str, publish_request: OONIFindingCreateUpdate, response: Response, - db=Depends(get_clickhouse_session), + db=Depends(get_postgresql_session), ): """ Publish/Unpublish an incident. @@ -456,24 +466,19 @@ def update_oonifinding_publish_status( raise HTTPException(status_code=400, detail="Invalid query action") assert publish_request - incident_id = publish_request.incident_id - - query = "SELECT * FROM incidents FINAL WHERE id = %(incident_id)s" - q = query_click(db, query, {"incident_id": incident_id}) - if len(q) < 1: - raise HTTPException(status_code=404, detail="Incident not found") - incident_dict = q[0] - incident_dict["published"] = bool(action == "publish") - - insert_query = """INSERT INTO incidents - (id, start_time, end_time, creator_account_id, reported_by, title, - text, event_type, published, CCs, ASNs, domains, tags, links, - test_names, short_description, email_address, create_time) - VALUES - """ - r = insert_click(db, insert_query, [incident_dict]) - log.debug(f"Result: {r}") - optimize_table(db, tblname="incidents") - + finding_id = publish_request.incident_id + + q = db.query(models.OONIFinding).filter( + models.OONIFinding.finding_id == finding_id + ) + + try: + oonifinding = q.one() + except sa.exc.NoResultFound: + raise HTTPException(status_code=404, detail="OONI Finding not found") + + oonifinding.published = int(action == "publish") + db.commit() + setnocacheresponse(response) - return OONIFindingsUpdateResponse(r=r, id=incident_id) + return OONIFindingsUpdateResponse(r=1, id=finding_id) diff --git a/ooniapi/services/oonifindings/tests/conftest.py b/ooniapi/services/oonifindings/tests/conftest.py index 31dbbbd5..5b217d07 100644 --- a/ooniapi/services/oonifindings/tests/conftest.py +++ b/ooniapi/services/oonifindings/tests/conftest.py @@ -5,7 +5,6 @@ import jwt from fastapi.testclient import TestClient -from clickhouse_driver import Client as ClickhouseClient from oonifindings.common.config import Settings from oonifindings.common.auth import hash_email_address @@ -15,65 +14,36 @@ THIS_DIR = Path(__file__).parent.resolve() -def is_clickhouse_running(url): - try: - with ClickhouseClient.from_url(url) as client: - client.execute("SELECT 1") - return True - except Exception: - return False - - -@pytest.fixture(scope="session") -def clickhouse_server(docker_ip, docker_services): - port = docker_services.port_for("clickhouse", 9000) - url = "clickhouse://{}:{}".format(docker_ip, port) - docker_services.wait_until_responsive( - timeout=30.0, pause=0.1, check=lambda: is_clickhouse_running(url) - ) - yield url - - -def run_migration(path: Path, click: ClickhouseClient): - sql_no_comment = "\n".join( - filter(lambda x: not x.startswith("--"), path.read_text().split("\n")) - ) - queries = sql_no_comment.split(";") - for q in queries: - q = q.strip() - if not q: - continue - click.execute(q) +def make_override_get_settings(**kw): + def override_get_settings(): + return Settings(**kw) + return override_get_settings -def create_db_for_fixture(conn_url): - try: - with ClickhouseClient.from_url(conn_url) as client: - migrations_dir = THIS_DIR / "migrations" - for fn in migrations_dir.iterdir(): - migration_path = fn.resolve() - run_migration(migration_path, click=client) - return conn_url - except Exception: - pytest.skip("database migration failed") +@pytest.fixture +def alembic_migration(postgresql): + from alembic import command + from alembic.config import Config -@pytest.fixture(scope="session") -def db(clickhouse_server): - yield create_db_for_fixture(conn_url=clickhouse_server) + db_url = f"postgresql://{postgresql.info.user}:@{postgresql.info.host}:{postgresql.info.port}/{postgresql.info.dbname}" + migrations_path = ( + Path(__file__).parent.parent / "src" / "oonifindings" / "common" / "alembic" + ).resolve() -def make_override_get_settings(**kw): - def override_get_settings(): - return Settings(**kw) + alembic_cfg = Config() + alembic_cfg.set_main_option("script_location", str(migrations_path)) + alembic_cfg.set_main_option("sqlalchemy.url", db_url) - return override_get_settings + command.upgrade(alembic_cfg, "head") + yield db_url @pytest.fixture def client_with_bad_settings(): app.dependency_overrides[get_settings] = make_override_get_settings( - clickhouse_url = "clickhouse://badhost:9000" + postgresql_url="postgresql://badhost:9000" ) client = TestClient(app) @@ -81,9 +51,9 @@ def client_with_bad_settings(): @pytest.fixture -def client(db): +def client(alembic_migration): app.dependency_overrides[get_settings] = make_override_get_settings( - clickhouse_url=db, + postgresql_url=alembic_migration, jwt_encryption_key="super_secure", prometheus_metrics_password="super_secure", account_id_hashing_key="super_secure" @@ -97,7 +67,7 @@ def create_jwt(payload: dict) -> str: return jwt.encode(payload, "super_secure", algorithm="HS256") -def create_session_token(account_id: str, role: str) -> str: +def create_session_token(account_id: str, email: str, role: str) -> str: now = int(time.time()) payload = { "nbf": now, @@ -105,15 +75,17 @@ def create_session_token(account_id: str, role: str) -> str: "exp": now + 10 * 86400, "aud": "user_auth", "account_id": account_id, + "email_address": email, "login_time": None, "role": role, } return create_jwt(payload) + @pytest.fixture def client_with_user_role(client): client = TestClient(app) - jwt_token = create_session_token("0" * 16, "user") + jwt_token = create_session_token("0" * 16, "oonitarian@example.com", "user") client.headers = {"Authorization": f"Bearer {jwt_token}"} yield client @@ -121,7 +93,7 @@ def client_with_user_role(client): @pytest.fixture def client_with_admin_role(client): client = TestClient(app) - jwt_token = create_session_token("0" * 16, "admin") + jwt_token = create_session_token("1" * 16, "admin@example.com", "admin") client.headers = {"Authorization": f"Bearer {jwt_token}"} yield client @@ -132,7 +104,7 @@ def client_with_hashed_email(client): def _hashed_email(email: str, role: str): client = TestClient(app) account_id = hash_email_address(email, "super_secure") - jwt_token = create_session_token(account_id, role) + jwt_token = create_session_token(account_id, email, role) client.headers = {"Authorization": f"Bearer {jwt_token}"} return client diff --git a/ooniapi/services/oonifindings/tests/docker-compose.yml b/ooniapi/services/oonifindings/tests/docker-compose.yml deleted file mode 100644 index 7546ca5b..00000000 --- a/ooniapi/services/oonifindings/tests/docker-compose.yml +++ /dev/null @@ -1,6 +0,0 @@ -version: '2' -services: - clickhouse: - image: "clickhouse/clickhouse-server" - ports: - - "9000:9000" diff --git a/ooniapi/services/oonifindings/tests/migrations/clickhouse_init_tables.sql b/ooniapi/services/oonifindings/tests/migrations/clickhouse_init_tables.sql deleted file mode 100644 index 8216cc7f..00000000 --- a/ooniapi/services/oonifindings/tests/migrations/clickhouse_init_tables.sql +++ /dev/null @@ -1,28 +0,0 @@ --- Create tables for integration tests - -CREATE TABLE default.incidents -( - `id` String, - `title` String, - `short_description` String, - `text` String, - `start_time` Datetime DEFAULT now(), - `end_time` Nullable(Datetime), - `create_time` Datetime, - `update_time` Datetime DEFAULT now(), - `creator_account_id` FixedString(32), - `reported_by` String, - `email_address` String, - `event_type` LowCardinality(String), - `published` UInt8, - `deleted` UInt8 DEFAULT 0, - `CCs` Array(FixedString(2)), - `ASNs` Array(String), - `domains` Array(String), - `tags` Array(String), - `links` Array(String), - `test_names` Array(String), -) -ENGINE=ReplacingMergeTree -ORDER BY (create_time, creator_account_id, id) -SETTINGS index_granularity = 8192; diff --git a/ooniapi/services/oonifindings/tests/test_database.py b/ooniapi/services/oonifindings/tests/test_database.py new file mode 100644 index 00000000..3d424f8a --- /dev/null +++ b/ooniapi/services/oonifindings/tests/test_database.py @@ -0,0 +1,138 @@ +from copy import deepcopy +from datetime import datetime, timedelta +import pathlib +import pytest + +import sqlalchemy as sa +from sqlalchemy.orm import sessionmaker +from sqlalchemy import create_engine + +from oonifindings import models +from oonifindings.routers.v1 import utcnow_seconds + +sample_start_time = (utcnow_seconds() + timedelta(minutes=-1)) + +SAMPLE_EMAIL = "sample@i.org" + +SAMPLE_OONIFINDING = { + "title": "sample oonifinding", + "short_description": "sample oonifinding description", + "reported_by": "sample user", + "email_address": SAMPLE_EMAIL, + "text": "this is a sample oonifinding incident", + "published": 0, + "event_type": "incident", + "start_time": sample_start_time, + "asns": [], + "country_codes": [ + "IN", "TZ", + ], + "tags": [], + "test_names": [ + "webconnectivity", + ], + "domains": [ + "www.google.com" + ], + "links": [] +} + +def config_alembic(db_url): + from alembic.config import Config + + migrations_path = ( + pathlib.Path(__file__).parent.parent / "src" / "oonifindings" / "common" / "alembic" + ).resolve() + + alembic_cfg = Config() + alembic_cfg.set_main_option("script_location", str(migrations_path)) + alembic_cfg.set_main_option("sqlalchemy.url", db_url) + return alembic_cfg + + +def upgrade_to_head(db_url): + from alembic import command + + command.upgrade(config_alembic(db_url), "head") + + +def get_db(pg_url): + engine = create_engine(pg_url) + SessionLocal = sessionmaker(autocommit=False, autoflush=False, bind=engine) + + return SessionLocal() + + +def test_downgrade(postgresql): + from alembic import command + + db_url = f"postgresql://{postgresql.info.user}:@{postgresql.info.host}:{postgresql.info.port}/{postgresql.info.dbname}" + + command.upgrade(config_alembic(db_url), "head") + command.downgrade(config_alembic(db_url), "-1") + + +def test_upgrade_to_head(postgresql): + db_url = f"postgresql://{postgresql.info.user}:@{postgresql.info.host}:{postgresql.info.port}/{postgresql.info.dbname}" + upgrade_to_head(db_url) + db = get_db(db_url) + + finding = deepcopy(SAMPLE_OONIFINDING) + + new_row = db.query(models.OONIFinding).first() + + db_finding = models.OONIFinding( + **finding, + finding_id="000000000", + finding_slug="2024-07-sample-finding", + create_time=utcnow_seconds(), + update_time=utcnow_seconds(), + creator_account_id="000000000", + ) + db.add(db_finding) + db.commit() + + new_row = db.query(models.OONIFinding).first() + assert new_row + + db.close() + + with pytest.raises(sa.exc.StatementError): + db_finding = models.OONIFinding( + **finding, + finding_id="000000000", + finding_slug="2024-07-sample-finding", + create_time="NOT A DATE", + update_time=utcnow_seconds(), + creator_account_id="000000000", + ) + db.add(db_finding) + db.commit() + db.rollback() + + with pytest.raises(sa.exc.StatementError): + naive_datetime = datetime.now() + db_finding = models.OONIFinding( + **finding, + finding_id="000000000", + finding_slug="2024-07-sample-finding", + create_time=naive_datetime, + update_time=utcnow_seconds(), + creator_account_id="000000000", + ) + db.add(db_finding) + db.commit() + db.rollback() + + with pytest.raises(sa.exc.StatementError): + db_finding = models.OONIFinding( + **finding, + finding_id="000000000", + finding_slug="2024-07-sample-finding", + create_time=None, + update_time=utcnow_seconds(), + creator_account_id="000000000", + ) + db.add(db_finding) + db.commit() + db.rollback() diff --git a/ooniapi/services/oonifindings/tests/test_integration.py b/ooniapi/services/oonifindings/tests/test_integration.py index 206e7df5..1c24f82a 100644 --- a/ooniapi/services/oonifindings/tests/test_integration.py +++ b/ooniapi/services/oonifindings/tests/test_integration.py @@ -13,22 +13,21 @@ @pytest.fixture -def server(clickhouse_server): - os.environ["CLICKHOUSE_URL"] = clickhouse_server +def server(alembic_migration): + os.environ["POSTGRESQL_URL"] = alembic_migration proc = Process( target=uvicorn.run, - args=("oonifindings.main:app"), - kwargs={"host": "127.0.0.1", "port": LISTEN_PORT, "log_level": "info"}, + args=("oonirun.main:app",), + kwargs={"host": "127.0.0.1", "port": LISTEN_PORT, "log_level": "debug"}, daemon=True, ) - proc.start() # Give it as second to start time.sleep(1) yield proc.kill() # Note: coverage is not being calculated properly - # TODO(art): https://pytest-cov.readthedocs.io/en/latest/subprocess-support.html + # TODO(decfox): https://pytest-cov.readthedocs.io/en/latest/subprocess-support.html proc.join() diff --git a/ooniapi/services/oonifindings/tests/test_oonifindings.py b/ooniapi/services/oonifindings/tests/test_oonifindings.py index 1256cefc..96ad842c 100644 --- a/ooniapi/services/oonifindings/tests/test_oonifindings.py +++ b/ooniapi/services/oonifindings/tests/test_oonifindings.py @@ -103,7 +103,7 @@ def test_oonifinding_creator_validation(client, client_with_hashed_email): sample_end_time = start_time + timedelta(minutes=-1) z["end_time"] = sample_end_time.strftime("%Y-%m-%dT%H:%M:%S.%fZ") r = client_with_admin_role.post("api/v1/incidents/create", json=z) - assert r.status_code == 400, "invalid end_time should be rejected" + assert r.status_code == 422, "invalid end_time should be rejected" sample_end_time = start_time + timedelta(minutes=1) z["end_time"] = sample_end_time.strftime("%Y-%m-%dT%H:%M:%S.%fZ") @@ -199,12 +199,12 @@ def test_oonifinding_delete(client, client_with_hashed_email): z["id"] = incident_id z["email_address"] = "" r = client_with_user_role.post("api/v1/incidents/delete", json=z) - assert r.status_code == 400 + assert r.status_code == 403 z["email_address"] = SAMPLE_EMAIL mismatched_client = client_with_hashed_email("user@ooni.org", "user") r = mismatched_client.post("api/v1/incidents/delete", json=z) - assert r.status_code == 400 + assert r.status_code == 403 r = client_with_user_role.post("api/v1/incidents/delete", json=z) assert r.status_code == 200 @@ -258,12 +258,12 @@ def test_oonifinding_update(client, client_with_hashed_email): incident_payload["email_address"] = "" r = client_with_user_role.post("api/v1/incidents/update", json=incident_payload) - assert r.status_code == 400, "cannot update with invalid email" + assert r.status_code == 403, "cannot update with invalid email" incident_payload["email_address"] = SAMPLE_EMAIL mismatched_client = client_with_hashed_email("user@ooni.org", "user") r = mismatched_client.post("api/v1/incidents/update", json=incident_payload) - assert r.status_code == 400, "email should match account id" + assert r.status_code == 403, "email should match account id" r = client_with_user_role.post("api/v1/incidents/update", json=incident_payload) assert r.status_code == 200 @@ -292,7 +292,7 @@ def test_oonifinding_update(client, client_with_hashed_email): incident_payload["published"] = True r = client_with_user_role.post("api/v1/incidents/update", json=incident_payload) - assert r.status_code == 400, "user role cannot publish incident" + assert r.status_code == 403, "user role cannot publish incident" r = client_with_admin_role.post("api/v1/incidents/update", json=incident_payload) assert r.status_code == 200 @@ -321,6 +321,11 @@ def test_oonifinding_workflow( assert r.json()["r"] == 1 assert r.headers["Cache-Control"] == "no-cache" + r = client_with_admin_role.post("api/v1/incidents/create", json=z) + assert r.status_code == 200 + assert r.json()["r"] == 1 + assert r.headers["Cache-Control"] == "no-cache" + incident_id = r.json()["id"] assert incident_id @@ -371,7 +376,7 @@ def test_oonifinding_workflow( r = client.get("api/v1/incidents/search?only_mine=false") assert r.status_code == 200 incidents = r.json()["incidents"] - assert len(incidents) == 2 + assert len(incidents) == 1 for incident in incidents: assert incident["email_address"] == "" assert incident["mine"] is False @@ -380,7 +385,7 @@ def test_oonifinding_workflow( r = client_with_user_role.get("api/v1/incidents/search?only_mine=false") incidents = r.json()["incidents"] - assert len(incidents) == 4 + assert len(incidents) == 2 for incident in incidents: assert incident["email_address"] == "" assert incident["mine"] is False @@ -388,7 +393,7 @@ def test_oonifinding_workflow( r = client_with_admin_role.get("api/v1/incidents/search?only_mine=false") incidents = r.json()["incidents"] - assert len(incidents) == 4 + assert len(incidents) == 2 for incident in incidents: assert incident["email_address"] == SAMPLE_EMAIL assert incident["mine"] is True @@ -409,7 +414,7 @@ def test_oonifinding_workflow( r = client_account_with_user_role.get("api/v1/incidents/search?only_mine=true") assert r.status_code == 200 incidents = r.json()["incidents"] - assert len(incidents) == 4 + assert len(incidents) == 2 for incident in incidents: assert incident["email_address"] == "" assert incident["mine"] is True @@ -418,7 +423,7 @@ def test_oonifinding_workflow( r = client_with_admin_role.get("api/v1/incidents/search?only_mine=true") assert r.status_code == 200 incidents = r.json()["incidents"] - assert len(incidents) == 4 + assert len(incidents) == 2 for incident in incidents: assert incident["email_address"] == SAMPLE_EMAIL assert incident["mine"] is True