Misc. UserGroup clean-up #10585
Draft
Misc. UserGroup clean-up #10585
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Not used anywhere
It's just a style difference at this point. This also makes it more obvious at call site if special OAuth behavior will occur
nanaya
requested changes
Oct 3, 2023
notbakaneko
requested changes
Oct 18, 2023
| */ | ||
| public function isGroup($group) | ||
| public function isGroup(Group|string $group, ?string $ruleset = null, bool $allowOAuth = false): bool |
There was a problem hiding this comment.
isGroup has always been for permission checks; existence in group and has permission of group checks should not share the same method.
| /** | ||
| * Get the user's usergroup corresponding to a specified group. | ||
| */ | ||
| public function findUserGroup(Group|string $group, bool $includePending = false): ?UserGroup |
There was a problem hiding this comment.
I'm not keen on this type of param type overloading and having conditionals in the body to convert it when the specific type is known before hand; this isn't a parsing function.
It also means findUserGroup implicitly picks up the auto group creating behaviour of Groups::byIdentifier as an unexpected side-effect.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
applying some of what I was thinking while working around usergroups / group permissions / group history / etc for a while. I thought this would be good to get out of the way before (eventually) looking into expanding group permissions over the API for some project loved things.
imo the main problem with a lot of group checking code is that it factors in the current request's oauth token, so then these methods are only really valid when called on the current user, and as the developer you have to be mindful of which methods concern authorization/permissions/etc and which just get the data they suggest. while not solved in this PR, I consolidated the various kinds of group checks so that in most cases it will be clear when things are intentionally bypassing the contained oauth check
some other time I will look into getting that oauth token check out of the User model. but this was already a rabbit hole so I'm just PRing what I did so far