diff options
author | Douwe Maan <douwe@gitlab.com> | 2018-01-18 19:07:06 +0300 |
---|---|---|
committer | Robert Speicher <rspeicher@gmail.com> | 2018-02-09 21:04:05 +0300 |
commit | 5e9e56924a56dcb84c3ae4ae6fc308f635f39f66 (patch) | |
tree | b7160c4277521c309d1f3cc97580c62474cfa759 | |
parent | 721fab661de4a01c2d73e88bdd000dfe2e094ced (diff) |
Merge branch 'security-10-4-25223-snippets-finder-doesnt-obey-feature-visibility' into 'security-10-4'
[Port for security-10-4]: Makes SnippetFinder ensure feature visibility
-rw-r--r-- | app/finders/snippets_finder.rb | 67 | ||||
-rw-r--r-- | app/models/snippet.rb | 21 | ||||
-rw-r--r-- | app/policies/project_policy.rb | 1 | ||||
-rw-r--r-- | spec/finders/snippets_finder_spec.rb | 67 | ||||
-rw-r--r-- | spec/policies/personal_snippet_policy_spec.rb | 1 | ||||
-rw-r--r-- | spec/policies/project_snippet_policy_spec.rb | 1 | ||||
-rw-r--r-- | spec/requests/api/snippets_spec.rb | 21 | ||||
-rw-r--r-- | spec/services/search/snippet_service_spec.rb | 2 | ||||
-rw-r--r-- | spec/support/snippet_visibility.rb | 304 |
9 files changed, 410 insertions, 75 deletions
diff --git a/app/finders/snippets_finder.rb b/app/finders/snippets_finder.rb index 4450766485f..33359fa1efb 100644 --- a/app/finders/snippets_finder.rb +++ b/app/finders/snippets_finder.rb @@ -1,14 +1,28 @@ +# Snippets Finder +# +# Used to filter Snippets collections by a set of params +# +# Arguments. +# +# current_user - The current user, nil also can be used. +# params: +# visibility (integer) - Individual snippet visibility: Public(20), internal(10) or private(0). +# project (Project) - Project related. +# author (User) - Author related. +# +# params are optional class SnippetsFinder < UnionFinder - attr_accessor :current_user, :params + include Gitlab::Allowable + attr_accessor :current_user, :params, :project def initialize(current_user, params = {}) @current_user = current_user @params = params + @project = params[:project] end def execute items = init_collection - items = by_project(items) items = by_author(items) items = by_visibility(items) @@ -18,25 +32,42 @@ class SnippetsFinder < UnionFinder private def init_collection - items = Snippet.all + if project.present? + authorized_snippets_from_project + else + authorized_snippets + end + end - accessible(items) + def authorized_snippets_from_project + if can?(current_user, :read_project_snippet, project) + if project.team.member?(current_user) + project.snippets + else + project.snippets.public_to_user(current_user) + end + else + Snippet.none + end end - def accessible(items) - segments = [] - segments << items.public_to_user(current_user) - segments << authorized_to_user(items) if current_user + def authorized_snippets + Snippet.where(feature_available_projects.or(not_project_related)).public_or_visible_to_user(current_user) + end - find_union(segments, Snippet.includes(:author)) + def feature_available_projects + projects = Project.public_or_visible_to_user(current_user) + .with_feature_available_for_user(:snippets, current_user).select(:id) + arel_query = Arel::Nodes::SqlLiteral.new(projects.to_sql) + table[:project_id].in(arel_query) 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)) + def not_project_related + table[:project_id].eq(nil) + end + + def table + Snippet.arel_table end def by_visibility(items) @@ -53,12 +84,6 @@ class SnippetsFinder < UnionFinder 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 visibility_from_scope case params[:scope].to_s when 'are_private' diff --git a/app/models/snippet.rb b/app/models/snippet.rb index 7c8716f8c18..a58c208279e 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -74,6 +74,27 @@ class Snippet < ActiveRecord::Base @link_reference_pattern ||= super("snippets", /(?<snippet>\d+)/) end + # Returns a collection of snippets that are either public or visible to the + # logged in user. + # + # This method does not verify the user actually has the access to the project + # the snippet is in, so it should be only used on a relation that's already scoped + # for project access + def self.public_or_visible_to_user(user = nil) + if user + authorized = user + .project_authorizations + .select(1) + .where('project_authorizations.project_id = snippets.project_id') + + levels = Gitlab::VisibilityLevel.levels_for_user(user) + + where('EXISTS (?) OR snippets.visibility_level IN (?) or snippets.author_id = (?)', authorized, levels, user.id) + else + public_to_user + end + end + def to_reference(from = nil, full: false) reference = "#{self.class.reference_prefix}#{id}" diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 1dd8f0a25a9..61a7bf02675 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -119,7 +119,6 @@ class ProjectPolicy < BasePolicy enable :create_note enable :upload_file enable :read_cycle_analytics - enable :read_project_snippet end rule { can?(:reporter_access) }.policy do diff --git a/spec/finders/snippets_finder_spec.rb b/spec/finders/snippets_finder_spec.rb index 0a018d2b417..54a07eccaba 100644 --- a/spec/finders/snippets_finder_spec.rb +++ b/spec/finders/snippets_finder_spec.rb @@ -1,57 +1,8 @@ require 'spec_helper' describe SnippetsFinder do - let(:user) { create :user } - let(:user1) { create :user } - let(:group) { create :group, :public } - - let(:project1) { create(:project, :public, group: group) } - let(:project2) { create(:project, :private, group: group) } - - 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) } - let!(:project_snippet1) { create(:project_snippet, :private) } - let!(:project_snippet2) { create(:project_snippet, :internal) } - let!(:project_snippet3) { create(:project_snippet, :public) } - - it "returns all private and internal snippets" do - snippets = described_class.new(user, scope: :all).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 = described_class.new(nil, scope: :all).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 internal snippets for normal user" do - snippets = described_class.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 for non authorized user" do - snippets = described_class.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 = described_class.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 + include Gitlab::Allowable + using RSpec::Parameterized::TableSyntax context 'filter by visibility' do let!(:snippet1) { create(:personal_snippet, :private) } @@ -67,6 +18,7 @@ describe SnippetsFinder do end context 'filter by scope' do + let(:user) { create :user } let!(:snippet1) { create(:personal_snippet, :private, author: user) } let!(:snippet2) { create(:personal_snippet, :internal, author: user) } let!(:snippet3) { create(:personal_snippet, :public, author: user) } @@ -84,7 +36,7 @@ describe SnippetsFinder do expect(snippets).not_to include(snippet2, snippet3) end - it "returns all snippets for 'are_interna;' scope" do + it "returns all snippets for 'are_internal' scope" do snippets = described_class.new(user, scope: :are_internal).execute expect(snippets).to include(snippet2) @@ -100,6 +52,8 @@ describe SnippetsFinder do end context 'filter by author' do + let(:user) { create :user } + let(:user1) { create :user } let!(:snippet1) { create(:personal_snippet, :private, author: user) } let!(:snippet2) { create(:personal_snippet, :internal, author: user) } let!(:snippet3) { create(:personal_snippet, :public, author: user) } @@ -147,6 +101,10 @@ describe SnippetsFinder do end context 'filter by project' do + let(:user) { create :user } + let(:group) { create :group, :public } + let(:project1) { create(:project, :public, group: group) } + before do @snippet1 = create(:project_snippet, :private, project: project1) @snippet2 = create(:project_snippet, :internal, project: project1) @@ -203,4 +161,9 @@ describe SnippetsFinder do expect(snippets).to include(@snippet1) end end + + describe "#execute" do + # Snippet visibility scenarios are included in more details in spec/support/snippet_visibility.rb + include_examples 'snippet visibility', described_class + end end diff --git a/spec/policies/personal_snippet_policy_spec.rb b/spec/policies/personal_snippet_policy_spec.rb index b70c8646a3d..50bb0899eba 100644 --- a/spec/policies/personal_snippet_policy_spec.rb +++ b/spec/policies/personal_snippet_policy_spec.rb @@ -1,5 +1,6 @@ require 'spec_helper' +# Snippet visibility scenarios are included in more details in spec/support/snippet_visibility.rb describe PersonalSnippetPolicy do let(:regular_user) { create(:user) } let(:external_user) { create(:user, :external) } diff --git a/spec/policies/project_snippet_policy_spec.rb b/spec/policies/project_snippet_policy_spec.rb index cdba1b09fc1..4d32e06b553 100644 --- a/spec/policies/project_snippet_policy_spec.rb +++ b/spec/policies/project_snippet_policy_spec.rb @@ -1,5 +1,6 @@ require 'spec_helper' +# Snippet visibility scenarios are included in more details in spec/support/snippet_visibility.rb describe ProjectSnippetPolicy do let(:regular_user) { create(:user) } let(:external_user) { create(:user, :external) } diff --git a/spec/requests/api/snippets_spec.rb b/spec/requests/api/snippets_spec.rb index 74198c8eb4f..b3e253befc6 100644 --- a/spec/requests/api/snippets_spec.rb +++ b/spec/requests/api/snippets_spec.rb @@ -32,6 +32,27 @@ describe API::Snippets do expect(json_response).to be_an Array expect(json_response.size).to eq(0) end + + it 'returns 404 for non-authenticated' do + create(:personal_snippet, :internal) + + get api("/snippets/") + + expect(response).to have_gitlab_http_status(401) + end + + it 'does not return snippets related to a project with disable feature visibility' do + project = create(:project) + create(:project_member, project: project, user: user) + public_snippet = create(:personal_snippet, :public, author: user, project: project) + project.project_feature.update_attribute(:snippets_access_level, 0) + + get api("/snippets/", user) + + json_response.each do |snippet| + expect(snippet["id"]).not_to eq(public_snippet.id) + end + end end describe 'GET /snippets/public' do diff --git a/spec/services/search/snippet_service_spec.rb b/spec/services/search/snippet_service_spec.rb index bc7885b03d9..8ad162ad66e 100644 --- a/spec/services/search/snippet_service_spec.rb +++ b/spec/services/search/snippet_service_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' describe Search::SnippetService do let(:author) { create(:author) } - let(:project) { create(:project) } + let(:project) { create(:project, :public) } let!(:public_snippet) { create(:snippet, :public, content: 'password: XXX') } let!(:internal_snippet) { create(:snippet, :internal, content: 'password: XXX') } diff --git a/spec/support/snippet_visibility.rb b/spec/support/snippet_visibility.rb new file mode 100644 index 00000000000..1cb904823d2 --- /dev/null +++ b/spec/support/snippet_visibility.rb @@ -0,0 +1,304 @@ +RSpec.shared_examples 'snippet visibility' do + let!(:author) { create(:user) } + let!(:member) { create(:user) } + let!(:external) { create(:user, :external) } + + let!(:snippet_type_visibilities) do + { + public: Snippet::PUBLIC, + internal: Snippet::INTERNAL, + private: Snippet::PRIVATE + } + end + + context "For project snippets" do + let!(:users) do + { + unauthenticated: nil, + external: external, + non_member: create(:user), + member: member, + author: author + } + end + + let!(:project_type_visibilities) do + { + public: Gitlab::VisibilityLevel::PUBLIC, + internal: Gitlab::VisibilityLevel::INTERNAL, + private: Gitlab::VisibilityLevel::PRIVATE + } + end + + let(:project_feature_visibilities) do + { + enabled: ProjectFeature::ENABLED, + private: ProjectFeature::PRIVATE, + disabled: ProjectFeature::DISABLED + } + end + + where(:project_type, :feature_visibility, :user_type, :snippet_type, :outcome) do + [ + # Public projects + [:public, :enabled, :unauthenticated, :public, true], + [:public, :enabled, :unauthenticated, :internal, false], + [:public, :enabled, :unauthenticated, :private, false], + + [:public, :enabled, :external, :public, true], + [:public, :enabled, :external, :internal, false], + [:public, :enabled, :external, :private, false], + + [:public, :enabled, :non_member, :public, true], + [:public, :enabled, :non_member, :internal, true], + [:public, :enabled, :non_member, :private, false], + + [:public, :enabled, :member, :public, true], + [:public, :enabled, :member, :internal, true], + [:public, :enabled, :member, :private, true], + + [:public, :enabled, :author, :public, true], + [:public, :enabled, :author, :internal, true], + [:public, :enabled, :author, :private, true], + + [:public, :private, :unauthenticated, :public, false], + [:public, :private, :unauthenticated, :internal, false], + [:public, :private, :unauthenticated, :private, false], + + [:public, :private, :external, :public, false], + [:public, :private, :external, :internal, false], + [:public, :private, :external, :private, false], + + [:public, :private, :non_member, :public, false], + [:public, :private, :non_member, :internal, false], + [:public, :private, :non_member, :private, false], + + [:public, :private, :member, :public, true], + [:public, :private, :member, :internal, true], + [:public, :private, :member, :private, true], + + [:public, :private, :author, :public, true], + [:public, :private, :author, :internal, true], + [:public, :private, :author, :private, true], + + [:public, :disabled, :unauthenticated, :public, false], + [:public, :disabled, :unauthenticated, :internal, false], + [:public, :disabled, :unauthenticated, :private, false], + + [:public, :disabled, :external, :public, false], + [:public, :disabled, :external, :internal, false], + [:public, :disabled, :external, :private, false], + + [:public, :disabled, :non_member, :public, false], + [:public, :disabled, :non_member, :internal, false], + [:public, :disabled, :non_member, :private, false], + + [:public, :disabled, :member, :public, false], + [:public, :disabled, :member, :internal, false], + [:public, :disabled, :member, :private, false], + + [:public, :disabled, :author, :public, false], + [:public, :disabled, :author, :internal, false], + [:public, :disabled, :author, :private, false], + + # Internal projects + [:internal, :enabled, :unauthenticated, :public, false], + [:internal, :enabled, :unauthenticated, :internal, false], + [:internal, :enabled, :unauthenticated, :private, false], + + [:internal, :enabled, :external, :public, false], + [:internal, :enabled, :external, :internal, false], + [:internal, :enabled, :external, :private, false], + + [:internal, :enabled, :non_member, :public, true], + [:internal, :enabled, :non_member, :internal, true], + [:internal, :enabled, :non_member, :private, false], + + [:internal, :enabled, :member, :public, true], + [:internal, :enabled, :member, :internal, true], + [:internal, :enabled, :member, :private, true], + + [:internal, :enabled, :author, :public, true], + [:internal, :enabled, :author, :internal, true], + [:internal, :enabled, :author, :private, true], + + [:internal, :private, :unauthenticated, :public, false], + [:internal, :private, :unauthenticated, :internal, false], + [:internal, :private, :unauthenticated, :private, false], + + [:internal, :private, :external, :public, false], + [:internal, :private, :external, :internal, false], + [:internal, :private, :external, :private, false], + + [:internal, :private, :non_member, :public, false], + [:internal, :private, :non_member, :internal, false], + [:internal, :private, :non_member, :private, false], + + [:internal, :private, :member, :public, true], + [:internal, :private, :member, :internal, true], + [:internal, :private, :member, :private, true], + + [:internal, :private, :author, :public, true], + [:internal, :private, :author, :internal, true], + [:internal, :private, :author, :private, true], + + [:internal, :disabled, :unauthenticated, :public, false], + [:internal, :disabled, :unauthenticated, :internal, false], + [:internal, :disabled, :unauthenticated, :private, false], + + [:internal, :disabled, :external, :public, false], + [:internal, :disabled, :external, :internal, false], + [:internal, :disabled, :external, :private, false], + + [:internal, :disabled, :non_member, :public, false], + [:internal, :disabled, :non_member, :internal, false], + [:internal, :disabled, :non_member, :private, false], + + [:internal, :disabled, :member, :public, false], + [:internal, :disabled, :member, :internal, false], + [:internal, :disabled, :member, :private, false], + + [:internal, :disabled, :author, :public, false], + [:internal, :disabled, :author, :internal, false], + [:internal, :disabled, :author, :private, false], + + # Private projects + [:private, :enabled, :unauthenticated, :public, false], + [:private, :enabled, :unauthenticated, :internal, false], + [:private, :enabled, :unauthenticated, :private, false], + + [:private, :enabled, :external, :public, true], + [:private, :enabled, :external, :internal, true], + [:private, :enabled, :external, :private, true], + + [:private, :enabled, :non_member, :public, false], + [:private, :enabled, :non_member, :internal, false], + [:private, :enabled, :non_member, :private, false], + + [:private, :enabled, :member, :public, true], + [:private, :enabled, :member, :internal, true], + [:private, :enabled, :member, :private, true], + + [:private, :enabled, :author, :public, true], + [:private, :enabled, :author, :internal, true], + [:private, :enabled, :author, :private, true], + + [:private, :private, :unauthenticated, :public, false], + [:private, :private, :unauthenticated, :internal, false], + [:private, :private, :unauthenticated, :private, false], + + [:private, :private, :external, :public, true], + [:private, :private, :external, :internal, true], + [:private, :private, :external, :private, true], + + [:private, :private, :non_member, :public, false], + [:private, :private, :non_member, :internal, false], + [:private, :private, :non_member, :private, false], + + [:private, :private, :member, :public, true], + [:private, :private, :member, :internal, true], + [:private, :private, :member, :private, true], + + [:private, :private, :author, :public, true], + [:private, :private, :author, :internal, true], + [:private, :private, :author, :private, true], + + [:private, :disabled, :unauthenticated, :public, false], + [:private, :disabled, :unauthenticated, :internal, false], + [:private, :disabled, :unauthenticated, :private, false], + + [:private, :disabled, :external, :public, false], + [:private, :disabled, :external, :internal, false], + [:private, :disabled, :external, :private, false], + + [:private, :disabled, :non_member, :public, false], + [:private, :disabled, :non_member, :internal, false], + [:private, :disabled, :non_member, :private, false], + + [:private, :disabled, :member, :public, false], + [:private, :disabled, :member, :internal, false], + [:private, :disabled, :member, :private, false], + + [:private, :disabled, :author, :public, false], + [:private, :disabled, :author, :internal, false], + [:private, :disabled, :author, :private, false] + ] + end + + with_them do + let!(:project) { create(:project, visibility_level: project_type_visibilities[project_type]) } + let!(:project_feature) { project.project_feature.update_column(:snippets_access_level, project_feature_visibilities[feature_visibility]) } + let!(:user) { users[user_type] } + let!(:snippet) { create(:project_snippet, visibility_level: snippet_type_visibilities[snippet_type], project: project, author: author) } + let!(:members) do + project.add_developer(author) + project.add_developer(member) + project.add_developer(external) if project.private? + end + + context "For #{params[:project_type]} project and #{params[:user_type]} users" do + it 'should agree with the read_project_snippet policy' do + expect(can?(user, :read_project_snippet, snippet)).to eq(outcome) + end + + it 'should return proper outcome' do + results = described_class.new(user, project: project).execute + expect(results.include?(snippet)).to eq(outcome) + end + end + + context "Without a given project and #{params[:user_type]} users" do + it 'should return proper outcome' do + results = described_class.new(user).execute + expect(results.include?(snippet)).to eq(outcome) + end + end + end + end + + context 'For personal snippets' do + let!(:users) do + { + unauthenticated: nil, + external: external, + non_member: create(:user), + author: author + } + end + + where(:snippet_visibility, :user_type, :outcome) do + [ + [:public, :unauthenticated, true], + [:public, :external, true], + [:public, :non_member, true], + [:public, :author, true], + + [:internal, :unauthenticated, false], + [:internal, :external, false], + [:internal, :non_member, true], + [:internal, :author, true], + + [:private, :unauthenticated, false], + [:private, :external, false], + [:private, :non_member, false], + [:private, :author, true] + ] + end + + with_them do + let!(:user) { users[user_type] } + let!(:snippet) { create(:personal_snippet, visibility_level: snippet_type_visibilities[snippet_visibility], author: author) } + + context "For personal and #{params[:snippet_visibility]} snippets with #{params[:user_type]} user" do + it 'should agree with read_personal_snippet policy' do + expect(can?(user, :read_personal_snippet, snippet)).to eq(outcome) + end + + it 'should return proper outcome' do + results = described_class.new(user).execute + expect(results.include?(snippet)).to eq(outcome) + end + end + end + end +end |