koji

Clone

#3912 anonymous getGroupMembers and getUserGroups
Merged 5 months ago by tkopecek. Opened a year ago by cobrien.
cobrien/koji anon-group-api  into  master

file modified
+21 -1 
@@ -9553,7 +9553,7 @@ 
 

  

  def get_group_members(group):
 

      """Get the members of a group"""
 

-     context.session.assertPerm('admin')
 

+ 

      ginfo = get_user(group)
 

      if not ginfo or ginfo['usertype'] != koji.USERTYPES['GROUP']:
 

          raise koji.GenericError("No such group: %s" % group)
 
@@ -13409,6 +13409,26 @@ 
 

      dropGroupMember = staticmethod(drop_group_member)
 

      getGroupMembers = staticmethod(get_group_members)
 

  

+     def getUserGroups(self, user):
 

+         """
 

+         The groups associated with the given user
 

+ 

+         :param user: a str (Kerberos principal or name) or an int (user id)
 

+                      or a dict:
 

+                          - id: User's ID
 

+                          - name: User's name
 

+                          - krb_principal: Kerberos principal
 

+ 

+         :returns: a list of dicts, each containing the id and name of
 

+                   a group
 

+ 

+         :raises: GenericError if the specified user is not found
 

+         """
 

+ 

+         uinfo = get_user(user, strict=True)
 

+         return [{'id': key, 'name': val} for key, val in
 

+                 get_user_groups(uinfo["id"]).items()]
 

