Add specs for showing lfs object in UI.

13 files changed, 111 insertions, 21 deletions

diff --git a/app/controllers/projects/raw_controller.rb b/app/controllers/projects/raw_controller.rb
index c56f432a1f1..be7d5c187fe 100644
--- a/app/controllers/projects/raw_controller.rb
+++ b/app/controllers/projects/raw_controller.rb
@@ -38,18 +38,28 @@ class Projects::RawController < Projects::ApplicationController
     type = get_blob_type
 
     send_data(
-        @blob.data,
-        type: type,
-        disposition: 'inline'
-      )
+      @blob.data,
+      type: type,
+      disposition: 'inline'
+    )
   end
 
   def send_lfs_object
-    lfs_object = LfsObject.find_by_oid(@blob.lfs_oid)
-    return nil unless lfs_object && lfs_object.file.exists?
+    lfs_object = find_lfs_object
 
-    if lfs_object.projects.exists?(lfs_object.storage_project(@project).id)
+    if lfs_object && lfs_object.project_allowed_access?(@project)
       send_file lfs_object.file.path, filename: @blob.name, disposition: 'attachment'
+    else
+      render_404
+    end
+  end
+
+  def find_lfs_object
+    lfs_object = LfsObject.find_by_oid(@blob.lfs_oid)
+    if lfs_object && lfs_object.file.exists?
+      lfs_object
+    else
+      nil
     end
   end
 end
diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb
index df5f5fae23c..fa1b2522051 100644
--- a/app/helpers/blob_helper.rb
+++ b/app/helpers/blob_helper.rb
@@ -30,7 +30,7 @@ module BlobHelper
         nil
       end
 
-    if blob && blob.text?
+    if blob && blob.text? && !blob.lfs_pointer?
       text = 'Edit'
       after = options[:after] || ''
       from_mr = options[:from_merge_request_id]
@@ -71,4 +71,16 @@ module BlobHelper
   def blob_icon(mode, name)
     icon("#{file_type_icon_class('file', mode, name)} fw")
   end
+
+  def viewable?(blob)
+    blob.text? && !blob.lfs_pointer?
+  end
+
+  def blob_size(blob)
+    if blob.lfs_pointer?
+      blob.lfs_size
+    else
+      blob.size
+    end
+  end
 end
diff --git a/app/helpers/tree_helper.rb b/app/helpers/tree_helper.rb
index 03a49e119b8..6afa1aacc5b 100644
--- a/app/helpers/tree_helper.rb
+++ b/app/helpers/tree_helper.rb
@@ -54,6 +54,10 @@ module TreeHelper
     ::Gitlab::GitAccess.new(current_user, project).can_push_to_branch?(ref)
   end
 
+  def can_delete_or_replace?(blob)
+    allowed_tree_edit? && !blob.lfs_pointer?
+  end
+
   def tree_breadcrumbs(tree, max_links = 2)
     if @path.present?
       part_path = ""
diff --git a/app/models/lfs_object.rb b/app/models/lfs_object.rb
index a243c7b77cc..18657c3e1c8 100644
--- a/app/models/lfs_object.rb
+++ b/app/models/lfs_object.rb
@@ -13,4 +13,8 @@ class LfsObject < ActiveRecord::Base
       project
     end
   end
+
+  def project_allowed_access?(project)
+    projects.exists?(storage_project(project).id)
+  end
 end
diff --git a/app/views/projects/blob/_actions.html.haml b/app/views/projects/blob/_actions.html.haml
index 15cd8f056f5..c31f6442f12 100644
--- a/app/views/projects/blob/_actions.html.haml
+++ b/app/views/projects/blob/_actions.html.haml
@@ -1,9 +1,9 @@
 .btn-group.tree-btn-group
-  = edit_blob_link(@project, @ref, @path) unless @blob.lfs_pointer?
+  = edit_blob_link(@project, @ref, @path)
   = link_to 'Raw', namespace_project_raw_path(@project.namespace, @project, @id),
       class: 'btn btn-sm', target: '_blank'
   -# only show normal/blame view links for text files
