Feature/virtual groups support #23
Conversation
There was a problem hiding this comment.
Looking good! Please see the recommendations in the in-line comments.
Also, have a look at the files tab for this PR, which is what I used to review the code. You'll notice that most of the changes are cosmetic spacing changes. They make it a little harder to pick out the substantive changes. I'm betting that Visual Studio applied these automatically. It's not a big deal, but if there is a way to turn that behavior off, that would be helpful in the future (but don't worry about it for now).
| @@ -386,7 +387,8 @@ def authorized(self, perm: Permissions, who: str = None): | |||
| if isinstance(perm, list): | |||
| perm = set(perm) | |||
|
|
|||
| idents = [who] + list(self._cli.user_groups) | |||
| idents = [who] + list(self._cli.all_groups_for(who)) | |||
There was a problem hiding this comment.
Since the user_groups property will now also include the virtual groups (since you updated recache_user_groups() to include them), let's stick with the original line. That way, the people service call will happen only once while processing the current request; with all_groups_for(), the people service will be called each time authorized() is called. (The people database is not updated that often--daily.) If we just go back to consulting user_groups, it looks like we don't need the all_groups_for() function
There was a problem hiding this comment.
The issue with the initial line is that it calls all groups for the user currently authentified (usually the superuser) and not the user 'who'. The new function has been implemented so we can get all groups for a specific user not necessarily the user that is making the request.
There was a problem hiding this comment.
good point! In actuality, if who is None, the currently authenticated is usually the real end-user. Nevertheless, you are correct: the interface allows you to specify who, so the implementation must comply. Good catch!
| def all_groups_for(self, who) -> frozenset: | ||
| """ | ||
| Return the frozen set of all groups a user or group belongs to. | ||
| """ |
There was a problem hiding this comment.
See comment above in authorized(); this function is no longer needed, so we can remove it here.
| out.append(f"divisionNumber:{person['divisionNumber']}") | ||
| if 'groupNumber' in person and person['groupNumber']: | ||
| out.append(f"groupNumber:{person['groupNumber']}") | ||
| return out |
There was a problem hiding this comment.
Let's change the group prefix from ouNumber, divisionNumber, and groupNumber to nistou, nistdiv, nistgrp, respectively.
| except dbio.ObjectNotFound: | ||
| return self.send_error_resp(404, "ID not found", | ||
| "Record with requested identifier not found", | ||
| self._id, ashead=ashead) |
There was a problem hiding this comment.
This try block is repeated below--that is, regardless of path, this code needs to be executed. Let's move this code up above the if not path: and remove the same block below. Yes?
Support for Virtual Groups permissions