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

Refactor snippets finder & dont return internal snippets for external users

See merge request !2094

10 files changed, 69 insertions, 81 deletions

diff --git a/app/controllers/dashboard/snippets_controller.rb b/app/controllers/dashboard/snippets_controller.rb
index bcfdbe14be9..8dd91264451 100644
--- a/app/controllers/dashboard/snippets_controller.rb
+++ b/app/controllers/dashboard/snippets_controller.rb
@@ -1,11 +1,10 @@
 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
diff --git a/app/controllers/explore/snippets_controller.rb b/app/controllers/explore/snippets_controller.rb
index 28760c3f84b..d3f0e033068 100644
--- a/app/controllers/explore/snippets_controller.rb
+++ b/app/controllers/explore/snippets_controller.rb
@@ -1,6 +1,6 @@
 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
diff --git a/app/controllers/projects/snippets_controller.rb b/app/controllers/projects/snippets_controller.rb
index 66f913f8f9d..3b2b0d9e502 100644
--- a/app/controllers/projects/snippets_controller.rb
+++ b/app/controllers/projects/snippets_controller.rb
@@ -23,12 +23,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)
diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb
index 656a365b701..7445f61195d 100644
--- a/app/controllers/snippets_controller.rb
+++ b/app/controllers/snippets_controller.rb
@@ -27,12 +27,8 @@ class SnippetsController < ApplicationController
 
       return render_404 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
diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index ca89ed221c6..ba22b2f9d29 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -128,12 +128,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
diff --git a/app/finders/notes_finder.rb b/app/finders/notes_finder.rb
index dc6a8ad1f66..02eb983bf55 100644
--- a/app/finders/notes_finder.rb
+++ b/app/finders/notes_finder.rb
@@ -67,7 +67,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
     when "personal_snippet"
       PersonalSnippet.all
     else
diff --git a/app/finders/snippets_finder.rb b/app/finders/snippets_finder.rb
index da6e6e87a6f..c04f61de79c 100644
--- a/app/finders/snippets_finder.rb
+++ b/app/finders/snippets_finder.rb
@@ -1,66 +1,74 @@
-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
+  def accessible(items)
+    segments = []
+    segments << items.public_to_user(current_user)
+    segments << authorized_to_user(items)  if current_user
 
-    if current_user
-      include_private = user == current_user
-      by_scope(snippets, scope, include_private)
-    else
-      snippets.are_public
-    end
+    find_union(segments, Snippet)
   end
 
-  def by_project(current_user, project, scope)
-    snippets = project.snippets.fresh
+  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
 
-    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 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
diff --git a/app/models/snippet.rb b/app/models/snippet.rb
index abfbefdf9a0..882e2fa0594 100644
--- a/app/models/snippet.rb
+++ b/app/models/snippet.rb
@@ -152,18 +152,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
diff --git a/app/policies/project_snippet_policy.rb b/app/policies/project_snippet_policy.rb
index 3a96836917e..cf8ff92617f 100644
--- a/app/policies/project_snippet_policy.rb
+++ b/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
diff --git a/app/services/search/snippet_service.rb b/app/services/search/snippet_service.rb
index 4f161beea4d..85da0be6fff 100644
--- a/app/services/search/snippet_service.rb
+++ b/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