Improvements, readability for artifacts browser

7 files changed, 19 insertions, 17 deletions

diff --git a/app/controllers/projects/artifacts_controller.rb b/app/controllers/projects/artifacts_controller.rb
index a1f82ddd9c5..00daac0cb30 100644
--- a/app/controllers/projects/artifacts_controller.rb
+++ b/app/controllers/projects/artifacts_controller.rb
@@ -1,6 +1,6 @@
 class Projects::ArtifactsController < Projects::ApplicationController
   layout 'project'
-  before_action :authorize_download_build_artifacts!
+  before_action :authorize_read_build_artifacts!
 
   def download
     unless artifacts_file.file_storage?
@@ -24,11 +24,11 @@ class Projects::ArtifactsController < Projects::ApplicationController
   end
 
   def file
-    file = build.artifacts_metadata_path(params[:path])
+    file_path = build.artifacts_metadata_path(params[:path])
 
-    if file.exists?
-      render json: { repository: build.artifacts_file.path,
-                     path: Base64.encode64(file.path) }
+    if file_path.exists?
+      render json: { archive: build.artifacts_file.path,
+                     path: Base64.encode64(file_path.path) }
     else
       render json: {}, status: 404
     end
@@ -44,8 +44,8 @@ class Projects::ArtifactsController < Projects::ApplicationController
     @artifacts_file ||= build.artifacts_file
   end
 
-  def authorize_download_build_artifacts!
-    unless can?(current_user, :download_build_artifacts, @project)
+  def authorize_read_build_artifacts!
+    unless can?(current_user, :read_build_artifacts, @project)
       if current_user.nil?
         return authenticate_user!
       else
diff --git a/app/models/ability.rb b/app/models/ability.rb
index 5a1a67db8e1..5375148a654 100644
--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -175,7 +175,7 @@ class Ability
         :create_merge_request,
         :create_wiki,
         :manage_builds,
-        :download_build_artifacts,
+        :read_build_artifacts,
         :push_code
       ]
     end
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 7593ceda488..fef667f865e 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -344,7 +344,7 @@ module Ci
     end
 
     def artifacts_browser_supported?
-      artifacts? && artifacts_file.path.end_with?('zip') && artifacts_metadata.exists?
+      artifacts? && artifacts_metadata.exists?
     end
 
     def artifacts_metadata_path(path)
diff --git a/app/views/admin/builds/_build.html.haml b/app/views/admin/builds/_build.html.haml
index 26cdb162b76..c395bd908c3 100644
--- a/app/views/admin/builds/_build.html.haml
+++ b/app/views/admin/builds/_build.html.haml
@@ -60,7 +60,7 @@
 
   %td
     .pull-right
-      - if current_user && can?(current_user, :download_build_artifacts, project) && build.artifacts?
+      - if current_user && can?(current_user, :read_build_artifacts, project) && build.artifacts?
         = link_to build.artifacts_download_url, title: 'Download artifacts' do
           %i.fa.fa-download
       - if current_user && can?(current_user, :manage_builds, build.project)
diff --git a/app/views/projects/builds/show.html.haml b/app/views/projects/builds/show.html.haml
index 7cc127244dc..f393d818656 100644
--- a/app/views/projects/builds/show.html.haml
+++ b/app/views/projects/builds/show.html.haml
@@ -89,7 +89,7 @@
             Test coverage
           %h1 #{@build.coverage}%
 
-      - if current_user && can?(current_user, :download_build_artifacts, @project) && @build.artifacts?
+      - if current_user && can?(current_user, :read_build_artifacts, @project) && @build.artifacts?
 
         .build-widget.artifacts
           %h4.title Build artifacts
diff --git a/app/views/projects/commit_statuses/_commit_status.html.haml b/app/views/projects/commit_statuses/_commit_status.html.haml
index 7872eec72b0..1736dccaf3c 100644
--- a/app/views/projects/commit_statuses/_commit_status.html.haml
+++ b/app/views/projects/commit_statuses/_commit_status.html.haml
@@ -66,7 +66,7 @@
 
   %td
     .pull-right
-      - if current_user && can?(current_user, :download_build_artifacts, commit_status.project) && commit_status.artifacts?
+      - if current_user && can?(current_user, :read_build_artifacts, commit_status.project) && commit_status.artifacts?
         = link_to commit_status.artifacts_download_url, title: 'Download artifacts' do
           %i.fa.fa-download
       - if current_user && can?(current_user, :manage_builds, commit_status.project)
diff --git a/lib/gitlab/ci/build/artifacts/metadata.rb b/lib/gitlab/ci/build/artifacts/metadata.rb
index d9c051be9f3..47efc51a76e 100644
--- a/lib/gitlab/ci/build/artifacts/metadata.rb
+++ b/lib/gitlab/ci/build/artifacts/metadata.rb
@@ -56,7 +56,7 @@ module Gitlab
                 next if path =~ invalid_pattern
 
                 paths.push(path)
-                metadata.push(JSON.parse(meta.chomp, symbolize_names: true))
+                metadata.push(JSON.parse(meta, symbolize_names: true))
               rescue JSON::ParserError, Encoding::CompatibilityError
                 next
               end
@@ -66,7 +66,7 @@ module Gitlab
           end
 
           def read_version
-            gzip do|gz|
+            gzip do |gz|
               version_string = read_string(gz)
 
               unless version_string
@@ -95,9 +95,11 @@ module Gitlab
           def gzip
             open do |file|
               gzip = Zlib::GzipReader.new(file)
-              result = yield gzip
-              gzip.close
-              result
+              begin
+                yield gzip
+              ensure
+                gzip.close
+              end
             end
           end