Remove web session authentication in API and share code relative to t…

…he stream controllers in concerns.

app/controllers/albums_controller.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 class AlbumsController < ApplicationController
-  include Playable
+  include PlayableConcern
 
   before_action :require_admin, only: [:update]
   before_action :find_album, except: [:index]

app/controllers/api/v1/api_controller.rb

@@ -7,14 +7,8 @@ class ApiController < ApplicationController
 
       private
 
-      # If user already has logged in then authenticate with current session,
-      # otherwise authenticate with api token.
       def find_current_user
-        Current.user = UserSession.find&.user
-
-        return if logged_in?
-
-        authenticate_with_http_token do |token, options|
+        authenticate_with_http_token do |token, _|
           user = User.find_by(api_token: token)
           return unless user.present?
 

app/controllers/api/v1/stream_controller.rb

@@ -3,45 +3,7 @@
 module Api
   module V1
     class StreamController < ApiController
-      before_action :find_stream
-
-      def new
-        send_local_file @stream.file_path
-      end
-
-      private
-
-      def set_nginx_header
-        # Let nginx can get value of media_path dynamically in the nginx config,
-        # when use X-Accel-Redirect header to send file.
-        response.headers["X-Media-Path"] = Setting.media_path
-        response.headers["X-Accel-Redirect"] = File.join("/private_media", @stream.file_path.sub(File.expand_path(Setting.media_path), ""))
-      end
-
-      def find_stream
-        song = Song.find(params[:song_id])
-        @stream = Stream.new(song)
-      end
-
-      def send_local_file(file_path)
-        if BlackCandy::Config.nginx_sendfile?
-          set_nginx_header
-
-          send_file file_path
-          return
-        end
-
-        # Use Rack::File to support HTTP range without nginx. see https://github.com/rails/rails/issues/32193
-        Rack::File.new(nil).serving(request, file_path).tap do |(status, headers, body)|
-          self.status = status
-          self.response_body = body
-
-          headers.each { |name, value| response.headers[name] = value }
-
-          response.headers["Content-Type"] = Mime[@stream.format]
-          response.headers["Content-Disposition"] = "attachment"
-        end
-      end
+      include StreamConcern
     end
   end
 end

app/controllers/api/v1/transcoded_stream_controller.rb

@@ -2,49 +2,8 @@
 
 module Api
   module V1
-    class TranscodedStreamController < StreamController
-      before_action :find_cache
-
-      # Similar to send_file in rails, but let response_body to be a stream object.
-      # The instance of Stream can respond to each() method. So the download can be streamed,
-      # instead of read whole data into memory.
-      def new
-        # Because the module ActionController::Live contains methods that can block send file.
-        # So can't simply include ActionController::Live in class, otherwise include this module only on this method.
-        self.class.send(:include, ActionController::Live)
-
-        response.headers["Content-Type"] = Mime[Stream::TRANSCODE_FORMAT]
-
-        send_stream(filename: "#{@stream.name}.mp3") do |stream_response|
-          File.open(@stream.transcode_cache_file_path, "w") do |file|
-            @stream.each do |data|
-              stream_response.write data
-              file.write data
-            end
-          end
-        end
-      end
-
-      private
-
-      def set_nginx_header
-        response.headers["X-Accel-Redirect"] = File.join("/private_cache_media", @stream.transcode_cache_file_path.sub(Stream::TRANSCODE_CACHE_DIRECTORY.to_s, ""))
-      end
-
-      def find_cache
-        return unless valid_cache?
-        send_local_file @stream.transcode_cache_file_path
-      end
-
-      def valid_cache?
-        return unless File.exist?(@stream.transcode_cache_file_path)
-
-        # Compare duration of cache file and original file to check integrity of cache file.
-        # Because the different format of the file, the duration will have a little difference,
-        # so the duration difference in two seconds are considered no problem.
-        cache_file_tag = WahWah.open(@stream.transcode_cache_file_path)
-        (@stream.duration - cache_file_tag.duration).abs <= 2
-      end
+    class TranscodedStreamController < ApiController
+      include TranscodedStreamConcern
     end
   end
 end

app/controllers/application_controller.rb

@@ -116,4 +116,24 @@ def turbo_android?
   def render_json_error(error, status)
     render json: {type: error.type, message: error.message}, status: status
   end
+
+  def send_local_file(file_path, format, nginx_headers: {})
+    if BlackCandy::Config.nginx_sendfile?
+      nginx_headers.each { |name, value| response.headers[name] = value }
+      send_file file_path
+
+      return
+    end
+
+    # Use Rack::File to support HTTP range without nginx. see https://github.com/rails/rails/issues/32193
+    Rack::File.new(nil).serving(request, file_path).tap do |(status, headers, body)|
+      self.status = status
+      self.response_body = body
+
+      headers.each { |name, value| response.headers[name] = value }
+
+      response.headers["Content-Type"] = Mime[format]
+      response.headers["Content-Disposition"] = "attachment"
+    end
+  end
 end

app/controllers/concerns/playable.rb → app/controllers/concerns/playable_concern.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-module Playable
+module PlayableConcern
   extend ActiveSupport::Concern
 
   included do

app/controllers/concerns/stream_concern.rb

