jneilliii / OctoPrint-BackupScheduler

11 stars 3 forks source link

0.2.0 #26

Closed NilsRo closed 1 week ago

NilsRo commented 2 months ago

Actually not finished. Open topics:

NilsRo commented 2 months ago

@jneilliii : I think I am finished. Will do the translation later, so you can take a look and test/merge/change something if you like.

NilsRo commented 2 months ago

Think I made a mistake with handling of old OctoPrint version. There should be perhaps another legacy branch. Or it can be ignored as it simply dies not work. There should be no error in that case.

jneilliii commented 2 months ago

What is the dependency requirement on 1.9.0?

I think the smtp password should be included in a get_settings_restricted_paths override allowing only admins to get this over api calls. Ideally the password would be encrypted and I can think of one example plugin that has implemented that here.

NilsRo commented 2 months ago

The backup hooks are implemented in 1.9.0. Would be also possible to simply disable the settings in Jinja, as without the hooks simply nothing happens.

That's a good idea with restricted parameters. If I encrypt is the key I need the key for decryption in the code. It is perhaps not that useful if somebody can simply find the key there. If you have an idea/example I would appreciate this.

jneilliii commented 2 months ago

That's a good idea with restricted parameters. If I encrypt is the key I need the key for decryption in the code. It is perhaps not that useful if somebody can simply find the key there. If you have an idea/example I would appreciate this.

Yeah, that's a good point. You can save the seed key in the plugin's data folder using self.get_plugin_data_folder() and then exclude the file from backup using octoprint.plugin.backup.additional_excludes hook. It would still be available from within the system and of course possible to reverse engineer the encryption, but at least adds a layer of separation and wouldn't be included in a system info bundle provided for support for example.

Another option might be to store the seed key into an environment variable instead of a file or use the devices mac address or other hardware identifier as seed key. Then you don't have to worry about the exclude hook but still gives you the separation and wouldn't need the backup exclusion.

In either instance, the settings would have to be redone on restore and might make sense to use the octoprint.plugin.backup.after_restore hook to clear/change specific settings to avoid possible errors.

jneilliii commented 2 months ago

I'm not opposed to limiting the version of the plugin to >= 1.9.0 either and that would allow the use of after_restore hook and you wouldn't have to deal with the settings visibility, etc.

NilsRo commented 2 months ago

Was not a big deal and perhaps will motivate somebody laying on the old version to update. Also it does not open another line to maintain so in my opinion its a simple and good option.

I would prefer to avoid some hardware dependencies outside of OctoPrint...more complexity. The first option sound good to me. If somebody can access the shell it still has many rights, backups of OctoPrint does not contain the encryption key so it stays on the OctoPrint server and cannot be downloaded with the backup.

NilsRo commented 2 months ago

Spend some thoughts about it and google a little bit. It seems common for Python automation to save passwords in the env variable directly. So it will be also not persistent in the backup. As somebody have access to the env variable he could ether decrypt the password if stored in the filesystem/config.yaml. With that the credentials do not leave the machine. Would be the most easiest implementation.

https://able.bio/rhett/how-to-set-and-get-environment-variables-in-python--274rgt5

NilsRo commented 1 month ago

@jneilliii : What do you think? Mergeable with storing it as an environment variable? Then I will go into the implementation.

jneilliii commented 1 month ago

Yeah, I think the environment variable is a good approach. Would gladly merge with that.

NilsRo commented 1 month ago

I think I am finished with it.

jneilliii commented 1 month ago

I've made some changes to the code in the above commits and would like to get verification that the notices and mount checks are still working properly? I ended up changing the password storage to encrypted local file because I was getting errors from the os.environ approach so you will need to re-enter those settings. Couple of additional tweaks and consolidation of notification messages and changes to how retained message can be cleared.

I'm thinking I may want to make another change to this PR prior to merging by converting the retain_message field to a list and timestamp/append error messages to it until cleared.

NilsRo commented 1 month ago

I did not implemented the list to avoid spamming the user with very old messages...perhaps we should constrained it to the last 20 notifications or something like this.

NilsRo commented 1 week ago

@jneilliii : Any updates? I am on a trip tomorrow so can take a look on something new. Like to close the actual iteration and finish anything important. So we can add something missing later also on user feedback.

jneilliii commented 1 week ago

Sorry, got side tracked with one of my other plugins and furthering its development (Bambu integration for OctoPrint). I just haven't gone back and validated everything is working as expected. Were you able to test the changes I made to the PR yet relative to checking the mounts, etc. Just want to make sure I didn't break anything with those changes.

NilsRo commented 1 week ago

From core functions it looks very fine. But I am not sure with the notifications as I did not test them. The last time they looks unfinished to me. If you push your last version I can work on it to get it ready if there are some todos still open.

jneilliii commented 1 week ago

thanks for the contribution, I have released version 0.2.0