Merge branch 'fix-14607' into 'master'

Ensure private project snippets are not viewable by unauthorized people Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/14607 See merge request !1946

Merge branch 'fix-14607' into 'master'
Ensure private project snippets are not viewable by unauthorized people Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/14607 See merge request !1946
a7ca8689 · Douwe Maan · d73e1288 · 4f07c0a1 · a7ca8689 · a7ca8689
--- a/app/controllers/projects/snippets_controller.rb
+++ b/app/controllers/projects/snippets_controller.rb
@@ -3,7 +3,7 @@ class Projects::SnippetsController < Projects::ApplicationController
  before_action :snippet, only: [:show, :edit, :destroy, :update, :raw]

  # Allow read any snippet
-  before_action :authorize_read_project_snippet!
+  before_action :authorize_read_project_snippet!, except: [:new, :create, :index]

  # Allow write(create) snippet
  before_action :authorize_create_project_snippet!, only: [:new, :create]
@@ -81,6 +81,10 @@ def snippet
    @snippet ||= @project.snippets.find(params[:id])
  end

+  def authorize_read_project_snippet!
+    return render_404 unless can?(current_user, :read_project_snippet, @snippet)
+  end
+
  def authorize_update_project_snippet!
    return render_404 unless can?(current_user, :update_project_snippet, @snippet)
  end

--- a/app/models/ability.rb
+++ b/app/models/ability.rb
@@ -27,6 +27,8 @@ def anonymous_abilities(user, subject)
      case true
      when subject.is_a?(PersonalSnippet)
        anonymous_personal_snippet_abilities(subject)
+      when subject.is_a?(ProjectSnippet)
+        anonymous_project_snippet_abilities(subject)
      when subject.is_a?(CommitStatus)
        anonymous_commit_status_abilities(subject)
      when subject.is_a?(Project) || subject.respond_to?(:project)
@@ -100,6 +102,14 @@ def anonymous_personal_snippet_abilities(snippet)
      end
    end

+    def anonymous_project_snippet_abilities(snippet)
+      if snippet.public?
+        [:read_project_snippet]
+      else
+        []
+      end
+    end
+
    def global_abilities(user)
      rules = []
      rules << :create_group if user.can_create_group
@@ -338,24 +348,22 @@ def namespace_abilities(user, namespace)
      end
    end

-    [:note, :project_snippet].each do |name|
-      define_method "#{name}_abilities" do |user, subject|
-        rules = []
-
-        if subject.author == user
-          rules += [
-            :"read_#{name}",
-            :"update_#{name}",
-            :"admin_#{name}"
-          ]
-        end
+    def note_abilities(user, note)
+      rules = []

-        if subject.respond_to?(:project) && subject.project
-          rules += project_abilities(user, subject.project)
-        end
+      if note.author == user
+        rules += [
+          :read_note,
+          :update_note,
+          :admin_note
+        ]
+      end

-        rules
+      if note.respond_to?(:project) && note.project
+        rules += project_abilities(user, note.project)
      end
+
+      rules
    end

    def personal_snippet_abilities(user, snippet)
@@ -376,6 +384,24 @@ def personal_snippet_abilities(user, snippet)
      rules
    end

+    def project_snippet_abilities(user, snippet)
+      rules = []
+
+      if snippet.author == user || user.admin?
+        rules += [
+          :read_project_snippet,
+          :update_project_snippet,
+          :admin_project_snippet
+        ]
+      end
+
+      if snippet.public? || (snippet.internal? && !user.external?) || (snippet.private? && snippet.project.team.member?(user))
+        rules << :read_project_snippet
+      end
+
+      rules
+    end
+
    def group_member_abilities(user, subject)
      rules = []
      target_user = subject.user

