Extract job artifacts API code to a separate file

lib/api/api.rb

@@ -108,6 +108,7 @@ module API
     mount ::API::Internal
     mount ::API::Issues
     mount ::API::Jobs
+    mount ::API::JobArtifacts
     mount ::API::Keys
     mount ::API::Labels
     mount ::API::Lint

lib/api/helpers.rb

@@ -128,6 +128,10 @@ module API
       merge_request
     end
+    def find_build!(id)
+      user_project.builds.find(id.to_i)
+    end
+
     def authenticate!
       unauthorized! unless current_user && can?(initial_current_user, :access_api)
     end
@@ -160,6 +164,14 @@ module API
       authorize! :admin_project, user_project
     end
+    def authorize_read_builds!
+      authorize! :read_build, user_project
+    end
+
+    def authorize_update_builds!
+      authorize! :update_build, user_project
+    end
+
     def require_gitlab_workhorse!
       unless env['HTTP_GITLAB_WORKHORSE'].present?
         forbidden!('Request should be executed via GitLab Workhorse')

lib/api/job_artifacts.rb

+module API
+  class JobArtifacts < Grape::API
+    before { authenticate_non_get! }
+
+    params do
+      requires :id, type: String, desc: 'The ID of a project'
+    end
+    resource :projects, requirements: API::PROJECT_ENDPOINT_REQUIREMENTS do
+      desc 'Download the artifacts file from a job' do
+        detail 'This feature was introduced in GitLab 8.10'
+      end
+      params do
+        requires :ref_name, type: String, desc: 'The ref from repository'
+        requires :job,      type: String, desc: 'The name for the job'
+      end
+      get ':id/jobs/artifacts/:ref_name/download',
+        requirements: { ref_name: /.+/ } do
+        authorize_read_builds!
+
+        builds = user_project.latest_successful_builds_for(params[:ref_name])
+        latest_build = builds.find_by!(name: params[:job])
+
+        present_artifacts!(latest_build.artifacts_file)
+      end
+
+      desc 'Download the artifacts file from a job' do
+        detail 'This feature was introduced in GitLab 8.5'
+      end
+      params do
+        requires :job_id, type: Integer, desc: 'The ID of a job'
+      end
+      get ':id/jobs/:job_id/artifacts' do
+        authorize_read_builds!
+
+        build = find_build!(params[:job_id])
+
+        present_artifacts!(build.artifacts_file)
+      end
+
+      desc 'Download a specific file from artifacts archive' do
+        detail 'This feature was introduced in GitLab 10.0'
+      end
+      params do
+        requires :job_id, type: Integer, desc: 'The ID of a job'
+        requires :artifact_path, type: String, desc: 'Artifact path'
+      end
+      get ':id/jobs/:job_id/artifacts/*artifact_path', format: false do
+        authorize_read_builds!
+
+        build = find_build!(params[:job_id])
+        not_found! unless build.artifacts?
+
+        path = Gitlab::Ci::Build::Artifacts::Path
+          .new(params[:artifact_path])
+        not_found! unless path.valid?
+
+        send_artifacts_entry(build, path)
+      end
+
+      desc 'Keep the artifacts to prevent them from being deleted' do
+        success Entities::Job
+      end
+      params do
+        requires :job_id, type: Integer, desc: 'The ID of a job'
+      end
+      post ':id/jobs/:job_id/artifacts/keep' do
+        authorize_update_builds!
+
+        build = find_build!(params[:job_id])
+        authorize!(:update_build, build)
+        return not_found!(build) unless build.artifacts?
+
+        build.keep_artifacts!
+
+        status 200
+        present build, with: Entities::Job
+      end
+    end
+  end
+end

lib/api/jobs.rb

@@ -66,28 +66,11 @@ module API
       get ':id/jobs/:job_id' do
         authorize_read_builds!
-        build = get_build!(params[:job_id])
+        build = find_build!(params[:job_id])
         present build, with: Entities::Job
       end
-      desc 'Download the artifacts file from a job' do
-        detail 'This feature was introduced in GitLab 8.10'
-      end
-      params do
-        requires :ref_name, type: String, desc: 'The ref from repository'
-        requires :job,      type: String, desc: 'The name for the job'
-      end
-      get ':id/jobs/artifacts/:ref_name/download',
-        requirements: { ref_name: /.+/ } do
-        authorize_read_builds!
-
-        builds = user_project.latest_successful_builds_for(params[:ref_name])
-        latest_build = builds.find_by!(name: params[:job])
-
-        present_artifacts!(latest_build.artifacts_file)
-      end
-
       # TODO: We should use `present_file!` and leave this implementation for backward compatibility (when build trace
       #       is saved in the DB instead of file). But before that, we need to consider how to replace the value of
       #       `runners_token` with some mask (like `xxxxxx`) when sending trace file directly by workhorse.
