Skip to content

Commit 0c1e452

Browse files
parmesantparmesant
committed
Updates: coderabbit suggestions
1 parent 7d2d714 commit 0c1e452

File tree

4 files changed

+80
-141
lines changed

4 files changed

+80
-141
lines changed

‎src/handlers/http/modal/ingest/ingestor_rbac.rs

Lines changed: 25 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ use tokio::sync::Mutex;
2424
use crate::{
2525
handlers::http::{modal::utils::rbac_utils::get_metadata, rbac::RBACError},
2626
rbac::{
27+
map::roles,
2728
user::{self, User as ParseableUser},
2829
Users,
2930
},
@@ -73,38 +74,6 @@ pub async fn delete_user(username: web::Path<String>) -> Result<impl Responder,
7374
Ok(format!("deleted user: {username}"))
7475
}
7576

76-
// // Handler PUT /user/{username}/roles => Put roles for user
77-
// // Put roles for given user
78-
// pub async fn put_role(
79-
// username: web::Path<String>,
80-
// role: web::Json<HashSet<String>>,
81-
// ) -> Result<String, RBACError> {
82-
// let username = username.into_inner();
83-
// let role = role.into_inner();
84-
85-
// if !Users.contains(&username) {
86-
// return Err(RBACError::UserDoesNotExist);
87-
// };
88-
// // update parseable.json first
89-
// let mut metadata = get_metadata().await?;
90-
// if let Some(user) = metadata
91-
// .users
92-
// .iter_mut()
93-
// .find(|user| user.username() == username)
94-
// {
95-
// user.roles.clone_from(&role);
96-
// } else {
97-
// // should be unreachable given state is always consistent
98-
// return Err(RBACError::UserDoesNotExist);
99-
// }
100-
101-
// let _ = storage::put_staging_metadata(&metadata);
102-
// // update in mem table
103-
// Users.add_roles(&username.clone(), role.clone());
104-
105-
// Ok(format!("Roles updated successfully for {username}"))
106-
// }
107-
10877
// Handler PATCH /user/{username}/role/sync/add => Add roles to a user
10978
pub async fn add_roles_to_user(
11079
username: web::Path<String>,
@@ -147,6 +116,30 @@ pub async fn remove_roles_from_user(
147116
if !Users.contains(&username) {
148117
return Err(RBACError::UserDoesNotExist);
149118
};
119+
120+
// check if all roles exist
121+
let mut non_existent_roles = Vec::new();
122+
roles_to_remove.iter().for_each(|r| {
123+
if roles().get(r).is_none() {
124+
non_existent_roles.push(r.clone());
125+
}
126+
});
127+
128+
if !non_existent_roles.is_empty() {
129+
return Err(RBACError::RolesDoNotExist(non_existent_roles));
130+
}
131+
132+
// check that user actually has these roles
133+
let user_roles: HashSet<String> = HashSet::from_iter(Users.get_role(&username));
134+
let roles_not_with_user: HashSet<String> =
135+
HashSet::from_iter(roles_to_remove.difference(&user_roles).cloned());
136+
137+
if !roles_not_with_user.is_empty() {
138+
return Err(RBACError::RolesNotAssigned(Vec::from_iter(
139+
roles_not_with_user,
140+
)));
141+
}
142+
150143
// update parseable.json first
151144
let mut metadata = get_metadata().await?;
152145
if let Some(user) = metadata

‎src/handlers/http/role.rs

Lines changed: 12 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,6 @@
1616
*
1717
*/
1818

19-
use std::collections::HashSet;
20-
2119
use actix_web::{
2220
http::header::ContentType,
2321
web::{self, Json},
@@ -103,23 +101,21 @@ pub async fn delete(name: web::Path<String>) -> Result<impl Responder, RoleError
103101
};
104102
}
105103

104+
// remove role from all user groups that have it
106105
let mut groups_to_update = Vec::new();
107-
for user_group in group_names {
108-
if let Some(ug) = write_user_groups().get_mut(&user_group) {
109-
ug.remove_roles(HashSet::from_iter([name.clone()]))
110-
.map_err(|e| RoleError::Anyhow(anyhow::Error::msg(e.to_string())))?;
111-
groups_to_update.push(ug.clone());
112-
// ug.update_in_metadata().await?;
113-
} else {
114-
continue;
115-
};
106+
for group in write_user_groups().values_mut() {
107+
if group.roles.remove(&name) {
108+
groups_to_update.push(group.clone());
109+
}
116110
}
117111

118-
// update in metadata
119-
metadata
120-
.user_groups
121-
.retain(|x| !groups_to_update.contains(x));
122-
metadata.user_groups.extend(groups_to_update);
112+
// update metadata only if there are changes
113+
if !groups_to_update.is_empty() {
114+
metadata
115+
.user_groups
116+
.retain(|x| !groups_to_update.contains(x));
117+
metadata.user_groups.extend(groups_to_update);
118+
}
123119
put_metadata(&metadata).await?;
124120

125121
Ok(HttpResponse::Ok().finish())

‎src/rbac/map.rs

Lines changed: 32 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -226,33 +226,9 @@ impl Sessions {
226226
context_user: Option<&str>,
227227
) -> Option<Response> {
228228
self.active_sessions.get(key).map(|(username, perms)| {
229-
// if user is a part of any user groups, then add permissions
230-
let perms: HashSet<Permission> = if let Some(user) = users().0.get(username) {
231-
let all_groups_roles = user
232-
.user_groups
233-
.iter()
234-
.filter(|id| (read_user_groups().0.contains_key(*id)))
235-
.map(|id| read_user_groups().0.get(id).unwrap().roles.clone())
236-
.reduce(|mut acc, e| {
237-
acc.extend(e);
238-
acc
239-
})
240-
.unwrap_or_default();
241-
242-
let mut privilege_list = Vec::new();
243-
all_groups_roles
244-
.iter()
245-
.filter_map(|role| roles().get(role).cloned())
246-
.for_each(|privileges| privilege_list.extend(privileges));
247-
248-
let mut perms = HashSet::from_iter(perms.clone());
249-
for privs in privilege_list {
250-
perms.extend(RoleBuilder::from(&privs).build())
251-
}
252-
perms
253-
} else {
254-
HashSet::from_iter(perms.clone())
255-
};
229+
let mut perms: HashSet<Permission> = HashSet::from_iter(perms.clone());
230+
perms.extend(aggregate_group_permissions(username));
231+
256232
if perms.iter().any(|user_perm| {
257233
match *user_perm {
258234
// if any action is ALL then we we authorize
@@ -317,6 +293,35 @@ impl From<Vec<User>> for Users {
317293
}
318294
}
319295

296+
fn aggregate_group_permissions(username: &str) -> HashSet<Permission> {
297+
let mut group_perms = HashSet::new();
298+
299+
let Some(user) = users().get(username).cloned() else {
300+
return group_perms;
301+
};
302+
303+
if user.user_groups.is_empty() {
304+
return group_perms;
305+
}
306+
307+
for group_name in &user.user_groups {
308+
let Some(group) = read_user_groups().get(group_name).cloned() else {
309+
continue;
310+
};
311+
312+
for role_name in &group.roles {
313+
let Some(privileges) = roles().get(role_name).cloned() else {
314+
continue;
315+
};
316+
317+
for privilege in privileges {
318+
group_perms.extend(RoleBuilder::from(&privilege).build());
319+
}
320+
}
321+
}
322+
323+
group_perms
324+
}
320325
// Map of [user group ID --> UserGroup]
321326
// This map is populated at startup with the list of user groups from parseable.json file
322327
#[derive(Debug, Default, Clone, derive_more::Deref, derive_more::DerefMut)]

‎src/rbac/role.rs

Lines changed: 11 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -90,8 +90,6 @@ pub enum ParseableResourceType {
9090
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
9191
pub enum Permission {
9292
Unit(Action),
93-
// Stream(Action, String),
94-
// StreamWithTag(Action, String, Option<String>),
9593
Resource(Action, ParseableResourceType),
9694
SelfUser,
9795
}
@@ -100,40 +98,16 @@ pub enum Permission {
10098
#[derive(Debug, Default)]
10199
pub struct RoleBuilder {
102100
actions: Vec<Action>,
103-
// stream: Option<String>,
104-
// tag: Option<String>,
105-
// resource_id: Option<String>,
106101
resource_type: Option<ParseableResourceType>,
107102
}
108103

109104
// R x P
110105
impl RoleBuilder {
111-
pub fn with_resource(
112-
mut self,
113-
resource_type: ParseableResourceType,
114-
// resource_id: String,
115-
) -> Self {
106+
pub fn with_resource(mut self, resource_type: ParseableResourceType) -> Self {
116107
self.resource_type = Some(resource_type);
117-
// self.resource_id = Some(resource_id);
118108
self
119109
}
120110

121-
// pub fn with_stream(mut self, stream: String) -> Self {
122-
// self.stream = Some(stream);
123-
// self
124-
// }
125-
126-
// pub fn with_tag(mut self, tag: String) -> Self {
127-
// self.tag = Some(tag);
128-
// self
129-
// }
130-
131-
// pub fn with_resource(mut self, resource_id: String, resource_type: ParseableResourceType) -> Self {
132-
// self.resource_id = Some(resource_id);
133-
// self.resource_type = Some(resource_type);
134-
// self
135-
// }
136-
137111
pub fn build(self) -> Vec<Permission> {
138112
let mut perms = Vec::new();
139113
for action in self.actions {
@@ -192,11 +166,7 @@ impl RoleBuilder {
192166
| Action::GetStats
193167
| Action::GetRetention
194168
| Action::PutRetention
195-
| Action::All => Permission::Resource(
196-
action,
197-
self.resource_type.clone().unwrap(),
198-
// self.resource_id.clone().unwrap(),
199-
),
169+
| Action::All => Permission::Resource(action, self.resource_type.clone().unwrap()),
200170
};
201171
perms.push(perm);
202172
}
@@ -218,32 +188,22 @@ pub mod model {
218188
pub enum DefaultPrivilege {
219189
Admin,
220190
Editor,
221-
Writer {
222-
resource: ParseableResourceType,
223-
// resource_id: String,
224-
},
225-
Ingestor {
226-
resource: ParseableResourceType,
227-
},
228-
Reader {
229-
resource: ParseableResourceType,
230-
// resource_id: String,
231-
},
191+
Writer { resource: ParseableResourceType },
192+
Ingestor { resource: ParseableResourceType },
193+
Reader { resource: ParseableResourceType },
232194
}
233195

234196
impl From<&DefaultPrivilege> for RoleBuilder {
235197
fn from(value: &DefaultPrivilege) -> Self {
236198
match value {
237199
DefaultPrivilege::Admin => admin_perm_builder(),
238200
DefaultPrivilege::Editor => editor_perm_builder(),
239-
DefaultPrivilege::Writer {
240-
resource,
241-
// resource_id,
242-
} => writer_perm_builder().with_resource(resource.to_owned()),
243-
DefaultPrivilege::Reader {
244-
resource,
245-
// resource_id,
246-
} => reader_perm_builder().with_resource(resource.to_owned()),
201+
DefaultPrivilege::Writer { resource } => {
202+
writer_perm_builder().with_resource(resource.to_owned())
203+
}
204+
DefaultPrivilege::Reader { resource } => {
205+
reader_perm_builder().with_resource(resource.to_owned())
206+
}
247207
DefaultPrivilege::Ingestor { resource } => {
248208
ingest_perm_builder().with_resource(resource.to_owned())
249209
}
@@ -254,10 +214,7 @@ pub mod model {
254214
fn admin_perm_builder() -> RoleBuilder {
255215
RoleBuilder {
256216
actions: vec![Action::All],
257-
// stream: Some("*".to_string()),
258-
// tag: None,
259217
resource_type: Some(ParseableResourceType::All),
260-
// resource_id: Some("*".to_string()),
261218
}
262219
}
263220

@@ -303,9 +260,6 @@ pub mod model {
303260
Action::DeleteDashboard,
304261
Action::GetUserRoles,
305262
],
306-
// stream: Some("*".to_string()),
307-
// tag: None,
308-
// resource_id: Some("*".to_string()),
309263
resource_type: Some(ParseableResourceType::All),
310264
}
311265
}
@@ -346,9 +300,6 @@ pub mod model {
346300
Action::DeleteFilter,
347301
Action::GetUserRoles,
348302
],
349-
// stream: None,
350-
// tag: None,
351-
// resource_id: None,
352303
resource_type: None,
353304
}
354305
}
@@ -382,19 +333,13 @@ pub mod model {
382333
Action::GetUserRoles,
383334
Action::GetAlert,
384335
],
385-
// stream: None,
386-
// tag: None,
387-
// resource_id: None,
388336
resource_type: None,
389337
}
390338
}
391339

392340
fn ingest_perm_builder() -> RoleBuilder {
393341
RoleBuilder {
394342
actions: vec![Action::Ingest],
395-
// stream: None,
396-
// tag: None,
397-
// resource_id: None,
398343
resource_type: None,
399344
}
400345
}

0 commit comments

Comments
 (0)