Merge branch 'allow-guests-to-see-tag-names-associated-with-releases' into 'master'

Resolve "Allow guests to see tag names associated with releases"

See merge request gitlab-org/gitlab!69861

app/controllers/projects/releases_controller.rb

@@ -5,9 +5,6 @@ class Projects::ReleasesController < Projects::ApplicationController
   before_action :require_non_empty_project, except: [:index]
   before_action :release, only: %i[edit show update downloads]
   before_action :authorize_read_release!
-  # We have to check `download_code` permission because detail URL path
-  # contains git-tag name.
-  before_action :authorize_download_code!, except: [:index]
   before_action :authorize_update_release!, only: %i[edit update]
   before_action :authorize_create_release!, only: :new
   before_action only: :index do

app/graphql/types/project_type.rb

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

app/graphql/types/release_links_type.rb

@@ -4,7 +4,7 @@ module Types
   class ReleaseLinksType < BaseObject
     graphql_name 'ReleaseLinks'
 
-    authorize :download_code
+    authorize :read_release
 
     alias_method :release, :object
 
@@ -16,14 +16,19 @@ module Types
           description: "HTTP URL of the release's edit page.",
           authorize: :update_release
     field :opened_merge_requests_url, GraphQL::Types::String, null: true,
-          description: 'HTTP URL of the merge request page, filtered by this release and `state=open`.'
+          description: 'HTTP URL of the merge request page, filtered by this release and `state=open`.',
+          authorize: :download_code
     field :merged_merge_requests_url, GraphQL::Types::String, null: true,
-          description: 'HTTP URL of the merge request page , filtered by this release and `state=merged`.'
+          description: 'HTTP URL of the merge request page , filtered by this release and `state=merged`.',
+          authorize: :download_code
     field :closed_merge_requests_url, GraphQL::Types::String, null: true,
-          description: 'HTTP URL of the merge request page , filtered by this release and `state=closed`.'
+          description: 'HTTP URL of the merge request page , filtered by this release and `state=closed`.',
+          authorize: :download_code
     field :opened_issues_url, GraphQL::Types::String, null: true,
-          description: 'HTTP URL of the issues page, filtered by this release and `state=open`.'
+          description: 'HTTP URL of the issues page, filtered by this release and `state=open`.',
+          authorize: :download_code
     field :closed_issues_url, GraphQL::Types::String, null: true,
-          description: 'HTTP URL of the issues page, filtered by this release and `state=closed`.'
+          description: 'HTTP URL of the issues page, filtered by this release and `state=closed`.',
+          authorize: :download_code
   end
 end

app/graphql/types/release_type.rb

@@ -14,8 +14,7 @@ module Types
     present_using ReleasePresenter
 
     field :tag_name, GraphQL::Types::String, null: true, method: :tag,
-          description: 'Name of the tag associated with the release.',
-          authorize: :download_code
+          description: 'Name of the tag associated with the release.'
     field :tag_path, GraphQL::Types::String, null: true,
           description: 'Relative web path to the tag associated with the release.',
           authorize: :download_code

app/presenters/release_presenter.rb

@@ -22,8 +22,6 @@ class ReleasePresenter < Gitlab::View::Presenter::Delegated
   end
 
   def self_url
-    return unless can_download_code?
-
     project_release_url(project, release)
   end
 
@@ -64,7 +62,7 @@ class ReleasePresenter < Gitlab::View::Presenter::Delegated
 
   delegator_override :name
   def name
-    can_download_code? ? release.name : "Release-#{release.id}"
+    release.name
   end
 
   def download_url(filepath)

doc/user/permissions.md

@@ -216,7 +216,7 @@ The following table lists project permissions available for each role:
 1. If **Public pipelines** is enabled in **Project Settings > CI/CD**.
 1. Not allowed for Guest, Reporter, Developer, Maintainer, or Owner. See [protected branches](project/protected_branches.md).
 1. If the [branch is protected](project/protected_branches.md), this depends on the access Developers and Maintainers are given.
