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

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

Open
prioux opened this issue Nov 17, 2020 · 5 comments · May be fixed by #1417
Open

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

prioux opened this issue Nov 17, 2020 · 5 comments · May be fixed by #1417

Comments

@prioux
Copy link
Member

prioux commented Nov 17, 2020

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.

@prioux prioux changed the title The framework should never try to ensure a CbrainFileList is read-write when launching a task Never try to ensure a CbrainFileList is read-write when launching a task Nov 17, 2020
@MontrealSergiy
Copy link
Contributor

then some bad tools can overwrite those files?

@MontrealSergiy
Copy link
Contributor

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
Copy link
Member Author

prioux commented Jul 23, 2024

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
Copy link
Contributor

MontrealSergiy commented Jul 25, 2024

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
Copy link
Contributor

MontrealSergiy commented Jul 25, 2024

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 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants