Merge pull request 'Admin API: correctly return aliased buckets for keys even if they have no permissions' (#1498) from fix-aliases-no-perms into main-v2

Reviewed-on: https://git.deuxfleurs.fr/Deuxfleurs/garage/pulls/1498

src/api/admin/bucket.rs

@@ -690,45 +690,36 @@ async fn bucket_info_results(
 		.map(|x| x.filtered_values(&garage.system.cluster_layout()))
 		.unwrap_or_default();
 
-	let mut relevant_keys = HashMap::new();
-	for (k, _) in bucket
-		.state
-		.as_option()
-		.unwrap()
+	let state = bucket.state.as_option().unwrap();
+
+	let keys1 = state
 		.authorized_keys
 		.items()
 		.iter()
-	{
-		if let Some(key) = garage
-			.key_table
-			.get(&EmptyKey, k)
-			.await?
-			.filter(|k| !k.is_deleted())
-		{
-			if !key.state.is_deleted() {
-				relevant_keys.insert(k.clone(), key);
-			}
-		}
-	}
-	for ((k, _), _, _) in bucket
-		.state
-		.as_option()
-		.unwrap()
+		.filter(|(_, p)| p.is_any())
+		.map(|(k, _)| k);
+	let keys2 = state
 		.local_aliases
 		.items()
 		.iter()
-	{
-		if relevant_keys.contains_key(k) {
+		.filter(|(_, _, p)| *p)
+		.map(|((k, _), _, _)| k);
+
+	let mut relevant_keys = HashMap::new();
+	for key_id in keys1.chain(keys2) {
+		if relevant_keys.contains_key(key_id) {
 			continue;
 		}
-		if let Some(key) = garage.key_table.get(&EmptyKey, k).await? {
-			if !key.state.is_deleted() {
-				relevant_keys.insert(k.clone(), key);
-			}
+		if let Some(key) = garage.key_table.get(&EmptyKey, key_id).await? {
+			relevant_keys.insert(key_id.clone(), key);
+		} else {
+			warn!(
+				"Bucket {:?} references non-existent key {}",
+				bucket.id, key_id
+			);
 		}
 	}
-
-	let state = bucket.state.as_option().unwrap();
+	relevant_keys.retain(|_, k| !k.is_deleted());
 
 	let quotas = state.quotas.get();
 	let res = GetBucketInfoResponse {
@@ -768,29 +759,30 @@ async fn bucket_info_results(
 		}),
 		keys: relevant_keys
 			.into_values()
-			.filter_map(|key| {
-				let p = key.state.as_option().unwrap();
-				let permissions = p
+			.map(|key| {
+				let st = key.state.as_option().unwrap();
+				let permissions = st
 					.authorized_buckets
 					.get(&bucket.id)
-					.filter(|p| p.is_any())
 					.map(|p| ApiBucketKeyPerm {
 						read: p.allow_read,
 						write: p.allow_write,
 						owner: p.allow_owner,
-					})?;
-				Some(GetBucketInfoKey {
+					})
+					.unwrap_or_default();
+				let bucket_local_aliases = st
+					.local_aliases
+					.items()
+					.iter()
+					.filter(|(_, _, b)| b.into_inner() == Some(bucket.id))
+					.map(|(n, _, _)| n.to_string())
+					.collect::<Vec<_>>();
+				GetBucketInfoKey {
 					access_key_id: key.key_id,
-					name: p.name.get().to_string(),
+					name: st.name.get().to_string(),
 					permissions,
-					bucket_local_aliases: p
-						.local_aliases
-						.items()
-						.iter()
-						.filter(|(_, _, b)| b.into_inner() == Some(bucket.id))
-						.map(|(n, _, _)| n.to_string())
-						.collect::<Vec<_>>(),
-				})
+					bucket_local_aliases,
+				}
 			})
 			.collect::<Vec<_>>(),
 		objects: *counters.get(OBJECTS).unwrap_or(&0),

src/api/admin/key.rs

@@ -188,30 +188,34 @@ async fn key_info_results(
 	key: Key,
 	show_secret: bool,
 ) -> Result<GetKeyInfoResponse, Error> {
-	let mut relevant_buckets = HashMap::new();
-
 	let key_state = key.state.as_option().unwrap();
 
-	for id in key_state
+	let buckets1 = key_state
 		.authorized_buckets
 		.items()
 		.iter()
-		.map(|(id, _)| id)
-		.chain(
-			key_state
-				.local_aliases
-				.items()
-				.iter()
-				.filter_map(|(_, _, v)| v.inner()),
-		) {
-		if !relevant_buckets.contains_key(id) {
-			if let Some(b) = garage.bucket_table.get(&EmptyKey, id).await? {
-				if b.state.as_option().is_some() {
-					relevant_buckets.insert(*id, b);
-				}
+		.filter(|(_, p)| p.is_any())
+		.map(|(id, _)| id);
+	let buckets2 = key_state
+		.local_aliases
+		.items()
+		.iter()
+		.filter_map(|(_, _, v)| v.inner());
+
+	let mut relevant_buckets = HashMap::new();
+	for bucket_id in buckets1.chain(buckets2) {
+		if !relevant_buckets.contains_key(bucket_id) {
+			if let Some(b) = garage.bucket_table.get(&EmptyKey, bucket_id).await? {
+				relevant_buckets.insert(*bucket_id, b);
+			} else {
+				warn!(
+					"Key {} references non-existent bucket {:?}",
+					key.key_id, bucket_id
+				);
 			}
 		}
 	}
+	relevant_buckets.retain(|_, b| !b.is_deleted());
 
 	let res = GetKeyInfoResponse {
 		name: key_state.name.get().clone(),
@@ -233,7 +237,7 @@ async fn key_info_results(
 		},
 		buckets: relevant_buckets
 			.into_values()
-			.filter_map(|bucket| {
+			.map(|bucket| {
 				let state = bucket.state.as_option().unwrap();
 				let permissions = key_state
 					.authorized_buckets
@@ -243,8 +247,9 @@ async fn key_info_results(
 						read: p.allow_read,
 						write: p.allow_write,
 						owner: p.allow_owner,
-					})?;
-				Some(KeyInfoBucketResponse {
+					})
+					.unwrap_or_default();
+				KeyInfoBucketResponse {
 					id: hex::encode(bucket.id),
 					global_aliases: state
 						.aliases
@@ -261,7 +266,7 @@ async fn key_info_results(
 						.map(|((_, n), _, _)| n.to_string())
 						.collect::<Vec<_>>(),
 					permissions,
-				})
+				}
 			})
 			.collect::<Vec<_>>(),
 	};