search

cameroncros / OctoPrint-DiscordRemote

Discord plugin for OctoPrint
MIT License
66 stars 34 forks source link

Added auto-unzipping #164

Closed Stwend closed 2 years ago

Stwend commented 3 years ago

By default, auto-unzipping is turned off, it can be turned on in the plugin's settings.

Auto-unzipping detects multi-volume files (and their total count if possible) and regular zip files, and unzips them if possible.

During sending single-volume files, the user is told:

During receiving multi-volume files, the user is told:

As a side effect, it is now possible to call the unzip command using any of the received files, not just the one ending in ".zip.001".

The unit test tests for the following cases:

1: File is a .zip and we're allowed to auto-unzip 2: File is a .zip but we're not allowed to auto-unzip 3: File is not a .zip file 4: File is a multi-volume .zip.001, ... file, all volumes present 5: File is a multi-volume .zip.001, ... file, one volume present 6: File is a multi-volume .zip.001, ... file, two volumes present, last one not included 7: File is a multi-volume .zip.001, ... file, two volumes present, last one included 8: File is a multi-volume .zip.001, ... file, all volumes present but we're not allowed to auto-unzip

cameroncros commented 3 years ago

Sorry for the delay in review, but I have this issue: image Neither method auto-unzipped? They are valid zip files.

Stwend commented 3 years ago

Hm, very weird. When you open Octoprint, are those files present in the file list? It seems like it's trying to unzip test.zip but can't find the file anymore. I'm not sure why it's not giving any feedback at all on the multivolume files, normally you'd at least get "1 of ??? files received". Could I get your test files? Really curious why it's not working.

cameroncros commented 3 years ago

Sorry for the delay, but life got in the way.

https://cameron.servebeer.com:28715/d/5a7b2f32a2/ <- these are the zips I was testing with.

Stwend commented 3 years ago

I finally had some time to work on the issue, turns out that with the changes I made, DiscordRemote no longer did a proper check for user permissions regarding uploads.

The conditions causing the bug are fairly simple - if a user doesn't have permission to upload a file and still tries, they get the bug.

This is what happened: download_file() was called, it realizes the user doesn't have permission to upload anything and rejects the file, so far so good. handle_dispatch() then ignores the embed it gets from download_file() - the user doesn't get any feedback informing him he's missing permissions. But even worse, handle_dispatch goes on and tries to unzip that file, which was never uploaded because download_file rejected it due to missing permissions.

To fix that, I made file_upload() return either False or True as a "snapshot payload" depending on permission status. handle_dispatch() picks it up and acts accordingly. Users without permission now get the proper "Denied" error, and users with permission can upload and auto-unzip stuff.

I know that this True/False payload is a bit hacky but I couldn't think of an easier way without meddling with the whole command class structure.

Btw, I can't make any sense of the CircleCI check failing - it seems to run through all the unit tests just fine...

Stwend commented 2 years ago

Hi @cameroncros , not to rush or anything, but should I do a small meaningless commit in order for CircleCI to run over the whole PR once more? It seemed to be broken the last time I committed...

cameroncros commented 2 years ago

Hi @cameroncros , not to rush or anything, but should I do a small meaningless commit in order for CircleCI to run over the whole PR once more? It seemed to be broken the last time I committed...

I'll try trigger a rerun. But, there are merge conflicts atm anyway, so you'll need to fix those up first, which will trigger the build anyway. Might be worth doing that as I can't seem to trigger the build manually.