-  - if @blob.text? && !@blob.lfs_pointer?
+  - if viewable?(@blob)
     - if current_page? namespace_project_blame_path(@project.namespace, @project, @id)
       = link_to 'Normal View', namespace_project_blob_path(@project.namespace, @project, @id),
           class: 'btn btn-sm'
@@ -16,7 +16,7 @@
     = link_to 'Permalink', namespace_project_blob_path(@project.namespace, @project,
         tree_join(@commit.sha, @path)), class: 'btn btn-sm'
 
-- if allowed_tree_edit? && !@blob.lfs_pointer?
+- if can_delete_or_replace?(@blob)
   .btn-group{ role: "group" }
     %button.btn.btn-default{ 'data-target' => '#modal-upload-blob', 'data-toggle' => 'modal' } Replace
     %button.btn.btn-remove{ 'data-target' => '#modal-remove-blob', 'data-toggle' => 'modal' } Delete
diff --git a/app/views/projects/blob/_blob.html.haml b/app/views/projects/blob/_blob.html.haml
index bb9e1c63413..2a3315da3db 100644
--- a/app/views/projects/blob/_blob.html.haml
+++ b/app/views/projects/blob/_blob.html.haml
@@ -29,7 +29,7 @@
       %strong
         = blob.name
       %small
-        = number_to_human_size(blob.size) unless blob.lfs_pointer?
+        = number_to_human_size(blob_size(blob))
       .file-actions.hidden-xs
         = render "actions"
     - if blob.lfs_pointer?
diff --git a/app/views/projects/blob/_download.html.haml b/app/views/projects/blob/_download.html.haml
index 39ec6f693e2..7908fcae3de 100644
--- a/app/views/projects/blob/_download.html.haml
+++ b/app/views/projects/blob/_download.html.haml
@@ -4,8 +4,4 @@
       %h1.light
         %i.fa.fa-download
       %h4
