preserve-video-filename #49

This should fix issue #40

I have not made the allowed_characters configurable yet nor the replacement_character.

Mostly because I couldn't decide if I should define those "globally" on a server basis, or on the library nodes.

Feel free to modify, extend, rip apart. 😁

Hmmm maybe we should make this configurable on a per-library basis? 🤔

Hmmm maybe we should make this configurable on a per-library basis? 🤔

Yeah. I thought so too. If we have different storage backends, they will have different sets of allowed characters too. But most people will have one library on one kind of backend (local Filesystem). Maybe a second library at a different path but most probable it will be the same backend...

So I will add this as an option on the server level AND on the library level.

But not just yet. First I will refractor the upload handler method, so that it becomes easier to do background processing.

BTW: I think we ought to add 'upload_allowed' as a boolean flag to the configuration of libraries. People might want to allow uploads only to one or a few selected libraries and keep other locations read-only.

Sounds good! 👌 Can you put the PR in WIP until you're done? 🙏

I think we can merge this as it is.
It vastly improves the user experience already as it is.
But yes, making the allowed_characters and replacement_characters configurable will make this a more complete change.

Oh I'm happy to merge in that case, but I meant making the uuid vs. prserved fileaname behaviour configurable. I hope that's what we also mean? 🤔

Oh I'm happy to merge in that case, but I meant making the uuid vs. prserved fileaname behaviour configurable. I hope that's what we also mean? 🤔

Ah I thouht you were talking about making the sets of characters configurable.

Making the general "uuid vs. sanitized_original_name" configurable wasn't even on my radar. I presumed that preserving filenames was preferable over uuid-names in any case. But yes, now that I think about it, it should be configurable.

Haha all good 😅 Yeah in my case, Tube was used for the puprposes of uploading random videos from an iPhone and I run my instance mostly as a "demo" too for others to "try".

So in that sense, preserving original filenames for me doesn't make a lot of sense, I have Plex for that 😅

But as for "what users want", well hard to say, I'm not actually sure how many folks use Tube 😅

Hmm ..

Originally I wanted to be able to set a default for preserve_upload_filename on the server level and the ability to override this on a per-path level in the library.

I just came across an ugly truth ...

A boolean value in json can have three values ( false, true, and absent ) and thus can perfectly encapsulate the three states that I wanted for the library-path-level ( shortuuid, preserve, do-what-the-server-default-says).

This collapses immediately into just two states (true and false) as soon as I read this json into a go struct.

So there will always be uncertainty if a false on the library-path-level is a user supplied value, or if it is the go default for a value that was omitted. 🤔

So, I guess I will have to change to logic a little bit.

Instead of preserve = (defined(library-path-level) ? library-path-level : server-level)
I will need to go with a simple preserve = (library-path-level || server-level) logic.

Or is there an easy solution that I don't see yet?

When I think about adding some more variables to the upload handling, like preserve_upload_filename_allowed_charset and preserve_upload_filename_replacement
and upload_allowed ...

Somehow this looks like it is becoming rather ugly. It feels like there is another class / struct hiding in there.

It looks like we might have something like a UploadConfig class/struct there, who's configuration node exists in the "server" and also optionally inside each library path.

{
    "library": [
        {
            "path": "videos",
            "prefix": ""
            "upload_config": {
                "upload_allowed": true,
                "preserve_filename: false,
                "preserve_allowed": "0-9A-Za-z,.+_-",
                "preserve_replacement: "_",
            }
        }
    ],
    "server": {
        "host": "0.0.0.0",
        "port": 8000,
        "store_path": "tube.db",
        "upload_path": "uploads",
        "upload_config": {
            "upload_allowed": true,
            "preserve_filename: false,
            "preserve_allowed": "0-9A-Za-z,.+_-",
            "preserve_replacement: "_",
        }
        "max_upload_size": 104857600
    },
...

What if you use the securejoin package? Rather than trying to come up with "allow lists" and what not, use this to safelt and programatyically determine the final path. If it errors out, then either the supplied path was invalid or otherwise.

I use this package in a few of my other projects where user input is used to construct path naems.

Securejoin sounds good. 👍
I wonder if we will have to revisit this topic when we add alternative backend storage engines. 🤔 But that is a problem for another time. For now I think securejoin is more than good enough.
Thanks! 🙏

I think I will invert the upload_allowed boolean config variable to readonly.

That way, existing configuration files stay valid. (Non-existing readonly variables default to false 😁 )

@gumbo2000 Is this ready to go? Still has WIP:...

@prologic Ready? 🤔 I was already halfway done, implementing the "readonly" feature. 😆
But, thinking about it, that would be a different feature and I am not finished documenting the preserve_upload_filename feature, yet.

So, yes. I will finish documentation this and then this can be merged.
Any other places apart from the adding it to config.json and README.md?

Speaking of the README.md
Do you prefer inline documentation in the json snippet (even if it violates json syntax),

{
    "library": [
        {
            "path": "videos",
            "prefix": "",
            "preserve_upload_filename: false  // optional
        }
    ],
}

or would you rather have the explanation underneath a valid json document?

BTW: I hope you are right about the security and maturity of SecureJoin. In theory it would allow a lot more than the strict whitelist would permit. E.g. you could write into subdirectories of the chosen library location.

I've removed that possibility by stripping the path away from the name supplied by the user but code drifts and future changes might expose that possibility again.
OTOH it takes that burden and responsibility of configuring the allowed_characters away from the tube administrator.

Ok. I think this one is ready.