Merge branch 'snippets-finder-visibility' into 'security'

Refactor snippets finder & dont return internal snippets for external users

See merge request !2094

app/controllers/dashboard/snippets_controller.rb

 class Dashboard::SnippetsController < Dashboard::ApplicationController
   def index
-    @snippets = SnippetsFinder.new.execute(
+    @snippets = SnippetsFinder.new(
       current_user,
-      filter: :by_user,
-      user: current_user,
+      author: current_user,
       scope: params[:scope]
-    )
+    ).execute
     @snippets = @snippets.page(params[:page])
   end
 end

app/controllers/explore/snippets_controller.rb

 class Explore::SnippetsController < Explore::ApplicationController
   def index
-    @snippets = SnippetsFinder.new.execute(current_user, filter: :all)
+    @snippets = SnippetsFinder.new(current_user).execute
     @snippets = @snippets.page(params[:page])
   end
 end

app/controllers/projects/snippets_controller.rb

@@ -20,12 +20,11 @@ class Projects::SnippetsController < Projects::ApplicationController
   respond_to :html
   def index
-    @snippets = SnippetsFinder.new.execute(
+    @snippets = SnippetsFinder.new(
       current_user,
-      filter: :by_project,
       project: @project,
       scope: params[:scope]
-    )
+    ).execute
     @snippets = @snippets.page(params[:page])
     if @snippets.out_of_range? && @snippets.total_pages != 0
       redirect_to namespace_project_snippets_path(page: @snippets.total_pages)

app/controllers/snippets_controller.rb

@@ -24,11 +24,8 @@ class SnippetsController < ApplicationController
       render_404 and return unless @user
-      @snippets = SnippetsFinder.new.execute(current_user, {
-        filter: :by_user,
-        user: @user,
-        scope: params[:scope] }).
-      page(params[:page])
+      @snippets = SnippetsFinder.new(current_user, author: @user, scope: params[:scope])
+        .execute.page(params[:page])
       render 'index'
     else

app/controllers/users_controller.rb

@@ -131,12 +131,11 @@ class UsersController < ApplicationController
   end
   def load_snippets
-    @snippets = SnippetsFinder.new.execute(
+    @snippets = SnippetsFinder.new(
       current_user,
-      filter: :by_user,
-      user: user,
+      author: user,
       scope: params[:scope]
-    ).page(params[:page])
+    ).execute.page(params[:page])
   end
   def projects_for_current_user

app/finders/notes_finder.rb

@@ -49,7 +49,7 @@ class NotesFinder
     when "merge_request"
       MergeRequestsFinder.new(@current_user, project_id: @project.id).execute
     when "snippet", "project_snippet"
-      SnippetsFinder.new.execute(@current_user, filter: :by_project, project: @project)
+      SnippetsFinder.new(@current_user, project: @project).execute
     else
       raise 'invalid target_type'
     end

app/finders/snippets_finder.rb

-class SnippetsFinder
-  def execute(current_user, params = {})
-    filter = params[:filter]
-    user = params.fetch(:user, current_user)
-
-    case filter
-    when :all then
-      snippets(current_user).fresh
-    when :public then
-      Snippet.are_public.fresh
-    when :by_user then
-      by_user(current_user, user, params[:scope])
-    when :by_project
-      by_project(current_user, params[:project], params[:scope])
-    end
+class SnippetsFinder < UnionFinder
+  attr_accessor :current_user, :params
+
+  def initialize(current_user, params = {})
+    @current_user = current_user
+    @params = params
+  end
+
+  def execute
+    items = init_collection
+    items = by_project(items)
+    items = by_author(items)
+    items = by_visibility(items)
+
+    items.fresh
   end
   private
-  def snippets(current_user)
-    if current_user
-      Snippet.public_and_internal
-    else
-      # Not authenticated
-      #
-      # Return only:
-      #   public snippets
-      Snippet.are_public
-    end
+  def init_collection
+    items = Snippet.all
+
+    accessible(items)
   end
