fix(admin): allow non-consoleAdmin self password update (#2082)

crates/policy/src/policy/policy.rs

@@ -70,13 +70,15 @@ impl Policy {
             }
         }
 
-        // Owner has all permissions
-        if args.is_owner {
+        // DenyOnly mode: only validate explicit Deny statements.
+        // If no Deny matched above, allow the request.
+        if args.deny_only {
             return true;
         }
 
-        if args.deny_only {
-            return false;
+        // Owner has all permissions
+        if args.is_owner {
+            return true;
         }
 
         // Check Allow statements
@@ -603,7 +605,7 @@ mod test {
     }
 
     #[tokio::test]
-    async fn test_deny_only_security_fix() -> Result<()> {
+    async fn test_deny_only_checks_only_deny_statements() -> Result<()> {
         let data = r#"
 {
   "Version": "2012-10-17",
@@ -621,7 +623,8 @@ mod test {
         let conditions = HashMap::new();
         let claims = HashMap::new();
 
-        // Test with deny_only=true but no matching Allow statement
+        // deny_only=true should allow if no Deny statement matches,
+        // even when no Allow statement matches.
         let args_deny_only = Args {
             account: "testuser",
             groups: &None,
@@ -631,16 +634,15 @@ mod test {
             is_owner: false,
             object: "test.txt",
             claims: &claims,
-            deny_only: true, // Should NOT automatically allow
+            deny_only: true,
         };
 
-        // Should return false because deny_only=true, regardless of whether there's a matching Allow statement
         assert!(
-            !policy.is_allowed(&args_deny_only).await,
-            "deny_only should return false when deny_only=true, regardless of Allow statements"
+            policy.is_allowed(&args_deny_only).await,
+            "deny_only should allow when no Deny statement matches"
         );
 
-        // Test with deny_only=true and matching Allow statement
+        // deny_only=true should also allow when action matches Allow statement.
         let args_deny_only_allowed = Args {
             account: "testuser",
             groups: &None,
@@ -653,13 +655,12 @@ mod test {
             deny_only: true,
         };
 
-        // Should return false because deny_only=true prevents checking Allow statements (unless is_owner=true)
         assert!(
-            !policy.is_allowed(&args_deny_only_allowed).await,
-            "deny_only should return false even with matching Allow statement"
+            policy.is_allowed(&args_deny_only_allowed).await,
+            "deny_only should allow when no Deny statement matches, even if Allow exists"
         );
 
-        // Test with deny_only=false (normal case)
+        // Normal policy evaluation remains unchanged when deny_only=false.
         let args_normal = Args {
             account: "testuser",
             groups: &None,
@@ -678,21 +679,40 @@ mod test {
             "normal policy evaluation should allow with matching Allow statement"
         );
 
+        // Explicit Deny must still block request in deny_only mode.
+        let deny_policy_data = r#"
+{
+  "Version": "2012-10-17",
+  "Statement": [
+    {
+      "Effect": "Deny",
+      "Action": ["s3:PutObject"],
+      "Resource": ["arn:aws:s3:::bucket2/*"]
+    }
+  ]
+}
+"#;
+        let deny_policy = Policy::parse_config(deny_policy_data.as_bytes())?;
+        assert!(
+            !deny_policy.is_allowed(&args_deny_only).await,
+            "deny_only should reject request when an explicit Deny matches"
+        );
+
         let args_owner_deny_only = Args {
             account: "testuser",
             groups: &None,
             action: Action::S3Action(crate::policy::action::S3Action::PutObjectAction),
             bucket: "bucket2",
             conditions: &conditions,
-            is_owner: true, // Owner has all permissions
+            is_owner: true,
             object: "test.txt",
             claims: &claims,
-            deny_only: true, // Even with deny_only=true, owner should be allowed
+            deny_only: true,
         };
 
         assert!(
             policy.is_allowed(&args_owner_deny_only).await,
-            "owner should retain all permissions even when deny_only=true"
+            "deny_only should allow when no Deny statement matches, including owner requests"
         );
 
         Ok(())

rustfs/src/admin/handlers/user.rs

@@ -25,7 +25,7 @@ use crate::{
 use http::{HeaderMap, StatusCode};
 use matchit::Params;
 use rustfs_config::{MAX_ADMIN_REQUEST_BODY_SIZE, MAX_IAM_IMPORT_SIZE};
-use rustfs_credentials::get_global_action_cred;
+use rustfs_credentials::{Credentials, get_global_action_cred};
 use rustfs_iam::{
     store::{GroupInfo, MappedPolicy, UserType},
     sys::NewServiceAccountOpts,
@@ -66,6 +66,13 @@ pub fn register_user_route(r: &mut S3Router<AdminOperation>) -> std::io::Result<
     Ok(())
 }
 
+fn should_check_deny_only(target_access_key: &str, requester: &Credentials) -> bool {
+    target_access_key == requester.access_key
+        && requester.parent_user.is_empty()
+        && !requester.is_temp()
+        && !requester.is_service_account()
+}
+
 pub struct AddUser {}
 #[async_trait::async_trait]
 impl Operation for AddUser {
@@ -134,14 +141,14 @@ impl Operation for AddUser {
             return Err(s3_error!(InvalidArgument, "access key is not utf8"));
         }
 
-        // Security fix: Always require explicit Allow permission for CreateUser
-        // Do not use deny_only to bypass permission checks, even when creating for self
-        // This ensures consistent security semantics and prevents privilege escalation
+        let check_deny_only = should_check_deny_only(ak, &cred);
+
+        // For eligible self operations, only explicit Deny should block the request.
         validate_admin_request(
             &req.headers,
             &cred,
             owner,
-            false, // Always require explicit Allow permission
+            check_deny_only,
             vec![Action::AdminAction(AdminAction::CreateUserAdminAction)],
             req.extensions.get::<Option<RemoteAddr>>().and_then(|opt| opt.map(|a| a.0)),
         )
@@ -393,14 +400,14 @@ impl Operation for GetUserInfo {
         let (cred, owner) =
             check_key_valid(get_session_token(&req.uri, &req.headers).unwrap_or_default(), &input_cred.access_key).await?;
 
-        // Security fix: Always require explicit Allow permission for GetUser
-        // Users should have explicit GetUser permission to view account information
-        // This ensures consistent security semantics across all admin operations
+        let check_deny_only = should_check_deny_only(ak, &cred);
+
+        // For eligible self operations, only explicit Deny should block the request.
         validate_admin_request(
             &req.headers,
             &cred,
             owner,
-            false, // Always require explicit Allow permission
+            check_deny_only,
             vec![Action::AdminAction(AdminAction::GetUserAdminAction)],
             req.extensions.get::<Option<RemoteAddr>>().and_then(|opt| opt.map(|a| a.0)),
         )
@@ -1054,3 +1061,62 @@ impl Operation for ImportIam {
         Ok(S3Response::with_headers((StatusCode::OK, Body::from(body)), header))
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::should_check_deny_only;
+    use rustfs_credentials::{Credentials, IAM_POLICY_CLAIM_NAME_SA};
+    use serde_json::Value;
+    use std::collections::HashMap;
+
+    #[test]
+    fn test_should_check_deny_only_for_regular_self_request() {
+        let cred = Credentials {
+            access_key: "alice".to_string(),
+            ..Default::default()
+        };
+        assert!(should_check_deny_only("alice", &cred));
+    }
+
+    #[test]
+    fn test_should_not_check_deny_only_for_other_user_request() {
+        let cred = Credentials {
+            access_key: "bob".to_string(),
+            ..Default::default()
+        };
+        assert!(!should_check_deny_only("alice", &cred));
+    }
+
+    #[test]
+    fn test_should_not_check_deny_only_for_temp_credentials() {
+        let cred = Credentials {
+            access_key: "alice".to_string(),
+            session_token: "temp-token".to_string(),
+            ..Default::default()
+        };
+        assert!(!should_check_deny_only("alice", &cred));
+    }
+
+    #[test]
+    fn test_should_not_check_deny_only_for_service_account_credentials() {
+        let mut claims = HashMap::new();
+        claims.insert(IAM_POLICY_CLAIM_NAME_SA.to_string(), Value::String("policy".to_string()));
+        let cred = Credentials {
+            access_key: "alice".to_string(),
+            parent_user: "parent-user".to_string(),
+            claims: Some(claims),
+            ..Default::default()
+        };
+        assert!(!should_check_deny_only("alice", &cred));
+    }
+
+    #[test]
+    fn test_should_not_check_deny_only_when_parent_user_present() {
+        let cred = Credentials {
+            access_key: "alice".to_string(),
+            parent_user: "parent-user".to_string(),
+            ..Default::default()
+        };
+        assert!(!should_check_deny_only("alice", &cred));
+    }
+}