Allow guest user to assign issue metadata on create

Open up permission to let guest users assign issue metadata
on issue create, but still keep guests unable to change issue
metadata afterwards.

re https://gitlab.com/gitlab-org/gitlab/-/issues/300100

Changelog: changed

app/policies/issue_policy.rb

@@ -15,6 +15,9 @@ class IssuePolicy < IssuablePolicy
   desc "Issue is confidential"
   condition(:confidential, scope: :subject) { @subject.confidential? }
 
+  desc "Issue is persisted"
+  condition(:persisted, scope: :subject) { @subject.persisted? }
+
   rule { confidential & ~can_read_confidential }.policy do
     prevent(*create_read_update_admin_destroy(:issue))
     prevent :read_issue_iid
@@ -40,6 +43,14 @@ class IssuePolicy < IssuablePolicy
     enable :create_todo
     enable :update_subscription
   end
+
+  rule { ~persisted & can?(:guest_access) }.policy do
+    enable :set_issue_metadata
+  end
+
+  rule { persisted & can?(:admin_issue) }.policy do
+    enable :set_issue_metadata
+  end
 end
 
 IssuePolicy.prepend_mod_with('IssuePolicy')

app/policies/merge_request_policy.rb

@@ -28,6 +28,10 @@ class MergeRequestPolicy < IssuablePolicy
   rule { can_merge }.policy do
     enable :accept_merge_request
   end
+
+  rule { can?(:admin_merge_request) }.policy do
+    enable :set_merge_request_metadata
+  end
 end
 
 MergeRequestPolicy.prepend_mod_with('MergeRequestPolicy')

app/services/issuable_base_service.rb

@@ -27,8 +27,14 @@ class IssuableBaseService < ::BaseProjectService
     can?(current_user, ability_name, issuable)
   end
 
+  def can_set_issuable_metadata?(issuable)
+    ability_name = :"set_#{issuable.to_ability_name}_metadata"
+
+    can?(current_user, ability_name, issuable)
+  end
+
   def filter_params(issuable)
-    unless can_admin_issuable?(issuable)
+    unless can_set_issuable_metadata?(issuable)
       params.delete(:milestone)
       params.delete(:milestone_id)
       params.delete(:labels)
@@ -45,6 +51,7 @@ class IssuableBaseService < ::BaseProjectService
       params.delete(:canonical_issue_id)
       params.delete(:project)
       params.delete(:discussion_locked)
+      params.delete(:confidential)
     end
 
     filter_assignees(issuable)

app/services/issues/update_service.rb

@@ -20,17 +20,6 @@ module Issues
       super
     end
 
-    override :filter_params
-    def filter_params(issue)
-      super
-
-      # filter confidential in `Issues::UpdateService` and not in `IssuableBaseService#filter_params`
-      # because we do allow users that cannot admin issues to set confidential flag when creating an issue
-      unless can_admin_issuable?(issue)
-        params.delete(:confidential)
-      end
-    end
-
     def before_update(issue, skip_spam_check: false)
       return if skip_spam_check
 

app/views/shared/issuable/_form.html.haml

@@ -24,14 +24,6 @@
 
 = render 'shared/form_elements/description', model: issuable, form: form, project: project
 
-- if issuable.respond_to?(:confidential)
-  .form-group.row
-    .offset-sm-2.col-sm-10
-      .form-check
-        = form.check_box :confidential, class: 'form-check-input'
-        = form.label :confidential, class: 'form-check-label' do
-          This issue is confidential and should only be visible to team members with at least Reporter access.
-
 = render 'shared/issuable/form/metadata', issuable: issuable, form: form, project: project, presenter: presenter
 
 = render_if_exists 'shared/issuable/approvals', issuable: issuable, presenter: presenter, form: form

app/views/shared/issuable/form/_metadata.html.haml

@@ -2,11 +2,19 @@
 - issuable = local_assigns.fetch(:issuable)
 - presenter = local_assigns.fetch(:presenter)
 
