Require read_* permission to set subscription state

When subscribing to an issue or merge request, we checked if user
can update the resource, but read permissions alone
are sufficient to change subscription state.

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