From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id C18CB69370 for ; Mon, 18 Jan 2021 06:57:12 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id B7E42EF14 for ; Mon, 18 Jan 2021 06:57:12 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 298BEEF0A for ; Mon, 18 Jan 2021 06:57:12 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id E682744823 for ; Mon, 18 Jan 2021 06:57:11 +0100 (CET) To: Proxmox Backup Server development discussion , =?UTF-8?Q?Fabian_Gr=c3=bcnbichler?= References: <20210115104855.3992674-1-f.gruenbichler@proxmox.com> <20210115104855.3992674-3-f.gruenbichler@proxmox.com> From: Thomas Lamprecht Message-ID: Date: Mon, 18 Jan 2021 06:57:11 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:85.0) Gecko/20100101 Thunderbird/85.0 MIME-Version: 1.0 In-Reply-To: <20210115104855.3992674-3-f.gruenbichler@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.029 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.194 Looks like a legit reply (A) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox-backup 3/3] pull: only remove owned groups X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Jan 2021 05:57:12 -0000 On 15.01.21 11:48, Fabian Gr=C3=BCnbichler wrote: > we also only create/add snapshots to owned groups when syncing, so > removing groups with different ownership is a rather confusing > side-effect.. >=20 > Signed-off-by: Fabian Gr=C3=BCnbichler > --- >=20 > 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 sour= ces with > different owners.. > =20 > datastores just used as sync target for a single job should still b= ehave 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.. >=20 > src/client/pull.rs | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) >=20 > 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( > =20 > let mut errors =3D false; > =20 > - let mut new_groups =3D std::collections::HashSet::new(); > + let mut remote_groups =3D std::collections::HashSet::new(); > for item in list.iter() { > - new_groups.insert(BackupGroup::new(&item.backup_type, &item.ba= ckup_id)); > + remote_groups.insert(BackupGroup::new(&item.backup_type, &item= =2Ebackup_id)); > } > =20 > + let correct_owner =3D |owner: &Authid, auth_id: &Authid| -> bool {= > + owner =3D=3D auth_id || (owner.is_token() && &Authid::from(own= er.user().clone()) =3D=3D auth_id) > + }; > + > let mut progress =3D StoreProgress::new(list.len() as u64); > =20 > for (done, item) in list.into_iter().enumerate() { > @@ -617,7 +621,7 @@ pub async fn pull_store( > }; > =20 > // permission check > - if auth_id !=3D owner { > + if !correct_owner(&owner, &auth_id) { this is now also changed to include token owned groups, or? As the `corre= ct_owner` closure checks not only the replaced (negated) auth_id =3D=3D owner but also an e= xplicit token check? (did not looked to much at code out of context, just FYI) > // only the owner is allowed to create additional snapshot= s > worker.log(format!( > "sync group {}/{} failed - owner check failed ({} !=3D= {})", > @@ -645,9 +649,16 @@ pub async fn pull_store( > =20 > if delete { > let result: Result<(), Error> =3D proxmox::try_block!({ > - let local_groups =3D BackupInfo::list_backup_groups(&tgt_s= tore.base_path())?; > - for local_group in local_groups { > - if new_groups.contains(&local_group) { > + let local_owned_groups: Vec =3D > + BackupInfo::list_backup_groups(&tgt_store.base_path())= ? > + .into_iter() > + .filter(|group| match tgt_store.get_owner(&group) = { > + Ok(owner) =3D> correct_owner(&owner, &auth_id)= , > + Err(_) =3D> false, > + }) > + .collect(); > + for local_group in local_owned_groups { > + if remote_groups.contains(&local_group) { > continue; > } > worker.log(format!( >=20