From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: "Proxmox Backup Server development discussion"
<pbs-devel@lists.proxmox.com>,
"Fabian Grünbichler" <f.gruenbichler@proxmox.com>
Subject: Re: [pbs-devel] [PATCH proxmox-backup 3/3] pull: only remove owned groups
Date: Mon, 18 Jan 2021 06:57:11 +0100 [thread overview]
Message-ID: <e17dda65-06be-efef-cd92-0a6658f8e0f7@proxmox.com> (raw)
In-Reply-To: <20210115104855.3992674-3-f.gruenbichler@proxmox.com>
On 15.01.21 11:48, Fabian Grünbichler wrote:
> we also only create/add snapshots to owned groups when syncing, so
> removing groups with different ownership is a rather confusing
> side-effect..
>
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
>
> Notes:
> came up in the forum, the restricted behaviour is better for mixed usage as
> sync target and regular datastore, or sync target for multiple sources with
> different owners..
>
> datastores just used as sync target for a single job should still behave the
> same (they have a single owner), datastores used as sync target for multiple
> jobs with the same owner should still not use remove_vanished.. we'd need to
> keep track of the sync origin inside the group for that to work..
>
> src/client/pull.rs | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/src/client/pull.rs b/src/client/pull.rs
> index 15514374..33a6c0f1 100644
> --- a/src/client/pull.rs
> +++ b/src/client/pull.rs
> @@ -590,11 +590,15 @@ pub async fn pull_store(
>
> let mut errors = false;
>
> - let mut new_groups = std::collections::HashSet::new();
> + let mut remote_groups = std::collections::HashSet::new();
> for item in list.iter() {
> - new_groups.insert(BackupGroup::new(&item.backup_type, &item.backup_id));
> + remote_groups.insert(BackupGroup::new(&item.backup_type, &item.backup_id));
> }
>
> + let correct_owner = |owner: &Authid, auth_id: &Authid| -> bool {
> + owner == auth_id || (owner.is_token() && &Authid::from(owner.user().clone()) == auth_id)
> + };
> +
> let mut progress = StoreProgress::new(list.len() as u64);
>
> for (done, item) in list.into_iter().enumerate() {
> @@ -617,7 +621,7 @@ pub async fn pull_store(
> };
>
> // permission check
> - if auth_id != owner {
> + if !correct_owner(&owner, &auth_id) {
this is now also changed to include token owned groups, or? As the `correct_owner` closure
checks not only the replaced (negated) auth_id == owner but also an explicit token check?
(did not looked to much at code out of context, just FYI)
> // only the owner is allowed to create additional snapshots
> worker.log(format!(
> "sync group {}/{} failed - owner check failed ({} != {})",
> @@ -645,9 +649,16 @@ pub async fn pull_store(
>
> if delete {
> let result: Result<(), Error> = proxmox::try_block!({
> - let local_groups = BackupInfo::list_backup_groups(&tgt_store.base_path())?;
> - for local_group in local_groups {
> - if new_groups.contains(&local_group) {
> + let local_owned_groups: Vec<BackupGroup> =
> + BackupInfo::list_backup_groups(&tgt_store.base_path())?
> + .into_iter()
> + .filter(|group| match tgt_store.get_owner(&group) {
> + Ok(owner) => correct_owner(&owner, &auth_id),
> + Err(_) => false,
> + })
> + .collect();
> + for local_group in local_owned_groups {
> + if remote_groups.contains(&local_group) {
> continue;
> }
> worker.log(format!(
>
next prev parent reply other threads:[~2021-01-18 5:57 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-15 10:48 [pbs-devel] [PATCH proxmox-backup 1/3] pull: rustfmt Fabian Grünbichler
2021-01-15 10:48 ` [pbs-devel] [PATCH proxmox-backup 2/3] pull: add error context for initial group list call Fabian Grünbichler
2021-01-18 5:52 ` [pbs-devel] applied: " Thomas Lamprecht
2021-01-15 10:48 ` [pbs-devel] [PATCH proxmox-backup 3/3] pull: only remove owned groups Fabian Grünbichler
2021-01-18 5:57 ` Thomas Lamprecht [this message]
2021-01-18 8:35 ` Fabian Grünbichler
2021-01-18 5:52 ` [pbs-devel] applied: [PATCH proxmox-backup 1/3] pull: rustfmt Thomas Lamprecht
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e17dda65-06be-efef-cd92-0a6658f8e0f7@proxmox.com \
--to=t.lamprecht@proxmox.com \
--cc=f.gruenbichler@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox