Support uploads for groupsjk-group-files-upload

# Conflicts:
#	config/routes/group.rb
#	lib/banzai/filter/upload_link_filter.rb
#	spec/lib/banzai/filter/upload_link_filter_spec.rb

11 files changed, 331 insertions, 266 deletions

diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb
index dec2e27335a..223fcc06562 100644
--- a/app/controllers/concerns/uploads_actions.rb
+++ b/app/controllers/concerns/uploads_actions.rb
@@ -24,4 +24,28 @@ module UploadsActions
 
     send_file uploader.file.path, disposition: disposition
   end
+
+  private
+
+  def uploader
+    return @uploader if defined?(@uploader)
+
+    if show_model.nil?
+      @uploader = nil
+      return
+    end
+
+    @uploader = FileUploader.new(show_model, params[:secret])
+    @uploader.retrieve_from_store!(params[:filename])
+
+    @uploader
+  end
+
+  def image_or_video?
+    uploader && uploader.exists? && uploader.image_or_video?
+  end
+
+  def uploader_class
+    FileUploader
+  end
 end
diff --git a/app/controllers/groups/uploads_controller.rb b/app/controllers/groups/uploads_controller.rb
new file mode 100644
index 00000000000..aa1bfe8264f
--- /dev/null
+++ b/app/controllers/groups/uploads_controller.rb
@@ -0,0 +1,17 @@
+class Groups::UploadsController < Groups::ApplicationController
+  skip_before_action :group, if: -> { action_name == 'show' && image_or_video? }
+
+  include UploadsActions
+
+  private
+
+  def show_model
+    return @show_model if defined?(@show_model)
+
+    group_id = params[:group_id]
+
+    @show_model = Group.find_by_full_path(group_id)
+  end
+
+  alias_method :model, :group
+end
diff --git a/app/controllers/projects/uploads_controller.rb b/app/controllers/projects/uploads_controller.rb
index 4d2fb17a19b..81a7fb105b0 100644
--- a/app/controllers/projects/uploads_controller.rb
+++ b/app/controllers/projects/uploads_controller.rb
@@ -8,31 +8,13 @@ class Projects::UploadsController < Projects::ApplicationController
 
   private
 
-  def uploader
-    return @uploader if defined?(@uploader)
+  def show_model
+    return @show_model if defined?(@show_model)
 
     namespace = params[:namespace_id]
     id = params[:project_id]
 
-    file_project = Project.find_by_full_path("#{namespace}/#{id}")
-
-    if file_project.nil?
-      @uploader = nil
-      return
-    end
-
-    @uploader = FileUploader.new(file_project, params[:secret])
-    @uploader.retrieve_from_store!(params[:filename])
-
-    @uploader
-  end
-
-  def image_or_video?
-    uploader && uploader.exists? && uploader.image_or_video?
-  end
-
-  def uploader_class
-    FileUploader
+    @show_model = Project.find_by_full_path("#{namespace}/#{id}")
   end
 
   alias_method :model, :project
diff --git a/app/models/group.rb b/app/models/group.rb
index 8cf632fb566..66d07b32ba2 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -303,6 +303,10 @@ class Group < Namespace
     "#{parent.full_path}/#{path_was}"
   end
 
+  def disk_path
+    full_path
+  end
+
   private
 
   def update_two_factor_requirement
diff --git a/app/views/layouts/group.html.haml b/app/views/layouts/group.html.haml
index 08bd6fc311e..bfbfeee7c4b 100644
--- a/app/views/layouts/group.html.haml
+++ b/app/views/layouts/group.html.haml
@@ -4,4 +4,10 @@
 - nav            "group"
 - @left_sidebar = true
 
+- content_for :page_specific_javascripts do
+  - if current_user
+    -# haml-lint:disable InlineJavaScript
+    :javascript
+      window.uploads_path = "#{group_uploads_path(@group)}";
+
 = render template: "layouts/application"
diff --git a/config/routes/group.rb b/config/routes/group.rb
index db99e10bb9a..976837a246d 100644
--- a/config/routes/group.rb
+++ b/config/routes/group.rb
@@ -49,6 +49,12 @@ constraints(GroupUrlConstrainer.new) do
       post :resend_invite, on: :member
       delete :leave, on: :collection
     end
+
+    resources :uploads, only: [:create] do
+      collection do
+        get ":secret/:filename", action: :show, as: :show, constraints: { filename: /[^\/]+/ }
+      end
+    end
   end
 
   scope(path: '*id',
diff --git a/lib/banzai/filter/upload_link_filter.rb b/lib/banzai/filter/upload_link_filter.rb
index 09844931be5..d061d1be5d0 100644
--- a/lib/banzai/filter/upload_link_filter.rb
+++ b/lib/banzai/filter/upload_link_filter.rb
@@ -8,7 +8,7 @@ module Banzai
     #
     class UploadLinkFilter < HTML::Pipeline::Filter
       def call
-        return doc unless project
+        return doc unless project || group
 
         doc.xpath('descendant-or-self::a[starts-with(@href, "/uploads/")]').each do |el|
           process_link_attr el.attribute('href')
@@ -28,13 +28,24 @@ module Banzai
       end
 
       def build_url(uri)
-        File.join(Gitlab.config.gitlab.url, project.full_path, uri)
+        base_path = Gitlab.config.gitlab.url
+        model_path = if group
+                       File.join('groups', group.full_path, '-')
+                     else
+                       project.full_path
+                     end
+
+        File.join(base_path, model_path, uri)
       end
 
       def project
         context[:project]
       end
 
+      def group
+        context[:group]
+      end
+
       # Ensure that a :project key exists in context
       #
       # Note that while the key might exist, its value could be nil!
diff --git a/spec/controllers/groups/uploads_controller_spec.rb b/spec/controllers/groups/uploads_controller_spec.rb
new file mode 100644
index 00000000000..56c56369ee4
--- /dev/null
+++ b/spec/controllers/groups/uploads_controller_spec.rb
@@ -0,0 +1,10 @@
+require 'spec_helper'
+
+describe Groups::UploadsController do
+  let(:model) { create(:group) }
+  let(:params) do
+    { group_id: model }
+  end
+
+  it_behaves_like 'handle uploads'
+end
diff --git a/spec/controllers/projects/uploads_controller_spec.rb b/spec/controllers/projects/uploads_controller_spec.rb
index c2550b1efa7..0285c502258 100644
--- a/spec/controllers/projects/uploads_controller_spec.rb
+++ b/spec/controllers/projects/uploads_controller_spec.rb
@@ -1,247 +1,10 @@
-require('spec_helper')
+require 'spec_helper'
 
 describe Projects::UploadsController do
-  let(:project) { create(:project) }
-  let(:user)    { create(:user) }
-  let(:jpg)     { fixture_file_upload(Rails.root + 'spec/fixtures/rails_sample.jpg', 'image/jpg') }
-  let(:txt)     { fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain') }
-
-  describe "POST #create" do
-    before do
-      sign_in(user)
-      project.team << [user, :developer]
-    end
-
-    context "without params['file']" do
-      it "returns an error" do
-        post :create,
-          namespace_id: project.namespace.to_param,
-          project_id: project,
-          format: :json
-        expect(response).to have_gitlab_http_status(422)
-      end
-    end
-
-    context 'with valid image' do
-      before do
-        post :create,
-          namespace_id: project.namespace.to_param,
-          project_id: project,
-          file: jpg,
-          format: :json
-      end
-
-      it 'returns a content with original filename, new link, and correct type.' do
-        expect(response.body).to match '\"alt\":\"rails_sample\"'
-        expect(response.body).to match "\"url\":\"/uploads"
-      end
-
-      # NOTE: This is as close as we're getting to an Integration test for this
-      # behavior. We're avoiding a proper Feature test because those should be
-      # testing things entirely user-facing, which the Upload model is very much
-      # not.
-      it 'creates a corresponding Upload record' do
-        upload = Upload.last
-
-        aggregate_failures do
-          expect(upload).to exist
-          expect(upload.model).to eq project
-        end
-      end
-    end
-
-    context 'with valid non-image file' do
-      before do
-        post :create,
-          namespace_id: project.namespace.to_param,
-          project_id: project,
-          file: txt,
-          format: :json
-      end
-
-      it 'returns a content with original filename, new link, and correct type.' do
-        expect(response.body).to match '\"alt\":\"doc_sample.txt\"'
-        expect(response.body).to match "\"url\":\"/uploads"
-      end
-    end
+  let(:model) { create(:project) }
+  let(:params) do
+    { namespace_id: model.namespace.to_param, project_id: model }
   end
 
-  describe "GET #show" do
-    let(:go) do
-      get :show,
-        namespace_id: project.namespace.to_param,
-        project_id:   project,
-        secret:       "123456",
-        filename:     "image.jpg"
-    end
-
-    context "when the project is public" do
-      before do
-        project.update_attribute(:visibility_level, Project::PUBLIC)
-      end
-
-      context "when not signed in" do
-        context "when the file exists" do
-          before do
-            allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
-            allow(jpg).to receive(:exists?).and_return(true)
-          end
-
-          it "responds with status 200" do
-            go
-
-            expect(response).to have_gitlab_http_status(200)
-          end
-        end
-
-        context "when the file doesn't exist" do
-          it "responds with status 404" do
-            go
-
-            expect(response).to have_gitlab_http_status(404)
-          end
-        end
-      end
-
-      context "when signed in" do
-        before do
-          sign_in(user)
-        end
-
-        context "when the file exists" do
-          before do
-            allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
-            allow(jpg).to receive(:exists?).and_return(true)
-          end
-
-          it "responds with status 200" do
-            go
-
-            expect(response).to have_gitlab_http_status(200)
-          end
-        end
-
-        context "when the file doesn't exist" do
-          it "responds with status 404" do
-            go
-
-            expect(response).to have_gitlab_http_status(404)
-          end
-        end
-      end
-    end
-
-    context "when the project is private" do
-      before do
-        project.update_attribute(:visibility_level, Project::PRIVATE)
-      end
-
-      context "when not signed in" do
-        context "when the file exists" do
-          before do
-            allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
-            allow(jpg).to receive(:exists?).and_return(true)
-          end
-
-          context "when the file is an image" do
-            before do
-              allow_any_instance_of(FileUploader).to receive(:image?).and_return(true)
-            end
-
-            it "responds with status 200" do
-              go
-
-              expect(response).to have_gitlab_http_status(200)
-            end
-          end
-
-          context "when the file is not an image" do
-            it "redirects to the sign in page" do
-              go
-
-              expect(response).to redirect_to(new_user_session_path)
-            end
-          end
-        end
-
-        context "when the file doesn't exist" do
-          it "redirects to the sign in page" do
-            go
-
-            expect(response).to redirect_to(new_user_session_path)
-          end
-        end
-      end
-
-      context "when signed in" do
-        before do
-          sign_in(user)
-        end
-
-        context "when the user has access to the project" do
-          before do
-            project.team << [user, :master]
-          end
-
-          context "when the file exists" do
-            before do
-              allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
-              allow(jpg).to receive(:exists?).and_return(true)
-            end
-
-            it "responds with status 200" do
-              go
-
-              expect(response).to have_gitlab_http_status(200)
-            end
-          end
-
-          context "when the file doesn't exist" do
-            it "responds with status 404" do
-              go
-
-              expect(response).to have_gitlab_http_status(404)
-            end
-          end
-        end
-
-        context "when the user doesn't have access to the project" do
-          context "when the file exists" do
-            before do
-              allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
-              allow(jpg).to receive(:exists?).and_return(true)
-            end
-
-            context "when the file is an image" do
-              before do
-                allow_any_instance_of(FileUploader).to receive(:image?).and_return(true)
-              end
-
-              it "responds with status 200" do
-                go
-
-                expect(response).to have_gitlab_http_status(200)
-              end
-            end
-
-            context "when the file is not an image" do
-              it "responds with status 404" do
-                go
-
-                expect(response).to have_gitlab_http_status(404)
-              end
-            end
-          end
-
-          context "when the file doesn't exist" do
-            it "responds with status 404" do
-              go
-
-              expect(response).to have_gitlab_http_status(404)
-            end
-          end
-        end
-      end
-    end
-  end
+  it_behaves_like 'handle uploads'
 end
diff --git a/spec/lib/banzai/filter/upload_link_filter_spec.rb b/spec/lib/banzai/filter/upload_link_filter_spec.rb
index 60a88e903ef..b317da3b539 100644
--- a/spec/lib/banzai/filter/upload_link_filter_spec.rb
+++ b/spec/lib/banzai/filter/upload_link_filter_spec.rb
@@ -89,7 +89,19 @@ describe Banzai::Filter::UploadLinkFilter do
     end
   end
 
-  context 'when project context does not exist' do
+  context 'in group context' do
+    let(:upload_link) { link('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg') }
+    let(:group) { create(:group) }
+
+    it 'rewrites the link correctly' do
+      doc = raw_filter(upload_link, project: nil, group: group)
+
+      expect(doc.at_css('a')['href'])
+        .to eq "#{Gitlab.config.gitlab.url}/groups/#{group.full_path}/-/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg"
+    end
+  end
+
+  context 'when project or group context does not exist' do
     let(:upload_link) { link('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg') }
 
     it 'does not raise error' do
diff --git a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb
new file mode 100644
index 00000000000..489ab6a3e94
--- /dev/null
+++ b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb
@@ -0,0 +1,230 @@
+shared_examples 'handle uploads' do
+  let(:user)  { create(:user) }
+  let(:jpg)   { fixture_file_upload(Rails.root + 'spec/fixtures/rails_sample.jpg', 'image/jpg') }
+  let(:txt)   { fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain') }
+
+  describe "POST #create" do
+    before do
+      sign_in(user)
+      model.add_developer(user)
+    end
+
+    context "without params['file']" do
+      it "returns an error" do
+        post :create, params.merge(format: :json)
+
+        expect(response).to have_gitlab_http_status(422)
+      end
+    end
+
+    context 'with valid image' do
+      before do
+        post :create, params.merge(file: jpg, format: :json)
+      end
+
+      it 'returns a content with original filename, new link, and correct type.' do
+        expect(response.body).to match '\"alt\":\"rails_sample\"'
+        expect(response.body).to match "\"url\":\"/uploads"
+      end
+
+      # NOTE: This is as close as we're getting to an Integration test for this
+      # behavior. We're avoiding a proper Feature test because those should be
+      # testing things entirely user-facing, which the Upload model is very much
+      # not.
+      it 'creates a corresponding Upload record' do
+        upload = Upload.last
+
+        aggregate_failures do
+          expect(upload).to exist
+          expect(upload.model).to eq(model)
+        end
+      end
+    end
+
+    context 'with valid non-image file' do
+      before do
+        post :create, params.merge(file: txt, format: :json)
+      end
+
+      it 'returns a content with original filename, new link, and correct type.' do
+        expect(response.body).to match '\"alt\":\"doc_sample.txt\"'
+        expect(response.body).to match "\"url\":\"/uploads"
+      end
+    end
+  end
+
+  describe "GET #show" do
+    let(:show_upload) do
+      get :show, params.merge(secret: "123456", filename: "image.jpg")
+    end
+
+    context "when the model is public" do
+      before do
+        model.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC)
+      end
+
+      context "when not signed in" do
+        context "when the file exists" do
+          before do
+            allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
+            allow(jpg).to receive(:exists?).and_return(true)
+          end
+
+          it "responds with status 200" do
+            show_upload
+
+            expect(response).to have_gitlab_http_status(200)
+          end
+        end
+
+        context "when the file doesn't exist" do
+          it "responds with status 404" do
+            show_upload
+
+            expect(response).to have_gitlab_http_status(404)
+          end
+        end
+      end
+
+      context "when signed in" do
+        before do
+          sign_in(user)
+        end
+
+        context "when the file exists" do
+          before do
+            allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
+            allow(jpg).to receive(:exists?).and_return(true)
+          end
+
+          it "responds with status 200" do
+            show_upload
+
+            expect(response).to have_gitlab_http_status(200)
+          end
+        end
+
+        context "when the file doesn't exist" do
+          it "responds with status 404" do
+            show_upload
+
+            expect(response).to have_gitlab_http_status(404)
+          end
+        end
+      end
+    end
+
+    context "when the model is private" do
+      before do
+        model.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PRIVATE)
+      end
+
+      context "when not signed in" do
+        context "when the file exists" do
+          before do
+            allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
+            allow(jpg).to receive(:exists?).and_return(true)
+          end
+
+          context "when the file is an image" do
+            before do
+              allow_any_instance_of(FileUploader).to receive(:image?).and_return(true)
+            end
+
+            it "responds with status 200" do
+              show_upload
+
+              expect(response).to have_gitlab_http_status(200)
+            end
+          end
+
+          context "when the file is not an image" do
+            it "redirects to the sign in page" do
+              show_upload
+
+              expect(response).to redirect_to(new_user_session_path)
+            end
+          end
+        end
+
+        context "when the file doesn't exist" do
+          it "redirects to the sign in page" do
+            show_upload
+
+            expect(response).to redirect_to(new_user_session_path)
+          end
+        end
+      end
+
+      context "when signed in" do
+        before do
+          sign_in(user)
+        end
+
+        context "when the user has access to the project" do
+          before do
+            model.add_developer(user)
+          end
+
+          context "when the file exists" do
+            before do
+              allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
+              allow(jpg).to receive(:exists?).and_return(true)
+            end
+
+            it "responds with status 200" do
+              show_upload
+
+              expect(response).to have_gitlab_http_status(200)
+            end
+          end
+
+          context "when the file doesn't exist" do
+            it "responds with status 404" do
+              show_upload
+
+              expect(response).to have_gitlab_http_status(404)
+            end
+          end
+        end
+
+        context "when the user doesn't have access to the model" do
+          context "when the file exists" do
+            before do
+              allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
+              allow(jpg).to receive(:exists?).and_return(true)
+            end
+
+            context "when the file is an image" do
+              before do
+                allow_any_instance_of(FileUploader).to receive(:image?).and_return(true)
+              end
+
+              it "responds with status 200" do
+                show_upload
+
+                expect(response).to have_gitlab_http_status(200)
+              end
+            end
+
+            context "when the file is not an image" do
+              it "responds with status 404" do
+                show_upload
+
+                expect(response).to have_gitlab_http_status(404)
+              end
+            end
+          end
+
+          context "when the file doesn't exist" do
+            it "responds with status 404" do
+              show_upload
+
+              expect(response).to have_gitlab_http_status(404)
+            end
+          end
+        end
+      end
+    end
+  end
+end