From cf19fd41b02c15c9d7fcc69d5431f1e966b3bd87 Mon Sep 17 00:00:00 2001 From: Valentin Tolmer Date: Fri, 8 Jul 2022 18:30:15 +0200 Subject: [PATCH] server: Update permission checks for strict_readonly --- server/src/infra/auth_service.rs | 53 ++++++++++++------- server/src/infra/graphql/mutation.rs | 11 ++-- server/src/infra/graphql/query.rs | 3 +- server/src/infra/ldap_handler.rs | 76 ++++++++++++++++++++-------- 4 files changed, 100 insertions(+), 43 deletions(-) diff --git a/server/src/infra/auth_service.rs b/server/src/infra/auth_service.rs index e96814e..a732215 100644 --- a/server/src/infra/auth_service.rs +++ b/server/src/infra/auth_service.rs @@ -430,7 +430,7 @@ async fn opaque_register_start( data: web::Data>, ) -> TcpResult where - Backend: OpaqueHandler + 'static, + Backend: BackendHandler + OpaqueHandler + 'static, { use actix_web::FromRequest; let validation_result = BearerAuth::from_request(&request, &mut payload.0) @@ -448,8 +448,14 @@ where .await .map_err(|e| TcpError::BadRequest(format!("{:#?}", e)))? .into_inner(); - let user_id = ®istration_start_request.username; - if !validation_result.can_write(user_id) { + let user_id = UserId::new(®istration_start_request.username); + let user_is_admin = data + .backend_handler + .get_user_groups(&user_id) + .await? + .iter() + .any(|g| g.display_name == "lldap_admin"); + if !validation_result.can_change_password(&user_id, user_is_admin) { return Err(TcpError::UnauthorizedError( "Not authorized to change the user's password".to_string(), )); @@ -466,7 +472,7 @@ async fn opaque_register_start_handler( data: web::Data>, ) -> ApiResult where - Backend: OpaqueHandler + 'static, + Backend: BackendHandler + OpaqueHandler + 'static, { opaque_register_start(request, payload, data) .await @@ -559,13 +565,14 @@ where #[derive(Clone, Copy, PartialEq, Debug)] pub enum Permission { Admin, + PasswordManager, Readonly, Regular, } -#[derive(Debug)] +#[derive(Debug, Clone, PartialEq)] pub struct ValidationResults { - pub user: String, + pub user: UserId, pub permission: Permission, } @@ -573,7 +580,7 @@ impl ValidationResults { #[cfg(test)] pub fn admin() -> Self { Self { - user: "admin".to_string(), + user: UserId::new("admin"), permission: Permission::Admin, } } @@ -585,19 +592,29 @@ impl ValidationResults { #[must_use] pub fn is_admin_or_readonly(&self) -> bool { - self.permission == Permission::Admin || self.permission == Permission::Readonly - } - - #[must_use] - pub fn can_read(&self, user: &str) -> bool { self.permission == Permission::Admin || self.permission == Permission::Readonly - || self.user == user + || self.permission == Permission::PasswordManager } #[must_use] - pub fn can_write(&self, user: &str) -> bool { - self.permission == Permission::Admin || self.user == user + pub fn can_read(&self, user: &UserId) -> bool { + self.permission == Permission::Admin + || self.permission == Permission::PasswordManager + || self.permission == Permission::Readonly + || &self.user == user + } + + #[must_use] + pub fn can_change_password(&self, user: &UserId, user_is_admin: bool) -> bool { + self.permission == Permission::Admin + || (self.permission == Permission::PasswordManager && !user_is_admin) + || &self.user == user + } + + #[must_use] + pub fn can_write(&self, user: &UserId) -> bool { + self.permission == Permission::Admin || &self.user == user } } @@ -627,10 +644,12 @@ pub(crate) fn check_if_token_is_valid( } let is_in_group = |name| token.claims().groups.contains(name); Ok(ValidationResults { - user: token.claims().user.clone(), + user: UserId::new(&token.claims().user), permission: if is_in_group("lldap_admin") { Permission::Admin - } else if is_in_group("lldap_readonly") { + } else if is_in_group("lldap_password_manager") { + Permission::PasswordManager + } else if is_in_group("lldap_strict_readonly") { Permission::Readonly } else { Permission::Regular diff --git a/server/src/infra/graphql/mutation.rs b/server/src/infra/graphql/mutation.rs index 13b2a10..743cd96 100644 --- a/server/src/infra/graphql/mutation.rs +++ b/server/src/infra/graphql/mutation.rs @@ -121,14 +121,15 @@ impl Mutation { span.in_scope(|| { debug!(?user.id); }); - if !context.validation_result.can_write(&user.id) { + let user_id = UserId::new(&user.id); + if !context.validation_result.can_write(&user_id) { span.in_scope(|| debug!("Unauthorized")); return Err("Unauthorized user update".into()); } context .handler .update_user(UpdateUserRequest { - user_id: UserId::new(&user.id), + user_id, email: user.email, display_name: user.display_name, first_name: user.first_name, @@ -200,13 +201,14 @@ impl Mutation { span.in_scope(|| debug!("Unauthorized")); return Err("Unauthorized group membership modification".into()); } + let user_id = UserId::new(&user_id); if context.validation_result.user == user_id && group_id == 1 { span.in_scope(|| debug!("Cannot remove admin rights for current user")); return Err("Cannot remove admin rights for current user".into()); } context .handler - .remove_user_from_group(&UserId::new(&user_id), GroupId(group_id)) + .remove_user_from_group(&user_id, GroupId(group_id)) .instrument(span) .await?; Ok(Success::new()) @@ -217,6 +219,7 @@ impl Mutation { span.in_scope(|| { debug!(?user_id); }); + let user_id = UserId::new(&user_id); if !context.validation_result.is_admin() { span.in_scope(|| debug!("Unauthorized")); return Err("Unauthorized user deletion".into()); @@ -227,7 +230,7 @@ impl Mutation { } context .handler - .delete_user(&UserId::new(&user_id)) + .delete_user(&user_id) .instrument(span) .await?; Ok(Success::new()) diff --git a/server/src/infra/graphql/query.rs b/server/src/infra/graphql/query.rs index 799cc35..19d0b65 100644 --- a/server/src/infra/graphql/query.rs +++ b/server/src/infra/graphql/query.rs @@ -113,13 +113,14 @@ impl Query { span.in_scope(|| { debug!(?user_id); }); + let user_id = UserId::new(&user_id); if !context.validation_result.can_read(&user_id) { span.in_scope(|| debug!("Unauthorized")); return Err("Unauthorized access to user data".into()); } Ok(context .handler - .get_user_details(&UserId::new(&user_id)) + .get_user_details(&user_id) .instrument(span) .await .map(Into::into)?) diff --git a/server/src/infra/ldap_handler.rs b/server/src/infra/ldap_handler.rs index e61e154..b80b0bf 100644 --- a/server/src/infra/ldap_handler.rs +++ b/server/src/infra/ldap_handler.rs @@ -6,7 +6,7 @@ use crate::{ }, opaque_handler::OpaqueHandler, }, - infra::auth_service::Permission, + infra::auth_service::{Permission, ValidationResults}, }; use anyhow::{bail, Context, Result}; use itertools::Itertools; @@ -450,7 +450,7 @@ fn root_dse_response(base_dn: &str) -> LdapOp { } pub struct LdapHandler { - user_info: Option<(UserId, Permission)>, + user_info: Option, backend_handler: Backend, pub base_dn: Vec<(String, String)>, base_dn_str: String, @@ -509,16 +509,18 @@ impl LdapHandler LdapHandler Vec { - let (user_id, permission) = match &self.user_info { + let credentials = match &self.user_info { Some(info) => info, _ => { return vec![make_search_error( @@ -578,15 +580,12 @@ impl LdapHandler LdapHandler Vec { - let user_filter = match self.user_info.clone() { - Some((_, Permission::Admin)) | Some((_, Permission::Readonly)) => None, - Some((user_id, Permission::Regular)) => Some(user_id), + let user_info = match &self.user_info { None => { return vec![make_search_error( LdapResultCode::InsufficentAccessRights, "No user currently bound".to_string(), - )]; + )] } + Some(u) => u, }; if request.base.is_empty() && request.scope == LdapSearchScope::Base @@ -643,6 +641,11 @@ impl LdapHandler LdapHandler { - setup_bound_handler_with_group(mock, "lldap_readonly").await + setup_bound_handler_with_group(mock, "lldap_strict_readonly").await + } + + async fn setup_bound_password_manager_handler( + mock: MockTestBackendHandler, + ) -> LdapHandler { + setup_bound_handler_with_group(mock, "lldap_password_manager").await } async fn setup_bound_admin_handler( @@ -2312,7 +2321,7 @@ mod tests { } #[tokio::test] - async fn test_password_change_readonly() { + async fn test_password_change_password_manager() { let mut mock = MockTestBackendHandler::new(); mock.expect_get_user_groups() .with(eq(UserId::new("bob"))) @@ -2340,7 +2349,7 @@ mod tests { mock.expect_registration_finish() .times(1) .return_once(|_| Ok(())); - let mut ldap_handler = setup_bound_readonly_handler(mock).await; + let mut ldap_handler = setup_bound_password_manager_handler(mock).await; let request = LdapOp::ExtendedRequest( LdapPasswordModifyRequest { user_identity: Some("uid=bob,ou=people,dc=example,dc=com".to_string()), @@ -2409,7 +2418,7 @@ mod tests { } #[tokio::test] - async fn test_password_change_unauthorized_readonly() { + async fn test_password_change_unauthorized_password_manager() { let mut mock = MockTestBackendHandler::new(); let mut groups = HashSet::new(); groups.insert(GroupDetails { @@ -2422,6 +2431,31 @@ mod tests { .with(eq(UserId::new("bob"))) .times(1) .return_once(|_| Ok(groups)); + let mut ldap_handler = setup_bound_password_manager_handler(mock).await; + let request = LdapOp::ExtendedRequest( + LdapPasswordModifyRequest { + user_identity: Some("uid=bob,ou=people,dc=example,dc=com".to_string()), + old_password: Some("pass".to_string()), + new_password: Some("password".to_string()), + } + .into(), + ); + assert_eq!( + ldap_handler.handle_ldap_message(request).await, + Some(vec![make_extended_response( + LdapResultCode::InsufficentAccessRights, + "User `test` cannot modify the password of user `bob`".to_string(), + )]) + ); + } + + #[tokio::test] + async fn test_password_change_unauthorized_readonly() { + let mut mock = MockTestBackendHandler::new(); + mock.expect_get_user_groups() + .with(eq(UserId::new("bob"))) + .times(1) + .return_once(|_| Ok(HashSet::new())); let mut ldap_handler = setup_bound_readonly_handler(mock).await; let request = LdapOp::ExtendedRequest( LdapPasswordModifyRequest {