bcgov / SIMS

Student Information Management System. Post-Secondary Student Financial Aid System
Apache License 2.0
22 stars 13 forks source link

Virus Scanning - Ministry User File Interactions #3297

Closed CarlyCotton closed 4 days ago

CarlyCotton commented 2 months ago

User Story As a Ministry User, I would like to ensure that malware is not propagated VIA SIMS due to the reputational risk and as such require that all files received via SIMS are scanned. This involves not allowing a file to be downloaded before it has been scanned as OK, and to put a placeholder file when files had scanning issues.

Acceptance Criteria

Technical Context

Context Follow up on how to deal with the virus scanning in #3180 for users.

CarlyCotton commented 2 months ago

@HRAGANBC @Joshua-Lakusta @JerPearson Please take a look and let me know if we're missing anything to push this up to lather.

HRAGANBC commented 2 months ago

Remove file attached. Insert placeholder text file. Add System message..."

Not clear to me how this is different than the following AC. Are you envisioning an error message AND a text file containing some contents?

CarlyCotton commented 2 months ago

@HRAGANBC I was thinking that we'd have the placeholder text file(with a note inside to provide context to the student) AND adding a note to the student profile (as a guide for ministry users). I've updated the AC to better show the two items as separate.

guru-aot commented 1 month ago

"Filename should be the same as the deleted file with "-OriginalFileError" at the end" can we update it to "-OriginalFileError.txt" ? @CarlyCotton @HRAGANBC Please confirm

CarlyCotton commented 3 weeks ago

@guru-aot Yes, can have the .txt extension after "-OriginalFileError"

andrepestana-aot commented 1 week ago

@CarlyCotton

  1. Should we really delete the files permanently or just do not return the infected content to the user? I'm confirming this now because a question was raised due to other requirements about soft deleting other data in the past.
  2. Could we return a toast message to the user in case of infected file the same way as file scan pending? This could make it more consistent/uniform when dealing with files in the application.
CarlyCotton commented 1 week ago

@andrepestana-aot

  1. Just want to have that file NOT accessible to users. So if that's a soft delete or "hiding" as long as that gets us to the result. I can update the AC to remove "Remove and delete file attached"?
  2. A toast message or a modal when they try to click on it to download? I think the discussion was around having a placeholder file since the scan may not be instant and we need a way to show why their original file isn't there anymore.
andrepestana-aot commented 1 week ago

@CarlyCotton

  1. Yes, then please update the AC to "Do not allow the download for files of pending scan. Instead of deleting the content, just do not return it to the user without changing the original file." or something similar.
  2. Yes, the way to show them why the file is not accessible could be through a toast message as we are doing for pending scan files. When they click on the filename they're going to see a toast message "Due to our security rules, the original file, [filename], was deleted. Please re-check your file and attempt to re-upload." or something similar.
CarlyCotton commented 1 week ago

@andrepestana-aot I have updated the AC and edited my comment because I don't actually want to ensure that a sketchy file is accessible to users. Is there a reason to change this to a toast message instead of the way it was written/groomed/estimated?

andrewsignori-aot commented 1 week ago

As per the conversation including the team and @ninosamson the decision is not to delete the file from the storage. Later ticket to change purge failed files if needed.

CarlyCotton commented 1 week ago

To clarify regarding Guru's question above - we were asked if the text to show it failed could be in front of the extension. I answered it can be. If I'm asked if it can be behind the extension, it can be. As long as there is an indication of the error in the file name,

CarlyCotton commented 6 days ago

@andrepestana-aot

Updated the last AC to:

Original:

Scanned but Failed/Virus Detected: Perform the following automatically:

  • Hide the failed file so it cannot be accessed by users. Insert placeholder text file (see next AC).
    • Filename should be the same as the deleted file with "-OriginalFileError" at the end. Inside the text file we can include more information: "Due to our security rules, the original file, [filename], was deleted. Please re-check your file and attempt to re-upload.