aces / cbrain

CBRAIN is a flexible Ruby on Rails framework for accessing and processing of large data on high-performance computing infrastructures.
GNU General Public License v3.0
70 stars 43 forks source link

Never try to ensure a CbrainFileList is read-write when launching a task #1057

Open prioux opened 3 years ago

prioux commented 3 years ago

As a followup to #1054 , we can make a further improvement in the task launch interface: if a selected file is a CbrainFileList, the framework doesn't have to check about read-write permissions not matter how the task has been integrated and no matter who the file belongs to.

MontrealSergiy commented 1 month ago

then some bad tools can overwrite those files?

MontrealSergiy commented 1 month ago

I would keep read access just in case. If user even does not have read access what he is doing with the task in the first case. Some user may consider their lists private info.

prioux commented 1 month ago

This is the patch for the issue. Please test it.

diff --git a/BrainPortal/app/controllers/tasks_controller.rb b/BrainPortal/app/controllers/tasks_controller.rb
index 04d0319d0..2f623f228 100644
--- a/BrainPortal/app/controllers/tasks_controller.rb
+++ b/BrainPortal/app/controllers/tasks_controller.rb
@@ -234,12 +234,16 @@ class TasksController < ApplicationController

     # Filter list of files as provided by the get request
     file_ids = params[:file_ids] || []
+    cb_file_ids    = CbrainFileList.where(:id => file_ids).pluck(:id)
+    other_file_ids = file_ids.map(&:to_i) - cb_file_ids
     if @tool_config.try(:inputs_readonly) || @task.class.properties[:readonly_input_files]
       access = :read
     else
       access = :write
     end
-    @files   = Userfile.find_accessible_by_user(file_ids, current_user, :access_requested => access) rescue []
+    cb_files     = Userfile.find_accessible_by_user(cb_file_ids, current_user, :access_requested => :read) rescue []
+    other_files  = Userfile.find_accessible_by_user(cb_file_ids, current_user, :access_requested => access) rescue []
+    @files = cb_files + other_files
     if @files.count == 0
       flash[:error] = "You must select at least one file to which you have write access."
       redirect_to :controller  => :userfiles, :action  => :index
MontrealSergiy commented 1 month ago

works in typical cases, though not entirely bug-free. In the presence if one submit usual files with insufficient access, in the presence of CBRAIN file lists, the former would be silently removed (but lists and other files with sufficient access will). Video for bug

Also, IMHO if CBRAIN files do not have read access, but normal files have they also would be silently ignored.

MontrealSergiy commented 1 month ago

I think I can modify it to avoid the pitfall

I think I can introduce an additional check, if it is ok

@files.count != files_ids.count

( Though, personally, I would prefer proper catching of not found exception )