-- return unless can?(current_user, :"admin_#{issuable.to_ability_name}", issuable.project)
+- return unless can?(current_user, :"set_#{issuable.to_ability_name}_metadata", issuable)
 
 - has_due_date = issuable.has_attribute?(:due_date)
 - form = local_assigns.fetch(:form)
 
+- if issuable.respond_to?(:confidential)
+  .form-group.row
+    .offset-sm-2.col-sm-10
+      .form-check
+        = form.check_box :confidential, class: 'form-check-input'
+        = form.label :confidential, class: 'form-check-label' do
+          This issue is confidential and should only be visible to team members with at least Reporter access.
+
 %hr
 .row
   %div{ class: (has_due_date ? "col-lg-6" : "col-12") }

ee/app/policies/epic_policy.rb

@@ -35,4 +35,8 @@ class EpicPolicy < BasePolicy
   rule { ~anonymous & can?(:read_epic) }.policy do
     enable :create_todo
   end
+
+  rule { can?(:admin_epic) }.policy do
+    enable :set_epic_metadata
+  end
 end

ee/spec/services/epics/create_service_spec.rb

@@ -12,6 +12,11 @@ RSpec.describe Epics::CreateService do
   subject { described_class.new(group: group, current_user: user, params: params).execute }
 
   describe '#execute' do
+    before do
+      group.add_reporter(user)
+      stub_licensed_features(epics: true, subepics: true)
+    end
+
     it 'creates one epic correctly' do
       allow(NewEpicWorker).to receive(:perform_async)
 
@@ -26,116 +31,111 @@ RSpec.describe Epics::CreateService do
       expect(epic.confidential).to be_truthy
       expect(NewEpicWorker).to have_received(:perform_async).with(epic.id, user.id)
     end
-  end
 
-  context 'handling fixed dates' do
-    it 'sets the fixed date correctly' do
-      date = Date.new(2019, 10, 10)
-      params[:start_date_fixed] = date
-      params[:start_date_is_fixed] = true
+    context 'handling fixed dates' do
+      it 'sets the fixed date correctly' do
+        date = Date.new(2019, 10, 10)
+        params[:start_date_fixed] = date
+        params[:start_date_is_fixed] = true
 
-      subject
+        subject
 
-      epic = Epic.last
-      expect(epic.start_date).to eq(date)
-      expect(epic.start_date_fixed).to eq(date)
-      expect(epic.start_date_is_fixed).to be_truthy
+        epic = Epic.last
+        expect(epic.start_date).to eq(date)
+        expect(epic.start_date_fixed).to eq(date)
+        expect(epic.start_date_is_fixed).to be_truthy
+      end
     end
-  end
 
-  context 'after_save callback to store_mentions' do
-    let(:labels) { create_pair(:group_label, group: group) }
+    context 'after_save callback to store_mentions' do
+      let(:labels) { create_pair(:group_label, group: group) }
 
-    context 'when mentionable attributes change' do
-      context 'when content has no mentions' do
-        let(:params) { { title: 'Title', description: "Description with no mentions" } }
-
-        it 'calls store_mentions! and saves no mentions' do
-          expect_next_instance_of(Epic) do |instance|
-            expect(instance).to receive(:store_mentions!).and_call_original
-          end
+      context 'when mentionable attributes change' do
+        context 'when content has no mentions' do
+          let(:params) { { title: 'Title', description: "Description with no mentions" } }
 
-          expect { subject }.not_to change { EpicUserMention.count }
-        end
-      end
-
-      context 'when content has mentions' do
-        let(:params) { { title: 'Title', description: "Description with #{user.to_reference}" } }
+          it 'calls store_mentions! and saves no mentions' do
+            expect_next_instance_of(Epic) do |instance|
+              expect(instance).to receive(:store_mentions!).and_call_original
+            end
 
-        it 'calls store_mentions! and saves mentions' do
-          expect_next_instance_of(Epic) do |instance|
-            expect(instance).to receive(:store_mentions!).and_call_original
+            expect { subject }.not_to change { EpicUserMention.count }
           end
-
-          expect { subject }.to change { EpicUserMention.count }.by(1)
         end
-      end
 