@@ -98,7 +81,7 @@ module API
       get ':id/jobs/:job_id/trace' do
         authorize_read_builds!
-        build = get_build!(params[:job_id])
+        build = find_build!(params[:job_id])
         header 'Content-Disposition', "infile; filename=\"#{build.id}.log\""
         content_type 'text/plain'
@@ -117,7 +100,7 @@ module API
       post ':id/jobs/:job_id/cancel' do
         authorize_update_builds!
-        build = get_build!(params[:job_id])
+        build = find_build!(params[:job_id])
         authorize!(:update_build, build)
         build.cancel
@@ -134,7 +117,7 @@ module API
       post ':id/jobs/:job_id/retry' do
         authorize_update_builds!
-        build = get_build!(params[:job_id])
+        build = find_build!(params[:job_id])
         authorize!(:update_build, build)
         return forbidden!('Job is not retryable') unless build.retryable?
@@ -152,7 +135,7 @@ module API
       post ':id/jobs/:job_id/erase' do
         authorize_update_builds!
-        build = get_build!(params[:job_id])
+        build = find_build!(params[:job_id])
         authorize!(:update_build, build)
         return forbidden!('Job is not erasable!') unless build.erasable?
@@ -160,25 +143,6 @@ module API
         present build, with: Entities::Job
       end
-      desc 'Keep the artifacts to prevent them from being deleted' do
-        success Entities::Job
-      end
-      params do
-        requires :job_id, type: Integer, desc: 'The ID of a job'
-      end
-      post ':id/jobs/:job_id/artifacts/keep' do
-        authorize_update_builds!
-
-        build = get_build!(params[:job_id])
-        authorize!(:update_build, build)
-        return not_found!(build) unless build.artifacts?
-
-        build.keep_artifacts!
-
-        status 200
-        present build, with: Entities::Job
-      end
-
       desc 'Trigger a manual job' do
         success Entities::Job
         detail 'This feature was added in GitLab 8.11'
@@ -189,7 +153,7 @@ module API
       post ":id/jobs/:job_id/play" do
         authorize_read_builds!
-        build = get_build!(params[:job_id])
+        build = find_build!(params[:job_id])
         authorize!(:update_build, build)
         bad_request!("Unplayable Job") unless build.playable?
@@ -201,56 +165,7 @@ module API
       end
     end
-    params do
-      requires :id, type: String, desc: 'The ID of a project'
-    end
-    resource :projects, requirements: API::PROJECT_ENDPOINT_REQUIREMENTS do
-      before { authenticate_non_get! }
-
-      desc 'Download the artifacts file from a job' do
-        detail 'This feature was introduced in GitLab 8.5'
-      end
-      params do
-        requires :job_id, type: Integer, desc: 'The ID of a job'
-      end
-      get ':id/jobs/:job_id/artifacts' do
-        authorize_read_builds!
-
-        build = get_build!(params[:job_id])
-
-        present_artifacts!(build.artifacts_file)
-      end
-
-      desc 'Download a specific file from artifacts archive' do
-        detail 'This feature was introduced in GitLab 10.0'
-      end
-      params do
-        requires :job_id, type: Integer, desc: 'The ID of a job'
-        requires :artifact_path, type: String, desc: 'Artifact path'
-      end
-      get ':id/jobs/:job_id/artifacts/*artifact_path', format: false do
-        authorize_read_builds!
-
-        build = get_build!(params[:job_id])
-        not_found! unless build.artifacts?
-
-        path = Gitlab::Ci::Build::Artifacts::Path
-          .new(params[:artifact_path])
-        not_found! unless path.valid?
-
-        send_artifacts_entry(build, path)
-      end
-    end
-
     helpers do
-      def find_build(id)
-        user_project.builds.find_by(id: id.to_i)
-      end
-
-      def get_build!(id)
-        find_build(id) || not_found!
-      end
-
       def filter_builds(builds, scope)
         return builds if scope.nil? || scope.empty?
@@ -261,14 +176,6 @@ module API
         builds.where(status: available_statuses && scope)
       end
-
-      def authorize_read_builds!
-        authorize! :read_build, user_project
-      end
-
-      def authorize_update_builds!
-        authorize! :update_build, user_project
-      end
     end
   end
 end

spec/requests/api/jobs_spec.rb

@@ -321,8 +321,9 @@ describe API::Jobs do
         get_for_ref
       end
-      it 'gives 401' do
-        expect(response).to have_http_status(401)
+      it 'does not find a resource in a private project' do
+        expect(project).to be_private
+        expect(response).to have_http_status(404)
       end
     end