diff --git a/app/Http/Controllers/Multiplayer/Rooms/Playlist/ScoresController.php b/app/Http/Controllers/Multiplayer/Rooms/Playlist/ScoresController.php index ea1f7321d1f..58ea39c74df 100644 --- a/app/Http/Controllers/Multiplayer/Rooms/Playlist/ScoresController.php +++ b/app/Http/Controllers/Multiplayer/Rooms/Playlist/ScoresController.php @@ -5,7 +5,6 @@ namespace App\Http\Controllers\Multiplayer\Rooms\Playlist; -use App\Exceptions\InvariantException; use App\Http\Controllers\Controller as BaseController; use App\Libraries\ClientCheck; use App\Models\Multiplayer\PlaylistItem; @@ -182,15 +181,10 @@ public function store($roomId, $playlistId) $playlistItem = $room->playlist()->findOrFail($playlistId); $user = \Auth::user(); $request = \Request::instance(); - $params = $request->all(); - - if (get_string($params['beatmap_hash'] ?? null) !== $playlistItem->beatmap->checksum) { - throw new InvariantException(osu_trans('score_tokens.create.beatmap_hash_invalid')); - } $buildId = ClientCheck::parseToken($request)['buildId']; - $scoreToken = $room->startPlay($user, $playlistItem, $buildId); + $scoreToken = $room->startPlay($user, $playlistItem, $buildId, $request->all()); return json_item($scoreToken, new ScoreTokenTransformer()); } diff --git a/app/Http/Controllers/ScoreTokensController.php b/app/Http/Controllers/ScoreTokensController.php index 06a392e7358..7a2cfdf56e7 100644 --- a/app/Http/Controllers/ScoreTokensController.php +++ b/app/Http/Controllers/ScoreTokensController.php @@ -34,27 +34,14 @@ public function store($beatmapId) 'ruleset_id:int', ]); - $checks = [ - 'beatmap_hash' => fn (string $value): bool => $value === $beatmap->checksum, - 'ruleset_id' => fn (int $value): bool => Beatmap::modeStr($value) !== null && $beatmap->canBeConvertedTo($value), - ]; - foreach ($checks as $key => $testFn) { - if (!isset($params[$key])) { - throw new InvariantException("missing {$key}"); - } - if (!$testFn($params[$key])) { - throw new InvariantException("invalid {$key}"); - } - } - $buildId = ClientCheck::parseToken($request)['buildId']; try { $scoreToken = ScoreToken::create([ 'beatmap_id' => $beatmap->getKey(), 'build_id' => $buildId, - 'ruleset_id' => $params['ruleset_id'], 'user_id' => $user->getKey(), + ...$params, ]); } catch (PDOException $e) { // TODO: move this to be a validation inside Score model diff --git a/app/Models/Multiplayer/Room.php b/app/Models/Multiplayer/Room.php index c50ed5d63e3..29f8aad998a 100644 --- a/app/Models/Multiplayer/Room.php +++ b/app/Models/Multiplayer/Room.php @@ -658,13 +658,19 @@ public function endGame(User $requestingUser) $this->save(); } - public function startPlay(User $user, PlaylistItem $playlistItem, int $buildId) + public function startPlay(User $user, PlaylistItem $playlistItem, int $buildId, array $rawParams): ScoreToken { priv_check_user($user, 'MultiplayerScoreSubmit', $this)->ensureCan(); + $params = [ + 'beatmap_hash' => get_string($rawParams['beatmap_hash'] ?? null), + 'beatmap_id' => $playlistItem->beatmap_id, + 'ruleset_id' => $playlistItem->ruleset_id, + ]; + $this->assertValidStartPlay($user, $playlistItem); - return $this->getConnection()->transaction(function () use ($buildId, $user, $playlistItem) { + return $this->getConnection()->transaction(function () use ($buildId, $user, $params, $playlistItem) { $agg = UserScoreAggregate::new($user, $this); if ($agg->wasRecentlyCreated) { $this->incrementInstance('participant_count'); @@ -676,11 +682,10 @@ public function startPlay(User $user, PlaylistItem $playlistItem, int $buildId) $playlistItemAgg->updateUserAttempts(); return ScoreToken::create([ - 'beatmap_id' => $playlistItem->beatmap_id, 'build_id' => $buildId, 'playlist_item_id' => $playlistItem->getKey(), - 'ruleset_id' => $playlistItem->ruleset_id, 'user_id' => $user->getKey(), + ...$params, ]); }); } diff --git a/app/Models/ScoreToken.php b/app/Models/ScoreToken.php index f75c4bb07c5..5427c55b505 100644 --- a/app/Models/ScoreToken.php +++ b/app/Models/ScoreToken.php @@ -5,6 +5,7 @@ namespace App\Models; +use App\Exceptions\InvariantException; use App\Models\Multiplayer\PlaylistItem; use App\Models\Solo\Score; use Illuminate\Database\Eloquent\Relations\BelongsTo; @@ -25,6 +26,8 @@ */ class ScoreToken extends Model { + public ?string $beatmapHash = null; + public function beatmap() { return $this->belongsTo(Beatmap::class, 'beatmap_id'); @@ -74,4 +77,34 @@ public function getAttribute($key) 'user' => $this->getRelationValue($key), }; } + + public function setBeatmapHashAttribute(?string $value): void + { + $this->beatmapHash = $value; + } + + public function assertValid(): void + { + $beatmap = $this->beatmap; + if ($this->beatmapHash !== $beatmap->checksum) { + throw new InvariantException(osu_trans('score_tokens.create.beatmap_hash_invalid')); + } + + $rulesetId = $this->ruleset_id; + if ($rulesetId === null) { + throw new InvariantException('missing ruleset_id'); + } + if (Beatmap::modeStr($rulesetId) === null || !$beatmap->canBeConvertedTo($rulesetId)) { + throw new InvariantException('invalid ruleset_id'); + } + } + + public function save(array $options = []): bool + { + if (!$this->exists) { + $this->assertValid(); + } + + return parent::save($options); + } } diff --git a/database/factories/ScoreTokenFactory.php b/database/factories/ScoreTokenFactory.php index aa7f4950d14..7c7b85f727a 100644 --- a/database/factories/ScoreTokenFactory.php +++ b/database/factories/ScoreTokenFactory.php @@ -23,7 +23,8 @@ public function definition(): array 'build_id' => Build::factory(), 'user_id' => User::factory(), - // depends on beatmap_id + // depend on beatmap_id + 'beatmap_hash' => fn (array $attr) => Beatmap::find($attr['beatmap_id'])->checksum, 'ruleset_id' => fn (array $attr) => Beatmap::find($attr['beatmap_id'])->playmode, ]; } diff --git a/tests/Controllers/Multiplayer/Rooms/Playlist/ScoresControllerTest.php b/tests/Controllers/Multiplayer/Rooms/Playlist/ScoresControllerTest.php index 2477e8d1518..bdbe0d69f72 100644 --- a/tests/Controllers/Multiplayer/Rooms/Playlist/ScoresControllerTest.php +++ b/tests/Controllers/Multiplayer/Rooms/Playlist/ScoresControllerTest.php @@ -169,7 +169,7 @@ public function testUpdate($bodyParams, $status) $playlistItem = PlaylistItem::factory()->create(); $room = $playlistItem->room; $build = Build::factory()->create(['allow_ranking' => true]); - $scoreToken = $room->startPlay($user, $playlistItem, 0); + $scoreToken = static::roomStartPlay($user, $playlistItem); $this->withHeaders(['x-token' => static::createClientToken($build)]); diff --git a/tests/Controllers/ScoreTokensControllerTest.php b/tests/Controllers/ScoreTokensControllerTest.php index e75d971fee0..1dc2c84a305 100644 --- a/tests/Controllers/ScoreTokensControllerTest.php +++ b/tests/Controllers/ScoreTokensControllerTest.php @@ -45,7 +45,7 @@ public function testStore(string $beatmapState, int $status): void /** * @dataProvider dataProviderForTestStoreInvalidParameter */ - public function testStoreInvalidParameter(string $paramKey, ?string $paramValue, int $status): void + public function testStoreInvalidParameter(string $paramKey, ?string $paramValue, int $status, string $errorMessage): void { $origClientCheckVersion = $GLOBALS['cfg']['osu']['client']['check_version']; config_set('osu.client.check_version', true); @@ -78,14 +78,6 @@ public function testStoreInvalidParameter(string $paramKey, ?string $paramValue, $this->expectCountChange(fn () => ScoreToken::count(), 0); - $errorMessage = $paramValue === null ? 'missing' : 'invalid'; - $errorMessage .= ' '; - $errorMessage .= $paramKey === 'client_token' - ? ($paramValue === null - ? 'token header' - : 'client hash' - ) : $paramKey; - $this->json( 'POST', route('api.beatmaps.solo.score-tokens.store', $routeParams), @@ -161,14 +153,14 @@ public static function dataProviderForTestStore(): array public static function dataProviderForTestStoreInvalidParameter(): array { return [ - 'invalid client token' => ['client_token', md5('invalid_'), 422], - 'missing client token' => ['client_token', null, 422], + 'invalid client token' => ['client_token', md5('invalid_'), 422, 'invalid client hash'], + 'missing client token' => ['client_token', null, 422, 'missing token header'], - 'invalid ruleset id' => ['ruleset_id', '5', 422], - 'missing ruleset id' => ['ruleset_id', null, 422], + 'invalid ruleset id' => ['ruleset_id', '5', 422, 'invalid ruleset_id'], + 'missing ruleset id' => ['ruleset_id', null, 422, 'missing ruleset_id'], - 'invalid beatmap hash' => ['beatmap_hash', 'xxx', 422], - 'missing beatmap hash' => ['beatmap_hash', null, 422], + 'invalid beatmap hash' => ['beatmap_hash', 'xxx', 422, 'invalid or missing beatmap_hash'], + 'missing beatmap hash' => ['beatmap_hash', null, 422, 'invalid or missing beatmap_hash'], ]; } diff --git a/tests/Models/Multiplayer/RoomTest.php b/tests/Models/Multiplayer/RoomTest.php index 3a1f8307417..ca8a951478b 100644 --- a/tests/Models/Multiplayer/RoomTest.php +++ b/tests/Models/Multiplayer/RoomTest.php @@ -124,7 +124,7 @@ public function testRoomHasEnded() ]); $this->expectException(InvariantException::class); - $room->startPlay($user, $playlistItem, 0); + static::roomStartPlay($user, $playlistItem); } public function testStartPlay(): void @@ -137,7 +137,7 @@ public function testStartPlay(): void $this->expectCountChange(fn () => $room->userHighScores()->count(), 1); $this->expectCountChange(fn () => $playlistItem->scoreTokens()->count(), 1); - $room->startPlay($user, $playlistItem, 0); + static::roomStartPlay($user, $playlistItem); $room->refresh(); $this->assertSame($user->getKey(), $playlistItem->scoreTokens()->last()->user_id); @@ -150,14 +150,14 @@ public function testMaxAttemptsReached() $playlistItem1 = PlaylistItem::factory()->create(['room_id' => $room]); $playlistItem2 = PlaylistItem::factory()->create(['room_id' => $room]); - $room->startPlay($user, $playlistItem1, 0); + static::roomStartPlay($user, $playlistItem1); $this->assertTrue(true); - $room->startPlay($user, $playlistItem2, 0); + static::roomStartPlay($user, $playlistItem2); $this->assertTrue(true); $this->expectException(InvariantException::class); - $room->startPlay($user, $playlistItem1, 0); + static::roomStartPlay($user, $playlistItem1); } public function testMaxAttemptsForItemReached() @@ -174,19 +174,19 @@ public function testMaxAttemptsForItemReached() ]); $initialCount = $playlistItem1->scoreTokens()->count(); - $room->startPlay($user, $playlistItem1, 0); + static::roomStartPlay($user, $playlistItem1); $this->assertSame($initialCount + 1, $playlistItem1->scoreTokens()->count()); $initialCount = $playlistItem1->scoreTokens()->count(); try { - $room->startPlay($user, $playlistItem1, 0); + static::roomStartPlay($user, $playlistItem1); } catch (Exception $ex) { $this->assertTrue($ex instanceof InvariantException); } $this->assertSame($initialCount, $playlistItem1->scoreTokens()->count()); $initialCount = $playlistItem2->scoreTokens()->count(); - $room->startPlay($user, $playlistItem2, 0); + static::roomStartPlay($user, $playlistItem2); $this->assertSame($initialCount + 1, $playlistItem2->scoreTokens()->count()); } diff --git a/tests/Models/Multiplayer/UserScoreAggregateTest.php b/tests/Models/Multiplayer/UserScoreAggregateTest.php index 710e5d52399..8ab32f33a75 100644 --- a/tests/Models/Multiplayer/UserScoreAggregateTest.php +++ b/tests/Models/Multiplayer/UserScoreAggregateTest.php @@ -173,7 +173,7 @@ public function testStartingPlayIncreasesAttempts(): void $user = User::factory()->create(); $playlistItem = $this->createPlaylistItem(); - $this->room->startPlay($user, $playlistItem, 0); + static::roomStartPlay($user, $playlistItem); $agg = UserScoreAggregate::new($user, $this->room); $this->assertSame(1, $agg->attempts); diff --git a/tests/TestCase.php b/tests/TestCase.php index 03c9ada27fe..1b4af0dae93 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -16,6 +16,7 @@ use App\Models\Multiplayer\PlaylistItem; use App\Models\Multiplayer\ScoreLink; use App\Models\OAuth\Client; +use App\Models\ScoreToken; use App\Models\User; use Artisan; use Carbon\CarbonInterface; @@ -111,7 +112,7 @@ protected static function resetAppDb(DatabaseManager $database): void protected static function roomAddPlay(User $user, PlaylistItem $playlistItem, array $scoreParams): ScoreLink { return $playlistItem->room->completePlay( - $playlistItem->room->startPlay($user, $playlistItem, 0), + static::roomStartPlay($user, $playlistItem), [ 'accuracy' => 0.5, 'beatmap_id' => $playlistItem->beatmap_id, @@ -126,6 +127,16 @@ protected static function roomAddPlay(User $user, PlaylistItem $playlistItem, ar ); } + protected static function roomStartPlay(User $user, PlaylistItem $playlistItem): ScoreToken + { + return $playlistItem->room->startPlay( + $user, + $playlistItem, + 0, + ['beatmap_hash' => $playlistItem->beatmap->checksum] + ); + } + protected function setUp(): void { $this->beforeApplicationDestroyed(fn () => $this->runExpectedCountsCallbacks());