-        - if blob.lfs_pointer?
-          - size = blob.lfs_size
-        - else
-          - size = blob.size
-        Download (#{number_to_human_size size})
+        Download (#{number_to_human_size blob_size(blob)})
diff --git a/app/views/projects/blob/show.html.haml b/app/views/projects/blob/show.html.haml
index b7276868ce6..09d6fc18e3e 100644
--- a/app/views/projects/blob/show.html.haml
+++ b/app/views/projects/blob/show.html.haml
@@ -6,7 +6,7 @@
 %div#tree-holder.tree-holder
   = render 'blob', blob: @blob
 
-- if allowed_tree_edit?
+- if can_delete_or_replace?(@blob)
   = render 'projects/blob/remove'
 
   - title = "Replace #{@blob.name}"
diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml
index ec1c665716e..f6ba64d31b5 100644
--- a/app/views/projects/diffs/_file.html.haml
+++ b/app/views/projects/diffs/_file.html.haml
@@ -25,7 +25,7 @@
             = "#{diff_file.diff.a_mode} → #{diff_file.diff.b_mode}"
 
       .diff-controls
-        - if blob.text? && !blob.lfs_pointer?
+        - if viewable?(blob)
           = link_to '#', class: 'js-toggle-diff-comments btn btn-sm active has_tooltip', title: "Toggle comments for this file" do
             %i.fa.fa-comments
           &nbsp;
@@ -40,7 +40,7 @@
   .diff-content.diff-wrap-lines
     -# Skipp all non non-supported blobs
     - return unless blob.respond_to?('text?')
-    - if blob.text? && !blob.lfs_pointer?
+    - if viewable?(blob)
       - if diff_view == 'parallel'
         = render "projects/diffs/parallel_view", diff_file: diff_file, project: project, blob: blob, index: i
       - else
diff --git a/features/project/source/browse_files.feature b/features/project/source/browse_files.feature
index e545ea63ca8..37f99b37619 100644
--- a/features/project/source/browse_files.feature
+++ b/features/project/source/browse_files.feature
@@ -221,3 +221,9 @@ Feature: Project Source Browse Files
     Given I switch ref to fix
     And I visit the fix tree
     Then I see the commit data for a directory with a leading dot
+
+  Scenario: I browse LFS object
+    Given I click on "files/lfs/lfs_object.iso" file in repo
+    Then I should see download link and object size
+    And I should not see lfs pointer details
+    And I should see buttons for allowed commands
diff --git a/features/steps/project/source/browse_files.rb b/features/steps/project/source/browse_files.rb
index ceab23b9a4d..2edbca9b7fe 100644
--- a/features/steps/project/source/browse_files.rb
+++ b/features/steps/project/source/browse_files.rb
@@ -305,6 +305,32 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps
     expect(page).not_to have_content('Loading commit data...')
   end
 
+  step 'I click on "files/lfs/lfs_object.iso" file in repo' do
+    click_link 'files'
+    click_link "lfs"
+    click_link "lfs_object.iso"
+  end
+
+  step 'I should see download link and object size' do
+    expect(page).to have_content 'Download (1.5 MB)'
+  end
+
+  step 'I should not see lfs pointer details' do
+    expect(page).not_to have_content 'version https://git-lfs.github.com/spec/v1'
+    expect(page).not_to have_content 'oid sha256:91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897'
+    expect(page).not_to have_content 'size 1575078'
+  end
+
+  step 'I should see buttons for allowed commands' do
+    expect(page).to have_content 'Raw'
+    expect(page).to have_content 'History'
+    expect(page).to have_content 'Permalink'
+    expect(page).not_to have_content 'Edit'
+    expect(page).not_to have_content 'Blame'
+    expect(page).not_to have_content 'Delete'
+    expect(page).not_to have_content 'Replace'
+  end
+
   private
 
   def set_new_content
diff --git a/spec/controllers/projects/raw_controller_spec.rb b/spec/controllers/projects/raw_controller_spec.rb
index c114f342021..2329c246daa 100644
--- a/spec/controllers/projects/raw_controller_spec.rb
+++ b/spec/controllers/projects/raw_controller_spec.rb
@@ -33,5 +33,37 @@ describe Projects::RawController do
         expect(response.header['Content-Type']).to eq('image/jpeg')
       end
     end
+
+    context 'lfs object' do
+      let(:id) { 'master/files/lfs/lfs_object.iso' }
+      let!(:lfs_object) { create(:lfs_object, oid: '91eff75a492a3ed0dfcb544d7f31326bc4014c8551849c192fd1e48d4dd2c897', size: '1575078') }
+
+      context 'when project has access' do
+        before do
+          public_project.lfs_objects << lfs_object
+        end
+
+        it 'serves the file' do
+          get(:show,
+              namespace_id: public_project.namespace.to_param,
+              project_id: public_project.to_param,
+              id: id)
+
+          expect(response.status).to eq(200)
+          expect(response.header['Content-Type']).to eq('application/octet-stream')
+        end
+      end
+
+      context 'when project does not have access' do
+        it 'does not serve the file' do
+          get(:show,
+              namespace_id: public_project.namespace.to_param,
+              project_id: public_project.to_param,
+              id: id)
+
+          expect(response.status).to eq(404)
+        end
+      end
+    end
   end
 end
diff --git a/spec/support/test_env.rb b/spec/support/test_env.rb
index 787670e9297..47c67475cd0 100644
--- a/spec/support/test_env.rb
+++ b/spec/support/test_env.rb
@@ -12,7 +12,7 @@ module TestEnv
     'fix'              => '48f0be4',
     'improve/awesome'  => '5937ac0',
     'markdown'         => '0ed8c6c',
-    'master'           => '5937ac0',
+    'master'           => 'be93687',
     "'test'"           => 'e56497b',
   }