Commit 829c5666 authored by Bala Kumar's avatar Bala Kumar Committed by Shinya Maeda

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

parent c5e45556
......@@ -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
......
......@@ -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,
......
......@@ -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
......@@ -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
......
......@@ -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)
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......@@ -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
......@@ -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
......
......@@ -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
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment