diff --git a/backend/src/cms_backend/api/routes/titles.py b/backend/src/cms_backend/api/routes/titles.py index d4244c82..c05f3d80 100644 --- a/backend/src/cms_backend/api/routes/titles.py +++ b/backend/src/cms_backend/api/routes/titles.py @@ -668,9 +668,14 @@ class TitleUploaderGrantSchema(BaseModel): account_id: UUID +class AvailableTitleUploadersGetSchema(BaseModel): + skip: SkipField = 0 + limit: LimitFieldMax200 = 20 + + @router.get( "/{title_identifier}/uploaders", - dependencies=[Depends(require_permission(namespace="account", name="update"))], + dependencies=[Depends(require_permission(namespace="title", name="update"))], ) def get_title_uploaders( title_identifier: Annotated[NotEmptyString, Path()], @@ -701,9 +706,45 @@ def get_title_uploaders( ) +@router.get( + "/{title_identifier}/uploaders/available", + dependencies=[Depends(require_permission(namespace="title", name="update"))], +) +def get_available_title_uploaders( + title_identifier: Annotated[NotEmptyString, Path()], + params: Annotated[AvailableTitleUploadersGetSchema, Query()], + session: Annotated[OrmSession, Depends(gen_dbsession)], + accessible_collection_ids: Annotated[ + Sequence[UUID] | None, Depends(get_accessible_collection_ids) + ], + accessible_title_ids: Annotated[ + Sequence[UUID] | None, Depends(get_accessible_title_ids) + ], +) -> ListResponse[AccountSchema]: + """List title-uploader accounts which may be granted upload access to a title""" + title = db_title.get_title( + session, + title_identifier=title_identifier, + accessible_collection_ids=accessible_collection_ids, + accessible_title_ids=accessible_title_ids, + ) + results = db_title_permission.get_available_title_uploaders( + session, title.id, skip=params.skip, limit=params.limit + ) + return ListResponse[AccountSchema]( + meta=calculate_pagination_metadata( + nb_records=results.nb_records, + skip=params.skip, + limit=params.limit, + page_size=len(results.records), + ), + items=results.records, + ) + + @router.post( "/{title_identifier}/uploaders", - dependencies=[Depends(require_permission(namespace="account", name="update"))], + dependencies=[Depends(require_permission(namespace="title", name="update"))], ) def grant_title_uploader( title_identifier: Annotated[NotEmptyString, Path()], @@ -738,7 +779,7 @@ def grant_title_uploader( @router.delete( "/{title_identifier}/uploaders/{account_id}", - dependencies=[Depends(require_permission(namespace="account", name="update"))], + dependencies=[Depends(require_permission(namespace="title", name="update"))], ) def revoke_title_uploader( title_identifier: Annotated[NotEmptyString, Path()], diff --git a/backend/src/cms_backend/db/title_permission.py b/backend/src/cms_backend/db/title_permission.py index baadf785..50826d6d 100644 --- a/backend/src/cms_backend/db/title_permission.py +++ b/backend/src/cms_backend/db/title_permission.py @@ -1,11 +1,14 @@ from collections.abc import Sequence from uuid import UUID -from sqlalchemy import delete, select +from sqlalchemy import delete, exists, select from sqlalchemy.orm import Session as OrmSession +from cms_backend.db import account as db_account +from cms_backend.db import count_from_stmt from cms_backend.db.models import Account, TitlePermission from cms_backend.roles import RoleEnum +from cms_backend.schemas.orms import AccountSchema, ListResult def get_accessible_title_ids( @@ -49,6 +52,31 @@ def get_title_uploaders(session: OrmSession, title_id: UUID) -> Sequence[Account ).all() +def get_available_title_uploaders( + session: OrmSession, title_id: UUID, *, skip: int, limit: int +) -> ListResult[AccountSchema]: + """Get title-uploader accounts not yet granted upload access to a title.""" + stmt = ( + select(Account) + .where( + Account.deleted.is_(False), + Account.role == RoleEnum.TITLE_UPLOADER, + ~exists().where( + TitlePermission.title_id == title_id, + TitlePermission.account_id == Account.id, + ), + ) + .order_by(Account.display_name, Account.id) + ) + return ListResult[AccountSchema]( + nb_records=count_from_stmt(session, stmt), + records=[ + db_account.create_account_schema(account) + for account in session.scalars(stmt.offset(skip).limit(limit)).all() + ], + ) + + def get_title_permission_or_none( session: OrmSession, title_id: UUID, account_id: UUID ) -> TitlePermission | None: diff --git a/backend/tests/api/routes/test_titles.py b/backend/tests/api/routes/test_titles.py index efe97011..ebee5c53 100644 --- a/backend/tests/api/routes/test_titles.py +++ b/backend/tests/api/routes/test_titles.py @@ -14,6 +14,7 @@ from cms_backend.api.context import Context as ApiContext from cms_backend.api.token import generate_access_token from cms_backend.context import Context +from cms_backend.db.collection_permission import create_collection_permission from cms_backend.db.models import ( Account, Book, @@ -1507,8 +1508,20 @@ def test_grant_title_uploader_rejects_other_roles( ["permission", "expected_status_code"], [ pytest.param(RoleEnum.ADMIN, HTTPStatus.NO_CONTENT, id="admin"), + pytest.param(RoleEnum.GLOBAL_EDITOR, HTTPStatus.NO_CONTENT, id="global-editor"), + pytest.param( + RoleEnum.TITLE_UPLOADER, HTTPStatus.UNAUTHORIZED, id="title-uploader" + ), + pytest.param( + RoleEnum.GLOBAL_VIEWER, HTTPStatus.UNAUTHORIZED, id="global-viewer" + ), pytest.param( - RoleEnum.GLOBAL_EDITOR, HTTPStatus.UNAUTHORIZED, id="global-editor" + RoleEnum.COLLECTION_VIEWER, + HTTPStatus.UNAUTHORIZED, + id="collection-viewer", + ), + pytest.param( + RoleEnum.PUBLIC_VIEWER, HTTPStatus.UNAUTHORIZED, id="public-viewer" ), ], ) @@ -1519,7 +1532,7 @@ def test_grant_title_uploader_required_permissions( permission: RoleEnum, expected_status_code: HTTPStatus, ): - """Granting upload access requires the account:update permission.""" + """Granting upload access requires the title:update permission.""" wikipedia_en_all = create_title(name="wikipedia_en_all") account1 = create_account(permission=RoleEnum.TITLE_UPLOADER) account2 = create_account(permission=permission) @@ -1533,3 +1546,113 @@ def test_grant_title_uploader_required_permissions( headers={"Authorization": f"Bearer {access_token}"}, ) assert response.status_code == expected_status_code + + +def test_collection_editor_can_grant_uploader_on_editable_title( + client: TestClient, + dbsession: OrmSession, + create_title: Callable[..., Title], + create_collection: Callable[..., Collection], + create_account: Callable[..., Account], +): + """A collection editor can grant upload access for a title in their collection.""" + wikipedia_en_all = create_title(name="wikipedia_en_all") + collection = create_collection( + title_ids_with_paths=[(wikipedia_en_all.id, "other")] + ) + editor = create_account(permission=RoleEnum.COLLECTION_EDITOR) + create_collection_permission(dbsession, collection.id, editor.id) + uploader = create_account(permission=RoleEnum.TITLE_UPLOADER) + access_token = generate_access_token(issue_time=getnow(), account_id=str(editor.id)) + headers = {"Authorization": f"Bearer {access_token}"} + + response = client.post( + f"/v1/titles/{wikipedia_en_all.name}/uploaders", + json={"account_id": str(uploader.id)}, + headers=headers, + ) + assert response.status_code == HTTPStatus.NO_CONTENT + + response = client.get( + f"/v1/titles/{wikipedia_en_all.name}/uploaders", headers=headers + ) + assert response.status_code == HTTPStatus.OK + assert [item["id"] for item in response.json()["items"]] == [str(uploader.id)] + + response = client.delete( + f"/v1/titles/{wikipedia_en_all.name}/uploaders/{uploader.id}", + headers=headers, + ) + assert response.status_code == HTTPStatus.NO_CONTENT + + +def test_collection_editor_cannot_grant_uploader_on_other_title( + client: TestClient, + dbsession: OrmSession, + create_title: Callable[..., Title], + create_collection: Callable[..., Collection], + create_account: Callable[..., Account], +): + """A collection editor cannot grant upload access for other collections' titles.""" + wikipedia_en_all = create_title(name="wikipedia_en_all") + create_collection(title_ids_with_paths=[(wikipedia_en_all.id, "other")]) + other_collection = create_collection() + editor = create_account(permission=RoleEnum.COLLECTION_EDITOR) + create_collection_permission(dbsession, other_collection.id, editor.id) + uploader = create_account(permission=RoleEnum.TITLE_UPLOADER) + access_token = generate_access_token(issue_time=getnow(), account_id=str(editor.id)) + + response = client.post( + f"/v1/titles/{wikipedia_en_all.name}/uploaders", + json={"account_id": str(uploader.id)}, + headers={"Authorization": f"Bearer {access_token}"}, + ) + assert response.status_code == HTTPStatus.NOT_FOUND + + +def test_get_available_title_uploaders( + client: TestClient, + dbsession: OrmSession, + create_title: Callable[..., Title], + create_collection: Callable[..., Collection], + create_account: Callable[..., Account], +): + """Editors can list title-uploader accounts without account:read permission.""" + wikipedia_en_all = create_title(name="wikipedia_en_all") + collection = create_collection( + title_ids_with_paths=[(wikipedia_en_all.id, "other")] + ) + editor = create_account(permission=RoleEnum.COLLECTION_EDITOR) + create_collection_permission(dbsession, collection.id, editor.id) + granted_uploader = create_account(permission=RoleEnum.TITLE_UPLOADER) + dbsession.add( + TitlePermission(title_id=wikipedia_en_all.id, account_id=granted_uploader.id) + ) + dbsession.flush() + + for _ in range(2): + create_account(permission=RoleEnum.TITLE_UPLOADER) + + access_token = generate_access_token(issue_time=getnow(), account_id=str(editor.id)) + headers = {"Authorization": f"Bearer {access_token}"} + + response = client.get( + f"/v1/titles/{wikipedia_en_all.name}/uploaders/available?skip=0&limit=1", + headers=headers, + ) + assert response.status_code == HTTPStatus.OK + first_page = response.json() + assert first_page["meta"]["count"] == 2 + assert first_page["meta"]["skip"] == 0 + assert first_page["meta"]["limit"] == 1 + assert len(first_page["items"]) == 1 + + response = client.get( + f"/v1/titles/{wikipedia_en_all.name}/uploaders/available?skip=1&limit=1", + headers=headers, + ) + assert response.status_code == HTTPStatus.OK + second_page = response.json() + assert second_page["meta"]["count"] == 2 + assert second_page["meta"]["skip"] == 1 + assert len(second_page["items"]) == 1 diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py index 4963c4c1..3c7854d8 100644 --- a/backend/tests/conftest.py +++ b/backend/tests/conftest.py @@ -500,11 +500,12 @@ def _create_account( permission: RoleEnum = RoleEnum.ADMIN, password: str | None = None, ): - username = username or faker.first_name() + display_name = username or faker.first_name() + username = username or f"{display_name}_{uuid4().hex[:8]}" account = Account( role=permission, username=username, - display_name=username, + display_name=display_name, idp_sub=uuid4(), password_hash=( None if password is None else generate_password_hash(password) diff --git a/frontend/src/components/TitleUploadersDialog.vue b/frontend/src/components/TitleUploadersDialog.vue index e17c18ad..f5a60419 100644 --- a/frontend/src/components/TitleUploadersDialog.vue +++ b/frontend/src/components/TitleUploadersDialog.vue @@ -79,7 +79,6 @@ import { computed, ref, watch } from 'vue' import ConfirmDialog from '@/components/ConfirmDialog.vue' import { useNotificationStore } from '@/stores/notification' import { useTitleStore } from '@/stores/title' -import { useUserStore } from '@/stores/user' import type { User } from '@/types/user' interface Props { @@ -94,7 +93,6 @@ const emit = defineEmits<{ }>() const titleStore = useTitleStore() -const userStore = useUserStore() const notificationStore = useNotificationStore() const errors = ref([]) @@ -121,7 +119,7 @@ const loadData = async () => { loading.value = true errors.value = [] const [accounts, currentUploaders] = await Promise.all([ - userStore.fetchUsers(0, 200, undefined, false, false, 'title-uploader'), + titleStore.fetchAvailableTitleUploaders(props.titleId), titleStore.fetchTitleUploaders(props.titleId), ]) titleUploaderAccounts.value = accounts ?? [] diff --git a/frontend/src/stores/title.ts b/frontend/src/stores/title.ts index da301dfe..2e8c460b 100644 --- a/frontend/src/stores/title.ts +++ b/frontend/src/stores/title.ts @@ -281,6 +281,26 @@ export const useTitleStore = defineStore('title', () => { } } + const fetchAvailableTitleUploaders = async ( + titleId: string, + skip: number = 0, + limit: number = 200, + ): Promise => { + const service = await authStore.getApiService('titles') + try { + const response = await service.get>( + `/${titleId}/uploaders/available`, + { params: { skip, limit } }, + ) + errors.value = [] + return response.items + } catch (_error) { + console.error('Failed to fetch available title uploaders', _error) + errors.value = translateErrors(_error as ErrorResponse) + return [] + } + } + const fetchTitleUploaders = async (titleId: string): Promise => { const service = await authStore.getApiService('titles') try { @@ -345,6 +365,7 @@ export const useTitleStore = defineStore('title', () => { completeZimUploadByFile, completeZimUploadByUrl, fetchTitleUploaders, + fetchAvailableTitleUploaders, grantTitleUploader, revokeTitleUploader, } diff --git a/frontend/src/views/TitleView.vue b/frontend/src/views/TitleView.vue index 0f50b7a9..7693a261 100644 --- a/frontend/src/views/TitleView.vue +++ b/frontend/src/views/TitleView.vue @@ -724,7 +724,7 @@ const canArchiveTitle = computed(() => authStore.hasPermission('title', 'archive const canUploadZim = computed(() => authStore.hasPermission('book', 'create')) -const canManageUploaders = computed(() => authStore.hasPermission('account', 'update')) +const canManageUploaders = computed(() => authStore.hasPermission('title', 'update')) const showUploadersDialog = ref(false) const titleUploadsViewRef = ref>()