Let's use "distinct" also here to not confuse with rpm's dist tags.

When we merge https://pagure.io/koji/pull-request/3973 , I'll continue with last check.

-    def listUsers(self, userType=koji.USERTYPES['NORMAL'], prefix=None, queryOpts=None):
+    def listUsers(self, userType=[koji.USERTYPES['NORMAL']], prefix=None, perm=None,
+                  queryOpts=None, inherited_perm=True):

You are changing the api in an incompatible way. While we are sometimes forced to do this, we try to avoid it if we possibly can. There are two things here:

• changing the meaning of the userType param. Any previous code that called this with a single value will now fail.
• inserting a new param. Previous code that relied on positional parameters could pass args incorrectly

It's possible the extend the handling of a parameter in a backward compatible way. It generally requires more checking. That said, extending the userType option here is outside the scope of the problem.

When adding new args, they should be optional and appended at the end so that existing invocations still work.

joins = ['LEFT JOIN permissions ON perm_id = permissions.id',
         'LEFT JOIN user_krb_principals ON users.id = user_krb_principals.user_id']

This join is added unconditionally, even when it is not needed. It results in duplicate values, even when there is no perm arg given.

It would be better to match the surrounding code and use joins.append("X") rather than joins = ["X"] + joins.

That said, I'm not sure the query structure here is what we want. The query is getting unwieldy and the need for distinct should probably be considered a warning sign.

It would probably be somewhat cleaner to use subqueries in the clause rather than joins in the query. The same should probably have been done for the krb_principals field.

If this proves too complex we may need a separate call, but for now I certainly see the value in adding this to the main listUsers.

Better to have list(koji.USERTYPES.values()) in case of future user type additions.

It could be rewritten into if perm block. If this test is False, second join is not used at all.

@tkopecek all fixed

@mikem I think that current solution is still quite readable even without subqueries?

cli

The command is written as a general list-users command, but:

• errors if the perm arg is not given
• doesn't include any other selection options from the api call

The handler currently errors if there are no users with the requested perm, but this is not an error. There is nothing incorrect about asking koji which users have a given perm, nor about koji correctly reporting an empty list of matching users.

The cli should probably validate the permission value before passing it to listUsers, or otherwise present a clearer error if the perm does not exist.

hub

The inherited_perm option is not used in the code. The current code appears to always inherit perms (despite this value being false by default).

Query clauses should always use %s. We have some old code that uses %i in places, and koji.db has a workaround for that, but when we add new clauses or update old ones we should make sure to use %s.

If we're going to update fields+aliases this much, we should keep them in a mapping to reduce the chance of a misalignment.

Your query in the perms case needs to also include a user_groups.active condition.

Also, both the user_groups.active condition and the user_perms.active condition need to be join conditions. Otherwise users that have never had a group will be filtered out, even if they had the perm directly.

The call should probably also return a permission_id field

tests

You cli testcases need a tearDown method to call mock.patch.stopall()

The groups approach here overall seems fine though, given that the call is already using grouping, so retracting my suggestion about subqueries.

maybe simplify for testing:

if userType is None:
    userType = list(koji.USERTYPES.values())
elif not isinstance(userType, list):
    userType = [userType]
clauses  = ['usertype IN %(userType)s']

No space here. It is just a visual fix now, but the grouping code does group_opt.split(','), so there will be additional space. As it is then rejoined it doesn't affect the query, but it is a bit "wrong".
Looking to that part, maybe another separate improvement would be to extend grouping option to handle also list, so such code would look like: queryOpts['group'] = ['users.id', 'permissions.id']. Wouldn't it be a bit more safer (@mikem)? Definitely not needed for this PR, just future improvement.

@tkopecek all fixed

There is an if perm three times around. It could be simplified to more readable:

if perm:
   ...
   if inherited_perm:
       ...
   else:
      ...

Also note, that some parts are duplicit (e.g. clauses are identical in both branches)

better to expect int and fail on others:

elif isinstance(userType, int):
    userType = [userType]
else:
    raise koji.ParameterError(...)

Maybe something like "Permission %s does not exist"

Commit eba8de24 fixes this pull-request