Merge branch 'fix_set_subscription_permission' into 'master'

Check read_ permission instead of update_ when setting subscription

See merge request gitlab-org/gitlab!61980

app/graphql/mutations/concerns/mutations/resolves_subscription.rb

@@ -3,6 +3,7 @@
 module Mutations
   module ResolvesSubscription
     extend ActiveSupport::Concern
+
     included do
       argument :subscribed_state,
                GraphQL::BOOLEAN_TYPE,

app/graphql/mutations/issues/set_subscription.rb

@@ -2,10 +2,32 @@
 
 module Mutations
   module Issues
-    class SetSubscription < Base
+    class SetSubscription < BaseMutation
       graphql_name 'IssueSetSubscription'
 
       include ResolvesSubscription
+      include Mutations::ResolvesIssuable
+
+      argument :project_path, GraphQL::ID_TYPE,
+               required: true,
+               description: "The project the issue to mutate is in."
+
+      argument :iid, GraphQL::STRING_TYPE,
+               required: true,
+               description: "The IID of the issue to mutate."
+
+      field :issue,
+            Types::IssueType,
+            null: true,
+            description: "The issue after mutation."
+
+      authorize :update_subscription
+
+      private
+
+      def find_object(project_path:, iid:)
+        resolve_issuable(type: :issue, parent_path: project_path, iid: iid)
+      end
     end
   end
 end

app/graphql/mutations/merge_requests/set_subscription.rb

@@ -2,10 +2,32 @@
 
 module Mutations
   module MergeRequests
-    class SetSubscription < Base
+    class SetSubscription < BaseMutation
       graphql_name 'MergeRequestSetSubscription'
 
       include ResolvesSubscription
+      include Mutations::ResolvesIssuable
+
+      argument :project_path, GraphQL::ID_TYPE,
+               required: true,
+               description: "The project the merge request to mutate is in."
+
+      argument :iid, GraphQL::STRING_TYPE,
+               required: true,
+               description: "The IID of the merge request to mutate."
+
+      field :merge_request,
+            Types::MergeRequestType,
+            null: true,
+            description: "The merge request after mutation."
+
+      authorize :update_subscription
+
+      private
+
+      def find_object(project_path:, iid:)
+        resolve_issuable(type: :merge_request, parent_path: project_path, iid: iid)
+      end
     end
   end
 end

app/policies/issue_policy.rb

@@ -38,6 +38,7 @@ class IssuePolicy < IssuablePolicy
 
   rule { ~anonymous & can?(:read_issue) }.policy do
     enable :create_todo
+    enable :update_subscription
   end
 end
 

app/policies/merge_request_policy.rb

@@ -20,6 +20,7 @@ class MergeRequestPolicy < IssuablePolicy
 
   rule { ~anonymous & can?(:read_merge_request) }.policy do
     enable :create_todo
+    enable :update_subscription
   end
 
   condition(:can_merge) { @subject.can_be_merged_by?(@user) }

changelogs/unreleased/fix_set_subscription_permission.yml

+---
+title: Fix permission check when setting issue/merge request subscription in GraphQL
+  API.
+merge_request: 61980
+author:
+type: fixed

spec/graphql/mutations/issues/set_subscription_spec.rb

@@ -3,8 +3,38 @@
 require 'spec_helper'
 
 RSpec.describe Mutations::Issues::SetSubscription do
-  it_behaves_like 'a subscribeable graphql resource' do
-    let_it_be(:resource) { create(:issue) }
-    let(:permission_name) { :update_issue }
+  let_it_be_with_reload(:project) { create(:project) }
+  let_it_be_with_reload(:resource) { create(:issue, project: project) }
+  let_it_be(:user) { create(:user) }
+
+  specify { expect(described_class).to require_graphql_authorizations(:update_subscription) }
+
+  context 'when user does not have access to the project' do
+    it_behaves_like 'a subscribeable not accessible graphql resource'
+  end
+
+  context 'when user is developer member of the project' do
+    before do
+      project.add_developer(user)
+    end
+
+    it_behaves_like 'a subscribeable graphql resource'
+  end
+
+  context 'when the project is public' do
+    before do
+      project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+    end
+
+    it_behaves_like 'a subscribeable graphql resource'
+  end
+
+  context 'when the project is public but the issue is confidential' do
+    before do
+      project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+      resource.update!(confidential: true)
+    end
+
+    it_behaves_like 'a subscribeable not accessible graphql resource'
   end
 end

spec/graphql/mutations/merge_requests/set_subscription_spec.rb

@@ -3,8 +3,30 @@
 require 'spec_helper'
 
 RSpec.describe Mutations::MergeRequests::SetSubscription do
-  it_behaves_like 'a subscribeable graphql resource' do
-    let_it_be(:resource) { create(:merge_request) }
-    let(:permission_name) { :update_merge_request }
+  let_it_be_with_reload(:project) { create(:project) }
+  let_it_be(:user) { create(:user) }
+
+  let(:resource) { create(:merge_request, source_project: project, target_project: project) }
+
+  specify { expect(described_class).to require_graphql_authorizations(:update_subscription) }
+
+  context 'when user does not have access to the project' do
+    it_behaves_like 'a subscribeable not accessible graphql resource'
+  end
+
+  context 'when user is developer member of the project' do
+    before do
+      project.add_developer(user)
+    end
+
+    it_behaves_like 'a subscribeable graphql resource'
+  end
+
+  context 'when the project is public' do
+    before do
+      project.update!(visibility_level: Gitlab::VisibilityLevel::PUBLIC)
+    end
+
+    it_behaves_like 'a subscribeable graphql resource'
   end
 end