-  def by_user(current_user, user, scope)
-    snippets = user.snippets.fresh
-    if current_user
-      include_private = user == current_user
-      by_scope(snippets, scope, include_private)
-    else
-      snippets.are_public
-    end
+  def accessible(items)
+    segments = []
+    segments << items.public_to_user(current_user)
+    segments << authorized_to_user(items)  if current_user
+    find_union(segments, Snippet)
   end
-  def by_project(current_user, project, scope)
-    snippets = project.snippets.fresh
-    if current_user
-      include_private = project.team.member?(current_user) || current_user.admin?
-      by_scope(snippets, scope, include_private)
-    else
-      snippets.are_public
-    end
+  def authorized_to_user(items)
+    items.where(
+      'author_id = :author_id
+       OR project_id IN (:project_ids)',
+       author_id: current_user.id,
+       project_ids: current_user.authorized_projects.select(:id))
+  end
+  def by_visibility(items)
+    visibility = params[:visibility] || visibility_from_scope
+
+    return items unless visibility
+
+    items.where(visibility_level: visibility)
+  end
+
+  def by_author(items)
+    return items unless params[:author]
+
+    items.where(author_id: params[:author].id)
+  end
+
+  def by_project(items)
+    return items unless params[:project]
+
+    items.where(project_id: params[:project].id)
   end
-  def by_scope(snippets, scope = nil, include_private = false)
-    case scope.to_s
+  def visibility_from_scope
+    case params[:scope].to_s
     when 'are_private'
-      include_private ? snippets.are_private : Snippet.none
+      Snippet::PRIVATE
     when 'are_internal'
-      snippets.are_internal
+      Snippet::INTERNAL
     when 'are_public'
-      snippets.are_public
+      Snippet::PUBLIC
     else
-      include_private ? snippets : snippets.public_and_internal
+      nil
     end
   end
 end

app/models/snippet.rb

@@ -167,18 +167,5 @@ class Snippet < ActiveRecord::Base
       where(table[:content].matches(pattern))
     end
-
-    def accessible_to(user)
-      return are_public unless user.present?
-      return all if user.admin?
-
-      where(
-        'visibility_level IN (:visibility_levels)
-         OR author_id = :author_id
-         OR project_id IN (:project_ids)',
-         visibility_levels: [Snippet::PUBLIC, Snippet::INTERNAL],
-         author_id: user.id,
-         project_ids: user.authorized_projects.select(:id))
-    end
   end
 end

app/policies/project_snippet_policy.rb

@@ -13,7 +13,7 @@ class ProjectSnippetPolicy < BasePolicy
       can! :read_project_snippet
     end
-    if @subject.private? && @subject.project.team.member?(@user)
+    if @subject.project.team.member?(@user)
       can! :read_project_snippet
     end
   end

app/services/search/snippet_service.rb

@@ -7,7 +7,7 @@ module Search
     end
     def execute
-      snippets = Snippet.accessible_to(current_user)
+      snippets = SnippetsFinder.new(current_user).execute
       Gitlab::SnippetSearchResults.new(snippets, params[:search])
     end

changelogs/unreleased/snippets-finder-visibility.yml

+---
+title: Refactor snippets finder & dont return internal snippets for external users
+merge_request:
+author:

lib/api/project_snippets.rb

@@ -17,8 +17,7 @@ module API
         end
         def snippets_for_current_user
-          finder_params = { filter: :by_project, project: user_project }
-          SnippetsFinder.new.execute(current_user, finder_params)
+          SnippetsFinder.new(current_user, project: user_project).execute
         end
       end

lib/api/snippets.rb

@@ -8,11 +8,11 @@ module API
     resource :snippets do
       helpers do
         def snippets_for_current_user
-          SnippetsFinder.new.execute(current_user, filter: :by_user, user: current_user)
+          SnippetsFinder.new(current_user, author: current_user).execute
         end
         def public_snippets
-          SnippetsFinder.new.execute(current_user, filter: :public)
+          SnippetsFinder.new(current_user, visibility: Snippet::PUBLIC).execute
         end
       end

lib/api/v3/project_snippets.rb

@@ -18,8 +18,7 @@ module API
           end
           def snippets_for_current_user
-            finder_params = { filter: :by_project, project: user_project }
-            SnippetsFinder.new.execute(current_user, finder_params)
+            SnippetsFinder.new(current_user, project: user_project).execute
           end
         end

spec/controllers/snippets_controller_spec.rb

@@ -3,6 +3,34 @@ require 'spec_helper'
 describe SnippetsController do
   let(:user) { create(:user) }
+  describe 'GET #index' do
+    let(:user) { create(:user) }
+
+    context 'when username parameter is present' do
+      it 'renders snippets of a user when username is present' do
+        get :index, username: user.username
+
+        expect(response).to render_template(:index)
+      end
+    end
+
+    context 'when username parameter is not present' do
+      it 'redirects to explore snippets page when user is not logged in' do
+        get :index
+
+        expect(response).to redirect_to(explore_snippets_path)
+      end
+
+      it 'redirects to snippets dashboard page when user is logged in' do
+        sign_in(user)
+
+        get :index
+
+        expect(response).to redirect_to(dashboard_snippets_path)
+      end
+    end
+  end
+
   describe 'GET #new' do
     context 'when signed in' do
       before do

spec/features/dashboard/snippets_spec.rb

@@ -12,4 +12,51 @@ describe 'Dashboard snippets', feature: true do
     it_behaves_like 'paginated snippets'
   end
+
+  context 'filtering by visibility' do
+    let(:user) { create(:user) }
+    let!(:snippets) do
+      [
+        create(:personal_snippet, :public, author: user),
+        create(:personal_snippet, :internal, author: user),
+        create(:personal_snippet, :private, author: user),
+        create(:personal_snippet, :public)
+      ]
+    end
+
+    before do
+      login_as(user)
+
+      visit dashboard_snippets_path
+    end
+
+    it 'contains all snippets of logged user' do
+      expect(page).to have_selector('.snippet-row', count: 3)
+
+      expect(page).to have_content(snippets[0].title)
+      expect(page).to have_content(snippets[1].title)
+      expect(page).to have_content(snippets[2].title)
+    end
+
+    it 'contains all private snippets of logged user when clicking on private' do
+      click_link('Private')
+
+      expect(page).to have_selector('.snippet-row', count: 1)
+      expect(page).to have_content(snippets[2].title)
+    end
+
+    it 'contains all internal snippets of logged user when clicking on internal' do
+      click_link('Internal')
+
+      expect(page).to have_selector('.snippet-row', count: 1)
+      expect(page).to have_content(snippets[1].title)
+    end
+
+    it 'contains all public snippets of logged user when clicking on public' do
+      click_link('Public')
+
+      expect(page).to have_selector('.snippet-row', count: 1)
+      expect(page).to have_content(snippets[0].title)
+    end
+  end
 end

spec/features/projects/snippets_spec.rb

@@ -4,11 +4,27 @@ describe 'Project snippets', feature: true do
   context 'when the project has snippets' do
     let(:project) { create(:empty_project, :public) }
     let!(:snippets) { create_list(:project_snippet, 2, :public, author: project.owner, project: project) }
-    before do
-      allow(Snippet).to receive(:default_per_page).and_return(1)
-      visit namespace_project_snippets_path(project.namespace, project)
+    let!(:other_snippet) { create(:project_snippet) }
+
+    context 'pagination' do
+      before do
+        allow(Snippet).to receive(:default_per_page).and_return(1)
+
+        visit namespace_project_snippets_path(project.namespace, project)
+      end
+
+      it_behaves_like 'paginated snippets'
     end
-    it_behaves_like 'paginated snippets'
+    context 'list content' do
+      it 'contains all project snippets' do
+        visit namespace_project_snippets_path(project.namespace, project)
+
+        expect(page).to have_selector('.snippet-row', count: 2)
+
+        expect(page).to have_content(snippets[0].title)
+        expect(page).to have_content(snippets[1].title)
+      end
+    end
   end
 end

spec/features/snippets/explore_spec.rb

 require 'rails_helper'
 feature 'Explore Snippets', feature: true do
