diff options
author | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2015-04-20 09:43:01 +0000 |
---|---|---|
committer | Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> | 2015-04-20 12:48:25 +0300 |
commit | 407489d6069e278a5722b45d86ef5b8a762cffa4 (patch) | |
tree | 9f43c7dfdde5789b89be6ce16429d758e926500b | |
parent | 59e3832b6e9c6a01b4e59fecff9d19eff7ec54f6 (diff) | |
download | gitlab-ce-407489d6069e278a5722b45d86ef5b8a762cffa4.tar.gz |
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
-rw-r--r-- | CHANGELOG | 1 | ||||
-rw-r--r-- | app/controllers/projects/uploads_controller.rb | 35 | ||||
-rw-r--r-- | spec/controllers/projects/uploads_controller_spec.rb | 223 |
3 files changed, 252 insertions, 7 deletions
diff --git a/CHANGELOG b/CHANGELOG index 98461c493df..0f250a0c365 100644 --- a/CHANGELOG +++ b/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. diff --git a/app/controllers/projects/uploads_controller.rb b/app/controllers/projects/uploads_controller.rb index 9020e86c44e..276dced8656 100644 --- a/app/controllers/projects/uploads_controller.rb +++ b/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 not_found! if uploader.nil? || !uploader.file.exists? - return redirect_to uploader.url unless uploader.file_storage? + disposition = uploader.image? ? 'inline' : 'attachment' + send_file uploader.file.path, disposition: disposition + end - uploader.retrieve_from_store!(params[:filename]) + def uploader + return @uploader if defined?(@uploader) - return not_found! unless uploader.file.exists? + namespace = params[:namespace_id] + id = params[:project_id] - disposition = uploader.image? ? 'inline' : 'attachment' - send_file uploader.file.path, disposition: disposition + 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 diff --git a/spec/controllers/projects/uploads_controller_spec.rb b/spec/controllers/projects/uploads_controller_spec.rb index 029f48b2d7a..f51abfedae5 100644 --- a/spec/controllers/projects/uploads_controller_spec.rb +++ b/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 |