spec/policies/issue_policy_spec.rb

@@ -139,13 +139,14 @@ RSpec.describe IssuePolicy do
       create(:project_group_link, group: group, project: project)
     end
 
-    it 'does not allow guest to create todos' do
+    it 'does not allow anonymous user to create todos' do
       expect(permissions(nil, issue)).to be_allowed(:read_issue)
       expect(permissions(nil, issue)).to be_disallowed(:create_todo)
+      expect(permissions(nil, issue)).to be_disallowed(:update_subscription)
     end
 
     it 'allows guests to read issues' do
-      expect(permissions(guest, issue)).to be_allowed(:read_issue, :read_issue_iid, :create_todo)
+      expect(permissions(guest, issue)).to be_allowed(:read_issue, :read_issue_iid, :create_todo, :update_subscription)
       expect(permissions(guest, issue)).to be_disallowed(:update_issue, :admin_issue, :reopen_issue)
 
       expect(permissions(guest, issue_no_assignee)).to be_allowed(:read_issue, :read_issue_iid)
@@ -205,12 +206,18 @@ RSpec.describe IssuePolicy do
       it 'forbids visitors from commenting' do
         expect(permissions(visitor, issue)).to be_disallowed(:create_note)
       end
+      it 'forbids visitors from subscribing' do
+        expect(permissions(visitor, issue)).to be_disallowed(:update_subscription)
+      end
       it 'allows guests to view' do
         expect(permissions(guest, issue)).to be_allowed(:read_issue)
       end
       it 'allows guests to comment' do
         expect(permissions(guest, issue)).to be_allowed(:create_note)
       end
+      it 'allows guests to subscribe' do
+        expect(permissions(guest, issue)).to be_allowed(:update_subscription)
+      end
 
       context 'when admin mode is enabled', :enable_admin_mode do
         it 'allows admins to view' do

spec/policies/merge_request_policy_spec.rb

@@ -26,7 +26,8 @@ RSpec.describe MergeRequestPolicy do
                 read_merge_request
                 create_todo
                 approve_merge_request
-                create_note].freeze
+                create_note
+                update_subscription].freeze
 
   shared_examples_for 'a denied user' do
     let(:perms) { permissions(subject, merge_request) }
@@ -55,7 +56,7 @@ RSpec.describe MergeRequestPolicy do
       subject { permissions(nil, merge_request) }
 
       it do
-        is_expected.to be_disallowed(:create_todo)
+        is_expected.to be_disallowed(:create_todo, :update_subscription)
       end
     end
   end

spec/support/shared_examples/graphql/mutations/resolves_subscription_shared_examples.rb

@@ -2,44 +2,37 @@
 
 require 'spec_helper'
 
-RSpec.shared_examples 'a subscribeable graphql resource' do
-  let(:project) { resource.project }
-  let_it_be(:user) { create(:user) }
-
-  subject(:mutation) { described_class.new(object: nil, context: { current_user: user }, field: nil) }
+RSpec.shared_examples 'a subscribeable not accessible graphql resource' do
+  let(:mutation) { described_class.new(object: nil, context: { current_user: user }, field: nil) }
 
-  specify { expect(described_class).to require_graphql_authorizations(permission_name) }
+  subject { mutation.resolve(project_path: resource.project.full_path, iid: resource.iid, subscribed_state: true) }
 
-  describe '#resolve' do
-    let(:subscribe) { true }
-    let(:mutated_resource) { subject[resource.class.name.underscore.to_sym] }
-
-    subject { mutation.resolve(project_path: resource.project.full_path, iid: resource.iid, subscribed_state: subscribe) }
+  it 'raises an error if the resource is not accessible to the user' do
+    expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
+  end
+end
 
-    it 'raises an error if the resource is not accessible to the user' do
-      expect { subject }.to raise_error(Gitlab::Graphql::Errors::ResourceNotAvailable)
-    end
+RSpec.shared_examples 'a subscribeable graphql resource' do
+  let(:mutated_resource) { subject[resource.class.name.underscore.to_sym] }
+  let(:mutation) { described_class.new(object: nil, context: { current_user: user }, field: nil) }
+  let(:subscribe) { true }
 
-    context 'when the user can update the resource' do
-      before do
-        resource.project.add_developer(user)
-      end
+  subject { mutation.resolve(project_path: resource.project.full_path, iid: resource.iid, subscribed_state: subscribe) }
 
-      it 'subscribes to the resource' do
-        expect(mutated_resource).to eq(resource)
-        expect(mutated_resource.subscribed?(user, project)).to eq(true)
-        expect(subject[:errors]).to be_empty
-      end
+  it 'subscribes to the resource' do
+    expect(mutated_resource).to eq(resource)
+    expect(mutated_resource.subscribed?(user, project)).to eq(true)
+    expect(subject[:errors]).to be_empty
+  end
 
-      context 'when passing subscribe as false' do
-        let(:subscribe) { false }
+  context 'when passing subscribe as false' do
+    let(:subscribe) { false }
 
-        it 'unsubscribes from the discussion' do
-          resource.subscribe(user, project)
+    it 'unsubscribes from the discussion' do
+      resource.subscribe(user, project)
 
-          expect(mutated_resource.subscribed?(user, project)).to eq(false)
-        end
-      end
+      expect(mutated_resource.subscribed?(user, project)).to eq(false)
+      expect(subject[:errors]).to be_empty
     end
   end
 end