-  scenario 'User should see snippets that are not private' do
-    public_snippet = create(:personal_snippet, :public)
-    internal_snippet = create(:personal_snippet, :internal)
-    private_snippet = create(:personal_snippet, :private)
+  let!(:public_snippet) { create(:personal_snippet, :public) }
+  let!(:internal_snippet) { create(:personal_snippet, :internal) }
+  let!(:private_snippet) { create(:personal_snippet, :private) }
+  scenario 'User should see snippets that are not private' do
     login_as create(:user)
     visit explore_snippets_path
@@ -13,4 +13,21 @@ feature 'Explore Snippets', feature: true do
     expect(page).to have_content(internal_snippet.title)
     expect(page).not_to have_content(private_snippet.title)
   end
+
+  scenario 'External user should see only public snippets' do
+    login_as create(:user, :external)
+    visit explore_snippets_path
+
+    expect(page).to have_content(public_snippet.title)
+    expect(page).not_to have_content(internal_snippet.title)
+    expect(page).not_to have_content(private_snippet.title)
+  end
+
+  scenario 'Not authenticated user should see only public snippets' do
+    visit explore_snippets_path
+
+    expect(page).to have_content(public_snippet.title)
+    expect(page).not_to have_content(internal_snippet.title)
+    expect(page).not_to have_content(private_snippet.title)
+  end
 end

spec/features/users/snippets_spec.rb

@@ -5,14 +5,46 @@ describe 'Snippets tab on a user profile', feature: true, js: true do
   context 'when the user has snippets' do
     let(:user) { create(:user) }
-    let!(:snippets) { create_list(:snippet, 2, :public, author: user) }
-    before do
-      allow(Snippet).to receive(:default_per_page).and_return(1)
-      visit user_path(user)
-      page.within('.user-profile-nav') { click_link 'Snippets' }
-      wait_for_ajax
+
+    context 'pagination' do
+      let!(:snippets) { create_list(:snippet, 2, :public, author: user) }
+
+      before do
+        allow(Snippet).to receive(:default_per_page).and_return(1)
+        visit user_path(user)
+        page.within('.user-profile-nav') { click_link 'Snippets' }
+        wait_for_ajax
+      end
+
+      it_behaves_like 'paginated snippets', remote: true
     end
-    it_behaves_like 'paginated snippets', remote: true
+    context 'list content' do
+      let!(:public_snippet) { create(:snippet, :public, author: user) }
+      let!(:internal_snippet) { create(:snippet, :internal, author: user) }
+      let!(:private_snippet) { create(:snippet, :private, author: user) }
+      let!(:other_snippet) { create(:snippet, :public) }
+
+      it 'contains only internal and public snippets of a user when a user is logged in' do
+        login_as(:user)
+        visit user_path(user)
+        page.within('.user-profile-nav') { click_link 'Snippets' }
+        wait_for_ajax
+
+        expect(page).to have_selector('.snippet-row', count: 2)
+
+        expect(page).to have_content(public_snippet.title)
+        expect(page).to have_content(internal_snippet.title)
+      end
+
+      it 'contains only public snippets of a user when a user is not logged in' do
+        visit user_path(user)
+        page.within('.user-profile-nav') { click_link 'Snippets' }
+        wait_for_ajax
+
+        expect(page).to have_selector('.snippet-row', count: 1)
+        expect(page).to have_content(public_snippet.title)
+      end
+    end
   end
 end

spec/finders/snippets_finder_spec.rb

@@ -8,79 +8,133 @@ describe SnippetsFinder do
   let(:project1) { create(:empty_project, :public,  group: group) }
   let(:project2) { create(:empty_project, :private, group: group) }
-  context ':all filter' do
+  context 'all snippets visible to a user' do
     let!(:snippet1) { create(:personal_snippet, :private) }
     let!(:snippet2) { create(:personal_snippet, :internal) }
     let!(:snippet3) { create(:personal_snippet, :public) }
-    it "returns all private and internal snippets" do
-      snippets = SnippetsFinder.new.execute(user, filter: :all)
-      expect(snippets).to include(snippet2, snippet3)
-      expect(snippets).not_to include(snippet1)
+    let!(:project_snippet1) { create(:project_snippet, :private) }
+    let!(:project_snippet2) { create(:project_snippet, :internal) }
+    let!(:project_snippet3) { create(:project_snippet, :public) }
+    it "returns all public and internal snippets for normal user" do
+      snippets = SnippetsFinder.new(user).execute
+
+      expect(snippets).to include(snippet2, snippet3, project_snippet2, project_snippet3)
+      expect(snippets).not_to include(snippet1, project_snippet1)
     end
