Merge branch 'haynes/gitlab-ce-remove_access_control_for_images' into 'master'

Remove access control for uploaded images to fix broken images in emails

Replaces !530.

> This MR removes the access control for uploaded images. This is needed to display the images in emails again.
>
> The previous solution to base64 encode the images had to be reverted, because not all email clients supported it.
>
> If possible this should go into the 7.10 release.

See merge request !533

CHANGELOG

@@ -14,6 +14,7 @@ v 7.11.0 (unreleased)
 v 7.10.0 (unreleased)
   - Ignore submodules that are defined in .gitmodules but are checked in as directories.
   - Allow projects to be imported from Google Code.
+  - Remove access control for uploaded images to fix broken images in emails (Hannes Rosenögger)
   - Allow users to be invited by email to join a group or project.
   - Don't crash when project repository doesn't exist.
   - Add config var to block auto-created LDAP users.

app/controllers/projects/uploads_controller.rb

@@ -1,7 +1,11 @@
 class Projects::UploadsController < Projects::ApplicationController
   layout 'project'
 
-  before_filter :project
+  # We want to skip these filters for only the `show` action if `image?` is true, 
+  # but `skip_before_filter` doesn't work with both `only` and `if`, so we accomplish the same like this.
+  skipped_filters = [:authenticate_user!, :reject_blocked!, :project, :repository]
+  skip_before_filter  *skipped_filters, only: [:show]
+  before_filter       *skipped_filters, only: [:show], unless: :image?
 
   def create
     link_to_file = ::Projects::UploadService.new(project, params[:file]).
@@ -21,15 +25,32 @@ class Projects::UploadsController < Projects::ApplicationController
   end
 
   def show
-    uploader = FileUploader.new(project, params[:secret])
-
-    return redirect_to uploader.url unless uploader.file_storage?
-
-    uploader.retrieve_from_store!(params[:filename])
-
-    return not_found! unless uploader.file.exists?
+    return not_found! if uploader.nil? || !uploader.file.exists?
 
     disposition = uploader.image? ? 'inline' : 'attachment'
     send_file uploader.file.path, disposition: disposition
   end
+
+  def uploader
+    return @uploader if defined?(@uploader)
+
+    namespace = params[:namespace_id]
+    id = params[:project_id]
+
+    file_project = Project.find_with_namespace("#{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?
+    uploader && uploader.file.exists? && uploader.image?
+  end
 end

spec/controllers/projects/uploads_controller_spec.rb

@@ -54,4 +54,227 @@ describe Projects::UploadsController do
       end
     end
   end
+
+  describe "GET #show" do
+    let(:go) do
+      get :show,
+        namespace_id: project.namespace.to_param,
+        project_id:   project.to_param,
+        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.status).to eq(200)
+          end
+        end
+
+        context "when the file doesn't exist" do
+          it "responds with status 404" do
+            go
+
+            expect(response.status).to eq(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.status).to eq(200)
+          end
+        end
+
+        context "when the file doesn't exist" do
+          it "responds with status 404" do
+            go
+
+            expect(response.status).to eq(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.status).to eq(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 user is blocked" do
+            before do
+              user.block
+              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
+
+              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.status).to eq(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 the user isn't blocked" 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.status).to eq(200)
+              end
+            end
+
+            context "when the file doesn't exist" do
+              it "responds with status 404" do
+                go
+
+                expect(response.status).to eq(404)
+              end
+            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.status).to eq(200)
+              end
+            end
+
+            context "when the file is not an image" do
+              it "responds with status 404" do
+                go
+
+                expect(response.status).to eq(404)
+              end
+            end
+          end
+
+          context "when the file doesn't exist" do
+            it "responds with status 404" do
+              go
+
+              expect(response.status).to eq(404)
+            end
+          end
+        end
+      end
+    end
+  end
 end