server: Update permission checks for strict_readonly

server/src/infra/auth_service.rs

@@ -430,7 +430,7 @@ async fn opaque_register_start<Backend>(
     data: web::Data<AppState<Backend>>,
 ) -> TcpResult<registration::ServerRegistrationStartResponse>
 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 = &registration_start_request.username;
+    let user_id = UserId::new(&registration_start_request.username);
-    if !validation_result.can_write(user_id) {
+    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<Backend>(
     data: web::Data<AppState<Backend>>,
 ) -> ApiResult<registration::ServerRegistrationStartResponse>
 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 {
+    pub fn can_read(&self, user: &UserId) -> bool {
-        self.permission == Permission::Admin || self.user == user
+        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<Backend>(
     }
     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

server/src/infra/graphql/mutation.rs

@@ -121,14 +121,15 @@ impl<Handler: BackendHandler + Sync> Mutation<Handler> {
         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<Handler: BackendHandler + Sync> Mutation<Handler> {
             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<Handler: BackendHandler + Sync> Mutation<Handler> {
         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<Handler: BackendHandler + Sync> Mutation<Handler> {
         }
         context
             .handler
-            .delete_user(&UserId::new(&user_id))
+            .delete_user(&user_id)
             .instrument(span)
             .await?;
         Ok(Success::new())

server/src/infra/graphql/query.rs

@@ -113,13 +113,14 @@ impl<Handler: BackendHandler + Sync> Query<Handler> {
         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)?)

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<Backend: BackendHandler + LoginHandler + OpaqueHandler> {
-    user_info: Option<(UserId, Permission)>,
+    user_info: Option<ValidationResults>,
     backend_handler: Backend,
     pub base_dn: Vec<(String, String)>,
     base_dn_str: String,
@@ -509,16 +509,18 @@ impl<Backend: BackendHandler + LoginHandler + OpaqueHandler> LdapHandler<Backend
                         .map(|groups| groups.iter().any(|g| g.display_name == name))
                         .unwrap_or(false)
                 };
-                self.user_info = Some((
+                self.user_info = Some(ValidationResults {
-                    user_id,
+                    user: user_id,
-                    if is_in_group("lldap_admin") {
+                    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
                     },
-                ));
+                });
                 debug!("Success!");
                 (LdapResultCode::Success, "".to_string())
             }
@@ -553,7 +555,7 @@ impl<Backend: BackendHandler + LoginHandler + OpaqueHandler> LdapHandler<Backend
         &mut self,
         request: &LdapPasswordModifyRequest,
     ) -> Vec<LdapOp> {
-        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<Backend: BackendHandler + LoginHandler + OpaqueHandler> LdapHandler<Backend
                                 )]
                             }
                         };
-                        if !(*permission == Permission::Admin
+                        if !credentials.can_change_password(&uid, user_is_admin) {
-                            || user_id == &uid
-                            || (*permission == Permission::Readonly && !user_is_admin))
-                        {
                             return vec![make_extended_response(
                                 LdapResultCode::InsufficentAccessRights,
                                 format!(
                                     r#"User `{}` cannot modify the password of user `{}`"#,
-                                    &user_id, &uid
+                                    &credentials.user, &uid
                                 ),
                             )];
                         }
@@ -626,15 +625,14 @@ impl<Backend: BackendHandler + LoginHandler + OpaqueHandler> LdapHandler<Backend
     }
 
     pub async fn do_search_or_dse(&mut self, request: &LdapSearchRequest) -> Vec<LdapOp> {
-        let user_filter = match self.user_info.clone() {
+        let user_info = match &self.user_info {
-            Some((_, Permission::Admin)) | Some((_, Permission::Readonly)) => None,
-            Some((user_id, Permission::Regular)) => Some(user_id),
             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<Backend: BackendHandler + LoginHandler + OpaqueHandler> LdapHandler<Backend
             debug!("rootDSE request");
             return vec![root_dse_response(&self.base_dn_str), make_search_success()];
         }
+        let user_filter = if user_info.is_admin_or_readonly() {
+            None
+        } else {
+            Some(user_info.user.clone())
+        };
         self.do_search(request, user_filter).await
     }
 
@@ -1135,7 +1138,13 @@ mod tests {
     async fn setup_bound_readonly_handler(
         mock: MockTestBackendHandler,
     ) -> LdapHandler<MockTestBackendHandler> {
-        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<MockTestBackendHandler> {
+        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 {