feat(plus): revise artist/image art upload policies

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

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

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

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

app/Policies/AlbumPolicy.php

@@ -0,0 +1,30 @@
+<?php
+
+namespace App\Policies;
+
+use App\Facades\License;
+use App\Models\Album;
+use App\Models\Song;
+use App\Models\User;
+use Illuminate\Auth\Access\Response;
+
+class AlbumPolicy
+{
+    /**
+     * If the user can update the album (e.g. upload cover).
+     */
+    public function update(User $user, Album $album): Response
+    {
+        if ($user->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.');
+    }
+}

app/Policies/ArtistPolicy.php

@@ -0,0 +1,27 @@
+<?php
+
+namespace App\Policies;
+
+use App\Facades\License;
+use App\Models\Artist;
+use App\Models\Song;
+use App\Models\User;
+use Illuminate\Auth\Access\Response;
+
+class ArtistPolicy
+{
+    public function update(User $user, Artist $artist): Response
+    {
+        if ($user->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.');
+    }
+}

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

resources/assets/js/stores/albumStore.spec.ts

@@ -57,7 +57,7 @@ new class extends UnitTestCase {
       const album = factory<Album>('album')
       albumStore.syncWithVault(album)
       const songsInAlbum = factory<Song>('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')

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

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>('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')
 

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

tests/Feature/KoelPlus/AlbumCoverTest.php

@@ -0,0 +1,78 @@
+<?php
+
+namespace Tests\Feature\KoelPlus;
+
+use App\Events\LibraryChanged;
+use App\Models\Album;
+use App\Models\Song;
+use App\Services\MediaMetadataService;
+use Mockery;
+use Tests\PlusTestCase;
+
+use function Tests\create_admin;
+use function Tests\create_user;
+
+class AlbumCoverTest extends PlusTestCase
+{
+    public function setUp(): void
+    {
+        parent::setUp();
+
+        $this->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();
+    }
+}

tests/Feature/KoelPlus/ArtistImageTest.php

@@ -0,0 +1,78 @@
+<?php
+
+namespace Tests\Feature\KoelPlus;
+
+use App\Events\LibraryChanged;
+use App\Models\Artist;
+use App\Models\Song;
+use App\Services\MediaMetadataService;
+use Mockery;
+use Tests\PlusTestCase;
+
+use function Tests\create_admin;
+use function Tests\create_user;
+
+class ArtistImageTest extends PlusTestCase
+{
+    public function setUp(): void
+    {
+        parent::setUp();
+
+        $this->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();
+    }
+}