Support uploads for groups

7 files changed, 336 insertions, 255 deletions

diff --git a/spec/controllers/groups/uploads_controller_spec.rb b/spec/controllers/groups/uploads_controller_spec.rb
new file mode 100644
index 00000000000..67a11e56e94
--- /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, :public) }
+  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..d572085661d 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, :public) }
+  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/factories/uploads.rb b/spec/factories/uploads.rb
index 3222c41c3d8..e18f1a6bd4a 100644
--- a/spec/factories/uploads.rb
+++ b/spec/factories/uploads.rb
@@ -4,5 +4,21 @@ FactoryGirl.define do
     path { "uploads/-/system/project/avatar/avatar.jpg" }
     size 100.kilobytes
     uploader "AvatarUploader"
+
+    trait :personal_snippet do
+      model { build(:personal_snippet) }
+      uploader "PersonalFileUploader"
+    end
+
+    trait :issuable_upload do
+      path { "#{SecureRandom.hex}/myfile.jpg" }
+      uploader "FileUploader"
+    end
+
+    trait :namespace_upload do
+      path { "#{SecureRandom.hex}/myfile.jpg" }
+      model { build(:group) }
+      uploader "NamespaceFileUploader"
+    end
   end
 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..76bc0c36ab7 100644
--- a/spec/lib/banzai/filter/upload_link_filter_spec.rb
+++ b/spec/lib/banzai/filter/upload_link_filter_spec.rb
@@ -89,7 +89,35 @@ 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) }
+    let(:filter_context) { { project: nil, group: group } }
+    let(:relative_path) { "groups/#{group.full_path}/-/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg" }
+
+    it 'rewrites the link correctly' do
+      doc = raw_filter(upload_link, filter_context)
+
+      expect(doc.at_css('a')['href']).to eq("#{Gitlab.config.gitlab.url}/#{relative_path}")
+    end
+
+    it 'rewrites the link correctly for subgroup' do
+      subgroup = create(:group, parent: group)
+      relative_path = "groups/#{subgroup.full_path}/-/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg"
+
+      doc = raw_filter(upload_link, { project: nil, group: subgroup })
+
+      expect(doc.at_css('a')['href']).to eq("#{Gitlab.config.gitlab.url}/#{relative_path}")
+    end
+
+    it 'does not modify absolute URL' do
+      doc = filter(link('http://example.com'), filter_context)
+
+      expect(doc.at_css('a')['href']).to eq 'http://example.com'
+    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/policies/group_policy_spec.rb b/spec/policies/group_policy_spec.rb
index 4f4e634829d..b4d25e06d9a 100644
--- a/spec/policies/group_policy_spec.rb
+++ b/spec/policies/group_policy_spec.rb
@@ -9,6 +9,8 @@ describe GroupPolicy do
   let(:admin) { create(:admin) }
   let(:group) { create(:group) }
 
+  let(:guest_permissions) { [:read_group, :upload_file, :read_namespace] }
+
   let(:reporter_permissions) { [:admin_label] }
 
   let(:developer_permissions) { [:admin_milestones] }
@@ -52,6 +54,7 @@ describe GroupPolicy do
 
     it do
       expect_allowed(:read_group)
+      expect_disallowed(:upload_file)
       expect_disallowed(*reporter_permissions)
       expect_disallowed(*developer_permissions)
       expect_disallowed(*master_permissions)
@@ -64,7 +67,7 @@ describe GroupPolicy do
     let(:current_user) { guest }
 
     it do
-      expect_allowed(:read_group, :read_namespace)
+      expect_allowed(*guest_permissions)
       expect_disallowed(*reporter_permissions)
       expect_disallowed(*developer_permissions)
       expect_disallowed(*master_permissions)
@@ -76,7 +79,7 @@ describe GroupPolicy do
     let(:current_user) { reporter }
 
     it do
-      expect_allowed(:read_group, :read_namespace)
+      expect_allowed(*guest_permissions)
       expect_allowed(*reporter_permissions)
       expect_disallowed(*developer_permissions)
       expect_disallowed(*master_permissions)
@@ -88,7 +91,7 @@ describe GroupPolicy do
     let(:current_user) { developer }
 
     it do
-      expect_allowed(:read_group, :read_namespace)
+      expect_allowed(*guest_permissions)
       expect_allowed(*reporter_permissions)
       expect_allowed(*developer_permissions)
       expect_disallowed(*master_permissions)
