From 5d2ff8727158a3d5681ca0cb980f35fcfede251a Mon Sep 17 00:00:00 2001 From: Phan An Date: Sat, 27 Jan 2024 12:24:34 +0100 Subject: [PATCH] feat(plus): revise artist/image art upload policies --- .../API/UploadAlbumCoverController.php | 4 +- .../API/UploadArtistImageController.php | 4 +- .../Requests/API/MediaImageUpdateRequest.php | 5 -- app/Models/Song.php | 7 +- app/Policies/AlbumPolicy.php | 30 +++++++ app/Policies/ArtistPolicy.php | 27 +++++++ .../js/components/ui/AlbumArtistThumbnail.vue | 20 ++++- resources/assets/js/stores/albumStore.spec.ts | 2 +- resources/assets/js/stores/albumStore.ts | 2 +- .../assets/js/stores/artistStore.spec.ts | 2 +- resources/assets/js/stores/artistStore.ts | 2 +- tests/Feature/KoelPlus/AlbumCoverTest.php | 78 +++++++++++++++++++ tests/Feature/KoelPlus/ArtistImageTest.php | 78 +++++++++++++++++++ 13 files changed, 247 insertions(+), 14 deletions(-) create mode 100644 app/Policies/AlbumPolicy.php create mode 100644 app/Policies/ArtistPolicy.php create mode 100644 tests/Feature/KoelPlus/AlbumCoverTest.php create mode 100644 tests/Feature/KoelPlus/ArtistImageTest.php diff --git a/app/Http/Controllers/API/UploadAlbumCoverController.php b/app/Http/Controllers/API/UploadAlbumCoverController.php index 6794661a..ae64fd6f 100644 --- a/app/Http/Controllers/API/UploadAlbumCoverController.php +++ b/app/Http/Controllers/API/UploadAlbumCoverController.php @@ -12,6 +12,8 @@ class UploadAlbumCoverController extends Controller { public function __invoke(UploadAlbumCoverRequest $request, Album $album, MediaMetadataService $mediaMetadataService) { + $this->authorize('update', $album); + $mediaMetadataService->writeAlbumCover( $album, $request->getFileContentAsBinaryString(), @@ -20,6 +22,6 @@ class UploadAlbumCoverController extends Controller event(new LibraryChanged()); - return response()->json(['coverUrl' => $album->cover]); + return response()->json(['cover_url' => $album->cover]); } } diff --git a/app/Http/Controllers/API/UploadArtistImageController.php b/app/Http/Controllers/API/UploadArtistImageController.php index b355b1f4..3b1f3ab8 100644 --- a/app/Http/Controllers/API/UploadArtistImageController.php +++ b/app/Http/Controllers/API/UploadArtistImageController.php @@ -15,6 +15,8 @@ class UploadArtistImageController extends Controller Artist $artist, MediaMetadataService $mediaMetadataService ) { + $this->authorize('update', $artist); + $mediaMetadataService->writeArtistImage( $artist, $request->getFileContentAsBinaryString(), @@ -23,6 +25,6 @@ class UploadArtistImageController extends Controller event(new LibraryChanged()); - return response()->json(['imageUrl' => $artist->image]); + return response()->json(['image_url' => $artist->image]); } } diff --git a/app/Http/Requests/API/MediaImageUpdateRequest.php b/app/Http/Requests/API/MediaImageUpdateRequest.php index 19ed2bf1..2d587f91 100644 --- a/app/Http/Requests/API/MediaImageUpdateRequest.php +++ b/app/Http/Requests/API/MediaImageUpdateRequest.php @@ -15,11 +15,6 @@ abstract class MediaImageUpdateRequest extends Request ]; } - public function authorize(): bool - { - return auth()->user()->is_admin; - } - public function getFileContentAsBinaryString(): string { return base64_decode(Str::after($this->{$this->getImageFieldName()}, ','), true); diff --git a/app/Models/Song.php b/app/Models/Song.php index b00a006d..027c5742 100644 --- a/app/Models/Song.php +++ b/app/Models/Song.php @@ -122,7 +122,12 @@ class Song extends Model public function accessibleBy(User $user): bool { - return $this->is_public || $this->owner_id === $user->id; + return $this->is_public || $this->ownedBy($user); + } + + public function ownedBy(User $user): bool + { + return $this->owner_id === $user->id; } protected function lyrics(): Attribute diff --git a/app/Policies/AlbumPolicy.php b/app/Policies/AlbumPolicy.php new file mode 100644 index 00000000..58ccd6fc --- /dev/null +++ b/app/Policies/AlbumPolicy.php @@ -0,0 +1,30 @@ +is_admin) { + return Response::allow(); + } + + if (License::isCommunity()) { + return Response::deny('This action is unauthorized.'); + } + + return $album->songs->every(static fn (Song $song) => $song->ownedBy($user)) + ? Response::allow() + : Response::deny('Album is not owned by the user.'); + } +} diff --git a/app/Policies/ArtistPolicy.php b/app/Policies/ArtistPolicy.php new file mode 100644 index 00000000..4a1c826a --- /dev/null +++ b/app/Policies/ArtistPolicy.php @@ -0,0 +1,27 @@ +is_admin) { + return Response::allow(); + } + + if (License::isCommunity()) { + return Response::deny('This action is unauthorized.'); + } + + return $artist->songs->every(static fn (Song $song) => $song->ownedBy($user)) + ? Response::allow() + : Response::deny('Artist is not owned by the user.'); + } +} diff --git a/resources/assets/js/components/ui/AlbumArtistThumbnail.vue b/resources/assets/js/components/ui/AlbumArtistThumbnail.vue index 5ffa8ab5..3474f582 100644 --- a/resources/assets/js/components/ui/AlbumArtistThumbnail.vue +++ b/resources/assets/js/components/ui/AlbumArtistThumbnail.vue @@ -27,7 +27,7 @@ import { computed, ref, toRef, toRefs } from 'vue' import { albumStore, artistStore, queueStore, songStore, userStore } from '@/stores' import { playbackService } from '@/services' import { defaultCover, fileReader, logger } from '@/utils' -import { useAuthorization, useMessageToaster, useRouter } from '@/composables' +import { useAuthorization, useMessageToaster, useRouter, useKoelPlus } from '@/composables' const VALID_IMAGE_TYPES = ['image/jpeg', 'image/gif', 'image/png', 'image/webp'] @@ -40,6 +40,10 @@ const { entity } = toRefs(props) const droppable = ref(false) const user = toRef(userStore.state, 'current') +const { isAdmin } = useAuthorization() +const { isPlus } = useKoelPlus() +const { toastError } = useMessageToaster() + const forAlbum = computed(() => entity.value.type === 'albums') const sortFields = computed(() => forAlbum.value ? ['disc', 'track'] : ['album_id', 'disc', 'track']) @@ -54,7 +58,7 @@ const buttonLabel = computed(() => forAlbum.value : `Play all songs by ${entity.value.name}` ) -const { isAdmin: allowsUpload } = useAuthorization() +const allowsUpload = computed(() => isAdmin.value || isPlus.value) // for Plus, the logic is handled in the backend const playOrQueue = async (event: MouseEvent) => { const songs = forAlbum.value @@ -101,6 +105,8 @@ const onDrop = async (event: DragEvent) => { return } + const backupImage = forAlbum.value ? (entity.value as Album).cover : (entity.value as Artist).image + try { const fileData = await fileReader.readAsDataUrl(event.dataTransfer!.files[0]) @@ -113,6 +119,16 @@ const onDrop = async (event: DragEvent) => { await artistStore.uploadImage(entity.value as Artist, fileData) } } catch (e) { + const message = e?.response?.data?.message ?? 'Unknown error.' + toastError(`Failed to upload: ${message}`) + + // restore the backup image + if (forAlbum.value) { + (entity.value as Album).cover = backupImage! + } else { + (entity.value as Artist).image = backupImage! + } + logger.error(e) } } diff --git a/resources/assets/js/stores/albumStore.spec.ts b/resources/assets/js/stores/albumStore.spec.ts index 6bb826b3..e100b331 100644 --- a/resources/assets/js/stores/albumStore.spec.ts +++ b/resources/assets/js/stores/albumStore.spec.ts @@ -57,7 +57,7 @@ new class extends UnitTestCase { const album = factory('album') albumStore.syncWithVault(album) const songsInAlbum = factory('song', 3, { album_id: album.id }) - const putMock = this.mock(http, 'put').mockResolvedValue({ coverUrl: 'http://test/cover.jpg' }) + const putMock = this.mock(http, 'put').mockResolvedValue({ cover_url: 'http://test/cover.jpg' }) this.mock(songStore, 'byAlbum', songsInAlbum) await albumStore.uploadCover(album, 'data://cover') diff --git a/resources/assets/js/stores/albumStore.ts b/resources/assets/js/stores/albumStore.ts index a85b86f4..244c0f47 100644 --- a/resources/assets/js/stores/albumStore.ts +++ b/resources/assets/js/stores/albumStore.ts @@ -47,7 +47,7 @@ export const albumStore = { * @param {string} cover The content data string of the cover */ async uploadCover (album: Album, cover: string) { - album.cover = (await http.put<{ coverUrl: string }>(`album/${album.id}/cover`, { cover })).coverUrl + album.cover = (await http.put<{ cover_url: string }>(`album/${album.id}/cover`, { cover })).cover_url songStore.byAlbum(album).forEach(song => song.album_cover = album.cover) // sync to vault diff --git a/resources/assets/js/stores/artistStore.spec.ts b/resources/assets/js/stores/artistStore.spec.ts index 62c1d9ef..709968c4 100644 --- a/resources/assets/js/stores/artistStore.spec.ts +++ b/resources/assets/js/stores/artistStore.spec.ts @@ -70,7 +70,7 @@ new class extends UnitTestCase { it('uploads an image for an artist', async () => { const artist = factory('artist') artistStore.syncWithVault(artist) - const putMock = this.mock(http, 'put').mockResolvedValue({ imageUrl: 'http://test/img.jpg' }) + const putMock = this.mock(http, 'put').mockResolvedValue({ image_url: 'http://test/img.jpg' }) await artistStore.uploadImage(artist, 'data://image') diff --git a/resources/assets/js/stores/artistStore.ts b/resources/assets/js/stores/artistStore.ts index a4f4fd3a..1f63de0e 100644 --- a/resources/assets/js/stores/artistStore.ts +++ b/resources/assets/js/stores/artistStore.ts @@ -35,7 +35,7 @@ export const artistStore = { }, async uploadImage (artist: Artist, image: string) { - artist.image = (await http.put<{ imageUrl: string }>(`artist/${artist.id}/image`, { image })).imageUrl + artist.image = (await http.put<{ image_url: string }>(`artist/${artist.id}/image`, { image })).image_url // sync to vault this.byId(artist.id)!.image = artist.image diff --git a/tests/Feature/KoelPlus/AlbumCoverTest.php b/tests/Feature/KoelPlus/AlbumCoverTest.php new file mode 100644 index 00000000..ba593350 --- /dev/null +++ b/tests/Feature/KoelPlus/AlbumCoverTest.php @@ -0,0 +1,78 @@ +mediaMetadataService = self::mock(MediaMetadataService::class); + } + + public function testNormalUserCanUploadCoverIfOwningAllSongsInAlbum(): void + { + $this->expectsEvents(LibraryChanged::class); + + $user = create_user(); + + /** @var Album $album */ + $album = Album::factory()->create(); + $album->songs()->saveMany(Song::factory()->for($user, 'owner')->count(3)->create()); + + $this->mediaMetadataService + ->shouldReceive('writeAlbumCover') + ->once() + ->with(Mockery::on(static fn (Album $target) => $target->is($album)), 'Foo', 'jpeg'); + + $this->putAs("api/album/$album->id/cover", ['cover' => 'data:image/jpeg;base64,Rm9v'], $user) + ->assertOk(); + } + + public function testNormalUserCannotUploadCoverIfNotOwningAllSongsInAlbum(): void + { + $user = create_user(); + + /** @var Album $album */ + $album = Album::factory()->create(); + $album->songs()->saveMany(Song::factory()->for($user, 'owner')->count(3)->create()); + $album->songs()->save(Song::factory()->create()); + + $this->mediaMetadataService + ->shouldReceive('writeAlbumCover') + ->never(); + + $this->putAs("api/album/$album->id/cover", ['cover' => 'data:image/jpeg;base64,Rm9v'], $user) + ->assertForbidden(); + } + + public function testAdminCanUploadCoverEvenIfNotOwningAllSongsInAlbum(): void + { + $this->expectsEvents(LibraryChanged::class); + + $user = create_user(); + + /** @var Album $album */ + $album = Album::factory()->create(); + $album->songs()->saveMany(Song::factory()->for($user, 'owner')->count(3)->create()); + + $this->mediaMetadataService + ->shouldReceive('writeAlbumCover') + ->once() + ->with(Mockery::on(static fn (Album $target) => $target->is($album)), 'Foo', 'jpeg'); + + $this->putAs("api/album/$album->id/cover", ['cover' => 'data:image/jpeg;base64,Rm9v'], create_admin()) + ->assertOk(); + } +} diff --git a/tests/Feature/KoelPlus/ArtistImageTest.php b/tests/Feature/KoelPlus/ArtistImageTest.php new file mode 100644 index 00000000..7e186cff --- /dev/null +++ b/tests/Feature/KoelPlus/ArtistImageTest.php @@ -0,0 +1,78 @@ +mediaMetadataService = self::mock(MediaMetadataService::class); + } + + public function testNormalUserCanUploadImageIfOwningAllSongsInArtist(): void + { + $this->expectsEvents(LibraryChanged::class); + + $user = create_user(); + + /** @var Artist $artist */ + $artist = Artist::factory()->create(); + $artist->songs()->saveMany(Song::factory()->for($user, 'owner')->count(3)->create()); + + $this->mediaMetadataService + ->shouldReceive('writeArtistImage') + ->once() + ->with(Mockery::on(static fn (Artist $target) => $target->is($artist)), 'Foo', 'jpeg'); + + $this->putAs("api/artist/$artist->id/image", ['image' => 'data:image/jpeg;base64,Rm9v'], $user) + ->assertOk(); + } + + public function testNormalUserCannotUploadImageIfNotOwningAllSongsInArtist(): void + { + $user = create_user(); + + /** @var Artist $artist */ + $artist = Artist::factory()->create(); + $artist->songs()->saveMany(Song::factory()->for($user, 'owner')->count(3)->create()); + $artist->songs()->save(Song::factory()->create()); + + $this->mediaMetadataService + ->shouldReceive('writeArtistImage') + ->never(); + + $this->putAs("api/artist/$artist->id/image", ['image' => 'data:image/jpeg;base64,Rm9v'], $user) + ->assertForbidden(); + } + + public function testAdminCanUploadImageEvenIfNotOwningAllSongsInArtist(): void + { + $this->expectsEvents(LibraryChanged::class); + + $user = create_user(); + + /** @var Artist $artist */ + $artist = Artist::factory()->create(); + $artist->songs()->saveMany(Song::factory()->for($user, 'owner')->count(3)->create()); + + $this->mediaMetadataService + ->shouldReceive('writeArtistImage') + ->once() + ->with(Mockery::on(static fn (Artist $target) => $target->is($artist)), 'Foo', 'jpeg'); + + $this->putAs("api/artist/$artist->id/image", ['image' => 'data:image/jpeg;base64,Rm9v'], create_admin()) + ->assertOk(); + } +}