From f65ccd9a2d7c91f18c28d17e9b700a2042bae9b6 Mon Sep 17 00:00:00 2001 From: Satyam Singh Date: Wed, 22 Nov 2023 14:12:37 +0530 Subject: [PATCH 1/7] Add userinfo --- server/src/handlers/http.rs | 7 ++++++ server/src/handlers/http/oidc.rs | 23 ++++++++++++++------ server/src/handlers/http/rbac.rs | 10 +++++++++ server/src/rbac/map.rs | 6 ++---- server/src/rbac/user.rs | 37 ++++++++++++++++++++++++++++++++ 5 files changed, 72 insertions(+), 11 deletions(-) diff --git a/server/src/handlers/http.rs b/server/src/handlers/http.rs index dfefd2ace..0c53eb14a 100644 --- a/server/src/handlers/http.rs +++ b/server/src/handlers/http.rs @@ -236,6 +236,13 @@ pub fn configure_routes( .authorize_for_user(Action::GetUserRoles), ), ) + .service( + web::resource("/{username}/info").route( + web::get() + .to(rbac::get_info) + .authorize_for_user(Action::GetUserRoles), + ), + ) .service( web::resource("/{username}/generate-new-password") // POST /user/{username}/generate-new-password => reset password for this user diff --git a/server/src/handlers/http/oidc.rs b/server/src/handlers/http/oidc.rs index 55d1d24f6..f2bf2a731 100644 --- a/server/src/handlers/http/oidc.rs +++ b/server/src/handlers/http/oidc.rs @@ -36,7 +36,7 @@ use crate::{ option::CONFIG, rbac::{ map::{SessionKey, DEFAULT_ROLE}, - user::{User, UserType}, + user::{self, User, UserType}, Users, }, storage::{self, ObjectStorageError, StorageMetadata}, @@ -138,7 +138,12 @@ pub async fn reply_login( else { return Ok(HttpResponse::Unauthorized().finish()); }; - let username = user_info.sub.unwrap(); + let username = user_info + .sub + .clone() + .expect("OIDC provider did not return a sub which is currently required."); + let user_info: user::UserInfo = user_info.into(); + let group: Option> = claims .other .remove("groups") @@ -148,9 +153,9 @@ pub async fn reply_login( // User may not exist // create a new one depending on state of metadata let user = match (Users.get_user(&username), group) { - (Some(user), Some(group)) => update_user_if_changed(user, group).await?, + (Some(user), Some(group)) => update_user_if_changed(user, group, user_info).await?, (Some(user), None) => user, - (None, group) => put_user(&username, group).await?, + (None, group) => put_user(&username, group, user_info).await?, }; let id = Ulid::new(); Users.new_session(&user, SessionKey::SessionId(id)); @@ -258,6 +263,7 @@ async fn request_token( async fn put_user( username: &str, group: Option>, + user_info: user::UserInfo, ) -> Result { let mut metadata = get_metadata().await?; let group = group.unwrap_or_else(|| { @@ -275,7 +281,8 @@ async fn put_user( .find(|user| user.username() == username) .cloned() .unwrap_or_else(|| { - let user = User::new_oauth(username.to_owned(), group); + let mut user = User::new_oauth(username.to_owned(), group); + user.user_info = user_info; metadata.users.push(user.clone()); user }); @@ -288,13 +295,15 @@ async fn put_user( async fn update_user_if_changed( mut user: User, group: HashSet, + user_info: user::UserInfo, ) -> Result { - // update user if roles have changed - if user.roles == group { + // update user only if roles or userinfo has changed + if user.roles == group && user.user_info == user_info { return Ok(user); } let metadata = get_metadata().await?; user.roles = group; + user.user_info = user_info; put_metadata(&metadata).await?; Users.put_user(user.clone()); Ok(user) diff --git a/server/src/handlers/http/rbac.rs b/server/src/handlers/http/rbac.rs index 09fc94fe7..42da732dd 100644 --- a/server/src/handlers/http/rbac.rs +++ b/server/src/handlers/http/rbac.rs @@ -139,6 +139,16 @@ pub async fn get_role(username: web::Path) -> Result) -> Result { + let Some(user) = Users.get_user(&username) else { + return Err(RBACError::UserDoesNotExist); + }; + + Ok(web::Json(user.user_info)) +} + // Handler for DELETE /api/v1/user/delete/{username} pub async fn delete_user(username: web::Path) -> Result { let username = username.into_inner(); diff --git a/server/src/rbac/map.rs b/server/src/rbac/map.rs index 7afac7090..3a627f65f 100644 --- a/server/src/rbac/map.rs +++ b/server/src/rbac/map.rs @@ -152,10 +152,8 @@ impl Sessions { permissions: Vec, ) { self.remove_expired_session(&user); - self.user_sessions - .entry(user.clone()) - .and_modify(|sessions| sessions.push((key.clone(), expiry))) - .or_default(); + let sessions = self.user_sessions.entry(user.clone()).or_default(); + sessions.push((key.clone(), expiry)); self.active_sessions.insert(key, (user, permissions)); } diff --git a/server/src/rbac/user.rs b/server/src/rbac/user.rs index 533976012..4f055874b 100644 --- a/server/src/rbac/user.rs +++ b/server/src/rbac/user.rs @@ -38,6 +38,7 @@ pub enum UserType { pub struct User { #[serde(flatten)] pub ty: UserType, + pub user_info: UserInfo, pub roles: HashSet, } @@ -51,6 +52,7 @@ impl User { username, password_hash: hash, }), + user_info: UserInfo::default(), roles: HashSet::new(), }, password, @@ -60,6 +62,7 @@ impl User { pub fn new_oauth(username: String, roles: HashSet) -> Self { Self { ty: UserType::OAuth(OAuth { userid: username }), + user_info: UserInfo::default(), roles, } } @@ -142,6 +145,7 @@ pub fn get_admin_user() -> User { username, password_hash: hashcode, }), + user_info: UserInfo::default(), roles: ["admin".to_string()].into(), } } @@ -150,3 +154,36 @@ pub fn get_admin_user() -> User { pub struct OAuth { userid: String, } + +#[derive(Debug, Default, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] +pub struct UserInfo { + #[serde(default)] + /// User's full name for display purposes. + pub name: Option, + #[serde(default)] + pub preferred_username: Option, + #[serde(default)] + pub picture: Option, + #[serde(default)] + pub email: Option, + #[serde(default)] + pub gender: Option, + #[serde(default)] + pub locale: Option, + #[serde(default)] + pub updated_at: Option, +} + +impl From for UserInfo { + fn from(user: openid::Userinfo) -> Self { + UserInfo { + name: user.name, + preferred_username: user.preferred_username, + picture: user.picture, + email: user.email, + gender: user.gender, + locale: user.locale, + updated_at: user.updated_at, + } + } +} From f4df05636757ce3204566bb887a0718e3da87818 Mon Sep 17 00:00:00 2001 From: Satyam Singh Date: Wed, 22 Nov 2023 18:49:05 +0530 Subject: [PATCH 2/7] Fix --- server/src/handlers/http/oidc.rs | 38 +++++++++++++++++++------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/server/src/handlers/http/oidc.rs b/server/src/handlers/http/oidc.rs index f2bf2a731..53063f022 100644 --- a/server/src/handlers/http/oidc.rs +++ b/server/src/handlers/http/oidc.rs @@ -144,17 +144,24 @@ pub async fn reply_login( .expect("OIDC provider did not return a sub which is currently required."); let user_info: user::UserInfo = user_info.into(); - let group: Option> = claims + let group: HashSet = claims .other .remove("groups") .map(serde_json::from_value) - .transpose()?; + .transpose()? + .unwrap_or_else(|| { + DEFAULT_ROLE + .lock() + .unwrap() + .clone() + .map(|role| HashSet::from([role])) + .unwrap_or_default() + }); // User may not exist // create a new one depending on state of metadata let user = match (Users.get_user(&username), group) { - (Some(user), Some(group)) => update_user_if_changed(user, group, user_info).await?, - (Some(user), None) => user, + (Some(user), group) => update_user_if_changed(user, group, user_info).await?, (None, group) => put_user(&username, group, user_info).await?, }; let id = Ulid::new(); @@ -262,18 +269,10 @@ async fn request_token( // update local cache async fn put_user( username: &str, - group: Option>, + group: HashSet, user_info: user::UserInfo, ) -> Result { let mut metadata = get_metadata().await?; - let group = group.unwrap_or_else(|| { - DEFAULT_ROLE - .lock() - .unwrap() - .clone() - .map(|role| HashSet::from([role])) - .unwrap_or_default() - }); let user = metadata .users @@ -301,10 +300,19 @@ async fn update_user_if_changed( if user.roles == group && user.user_info == user_info { return Ok(user); } - let metadata = get_metadata().await?; + + let mut metadata = get_metadata().await?; user.roles = group; user.user_info = user_info; - put_metadata(&metadata).await?; + if let Some(entry) = metadata + .users + .iter_mut() + .find(|x| x.username() == user.username()) + { + *entry = user.clone(); + put_metadata(&metadata).await?; + } + Users.put_user(user.clone()); Ok(user) } From efef78c6c12106902b953cd378c6fb2e367f32a0 Mon Sep 17 00:00:00 2001 From: Satyam Singh Date: Wed, 22 Nov 2023 19:15:15 +0530 Subject: [PATCH 3/7] Update role --- server/src/handlers/http.rs | 3 ++- server/src/rbac/map.rs | 7 ++++++- server/src/rbac/role.rs | 6 ++++-- 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/server/src/handlers/http.rs b/server/src/handlers/http.rs index 0c53eb14a..187a0c102 100644 --- a/server/src/handlers/http.rs +++ b/server/src/handlers/http.rs @@ -238,9 +238,10 @@ pub fn configure_routes( ) .service( web::resource("/{username}/info").route( + // GET /user/{username}/info => return user information web::get() .to(rbac::get_info) - .authorize_for_user(Action::GetUserRoles), + .authorize_for_user(Action::GetUserInfo), ), ) .service( diff --git a/server/src/rbac/map.rs b/server/src/rbac/map.rs index 3a627f65f..c8a5bd1a4 100644 --- a/server/src/rbac/map.rs +++ b/server/src/rbac/map.rs @@ -218,7 +218,12 @@ impl Sessions { }; (action == required_action || action == Action::All) && ok_stream } - Permission::SelfRole if required_action == Action::GetUserRoles => { + Permission::SelfUser + if matches!( + required_action, + Action::GetUserRoles | Action::GetUserInfo + ) => + { context_user.map(|x| x == username).unwrap_or_default() } _ => false, diff --git a/server/src/rbac/role.rs b/server/src/rbac/role.rs index 0b8f70f52..55393e8d9 100644 --- a/server/src/rbac/role.rs +++ b/server/src/rbac/role.rs @@ -36,6 +36,7 @@ pub enum Action { DeleteUser, PutUserRoles, GetUserRoles, + GetUserInfo, PutRole, GetRole, DeleteRole, @@ -50,7 +51,7 @@ pub enum Permission { Unit(Action), Stream(Action, String), StreamWithTag(Action, String, Option), - SelfRole, + SelfUser, } // Currently Roles are tied to one stream @@ -96,6 +97,7 @@ impl RoleBuilder { Action::ListUser => Permission::Unit(action), Action::PutUserRoles => Permission::Unit(action), Action::GetUserRoles => Permission::Unit(action), + Action::GetUserInfo => Permission::Unit(action), Action::DeleteUser => Permission::Unit(action), Action::GetAbout => Permission::Unit(action), Action::QueryLLM => Permission::Unit(action), @@ -107,7 +109,7 @@ impl RoleBuilder { }; perms.push(perm); } - perms.push(Permission::SelfRole); + perms.push(Permission::SelfUser); perms } } From 4349b7a93fb476d70bcd18c499f25c406b232428 Mon Sep 17 00:00:00 2001 From: Satyam Singh Date: Wed, 22 Nov 2023 19:55:19 +0530 Subject: [PATCH 4/7] Lint fix --- server/src/handlers/http/oidc.rs | 2 +- server/src/rbac/role.rs | 46 ++++++++++++++++---------------- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/server/src/handlers/http/oidc.rs b/server/src/handlers/http/oidc.rs index 53063f022..a10e11d24 100644 --- a/server/src/handlers/http/oidc.rs +++ b/server/src/handlers/http/oidc.rs @@ -309,7 +309,7 @@ async fn update_user_if_changed( .iter_mut() .find(|x| x.username() == user.username()) { - *entry = user.clone(); + entry.clone_from(&user); put_metadata(&metadata).await?; } diff --git a/server/src/rbac/role.rs b/server/src/rbac/role.rs index 55393e8d9..fc3bd4679 100644 --- a/server/src/rbac/role.rs +++ b/server/src/rbac/role.rs @@ -78,34 +78,34 @@ impl RoleBuilder { let mut perms = Vec::new(); for action in self.actions { let perm = match action { - Action::Ingest => Permission::Stream(action, self.stream.clone().unwrap()), Action::Query => Permission::StreamWithTag( action, self.stream.clone().unwrap(), self.tag.clone(), ), - Action::CreateStream => Permission::Unit(action), - Action::DeleteStream => Permission::Unit(action), - Action::ListStream => Permission::Unit(action), - Action::GetSchema => Permission::Stream(action, self.stream.clone().unwrap()), - Action::GetStats => Permission::Stream(action, self.stream.clone().unwrap()), - Action::GetRetention => Permission::Stream(action, self.stream.clone().unwrap()), - Action::PutRetention => Permission::Stream(action, self.stream.clone().unwrap()), - Action::PutAlert => Permission::Stream(action, self.stream.clone().unwrap()), - Action::GetAlert => Permission::Stream(action, self.stream.clone().unwrap()), - Action::PutUser => Permission::Unit(action), - Action::ListUser => Permission::Unit(action), - Action::PutUserRoles => Permission::Unit(action), - Action::GetUserRoles => Permission::Unit(action), - Action::GetUserInfo => Permission::Unit(action), - Action::DeleteUser => Permission::Unit(action), - Action::GetAbout => Permission::Unit(action), - Action::QueryLLM => Permission::Unit(action), - Action::PutRole => Permission::Unit(action), - Action::GetRole => Permission::Unit(action), - Action::DeleteRole => Permission::Unit(action), - Action::ListRole => Permission::Unit(action), - Action::All => Permission::Stream(action, self.stream.clone().unwrap()), + Action::PutUser + | Action::ListUser + | Action::PutUserRoles + | Action::GetUserRoles + | Action::GetUserInfo + | Action::DeleteUser + | Action::GetAbout + | Action::QueryLLM + | Action::PutRole + | Action::GetRole + | Action::DeleteRole + | Action::ListRole + | Action::CreateStream + | Action::DeleteStream + | Action::ListStream => Permission::Unit(action), + Action::Ingest + | Action::GetSchema + | Action::GetStats + | Action::GetRetention + | Action::PutRetention + | Action::PutAlert + | Action::GetAlert + | Action::All => Permission::Stream(action, self.stream.clone().unwrap()), }; perms.push(perm); } From 2a76aaea6d4632c6ddb27738781a3d6fefa43fa7 Mon Sep 17 00:00:00 2001 From: Satyam Singh Date: Wed, 22 Nov 2023 21:24:37 +0530 Subject: [PATCH 5/7] Fix --- server/src/handlers/http.rs | 8 -------- server/src/handlers/http/rbac.rs | 10 ---------- server/src/rbac/map.rs | 7 +------ server/src/rbac/role.rs | 2 -- 4 files changed, 1 insertion(+), 26 deletions(-) diff --git a/server/src/handlers/http.rs b/server/src/handlers/http.rs index 187a0c102..dfefd2ace 100644 --- a/server/src/handlers/http.rs +++ b/server/src/handlers/http.rs @@ -236,14 +236,6 @@ pub fn configure_routes( .authorize_for_user(Action::GetUserRoles), ), ) - .service( - web::resource("/{username}/info").route( - // GET /user/{username}/info => return user information - web::get() - .to(rbac::get_info) - .authorize_for_user(Action::GetUserInfo), - ), - ) .service( web::resource("/{username}/generate-new-password") // POST /user/{username}/generate-new-password => reset password for this user diff --git a/server/src/handlers/http/rbac.rs b/server/src/handlers/http/rbac.rs index 42da732dd..09fc94fe7 100644 --- a/server/src/handlers/http/rbac.rs +++ b/server/src/handlers/http/rbac.rs @@ -139,16 +139,6 @@ pub async fn get_role(username: web::Path) -> Result) -> Result { - let Some(user) = Users.get_user(&username) else { - return Err(RBACError::UserDoesNotExist); - }; - - Ok(web::Json(user.user_info)) -} - // Handler for DELETE /api/v1/user/delete/{username} pub async fn delete_user(username: web::Path) -> Result { let username = username.into_inner(); diff --git a/server/src/rbac/map.rs b/server/src/rbac/map.rs index c8a5bd1a4..d76c64b59 100644 --- a/server/src/rbac/map.rs +++ b/server/src/rbac/map.rs @@ -218,12 +218,7 @@ impl Sessions { }; (action == required_action || action == Action::All) && ok_stream } - Permission::SelfUser - if matches!( - required_action, - Action::GetUserRoles | Action::GetUserInfo - ) => - { + Permission::SelfUser if required_action == Action::GetUserRoles => { context_user.map(|x| x == username).unwrap_or_default() } _ => false, diff --git a/server/src/rbac/role.rs b/server/src/rbac/role.rs index fc3bd4679..968e58e34 100644 --- a/server/src/rbac/role.rs +++ b/server/src/rbac/role.rs @@ -36,7 +36,6 @@ pub enum Action { DeleteUser, PutUserRoles, GetUserRoles, - GetUserInfo, PutRole, GetRole, DeleteRole, @@ -87,7 +86,6 @@ impl RoleBuilder { | Action::ListUser | Action::PutUserRoles | Action::GetUserRoles - | Action::GetUserInfo | Action::DeleteUser | Action::GetAbout | Action::QueryLLM From 906f728958bcfa3eec8183d98dd16b697ffccd15 Mon Sep 17 00:00:00 2001 From: Satyam Singh Date: Wed, 22 Nov 2023 21:44:36 +0530 Subject: [PATCH 6/7] Fix --- server/src/handlers/http/oidc.rs | 16 +++++++++++----- server/src/rbac/user.rs | 18 ++++++++---------- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/server/src/handlers/http/oidc.rs b/server/src/handlers/http/oidc.rs index a10e11d24..b0cfc1a94 100644 --- a/server/src/handlers/http/oidc.rs +++ b/server/src/handlers/http/oidc.rs @@ -280,8 +280,7 @@ async fn put_user( .find(|user| user.username() == username) .cloned() .unwrap_or_else(|| { - let mut user = User::new_oauth(username.to_owned(), group); - user.user_info = user_info; + let user = User::new_oauth(username.to_owned(), group, user_info); metadata.users.push(user.clone()); user }); @@ -296,14 +295,21 @@ async fn update_user_if_changed( group: HashSet, user_info: user::UserInfo, ) -> Result { + let User { ty, roles } = &mut user; + let UserType::OAuth(oauth_user) = ty else { + unreachable!() + }; + // update user only if roles or userinfo has changed - if user.roles == group && user.user_info == user_info { + if roles == &group && &oauth_user.user_info == &user_info { return Ok(user); } + oauth_user.user_info = user_info; + *roles = group; + let mut metadata = get_metadata().await?; - user.roles = group; - user.user_info = user_info; + if let Some(entry) = metadata .users .iter_mut() diff --git a/server/src/rbac/user.rs b/server/src/rbac/user.rs index 4f055874b..0dc188b1d 100644 --- a/server/src/rbac/user.rs +++ b/server/src/rbac/user.rs @@ -38,7 +38,6 @@ pub enum UserType { pub struct User { #[serde(flatten)] pub ty: UserType, - pub user_info: UserInfo, pub roles: HashSet, } @@ -52,17 +51,18 @@ impl User { username, password_hash: hash, }), - user_info: UserInfo::default(), roles: HashSet::new(), }, password, ) } - pub fn new_oauth(username: String, roles: HashSet) -> Self { + pub fn new_oauth(username: String, roles: HashSet, user_info: UserInfo) -> Self { Self { - ty: UserType::OAuth(OAuth { userid: username }), - user_info: UserInfo::default(), + ty: UserType::OAuth(OAuth { + userid: username, + user_info, + }), roles, } } @@ -72,6 +72,7 @@ impl User { UserType::Native(Basic { ref username, .. }) => username, UserType::OAuth(OAuth { userid: ref username, + .. }) => username, } } @@ -145,14 +146,14 @@ pub fn get_admin_user() -> User { username, password_hash: hashcode, }), - user_info: UserInfo::default(), roles: ["admin".to_string()].into(), } } #[derive(Debug, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] pub struct OAuth { - userid: String, + pub userid: String, + pub user_info: UserInfo, } #[derive(Debug, Default, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)] @@ -169,8 +170,6 @@ pub struct UserInfo { #[serde(default)] pub gender: Option, #[serde(default)] - pub locale: Option, - #[serde(default)] pub updated_at: Option, } @@ -182,7 +181,6 @@ impl From for UserInfo { picture: user.picture, email: user.email, gender: user.gender, - locale: user.locale, updated_at: user.updated_at, } } From ea0ec9859258ff0cf8490c4101c35a56cec8523c Mon Sep 17 00:00:00 2001 From: Satyam Singh Date: Wed, 22 Nov 2023 21:46:01 +0530 Subject: [PATCH 7/7] Fix --- server/src/handlers/http/oidc.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/handlers/http/oidc.rs b/server/src/handlers/http/oidc.rs index b0cfc1a94..a2d277cca 100644 --- a/server/src/handlers/http/oidc.rs +++ b/server/src/handlers/http/oidc.rs @@ -301,7 +301,7 @@ async fn update_user_if_changed( }; // update user only if roles or userinfo has changed - if roles == &group && &oauth_user.user_info == &user_info { + if roles == &group && oauth_user.user_info == user_info { return Ok(user); }