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

Copy file endpoint #1382

Merged
merged 4 commits into from
Mar 7, 2024
Merged

Copy file endpoint #1382

merged 4 commits into from
Mar 7, 2024

Conversation

natacha-beck
Copy link
Contributor

No description provided.

@natacha-beck natacha-beck added Enhancement API API issues or Swagger description labels Mar 4, 2024
@natacha-beck natacha-beck self-assigned this Mar 4, 2024
@@ -610,6 +610,29 @@ def rr_access_dp

end

# API method to copy files from one DP to another via a bourreau
def file_copy #:nodoc:
Copy link
Member

Choose a reason for hiding this comment

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

Rejected for security. This action has no validation or verifications. It will copy any files to anywhere. Which means that if we deployed this, a normal user could copy any file, including those that belongs to the admins or other users, to their own data provider.

@prioux
Copy link
Member

prioux commented Mar 4, 2024

Rejected. The role of a controller action is to make sure all parameters are valid for the current session!

@prioux prioux closed this Mar 4, 2024
@prioux prioux reopened this Mar 4, 2024
@prioux
Copy link
Member

prioux commented Mar 4, 2024

Check legitimate access for

  1. the DP
  2. the Bourreau
  3. the userfiles (for these, there is a loop, replace this with a mass accessible check instead of going to the database for each and every file ID)

@prioux
Copy link
Member

prioux commented Mar 4, 2024

For point 3 of my previous comment, there is an excellent example already in the controllers, e.g. in userfiles_controller for the action manage_compression:

    userfiles = Userfile
      .find_all_accessible_by_user(current_user, :access_requested => :write)
      .where(:id => file_ids)

except you'll have to change the access_requested to :read

# Filter out userfile_ids that are not readable by the user
userfile_ids = Userfile.find_all_accessible_by_user(current_user, :access_requested => :read)
.where(:id => userfile_ids).pluck(:id)

bourreau.send_command_copy_files(userfile_ids, data_provider_id, current_user.id)
render :json => { :status => "ok" }
Copy link
Member

Choose a reason for hiding this comment

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

While you are at it, when not return some useful information in the JSON reply, like the number of files that were actually selected for the move? (userfile_ids.size) ?

@prioux
Copy link
Member

prioux commented Mar 6, 2024

Looks good to me. Before I merge, I want to discuss if this action is really a 'bourreau' action, or if it would be more appropriate in a different controller. We are selecting files to be copied to a data provider, so maybe it's userfile action, or a data provider action?

@prioux prioux merged commit 7c68e84 into aces:master Mar 7, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API API issues or Swagger description Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants