Merge branch 'nfriend-adjust-release-graphql-permissions' into 'master'

Adjust permissions for Release data in GraphQL endpoint

See merge request gitlab-org/gitlab!33538

app/graphql/types/project_type.rb

@@ -251,7 +251,8 @@ module Types
           null: true,
           description: 'A single release of the project',
           resolver: Resolvers::ReleasesResolver.single,
-          feature_flag: :graphql_release_data
+          feature_flag: :graphql_release_data,
+          authorize: :download_code
 
     field :container_expiration_policy,
           Types::ContainerExpirationPolicyType,

app/graphql/types/release_assets_type.rb

@@ -3,6 +3,7 @@
 module Types
   class ReleaseAssetsType < BaseObject
     graphql_name 'ReleaseAssets'
+    description 'A container for all assets associated with a release'
 
     authorize :read_release
 
@@ -10,7 +11,7 @@ module Types
 
     present_using ReleasePresenter
 
-    field :assets_count, GraphQL::INT_TYPE, null: true,
+    field :count, GraphQL::INT_TYPE, null: true, method: :assets_count,
           description: 'Number of assets of the release'
     field :links, Types::ReleaseLinkType.connection_type, null: true,
           description: 'Asset links of the release'

app/graphql/types/release_link_type.rb

@@ -3,6 +3,7 @@
 module Types
   class ReleaseLinkType < BaseObject
     graphql_name 'ReleaseLink'
+    description 'Represents an asset link associated with a release'
 
     authorize :read_release
 

app/graphql/types/release_source_type.rb

@@ -3,8 +3,9 @@
 module Types
   class ReleaseSourceType < BaseObject
     graphql_name 'ReleaseSource'
+    description 'Represents the source code attached to a release in a particular format'
 
-    authorize :read_release_sources
+    authorize :download_code
 
     field :format, GraphQL::STRING_TYPE, null: true,
           description: 'Format of the source'

app/graphql/types/release_type.rb

@@ -3,6 +3,7 @@
 module Types
   class ReleaseType < BaseObject
     graphql_name 'Release'
+    description 'Represents a release'
 
     authorize :read_release
 
@@ -10,10 +11,12 @@ module Types
 
     present_using ReleasePresenter
 
-    field :tag_name, GraphQL::STRING_TYPE, null: false, method: :tag,
-          description: 'Name of the tag associated with the release'
+    field :tag_name, GraphQL::STRING_TYPE, null: true, method: :tag,
+          description: 'Name of the tag associated with the release',
+          authorize: :download_code
     field :tag_path, GraphQL::STRING_TYPE, null: true,
-          description: 'Relative web path to the tag associated with the release'
+          description: 'Relative web path to the tag associated with the release',
+          authorize: :download_code
     field :description, GraphQL::STRING_TYPE, null: true,
           description: 'Description (also known as "release notes") of the release'
     markdown_field :description_html, null: true
@@ -39,8 +42,7 @@ module Types
 
     field :commit, Types::CommitType, null: true,
           complexity: 10, calls_gitaly: true,
-          description: 'The commit associated with the release',
-          authorize: :reporter_access
+          description: 'The commit associated with the release'
 
     def commit
       return if release.sha.nil?

app/policies/releases/source_policy.rb

@@ -3,11 +3,5 @@
 module Releases
   class SourcePolicy < BasePolicy
     delegate { @subject.project }
-
-    rule { can?(:public_access) | can?(:reporter_access) }.policy do
-      enable :read_release_sources
-    end
-
-    rule { ~can?(:read_release) }.prevent :read_release_sources
   end
 end

app/presenters/release_presenter.rb

@@ -5,7 +5,7 @@ class ReleasePresenter < Gitlab::View::Presenter::Delegated
 
   presents :release
 
-  delegate :project, :tag, :assets_count, to: :release
+  delegate :project, :tag, to: :release
 
   def commit_path
     return unless release.commit && can_download_code?
@@ -43,6 +43,18 @@ class ReleasePresenter < Gitlab::View::Presenter::Delegated
     edit_project_release_url(project, release)
   end
 
+  def assets_count
+    if can_download_code?
+      release.assets_count
+    else
+      release.assets_count(except: [:sources])
+    end
+  end
+
+  def name
+    can_download_code? ? release.name : "Release-#{release.id}"
+  end
+
   private
 
   def can_download_code?

doc/api/graphql/reference/gitlab_schema.graphql

@@ -10102,6 +10102,9 @@ enum RegistryState {
   SYNCED
 }
 
+"""
+Represents a release
+"""
 type Release {
   """
   Assets of the release
@@ -10196,7 +10199,7 @@ type Release {
   """
   Name of the tag associated with the release
   """
-  tagName: String!
+  tagName: String
 
   """
   Relative web path to the tag associated with the release
@@ -10204,11 +10207,14 @@ type Release {
   tagPath: String
 }
 
+"""
+A container for all assets associated with a release
+"""
 type ReleaseAssets {
   """
   Number of assets of the release
   """
-  assetsCount: Int
+  count: Int
 
   """
   Asset links of the release
@@ -10356,6 +10362,9 @@ type ReleaseEvidenceEdge {
   node: ReleaseEvidence
 }
 
+"""
+Represents an asset link associated with a release
+"""
 type ReleaseLink {
   """
   Indicates the link points to an external resource
@@ -10443,6 +10452,9 @@ enum ReleaseLinkType {
   RUNBOOK
 }
 
+"""
+Represents the source code attached to a release in a particular format
+"""
 type ReleaseSource {
   """
   Format of the source

doc/api/graphql/reference/gitlab_schema.json

@@ -29588,7 +29588,7 @@
         {
           "kind": "OBJECT",
           "name": "Release",
-          "description": null,
+          "description": "Represents a release",
           "fields": [
             {
               "name": "assets",
@@ -29815,13 +29815,9 @@
 
               ],
               "type": {
-                "kind": "NON_NULL",
-                "name": null,
-                "ofType": {
                 "kind": "SCALAR",
                 "name": "String",
                 "ofType": null
-                }
               },
               "isDeprecated": false,
               "deprecationReason": null
@@ -29851,10 +29847,10 @@
         {
           "kind": "OBJECT",
           "name": "ReleaseAssets",
-          "description": null,
+          "description": "A container for all assets associated with a release",
           "fields": [
             {
-              "name": "assetsCount",
+              "name": "count",
               "description": "Number of assets of the release",
               "args": [
 
@@ -30281,7 +30277,7 @@
         {
           "kind": "OBJECT",
           "name": "ReleaseLink",
-          "description": null,
+          "description": "Represents an asset link associated with a release",
           "fields": [
             {
               "name": "external",
@@ -30515,7 +30511,7 @@
         {
           "kind": "OBJECT",
           "name": "ReleaseSource",
-          "description": null,
+          "description": "Represents the source code attached to a release in a particular format",
           "fields": [
             {
               "name": "format",

doc/api/graphql/reference/index.md

@@ -1400,6 +1400,8 @@ Represents a Project Member
 
 ## Release
 
+Represents a release
+
 | Name  | Type  | Description |
 | ---   |  ---- | ----------  |
 | `assets` | ReleaseAssets | Assets of the release |
@@ -1410,14 +1412,16 @@ Represents a Project Member
 | `descriptionHtml` | String | The GitLab Flavored Markdown rendering of `description` |
 | `name` | String | Name of the release |
 | `releasedAt` | Time | Timestamp of when the release was released |
-| `tagName` | String! | Name of the tag associated with the release |
+| `tagName` | String | Name of the tag associated with the release |
 | `tagPath` | String | Relative web path to the tag associated with the release |
 
 ## ReleaseAssets
 
+A container for all assets associated with a release
+
 | Name  | Type  | Description |
 | ---   |  ---- | ----------  |
-| `assetsCount` | Int | Number of assets of the release |
+| `count` | Int | Number of assets of the release |
 
 ## ReleaseEvidence
 
@@ -1432,6 +1436,8 @@ Evidence for a release
 
 ## ReleaseLink
 
+Represents an asset link associated with a release
+
 | Name  | Type  | Description |
 | ---   |  ---- | ----------  |
 | `external` | Boolean | Indicates the link points to an external resource |
@@ -1442,6 +1448,8 @@ Evidence for a release
 
 ## ReleaseSource
 
+Represents the source code attached to a release in a particular format
+
 | Name  | Type  | Description |
 | ---   |  ---- | ----------  |
 | `format` | String | Format of the source |

lib/api/entities/release.rb

@@ -5,9 +5,7 @@ module API
     class Release < Grape::Entity
       include ::API::Helpers::Presentable
 
-      expose :name do |release, _|
-        can_download_code? ? release.name : "Release-#{release.id}"
-      end
+      expose :name
       expose :tag, as: :tag_name, if: ->(_, _) { can_download_code? }
       expose :description
       expose :description_html do |entity|
@@ -23,10 +21,7 @@ module API
       expose :tag_path, expose_nil: false
 
       expose :assets do
-        expose :assets_count, as: :count do |release, _|
-          assets_to_exclude = can_download_code? ? [] : [:sources]
-          release.assets_count(except: assets_to_exclude)
-        end
+        expose :assets_count, as: :count
         expose :sources, using: Entities::Releases::Source, if: ->(_, _) { can_download_code? }
         expose :links, using: Entities::Releases::Link do |release, options|
           release.links.sorted

spec/graphql/types/release_assets_type_spec.rb

@@ -7,7 +7,7 @@ describe GitlabSchema.types['ReleaseAssets'] do
 
   it 'has the expected fields' do
     expected_fields = %w[
-      assets_count links sources
+      count links sources
     ]
 
     expect(described_class).to include_graphql_fields(*expected_fields)

spec/graphql/types/release_source_type_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 describe GitlabSchema.types['ReleaseSource'] do
-  it { expect(described_class).to require_graphql_authorizations(:read_release_sources) }
+  it { expect(described_class).to require_graphql_authorizations(:download_code) }
 
   it 'has the expected fields' do
     expected_fields = %w[

spec/graphql/types/release_type_spec.rb

@@ -44,6 +44,5 @@ describe GitlabSchema.types['Release'] do
     subject { described_class.fields['commit'] }
 
     it { is_expected.to have_graphql_type(Types::CommitType) }
-    it { is_expected.to require_graphql_authorizations(:reporter_access) }
   end
 end

spec/policies/releases/source_policy_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-describe Releases::SourcePolicy do
-  using RSpec::Parameterized::TableSyntax
-
-  let(:policy) { described_class.new(user, source) }
-
-  let_it_be(:public_user) { create(:user) }
-  let_it_be(:guest) { create(:user) }
-  let_it_be(:reporter) { create(:user) }
-
-  let(:release) { create(:release, project: project) }
-  let(:source) { release.sources.first }
-
-  shared_examples 'source code access' do
-    it "allows access a release's source code" do
-      expect(policy).to be_allowed(:read_release_sources)
-    end
-  end
-
-  shared_examples 'no source code access' do
-    it "does not allow access a release's source code" do
-      expect(policy).to be_disallowed(:read_release_sources)
-    end
-  end
-
-  context 'a private project' do
-    let_it_be(:project) { create(:project, :private) }
-
-    context 'accessed by a public user' do
-      let(:user) { public_user }
-
-      it_behaves_like 'no source code access'
-    end
-
-    context 'accessed by a user with Guest permissions' do
-      let(:user) { guest }
-
-      before do
-        project.add_guest(user)
-      end
-
-      it_behaves_like 'no source code access'
-    end
-
-    context 'accessed by a user with Reporter permissions' do
-      let(:user) { reporter }
-
-      before do
-        project.add_reporter(user)
-      end
-
-      it_behaves_like 'source code access'
-    end
-  end
-
-  context 'a public project' do
-    let_it_be(:project) { create(:project, :public) }
-
-    context 'accessed by a public user' do
-      let(:user) { public_user }
-
-      it_behaves_like 'source code access'
-    end
-
-    context 'accessed by a user with Guest permissions' do
-      let(:user) { guest }
-
-      before do
-        project.add_guest(user)
-      end
-
-      it_behaves_like 'source code access'
-    end
-
-    context 'accessed by a user with Reporter permissions' do
-      let(:user) { reporter }
-
-      before do
-        project.add_reporter(user)
-      end
-
-      it_behaves_like 'source code access'
-    end
-  end
-end

spec/presenters/release_presenter_spec.rb

@@ -112,4 +112,36 @@ describe ReleasePresenter do
       it { is_expected.to be_nil }
     end
   end
+
+  describe '#assets_count' do
+    subject { presenter.assets_count }
+
+    it 'returns the number of assets associated to the release' do
+      is_expected.to be release.assets_count
+    end
+
+    context 'when a user is not allowed to download release sources' do
+      let(:presenter) { described_class.new(release, current_user: guest) }
+
+      it 'returns the number of all non-source assets associated to the release' do
+        is_expected.to be release.assets_count(except: [:sources])
+      end
+    end
+  end
+
+  describe '#name' do
+    subject { presenter.name }
+
+    it 'returns the release name' do
+      is_expected.to eq release.name
+    end
+
+    context "when a user is not allowed to access any repository information" do
+      let(:presenter) { described_class.new(release, current_user: guest) }
+
+      it 'returns a replacement name to avoid potentially leaking tag information' do
+        is_expected.to eq "Release-#{release.id}"
+      end
+    end
+  end
 end

spec/requests/api/graphql/project/release_spec.rb

@@ -7,15 +7,10 @@ describe 'Query.project(fullPath).release(tagName)' do
   include GraphqlHelpers
   include Presentable
 
-  let_it_be(:project) { create(:project, :repository) }
-  let_it_be(:milestone_1) { create(:milestone, project: project) }
-  let_it_be(:milestone_2) { create(:milestone, project: project) }
-  let_it_be(:release) { create(:release, :with_evidence, project: project, milestones: [milestone_1, milestone_2]) }
-  let_it_be(:release_link_1) { create(:release_link, release: release) }
-  let_it_be(:release_link_2) { create(:release_link, release: release) }
   let_it_be(:developer) { create(:user) }
-
-  let(:current_user) { developer }
+  let_it_be(:guest) { create(:user) }
+  let_it_be(:reporter) { create(:user) }
+  let_it_be(:stranger) { create(:user) }
 
   def query(rq = release_fields)
     graphql_query_for(:project, { fullPath: project.full_path },
@@ -27,12 +22,10 @@ describe 'Query.project(fullPath).release(tagName)' do
 
   let(:data) { graphql_data.dig(*path) }
 
-  before do
-    project.add_developer(developer)
-  end
-
+  shared_examples 'full access to the release field' do
     describe 'scalar fields' do
       let(:path) { path_prefix }
+
       let(:release_fields) do
         query_graphql_field(%{
           tagName
@@ -64,6 +57,7 @@ describe 'Query.project(fullPath).release(tagName)' do
 
     describe 'milestones' do
       let(:path) { path_prefix + %w[milestones nodes] }
+
       let(:release_fields) do
         query_graphql_field(:milestones, nil, 'nodes { id title }')
       end
@@ -81,6 +75,7 @@ describe 'Query.project(fullPath).release(tagName)' do
 
     describe 'author' do
       let(:path) { path_prefix + %w[author] }
+
       let(:release_fields) do
         query_graphql_field(:author, nil, 'id username')
       end
@@ -97,6 +92,7 @@ describe 'Query.project(fullPath).release(tagName)' do
 
     describe 'commit' do
       let(:path) { path_prefix + %w[commit] }
+
       let(:release_fields) do
         query_graphql_field(:commit, nil, 'sha')
       end
@@ -109,21 +105,23 @@ describe 'Query.project(fullPath).release(tagName)' do
     end
 
     describe 'assets' do
-    describe 'assetsCount' do
+      describe 'count' do
         let(:path) { path_prefix + %w[assets] }
+
         let(:release_fields) do
-        query_graphql_field(:assets, nil, 'assetsCount')
+          query_graphql_field(:assets, nil, 'count')
         end
 
         it 'returns the number of assets associated to the release' do
           post_query
 
-        expect(data).to eq({ 'assetsCount' => release.sources.size + release.links.size })
+          expect(data).to eq({ 'count' => release.sources.size + release.links.size })
         end
       end
 
       describe 'links' do
         let(:path) { path_prefix + %w[assets links nodes] }
+
         let(:release_fields) do
           query_graphql_field(:assets, nil,
             query_graphql_field(:links, nil, 'nodes { id name url external }'))
@@ -147,6 +145,7 @@ describe 'Query.project(fullPath).release(tagName)' do
 
       describe 'sources' do
         let(:path) { path_prefix + %w[assets sources nodes] }
+
         let(:release_fields) do
           query_graphql_field(:assets, nil,
             query_graphql_field(:sources, nil, 'nodes { format url }'))
@@ -165,14 +164,15 @@ describe 'Query.project(fullPath).release(tagName)' do
           expect(data).to match_array(expected)
         end
       end
+    end
 
     describe 'evidences' do
       let(:path) { path_prefix + %w[evidences] }
+
       let(:release_fields) do
         query_graphql_field(:evidences, nil, 'nodes { id sha filepath collectedAt }')
       end
 
-      context 'for a developer' do
       it 'finds all evidence fields' do
         post_query
 
@@ -187,19 +187,102 @@ describe 'Query.project(fullPath).release(tagName)' do
         expect(data["nodes"].first).to eq(expected)
       end
     end
+  end
 
-      context 'for a guest' do
-        let(:current_user) { create :user }
+  shared_examples 'no access to the release field' do
+    describe 'repository-related fields' do
+      let(:path) { path_prefix }
 
-        before do
-          project.add_guest(current_user)
+      let(:release_fields) do
+        query_graphql_field('description')
       end
 
-        it 'denies access' do
+      before do
         post_query
+      end
+
+      it 'returns nil' do
+        expect(data).to eq(nil)
+      end
+    end
+  end
+
+  describe "ensures that the correct data is returned based on the project's visibility and the user's access level" do
+    context 'when the project is private' do
+      let_it_be(:project) { create(:project, :repository, :private) }
+      let_it_be(:milestone_1) { create(:milestone, project: project) }
+      let_it_be(:milestone_2) { create(:milestone, project: project) }
+      let_it_be(:release) { create(:release, :with_evidence, project: project, milestones: [milestone_1, milestone_2]) }
+      let_it_be(:release_link_1) { create(:release_link, release: release) }
+      let_it_be(:release_link_2) { create(:release_link, release: release) }
 
-          expect(data['node']).to be_nil
+      before_all do
+        project.add_developer(developer)
+        project.add_guest(guest)
+        project.add_reporter(reporter)
+      end
+
+      context 'when the user is not logged in' do
+        let(:current_user) { stranger }
+
+        it_behaves_like 'no access to the release field'
+      end
+
+      context 'when the user has Guest permissions' do
+        let(:current_user) { guest }
+
+        it_behaves_like 'no access to the release field'
+      end
+
+      context 'when the user has Reporter permissions' do
+        let(:current_user) { reporter }
+
+        it_behaves_like 'full access to the release field'
+      end
+
+      context 'when the user has Developer permissions' do
+        let(:current_user) { developer }
+
+        it_behaves_like 'full access to the release field'
+      end
+    end
+
+    context 'when the project is public' do
+      let_it_be(:project) { create(:project, :repository, :public) }
+      let_it_be(:milestone_1) { create(:milestone, project: project) }
+      let_it_be(:milestone_2) { create(:milestone, project: project) }
+      let_it_be(:release) { create(:release, :with_evidence, project: project, milestones: [milestone_1, milestone_2]) }
+      let_it_be(:release_link_1) { create(:release_link, release: release) }
+      let_it_be(:release_link_2) { create(:release_link, release: release) }
+
+      before_all do
+        project.add_developer(developer)
+        project.add_guest(guest)
+        project.add_reporter(reporter)
+      end
+
+      context 'when the user is not logged in' do
+        let(:current_user) { stranger }
+
+        it_behaves_like 'full access to the release field'
       end
+
+      context 'when the user has Guest permissions' do
+        let(:current_user) { guest }
+
+        it_behaves_like 'full access to the release field'
+      end
+
+      context 'when the user has Reporter permissions' do
+        let(:current_user) { reporter }
+
+        it_behaves_like 'full access to the release field'
+      end
+
+      context 'when the user has Developer permissions' do
+        let(:current_user) { developer }
+
+        it_behaves_like 'full access to the release field'
       end
     end
   end

spec/requests/api/graphql/project/releases_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'Query.project(fullPath).releases()' do
+  include GraphqlHelpers
+
+  let_it_be(:guest) { create(:user) }
+  let_it_be(:reporter) { create(:user) }
+  let_it_be(:stranger) { create(:user) }
+
+  let(:query) do
+    graphql_query_for(:project, { fullPath: project.full_path },
+    %{
+      releases {
+        nodes {
+          tagName
+          tagPath
+          name
+          commit {
+            sha
+          }
+          assets {
+            count
+            sources {
+              nodes {
+                url
+              }
+            }
+          }
+          evidences {
+            nodes {
+              sha
+            }
+          }
+        }
+      }
+    })
+  end
+
+  let(:post_query) { post_graphql(query, current_user: current_user) }
+
+  let(:data) { graphql_data.dig('project', 'releases', 'nodes', 0) }
+
+  shared_examples 'full access to all repository-related fields' do
+    describe 'repository-related fields' do
+      before do
+        post_query
+      end
+
+      it 'returns data for fields that are protected in private projects' do
+        expected_sources = release.sources.map do |s|
+          { 'url' => s.url }
+        end
+
+        expected_evidences = release.evidences.map do |e|
+          { 'sha' => e.sha }
+        end
+
+        expect(data).to eq({
+          'tagName' => release.tag,
+          'tagPath' => project_tag_path(project, release.tag),
+          'name' => release.name,
+          'commit' => {
+            'sha' => release.commit.sha
+          },
+          'assets' => {
+            'count' => release.assets_count,
+            'sources' => {
+              'nodes' => expected_sources
+            }
+          },
+          'evidences' => {
+            'nodes' => expected_evidences
+          }
+        })
+      end
+    end
+  end
+
+  shared_examples 'no access to any repository-related fields' do
+    describe 'repository-related fields' do
+      before do
+        post_query
+      end
+
+      it 'does not return data for fields that expose repository information' do
+        expect(data).to eq({
+          'tagName' => nil,
+          'tagPath' => nil,
+          'name' => "Release-#{release.id}",
+          'commit' => nil,
+          'assets' => {
+            'count' => release.assets_count(except: [:sources]),
+            'sources' => {
+              'nodes' => []
+            }
+          },
+          'evidences' => {
+            'nodes' => []
+          }
+        })
+      end
+    end
+  end
+
+  describe "ensures that the correct data is returned based on the project's visibility and the user's access level" do
+    context 'when the project is private' do
+      let_it_be(:project) { create(:project, :repository, :private) }
+      let_it_be(:release) { create(:release, :with_evidence, project: project) }
+
+      before_all do
+        project.add_guest(guest)
+        project.add_reporter(reporter)
+      end
+
+      context 'when the user has Guest permissions' do
+        let(:current_user) { guest }
+
+        it_behaves_like 'no access to any repository-related fields'
+      end
+
+      context 'when the user has Reporter permissions' do
+        let(:current_user) { reporter }
+
+        it_behaves_like 'full access to all repository-related fields'
+      end
+    end
+
+    context 'when the project is public' do
+      let_it_be(:project) { create(:project, :repository, :public) }
+      let_it_be(:release) { create(:release, :with_evidence, project: project) }
+
+      before_all do
+        project.add_guest(guest)
+        project.add_reporter(reporter)
+      end
+
+      context 'when the user is not logged in' do
+        let(:current_user) { stranger }
+
+        it_behaves_like 'full access to all repository-related fields'
+      end
+
+      context 'when the user has Guest permissions' do
+        let(:current_user) { guest }
+
+        it_behaves_like 'full access to all repository-related fields'
+      end
+    end
+  end
+end