-      context 'when mentionable.save fails' do
-        let(:params) { { title: '', label_ids: labels.map(&:id) } }
+        context 'when content has mentions' do
+          let(:params) { { title: 'Title', description: "Description with #{user.to_reference}" } }
 
-        it 'does not call store_mentions and saves no mentions' do
-          expect_next_instance_of(Epic) do |instance|
-            expect(instance).not_to receive(:store_mentions!).and_call_original
-          end
-
-          expect { subject }.not_to change { EpicUserMention.count }
-          expect(subject.valid?).to be false
-        end
-      end
+          it 'calls store_mentions! and saves mentions' do
+            expect_next_instance_of(Epic) do |instance|
+              expect(instance).to receive(:store_mentions!).and_call_original
+            end
 
-      context 'when description param has quick action' do
-        before do
-          stub_licensed_features(epics: true, subepics: true)
-          group.add_developer(user)
+            expect { subject }.to change { EpicUserMention.count }.by(1)
+          end
         end
 
-        context 'for /parent_epic' do
-          it 'assigns parent epic' do
-            parent_epic = create(:epic, group: group)
-            description = "/parent_epic #{parent_epic.to_reference}"
-            params = { title: 'New epic with parent', description: description }
+        context 'when mentionable.save fails' do
+          let(:params) { { title: '', label_ids: labels.map(&:id) } }
 
-            epic = described_class.new(group: group, current_user: user, params: params).execute
+          it 'does not call store_mentions and saves no mentions' do
+            expect_next_instance_of(Epic) do |instance|
+              expect(instance).not_to receive(:store_mentions!).and_call_original
+            end
 
-            expect(epic.parent).to eq(parent_epic)
+            expect { subject }.not_to change { EpicUserMention.count }
+            expect(subject.valid?).to be false
           end
+        end
 
-          context 'when parent epic cannot be assigned' do
-            it 'does not assign parent epic' do
-              other_group = create(:group, :private)
-              parent_epic = create(:epic, group: other_group)
-              description = "/parent_epic #{parent_epic.to_reference(group)}"
+        context 'when description param has quick action' do
+          context 'for /parent_epic' do
+            it 'assigns parent epic' do
+              parent_epic = create(:epic, group: group)
+              description = "/parent_epic #{parent_epic.to_reference}"
               params = { title: 'New epic with parent', description: description }
 
               epic = described_class.new(group: group, current_user: user, params: params).execute
 
-              expect(epic.parent).to eq(nil)
+              expect(epic.parent).to eq(parent_epic)
             end
-          end
-        end
 
-        context 'for /child_epic' do
-          it 'sets a child epic' do
-            child_epic = create(:epic, group: group)
-            description = "/child_epic #{child_epic.to_reference}"
-            params = { title: 'New epic with child', description: description }
+            context 'when parent epic cannot be assigned' do
+              it 'does not assign parent epic' do
+                other_group = create(:group, :private)
+                parent_epic = create(:epic, group: other_group)
+                description = "/parent_epic #{parent_epic.to_reference(group)}"
+                params = { title: 'New epic with parent', description: description }
 
-            epic = described_class.new(group: group, current_user: user, params: params).execute
+                epic = described_class.new(group: group, current_user: user, params: params).execute
 
-            expect(epic.reload.children).to include(child_epic)
+                expect(epic.parent).to eq(nil)
+              end
+            end
           end
 
-          context 'when child epic cannot be assigned' do
-            it 'does not set child epic' do
-              other_group = create(:group, :private)
-              child_epic = create(:epic, group: other_group)
-              description = "/child_epic #{child_epic.to_reference(group)}"
+          context 'for /child_epic' do
+            it 'sets a child epic' do
+              child_epic = create(:epic, group: group)
+              description = "/child_epic #{child_epic.to_reference}"
               params = { title: 'New epic with child', description: description }
 
               epic = described_class.new(group: group, current_user: user, params: params).execute
 
