Merge branch 'uploads-authorization' into 'master'

Reject access to group/project avatar if the user doesn't have access.

Closes #2050.

I'll add tests for EE logos once this is merged here and merged into EE.

See merge request !1671

CHANGELOG

@@ -45,6 +45,7 @@ v 7.9.0 (unreleased)
   - Send EmailsOnPush emails when deleting commits using force push.  
   - Fix EmailsOnPush email comparison link to include first commit.
   - Fix highliht of selected lines in file
+  - Reject access to group/project avatar if the user doesn't have access.
 
 v 7.8.2
   - Fix service migration issue when upgrading from versions prior to 7.3

app/controllers/uploads_controller.rb

 class UploadsController < ApplicationController
-  skip_before_filter :authenticate_user!, :reject_blocked!
-  before_filter :authorize_access
+  skip_before_filter :authenticate_user!
+  before_filter :find_model, :authorize_access!
 
   def show
-    unless upload_model && upload_mount
-      return not_found!
-    end
-
-    model = upload_model.find(params[:id])
-    uploader = model.send(upload_mount)
-
-    if model.respond_to?(:project) && !can?(current_user, :read_project, model.project)
-      return not_found!
-    end
+    uploader = @model.send(upload_mount)
 
     unless uploader.file_storage?
       return redirect_to uploader.url
     end
 
-    unless uploader.file.exists?
+    unless uploader.file && uploader.file.exists?
       return not_found!
     end
 
@@ -28,9 +19,34 @@ class UploadsController < ApplicationController
 
   private
 
-  def authorize_access
-    unless params[:mounted_as] == 'avatar'
-      authenticate_user! && reject_blocked!
+  def find_model
+    unless upload_model && upload_mount
+      return not_found!
+    end
+
+    @model = upload_model.find(params[:id])
+  end
+
+  def authorize_access!
+    authorized = 
+      case @model
+      when Project
+        can?(current_user, :read_project, @model)
+      when Group
+        can?(current_user, :read_group, @model)
+      when Note
+        can?(current_user, :read_project, @model.project)
+      else
+        # No authentication required for user avatars.
+        true
+      end
+
+    return if authorized
+
+    if current_user
+      not_found!
+    else
+      authenticate_user!
     end
   end
 

spec/controllers/uploads_controller_spec.rb

