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

files: fix files permissions #754

Merged
merged 1 commit into from
Feb 2, 2022
Merged

Conversation

jma
Copy link
Contributor

@jma jma commented Jan 19, 2022

Signed-off-by: Johnny Mariéthoz [email protected]

Why are you opening this PR?

How to test?

  • As the permission has been touched it is important to test to upload files for document, deposit, organisation and collection with several roles.
  • Try to read a file url for a deposit for an anonymous user.
  • Try to read restricted files, embargo files and masked document.

Code review check list

  • Commit message template compliance.
  • Commit message without typos.
  • File names.
  • Functions names.
  • Functions docstrings.
  • Unnecessary commited files?
  • Extracted translations?

@jma jma force-pushed the maj-fix-files-permissions branch 6 times, most recently from 7c94764 to 85e3eb3 Compare January 25, 2022 15:54
@jma jma changed the title files: fix files permissions (In progress) files: fix files permissions Jan 25, 2022
@jma jma marked this pull request as ready for review January 25, 2022 15:55
@jma jma force-pushed the maj-fix-files-permissions branch 2 times, most recently from 54b1ee9 to c56134b Compare January 26, 2022 06:55
@jma jma requested review from BadrAly, Garfield-fr, mmo, rerowep, vgranata and zannkukai and removed request for zannkukai and BadrAly January 26, 2022 06:56
@jma jma force-pushed the maj-fix-files-permissions branch from c56134b to 0ce3750 Compare January 26, 2022 07:12
"""Update permission check.

:param user: Current user record.
:param recor: Record to check.
Copy link
Contributor

Choose a reason for hiding this comment

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

:param record: Record to check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will fix it with a find and replace.

"""Delete permission check.

:param user: Current user record.
:param recor: Record to check.
Copy link
Contributor

Choose a reason for hiding this comment

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

:param record: Record to check.

Comment on lines 115 to 116
:param for_permission: True if it is used to compute permissions.
:param file: File dict.
:param organisations: List of organisations.
Copy link
Contributor

Choose a reason for hiding this comment

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

:param file: File dict.
:param organisations: List of organisations.
:param for_permission: True if it is used to compute permissions.

Mainly the same behavior than the corresponding organisation record.

:param user: Current user record.
:param recor: Record to check.
Copy link
Contributor

Choose a reason for hiding this comment

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

:param record: Record to check.

"""Delete permission check.

:param user: Current user record.
:param recor: Record to check.
Copy link
Contributor

Choose a reason for hiding this comment

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

:param record: Record to check.

Comment on lines 159 to 163
if deposit:
can = DepositPermission.read(user, deposit)
if can:
return True
return False

Choose a reason for hiding this comment

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

same here

Comment on lines 177 to 180
if deposit:
can = DepositPermission.update(user, deposit)
if can:
return True

Choose a reason for hiding this comment

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

same here

sonar/modules/collections/permissions.py Outdated Show resolved Hide resolved
Comment on lines 195 to 199
if deposit:
can = DepositPermission.delete(user, deposit)
if can:
return True
return False

Choose a reason for hiding this comment

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

same here

sonar/modules/documents/api.py Show resolved Hide resolved
Comment on lines 157 to 159
if document:
can_read_document = DocumentPermission.read(user, document)
if not can_read_document:

Choose a reason for hiding this comment

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

could be merged

sonar/modules/documents/api.py Show resolved Hide resolved
Comment on lines 138 to 141
if organisation:
can = OrganisationPermission.update(user, organisation)
if can:
return True

Choose a reason for hiding this comment

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

same again

Comment on lines 156 to 169
(pid, rec) = request.view_args.get('pid_value').data
if pid.pid_type == 'doc':
from .documents.permissions import DocumentFilesPermission
return DocumentFilesPermission.create_permission(obj, action)
if pid.pid_type == 'coll':
from .collections.permissions import FilesPermission as CollectionFilesPermission
return CollectionFilesPermission.create_permission(obj, action)
if pid.pid_type == 'org':
from .organisations.permissions import OrganisationFilesPermission
return OrganisationFilesPermission.create_permission(obj, action)
if pid.pid_type == 'depo':
from .deposits.permissions import DepositFilesPermission
return DepositFilesPermission.create_permission(obj, action)
return FilesPermission.create_permission(obj, action)

Choose a reason for hiding this comment

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

Maybe a good idea to create an utils function returning the FilesPermissions subclass based on pid_type.
The code should be simplifer

permission_class = utils_get_permissions_class(pid_type)  // should return FilePermission as default value
return permission_class.create_permission(obj, action)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have create a config instead.

@jma jma force-pushed the maj-fix-files-permissions branch from 0ce3750 to 68ff0fd Compare January 26, 2022 14:02
@jma jma requested a review from zannkukai January 26, 2022 14:06
if request.view_args.get('pid_value'):
(pid, rec) = request.view_args.get('pid_value').data
pid_type = pid.pid_type
print('===============>',

Choose a reason for hiding this comment

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

remove this beautiful print ;-)

@jma jma force-pushed the maj-fix-files-permissions branch 2 times, most recently from 339a8f2 to a0e5c5c Compare January 27, 2022 08:54
@pronguen
Copy link
Contributor

pronguen commented Jan 28, 2022

  • I have an internal server error when trying to download the file of any document from the thumbnail (ex: https://sonardev.test.rero.ch/global/documents/2162). The thumbnail is not displayed. The download from the file management in the professional interface is working.
    image

@jma jma force-pushed the maj-fix-files-permissions branch 5 times, most recently from 7647014 to bbae1bd Compare January 31, 2022 13:42
* Adds files restriction for documents.
* Adds files restriction for deposits.
* Adds files restriction for organisations.
* Adds files restriction for collections.
* Fixes document restrictions.
* Closes rero#746.

Co-Authored-by: Johnny Mariéthoz <[email protected]>
@jma jma force-pushed the maj-fix-files-permissions branch from bbae1bd to 0d0046c Compare February 1, 2022 10:50
@jma jma merged commit 01282bf into rero:staging Feb 2, 2022
@jma jma deleted the maj-fix-files-permissions branch February 7, 2022 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Files in masked records: access rights should be limited to those of its record
5 participants