-1. Guest users can access GitLab [**Releases**](project/releases/index.md) for downloading assets but are not allowed to download the source code nor see repository information like tags and commits.
+1. Guest users can access GitLab [**Releases**](project/releases/index.md) for downloading assets but are not allowed to download the source code nor see [repository information like commits and release evidence](project/releases/index.md#view-a-release-and-download-assets).
 1. Actions are limited only to records owned (referenced) by user.
 1. When [Share Group Lock](group/index.md#prevent-a-project-from-being-shared-with-groups) is enabled the project can't be shared with other groups. It does not affect group with group sharing.
 1. For information on eligible approvers for merge requests, see

doc/user/project/releases/index.md

@@ -393,14 +393,6 @@ The release title can be customized using the **Release title** field when
 creating or editing a release. If no title is provided, the release's tag name
 is used instead.
 
-Guest users of private projects are allowed to view the **Releases** page
-but are _not_ allowed to view details about the Git repository (in particular,
-tag names). Because of this, release titles are replaced with a generic
-title like "Release-1234" for Guest users to avoid leaking tag name information.
-
-See the [Release permissions](#release-permissions) section for
-more information about permissions.
-
 ### Tag name
 
 The release tag name should include the release version. GitLab uses [Semantic Versioning](https://semver.org/)
@@ -724,11 +716,14 @@ In the API:
 
 ### View a release and download assets
 
+> [The Guest permission for read action was adjusted](https://gitlab.com/gitlab-org/gitlab/-/issues/335209) in GitLab 14.5.
+
 - Users with [Reporter role or above](../../../user/permissions.md#project-members-permissions)
   have read and download access to the project releases.
 - Users with [Guest role](../../../user/permissions.md#project-members-permissions)
-  have read and download access to the project releases, however,
-  repository-related information are redacted (for example the Git tag name).
+  have read and download access to the project releases.
+  This includes associated Git-tag-names, release description, author information of the releases.
+  However, other repository-related information, such as [source code](#source-code), [release evidence](#release-evidence) are redacted.
 
 ### Create, update, and delete a release and its assets
 

spec/controllers/projects/releases_controller_spec.rb

@@ -207,7 +207,18 @@ RSpec.describe Projects::ReleasesController do
       let(:project) { private_project }
       let(:user) { guest }
 
-      it_behaves_like 'not found'
+      it_behaves_like 'successful request'
+    end
+
+    context 'when user is an external user for the project' do
+      let(:project) { private_project }
+      let(:user) { create(:user) }
+
+      it 'behaves like not found' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
     end
   end
 

spec/features/projects/releases/user_views_releases_spec.rb

@@ -123,11 +123,11 @@ RSpec.describe 'User views releases', :js do
 
         within('.release-block', match: :first) do
           expect(page).to have_content(release_v3.description)
+          expect(page).to have_content(release_v3.tag)
+          expect(page).to have_content(release_v3.name)
 
           # The following properties (sometimes) include Git info,
           # so they are not rendered for Guest users
-          expect(page).not_to have_content(release_v3.name)
-          expect(page).not_to have_content(release_v3.tag)
           expect(page).not_to have_content(release_v3.commit.short_id)
         end
       end

spec/graphql/types/release_links_type_spec.rb

@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 RSpec.describe GitlabSchema.types['ReleaseLinks'] do
-  it { expect(described_class).to require_graphql_authorizations(:download_code) }
+  it { expect(described_class).to require_graphql_authorizations(:read_release) }
 
   it 'has the expected fields' do
     expected_fields = %w[
@@ -18,4 +18,46 @@ RSpec.describe GitlabSchema.types['ReleaseLinks'] do
 
     expect(described_class).to include_graphql_fields(*expected_fields)
   end
+
+  context 'individual field authorization' do
+    def fetch_authorizations(field_name)
+      described_class.fields.dig(field_name).instance_variable_get(:@authorize)
+    end
+
+    describe 'openedMergeRequestsUrl' do
+      it 'has valid authorization' do
+        expect(fetch_authorizations('openedMergeRequestsUrl')).to include(:download_code)
+      end
+    end
+
+    describe 'mergedMergeRequestsUrl' do
+      it 'has valid authorization' do
+        expect(fetch_authorizations('mergedMergeRequestsUrl')).to include(:download_code)
+      end
+    end
+
+    describe 'closedMergeRequestsUrl' do
+      it 'has valid authorization' do
+        expect(fetch_authorizations('closedMergeRequestsUrl')).to include(:download_code)
+      end
+    end
+
+    describe 'openedIssuesUrl' do
+      it 'has valid authorization' do
+        expect(fetch_authorizations('openedIssuesUrl')).to include(:download_code)
+      end
+    end
+
+    describe 'closedIssuesUrl' do
+      it 'has valid authorization' do
+        expect(fetch_authorizations('closedIssuesUrl')).to include(:download_code)
+      end
+    end
+
+    describe 'editUrl' do
+      it 'has valid authorization' do
+        expect(fetch_authorizations('editUrl')).to include(:update_release)
+      end
+    end
+  end
 end

spec/presenters/release_presenter_spec.rb

@@ -63,12 +63,6 @@ RSpec.describe ReleasePresenter do
     it 'returns its own url' do
       is_expected.to eq(project_release_url(project, release))
     end
-
-    context 'when user is guest' do
-      let(:user) { guest }
-
-      it { is_expected.to be_nil }
-    end
   end
 
   describe '#opened_merge_requests_url' do
@@ -147,13 +141,5 @@ RSpec.describe ReleasePresenter do
     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

@@ -228,6 +228,189 @@ RSpec.describe 'Query.project(fullPath).release(tagName)' do
     end
   end
 
+  shared_examples 'restricted access to release fields' do
+    describe 'scalar fields' do
+      let(:path) { path_prefix }
+
+      let(:release_fields) do
+        %{
+          tagName
+          tagPath
+          description
+          descriptionHtml
+          name
+          createdAt
+          releasedAt
+          upcomingRelease
+        }
+      end
+
+      before do
+        post_query
+      end
+
+      it 'finds all release data' do
+        expect(data).to eq({
+          'tagName' => release.tag,
+          'tagPath' => nil,
+          'description' => release.description,
+          'descriptionHtml' => release.description_html,
+          'name' => release.name,
+          'createdAt' => release.created_at.iso8601,
+          'releasedAt' => release.released_at.iso8601,
+          'upcomingRelease' => false
+        })
+      end
+    end
+
+    describe 'milestones' do
+      let(:path) { path_prefix + %w[milestones nodes] }
+
+      let(:release_fields) do
+        query_graphql_field(:milestones, nil, 'nodes { id title }')
+      end
+
+      it 'finds milestones associated to a release' do
+        post_query
+
+        expected = release.milestones.order_by_dates_and_title.map do |milestone|
+          { 'id' => global_id_of(milestone), 'title' => milestone.title }
+        end
+
+        expect(data).to eq(expected)
+      end
+    end
+
+    describe 'author' do
+      let(:path) { path_prefix + %w[author] }
+
+      let(:release_fields) do
+        query_graphql_field(:author, nil, 'id username')
+      end
+
+      it 'finds the author of the release' do
+        post_query
+
+        expect(data).to eq(
+          'id' => global_id_of(release.author),
+          'username' => release.author.username
+        )
+      end
+    end
+
+    describe 'commit' do
+      let(:path) { path_prefix + %w[commit] }
+
+      let(:release_fields) do
+        query_graphql_field(:commit, nil, 'sha')
+      end
+
+      it 'restricts commit associated with the release' do
+        post_query
+
+        expect(data).to eq(nil)
+      end
+    end
+
+    describe 'assets' do
+      describe 'count' do
+        let(:path) { path_prefix + %w[assets] }
+
+        let(:release_fields) do
+          query_graphql_field(:assets, nil, 'count')
+        end
+
+        it 'returns non source release links count' do
+          post_query
+
+          expect(data).to eq('count' => release.assets_count(except: [:sources]))
+        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, directAssetUrl }'))
+        end
+
+        it 'finds all non source external release links' do
+          post_query
+
+          expected = release.links.map do |link|
+            {
+              'id' => global_id_of(link),
+              'name' => link.name,
+              'url' => link.url,
+              'external' => true,
+              'directAssetUrl' => link.filepath ? Gitlab::Routing.url_helpers.project_release_url(project, release) << "/downloads#{link.filepath}" : link.url
+            }
+          end
+
+          expect(data).to match_array(expected)
+        end
+      end
+
+      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 }'))
+        end
+
+        it 'restricts release sources' do
+          post_query
+
+          expect(data).to match_array([])
+        end
+      end
+    end
+
+    describe 'links' do
+      let(:path) { path_prefix + %w[links] }
+
+      let(:release_fields) do
+        query_graphql_field(:links, nil, %{
+          selfUrl
+          openedMergeRequestsUrl
+          mergedMergeRequestsUrl
+          closedMergeRequestsUrl
+          openedIssuesUrl
+          closedIssuesUrl
+        })
+      end
+
+      it 'finds only selfUrl' do
+        post_query
+
+        expect(data).to eq(
+          'selfUrl' => project_release_url(project, release),
+          'openedMergeRequestsUrl' => nil,
+          'mergedMergeRequestsUrl' => nil,
+          'closedMergeRequestsUrl' => nil,
+          'openedIssuesUrl' => nil,
+          'closedIssuesUrl' => nil
+        )
+      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
+
+      it 'restricts all evidence fields' do
+        post_query
+
+        expect(data).to eq('nodes' => [])
+      end
+    end
+  end
+
   shared_examples 'no access to the release field' do
     describe 'repository-related fields' do
       let(:path) { path_prefix }
@@ -302,7 +485,8 @@ RSpec.describe 'Query.project(fullPath).release(tagName)' do
       context 'when the user has Guest permissions' do
         let(:current_user) { guest }
 
-        it_behaves_like 'no access to the release field'
+        it_behaves_like 'restricted access to release fields'
+        it_behaves_like 'no access to editUrl'
       end
 
       context 'when the user has Reporter permissions' do

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

@@ -129,10 +129,12 @@ RSpec.describe 'Query.project(fullPath).releases()' do
       end
 
       it 'does not return data for fields that expose repository information' do
+        tag_name = release.tag
+        release_name = release.name
         expect(data).to eq(
-          'tagName' => nil,
+          'tagName' => tag_name,
           'tagPath' => nil,
-          'name' => "Release-#{release.id}",
+          'name' => release_name,
           'commit' => nil,
           'assets' => {
             'count' => release.assets_count(except: [:sources]),
@@ -143,7 +145,14 @@ RSpec.describe 'Query.project(fullPath).releases()' do
           'evidences' => {
             'nodes' => []
           },
-          'links' => nil
+          'links' => {
+            'closedIssuesUrl' => nil,
+            'closedMergeRequestsUrl' => nil,
+            'mergedMergeRequestsUrl' => nil,
+            'openedIssuesUrl' => nil,
+            'openedMergeRequestsUrl' => nil,
+            'selfUrl' => project_release_url(project, release)
+          }
         )
       end
     end