Repository: duosecurity/duo_client_python

Issue:
group pagination not part of the code
Hi,

In `duo_client/admin.py`, there are `get_user_groups` and `get_user_groups_iterator` which (via `json_paging_api_call` in `duo_client/client.py`) seem to be doing the pagination for us (kinda already touched on in #63).

Minor note: per https://duo.com/docs/adminapi#retrieve-groups-by-user-id, `get_user_groups` should have limits of 100/500, but the API shows None.  I don't particularly care here - you're doing the paging for me, so, "cool!"

What I'm opening an issue over is, `get_group_users` looks the same as `get_user_groups` from the docs https://duo.com/docs/adminapi#v2-groups-get-users.  ("Same" as in, both API calls take optional paging-related parameters, with limits of 100/500 defined; obviously they return different data.)  But there's no similar iterator/listmaker offered for `get_group_users` - you get back one limit-sized slurp of data.  I'm not seeing the metadata get to me from a call to `get_group_users` in order to manage my own pagination, but that could be an error on my part.

If it's an oversight that `get_group_users` doesn't page, well, here's a request.  If it's deliberate, is there something you can maybe drop in a comment for why this call is seemingly unique/different?

Thanks!

Discussion:

Patch:
diff --git a/duo_client/admin.py b/duo_client/admin.py
index 48bb1d6..c482019 100644
--- a/duo_client/admin.py
+++ b/duo_client/admin.py
@@ -2001,7 +2001,7 @@ class Admin(client.Client):
 
         return self.json_api_call('GET', url + group_id, {})
 
-    def get_group_users(self, group_id, limit=100, offset=0):
+    def get_group_users(self, group_id, limit=None, offset=0):
         """
         Get a paginated list of users associated with the specified
         group.
@@ -2010,13 +2010,28 @@ class Admin(client.Client):
         limit - The maximum number of records to return. Maximum is 500. (Optional)
         offset - The offset of the first record to return. (Optional)
         """
-        return self.json_api_call(
+        (limit, offset) = self.normalize_paging_args(limit, offset)
+        if limit:
+            return self.json_api_call(
+                'GET',
+                '/admin/v2/groups/' + group_id + '/users',
+                {
+                    'limit': limit,
+                    'offset': offset,
+                })
+        return list(self.get_group_users_iterator(group_id))
+
+    def get_group_users_iterator(self, group_id):
+        """
+        Returns an iterator of users associated with the specified group.
+
+        group_id - The id of the group (Required)
+        """
+        return self.json_paging_api_call(
             'GET',
             '/admin/v2/groups/' + group_id + '/users',
-            {
-                'limit': str(limit),
-                'offset': str(offset),
-            })
+            {}
+        )
 
     def create_group(self, name,
                     desc=None,


Test patch:
diff --git a/tests/admin/test_groups.py b/tests/admin/test_groups.py
index 6bbed7f..7e516e1 100644
--- a/tests/admin/test_groups.py
+++ b/tests/admin/test_groups.py
@@ -119,7 +119,7 @@ class TestGroups(TestAdmin):
     def test_get_group_users(self):
         """ Test for getting list of users associated with a group
         """
-        response = self.client.get_group_users('ABC123')
+        response = self.client_list.get_group_users('ABC123')[0]
         uri, args = response['uri'].split('?')
 
         self.assertEqual(response['method'], 'GET')
@@ -132,6 +132,69 @@ class TestGroups(TestAdmin):
                 'offset': ['0'],
             })
 
+    def test_get_group_users_with_offset(self):
+        """Test to get users by group id with pagination params
+        """
+        response = self.client_list.get_group_users('ABC123', offset=30)[0]
+        uri, args = response['uri'].split('?')
+
+        self.assertEqual(response['method'], 'GET')
+        self.assertEqual(uri, '/admin/v2/groups/ABC123/users')
+        self.assertEqual(util.params_to_dict(args),
+                         {
+                            'account_id':[self.client.account_id],
+                            'limit': ['100'],
+                            'offset': ['0'],
+                         })
+
+    def test_get_group_users_with_limit(self):
+        """Test to get users by group id with pagination params
+        """
+        response = self.client_list.get_group_users('ABC123', limit=30)[0]
+        uri, args = response['uri'].split('?')
+
+        self.assertEqual(response['method'], 'GET')
+        self.assertEqual(uri, '/admin/v2/groups/ABC123/users')
+        self.assertEqual(util.params_to_dict(args),
+                         {
+                            'account_id':[self.client.account_id],
+                            'limit': ['30'],
+                            'offset': ['0'],
+                         })
+
+    def test_get_group_users_with_limit_and_offset(self):
+        """Test to get users by group id with pagination params
+        """
+        response = self.client_list.get_group_users(
+            'ABC123', limit=30, offset=60)[0]
+        uri, args = response['uri'].split('?')
+
+        self.assertEqual(response['method'], 'GET')
+        self.assertEqual(uri, '/admin/v2/groups/ABC123/users')
+        self.assertEqual(util.params_to_dict(args),
+                         {
+                            'account_id':[self.client.account_id],
+                            'limit': ['30'],
+                            'offset': ['60'],
+                         })
+
+    def test_get_group_users_iterator(self):
+        """Test to get user iterator by group id
+        """
+        iterator = self.client_list.get_group_users_iterator(
+            'ABC123')
+        response = next(iterator)
+        uri, args = response['uri'].split('?')
+
+        self.assertEqual(response['method'], 'GET')
+        self.assertEqual(uri, '/admin/v2/groups/ABC123/users')
+        self.assertEqual(util.params_to_dict(args),
+                         {
+                            'account_id':[self.client.account_id],
+                            'limit': ['100'],
+                            'offset': ['0'],
+                         })
+
     def test_delete_group(self):
         """ Test for deleting a group
         """