@@ -100,7 +103,7 @@ describe GroupPolicy do
     let(:current_user) { master }
 
     it do
-      expect_allowed(:read_group, :read_namespace)
+      expect_allowed(*guest_permissions)
       expect_allowed(*reporter_permissions)
       expect_allowed(*developer_permissions)
       expect_allowed(*master_permissions)
@@ -114,7 +117,7 @@ describe GroupPolicy do
     it do
       allow(Group).to receive(:supports_nested_groups?).and_return(true)
 
-      expect_allowed(:read_group, :read_namespace)
+      expect_allowed(*guest_permissions)
       expect_allowed(*reporter_permissions)
       expect_allowed(*developer_permissions)
       expect_allowed(*master_permissions)
@@ -128,7 +131,7 @@ describe GroupPolicy do
     it do
       allow(Group).to receive(:supports_nested_groups?).and_return(true)
 
-      expect_allowed(:read_group, :read_namespace)
+      expect_allowed(*guest_permissions)
       expect_allowed(*reporter_permissions)
       expect_allowed(*developer_permissions)
       expect_allowed(*master_permissions)
@@ -187,7 +190,7 @@ describe GroupPolicy do
       let(:current_user) { nil }
 
       it do
-        expect_disallowed(:read_group)
+        expect_disallowed(*guest_permissions)
         expect_disallowed(*reporter_permissions)
         expect_disallowed(*developer_permissions)
         expect_disallowed(*master_permissions)
@@ -199,7 +202,7 @@ describe GroupPolicy do
       let(:current_user) { guest }
 
       it do
-        expect_allowed(:read_group)
+        expect_allowed(*guest_permissions)
         expect_disallowed(*reporter_permissions)
         expect_disallowed(*developer_permissions)
         expect_disallowed(*master_permissions)
@@ -211,7 +214,7 @@ describe GroupPolicy do
       let(:current_user) { reporter }
 
       it do
-        expect_allowed(:read_group)
+        expect_allowed(*guest_permissions)
         expect_allowed(*reporter_permissions)
         expect_disallowed(*developer_permissions)
         expect_disallowed(*master_permissions)
@@ -223,7 +226,7 @@ describe GroupPolicy do
       let(:current_user) { developer }
 
       it do
-        expect_allowed(:read_group)
+        expect_allowed(*guest_permissions)
         expect_allowed(*reporter_permissions)
         expect_allowed(*developer_permissions)
         expect_disallowed(*master_permissions)
@@ -235,7 +238,7 @@ describe GroupPolicy do
       let(:current_user) { master }
 
       it do
-        expect_allowed(:read_group)
+        expect_allowed(*guest_permissions)
         expect_allowed(*reporter_permissions)
         expect_allowed(*developer_permissions)
         expect_allowed(*master_permissions)
@@ -249,7 +252,7 @@ describe GroupPolicy do
       it do
         allow(Group).to receive(:supports_nested_groups?).and_return(true)
 
-        expect_allowed(:read_group)
+        expect_allowed(*guest_permissions)
         expect_allowed(*reporter_permissions)
         expect_allowed(*developer_permissions)
         expect_allowed(*master_permissions)
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..935c08221e0
--- /dev/null
+++ b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb
@@ -0,0 +1,240 @@
+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
+    context 'when a user is not authorized to upload a file' do
+      it 'returns 404 status' do
+        post :create, params.merge(file: jpg, format: :json)
+
+        expect(response.status).to eq(404)
+      end
+    end
+
+    context 'when a user can upload a file' 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
+  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
diff --git a/spec/uploaders/namespace_file_uploader_spec.rb b/spec/uploaders/namespace_file_uploader_spec.rb
new file mode 100644
index 00000000000..c6c4500c179
--- /dev/null
+++ b/spec/uploaders/namespace_file_uploader_spec.rb
@@ -0,0 +1,21 @@
+require 'spec_helper'
+
+describe NamespaceFileUploader do
+  let(:group) { build_stubbed(:group) }
+  let(:uploader) { described_class.new(group) }
+
+  describe "#store_dir" do
+    it "stores in the namespace id directory" do
+      expect(uploader.store_dir).to include(group.id.to_s)
+    end
+  end
+
+  describe ".absolute_path" do
+    it "stores in thecorrect directory" do
+      upload_record = create(:upload, :namespace_upload, model: group)
+
+      expect(described_class.absolute_path(upload_record))
+        .to include("-/system/namespace/#{group.id}")
+    end
+  end
+end