@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+
+module StreamConcern
+  extend ActiveSupport::Concern
+
+  included do
+    before_action :find_stream
+  end
+
+  def new
+    send_local_file @stream.file_path, @stream.format, nginx_headers: {
+      # Let nginx can get value of media_path dynamically in the nginx config
+      # when use X-Accel-Redirect header to send file.
+      "X-Media-Path" => Setting.media_path,
+      "X-Accel-Redirect" => File.join("/private_media", @stream.file_path.sub(File.expand_path(Setting.media_path), ""))
+    }
+  end
+
+  private
+
+  def find_stream
+    @stream = Stream.new(Song.find(params[:song_id]))
+  end
+end

app/controllers/concerns/transcoded_stream_concern.rb

@@ -0,0 +1,53 @@
+# frozen_string_literal: true
+
+module TranscodedStreamConcern
+  extend ActiveSupport::Concern
+
+  included do
+    before_action :find_stream
+    before_action :find_cache
+  end
+
+  # Similar to send_file in rails, but let response_body to be a stream object.
+  # The instance of Stream can respond to each() method. So the download can be streamed,
+  # instead of read whole data into memory.
+  def new
+    # Because the module ActionController::Live contains methods that can block send file.
+    # So can't simply include ActionController::Live in class, otherwise include this module only on this method.
+    self.class.send(:include, ActionController::Live)
+
+    response.headers["Content-Type"] = Mime[Stream::TRANSCODE_FORMAT]
+
+    send_stream(filename: "#{@stream.name}.mp3") do |stream_response|
+      File.open(@stream.transcode_cache_file_path, "w") do |file|
+        @stream.each do |data|
+          stream_response.write data
+          file.write data
+        end
+      end
+    end
+  end
+
+  private
+
+  def find_stream
+    @stream = Stream.new(Song.find(params[:song_id]))
+  end
+
+  def find_cache
+    return unless valid_cache?
+    send_local_file @stream.transcode_cache_file_path, @stream.format, nginx_headers: {
+      "X-Accel-Redirect" => File.join("/private_cache_media", @stream.transcode_cache_file_path.sub(Stream::TRANSCODE_CACHE_DIRECTORY.to_s, ""))
+    }
+  end
+
+  def valid_cache?
+    return unless File.exist?(@stream.transcode_cache_file_path)
+
+    # Compare duration of cache file and original file to check integrity of cache file.
+    # Because the different format of the file, the duration will have a little difference,
+    # so the duration difference in two seconds are considered no problem.
+    cache_file_tag = WahWah.open(@stream.transcode_cache_file_path)
+    (@stream.duration - cache_file_tag.duration).abs <= 2
+  end
+end

app/controllers/playlists/songs_controller.rb

@@ -4,7 +4,7 @@ class Playlists::SongsController < ApplicationController
   before_action :find_playlist
   before_action :find_song, only: [:create, :destroy]
 
-  include Playable
+  include PlayableConcern
 
   def show
     @pagy, @songs = pagy(@playlist.songs.includes(:artist))

app/controllers/stream_controller.rb

@@ -0,0 +1,5 @@
+# frozen_string_literal: true
+
+class StreamController < ApplicationController
+  include StreamConcern
+end

app/controllers/transcoded_stream_controller.rb

@@ -0,0 +1,5 @@
+# frozen_string_literal: true
+
+class TranscodedStreamController < ApplicationController
+  include TranscodedStreamConcern
+end

app/helpers/song_helper.rb

@@ -1,10 +1,13 @@
 # frozen_string_literal: true
 
 module SongHelper
-  def song_json_builder(song)
+  def song_json_builder(song, for_api: false)
+    stream_url = for_api ? new_api_v1_stream_url(song_id: song.id) : new_stream_url(song_id: song.id)
+    transcoded_stream_url = for_api ? new_api_v1_transcoded_stream_url(song_id: song.id) : new_transcoded_stream_url(song_id: song.id)
+
     Jbuilder.new do |json|
       json.call(song, :id, :name, :duration)
-      json.url need_transcode?(song) ? new_api_v1_transcoded_stream_url(song_id: song.id) : new_api_v1_stream_url(song_id: song.id)
+      json.url need_transcode?(song) ? transcoded_stream_url : stream_url
       json.album_name song.album.title
       json.artist_name song.artist.title
       json.is_favorited song.is_favorited.nil? ? Current.user.favorited?(song) : song.is_favorited

app/views/api/v1/songs/_song.json.jbuilder

@@ -1 +1 @@
-json.merge! song_json_builder(song)
+json.merge! song_json_builder(song, for_api: true)

config/routes.rb

@@ -60,6 +60,9 @@
     end
   end
 
+  resources :stream, only: [:new]
+  resources :transcoded_stream, only: [:new]
+
   get "/403", to: "errors#forbidden", as: :forbidden
   get "/404", to: "errors#not_found", as: :not_found
   get "/422", to: "errors#unprocessable_entity", as: :unprocessable_entity

test/controllers/api/v1/api_controller_test.rb

@@ -8,11 +8,11 @@ class Api::V1::ApiControllerTest < ActionDispatch::IntegrationTest
     @song = songs(:mp3_sample)
   end
 
-  test "should authenticate when have user session" do
+  test "should not authenticate when have user session" do
     login(@user)
     get api_v1_song_url(@song), as: :json
 
-    assert_response :success
+    assert_response :unauthorized
   end
 
   test "should authenticate when have api token" do
@@ -21,7 +21,7 @@ class Api::V1::ApiControllerTest < ActionDispatch::IntegrationTest
     assert_response :success
   end
 
-  test "should not authenticate when do not have user seesion or api token" do
+  test "should not authenticate when do not have api token" do
     get api_v1_song_url(@song), as: :json
 
     assert_response :unauthorized