MinioAdmin: allow specifying policies as dict besides file

[Alveel]

It would be nice to be able to add/update IAM/service account policies through Python dict objects as well as what's currently available through JSON files.

This PR tries to accomplish this.

Feedback/questions welcome.

Depends on #1483

[darix]

i wonder if it makes sense to refactor out something like

def _load_policy(policy_file, policy):
        if policy and policy_file:
            raise ValueError(
                "only one of policy or policy_file may be specified")
        if not policy or not policy_file:
            raise ValueError("either policy or policy_file must be specified")
        body = ''
        if policy:
            body = json.dumps(policy)
        elif policy_file:
            with open(policy_file, encoding='utf-8') as file:
                body = file.read()
       return body

given in how many places we basically have the same code now.

[Alveel]

Sounds fairly sensible actually. I'll wait for feedback from a member and see what they think.

[Alveel]

@balamurugana please review

[Alveel]

I'd love to hear some feedback on the current state.

[harshavardhana]

I'd love to hear some feedback on the current state.

Please fix the CI tests

[Alveel]

Fixed. Moved the .open() for Path to its own line so I can put the mypy comment on the same line while keeping it within 80 characters to keep autopep8 happy.

[balamurugana]

Below is the exact diff, I am referring

diff --git a/minio/minioadmin.py b/minio/minioadmin.py
index d62ac21..0fd2979 100644
--- a/minio/minioadmin.py
+++ b/minio/minioadmin.py
@@ -452,16 +452,24 @@ class MinioAdmin:
         response = self._url_open("GET", _COMMAND.LIST_GROUPS)
         return response.data.decode()
 
-    def policy_add(self, policy_name: str, policy_file: str) -> str:
+    def policy_add(self,
+                   policy_name: str,
+                   policy_file: str | None = None,
+                   policy: bytes | None = None) -> str:
         """Add new policy."""
-        with open(policy_file, encoding='utf-8') as file:
-            response = self._url_open(
-                "PUT",
-                _COMMAND.ADD_CANNED_POLICY,
-                query_params={"name": policy_name},
-                body=file.read().encode(),
-            )
-            return response.data.decode()
+        if not (policy_file is not None) ^ (policy is not None):
+            raise ValueError("either policy_file or policy must be provided")
+        body = policy
+        if policy_file:
+            with open(policy_file, encoding='utf-8') as file:
+                body = file.read().encode()
+        response = self._url_open(
+            "PUT",
+            _COMMAND.ADD_CANNED_POLICY,
+            query_params={"name": policy_name},
+            body=body,
+        )
+        return response.data.decode()
 
     def policy_remove(self, policy_name: str) -> str:
         """Remove policy."""
@@ -754,6 +762,7 @@ class MinioAdmin:
                             name: str | None = None,
                             description: str | None = None,
                             policy_file: str | None = None,
+                            policy: dict | None = None,
                             expiration: str | None = None,
                             status: str | None = None) -> str:
         """
@@ -763,7 +772,9 @@ class MinioAdmin:
             raise ValueError("both access key and secret key must be provided")
         if access_key == "" or secret_key == "":
             raise ValueError("access key or secret key must not be empty")
-        data = {
+        if policy_file is not None and policy is not None:
+            raise ValueError("either policy_file or policy must be provided")
+        data: dict[str, Any] = {
             "status": "enabled",
             "accessKey": access_key,
             "secretKey": secret_key,
@@ -775,6 +786,8 @@ class MinioAdmin:
         if policy_file:
             with open(policy_file, encoding="utf-8") as file:
                 data["policy"] = json.load(file)
+        if policy:
+            data["policy"] = policy
         if expiration:
             data["expiration"] = expiration
         if status:
@@ -798,15 +811,19 @@ class MinioAdmin:
                                name: str | None = None,
                                description: str | None = None,
                                policy_file: str | None = None,
+                               policy: dict | None = None,
                                expiration: str | None = None,
                                status: str | None = None) -> str:
         """Update an existing service account"""
-        args = [secret_key, name, description, policy_file, expiration, status]
+        args = [secret_key, name, description,
+                policy_file, policy, expiration, status]
         if not any(arg for arg in args):
             raise ValueError("at least one of secret_key, name, description, "
                              "policy_file, expiration or status must be "
                              "specified")
-        data = {}
+        if policy_file is not None and policy is not None:
+            raise ValueError("either policy_file or policy must be provided")
+        data: dict[str, Any] = {}
         if secret_key:
             data["newSecretKey"] = secret_key
         if name:
@@ -816,6 +833,8 @@ class MinioAdmin:
         if policy_file:
             with open(policy_file, encoding="utf-8") as file:
                 data["newPolicy"] = json.load(file)
+        if policy:
+            data["newPolicy"] = policy
         if expiration:
             data["newExpiration"] = expiration
         if status:

[Alveel]

I think that's it? I still see "1 requested change" but I don't see where or what it is.

Do you want me to squash my commits?

[Alveel]

@balamurugana can you take a look? 🙂

[Alveel]

Apologies, that was a dumb error I introduced and insufficiently tested. Tested properly now and passing all tests.

[Alveel]

Squashed commits, should be all clean now. Can you please take a look?

If needed, I still have the original commits in a separate branch locally.

[balamurugana]

Depends on #1483