Merge branch 'al-31836-guest-access-to-delete-snippets-bug' into 'master'

Guest users should not be able to delete project snippets

See merge request gitlab-org/gitlab!20477

app/policies/project_snippet_policy.rb

@@ -38,6 +38,10 @@ class ProjectSnippetPolicy < BasePolicy
 
   rule { public_snippet }.enable :read_project_snippet
 
+  rule { is_author & ~project.reporter & ~admin }.policy do
+    prevent :admin_project_snippet
+  end
+
   rule { is_author | admin }.policy do
     enable :read_project_snippet
     enable :update_project_snippet

app/views/projects/snippets/_actions.html.haml

@@ -4,7 +4,7 @@
   - if can?(current_user, :update_project_snippet, @snippet)
     = link_to edit_project_snippet_path(@project, @snippet), class: "btn btn-grouped" do
       = _('Edit')
-  - if can?(current_user, :update_project_snippet, @snippet)
+  - if can?(current_user, :admin_project_snippet, @snippet)
     = link_to project_snippet_path(@project, @snippet), method: :delete, data: { confirm: _("Are you sure?") }, class: "btn btn-grouped btn-inverted btn-remove", title: _('Delete Snippet') do
       = _('Delete')
   - if can?(current_user, :create_project_snippet, @project)
@@ -23,7 +23,7 @@
           %li
             = link_to new_project_snippet_path(@project), title: _("New snippet") do
               = _('New snippet')
-        - if can?(current_user, :update_project_snippet, @snippet)
+        - if can?(current_user, :admin_project_snippet, @snippet)
           %li
             = link_to project_snippet_path(@project, @snippet), method: :delete, data: { confirm: _("Are you sure?") }, title: _('Delete Snippet') do
               = _('Delete')

changelogs/unreleased/al-31836-guest-access-to-delete-snippets-bug.yml

+---
+title: Guest users should not delete project snippets they created
+merge_request: 20477
+author:
+type: fixed

spec/policies/project_snippet_policy_spec.rb

@@ -4,10 +4,12 @@ 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) }
-  let(:project) { create(:project, :public) }
-  let(:snippet) { create(:project_snippet, snippet_visibility, project: project) }
+  let_it_be(:regular_user) { create(:user) }
+  let_it_be(:other_user) { create(:user) }
+  let_it_be(:external_user) { create(:user, :external) }
+  let_it_be(:project) { create(:project, :public) }
+  let(:snippet) { create(:project_snippet, snippet_visibility, project: project, author: author) }
+  let(:author) { other_user }
   let(:author_permissions) do
     [
       :update_project_snippet,
@@ -17,6 +19,65 @@ describe ProjectSnippetPolicy do
 
   subject { described_class.new(current_user, snippet) }
 
+  shared_examples 'regular user access rights' do
+    context 'project team member (non guest)' do
+      before do
+        project.add_developer(current_user)
+      end
+
+      it do
+        expect_allowed(:read_project_snippet, :create_note)
+        expect_disallowed(*author_permissions)
+      end
+    end
+
+    context 'project team member (guest)' do
+      before do
+        project.add_guest(current_user)
+      end
+
+      context 'not snippet author' do
+        it do
+          expect_allowed(:read_project_snippet, :create_note)
+          expect_disallowed(:admin_project_snippet)
+        end
+      end
+    end
+
+    context 'snippet author' do
+      let(:author) { current_user }
+
+      context 'project member (non guest)' do
+        before do
+          project.add_developer(current_user)
+        end
+
+        it do
+          expect_allowed(:read_project_snippet, :create_note)
+          expect_allowed(*author_permissions)
+        end
+      end
+
+      context 'project member (guest)' do
+        before do
+          project.add_guest(current_user)
+        end
+
+        it do
+          expect_allowed(:read_project_snippet, :create_note)
+          expect_disallowed(:admin_project_snippet)
+        end
+      end
+
+      context 'not a project member' do
+        it do
+          expect_allowed(:read_project_snippet, :create_note)
+          expect_disallowed(:admin_project_snippet)
+        end
+      end
+    end
+  end
+
   context 'public snippet' do
     let(:snippet_visibility) { :public }
 
@@ -36,6 +97,8 @@ describe ProjectSnippetPolicy do
         expect_allowed(:read_project_snippet, :create_note)
         expect_disallowed(*author_permissions)
       end
+
+      it_behaves_like 'regular user access rights'
     end
 
     context 'external user' do
@@ -45,6 +108,17 @@ describe ProjectSnippetPolicy do
         expect_allowed(:read_project_snippet, :create_note)
         expect_disallowed(*author_permissions)
       end
+
+      context 'project team member' do
+        before do
+          project.add_developer(external_user)
+        end
+
+        it do
+          expect_allowed(:read_project_snippet, :create_note)
+          expect_disallowed(*author_permissions)
+        end
+      end
     end
   end
 
@@ -67,6 +141,8 @@ describe ProjectSnippetPolicy do
         expect_allowed(:read_project_snippet, :create_note)
         expect_disallowed(*author_permissions)
       end
+
+      it_behaves_like 'regular user access rights'
     end
 
     context 'external user' do
@@ -110,33 +186,20 @@ describe ProjectSnippetPolicy do
         expect_disallowed(*author_permissions)
       end
 
-      context 'snippet author' do
-        let(:snippet) { create(:project_snippet, :private, author: regular_user, project: project) }
-
-        it do
-          expect_allowed(:read_project_snippet, :create_note)
-          expect_allowed(*author_permissions)
-        end
-      end
+      it_behaves_like 'regular user access rights'
+    end
 
-      context 'project team member normal user' do
-        before do
-          project.add_developer(regular_user)
-        end
+    context 'external user' do
+      let(:current_user) { external_user }
 
-        it do
-          expect_allowed(:read_project_snippet, :create_note)
-          expect_disallowed(*author_permissions)
-        end
+      it do
+        expect_disallowed(:read_project_snippet, :create_note)
+        expect_disallowed(*author_permissions)
       end
-    end
 
-    context 'external user' do
       context 'project team member' do
-        let(:current_user) { external_user }
-
         before do
-          project.add_developer(external_user)
+          project.add_developer(current_user)
         end
 
         it do