Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restrict dataset creation endpoint to users in requested organization #435

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion client/src/lib/repositories/datasets.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,16 +84,18 @@ export const getDatasets: GetDatasets = async ({
type CreateDataset = (opts: {
fetch: Fetch;
apiToken: string;
siret: string;
data: DatasetCreateData;
}) => Promise<Maybe<Dataset>>;

export const createDataset: CreateDataset = async ({
fetch,
apiToken,
siret,
data,
}) => {
const body = JSON.stringify(toPayload(data));
const url = `${getApiUrl()}/datasets/`;
const url = `${getApiUrl()}/catalogs/${siret}/datasets/`;
const request = new Request(url, {
method: "POST",
headers: new Headers([
Expand Down
1 change: 1 addition & 0 deletions client/src/routes/(app)/contribuer/+page.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
const dataset = await createDataset({
fetch,
apiToken: $apiTokenStore,
siret: Maybe.expect(catalog, "catalog").organization.siret,
data: { tagIds, ...event.detail },
});

Expand Down
6 changes: 4 additions & 2 deletions client/src/tests/e2e/fixtures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,9 @@ export const test = base.extend<AppTestArgs>({
dataset: async ({ apiContext, adminApiToken }, use) => {
const headers = { Authorization: `Bearer ${adminApiToken}` };

const siret = "11004601800013";

const data = {
organization_siret: "11004601800013",
title: "Sample title",
description: "Sample description",
formats: ["api"],
Expand All @@ -77,7 +78,8 @@ export const test = base.extend<AppTestArgs>({
},
],
};
let response = await apiContext.post("/datasets/", {

let response = await apiContext.post(`/catalogs/${siret}/datasets/`, {
data,
headers,
});
Expand Down
34 changes: 22 additions & 12 deletions server/api/datasets/routes.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from fastapi.exceptions import HTTPException
from starlette.responses import Response

from server.api.organizations.permissions import MatchesUserSiret
from server.application.datasets.commands import (
CreateDataset,
DeleteDataset,
Expand All @@ -11,24 +12,25 @@
from server.application.datasets.views import DatasetView
from server.config.di import resolve
from server.domain.auth.entities import UserRole
from server.domain.catalogs.exceptions import CatalogDoesNotExist
from server.domain.common.pagination import Page, Pagination
from server.domain.common.types import ID
from server.domain.datasets.exceptions import DatasetDoesNotExist
from server.domain.datasets.specifications import DatasetSpec
from server.domain.organizations.exceptions import OrganizationDoesNotExist
from server.domain.organizations.types import Siret
from server.seedwork.application.messages import MessageBus

from ..auth.permissions import HasRole, IsAuthenticated
from . import filters
from .schemas import DatasetCreate, DatasetListParams, DatasetUpdate

router = APIRouter(prefix="/datasets", tags=["datasets"])
router = APIRouter(tags=["datasets"])

router.include_router(filters.router)
router.include_router(filters.router, prefix="/datasets")


@router.get(
"/",
"/datasets/",
dependencies=[Depends(IsAuthenticated())],
response_model=Pagination[DatasetView],
)
Expand Down Expand Up @@ -56,7 +58,7 @@ async def list_datasets(


@router.get(
"/{id}/",
"/datasets/{id}/",
dependencies=[Depends(IsAuthenticated())],
response_model=DatasetView,
responses={404: {}},
Expand All @@ -72,27 +74,35 @@ async def get_dataset_by_id(id: ID) -> DatasetView:


@router.post(
"/",
dependencies=[Depends(IsAuthenticated())],
"/catalogs/{siret}/datasets/",
dependencies=[
Depends(
IsAuthenticated()
& (
HasRole(UserRole.ADMIN)
| MatchesUserSiret(key=lambda r: r.path_params["siret"])
)
)
],
response_model=DatasetView,
status_code=201,
)
async def create_dataset(data: DatasetCreate) -> DatasetView:
async def create_dataset(siret: Siret, data: DatasetCreate) -> DatasetView:
bus = resolve(MessageBus)

command = CreateDataset(**data.dict())
command = CreateDataset(organization_siret=siret, **data.dict())

try:
id = await bus.execute(command)
except OrganizationDoesNotExist as exc:
except CatalogDoesNotExist as exc:
raise HTTPException(400, detail=str(exc))

query = GetDatasetByID(id=id)
return await bus.execute(query)


@router.put(
"/{id}/",
"/datasets/{id}/",
dependencies=[Depends(IsAuthenticated())],
response_model=DatasetView,
responses={404: {}},
Expand All @@ -112,7 +122,7 @@ async def update_dataset(id: ID, data: DatasetUpdate) -> DatasetView:


@router.delete(
"/{id}/",
"/datasets/{id}/",
dependencies=[Depends(IsAuthenticated() & HasRole(UserRole.ADMIN))],
status_code=204,
response_class=Response,
Expand Down
3 changes: 0 additions & 3 deletions server/api/datasets/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@
)
from server.domain.common.types import ID
from server.domain.datasets.entities import DataFormat, UpdateFrequency
from server.domain.organizations.entities import LEGACY_ORGANIZATION
from server.domain.organizations.types import Siret


class DatasetListParams:
Expand Down Expand Up @@ -40,7 +38,6 @@ def __init__(


class DatasetCreate(CreateDatasetValidationMixin, BaseModel):
organization_siret: Siret = LEGACY_ORGANIZATION.siret
title: str
description: str
service: str
Expand Down
22 changes: 22 additions & 0 deletions server/api/organizations/permissions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
from typing import Callable

from ..auth.permissions import BasePermission
from ..types import APIRequest


class MatchesUserSiret(BasePermission):
def __init__(self, key: Callable[[APIRequest], str]) -> None:
super().__init__()
self._key = key

def has_permission(self, request: APIRequest) -> bool:
if not request.user.is_authenticated:
raise RuntimeError(
"Running BelongsToOrganization but user is not authenticated. "
"Hint: use IsAuthenticated() & BelongsToOrganization(...)"
)

user_siret = request.user.account.organization_siret
claimed_siret = self._key(request)

return user_siret == claimed_siret
16 changes: 7 additions & 9 deletions server/application/datasets/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@
from server.config.di import resolve
from server.domain.catalog_records.entities import CatalogRecord
from server.domain.catalog_records.repositories import CatalogRecordRepository
from server.domain.catalogs.exceptions import CatalogDoesNotExist
from server.domain.catalogs.repositories import CatalogRepository
from server.domain.common.pagination import Pagination
from server.domain.common.types import ID
from server.domain.datasets.entities import DataFormat, Dataset
from server.domain.datasets.exceptions import DatasetDoesNotExist
from server.domain.datasets.repositories import DatasetRepository
from server.domain.organizations.exceptions import OrganizationDoesNotExist
from server.domain.organizations.repositories import OrganizationRepository
from server.domain.tags.repositories import TagRepository
from server.seedwork.application.messages import MessageBus

Expand All @@ -20,24 +20,22 @@

async def create_dataset(command: CreateDataset, *, id_: ID = None) -> ID:
repository = resolve(DatasetRepository)
organization_repository = resolve(OrganizationRepository)
catalog_repository = resolve(CatalogRepository)
catalog_record_repository = resolve(CatalogRecordRepository)
tag_repository = resolve(TagRepository)

if id_ is None:
id_ = repository.make_id()

organization = await organization_repository.get_by_siret(
siret=command.organization_siret
)
catalog = await catalog_repository.get_by_siret(siret=command.organization_siret)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

J'ai besoin de ce changement qui corrige un bug au passage : il ne faut pas tant vérifier que l'orga existe que son catalogue existe. Si une orga existait mais qu'elle n'avait pas de catalogue, auparavant on se retrouvait avec une IntegrityError renvoyée par Postgres (= erreur 500). Désormais on a bien une 400 Bad Request.


if organization is None:
raise OrganizationDoesNotExist(command.organization_siret)
if catalog is None:
raise CatalogDoesNotExist(command.organization_siret)

catalog_record_id = await catalog_record_repository.insert(
CatalogRecord(
id=catalog_record_repository.make_id(),
organization=organization,
organization=catalog.organization,
)
)
catalog_record = await catalog_record_repository.get_by_id(catalog_record_id)
Expand Down
Loading