Rework score token validation

Mainly in relation to multiplayer play.
nanaya · Jan 8, 2025 · 4a6640e · 4a6640e
1 parent e31f878
commit 4a6640e
Show file tree

Hide file tree

Showing 10 changed files with 110 additions and 81 deletions.
diff --git a/app/Http/Controllers/Multiplayer/Rooms/Playlist/ScoresController.php b/app/Http/Controllers/Multiplayer/Rooms/Playlist/ScoresController.php
@@ -5,10 +5,8 @@
 
 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\Beatmap;
 use App\Models\Multiplayer\PlaylistItem;
 use App\Models\Multiplayer\PlaylistItemUserHighScore;
 use App\Models\Multiplayer\Room;
@@ -183,23 +181,10 @@ public function store($roomId, $playlistId)
         $playlistItem = $room->playlist()->findOrFail($playlistId);
         $user = \Auth::user();
         $request = \Request::instance();
-        $params = $request->all();
-
-        if ($playlistItem->freestyle) {
-            // Todo: Ensure beatmap_hash matches any beatmap from the playlist item's beatmap set.
-            // Todo: Ensure ruleset_id is valid (converts only allowed for id=0 otherwise must match beatmap playmode, and value within 0..3).
-            // Todo: Modifying the playlist item looks dodgy to me :)!
-            $playlistItem->beatmap_id = Beatmap::firstWhere('checksum', get_string($params['beatmap_hash'] ?? null))->beatmap_id;
-            $playlistItem->ruleset_id = get_int($params['ruleset_id'] ?? null);
-        } else {
-            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,24 @@ 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();
 
-        $this->assertValidStartPlay($user, $playlistItem);
+        $params = get_params($rawParams, null, [
+            'beatmap_hash',
+            'beatmap_id:int',
+            'ruleset_id:int',
+        ], ['null_missing' => true]);
+
+        if (!$playlistItem->freestyle) {
+            $params['beatmap_id'] ??= $playlistItem->beatmap_id;
+            $params['ruleset_id'] ??= $playlistItem->ruleset_id;
+        }
+
+        $this->assertValidStartPlay($user, $playlistItem, $params);
 
-        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 +687,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,
             ]);
         });
     }
@@ -740,14 +750,24 @@ private function assertValidStartGame()
         }
     }
 
-    private function assertValidStartPlay(User $user, PlaylistItem $playlistItem)
+    private function assertValidStartPlay(User $user, PlaylistItem $playlistItem, array $params)
     {
         // todo: check against room's end time (to see if player has enough time to play this beatmap) and is under the room's max attempts limit
-
         if ($this->hasEnded()) {
             throw new InvariantException('Room has already ended.');
         }
 
+        if ($playlistItem->freestyle) {
+            // assert the beatmap_id is part of playlist item's beatmapset
+            $beatmapsetIdCount = Beatmap
+                ::whereKey([$playlistItem->beatmap_id, $params['beatmap_id']])
+                ->distinct('beatmapset_id')
+                ->count();
+            if ($beatmapsetIdCount !== 1) {
+                throw new InvariantException('Specified beatmap_id is not allowed');
+            }
+        }
+
         $userId = $user->getKey();
         if ($this->max_attempts !== null) {
             $roomStats = $this->userHighScores()->where('user_id', $userId)->first();

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());
     }