-              expect(epic.reload.children).to be_empty
+              expect(epic.reload.children).to include(child_epic)
+            end
+
+            context 'when child epic cannot be assigned' do
+              it 'does not set child epic' do
+                other_group = create(:group, :private)
+                child_epic = create(:epic, group: other_group)
+                description = "/child_epic #{child_epic.to_reference(group)}"
+                params = { title: 'New epic with child', description: description }
+
+                epic = described_class.new(group: group, current_user: user, params: params).execute
+
+                expect(epic.reload.children).to be_empty
+              end
             end
           end
         end

spec/features/issues/user_creates_issue_spec.rb

@@ -30,7 +30,7 @@ RSpec.describe "User creates issue" do
     end
   end
 
-  context "when signed in as guest" do
+  context "when signed in as guest", :js do
     before do
       project.add_guest(user)
       sign_in(user)
@@ -38,41 +38,19 @@ RSpec.describe "User creates issue" do
       visit(new_project_issue_path(project))
     end
 
-    it "creates issue", :js do
-      page.within(".issue-form") do
-        expect(page).to have_no_content("Assign to")
-        .and have_no_content("Labels")
-        .and have_no_content("Milestone")
-
-        expect(page.find('#issue_title')['placeholder']).to eq 'Title'
-        expect(page.find('#issue_description')['placeholder']).to eq 'Write a description or drag your files here…'
+    context 'available metadata' do
+      it 'allows guest to set issue metadata' do
+        page.within(".issue-form") do
+          expect(page).to have_content("Title")
+            .and have_content("Description")
+            .and have_content("Type")
+            .and have_content("Assignee")
+            .and have_content("Milestone")
+            .and have_content("Labels")
+            .and have_content("Due date")
+            .and have_content("This issue is confidential and should only be visible to team members with at least Reporter access.")
+        end
       end
-
-      issue_title = "500 error on profile"
-
-      fill_in("Title", with: issue_title)
-      first('.js-md').click
-      first('.rspec-issuable-form-description').native.send_keys('Description')
-
-      click_button("Create issue")
-
-      expect(page).to have_content(issue_title)
-        .and have_content(user.name)
-        .and have_content(project.name)
-      expect(page).to have_selector('strong', text: 'Description')
-    end
-
-    it 'does not render the issue type dropdown' do
-      expect(page).not_to have_selector('.s-issuable-type-filter-dropdown-wrap')
-    end
-  end
-
-  context "when signed in as developer", :js do
-    before do
-      project.add_developer(user)
-      sign_in(user)
-
-      visit(new_project_issue_path(project))
     end
 
     context "when previewing" do

spec/services/issues/create_service_spec.rb

@@ -18,8 +18,8 @@ RSpec.describe Issues::CreateService do
       let_it_be(:labels) { create_pair(:label, project: project) }
 
       before_all do
-        project.add_maintainer(user)
-        project.add_maintainer(assignee)
+        project.add_guest(user)
+        project.add_guest(assignee)
       end
 
       let(:opts) do
@@ -88,15 +88,11 @@ RSpec.describe Issues::CreateService do
         expect { issue }.to change { project.open_issues_count }.from(0).to(1)
       end
 
-      context 'when current user cannot admin issues in the project' do
-        let_it_be(:guest) { create(:user) }
-
-        before_all do
-          project.add_guest(guest)
-        end
+      context 'when current user cannot set issue metadata in the project' do
+        let_it_be(:non_member) { create(:user) }
 
-        it 'filters out params that cannot be set without the :admin_issue permission' do
-          issue = described_class.new(project: project, current_user: guest, params: opts).execute
+        it 'filters out params that cannot be set without the :set_issue_metadata permission' do
+          issue = described_class.new(project: project, current_user: non_member, params: opts).execute
 
           expect(issue).to be_persisted
           expect(issue.title).to eq('Awesome issue')
@@ -107,8 +103,8 @@ RSpec.describe Issues::CreateService do
           expect(issue.due_date).to be_nil
         end
 
-        it 'creates confidential issues' do
-          issue = described_class.new(project: project, current_user: guest, params: { confidential: true }).execute
+        it 'can create confidential issues' do
+          issue = described_class.new(project: project, current_user: non_member, params: { confidential: true }).execute
 
           expect(issue.confidential).to be_truthy
         end