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

Only ensure a CbrainFileList read access at task launch, resolves #1057 #1417

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

MontrealSergiy
Copy link
Contributor

see #1057 for details

@prioux
Copy link
Member

prioux commented Jul 30, 2024

Looks good to me.

@MontrealSergiy
Copy link
Contributor Author

MontrealSergiy commented Jul 30, 2024

Hi, thanks. Indeed technically feature seems working, and task launch renders task submission forms successfully. Still there could be more problems down the road: task submission, validation, etc whenever file access is double-checked. (Repeat check servers either as anti-hack or access change support I guess). It may deserve a separate ticket, but if you like I can add they here. I think rest of task access check should be modified, case by case ( or, if you prefer keep code dried, by adding skip_cbcvs / skip_lists etc param to find_accessible_userfiles method, to keep it drier ).

@MontrealSergiy
Copy link
Contributor Author

MontrealSergiy commented Jul 30, 2024

I am a bit unsure with files that have cbcsv extension yet not type

@MontrealSergiy
Copy link
Contributor Author

@prioux I pushed a few changes to make sure that the task with read-only list can pass the validation

@MontrealSergiy MontrealSergiy changed the base branch from master to dev August 1, 2024 21:01
@prioux
Copy link
Member

prioux commented Jan 21, 2025

As I review this right now, there are three changed files in the pull request.

  1. the changes to the tasks_controller.rb seem fine (as I already reviewed them) and I think they are the only necessary changes needed;
  2. the changes to boutiques_portal_task.rb seem wrong, because we no longer call the method file_access_symbol at all for any file, which means we could let a user launch a task that modifies files even when they are not supposed to;
  3. the changes to the old integrator template BrainPortal/lib/cbrain_task_generators/templates/portal.rb.erb are unnecessary because we no longer support that old integrator code.

Copy link
Member

@prioux prioux left a comment

Choose a reason for hiding this comment

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

I think the changes to the second file are wrong. Something doesn't seem right

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

Successfully merging this pull request may close these issues.

Never try to ensure a CbrainFileList is read-write when launching a task
2 participants