--- a/spec/controllers/projects/snippets_controller_spec.rb
+++ b/spec/controllers/projects/snippets_controller_spec.rb
+require 'spec_helper'
+
+describe Projects::SnippetsController do
+  let(:project) { create(:project_empty_repo, :public, snippets_enabled: true) }
+  let(:user)    { create(:user) }
+  let(:user2)   { create(:user) }
+
+  before do
+    project.team << [user, :master]
+    project.team << [user2, :master]
+  end
+
+  describe 'GET #index' do
+    context 'when the project snippet is private' do
+      let!(:project_snippet) { create(:project_snippet, :private, project: project, author: user) }
+
+      context 'when anonymous' do
+        it 'does not include the private snippet' do
+          get :index, namespace_id: project.namespace.path, project_id: project.path
+
+          expect(assigns(:snippets)).not_to include(project_snippet)
+          expect(response.status).to eq(200)
+        end
+      end
+
+      context 'when signed in as the author' do
+        before { sign_in(user) }
+
+        it 'renders the snippet' do
+          get :index, namespace_id: project.namespace.path, project_id: project.path
+
+          expect(assigns(:snippets)).to include(project_snippet)
+          expect(response.status).to eq(200)
+        end
+      end
+
+      context 'when signed in as a project member' do
+        before { sign_in(user2) }
+
+        it 'renders the snippet' do
+          get :index, namespace_id: project.namespace.path, project_id: project.path
+
+          expect(assigns(:snippets)).to include(project_snippet)
+          expect(response.status).to eq(200)
+        end
+      end
+    end
+  end
+
+  %w[show raw].each do |action|
+    describe "GET ##{action}" do
+      context 'when the project snippet is private' do
+        let(:project_snippet) { create(:project_snippet, :private, project: project, author: user) }
+
+        context 'when anonymous' do
+          it 'responds with status 404' do
+            get action, namespace_id: project.namespace.path, project_id: project.path, id: project_snippet.to_param
+
+            expect(response.status).to eq(404)
+          end
+        end
+
+        context 'when signed in as the author' do
+          before { sign_in(user) }
+
+          it 'renders the snippet' do
+            get action, namespace_id: project.namespace.path, project_id: project.path, id: project_snippet.to_param
+
+            expect(assigns(:snippet)).to eq(project_snippet)
+            expect(response.status).to eq(200)
+          end
+        end
+
+        context 'when signed in as a project member' do
+          before { sign_in(user2) }
+
+          it 'renders the snippet' do
+            get action, namespace_id: project.namespace.path, project_id: project.path, id: project_snippet.to_param
+
+            expect(assigns(:snippet)).to eq(project_snippet)
+            expect(response.status).to eq(200)
+          end
+        end
+      end
+
+      context 'when the project snippet does not exist' do
+        context 'when anonymous' do
+          it 'responds with status 404' do
+            get action, namespace_id: project.namespace.path, project_id: project.path, id: 42
+
+            expect(response.status).to eq(404)
+          end
+        end
+
+        context 'when signed in' do
+          before { sign_in(user) }
+
+          it 'responds with status 404' do
+            get action, namespace_id: project.namespace.path, project_id: project.path, id: 42
+
+            expect(response.status).to eq(404)
+          end
+        end
+      end
+    end
+  end
+end
--- a/spec/features/security/project/snippet/internal_access_spec.rb
+++ b/spec/features/security/project/snippet/internal_access_spec.rb
+require 'spec_helper'
+
+describe "Internal Project Snippets Access", feature: true  do
+  include AccessMatchers
+
+  let(:project) { create(:project, :internal) }
+
+  let(:owner)     { project.owner }
+  let(:master)    { create(:user) }
+  let(:developer) { create(:user) }
+  let(:reporter)  { create(:user) }
+  let(:guest)     { create(:user) }
+  let(:internal_snippet) { create(:project_snippet, :internal, project: project, author: owner) }
+  let(:private_snippet)  { create(:project_snippet, :private, project: project, author: owner) }
+
+  before do
+    project.team << [master, :master]
+    project.team << [developer, :developer]
+    project.team << [reporter, :reporter]
+    project.team << [guest, :guest]
+  end
+
+  describe "GET /:project_path/snippets" do
+    subject { namespace_project_snippets_path(project.namespace, project) }
+
+    it { is_expected.to be_allowed_for :admin }
+    it { is_expected.to be_allowed_for owner }
+    it { is_expected.to be_allowed_for master }
+    it { is_expected.to be_allowed_for developer }
+    it { is_expected.to be_allowed_for reporter }
+    it { is_expected.to be_allowed_for guest }
+    it { is_expected.to be_allowed_for :user }
+    it { is_expected.to be_denied_for :external }
+    it { is_expected.to be_denied_for :visitor }
+  end
+
+  describe "GET /:project_path/snippets/new" do
+    subject { new_namespace_project_snippet_path(project.namespace, project) }
+
+    it { is_expected.to be_allowed_for :admin }
+    it { is_expected.to be_allowed_for owner }
+    it { is_expected.to be_allowed_for master }
+    it { is_expected.to be_allowed_for developer }
+    it { is_expected.to be_allowed_for reporter }
+    it { is_expected.to be_denied_for guest }
+    it { is_expected.to be_denied_for :user }
+    it { is_expected.to be_denied_for :external }
+    it { is_expected.to be_denied_for :visitor }
+  end
+
+  describe "GET /:project_path/snippets/:id for an internal snippet" do
+    subject { namespace_project_snippet_path(project.namespace, project, internal_snippet) }
+
+    it { is_expected.to be_allowed_for :admin }
+    it { is_expected.to be_allowed_for owner }
+    it { is_expected.to be_allowed_for master }
+    it { is_expected.to be_allowed_for developer }
+    it { is_expected.to be_allowed_for reporter }
+    it { is_expected.to be_allowed_for guest }
+    it { is_expected.to be_allowed_for :user }
+    it { is_expected.to be_denied_for :external }
+    it { is_expected.to be_denied_for :visitor }
+  end
+
+  describe "GET /:project_path/snippets/:id for a private snippet" do
+    subject { namespace_project_snippet_path(project.namespace, project, private_snippet) }
+
+    it { is_expected.to be_allowed_for :admin }
+    it { is_expected.to be_allowed_for owner }
+    it { is_expected.to be_allowed_for master }
+    it { is_expected.to be_allowed_for developer }
+    it { is_expected.to be_allowed_for reporter }
+    it { is_expected.to be_allowed_for guest }
+    it { is_expected.to be_denied_for :user }
+    it { is_expected.to be_denied_for :external }
+    it { is_expected.to be_denied_for :visitor }
+  end
+end
--- a/spec/features/security/project/snippet/private_access_spec.rb
+++ b/spec/features/security/project/snippet/private_access_spec.rb
+require 'spec_helper'
+
+describe "Private Project Snippets Access", feature: true  do
+  include AccessMatchers
+
+  let(:project) { create(:project, :private) }
+
+  let(:owner)     { project.owner }
+  let(:master)    { create(:user) }
+  let(:developer) { create(:user) }
+  let(:reporter)  { create(:user) }
+  let(:guest)     { create(:user) }
+  let(:private_snippet)  { create(:project_snippet, :private, project: project, author: owner) }
+
+  before do
+    project.team << [master, :master]
+    project.team << [developer, :developer]
+    project.team << [reporter, :reporter]
+    project.team << [guest, :guest]
+  end
+
+  describe "GET /:project_path/snippets" do
+    subject { namespace_project_snippets_path(project.namespace, project) }
+
+    it { is_expected.to be_allowed_for :admin }
+    it { is_expected.to be_allowed_for owner }
+    it { is_expected.to be_allowed_for master }
+    it { is_expected.to be_allowed_for developer }
+    it { is_expected.to be_allowed_for reporter }
+    it { is_expected.to be_allowed_for guest }
+    it { is_expected.to be_denied_for :user }
+    it { is_expected.to be_denied_for :external }
+    it { is_expected.to be_denied_for :visitor }
+  end
+
+  describe "GET /:project_path/snippets/new" do
+    subject { new_namespace_project_snippet_path(project.namespace, project) }
+
+    it { is_expected.to be_allowed_for :admin }
+    it { is_expected.to be_allowed_for owner }
+    it { is_expected.to be_allowed_for master }
+    it { is_expected.to be_allowed_for developer }
+    it { is_expected.to be_allowed_for reporter }
+    it { is_expected.to be_denied_for guest }
+    it { is_expected.to be_denied_for :user }
+    it { is_expected.to be_denied_for :external }
+    it { is_expected.to be_denied_for :visitor }
+  end
+
+  describe "GET /:project_path/snippets/:id for a private snippet" do
+    subject { namespace_project_snippet_path(project.namespace, project, private_snippet) }
+
+    it { is_expected.to be_allowed_for :admin }
+    it { is_expected.to be_allowed_for owner }
+    it { is_expected.to be_allowed_for master }
+    it { is_expected.to be_allowed_for developer }
+    it { is_expected.to be_allowed_for reporter }
+    it { is_expected.to be_allowed_for guest }
+    it { is_expected.to be_denied_for :user }
+    it { is_expected.to be_denied_for :external }
+    it { is_expected.to be_denied_for :visitor }
+  end
+end
--- a/spec/features/security/project/snippet/public_access_spec.rb
+++ b/spec/features/security/project/snippet/public_access_spec.rb
+require 'spec_helper'
+
+describe "Public Project Snippets Access", feature: true  do
+  include AccessMatchers
+
+  let(:project) { create(:project, :public) }
+
+  let(:owner)     { project.owner }
+  let(:master)    { create(:user) }
+  let(:developer) { create(:user) }
+  let(:reporter)  { create(:user) }
+  let(:guest)     { create(:user) }
+  let(:public_snippet)   { create(:project_snippet, :public, project: project, author: owner) }
+  let(:internal_snippet) { create(:project_snippet, :internal, project: project, author: owner) }
+  let(:private_snippet)  { create(:project_snippet, :private, project: project, author: owner) }
+
+  before do
+    project.team << [master, :master]
+    project.team << [developer, :developer]
+    project.team << [reporter, :reporter]
+    project.team << [guest, :guest]
+  end
+
+  describe "GET /:project_path/snippets" do
+    subject { namespace_project_snippets_path(project.namespace, project) }
+
+    it { is_expected.to be_allowed_for :admin }
+    it { is_expected.to be_allowed_for owner }
+    it { is_expected.to be_allowed_for master }
+    it { is_expected.to be_allowed_for developer }
+    it { is_expected.to be_allowed_for reporter }
+    it { is_expected.to be_allowed_for guest }
+    it { is_expected.to be_allowed_for :user }
+    it { is_expected.to be_allowed_for :external }
+    it { is_expected.to be_allowed_for :visitor }
+  end
+
+  describe "GET /:project_path/snippets/new" do
+    subject { new_namespace_project_snippet_path(project.namespace, project) }
+
+    it { is_expected.to be_allowed_for :admin }
+    it { is_expected.to be_allowed_for owner }
+    it { is_expected.to be_allowed_for master }
+    it { is_expected.to be_allowed_for developer }
+    it { is_expected.to be_allowed_for reporter }
+    it { is_expected.to be_denied_for guest }
+    it { is_expected.to be_denied_for :user }
+    it { is_expected.to be_denied_for :external }
+    it { is_expected.to be_denied_for :visitor }
+  end
+
+  describe "GET /:project_path/snippets/:id for a public snippet" do
+    subject { namespace_project_snippet_path(project.namespace, project, public_snippet) }
+
+    it { is_expected.to be_allowed_for :admin }
+    it { is_expected.to be_allowed_for owner }
+    it { is_expected.to be_allowed_for master }
+    it { is_expected.to be_allowed_for developer }
+    it { is_expected.to be_allowed_for reporter }
+    it { is_expected.to be_allowed_for guest }
+    it { is_expected.to be_allowed_for :user }
+    it { is_expected.to be_allowed_for :external }
+    it { is_expected.to be_allowed_for :visitor }
+  end
+
+  describe "GET /:project_path/snippets/:id for an internal snippet" do
+    subject { namespace_project_snippet_path(project.namespace, project, internal_snippet) }
+
+    it { is_expected.to be_allowed_for :admin }
+    it { is_expected.to be_allowed_for owner }
+    it { is_expected.to be_allowed_for master }
+    it { is_expected.to be_allowed_for developer }
+    it { is_expected.to be_allowed_for reporter }
+    it { is_expected.to be_allowed_for guest }
+    it { is_expected.to be_allowed_for :user }
+    it { is_expected.to be_denied_for :external }
+    it { is_expected.to be_denied_for :visitor }
+  end
+
+  describe "GET /:project_path/snippets/:id for a private snippet" do
+    subject { namespace_project_snippet_path(project.namespace, project, private_snippet) }
+
+    it { is_expected.to be_allowed_for :admin }
+    it { is_expected.to be_allowed_for owner }
+    it { is_expected.to be_allowed_for master }
+    it { is_expected.to be_allowed_for developer }
+    it { is_expected.to be_allowed_for reporter }
+    it { is_expected.to be_allowed_for guest }
+    it { is_expected.to be_denied_for :user }
+    it { is_expected.to be_denied_for :external }
+    it { is_expected.to be_denied_for :visitor }
+  end
+end