+ 

      def listUsers(self, userType=koji.USERTYPES['NORMAL'], prefix=None, queryOpts=None, perm=None,
 

                    inherited_perm=False):
 

          """List users in the system

file modified
+3 
@@ -104,6 +104,9 @@ 
 

      def setUp(self):
 

          self.get_user = mock.patch('kojihub.kojihub.get_user').start()
 

  

+     def tearDown(self):
 

+         mock.patch.stopall()
 

+ 

      def test_wrong_type_krb_principal(self):
 

          krb_principal = ['test-user']
 

          with self.assertRaises(koji.GenericError) as cm:

file added
+52 
@@ -0,0 +1,52 @@ 
 

+ import mock
 

+ 

+ import koji
 

+ import kojihub
 

+ from .utils import DBQueryTestCase
 

+ 

+ 

+ class TestGetUserGroups(DBQueryTestCase):
 

+ 

+     def setUp(self):
 

+         super(TestGetUserGroups, self).setUp()
 

+         self.exports = kojihub.RootExports()
 

+ 

+         self.context = mock.patch('kojihub.kojihub.context').start()
 

+         self.context_db = mock.patch('kojihub.db.context').start()
 

+ 

+     def tearDown(self):
 

+         mock.patch.stopall()
 

+ 

+     def test_no_such_user(self):
 

+         user = 'test-user'
 

+         self.qp_execute_return_value = []
 

+         with self.assertRaises(koji.GenericError) as cm:
 

+             self.exports.getUserGroups(user)
 

+         self.assertEqual(f"No such user: {user!r}", str(cm.exception))
 

+         self.assertEqual(len(self.queries), 1)
 

+ 

+     def test_valid(self):
 

+         get_user = mock.patch('kojihub.kojihub.get_user').start()
 

+         get_user.return_value = {'id': 23, 'usertype': 0}
 

+ 

+         user = 'test-user'
 

+         self.qp_execute_return_value = [{'group_id': 123, 'name': 'grp_123'},
 

+                                         {'group_id': 456, 'name': 'grp_456'}]
 

+ 

+         grps = self.exports.getUserGroups(user)
 

+ 

+         get_user.assert_called_once_with(user, strict=True)
 

+ 

+         self.assertEqual(len(self.queries), 1)
 

+         query = self.queries[0]
 

+         self.assertEqual(query.tables, ['user_groups'])
 

+         self.assertEqual(query.joins, ['users ON group_id = users.id'])
 

+         self.assertEqual(query.clauses, ['active IS TRUE',
 

+                                          'user_id=%(user_id)i',
 

+                                          'users.usertype=%(t_group)i'])
 

+         self.assertEqual(query.values, {'t_group': 2,
 

+                                         'user_id': 23})
 

+         self.assertEqual(query.columns, ['group_id', 'name'])
 

+ 

+         self.assertEqual(grps, [{'id': 123, 'name': 'grp_123'},
 

+                                 {'id': 456, 'name': 'grp_456'}])

file modified
+4 -4 
@@ -262,11 +262,11 @@ 
 

      def test_get_group_members(self):
 

          group, gid = 'test_group', 1
 

  

-         # no permission
 

-         self.context.session.assertPerm.side_effect = koji.ActionNotAllowed
 

-         with self.assertRaises(koji.ActionNotAllowed):
 

+         # no permission needed, verify that it's not being checked
 

+         self.context.session.assertPerm.side_effect = Exception
 

+         with self.assertRaises(koji.GenericError):
 

              kojihub.get_group_members(group)
 

-         self.context.session.assertPerm.assert_called_with('admin')
 

+         self.context.session.assertPerm.assert_not_called()
 

          self.assertEqual(len(self.inserts), 0)
 

          self.assertEqual(len(self.updates), 0)

Removes admin permission check from getGroupMembers hub API. Adds anonymous getUserGroups hub API. Updates and adds unit tests for both.

Related: https://pagure.io/koji/issue/3900

note: the query for getUserGroups is lifted almost verbatim from the existing kojihub.auth:get_user_groups function. We're simply skipping the step of converting that into a single dictionary.
Edited a year ago by cobrien

rebased onto c65cf43525ffb49806c28ee43f2fa4ce0a2ee8ce
10 months ago

rebased onto 49cdd26cc14aa8406d77074b2fa1b6766e795025
10 months ago

rebased onto c2d619bc117f6c98f3c598d3a0d075beb1d38d39
10 months ago

I've rebased this onto master and that required reworking some tests to mock correctly. I've also reworked this a bit to remove duplication of queries, so now the original get_user_groups is being fed from an iter_user_groups. iter_user_groups is what's used to populate the data from the hub call getUserGroups.

Sorry, can you rebase it once more? I'll put it into test-ready then.

I've got a rebase here, lemme post it

The overall pattern here is fine. A couple things though

It doesn't feel right to use query.iterate here. We're always querying a specific user_id and never expect a large number of rows to be returned. This turns a simple query into four.

DECLARE qp_cursor_139890124317536_2146560_0 NO SCROLL CURSOR FOR 
SELECT group_id, name
  FROM user_groups
  JOIN users ON group_id = users.id
 WHERE (active IS TRUE)
   AND (user_id=1)
   AND (users.usertype=2)

FETCH 1000 FROM qp_cursor_139890124317536_2146560_0

FETCH 1000 FROM qp_cursor_139890124317536_2146560_0

CLOSE qp_cursor_139890124317536_2146560_0

Also, the PR adds tests for the new function in multiple places. Seems like there might be overlap. Perhaps the single test in test_user_groups.py was meant to be dropped in favor of test_get_user_groups.py ?

You're right. I'd started adding the new tests into the existing test file, and then migrated them to their own, and neglected to commit the removal from the original. I'm also probably copying the wrong/outdated method for setting up a QueryProcessor test, since I think that was reworked since I originally hacked on this.

I will poke at this again later today, and see if I can't simplify it in so doing. In particular I think I'm going to just abandon the iterative refactor and make the exposed hub api reveal a staticmethod wrapper on the auth module's existing function.
Edited 6 months ago by cobrien 
uinfo = get_user(user)
if not uinfo or uinfo['usertype'] != koji.USERTYPES['NORMAL']:
    raise koji.GenericError("No such user: %s" % user)

This reports a misleading error for other user types. Better to call get_user with strict=True for basic validation.

I'm not sure if we even need to restrict the call to normal users. Simpler to let the query just report [], which is a true answer. Also it's not 100% clear to me that a host can't have groups. I wouldn't recommend it, but add_group_member doesn't block it.

If we were to check usertype here, probably best only assert that the user is not a group (as add_group_member does).

rebased onto 9544b86
6 months ago

rebased, simplified.

I fought for a bit with the get_user api -- it worked when I ran my tests alone, but failed when running the whole suite. Turns out that the TestGetUserByKrbPrincipal wasn't ever tearing itself down, so I fixed that.

I also identified a problem in the api json while adding my own change, simply that the version had changed and the Retry type had as well.

Thank you very much for your reviews
Edited 6 months ago by cobrien

Turns out that the TestGetUserByKrbPrincipal wasn't ever tearing itself down

Thanks. I thought I'd found all of those. I'll have to search deeper.

I also identified a problem in the api json while adding my own change, simply that the version had changed and the Retry type had as well.

This is actually causing the test to fail for me here. This value is an import, and the check-api does detect that it's external to the code, but still tries to enforce a type check. Unfortunately the type varies depending on the version of requests that is installed.

I'll make a separate PR for the script. In the meantime, probably best to leave the api.json change out of this PR.

filed #4132 for the api test

fyi, previous tearDown cleanup in #4068 and #4082

More tearDown fixes -- #4133

The current version uncovers the reason that you initially wanted to separate out the underlying query. xmlrpc can't handle integer keys.

TypeError: dictionary key must be string

Options:

  • just return the names, e.g. with .values()
  • reconstruct the list of dicts, e.g. dict(zip(('id', 'name'), *gdict.items()))
  • split out the query portion of the function similar to before

1 new commit added

  • change return type to list of dicts
6 months ago

Went with the list of dicts using the id and name keys, since that's a pattern used elsewhere in the API.

1 new commit added

  • undoing api json changes
6 months ago

Set the api.json back to the state from master.

Thanks for the quick fix!

:thumbsup:

Metadata Update from @mikem:
- Pull-request tagged with: testing-ready
6 months ago

Metadata Update from @mfilip:
- Pull-request tagged with: testing-done
5 months ago

Commit 3dd7665 fixes this pull-request

Pull-Request has been merged by tkopecek
5 months ago
Metadata
None

testing-ready testing-done

Changes Summary 4
+21 -1
file changed
kojihub/kojihub.py
+3 -0
file changed
tests/test_hub/test_get_user.py
+52
file added
tests/test_hub/test_get_user_groups.py
+4 -4
file changed
tests/test_hub/test_user_groups.py
Powered by Pagure 5.14.1
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.