From 0245ffcea1eea7ee92f0dfd3ff81bf7f4e631549 Mon Sep 17 00:00:00 2001 From: clayton Date: Fri, 22 Sep 2023 18:46:09 -0700 Subject: [PATCH 01/14] Fix types and cast for group_leader and user_pending --- app/Models/UserGroup.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/Models/UserGroup.php b/app/Models/UserGroup.php index 0cc18fdfc68..ffef8973edb 100644 --- a/app/Models/UserGroup.php +++ b/app/Models/UserGroup.php @@ -8,10 +8,10 @@ /** * @property Group $group * @property int $group_id - * @property int $group_leader + * @property bool $group_leader * @property User $user * @property int $user_id - * @property int $user_pending + * @property bool $user_pending * @property array|null $playmodes */ class UserGroup extends Model @@ -20,6 +20,7 @@ class UserGroup extends Model public $incrementing = false; protected $casts = [ + 'group_leader' => 'boolean', 'user_pending' => 'boolean', ]; protected $primaryKey = ':composite'; From 64b795cf4877cbe8b95cdf678d50e83eaf601292 Mon Sep 17 00:00:00 2001 From: clayton Date: Fri, 22 Sep 2023 18:46:09 -0700 Subject: [PATCH 02/14] Don't return pending groups from `groupIds()` Not used anywhere --- app/Libraries/OsuAuthorize.php | 2 +- app/Models/Forum/Authorize.php | 4 ++-- app/Models/User.php | 26 ++++++++++++-------------- app/Models/UserGroupEvent.php | 2 +- 4 files changed, 16 insertions(+), 18 deletions(-) diff --git a/app/Libraries/OsuAuthorize.php b/app/Libraries/OsuAuthorize.php index 2ed38bad822..57087956e68 100644 --- a/app/Libraries/OsuAuthorize.php +++ b/app/Libraries/OsuAuthorize.php @@ -1363,7 +1363,7 @@ public function checkForumModerate(?User $user, Forum $forum): string return 'ok'; } - if ($forum->moderator_groups !== null && !empty(array_intersect($user->groupIds()['active'], $forum->moderator_groups))) { + if ($forum->moderator_groups !== null && !empty(array_intersect($user->groupIds(), $forum->moderator_groups))) { return 'ok'; } diff --git a/app/Models/Forum/Authorize.php b/app/Models/Forum/Authorize.php index 316aa011957..f4dea498b71 100644 --- a/app/Models/Forum/Authorize.php +++ b/app/Models/Forum/Authorize.php @@ -25,7 +25,7 @@ class Authorize extends Model public static function aclCheck($user, $authOption, $forum) { - $groupIds = $user->groupIds()['active']; + $groupIds = $user->groupIds(); $authOptionId = AuthOption::where('auth_option', $authOption)->value('auth_option_id'); // the group may contain direct acl entry @@ -48,7 +48,7 @@ public static function aclCheck($user, $authOption, $forum) public static function aclGetAllowedForums($user, $authOption) { - $groupIds = $user->groupIds()['active']; + $groupIds = $user->groupIds(); $authOptionId = AuthOption::where('auth_option', $authOption)->value('auth_option_id'); $directAclForumIds = model_pluck(static::directAcl($groupIds, $authOptionId), 'forum_id'); diff --git a/app/Models/User.php b/app/Models/User.php index 9636bc1e2c6..a13d860f0f6 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -444,7 +444,7 @@ public function getUsernameAvailableAt(): Carbon { $playCount = $this->playCount(); - $allGroupIds = array_merge([$this->group_id], $this->groupIds()['active']); + $allGroupIds = array_merge([$this->group_id], $this->groupIds()); $allowedGroupIds = array_map(function ($groupIdentifier) { return app('groups')->byIdentifier($groupIdentifier)->getKey(); }, config('osu.user.allowed_rename_groups')); @@ -1132,24 +1132,22 @@ public function defaultGroup(): Group return $groups->byId($this->group_id) ?? $groups->byIdentifier('default'); } - public function groupIds() + /** + * Get the group IDs of the user's active usergroups. + * + * @return array + */ + public function groupIds(): array { return $this->memoize(__FUNCTION__, function () { - $ret = [ - 'active' => [], - 'pending' => [], - ]; - - foreach ($this->userGroups as $userGroup) { - $key = $userGroup->user_pending ? 'pending' : 'active'; - $ret[$key][] = $userGroup->group_id; - } - - return $ret; + return $this + ->userGroups + ->where('user_pending', false) + ->pluck('group_id') + ->all(); }); } - public function findUserGroup($group, bool $activeOnly): ?UserGroup { $byGroupId = $this->memoize(__FUNCTION__.':byGroupId', fn () => $this->userGroups->keyBy('group_id')); diff --git a/app/Models/UserGroupEvent.php b/app/Models/UserGroupEvent.php index 90ee7aebc68..61a2bc793b1 100644 --- a/app/Models/UserGroupEvent.php +++ b/app/Models/UserGroupEvent.php @@ -171,7 +171,7 @@ public function scopeVisibleForUser(Builder $query, ?User $user): void $query->where('hidden', false); $userGroupIds = priv_check_user($user, 'IsSpecialScope')->can() - ? $user->groupIds()['active'] + ? $user->groupIds() : []; if (!empty($userGroupIds)) { From 4de3fb9e3bd8540b3bc69248b6d7eb09d0a4312a Mon Sep 17 00:00:00 2001 From: clayton Date: Fri, 22 Sep 2023 18:46:10 -0700 Subject: [PATCH 03/14] Use default argument for `findUserGroup()` --- app/Libraries/ClientCheck.php | 2 +- app/Models/User.php | 25 +++++++++++-------- database/factories/UserFactory.php | 2 +- .../InterOp/UserGroupsControllerTest.php | 4 +-- 4 files changed, 18 insertions(+), 15 deletions(-) diff --git a/app/Libraries/ClientCheck.php b/app/Libraries/ClientCheck.php index fd75549f7ad..ae31b5ece0d 100644 --- a/app/Libraries/ClientCheck.php +++ b/app/Libraries/ClientCheck.php @@ -11,7 +11,7 @@ class ClientCheck { public static function findBuild($user, $params): ?Build { - $assertValid = config('osu.client.check_version') && $user->findUserGroup(app('groups')->byIdentifier('admin'), true) === null; + $assertValid = config('osu.client.check_version') && $user->findUserGroup(app('groups')->byIdentifier('admin')) === null; $clientHash = presence(get_string($params['version_hash'] ?? null)); if ($clientHash === null) { diff --git a/app/Models/User.php b/app/Models/User.php index a13d860f0f6..30c95eaa1c5 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -583,7 +583,7 @@ public function addToGroup(Group $group, ?array $playmodes = null, ?self $actor throw new InvariantException('Invalid playmodes: '.implode(', ', $invalidPlaymodes)); } - $activeUserGroup = $this->findUserGroup($group, true); + $activeUserGroup = $this->findUserGroup($group); if ($activeUserGroup === null) { $userGroup = $this @@ -628,7 +628,7 @@ public function addToGroup(Group $group, ?array $playmodes = null, ?self $actor public function removeFromGroup(Group $group, ?self $actor = null): void { - $userGroup = $this->findUserGroup($group, false); + $userGroup = $this->findUserGroup($group, includePending: true); if ($userGroup === null) { return; @@ -653,7 +653,7 @@ public function removeFromGroup(Group $group, ?self $actor = null): void public function setDefaultGroup(Group $group, ?self $actor = null): void { $this->getConnection()->transaction(function () use ($actor, $group) { - if ($this->findUserGroup($group, true) === null) { + if ($this->findUserGroup($group) === null) { $this->addToGroup($group, null, $actor); } @@ -972,7 +972,7 @@ public function inGroupWithPlaymode($groupIdentifier, $playmode = null) return $isGroup; } - $groupModes = $this->findUserGroup($group, true)->actualRulesets(); + $groupModes = $this->findUserGroup($group)->actualRulesets(); return in_array($playmode, $groupModes ?? [], true); } @@ -989,7 +989,7 @@ public function isAdmin() public function isChatAnnouncer() { - return $this->findUserGroup(app('groups')->byIdentifier('announce'), true) !== null; + return $this->findUserGroup(app('groups')->byIdentifier('announce')) !== null; } public function isGMT() @@ -1148,13 +1148,16 @@ public function groupIds(): array }); } - public function findUserGroup($group, bool $activeOnly): ?UserGroup + /** + * Get the user's usergroup corresponding to a specified group. + */ + public function findUserGroup(Group $group, bool $includePending = false): ?UserGroup { $byGroupId = $this->memoize(__FUNCTION__.':byGroupId', fn () => $this->userGroups->keyBy('group_id')); $userGroup = $byGroupId->get($group->getKey()); - if ($userGroup === null || ($activeOnly && $userGroup->user_pending)) { + if ($userGroup === null || (!$includePending && $userGroup->user_pending)) { return null; } @@ -1172,7 +1175,7 @@ public function findUserGroup($group, bool $activeOnly): ?UserGroup */ public function isGroup($group) { - return $this->findUserGroup($group, true) !== null && $this->token() === null; + return $this->findUserGroup($group) !== null && $this->token() === null; } public function badges() @@ -1728,21 +1731,21 @@ public function nominationModes() $modes = []; if ($this->isLimitedBN()) { - $playmodes = $this->findUserGroup(app('groups')->byIdentifier('bng_limited'), true)->actualRulesets(); + $playmodes = $this->findUserGroup(app('groups')->byIdentifier('bng_limited'))->actualRulesets(); foreach ($playmodes as $playmode) { $modes[$playmode] = 'limited'; } } if ($this->isFullBN()) { - $playmodes = $this->findUserGroup(app('groups')->byIdentifier('bng'), true)->actualRulesets(); + $playmodes = $this->findUserGroup(app('groups')->byIdentifier('bng'))->actualRulesets(); foreach ($playmodes as $playmode) { $modes[$playmode] = 'full'; } } if ($this->isNAT()) { - $playmodes = $this->findUserGroup(app('groups')->byIdentifier('nat'), true)->actualRulesets(); + $playmodes = $this->findUserGroup(app('groups')->byIdentifier('nat'))->actualRulesets(); foreach ($playmodes as $playmode) { $modes[$playmode] = 'full'; } diff --git a/database/factories/UserFactory.php b/database/factories/UserFactory.php index ce517ee827f..49591bde133 100644 --- a/database/factories/UserFactory.php +++ b/database/factories/UserFactory.php @@ -129,7 +129,7 @@ public function withGroup(?string $groupIdentifier, ?array $playmodes = null) app('groups')->resetMemoized(); } - $user->findUserGroup($group, true)->update(['playmodes' => $playmodes]); + $user->findUserGroup($group)->update(['playmodes' => $playmodes]); } }); } diff --git a/tests/Controllers/InterOp/UserGroupsControllerTest.php b/tests/Controllers/InterOp/UserGroupsControllerTest.php index add45663392..a8fe056ae72 100644 --- a/tests/Controllers/InterOp/UserGroupsControllerTest.php +++ b/tests/Controllers/InterOp/UserGroupsControllerTest.php @@ -108,7 +108,7 @@ public function testUserGroupChangePlaymodes() $user->refresh(); - $actualPlaymodes = $user->findUserGroup($group, true)->playmodes; + $actualPlaymodes = $user->findUserGroup($group)->playmodes; $this->assertCount(count($playmodes), $actualPlaymodes); $this->assertArraySubset($playmodes, $actualPlaymodes); @@ -226,7 +226,7 @@ public function testUserGroupSetDefaultLeavesPlaymodesUnchanged() $user->refresh(); - $actualPlaymodes = $user->findUserGroup($group, true)->playmodes; + $actualPlaymodes = $user->findUserGroup($group)->playmodes; $this->assertCount(count($playmodes), $actualPlaymodes); $this->assertArraySubset($playmodes, $actualPlaymodes); From 5f579a5b91cad280c0e3cdb8972d7f5c07fb1b29 Mon Sep 17 00:00:00 2001 From: clayton Date: Fri, 22 Sep 2023 18:46:10 -0700 Subject: [PATCH 04/14] Combine `inGroupWithPlaymode()` with `isGroup()` --- app/Models/User.php | 43 +++++++++++++++++++------------------------ 1 file changed, 19 insertions(+), 24 deletions(-) diff --git a/app/Models/User.php b/app/Models/User.php index 30c95eaa1c5..666ab8a918f 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -963,23 +963,9 @@ public function getAttribute($key) | */ - public function inGroupWithPlaymode($groupIdentifier, $playmode = null) - { - $group = app('groups')->byIdentifier($groupIdentifier); - $isGroup = $this->isGroup($group); - - if ($isGroup === false || $playmode === null) { - return $isGroup; - } - - $groupModes = $this->findUserGroup($group)->actualRulesets(); - - return in_array($playmode, $groupModes ?? [], true); - } - public function isNAT($mode = null) { - return $this->inGroupWithPlaymode('nat', $mode); + return $this->isGroup(app('groups')->byIdentifier('nat'), $mode); } public function isAdmin() @@ -1004,12 +990,12 @@ public function isBNG($mode = null) public function isFullBN($mode = null) { - return $this->inGroupWithPlaymode('bng', $mode); + return $this->isGroup(app('groups')->byIdentifier('bng'), $mode); } public function isLimitedBN($mode = null) { - return $this->inGroupWithPlaymode('bng_limited', $mode); + return $this->isGroup(app('groups')->byIdentifier('bng_limited'), $mode); } public function isDev() @@ -1165,17 +1151,26 @@ public function findUserGroup(Group $group, bool $includePending = false): ?User } /** - * Check if a user is in a specific group. + * Check if the user is in a specified group. * - * This will always return false when called on user authenticated using OAuth. + * This will always return false if the user was authenticated using OAuth. * - * @param Group $group - * - * @return bool + * @param \App\Models\Group $group + * @param string|null $ruleset Additionally check if the usergroup has a specified ruleset. */ - public function isGroup($group) + public function isGroup(Group $group, ?string $ruleset = null): bool { - return $this->findUserGroup($group) !== null && $this->token() === null; + if ($this->token() !== null) { + return false; + } + + $userGroup = $this->findUserGroup($group); + + if ($userGroup === null) { + return false; + } + + return $ruleset === null || in_array($ruleset, $userGroup->actualRulesets(), true); } public function badges() From 5915c82eb679501a25e12f5c893adcb37b840152 Mon Sep 17 00:00:00 2001 From: clayton Date: Fri, 22 Sep 2023 18:46:10 -0700 Subject: [PATCH 05/14] Add option to allow checking group with OAuth --- app/Libraries/ClientCheck.php | 2 +- app/Models/User.php | 11 +++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/app/Libraries/ClientCheck.php b/app/Libraries/ClientCheck.php index ae31b5ece0d..56e6a1dd34b 100644 --- a/app/Libraries/ClientCheck.php +++ b/app/Libraries/ClientCheck.php @@ -11,7 +11,7 @@ class ClientCheck { public static function findBuild($user, $params): ?Build { - $assertValid = config('osu.client.check_version') && $user->findUserGroup(app('groups')->byIdentifier('admin')) === null; + $assertValid = config('osu.client.check_version') && !$user->isGroup(app('groups')->byIdentifier('admin'), allowOAuth: true); $clientHash = presence(get_string($params['version_hash'] ?? null)); if ($clientHash === null) { diff --git a/app/Models/User.php b/app/Models/User.php index 666ab8a918f..62430167961 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -653,7 +653,7 @@ public function removeFromGroup(Group $group, ?self $actor = null): void public function setDefaultGroup(Group $group, ?self $actor = null): void { $this->getConnection()->transaction(function () use ($actor, $group) { - if ($this->findUserGroup($group) === null) { + if (!$this->isGroup($group, allowOAuth: true)) { $this->addToGroup($group, null, $actor); } @@ -975,7 +975,7 @@ public function isAdmin() public function isChatAnnouncer() { - return $this->findUserGroup(app('groups')->byIdentifier('announce')) !== null; + return $this->isGroup(app('groups')->byIdentifier('announce'), allowOAuth: true); } public function isGMT() @@ -1153,14 +1153,13 @@ public function findUserGroup(Group $group, bool $includePending = false): ?User /** * Check if the user is in a specified group. * - * This will always return false if the user was authenticated using OAuth. - * * @param \App\Models\Group $group * @param string|null $ruleset Additionally check if the usergroup has a specified ruleset. + * @param bool $allowOAuth Whether to perform the check if the user was authenticated using OAuth. */ - public function isGroup(Group $group, ?string $ruleset = null): bool + public function isGroup(Group $group, ?string $ruleset = null, bool $allowOAuth = false): bool { - if ($this->token() !== null) { + if (!$allowOAuth && $this->token() !== null) { return false; } From 624e55b7e93a241f0197990a3163e16b0fd1a5ab Mon Sep 17 00:00:00 2001 From: clayton Date: Fri, 22 Sep 2023 18:46:10 -0700 Subject: [PATCH 06/14] Add note about mistake in forum moderator permission --- app/Libraries/OsuAuthorize.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/app/Libraries/OsuAuthorize.php b/app/Libraries/OsuAuthorize.php index 57087956e68..20acf445dc9 100644 --- a/app/Libraries/OsuAuthorize.php +++ b/app/Libraries/OsuAuthorize.php @@ -1363,6 +1363,13 @@ public function checkForumModerate(?User $user, Forum $forum): string return 'ok'; } + // TODO: If `$user` is the resource owner, authorizing with a + // third-party OAuth client, `$user->groupIds()` should be empty + // here to match the restrictions in `User::isGroup()`. + // + // Some third-party clients currently rely on this mistake, so an + // alternative method to request usage of group permissions needs + // to be provided before fixing this. if ($forum->moderator_groups !== null && !empty(array_intersect($user->groupIds(), $forum->moderator_groups))) { return 'ok'; } From 253ea950ddba9e2a714c3fbb7bbc9119558f44a2 Mon Sep 17 00:00:00 2001 From: clayton Date: Fri, 22 Sep 2023 18:46:11 -0700 Subject: [PATCH 07/14] Remove unused factory --- database/factories/UserGroupFactory.php | 23 ----------------------- 1 file changed, 23 deletions(-) delete mode 100644 database/factories/UserGroupFactory.php diff --git a/database/factories/UserGroupFactory.php b/database/factories/UserGroupFactory.php deleted file mode 100644 index 3ccd2c541e9..00000000000 --- a/database/factories/UserGroupFactory.php +++ /dev/null @@ -1,23 +0,0 @@ -. Licensed under the GNU Affero General Public License v3.0. -// See the LICENCE file in the repository root for full licence text. - -declare(strict_types=1); - -namespace Database\Factories; - -use App\Models\UserGroup; - -class UserGroupFactory extends Factory -{ - protected $model = UserGroup::class; - - public function definition(): array - { - return [ - 'group_leader' => 0, - 'user_pending' => 0, - ]; - } -} From b93cf883963a41f85cd6f0f9c056cfbb8400808d Mon Sep 17 00:00:00 2001 From: clayton Date: Fri, 22 Sep 2023 18:46:11 -0700 Subject: [PATCH 08/14] Allow specifying group as string in `findUserGroup()` and `isGroup()` --- app/Libraries/ClientCheck.php | 2 +- app/Libraries/OsuAuthorize.php | 2 +- app/Models/User.php | 36 ++++++++++--------- .../InterOp/UsersControllerTest.php | 2 +- 4 files changed, 23 insertions(+), 19 deletions(-) diff --git a/app/Libraries/ClientCheck.php b/app/Libraries/ClientCheck.php index 56e6a1dd34b..41f4a11d132 100644 --- a/app/Libraries/ClientCheck.php +++ b/app/Libraries/ClientCheck.php @@ -11,7 +11,7 @@ class ClientCheck { public static function findBuild($user, $params): ?Build { - $assertValid = config('osu.client.check_version') && !$user->isGroup(app('groups')->byIdentifier('admin'), allowOAuth: true); + $assertValid = config('osu.client.check_version') && !$user->isGroup('admin', allowOAuth: true); $clientHash = presence(get_string($params['version_hash'] ?? null)); if ($clientHash === null) { diff --git a/app/Libraries/OsuAuthorize.php b/app/Libraries/OsuAuthorize.php index 20acf445dc9..bdc9e528fa2 100644 --- a/app/Libraries/OsuAuthorize.php +++ b/app/Libraries/OsuAuthorize.php @@ -1816,7 +1816,7 @@ public function checkLegacyIrcKeyStore(?User $user): string $this->ensureCleanRecord($user); // isBot checks user primary group - if (!$user->isGroup(app('groups')->byIdentifier('bot')) && $user->playCount() < 100) { + if (!$user->isGroup('bot') && $user->playCount() < 100) { return 'play_more'; } diff --git a/app/Models/User.php b/app/Models/User.php index 62430167961..e00690dd98b 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -965,22 +965,22 @@ public function getAttribute($key) public function isNAT($mode = null) { - return $this->isGroup(app('groups')->byIdentifier('nat'), $mode); + return $this->isGroup('nat', $mode); } public function isAdmin() { - return $this->isGroup(app('groups')->byIdentifier('admin')); + return $this->isGroup('admin'); } public function isChatAnnouncer() { - return $this->isGroup(app('groups')->byIdentifier('announce'), allowOAuth: true); + return $this->isGroup('announce', allowOAuth: true); } public function isGMT() { - return $this->isGroup(app('groups')->byIdentifier('gmt')); + return $this->isGroup('gmt'); } public function isBNG($mode = null) @@ -990,17 +990,17 @@ public function isBNG($mode = null) public function isFullBN($mode = null) { - return $this->isGroup(app('groups')->byIdentifier('bng'), $mode); + return $this->isGroup('bng', $mode); } public function isLimitedBN($mode = null) { - return $this->isGroup(app('groups')->byIdentifier('bng_limited'), $mode); + return $this->isGroup('bng_limited', $mode); } public function isDev() { - return $this->isGroup(app('groups')->byIdentifier('dev')); + return $this->isGroup('dev'); } public function isModerator() @@ -1010,17 +1010,17 @@ public function isModerator() public function isAlumni() { - return $this->isGroup(app('groups')->byIdentifier('alumni')); + return $this->isGroup('alumni'); } public function isRegistered() { - return $this->isGroup(app('groups')->byIdentifier('default')); + return $this->isGroup('default'); } public function isProjectLoved() { - return $this->isGroup(app('groups')->byIdentifier('loved')); + return $this->isGroup('loved'); } public function isBot() @@ -1137,10 +1137,14 @@ public function groupIds(): array /** * Get the user's usergroup corresponding to a specified group. */ - public function findUserGroup(Group $group, bool $includePending = false): ?UserGroup + public function findUserGroup(Group|string $group, bool $includePending = false): ?UserGroup { $byGroupId = $this->memoize(__FUNCTION__.':byGroupId', fn () => $this->userGroups->keyBy('group_id')); + if (is_string($group)) { + $group = app('groups')->byIdentifier($group); + } + $userGroup = $byGroupId->get($group->getKey()); if ($userGroup === null || (!$includePending && $userGroup->user_pending)) { @@ -1153,11 +1157,11 @@ public function findUserGroup(Group $group, bool $includePending = false): ?User /** * Check if the user is in a specified group. * - * @param \App\Models\Group $group + * @param \App\Models\Group|string $group * @param string|null $ruleset Additionally check if the usergroup has a specified ruleset. * @param bool $allowOAuth Whether to perform the check if the user was authenticated using OAuth. */ - public function isGroup(Group $group, ?string $ruleset = null, bool $allowOAuth = false): bool + public function isGroup(Group|string $group, ?string $ruleset = null, bool $allowOAuth = false): bool { if (!$allowOAuth && $this->token() !== null) { return false; @@ -1725,21 +1729,21 @@ public function nominationModes() $modes = []; if ($this->isLimitedBN()) { - $playmodes = $this->findUserGroup(app('groups')->byIdentifier('bng_limited'))->actualRulesets(); + $playmodes = $this->findUserGroup('bng_limited')->actualRulesets(); foreach ($playmodes as $playmode) { $modes[$playmode] = 'limited'; } } if ($this->isFullBN()) { - $playmodes = $this->findUserGroup(app('groups')->byIdentifier('bng'))->actualRulesets(); + $playmodes = $this->findUserGroup('bng')->actualRulesets(); foreach ($playmodes as $playmode) { $modes[$playmode] = 'full'; } } if ($this->isNAT()) { - $playmodes = $this->findUserGroup(app('groups')->byIdentifier('nat'))->actualRulesets(); + $playmodes = $this->findUserGroup('nat')->actualRulesets(); foreach ($playmodes as $playmode) { $modes[$playmode] = 'full'; } diff --git a/tests/Controllers/InterOp/UsersControllerTest.php b/tests/Controllers/InterOp/UsersControllerTest.php index a2385cf4051..f6f89f5db70 100644 --- a/tests/Controllers/InterOp/UsersControllerTest.php +++ b/tests/Controllers/InterOp/UsersControllerTest.php @@ -119,7 +119,7 @@ public function testStoreUserCopy() } $this->assertStringContainsString("+{$username}@", $user->user_email); - $this->assertTrue($user->isGroup(app('groups')->byIdentifier($group))); + $this->assertTrue($user->isGroup($group)); } public function testStoreUserCopyMissingUsername() From 3eb99fc3660380c676c9b1f6fcc4992d925b4204 Mon Sep 17 00:00:00 2001 From: clayton Date: Fri, 22 Sep 2023 18:46:11 -0700 Subject: [PATCH 09/14] Use `isGroup()` directly It's just a style difference at this point. This also makes it more obvious at call site if special OAuth behavior will occur --- app/Http/Controllers/Admin/Controller.php | 2 +- app/Http/Controllers/ArtistsController.php | 4 +- app/Http/Controllers/ContestsController.php | 4 +- .../Forum/ForumCoversController.php | 2 +- app/Http/Controllers/StoreController.php | 2 +- app/Libraries/OsuAuthorize.php | 14 +-- app/Models/Beatmapset.php | 10 +-- app/Models/Chat/Channel.php | 2 +- app/Models/Forum/TopicCover.php | 2 +- app/Models/User.php | 87 +++++-------------- app/Transformers/UserCompactTransformer.php | 10 +-- resources/views/beatmapsets/show.blade.php | 2 +- resources/views/forum/forums/show.blade.php | 2 +- .../views/store/orders/_details.blade.php | 2 +- .../views/users/_restricted_banner.blade.php | 2 +- 15 files changed, 50 insertions(+), 97 deletions(-) diff --git a/app/Http/Controllers/Admin/Controller.php b/app/Http/Controllers/Admin/Controller.php index d96487f9a4a..bf27ceb1981 100644 --- a/app/Http/Controllers/Admin/Controller.php +++ b/app/Http/Controllers/Admin/Controller.php @@ -15,7 +15,7 @@ public function __construct() $this->middleware('auth'); $this->middleware(function ($request, $next) { - if (Auth::check() && !Auth::user()->isAdmin()) { + if (Auth::check() && !Auth::user()->isGroup('admin')) { abort(403); } diff --git a/app/Http/Controllers/ArtistsController.php b/app/Http/Controllers/ArtistsController.php index fb5407f115e..147e56622b2 100644 --- a/app/Http/Controllers/ArtistsController.php +++ b/app/Http/Controllers/ArtistsController.php @@ -18,7 +18,7 @@ public function index() $artists = Artist::with('label')->withMax('tracks', 'created_at')->withCount('tracks')->orderBy('name', 'asc'); $user = Auth::user(); - if ($user === null || !$user->isAdmin()) { + if ($user === null || !$user->isGroup('admin')) { $artists->where('visible', true); } @@ -32,7 +32,7 @@ public function show($id) $artist = Artist::with('label')->findOrFail($id); $user = Auth::user(); - if (!$artist->visible && ($user === null || !$user->isAdmin())) { + if (!$artist->visible && ($user === null || !$user->isGroup('admin'))) { abort(404); } diff --git a/app/Http/Controllers/ContestsController.php b/app/Http/Controllers/ContestsController.php index 3ed5d131fd4..d04074d7655 100644 --- a/app/Http/Controllers/ContestsController.php +++ b/app/Http/Controllers/ContestsController.php @@ -15,7 +15,7 @@ public function index() { $contests = Contest::orderBy('id', 'desc'); - if (!Auth::check() || !Auth::user()->isAdmin()) { + if (!Auth::check() || !Auth::user()->isGroup('admin')) { $contests->where('visible', true); } @@ -29,7 +29,7 @@ public function show($id) $contest = Contest::findOrFail($id); $user = Auth::user(); - if (!$contest->visible && (!$user || !$user->isAdmin())) { + if (!$contest->visible && (!$user || !$user->isGroup('admin'))) { abort(404); } diff --git a/app/Http/Controllers/Forum/ForumCoversController.php b/app/Http/Controllers/Forum/ForumCoversController.php index e94cb7b995e..a9fb6c07127 100644 --- a/app/Http/Controllers/Forum/ForumCoversController.php +++ b/app/Http/Controllers/Forum/ForumCoversController.php @@ -25,7 +25,7 @@ public function __construct() ]]); $this->middleware(function ($request, $next) { - if (Auth::check() && !Auth::user()->isAdmin()) { + if (Auth::check() && !Auth::user()->isGroup('admin')) { abort(403); } diff --git a/app/Http/Controllers/StoreController.php b/app/Http/Controllers/StoreController.php index 4248f8bd226..df12089c3a8 100644 --- a/app/Http/Controllers/StoreController.php +++ b/app/Http/Controllers/StoreController.php @@ -47,7 +47,7 @@ public function getInvoice($id = null) ->with('items.product') ->findOrFail($id); - if (Auth::user()->user_id !== $order->user_id && !Auth::user()->isAdmin()) { + if (Auth::user()->user_id !== $order->user_id && !Auth::user()->isGroup('admin')) { abort(403); } diff --git a/app/Libraries/OsuAuthorize.php b/app/Libraries/OsuAuthorize.php index bdc9e528fa2..95a5e67d3c8 100644 --- a/app/Libraries/OsuAuthorize.php +++ b/app/Libraries/OsuAuthorize.php @@ -82,7 +82,7 @@ public function doCheckUser(?User $user, string $ability, object $object = null) $auth = $authMap->get($cacheKey, null); if ($auth === null) { - if ($user !== null && $user->isAdmin() && !static::alwaysCheck($ability)) { + if ($user !== null && $user->isGroup('admin') && !static::alwaysCheck($ability)) { $message = 'ok'; } else { $function = "check{$ability}"; @@ -141,7 +141,7 @@ public function checkBeatmapUpdateOwner(?User $user, ?Beatmapset $beatmapset): s Beatmapset::STATES['graveyard'], Beatmapset::STATES['loved'], ]; - if ($user->isProjectLoved() && in_array($status, $lovedModifiable, true)) { + if ($user->isGroup('loved') && in_array($status, $lovedModifiable, true)) { return 'ok'; } @@ -628,7 +628,7 @@ public function checkBeatmapsetLove(?User $user): string { $this->ensureLoggedIn($user); - if (!$user->isProjectLoved()) { + if (!$user->isGroup('loved')) { return 'unauthorized'; } @@ -648,7 +648,7 @@ public function checkBeatmapsetNominate(?User $user, Beatmapset $beatmapset): st static $prefix = 'beatmap_discussion.nominate.'; - if (!$user->isBNG() && !$user->isNAT()) { + if (!$user->isBNG() && !$user->isGroup('nat')) { return 'unauthorized'; } @@ -770,7 +770,7 @@ public function checkBeatmapsetDisqualify(?User $user, Beatmapset $beatmapset): { $this->ensureLoggedIn($user); - if (!$user->isFullBN() && !$user->isModerator()) { + if (!$user->isGroup('bng') && !$user->isModerator()) { return 'unauthorized'; } @@ -841,7 +841,7 @@ public function checkBeatmapsetMetadataEdit(?User $user, Beatmapset $beatmapset) Beatmapset::STATES['graveyard'], Beatmapset::STATES['loved'], ]; - if ($user->isProjectLoved() && in_array($beatmapset->approved, $lovedEditable, true)) { + if ($user->isGroup('loved') && in_array($beatmapset->approved, $lovedEditable, true)) { return 'ok'; } @@ -912,7 +912,7 @@ public function checkChatAnnounce(?User $user): string $this->ensureLoggedIn($user); $this->ensureCleanRecord($user, $prefix); - if ($user->isModerator() || $user->isChatAnnouncer()) { + if ($user->isModerator() || $user->isGroup('announce', allowOAuth: true)) { return 'ok'; } diff --git a/app/Models/Beatmapset.php b/app/Models/Beatmapset.php index 3bd7f67f44f..b38e8f87fa4 100644 --- a/app/Models/Beatmapset.php +++ b/app/Models/Beatmapset.php @@ -724,10 +724,10 @@ public function nominate(User $user, array $playmodes = []) $canNominate = false; $canFullNominate = false; foreach ($this->playmodesStr() as $mode) { - if ($user->isFullBN($mode) || $user->isNAT($mode)) { + if ($user->isGroup('bng', $mode) || $user->isGroup('nat', $mode)) { $canNominate = true; $canFullNominate = true; - } else if ($user->isLimitedBN($mode)) { + } else if ($user->isGroup('bng_limited', $mode)) { $canNominate = true; } } @@ -747,8 +747,8 @@ public function nominate(User $user, array $playmodes = []) } foreach ($playmodes as $mode) { - if (!$user->isFullBN($mode) && !$user->isNAT($mode)) { - if (!$user->isLimitedBN($mode)) { + if (!$user->isGroup('bng', $mode) && !$user->isGroup('nat', $mode)) { + if (!$user->isGroup('bng_limited', $mode)) { throw new InvariantException(osu_trans('beatmapsets.nominate.incorrect_mode', ['mode' => $mode])); } @@ -1270,7 +1270,7 @@ public function hasFullBNNomination($mode = null) ->get() ->pluck('user') ->contains(function ($user) use ($mode) { - return $user->isNAT($mode) || $user->isFullBN($mode); + return $user->isGroup('nat', $mode) || $user->isGroup('bng', $mode); }); } diff --git a/app/Models/Chat/Channel.php b/app/Models/Chat/Channel.php index adab21408ee..580c5999e33 100644 --- a/app/Models/Chat/Channel.php +++ b/app/Models/Chat/Channel.php @@ -241,7 +241,7 @@ public function isVisibleFor(User $user): bool return !( $targetUser === null || $user->hasBlocked($targetUser) - && !($targetUser->isBot() || $targetUser->isModerator() || $targetUser->isAdmin()) + && !($targetUser->isBot() || $targetUser->isModerator() || $targetUser->isGroup('admin')) ); } diff --git a/app/Models/Forum/TopicCover.php b/app/Models/Forum/TopicCover.php index 199a70ce311..27e41195c10 100644 --- a/app/Models/Forum/TopicCover.php +++ b/app/Models/Forum/TopicCover.php @@ -52,7 +52,7 @@ public static function findForUse($id, $user) $covers = static::select(); - if ($user->isAdmin() === false) { + if ($user->isGroup('admin') === false) { $covers->where('user_id', $user->user_id); } diff --git a/app/Models/User.php b/app/Models/User.php index e00690dd98b..0a5e8ca3ed0 100644 --- a/app/Models/User.php +++ b/app/Models/User.php @@ -963,69 +963,19 @@ public function getAttribute($key) | */ - public function isNAT($mode = null) + public function isBNG(): bool { - return $this->isGroup('nat', $mode); + return $this->isGroup('bng') || $this->isGroup('bng_limited'); } - public function isAdmin() + public function isModerator(): bool { - return $this->isGroup('admin'); + return $this->isGroup('gmt') || $this->isGroup('nat'); } - public function isChatAnnouncer() + public function isBot(): bool { - return $this->isGroup('announce', allowOAuth: true); - } - - public function isGMT() - { - return $this->isGroup('gmt'); - } - - public function isBNG($mode = null) - { - return $this->isFullBN($mode) || $this->isLimitedBN($mode); - } - - public function isFullBN($mode = null) - { - return $this->isGroup('bng', $mode); - } - - public function isLimitedBN($mode = null) - { - return $this->isGroup('bng_limited', $mode); - } - - public function isDev() - { - return $this->isGroup('dev'); - } - - public function isModerator() - { - return $this->isGMT() || $this->isNAT(); - } - - public function isAlumni() - { - return $this->isGroup('alumni'); - } - - public function isRegistered() - { - return $this->isGroup('default'); - } - - public function isProjectLoved() - { - return $this->isGroup('loved'); - } - - public function isBot() - { - return $this->group_id === app('groups')->byIdentifier('bot')->getKey(); + return $this->defaultGroup()->identifier === 'bot'; } public function hasSupported() @@ -1059,14 +1009,17 @@ public function isOnline() && $this->user_lastvisit > Carbon::now()->subMinutes(config('osu.user.online_window')); } - public function isPrivileged() + public function isPrivileged(): bool { - return $this->isAdmin() - || $this->isDev() - || $this->isGMT() - || $this->isBNG() - || $this->isNAT() - || $this->isProjectLoved(); + static $groups = ['admin', 'bng', 'bng_limited', 'dev', 'gmt', 'loved', 'nat']; + + foreach ($groups as $group) { + if ($this->isGroup($group)) { + return true; + } + } + + return false; } public function isBanned() @@ -1722,27 +1675,27 @@ public function userGroupsForBadges() public function nominationModes() { return $this->memoize(__FUNCTION__, function () { - if (!$this->isNAT() && !$this->isBNG()) { + if (!$this->isGroup('nat') && !$this->isBNG()) { return; } $modes = []; - if ($this->isLimitedBN()) { + if ($this->isGroup('bng_limited')) { $playmodes = $this->findUserGroup('bng_limited')->actualRulesets(); foreach ($playmodes as $playmode) { $modes[$playmode] = 'limited'; } } - if ($this->isFullBN()) { + if ($this->isGroup('bng')) { $playmodes = $this->findUserGroup('bng')->actualRulesets(); foreach ($playmodes as $playmode) { $modes[$playmode] = 'full'; } } - if ($this->isNAT()) { + if ($this->isGroup('nat')) { $playmodes = $this->findUserGroup('nat')->actualRulesets(); foreach ($playmodes as $playmode) { $modes[$playmode] = 'full'; diff --git a/app/Transformers/UserCompactTransformer.php b/app/Transformers/UserCompactTransformer.php index fbbbf529141..1edcb93aa46 100644 --- a/app/Transformers/UserCompactTransformer.php +++ b/app/Transformers/UserCompactTransformer.php @@ -243,7 +243,7 @@ public function includeGuestBeatmapsetCount(User $user) public function includeIsAdmin(User $user) { - return $this->primitive($user->isAdmin()); + return $this->primitive($user->isGroup('admin')); } public function includeIsBng(User $user) @@ -253,17 +253,17 @@ public function includeIsBng(User $user) public function includeIsFullBn(User $user) { - return $this->primitive($user->isFullBN()); + return $this->primitive($user->isGroup('bng')); } public function includeIsGmt(User $user) { - return $this->primitive($user->isGMT()); + return $this->primitive($user->isGroup('gmt')); } public function includeIsLimitedBn(User $user) { - return $this->primitive($user->isLimitedBN()); + return $this->primitive($user->isGroup('bng_limited')); } public function includeIsModerator(User $user) @@ -273,7 +273,7 @@ public function includeIsModerator(User $user) public function includeIsNat(User $user) { - return $this->primitive($user->isNAT()); + return $this->primitive($user->isGroup('nat')); } public function includeIsRestricted(User $user) diff --git a/resources/views/beatmapsets/show.blade.php b/resources/views/beatmapsets/show.blade.php index 14e3036f4b0..d46beb26ea1 100644 --- a/resources/views/beatmapsets/show.blade.php +++ b/resources/views/beatmapsets/show.blade.php @@ -2,7 +2,7 @@ Copyright (c) ppy Pty Ltd . Licensed under the GNU Affero General Public License v3.0. See the LICENCE file in the repository root for full licence text. --}} -@if (optional(Auth::user())->isAdmin()) +@if (optional(Auth::user())->isGroup('admin')) @php $extraFooterLinks = [ osu_trans('common.buttons.admin') => route('admin.beatmapsets.show', $beatmapset->getKey()), diff --git a/resources/views/forum/forums/show.blade.php b/resources/views/forum/forums/show.blade.php index 0c621e36a0f..523b7300bab 100644 --- a/resources/views/forum/forums/show.blade.php +++ b/resources/views/forum/forums/show.blade.php @@ -132,7 +132,7 @@ class="simple-menu simple-menu--forum-list js-click-menu" @endif - @if (auth()->check() && auth()->user()->isAdmin()) + @if (auth()->check() && auth()->user()->isGroup('admin'))