Add endpoint for showing screenshots #12664
Conversation
| abort_if(Screenshot::urlHash(intval($id)) !== $hash, 404); | ||
|
|
||
| return $this->showBase($id); | ||
| } | ||
|
|
||
| public function showLegacy(int $id) | ||
| { | ||
| abort_if(!Screenshot::isLegacyId(intval($id)), 404); |
There was a problem hiding this comment.
the routes can be combined with a lookup function in the model which does something like
if hash == null
if id > legacy_id
return null
else
expectedHash = ...
if !hash_equals(expectedHash, hash)
return null
return findOrFail(...)
| $screenshot->update([ | ||
| 'hits' => $screenshot->hits + 1, | ||
| 'last_access' => Carbon::now(), | ||
| ]); |
There was a problem hiding this comment.
should use incrementInstance('hits', 1, ['last_access' => ...])
|
|
||
| return response()->stream(function () use ($file) { | ||
| echo $file; | ||
| }, 200, ['Content-Type' => 'image/jpeg']); |
There was a problem hiding this comment.
need cache-control header too (it's missing in the current one)
|
|
||
| $file = $screenshot->fetch(); | ||
|
|
||
| abort_if(!$file, 404); |
There was a problem hiding this comment.
doesn't really matter here but generally should avoid casting php string to boolean (explicitly check with === null instead)
|
|
||
| $user = User::factory()->create(); | ||
| $screenshot = Screenshot::factory()->create(['user_id' => $user->getKey()]); | ||
| \Storage::disk('local-screenshot')->putFileAs('/', UploadedFile::fake()->image('ss.jpg'), "{$screenshot->getKey()}.jpg"); |
There was a problem hiding this comment.
use the existing store function?
| $this->assertGreaterThan(0, preg_match('/https?:\/\/.+\/(\d+)\/(.{4})/', $url, $matches)); | ||
|
|
||
| \Storage::disk('local-screenshot')->assertExists("{$matches[1]}.jpg"); | ||
| $this->assertEquals(Screenshot::urlHash(intval($matches[1])), $matches[2]); |
|
|
||
| $matches = []; | ||
|
|
||
| $this->assertGreaterThan(0, preg_match('/https?:\/\/.+\/(\d+)\/(.{4})/', $url, $matches)); |
There was a problem hiding this comment.
use a different regexp delimiter so / doesn't need to be escaped. No need to match https? (or explicitly match the start), but the end should probably be matched explicitly with $
|
|
||
| $this->assertGreaterThan(0, preg_match('/https?:\/\/.+\/(\d+)\/(.{4})/', $url, $matches)); | ||
|
|
||
| \Storage::disk('local-screenshot')->assertExists("{$matches[1]}.jpg"); |
There was a problem hiding this comment.
or get the object and check its fetch result instead of relying on specific file structure
.env.example
Outdated
|
|
||
| # S3_SCREENSHOT_BUCKET= | ||
| # SCREENSHOTS_SHARED_SECRET= | ||
| # SCREENSHOTS_LEGACY_ID_CUTOFF= |
There was a problem hiding this comment.
I forgot in the other pr but nowadays the example should have the default value
|
|
||
| public function testShow() | ||
| { | ||
| $user = User::factory()->create(); |
There was a problem hiding this comment.
apart for the store test, this doesn't do anything
app/Models/Screenshot.php
Outdated
| if (!self::isLegacyId($id)) { | ||
| return null; | ||
| } | ||
| } else if (self::hashForId($id) !== $hash) { |
There was a problem hiding this comment.
use hash_equals when comparing secrets
app/Models/Screenshot.php
Outdated
| public function hash(): ?string | ||
| { | ||
| return substr(md5($this->getKey().$GLOBALS['cfg']['osu']['screenshots']['shared_secret']), 0, 4); | ||
| if (self::isLegacyId($this->getKey())) { |
There was a problem hiding this comment.
I don't think this is needed
| $this->actAsScopedUser($user); | ||
|
|
||
| $this->postJson(route('api.screenshots.store'), [ | ||
| \Storage::fake('local-screenshot'); |
There was a problem hiding this comment.
might as well put this in setUp
3 participants
Depends on #12618. Recreated the original PR (#12623) to pull in changes from base PR code review and some cleanup.
Steps to deploy:
SCREENSHOTS_LEGACY_ID_CUTOFFenv var.