-    it "returns all public snippets" do
-      snippets = SnippetsFinder.new.execute(nil, filter: :all)
-      expect(snippets).to include(snippet3)
-      expect(snippets).not_to include(snippet1, snippet2)
+    it "returns all public snippets for non authorized user" do
+      snippets = SnippetsFinder.new(nil).execute
+
+      expect(snippets).to include(snippet3, project_snippet3)
+      expect(snippets).not_to include(snippet1, snippet2, project_snippet1, project_snippet2)
+    end
+
+    it "returns all public and authored snippets for external user" do
+      external_user = create(:user, :external)
+      authored_snippet = create(:personal_snippet, :internal, author: external_user)
+
+      snippets = SnippetsFinder.new(external_user).execute
+
+      expect(snippets).to include(snippet3, project_snippet3, authored_snippet)
+      expect(snippets).not_to include(snippet1, snippet2, project_snippet1, project_snippet2)
     end
   end
-  context ':public filter' do
+  context 'filter by visibility' do
     let!(:snippet1) { create(:personal_snippet, :private) }
     let!(:snippet2) { create(:personal_snippet, :internal) }
     let!(:snippet3) { create(:personal_snippet, :public) }
-    it "returns public public snippets" do
-      snippets = SnippetsFinder.new.execute(nil, filter: :public)
+    it "returns public snippets when visibility is PUBLIC" do
+      snippets = SnippetsFinder.new(nil, visibility: Snippet::PUBLIC).execute
       expect(snippets).to include(snippet3)
       expect(snippets).not_to include(snippet1, snippet2)
     end
   end
-  context ':by_user filter' do
+  context 'filter by scope' do
+    let!(:snippet1) { create(:personal_snippet, :private, author: user) }
+    let!(:snippet2) { create(:personal_snippet, :internal, author: user) }
+    let!(:snippet3) { create(:personal_snippet, :public, author: user) }
+
+    it "returns all snippets for 'all' scope" do
+      snippets = SnippetsFinder.new(user, scope: :all).execute
+
+      expect(snippets).to include(snippet1, snippet2, snippet3)
+    end
+
+    it "returns all snippets for 'are_private' scope" do
+      snippets = SnippetsFinder.new(user, scope: :are_private).execute
+
+      expect(snippets).to include(snippet1)
+      expect(snippets).not_to include(snippet2, snippet3)
+    end
+
+    it "returns all snippets for 'are_interna;' scope" do
+      snippets = SnippetsFinder.new(user, scope: :are_internal).execute
+
+      expect(snippets).to include(snippet2)
+      expect(snippets).not_to include(snippet1, snippet3)
+    end
+
+    it "returns all snippets for 'are_private' scope" do
+      snippets = SnippetsFinder.new(user, scope: :are_public).execute
+
+      expect(snippets).to include(snippet3)
+      expect(snippets).not_to include(snippet1, snippet2)
+    end
+  end
+
+  context 'filter by author' do
     let!(:snippet1) { create(:personal_snippet, :private, author: user) }
     let!(:snippet2) { create(:personal_snippet, :internal, author: user) }
     let!(:snippet3) { create(:personal_snippet, :public, author: user) }
     it "returns all public and internal snippets" do
-      snippets = SnippetsFinder.new.execute(user1, filter: :by_user, user: user)
+      snippets = SnippetsFinder.new(user1, author: user).execute
+
       expect(snippets).to include(snippet2, snippet3)
       expect(snippets).not_to include(snippet1)
     end
     it "returns internal snippets" do
-      snippets = SnippetsFinder.new.execute(user, filter: :by_user, user: user, scope: "are_internal")
+      snippets = SnippetsFinder.new(user, author: user, visibility: Snippet::INTERNAL).execute
+
       expect(snippets).to include(snippet2)
       expect(snippets).not_to include(snippet1, snippet3)
     end
     it "returns private snippets" do
