Re: [pbs-devel] [PATCH v4 proxmox{, -backup} 0/2] close #4763: client: added command to forget backup group

["Gabriel Goller" <g.goller@proxmox.com>]

On Wed Apr 17, 2024 at 4:15 PM CEST, Christian Ebner wrote:
> Hi,
> thanks for tackling this issue.
>
> The group forget command is something I missed quite a lot when doing 
> testing on PBS with accumulating snapshots in a group an not wanting to 
> fallback to the UI (I actually always went the route of deleting the 
> snapshot folder).
>
> Tested the following:
> - Created a few backup snapshots, creating thereby a new backup group
> - Tried to delete the group while backup is running
>    result was as expected: delete failed, missing lock error
> - Tried to delete the group while a restore is running
>    result was as expected: delete failed, missing lock (although 
> different from the previous one)
> - Tried to remove without any other task running
>    result was as expected: group deleted, including the group folder in 
> the datastore
> - Tried to delete empty group
>    result was as expected: group deleted, including the group folder in 
> the datastore
>
> Two further things I noticed:
>
> The confirmation dialog also allows me to type in `yolo` or any string 
> starting with `yY` and nevertheless accepts this as valid confirmation 
> input (similar for negation). Should we limit this further?

It seems 'apt' also matches just on the beginning character, but
nevertheless we can do this, wouldn't hurt restricting it more

I think adding a '$' on the regex should do the trick:

    let no_regex: Regex = Regex::new("^[nN]$").unwrap();


> When one tries to delete a non existing group, the dialog asks me for 
> confirmation, failing however afterwards with an error message, leaking 
> also the datastore path to the client. While the former is not an issue 
> and the intention is to be able to remove empty groups, the latter is 
> not okay in my opinion.
> So either check if the group even exists before asking for confirmation, 
> or map the error to not leak the datastore path.

The thing is that we don't differentiate between an empty group or a
nonexistent group — at least when using the api. This means that even
the list-groups api call will **not** return a group if it doesn't contain
any snapshots, but deleting it will succeed (because it still exists).

What we can do is obviously ignore the error message and simply return a
generic "failed to remove group" or "group not found" to avoid leaking
stuff. Although debugging a issue will be much harder with these vague 
error messages. 

IMO leaking the datastore is not a big issue, as the client can also
list task logs and the datastore path is going to be visible in there as
well.

> For the rest consider this:
>
> Tested-by: Christian Ebner <c.ebner@proxmox.com>

Thanks for testing this!


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel