diff --git a/app/Http/Controllers/API/LastfmController.php b/app/Http/Controllers/API/LastfmController.php index e9a454b8..619f712b 100644 --- a/app/Http/Controllers/API/LastfmController.php +++ b/app/Http/Controllers/API/LastfmController.php @@ -2,83 +2,24 @@ namespace App\Http\Controllers\API; -use App\Http\Requests\API\LastfmCallbackRequest; use App\Http\Requests\API\LastfmSetSessionKeyRequest; use App\Models\User; -use App\Services\LastfmService; -use App\Services\TokenManager; use Illuminate\Contracts\Auth\Authenticatable; use Illuminate\Http\JsonResponse; -use Illuminate\Http\RedirectResponse; -use Illuminate\Http\Response; /** * @group Last.fm integration */ class LastfmController extends Controller { - private $lastfmService; - private $tokenManager; - /** @var User */ private $currentUser; - public function __construct(LastfmService $lastfmService, TokenManager $tokenManager, ?Authenticatable $currentUser) + public function __construct(?Authenticatable $currentUser) { - $this->lastfmService = $lastfmService; - $this->tokenManager = $tokenManager; $this->currentUser = $currentUser; } - /** - * Connect to Last.fm - * - * [Connect](https://www.last.fm/api/authentication) the current user to Last.fm. - * This is actually NOT an API request. The application should instead redirect the current user to this route, - * which will send them to Last.fm for authentication. After authentication is successful, the user will be - * redirected back to `lastfm/callback?token=`. - * - * @queryParam jwt-token required The JWT token of the user. (Deprecated. Use api_token instead). - * @queryParam api_token required Authentication token of the current user. - * @response [] - * - * @return RedirectResponse - */ - public function connect() - { - abort_unless( - $this->lastfmService->enabled(), - Response::HTTP_NOT_IMPLEMENTED, - 'Koel is not configured to use with Last.fm yet.' - ); - - $callbackUrl = urlencode(sprintf( - '%s?api_token=%s', - route('lastfm.callback'), - request('api_token') - )); - - $url = sprintf('https://www.last.fm/api/auth/?api_key=%s&cb=%s', $this->lastfmService->getKey(), $callbackUrl); - - return redirect($url); - } - - /** - * Serve the callback request from Last.fm. - */ - public function callback(LastfmCallbackRequest $request) - { - $this->currentUser = $this->tokenManager->getUserFromPlainTextToken($request->api_token); - abort_unless((bool) $this->currentUser, Response::HTTP_UNAUTHORIZED); - - $sessionKey = $this->lastfmService->getSessionKey($request->token); - abort_unless($sessionKey, Response::HTTP_INTERNAL_SERVER_ERROR, 'Invalid token key.'); - - $this->currentUser->savePreference('lastfm_session_key', $sessionKey); - - return view('api.lastfm.callback'); - } - /** * Set Last.fm session key * @@ -98,7 +39,7 @@ class LastfmController extends Controller } /** - * Disconnect the current user from Last.fm. + * Disconnect the current user from Last.fm * * @return JsonResponse */ diff --git a/app/Http/Controllers/API/PlaylistController.php b/app/Http/Controllers/API/PlaylistController.php index 17e27176..97ba42ab 100644 --- a/app/Http/Controllers/API/PlaylistController.php +++ b/app/Http/Controllers/API/PlaylistController.php @@ -29,7 +29,7 @@ class PlaylistController extends Controller public function __construct( PlaylistRepository $playlistRepository, SmartPlaylistService $smartPlaylistService, - Authenticatable $currentUser + ?Authenticatable $currentUser ) { $this->playlistRepository = $playlistRepository; $this->smartPlaylistService = $smartPlaylistService; @@ -88,7 +88,7 @@ class PlaylistController extends Controller */ public function update(Request $request, Playlist $playlist) { - abort_unless($this->currentUser->can('owner', $playlist), Response::HTTP_FORBIDDEN); + $this->authorize('owner', $playlist); $playlist->update($request->only('name', 'rules')); diff --git a/app/Http/Controllers/API/SongController.php b/app/Http/Controllers/API/SongController.php index 1e8a8604..9e1121ad 100644 --- a/app/Http/Controllers/API/SongController.php +++ b/app/Http/Controllers/API/SongController.php @@ -2,65 +2,28 @@ namespace App\Http\Controllers\API; -use App\Factories\StreamerFactory; -use App\Http\Requests\API\SongPlayRequest; use App\Http\Requests\API\SongUpdateRequest; use App\Models\Song; use App\Repositories\AlbumRepository; use App\Repositories\ArtistRepository; -use App\Services\MediaInformationService; use Illuminate\Http\JsonResponse; -use Illuminate\Http\RedirectResponse; -use Illuminate\Routing\Redirector; /** * @group 3. Song interactions */ class SongController extends Controller { - private $mediaInformationService; - private $streamerFactory; private $artistRepository; private $albumRepository; public function __construct( - MediaInformationService $mediaInformationService, - StreamerFactory $streamerFactory, ArtistRepository $artistRepository, AlbumRepository $albumRepository ) { - $this->mediaInformationService = $mediaInformationService; - $this->streamerFactory = $streamerFactory; $this->artistRepository = $artistRepository; $this->albumRepository = $albumRepository; } - /** - * Play a song - * - * The GET request to play/stream a song. By default Koel will serve the file as-is, unless it's a FLAC. - * If the value of `transcode` is truthy, Koel will attempt to transcode the file into `bitRate`kbps using ffmpeg. - * - * @response {} - * - * @queryParam jwt-token required The JWT token. - * - * @see https://github.com/phanan/koel/wiki#streaming-music - * - * @param bool|null $transcode Whether to force transcoding the song. - * If this is omitted, by default Koel will transcode FLAC. - * @param int|null $bitRate The target bit rate to transcode, defaults to OUTPUT_BIT_RATE. - * Only taken into account if $transcode is truthy. - * - * @return RedirectResponse|Redirector - */ - public function play(SongPlayRequest $request, Song $song, ?bool $transcode = null, ?int $bitRate = null) - { - return $this->streamerFactory - ->createStreamer($song, $transcode, $bitRate, floatval($request->time)) - ->stream(); - } - /** * Update song information * diff --git a/app/Http/Controllers/API/Download/AlbumController.php b/app/Http/Controllers/Download/AlbumController.php similarity index 87% rename from app/Http/Controllers/API/Download/AlbumController.php rename to app/Http/Controllers/Download/AlbumController.php index b60e7462..786401e7 100644 --- a/app/Http/Controllers/API/Download/AlbumController.php +++ b/app/Http/Controllers/Download/AlbumController.php @@ -1,6 +1,6 @@ lastfmService = $lastfmService; + $this->tokenManager = $tokenManager; + $this->currentUser = $currentUser; + } + + /** + * Connect to Last.fm + * + * [Connect](https://www.last.fm/api/authentication) the current user to Last.fm. + * This is actually NOT an API request. The application should instead redirect the current user to this route, + * which will send them to Last.fm for authentication. After authentication is successful, the user will be + * redirected back to `lastfm/callback?token=`. + * + * @queryParam api_token required Authentication token of the current user. + * @response [] + * + * @return RedirectResponse + */ + public function connect() + { + abort_unless( + $this->lastfmService->enabled(), + Response::HTTP_NOT_IMPLEMENTED, + 'Koel is not configured to use with Last.fm yet.' + ); + + $callbackUrl = urlencode(sprintf( + '%s?api_token=%s', + route('lastfm.callback'), + // for enhanced security, create a temporary token that can be deleted later + $this->tokenManager->createToken($this->currentUser)->plainTextToken + )); + + $url = sprintf('https://www.last.fm/api/auth/?api_key=%s&cb=%s', $this->lastfmService->getKey(), $callbackUrl); + + return redirect($url); + } + + /** + * Serve the callback request from Last.fm. + */ + public function callback(LastfmCallbackRequest $request) + { + $sessionKey = $this->lastfmService->getSessionKey($request->token); + abort_unless($sessionKey, Response::HTTP_INTERNAL_SERVER_ERROR, 'Invalid token key.'); + + $this->currentUser->savePreference('lastfm_session_key', $sessionKey); + + // delete the tmp. token we created earlier + $this->tokenManager->deleteTokenByPlainTextToken($request->api_token); + + return view('lastfm.callback'); + } +} diff --git a/app/Http/Controllers/PlayController.php b/app/Http/Controllers/PlayController.php new file mode 100644 index 00000000..960c3508 --- /dev/null +++ b/app/Http/Controllers/PlayController.php @@ -0,0 +1,48 @@ +tokenManager = $tokenManager; + $this->streamerFactory = $streamerFactory; + } + + /** + * Play a song + * + * The GET request to play/stream a song. By default Koel will serve the file as-is, unless it's a FLAC. + * If the value of `transcode` is truthy, Koel will attempt to transcode the file into `bitRate`kbps using ffmpeg. + * + * @response {} + * + * @queryParam api_token required The API token. + * + * @see https://github.com/phanan/koel/wiki#streaming-music + * + * @param bool|null $transcode Whether to force transcoding the song. + * If this is omitted, by default Koel will transcode FLAC. + * @param int|null $bitRate The target bit rate to transcode, defaults to OUTPUT_BIT_RATE. + * Only taken into account if $transcode is truthy. + * + * @return RedirectResponse|Redirector + */ + public function show(SongPlayRequest $request, Song $song, ?bool $transcode = null, ?int $bitRate = null) + { + return $this->streamerFactory + ->createStreamer($song, $transcode, $bitRate, floatval($request->time)) + ->stream(); + } +} diff --git a/app/Http/Controllers/API/iTunesController.php b/app/Http/Controllers/iTunesController.php similarity index 61% rename from app/Http/Controllers/API/iTunesController.php rename to app/Http/Controllers/iTunesController.php index 6fbbffc2..930c1ea9 100644 --- a/app/Http/Controllers/API/iTunesController.php +++ b/app/Http/Controllers/iTunesController.php @@ -1,19 +1,23 @@ iTunesService = $iTunesService; + $this->tokenManager = $tokenManager; } /** @@ -23,6 +27,11 @@ class iTunesController extends Controller */ public function viewSong(ViewSongOnITunesRequest $request, Album $album) { + abort_unless( + (bool) $this->tokenManager->getUserFromPlainTextToken($request->api_token), + Response::HTTP_UNAUTHORIZED + ); + $url = $this->iTunesService->getTrackUrl($request->q, $album->name, $album->artist->name); abort_unless($url, 404, "Koel can't find such a song on iTunes Store."); diff --git a/app/Http/Kernel.php b/app/Http/Kernel.php index b4012a38..b032642a 100644 --- a/app/Http/Kernel.php +++ b/app/Http/Kernel.php @@ -36,7 +36,7 @@ class Kernel extends HttpKernel */ protected $middlewareGroups = [ 'web' => [ - // Koel doesn't use any default Laravel middleware for web. + 'bindings', ], 'api' => [ 'throttle:60,1', diff --git a/app/Http/Requests/API/SongPlayRequest.php b/app/Http/Requests/API/SongPlayRequest.php deleted file mode 100644 index a6204ba5..00000000 --- a/app/Http/Requests/API/SongPlayRequest.php +++ /dev/null @@ -1,10 +0,0 @@ - 'required', + 'api_token' => 'required', ]; } } diff --git a/app/Http/Requests/API/Download/Request.php b/app/Http/Requests/Download/Request.php similarity index 82% rename from app/Http/Requests/API/Download/Request.php rename to app/Http/Requests/Download/Request.php index 1cffe018..d2f9d1b1 100644 --- a/app/Http/Requests/API/Download/Request.php +++ b/app/Http/Requests/Download/Request.php @@ -1,6 +1,6 @@ registerPolicies(); + + Auth::viaRequest('token-via-query-parameter', static function (Request $request): ?User { + /** @var TokenManager $tokenManager */ + $tokenManager = app(TokenManager::class); + + return $tokenManager->getUserFromPlainTextToken($request->api_token ?: ''); + }); } } diff --git a/app/Services/TokenManager.php b/app/Services/TokenManager.php index 7e0695e7..4fccc5b1 100644 --- a/app/Services/TokenManager.php +++ b/app/Services/TokenManager.php @@ -18,6 +18,15 @@ class TokenManager $user->tokens()->delete(); } + public function deleteTokenByPlainTextToken(string $plainTextToken): void + { + $token = PersonalAccessToken::findToken($plainTextToken); + + if ($token) { + $token->delete(); + } + } + public function getUserFromPlainTextToken(string $plainTextToken): ?User { $token = PersonalAccessToken::findToken($plainTextToken); diff --git a/config/auth.php b/config/auth.php index fb421265..4882b5a1 100644 --- a/config/auth.php +++ b/config/auth.php @@ -33,7 +33,7 @@ return [ */ 'guards' => [ 'web' => [ - 'driver' => 'session', + 'driver' => 'token-via-query-parameter', 'provider' => 'users', ], 'api' => [ diff --git a/resources/assets b/resources/assets index b19f6631..af31eddb 160000 --- a/resources/assets +++ b/resources/assets @@ -1 +1 @@ -Subproject commit b19f6631881a7a3062563a93de2f4c6816f8822b +Subproject commit af31eddbc2ae71c46d403f985c06e79ad52ae11b diff --git a/resources/views/api/lastfm/callback.blade.php b/resources/views/lastfm/callback.blade.php similarity index 96% rename from resources/views/api/lastfm/callback.blade.php rename to resources/views/lastfm/callback.blade.php index 9dfe3088..e40cb47e 100644 --- a/resources/views/api/lastfm/callback.blade.php +++ b/resources/views/lastfm/callback.blade.php @@ -1,5 +1,5 @@ - + Authentication successful! diff --git a/routes/api.php b/routes/api.php index 984601e8..c866837f 100644 --- a/routes/api.php +++ b/routes/api.php @@ -29,10 +29,7 @@ Route::group(['namespace' => 'API'], function () { Route::post('settings', 'SettingController@store'); - Route::get('{song}/play/{transcode?}/{bitrate?}', 'SongController@play')->name('song.play'); - Route::post('{song}/scrobble/{timestamp}', 'ScrobbleController@store')->where([ - 'timestamp' => '\d+', - ]); + Route::post('{song}/scrobble/{timestamp}', 'ScrobbleController@store')->where(['timestamp' => '\d+']); Route::put('songs', 'SongController@update'); Route::resource('upload', 'UploadController'); @@ -65,15 +62,6 @@ Route::group(['namespace' => 'API'], function () { Route::get('youtube/search/song/{song}', 'YouTubeController@searchVideosRelatedToSong'); } - // Download routes - Route::group(['prefix' => 'download', 'namespace' => 'Download'], function () { - Route::get('songs', 'SongController@show'); - Route::get('album/{album}', 'AlbumController@show'); - Route::get('artist/{artist}', 'ArtistController@show'); - Route::get('playlist/{playlist}', 'PlaylistController@show'); - Route::get('favorites', 'FavoritesController@show'); - }); - // Info routes Route::group(['namespace' => 'MediaInformation'], function () { Route::get('album/{album}/info', 'AlbumController@show'); @@ -87,11 +75,6 @@ Route::group(['namespace' => 'API'], function () { Route::put('artist/{artist}/image', 'ArtistImageController@update'); Route::get('album/{album}/thumbnail', 'AlbumThumbnailController@get'); - - // iTunes routes - if (iTunes::used()) { - Route::get('itunes/song/{album}', 'iTunesController@viewSong')->name('iTunes.viewSong'); - } }); Route::group(['middleware' => 'os.auth', 'prefix' => 'os', 'namespace' => 'ObjectStorage'], function () { diff --git a/routes/web.php b/routes/web.php index 58c9ee7b..975bdedb 100644 --- a/routes/web.php +++ b/routes/web.php @@ -1,22 +1,34 @@ name('lastfm.connect'); +Route::group(['middleware' => 'auth'], static function (): void { + Route::get('/{song}/play/{transcode?}/{bitrate?}', 'PlayController@show') + ->name('song.play'); -Route::get('/lastfm/callback', 'API\LastfmController@callback') - ->name('lastfm.callback'); + Route::group(['prefix' => 'lastfm'], static function (): void { + Route::get('connect', 'LastfmController@connect')->name('lastfm.connect'); + Route::get('callback', 'LastfmController@callback')->name('lastfm.callback'); + }); + + if (iTunes::used()) { + Route::get('itunes/song/{album}', 'iTunesController@viewSong')->name('iTunes.viewSong'); + } + + Route::group(['prefix' => 'download', 'namespace' => 'Download'], static function (): void { + Route::get('songs', 'SongController@show'); + Route::get('album/{album}', 'AlbumController@show'); + Route::get('artist/{artist}', 'ArtistController@show'); + Route::get('playlist/{playlist}', 'PlaylistController@show'); + Route::get('favorites', 'FavoritesController@show'); + }); +}); diff --git a/tests/Feature/DownloadTest.php b/tests/Feature/DownloadTest.php index a50cbb07..97a91912 100644 --- a/tests/Feature/DownloadTest.php +++ b/tests/Feature/DownloadTest.php @@ -10,6 +10,7 @@ use App\Models\User; use App\Repositories\InteractionRepository; use App\Services\DownloadService; use Exception; +use Illuminate\Http\Response; use Illuminate\Support\Collection; use Mockery; use Mockery\MockInterface; @@ -27,14 +28,30 @@ class DownloadTest extends TestCase public function setUp(): void { parent::setUp(); + static::createSampleMediaSet(); $this->downloadService = static::mockIocDependency(DownloadService::class); } + public function testNonLoggedInUserCannotDownload(): void + { + $song = Song::first(); + + $this->downloadService + ->shouldReceive('from') + ->never(); + + $this->get("download/songs?songs[]={$song->id}") + ->assertRedirect('/'); + } + public function testDownloadOneSong(): void { $song = Song::first(); + /** @var User $user */ + $user = factory(User::class)->create(); + $this->downloadService ->shouldReceive('from') ->once() @@ -43,18 +60,21 @@ class DownloadTest extends TestCase })) ->andReturn($this->mediaPath.'/blank.mp3'); - $this->getAsUser("api/download/songs?songs[]={$song->id}") + $this->get("download/songs?songs[]={$song->id}&api_token=".$user->createToken('Koel')->plainTextToken) ->assertOk(); } public function testDownloadMultipleSongs(): void { + /** @var User $user */ + $user = factory(User::class)->create(); + $songs = Song::take(2)->orderBy('id')->get(); $this->downloadService ->shouldReceive('from') ->once() - ->with(Mockery::on(static function (Collection $retrievedSongs) use ($songs) { + ->with(Mockery::on(static function (Collection $retrievedSongs) use ($songs): bool { $retrievedIds = $retrievedSongs->pluck('id')->toArray(); $requestedIds = $songs->pluck('id')->toArray(); @@ -62,7 +82,10 @@ class DownloadTest extends TestCase })) ->andReturn($this->mediaPath.'/blank.mp3'); // should be a zip file, but we're testing here… - $this->getAsUser("api/download/songs?songs[]={$songs[0]->id}&songs[]={$songs[1]->id}") + $this->get( + "download/songs?songs[]={$songs[0]->id}&songs[]={$songs[1]->id}&api_token=" + .$user->createToken('Koel')->plainTextToken + ) ->assertOk(); } @@ -70,15 +93,18 @@ class DownloadTest extends TestCase { $album = Album::first(); + /** @var User $user */ + $user = factory(User::class)->create(); + $this->downloadService ->shouldReceive('from') ->once() - ->with(Mockery::on(static function (Album $retrievedAlbum) use ($album) { + ->with(Mockery::on(static function (Album $retrievedAlbum) use ($album): bool { return $retrievedAlbum->id === $album->id; })) ->andReturn($this->mediaPath.'/blank.mp3'); - $this->getAsUser("api/download/album/{$album->id}") + $this->get("download/album/{$album->id}?api_token=".$user->createToken('Koel')->plainTextToken) ->assertOk(); } @@ -86,15 +112,18 @@ class DownloadTest extends TestCase { $artist = Artist::first(); + /** @var User $user */ + $user = factory(User::class)->create(); + $this->downloadService ->shouldReceive('from') ->once() - ->with(Mockery::on(static function (Artist $retrievedArtist) use ($artist) { + ->with(Mockery::on(static function (Artist $retrievedArtist) use ($artist): bool { return $retrievedArtist->id === $artist->id; })) ->andReturn($this->mediaPath.'/blank.mp3'); - $this->getAsUser("api/download/artist/{$artist->id}") + $this->get("download/artist/{$artist->id}?api_token=".$user->createToken('Koel')->plainTextToken) ->assertOk(); } @@ -110,13 +139,13 @@ class DownloadTest extends TestCase $this->downloadService ->shouldReceive('from') - ->with(Mockery::on(static function (Playlist $retrievedPlaylist) use ($playlist) { + ->with(Mockery::on(static function (Playlist $retrievedPlaylist) use ($playlist): bool { return $retrievedPlaylist->id === $playlist->id; })) ->once() ->andReturn($this->mediaPath.'/blank.mp3'); - $this->getAsUser("api/download/playlist/{$playlist->id}", $user) + $this->get("download/playlist/{$playlist->id}?api_token=".$user->createToken('Koel')->plainTextToken) ->assertOk(); } @@ -125,8 +154,11 @@ class DownloadTest extends TestCase /** @var Playlist $playlist */ $playlist = factory(Playlist::class)->create(); - $this->getAsUser("api/download/playlist/{$playlist->id}") - ->assertStatus(403); + /** @var User $user */ + $user = factory(User::class)->create(); + + $this->get("download/playlist/{$playlist->id}?api_token=".$user->createToken('Koel')->plainTextToken) + ->assertStatus(Response::HTTP_FORBIDDEN); } public function testDownloadFavorites(): void @@ -138,7 +170,7 @@ class DownloadTest extends TestCase static::mockIocDependency(InteractionRepository::class) ->shouldReceive('getUserFavorites') ->once() - ->with(Mockery::on(static function (User $retrievedUser) use ($user) { + ->with(Mockery::on(static function (User $retrievedUser) use ($user): bool { return $retrievedUser->id === $user->id; })) ->andReturn($favorites); @@ -149,7 +181,7 @@ class DownloadTest extends TestCase ->once() ->andReturn($this->mediaPath.'/blank.mp3'); - $this->getAsUser('api/download/favorites', $user) - ->assertStatus(200); + $this->get('download/favorites?api_token='.$user->createToken('Koel')->plainTextToken) + ->assertOk(); } } diff --git a/tests/Feature/LastfmTest.php b/tests/Feature/LastfmTest.php index 8ff1fc1a..8f813bd9 100644 --- a/tests/Feature/LastfmTest.php +++ b/tests/Feature/LastfmTest.php @@ -9,6 +9,8 @@ use GuzzleHttp\Client; use GuzzleHttp\Psr7\Response; use Illuminate\Contracts\Cache\Repository as Cache; use Illuminate\Log\Logger; +use Laravel\Sanctum\NewAccessToken; +use Laravel\Sanctum\PersonalAccessToken; use Mockery; class LastfmTest extends TestCase @@ -40,13 +42,49 @@ class LastfmTest extends TestCase $user = factory(User::class)->create(); $token = $user->createToken('Koel')->plainTextToken; + $temporaryToken = Mockery::mock(NewAccessToken::class); + $temporaryToken->plainTextToken = 'tmp-token'; + + $tokenManager = static::mockIocDependency(TokenManager::class); + + $tokenManager->shouldReceive('getUserFromPlainTextToken') + ->with($token) + ->andReturn($user); + + $tokenManager->shouldReceive('createToken') + ->once() + ->with($user) + ->andReturn($temporaryToken); + $this->get('lastfm/connect?api_token='.$token) ->assertRedirect( - 'https://www.last.fm/api/auth/?api_key=foo&cb=http%3A%2F%2Flocalhost%2Flastfm%2Fcallback%3Fapi_token%3D' - .urlencode($token) + 'https://www.last.fm/api/auth/?api_key=foo&cb=http%3A%2F%2Flocalhost%2Flastfm%2Fcallback%3Fapi_token%3Dtmp-token' ); } + public function testCallback(): void + { + /** @var User $user */ + $user = factory(User::class)->create(); + $token = $user->createToken('Koel')->plainTextToken; + + self::assertNotNull(PersonalAccessToken::findToken($token)); + + $lastfm = static::mockIocDependency(LastfmService::class); + + $lastfm->shouldReceive('getSessionKey') + ->with('lastfm-token') + ->once() + ->andReturn('my-session-key'); + + $this->get('lastfm/callback?token=lastfm-token&api_token='.urlencode($token)) + ->assertOk(); + + self::assertSame('my-session-key', $user->refresh()->lastfm_session_key); + // make sure the user's api token is deleted + self::assertNull(PersonalAccessToken::findToken($token)); + } + public function testRetrieveAndStoreSessionKey(): void { /** @var User $user */