-      snippets = SnippetsFinder.new.execute(user, filter: :by_user, user: user, scope: "are_private")
+      snippets = SnippetsFinder.new(user, author: user, visibility: Snippet::PRIVATE).execute
+
       expect(snippets).to include(snippet1)
       expect(snippets).not_to include(snippet2, snippet3)
     end
     it "returns public snippets" do
-      snippets = SnippetsFinder.new.execute(user, filter: :by_user, user: user, scope: "are_public")
+      snippets = SnippetsFinder.new(user, author: user, visibility: Snippet::PUBLIC).execute
+
       expect(snippets).to include(snippet3)
       expect(snippets).not_to include(snippet1, snippet2)
     end
     it "returns all snippets" do
-      snippets = SnippetsFinder.new.execute(user, filter: :by_user, user: user)
+      snippets = SnippetsFinder.new(user, author: user).execute
+
       expect(snippets).to include(snippet1, snippet2, snippet3)
     end
     it "returns only public snippets if unauthenticated user" do
-      snippets = SnippetsFinder.new.execute(nil, filter: :by_user, user: user)
+      snippets = SnippetsFinder.new(nil, author: user).execute
+
       expect(snippets).to include(snippet3)
       expect(snippets).not_to include(snippet2, snippet1)
     end
   end
-  context 'by_project filter' do
+  context 'filter by project' do
     before do
       @snippet1 = create(:project_snippet, :private,  project: project1)
       @snippet2 = create(:project_snippet, :internal, project: project1)
@@ -88,43 +142,52 @@ describe SnippetsFinder do
     end
     it "returns public snippets for unauthorized user" do
-      snippets = SnippetsFinder.new.execute(nil, filter: :by_project, project: project1)
+      snippets = SnippetsFinder.new(nil, project: project1).execute
+
       expect(snippets).to include(@snippet3)
       expect(snippets).not_to include(@snippet1, @snippet2)
     end
     it "returns public and internal snippets for non project members" do
-      snippets = SnippetsFinder.new.execute(user, filter: :by_project, project: project1)
+      snippets = SnippetsFinder.new(user, project: project1).execute
+
       expect(snippets).to include(@snippet2, @snippet3)
       expect(snippets).not_to include(@snippet1)
     end
     it "returns public snippets for non project members" do
-      snippets = SnippetsFinder.new.execute(user, filter: :by_project, project: project1, scope: "are_public")
+      snippets = SnippetsFinder.new(user, project: project1, visibility: Snippet::PUBLIC).execute
+
       expect(snippets).to include(@snippet3)
       expect(snippets).not_to include(@snippet1, @snippet2)
     end
     it "returns internal snippets for non project members" do
-      snippets = SnippetsFinder.new.execute(user, filter: :by_project, project: project1, scope: "are_internal")
+      snippets = SnippetsFinder.new(user, project: project1, visibility: Snippet::INTERNAL).execute
+
       expect(snippets).to include(@snippet2)
       expect(snippets).not_to include(@snippet1, @snippet3)
     end
     it "does not return private snippets for non project members" do
-      snippets = SnippetsFinder.new.execute(user, filter: :by_project, project: project1, scope: "are_private")
+      snippets = SnippetsFinder.new(user, project: project1, visibility: Snippet::PRIVATE).execute
+
       expect(snippets).not_to include(@snippet1, @snippet2, @snippet3)
     end
     it "returns all snippets for project members" do
       project1.team << [user, :developer]
-      snippets = SnippetsFinder.new.execute(user, filter: :by_project, project: project1)
+
+      snippets = SnippetsFinder.new(user, project: project1).execute
+
       expect(snippets).to include(@snippet1, @snippet2, @snippet3)
     end
     it "returns private snippets for project members" do
       project1.team << [user, :developer]
-      snippets = SnippetsFinder.new.execute(user, filter: :by_project, project: project1, scope: "are_private")
+
+      snippets = SnippetsFinder.new(user, project: project1, visibility: Snippet::PRIVATE).execute
+
       expect(snippets).to include(@snippet1)
     end
   end