+require 'spec_helper'
+
+describe UploadsController do
+  let!(:user) { create(:user, avatar: fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png")) }
+
+  describe "GET show" do
+    context "when viewing a user avatar" do
+      context "when signed in" do
+        before do
+          sign_in(user)
+        end
+
+        context "when the user is blocked" do
+          before do
+            user.block
+          end
+
+          it "redirects to the sign in page" do
+            get :show, model: "user", mounted_as: "avatar", id: user.id, filename: "image.png"
+
+            expect(response).to redirect_to(new_user_session_path)
+          end
+        end
+
+        context "when the user isn't blocked" do
+          it "responds with status 200" do
+            get :show, model: "user", mounted_as: "avatar", id: user.id, filename: "image.png"
+
+            expect(response.status).to eq(200)
+          end
+        end
+      end
+      
+      context "when not signed in" do
+        it "responds with status 200" do
+          get :show, model: "user", mounted_as: "avatar", id: user.id, filename: "image.png"
+
+          expect(response.status).to eq(200)
+        end
+      end
+    end
+
+    context "when viewing a project avatar" do
+      let!(:project) { create(:project, avatar: fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png")) }
+
+      context "when the project is public" do
+        before do
+          project.update_attribute(:visibility_level, Project::PUBLIC)
+        end
+
+        context "when not signed in" do
+          it "responds with status 200" do
+            get :show, model: "project", mounted_as: "avatar", id: project.id, filename: "image.png"
+
+            expect(response.status).to eq(200)
+          end
+        end
+
+        context "when signed in" do
+          before do
+            sign_in(user)
+          end
+
+          it "responds with status 200" do
+            get :show, model: "project", mounted_as: "avatar", id: project.id, filename: "image.png"
+
+            expect(response.status).to eq(200)
+          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
+          it "redirects to the sign in page" do
+            get :show, model: "project", mounted_as: "avatar", id: project.id, filename: "image.png"
+
+            expect(response).to redirect_to(new_user_session_path)
+          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
+
+              it "redirects to the sign in page" do
+                get :show, model: "project", mounted_as: "avatar", id: project.id, filename: "image.png"
+
+                expect(response).to redirect_to(new_user_session_path)
+              end
+            end
+
+            context "when the user isn't blocked" do
+              it "responds with status 200" do
+                get :show, model: "project", mounted_as: "avatar", id: project.id, filename: "image.png"
+
+                expect(response.status).to eq(200)
+              end
+            end
+          end
+
+          context "when the user doesn't have access to the project" do
+            it "responds with status 404" do
+              get :show, model: "project", mounted_as: "avatar", id: project.id, filename: "image.png"
+
+              expect(response.status).to eq(404)
+            end
+          end
+        end
+      end
+    end
+
+    context "when viewing a group avatar" do
+      let!(:group) { create(:group, avatar: fixture_file_upload(Rails.root + "spec/fixtures/dk.png", "image/png")) }
+      let!(:project) { create(:project, namespace: group) }
+
+      context "when the group has public projects" do
+        before do
+          project.update_attribute(:visibility_level, Project::PUBLIC)
+        end
+
+        context "when not signed in" do
+          it "responds with status 200" do
+            get :show, model: "group", mounted_as: "avatar", id: group.id, filename: "image.png"
+
+            expect(response.status).to eq(200)
+          end
+        end
+
+        context "when signed in" do
+          before do
+            sign_in(user)
+          end
+
+          it "responds with status 200" do
+            get :show, model: "group", mounted_as: "avatar", id: group.id, filename: "image.png"
+
+            expect(response.status).to eq(200)
+          end
+        end
+      end
+
+      context "when the project doesn't have public projects" do
+        context "when not signed in" do
+          it "redirects to the sign in page" do
+            get :show, model: "group", mounted_as: "avatar", id: group.id, filename: "image.png"
+
+            expect(response).to redirect_to(new_user_session_path)
+          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
+
+              it "redirects to the sign in page" do
+                get :show, model: "group", mounted_as: "avatar", id: group.id, filename: "image.png"
+
+                expect(response).to redirect_to(new_user_session_path)
+              end
+            end
+
+            context "when the user isn't blocked" do
+              it "responds with status 200" do
+                get :show, model: "group", mounted_as: "avatar", id: group.id, filename: "image.png"
+
+                expect(response.status).to eq(200)
+              end
+            end
+          end
+
+          context "when the user doesn't have access to the project" do
+            it "responds with status 404" do
+              get :show, model: "group", mounted_as: "avatar", id: group.id, filename: "image.png"
+
+              expect(response.status).to eq(404)
+            end
+          end
+        end
+      end
+    end
+
+    context "when viewing a note attachment" do
+      let!(:note) { create(:note, :with_attachment) }
+      let(:project) { note.project }
+
+      context "when the project is public" do
+        before do
+          project.update_attribute(:visibility_level, Project::PUBLIC)
+        end
+
+        context "when not signed in" do
+          it "responds with status 200" do
+            get :show, model: "note", mounted_as: "attachment", id: note.id, filename: "image.png"
+
+            expect(response.status).to eq(200)
+          end
+        end
+
+        context "when signed in" do
+          before do
+            sign_in(user)
+          end
+
+          it "responds with status 200" do
+            get :show, model: "note", mounted_as: "attachment", id: note.id, filename: "image.png"
+
+            expect(response.status).to eq(200)
+          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
+          it "redirects to the sign in page" do
+            get :show, model: "note", mounted_as: "attachment", id: note.id, filename: "image.png"
+
+            expect(response).to redirect_to(new_user_session_path)
+          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
+
+              it "redirects to the sign in page" do
+                get :show, model: "note", mounted_as: "attachment", id: note.id, filename: "image.png"
+
+                expect(response).to redirect_to(new_user_session_path)
+              end
+            end
+
+            context "when the user isn't blocked" do
+              it "responds with status 200" do
+                get :show, model: "note", mounted_as: "attachment", id: note.id, filename: "image.png"
+
+                expect(response.status).to eq(200)
+              end
+            end
+          end
+
+          context "when the user doesn't have access to the project" do
+            it "responds with status 404" do
+              get :show, model: "note", mounted_as: "attachment", id: note.id, filename: "image.png"
+
+              expect(response.status).to eq(404)
+            end
+          end
+        end
+      end
+    end
+  end
+end