Rework score token validation

Mainly in relation to multiplayer play.
nanaya · Jan 8, 2025 · 1eabf9c · 1eabf9c
1 parent 52ea0cf
commit 1eabf9c
Show file tree

Hide file tree

Showing 10 changed files with 75 additions and 52 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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());