From 5af5eeb1eff92e8ffd53d9028aa5b05f2e0c59b8 Mon Sep 17 00:00:00 2001 From: Juan Nicolas Herrera <54152074+juanNH@users.noreply.github.com> Date: Wed, 26 Aug 2026 23:31:12 -0300 Subject: [PATCH 1/4] Remove deprecated user file permission flag --- src/common/access_control.py | 16 +-- .../components/files-manager/FilesManager.tsx | 70 ++++----- .../frontend/src/utils/django_interfaces.ts | 2 +- src/user_files/serializers.py | 12 +- .../tests/test_owner_permissions.py | 134 ++++++++++++++++++ src/user_files/views.py | 25 ++-- 6 files changed, 185 insertions(+), 74 deletions(-) create mode 100644 src/user_files/tests/test_owner_permissions.py diff --git a/src/common/access_control.py b/src/common/access_control.py index a486b559..8b5ae075 100644 --- a/src/common/access_control.py +++ b/src/common/access_control.py @@ -92,14 +92,10 @@ def user_file_visibility_q(user: AbstractBaseUser) -> Q: def can_edit_user_file(user_file: Any, user: AbstractBaseUser) -> bool: - """Return whether the user may edit/delete a UserFile.""" - if not user or not user.is_authenticated: - return False + """Return whether the user may mutate a UserFile. - return ( - user_file.user_id == user.id - or user_file.institutions.filter( - users=user, - institutionadministration__is_institution_admin=True, - ).exists() - ) + Institution membership controls visibility, but it does not grant + ownership or mutation permissions. Dataset edits, deletion, visibility + changes, and sharing changes are therefore restricted to the owner. + """ + return bool(user and user.is_authenticated and user_file.user_id == user.id) diff --git a/src/frontend/static/frontend/src/components/files-manager/FilesManager.tsx b/src/frontend/static/frontend/src/components/files-manager/FilesManager.tsx index 9b3827dc..858867f1 100644 --- a/src/frontend/static/frontend/src/components/files-manager/FilesManager.tsx +++ b/src/frontend/static/frontend/src/components/files-manager/FilesManager.tsx @@ -15,7 +15,7 @@ import { PopupIcons } from '../common/PopupIcons' import { SwitchPublicButton } from '../common/SwitchPublicButton' import { useIntl, IntlShape } from 'react-intl' import { DeleteButton } from '../common/DeleteButton' -import { getTissueDropdownOptions, getTissueIds, TissueLabels } from '../common/TissueLabels' +import { getTissueDropdownOptions, getTissueIds } from '../common/TissueLabels' import { SharedInstitutionsUserFile } from './SharedInstitutionsUserFile' /** Structure returned from the chunk upload service. */ @@ -31,10 +31,9 @@ declare const urlUserFilesCRUD: string declare const urlUserInstitutions: string declare const urlTissuesCRUD: string declare const urlChunkUpload: string -declare const urlChunkUploadComplete: string -declare const downloadFileURL: string -declare const downloadFileHeaders: string -declare const currentUserId: string +declare const urlChunkUploadComplete: string +declare const downloadFileURL: string +declare const downloadFileHeaders: string /** * New File Form fields @@ -843,35 +842,27 @@ class FilesManager extends React.Component {getFileTypeName(userFileRow.file_type)} - - {userFileRow.institutions.length > 0 && ( - i.name).join(', ') } - )} - /> - )} - - - } - tagType={TagType.FILE} - onTagSelect={(tagId) => this.updateUserFileTag(userFileRow, tagId)} - onTagCreated={() => this.getUserTags()} - onTagEdited={() => this.getUserTags()} - onTagDeleted={(deletedTagId) => { - if (this.state.newFile.newTag === deletedTagId) { - this.handleAddFileInputsChange('newTag', null) - } - - this.getUserTags() - }} - /> - + + {userFileRow.is_owner + ? ( + } + tagType={TagType.FILE} + onTagSelect={(tagId) => this.updateUserFileTag(userFileRow, tagId)} + onTagCreated={() => this.getUserTags()} + onTagEdited={() => this.getUserTags()} + onTagDeleted={(deletedTagId) => { + if (this.state.newFile.newTag === deletedTagId) { + this.handleAddFileInputsChange('newTag', null) + } + + this.getUserTags() + }} + /> + ) + : } + {(this.state.sharedInstitutionsByFile[userFileRow.id as number] ?? userFileRow.institutions).length > 0 ? ( @@ -886,9 +877,7 @@ class FilesManager extends React.Component ) : '-'} - - - + { userFileRow.is_public ? ( @@ -918,7 +907,7 @@ class FilesManager extends React.Component { columnName: userFileRow.column_used_as_index } )} /> - {Number(currentUserId) === userFileRow.user.id && ( + {userFileRow.is_owner && ( onClick={() => this.openShareInstitutions(userFileRow)} /> )} - {/* Users can modify or delete own files or the ones which belongs to an - Institution which the user is admin of */} - {userFileRow.is_private_or_institution_admin && ( + {/* Only the dataset owner can modify or delete it. */} + {userFileRow.is_owner && ( <> {/* Shows a edit button if specified */} UserFile: return get_object_or_404(queryset, pk=user_file_pk) -def get_own_or_as_admin_user_files(user: AbstractBaseUser): +def get_own_user_files(user: AbstractBaseUser): """ - Returns only the User's Files which were uploaded by himself or belongs to an Institution which his is the admin of + Returns only the User's Files which were uploaded by himself. @param user: User to retrieve his Datasets @return: User's Files """ - return UserFile.objects.filter(Q(user=user) | ( - Q(institutions__institutionadministration__user=user) - & Q(institutions__institutionadministration__is_institution_admin=True) - )).distinct() + return UserFile.objects.filter(user=user) def get_user_files(user: AbstractBaseUser, public_only: bool, private_only: bool, with_survival_only: bool) -> QuerySet: @@ -157,14 +154,14 @@ def get_queryset(self): class UserFileDetail(generics.RetrieveUpdateDestroyAPIView): """ - REST endpoint: get, modify or delete for UserFile model. A User can modify or delete ONLY the files which were - uploaded by himself or belongs to an Institution which his is the admin of + REST endpoint: get, modify or delete for UserFile model. Only the owner + can modify or delete a UserFile; institutional visibility is read-only. """ def get_queryset(self): if self.request.method == 'GET': return UserFile.objects.filter(user_file_visibility_q(self.request.user)).distinct() - return get_own_or_as_admin_user_files(self.request.user) + return get_own_user_files(self.request.user) serializer_class = UserFileSerializer permission_classes = [permissions.IsAuthenticated] @@ -180,7 +177,7 @@ def get_queryset(self): """Return the current user's institutions not already shared with the dataset.""" user_file_id = self.kwargs.get('user_file_id') user_file = get_object_or_404(UserFile, pk=user_file_id) - if not can_edit_shared_resource(user_file, self.request.user): + if not can_edit_user_file(user_file, self.request.user): raise PermissionDenied('You do not have permission to modify this dataset.') shared_institution_ids = user_file.institutions.values_list('pk', flat=True) @@ -220,7 +217,7 @@ def post(request): ) user_file = get_object_or_404(UserFile, pk=user_file_id) - if not can_edit_shared_resource(user_file, request.user): + if not can_edit_user_file(user_file, request.user): raise PermissionDenied('You do not have permission to modify this dataset.') institution = get_object_or_404( @@ -244,7 +241,7 @@ def post(request): user_file_id = request.data.get('userFileId') user_file = get_object_or_404(UserFile, pk=user_file_id) - if not can_edit_shared_resource(user_file, request.user): + if not can_edit_user_file(user_file, request.user): raise PermissionDenied('You do not have permission to modify this dataset.') institution = get_object_or_404(Institution, pk=institution_id) @@ -293,7 +290,7 @@ def post(request): user_file_id = data.get('userFileId') user_file: UserFile = get_object_or_404(UserFile, id=user_file_id) - if user_file.user.id != request.user.id: + if not can_edit_user_file(user_file, request.user): raise PermissionDenied('You do not have permission to modify this user file.') user_file.is_public = not user_file.is_public From be6c6377c3a1a298508f673b892c090bc15e5ed0 Mon Sep 17 00:00:00 2001 From: Juan Nicolas Herrera <54152074+juanNH@users.noreply.github.com> Date: Wed, 26 Aug 2026 23:46:31 -0300 Subject: [PATCH 2/4] Improve shared institutions modal UX --- .../SharedInstitutions.tsx | 154 +++++++++++------- .../frontend/src/css/shared-institutions.css | 22 +++ .../static/frontend/src/locales/en.ts | 1 + .../static/frontend/src/locales/es.ts | 1 + 4 files changed, 118 insertions(+), 60 deletions(-) create mode 100644 src/frontend/static/frontend/src/css/shared-institutions.css diff --git a/src/frontend/static/frontend/src/components/pipeline/all-experiments-view/SharedInstitutions.tsx b/src/frontend/static/frontend/src/components/pipeline/all-experiments-view/SharedInstitutions.tsx index 71d6c110..7da0e825 100644 --- a/src/frontend/static/frontend/src/components/pipeline/all-experiments-view/SharedInstitutions.tsx +++ b/src/frontend/static/frontend/src/components/pipeline/all-experiments-view/SharedInstitutions.tsx @@ -1,13 +1,13 @@ import React, { useContext, useEffect, useRef, useState } from 'react' -import { Button, Divider, Grid, GridColumn, Icon, List, ListContent, ListHeader, ListItem, Modal, ModalContent, ModalHeader, Segment, Select, Table } from 'semantic-ui-react' +import { Button, Icon, List, ListContent, ListHeader, ListItem, Modal, ModalContent, ModalHeader, Select, Tab, Table } from 'semantic-ui-react' import { PaginatedTable } from '../../common/PaginatedTable' import { DjangoInstitutionUserLimited } from '../../../utils/django_interfaces' -import { TableCellWithTitle } from '../../common/TableCellWithTitle' import { getDjangoHeader } from '../../../utils/util_functions' import ky from 'ky' import { SemanticListItem } from '../../../utils/interfaces' import { CurrentUserContext } from '../../Base' import { useIntl } from 'react-intl' +import '../../../css/shared-institutions.css' declare const urlGetUsersCandidatesLimited: string declare const urlGetInstitutionsNonInExperiment: string @@ -37,6 +37,7 @@ const InstitutionUserList = (props: InstitutionUserListProps) => { return (
+ key={props.institutionId} headerTitle={`${props.institutionName} ${intl.formatMessage({ id: 'sharedInstitutions.users' })}`} headers={[ { name: intl.formatMessage({ id: 'sharedInstitutions.userName' }), serverCodeToSort: 'user__username' as any, width: 3 } @@ -49,7 +50,10 @@ const InstitutionUserList = (props: InstitutionUserListProps) => { mapFunction={(userCandidate: DjangoInstitutionUserLimited) => { return ( - + + + {userCandidate.user.username} + ) }} @@ -61,10 +65,11 @@ const InstitutionUserList = (props: InstitutionUserListProps) => { export const SharedInstitutions = (props: Props) => { const [activeInstitution, setActiveInstitution] = useState<{ id: number, name: string }>({ id: 0, name: '' }) const [listOfInstitutionNonPart, setListOfInstitutionNonPart] = useState([]) - const [institutionIdToAdd, setInstitutionIdToAdd] = useState(0) + const [institutionIdToAdd, setInstitutionIdToAdd] = useState(null) const abortController = useRef(new AbortController()) const [institutionList, setInstitutionList] = useState<{ id: number, name: string }[]>([]) const [isLoadingInstitution, setIsLoadingInstitution] = useState(false) + const [activeTabIndex, setActiveTabIndex] = useState(0) const currentUser = useContext(CurrentUserContext) const intl = useIntl() @@ -118,11 +123,12 @@ export const SharedInstitutions = (props: Props) => { setIsLoadingInstitution(true) const myHeaders = getDjangoHeader() const body = { - institutionId: institutionIdToAdd, + institutionId: Number(institutionIdToAdd), experimentId: props.experimentId } ky.post(urlShareExperimentToInstitution, { headers: myHeaders, signal: abortController.current.signal, json: body }).then((response) => { response.json().then(() => { + setInstitutionIdToAdd(null) InstitutionsistNonInExperiment() usersListInstitution() }).catch((err) => { @@ -163,30 +169,101 @@ export const SharedInstitutions = (props: Props) => { } useEffect(() => { - if (props.experimentId) { + if (props.isOpen && props.experimentId) { + setActiveTabIndex(0) + setInstitutionIdToAdd(null) InstitutionsistNonInExperiment() usersListInstitution() } - }, [props.experimentId]) + }, [props.experimentId, props.isOpen]) + + const canManage = props.user.id === currentUser?.id + const panes = [ + { + menuItem: intl.formatMessage({ id: 'sharedInstitutions.institutions' }), + render: () => ( + + +
+ {institutionList.map(institution => ( + + + { + setActiveInstitution(institution) + setActiveTabIndex(1) + }} + > + {institution.name} + + {canManage && ( + props.handleChangeConfirmModalState( + true, + intl.formatMessage({ id: 'sharedInstitutions.stopSharing' }), + intl.formatMessage({ id: 'sharedInstitutions.confirmStopSharing' }), + () => handleRemoveInstitution(institution.id) + )} + /> + )} + + ))} +
+ + + ) + }, + { + menuItem: intl.formatMessage({ id: 'sharedInstitutions.users' }), + render: () => ( + + {activeInstitution.id + ? ( + + ) + : null} + + ) + } + ] return ( props.handleClose()} open={props.isOpen} closeIcon={ props.handleClose()} />} - style={{ width: '60%', maxWidth: '1000px' }} + style={{ width: '50%', maxWidth: '800px' }} > {intl.formatMessage({ id: 'sharedInstitutions.title' })} { - props.user.id === currentUser?.id && + canManage && ( <> setInstitutionIdToAdd(Number(value))} - /> - - - ) - } + const canManage = props.user.id === currentUser?.id + const selectedInstitution: SharedInstitution | null = activeInstitution.id ? activeInstitution : null - - - - -
- {institutionList.map(institution => ( - -
-
- setActiveInstitution(institution)}> - - {institution.name} - - -
- {currentUser?.id === props.user.id && - ( - props.handleChangeConfirmModalState(true, 'Stop sharing experiment', 'Are you sure to stop sharing experiment to this institution?', () => handleRemoveInstitution(institution.id))} - /> - )} -
- - ))} -
- - - - - { - activeInstitution.id - ? ( - - ) - : null - } - - - - - + return ( + intl.formatMessage( + { id: 'sharedInstitutionsBiomarker.users.title' }, + { institutionName } + )} + userNameLabel={intl.formatMessage({ id: 'sharedInstitutionsBiomarker.users.column' })} + searchUserNamePlaceholder={intl.formatMessage({ id: 'sharedInstitutionsBiomarker.users.searchPlaceholder' })} + removeInstitutionTitle={intl.formatMessage({ id: 'sharedInstitutionsBiomarker.removeInstitution' })} + isLoading={isLoadingInstitution} + canManage={canManage} + onClose={props.handleClose} + onInstitutionToAddChange={setInstitutionIdToAdd} + onAddInstitution={() => props.handleChangeConfirmModalState( + true, + intl.formatMessage({ id: 'sharedInstitutionsBiomarker.confirm.share.header' }), + intl.formatMessage({ id: 'sharedInstitutionsBiomarker.confirm.share.content' }), + handleAddInstitution + )} + onSelectInstitution={setActiveInstitution} + onRemoveInstitution={(institutionId) => props.handleChangeConfirmModalState( + true, + intl.formatMessage({ id: 'sharedInstitutionsBiomarker.confirm.stopShare.header' }), + intl.formatMessage({ id: 'sharedInstitutionsBiomarker.confirm.stopShare.content' }), + () => handleRemoveInstitution(institutionId) + )} + /> ) } diff --git a/src/frontend/static/frontend/src/components/common/SharedInstitutionsModal.tsx b/src/frontend/static/frontend/src/components/common/SharedInstitutionsModal.tsx new file mode 100644 index 00000000..b28bbac1 --- /dev/null +++ b/src/frontend/static/frontend/src/components/common/SharedInstitutionsModal.tsx @@ -0,0 +1,165 @@ +import React, { ReactNode } from 'react' +import { Button, Divider, Grid, GridColumn, Icon, List, ListContent, ListHeader, ListItem, Modal, ModalContent, ModalHeader, Segment, Select, Table } from 'semantic-ui-react' +import { DjangoInstitutionUserLimited } from '../../utils/django_interfaces' +import { SemanticListItem } from '../../utils/interfaces' +import { PaginatedTable } from './PaginatedTable' +import '../../css/shared-institutions.css' + +export interface SharedInstitution { + id: number, + name: string +} + +interface SharedInstitutionsModalProps { + isOpen: boolean, + title: ReactNode, + selectPlaceholder: string, + addButtonText: string, + availableInstitutions: SemanticListItem[], + selectedInstitutionToAdd: string | null, + sharedInstitutions: SharedInstitution[], + selectedInstitution: SharedInstitution | null, + usersUrl: string, + usersHeaderTitle: (institutionName: string) => string, + userNameLabel: string, + searchUserNamePlaceholder: string, + removeInstitutionTitle: string, + isLoading: boolean, + canManage: boolean, + onClose: VoidFunction, + onInstitutionToAddChange: (value: string | null) => void, + onAddInstitution: VoidFunction, + onSelectInstitution: (institution: SharedInstitution) => void, + onRemoveInstitution: (institutionId: number) => void, +} + +interface InstitutionUserListProps { + institutionName: string, + institutionId: number, + usersUrl: string, + usersHeaderTitle: (institutionName: string) => string, + userNameLabel: string, + searchUserNamePlaceholder: string, +} + +const InstitutionUserList = (props: InstitutionUserListProps) => { + return ( +
+ + key={props.institutionId} + headerTitle={props.usersHeaderTitle(props.institutionName)} + headers={[ + { name: props.userNameLabel, serverCodeToSort: 'user__username' as any, width: 3 } + ]} + showSearchInput + searchLabel={props.userNameLabel} + searchPlaceholder={props.searchUserNamePlaceholder} + urlToRetrieveData={`${props.usersUrl}/${props.institutionId}/`} + updateWSKey='update_user_for_institution' + mapFunction={(userCandidate: DjangoInstitutionUserLimited) => { + return ( + + + + {userCandidate.user.username} + + + ) + }} + /> +
+ ) +} + +export const SharedInstitutionsModal = (props: SharedInstitutionsModalProps) => { + return ( + } + style={{ width: '70%', maxWidth: '1100px' }} + > + {props.title} + + {props.canManage && ( +
+ setInstitutionIdToAdd(Number(value))} - /> - -
- )} - - - {sharedInstitutions.map(institution => ( - -
- - {institution.name} - - {canManage && ( - props.handleChangeConfirmModalState( - true, - intl.formatMessage({ id: 'files.manager.sharedInstitutions.removeTitle' }), - intl.formatMessage({ id: 'files.manager.sharedInstitutions.removeConfirm' }), - () => handleRemoveInstitution(institution.id) - )} - /> - )} -
- - ))} - - - + onInstitutionToAddChange={setInstitutionIdToAdd} + onAddInstitution={() => props.handleChangeConfirmModalState( + true, + intl.formatMessage({ id: 'files.manager.sharedInstitutions.addTitle' }), + intl.formatMessage({ id: 'files.manager.sharedInstitutions.addConfirm' }), + handleAddInstitution + )} + onSelectInstitution={setActiveInstitution} + onRemoveInstitution={(institutionId) => props.handleChangeConfirmModalState( + true, + intl.formatMessage({ id: 'files.manager.sharedInstitutions.removeTitle' }), + intl.formatMessage({ id: 'files.manager.sharedInstitutions.removeConfirm' }), + () => handleRemoveInstitution(institutionId) + )} + /> ) } diff --git a/src/frontend/static/frontend/src/components/pipeline/all-experiments-view/SharedInstitutions.tsx b/src/frontend/static/frontend/src/components/pipeline/all-experiments-view/SharedInstitutions.tsx index 7da0e825..bfb0bf70 100644 --- a/src/frontend/static/frontend/src/components/pipeline/all-experiments-view/SharedInstitutions.tsx +++ b/src/frontend/static/frontend/src/components/pipeline/all-experiments-view/SharedInstitutions.tsx @@ -1,13 +1,10 @@ import React, { useContext, useEffect, useRef, useState } from 'react' -import { Button, Icon, List, ListContent, ListHeader, ListItem, Modal, ModalContent, ModalHeader, Select, Tab, Table } from 'semantic-ui-react' -import { PaginatedTable } from '../../common/PaginatedTable' -import { DjangoInstitutionUserLimited } from '../../../utils/django_interfaces' import { getDjangoHeader } from '../../../utils/util_functions' import ky from 'ky' import { SemanticListItem } from '../../../utils/interfaces' import { CurrentUserContext } from '../../Base' import { useIntl } from 'react-intl' -import '../../../css/shared-institutions.css' +import { SharedInstitution, SharedInstitutionsModal } from '../../common/SharedInstitutionsModal' declare const urlGetUsersCandidatesLimited: string declare const urlGetInstitutionsNonInExperiment: string @@ -27,41 +24,6 @@ interface Props extends SharedInstitutionsProps { handleChangeConfirmModalState: (setOption: boolean, headerText: string, contentText: string, onConfirm: () => void) => void, } -interface InstitutionUserListProps { - institutionName: string, - institutionId: number, -} - -const InstitutionUserList = (props: InstitutionUserListProps) => { - const intl = useIntl() - return ( -
- - key={props.institutionId} - headerTitle={`${props.institutionName} ${intl.formatMessage({ id: 'sharedInstitutions.users' })}`} - headers={[ - { name: intl.formatMessage({ id: 'sharedInstitutions.userName' }), serverCodeToSort: 'user__username' as any, width: 3 } - ]} - showSearchInput - searchLabel={intl.formatMessage({ id: 'sharedInstitutions.userName' })} - searchPlaceholder={intl.formatMessage({ id: 'sharedInstitutions.searchUserName' })} - urlToRetrieveData={urlGetUsersCandidatesLimited + '/' + props.institutionId + '/'} - updateWSKey='update_user_for_institution' - mapFunction={(userCandidate: DjangoInstitutionUserLimited) => { - return ( - - - - {userCandidate.user.username} - - - ) - }} - /> -
- ) -} - export const SharedInstitutions = (props: Props) => { const [activeInstitution, setActiveInstitution] = useState<{ id: number, name: string }>({ id: 0, name: '' }) const [listOfInstitutionNonPart, setListOfInstitutionNonPart] = useState([]) @@ -69,7 +31,6 @@ export const SharedInstitutions = (props: Props) => { const abortController = useRef(new AbortController()) const [institutionList, setInstitutionList] = useState<{ id: number, name: string }[]>([]) const [isLoadingInstitution, setIsLoadingInstitution] = useState(false) - const [activeTabIndex, setActiveTabIndex] = useState(0) const currentUser = useContext(CurrentUserContext) const intl = useIntl() @@ -170,7 +131,6 @@ export const SharedInstitutions = (props: Props) => { useEffect(() => { if (props.isOpen && props.experimentId) { - setActiveTabIndex(0) setInstitutionIdToAdd(null) InstitutionsistNonInExperiment() usersListInstitution() @@ -178,111 +138,40 @@ export const SharedInstitutions = (props: Props) => { }, [props.experimentId, props.isOpen]) const canManage = props.user.id === currentUser?.id - const panes = [ - { - menuItem: intl.formatMessage({ id: 'sharedInstitutions.institutions' }), - render: () => ( - - -
- {institutionList.map(institution => ( - - - { - setActiveInstitution(institution) - setActiveTabIndex(1) - }} - > - {institution.name} - - {canManage && ( - props.handleChangeConfirmModalState( - true, - intl.formatMessage({ id: 'sharedInstitutions.stopSharing' }), - intl.formatMessage({ id: 'sharedInstitutions.confirmStopSharing' }), - () => handleRemoveInstitution(institution.id) - )} - /> - )} - - ))} -
- - - ) - }, - { - menuItem: intl.formatMessage({ id: 'sharedInstitutions.users' }), - render: () => ( - - {activeInstitution.id - ? ( - - ) - : null} - - ) - } - ] + const selectedInstitution: SharedInstitution | null = activeInstitution.id ? activeInstitution : null return ( - props.handleClose()} - open={props.isOpen} - closeIcon={ props.handleClose()} />} - style={{ width: '50%', maxWidth: '800px' }} - > - {intl.formatMessage({ id: 'sharedInstitutions.title' })} - - { - canManage && - ( - <> -