Commit 241ba4be authored by Krasimir Angelov's avatar Krasimir Angelov Committed by Lin Jen-Shin

Allow guests users to access project releases

This is step one of resolving
https://gitlab.com/gitlab-org/gitlab-ce/issues/56838.

Here is what changed:
- Revert the security fix from bdee9e84.
- Do not leak repository information (tag name, commit) to guests in API
responses.
- Do not include links to source code in API responses for users that do
not have download_code access.
- Show Releases in sidebar for guests.
- Do not display links to source code under Assets for users that do not
have download_code access.

GET ':id/releases/:tag_name' still do not allow guests to access
releases. This is to prevent guessing tag existence.
parent 9a9aa223
...@@ -86,7 +86,7 @@ export default { ...@@ -86,7 +86,7 @@ export default {
</div> </div>
<div <div
v-if="assets.links.length || assets.sources.length" v-if="assets.links.length || (assets.sources && assets.sources.length)"
class="card-text prepend-top-default" class="card-text prepend-top-default"
> >
<b> <b>
...@@ -103,7 +103,7 @@ export default { ...@@ -103,7 +103,7 @@ export default {
</li> </li>
</ul> </ul>
<div v-if="assets.sources.length" class="dropdown"> <div v-if="assets.sources && assets.sources.length" class="dropdown">
<button <button
type="button" type="button"
class="btn btn-link" class="btn btn-link"
......
...@@ -318,8 +318,9 @@ module ProjectsHelper ...@@ -318,8 +318,9 @@ module ProjectsHelper
def get_project_nav_tabs(project, current_user) def get_project_nav_tabs(project, current_user)
nav_tabs = [:home] nav_tabs = [:home]
if !project.empty_repo? && can?(current_user, :download_code, project) unless project.empty_repo?
nav_tabs << [:files, :commits, :network, :graphs, :forks, :releases] nav_tabs << [:files, :commits, :network, :graphs, :forks] if can?(current_user, :download_code, project)
nav_tabs << :releases if can?(current_user, :read_release, project)
end end
if project.repo_exists? && can?(current_user, :read_merge_request, project) if project.repo_exists? && can?(current_user, :read_merge_request, project)
......
...@@ -31,8 +31,11 @@ class Release < ApplicationRecord ...@@ -31,8 +31,11 @@ class Release < ApplicationRecord
actual_tag.nil? actual_tag.nil?
end end
def assets_count def assets_count(except: [])
links.count + sources.count links_count = links.count
sources_count = except.include?(:sources) ? 0 : sources.count
links_count + sources_count
end end
def sources def sources
......
...@@ -186,6 +186,7 @@ class ProjectPolicy < BasePolicy ...@@ -186,6 +186,7 @@ class ProjectPolicy < BasePolicy
enable :read_cycle_analytics enable :read_cycle_analytics
enable :award_emoji enable :award_emoji
enable :read_pages_content enable :read_pages_content
enable :read_release
end end
# These abilities are not allowed to admins that are not members of the project, # These abilities are not allowed to admins that are not members of the project,
...@@ -212,7 +213,6 @@ class ProjectPolicy < BasePolicy ...@@ -212,7 +213,6 @@ class ProjectPolicy < BasePolicy
enable :read_deployment enable :read_deployment
enable :read_merge_request enable :read_merge_request
enable :read_sentry_issue enable :read_sentry_issue
enable :read_release
enable :read_prometheus enable :read_prometheus
end end
......
---
title: Allow guests users to access project releases
merge_request: 27247
author:
type: changed
...@@ -1156,22 +1156,33 @@ module API ...@@ -1156,22 +1156,33 @@ module API
end end
end end
class Release < TagRelease class Release < Grape::Entity
expose :name expose :name
expose :tag, as: :tag_name, if: lambda { |_, _| can_download_code? }
expose :description
expose :description_html do |entity| expose :description_html do |entity|
MarkupHelper.markdown_field(entity, :description) MarkupHelper.markdown_field(entity, :description)
end end
expose :created_at expose :created_at
expose :author, using: Entities::UserBasic, if: -> (release, _) { release.author.present? } expose :author, using: Entities::UserBasic, if: -> (release, _) { release.author.present? }
expose :commit, using: Entities::Commit expose :commit, using: Entities::Commit, if: lambda { |_, _| can_download_code? }
expose :assets do expose :assets do
expose :assets_count, as: :count expose :assets_count, as: :count do |release, _|
expose :sources, using: Entities::Releases::Source assets_to_exclude = can_download_code? ? [] : [:sources]
release.assets_count(except: assets_to_exclude)
end
expose :sources, using: Entities::Releases::Source, if: lambda { |_, _| can_download_code? }
expose :links, using: Entities::Releases::Link do |release, options| expose :links, using: Entities::Releases::Link do |release, options|
release.links.sorted release.links.sorted
end end
end end
private
def can_download_code?
Ability.allowed?(options[:current_user], :download_code, object.project)
end
end end
class Tag < Grape::Entity class Tag < Grape::Entity
......
...@@ -23,7 +23,7 @@ module API ...@@ -23,7 +23,7 @@ module API
get ':id/releases' do get ':id/releases' do
releases = ::ReleasesFinder.new(user_project, current_user).execute releases = ::ReleasesFinder.new(user_project, current_user).execute
present paginate(releases), with: Entities::Release present paginate(releases), with: Entities::Release, current_user: current_user
end end
desc 'Get a single project release' do desc 'Get a single project release' do
...@@ -34,9 +34,9 @@ module API ...@@ -34,9 +34,9 @@ module API
requires :tag_name, type: String, desc: 'The name of the tag', as: :tag requires :tag_name, type: String, desc: 'The name of the tag', as: :tag
end end
get ':id/releases/:tag_name', requirements: RELEASE_ENDPOINT_REQUIREMETS do get ':id/releases/:tag_name', requirements: RELEASE_ENDPOINT_REQUIREMETS do
authorize_read_release! authorize_download_code!
present release, with: Entities::Release present release, with: Entities::Release, current_user: current_user
end end
desc 'Create a new release' do desc 'Create a new release' do
...@@ -63,7 +63,7 @@ module API ...@@ -63,7 +63,7 @@ module API
.execute .execute
if result[:status] == :success if result[:status] == :success
present result[:release], with: Entities::Release present result[:release], with: Entities::Release, current_user: current_user
else else
render_api_error!(result[:message], result[:http_status]) render_api_error!(result[:message], result[:http_status])
end end
...@@ -86,7 +86,7 @@ module API ...@@ -86,7 +86,7 @@ module API
.execute .execute
if result[:status] == :success if result[:status] == :success
present result[:release], with: Entities::Release present result[:release], with: Entities::Release, current_user: current_user
else else
render_api_error!(result[:message], result[:http_status]) render_api_error!(result[:message], result[:http_status])
end end
...@@ -107,7 +107,7 @@ module API ...@@ -107,7 +107,7 @@ module API
.execute .execute
if result[:status] == :success if result[:status] == :success
present result[:release], with: Entities::Release present result[:release], with: Entities::Release, current_user: current_user
else else
render_api_error!(result[:message], result[:http_status]) render_api_error!(result[:message], result[:http_status])
end end
...@@ -135,6 +135,10 @@ module API ...@@ -135,6 +135,10 @@ module API
authorize! :destroy_release, release authorize! :destroy_release, release
end end
def authorize_download_code!
authorize! :download_code, release
end
def release def release
@release ||= user_project.releases.find_by_tag(params[:tag]) @release ||= user_project.releases.find_by_tag(params[:tag])
end end
......
{ {
"type": "object", "type": "object",
"required" : [ "required": ["name", "tag_name", "commit"],
"tag_name", "properties": {
"description" "name": { "type": "string" },
], "tag_name": { "type": "string" },
"properties" : { "description": { "type": "string" },
"tag_name": { "type": ["string", "null"] }, "description_html": { "type": "string" },
"description": { "type": "string" } "created_at": { "type": "date" },
"commit": {
"oneOf": [{ "type": "null" }, { "$ref": "commit/basic.json" }]
},
"author": {
"oneOf": [{ "type": "null" }, { "$ref": "user/basic.json" }]
},
"assets": {
"required": ["count", "links", "sources"],
"properties": {
"count": { "type": "integer" },
"links": { "$ref": "../../release/links.json" },
"sources": {
"type": "array",
"items": {
"format": "zip",
"url": "string"
}
}
},
"additionalProperties": false
}
}, },
"additionalProperties": false "additionalProperties": false
} }
{
"type": "object",
"required": ["name"],
"properties": {
"name": { "type": "string" },
"description": { "type": "string" },
"description_html": { "type": "string" },
"created_at": { "type": "date" },
"author": {
"oneOf": [{ "type": "null" }, { "$ref": "../user/basic.json" }]
},
"assets": {
"required": ["count", "links"],
"properties": {
"count": { "type": "integer" },
"links": { "$ref": "../../../release/links.json" }
},
"additionalProperties": false
}
},
"additionalProperties": false
}
{
"type": "array",
"items": { "$ref": "release_for_guest.json" }
}
{
"type": "object",
"required" : [
"tag_name",
"description"
],
"properties" : {
"tag_name": { "type": ["string", "null"] },
"description": { "type": "string" }
},
"additionalProperties": false
}
{
"type": "array",
"items": { "$ref": "release.json" }
}
...@@ -14,7 +14,7 @@ ...@@ -14,7 +14,7 @@
"release": { "release": {
"oneOf": [ "oneOf": [
{ "type": "null" }, { "type": "null" },
{ "$ref": "release.json" } { "$ref": "release/tag_release.json" }
] ]
} }
}, },
......
...@@ -49,6 +49,11 @@ RSpec.describe Release do ...@@ -49,6 +49,11 @@ RSpec.describe Release do
it 'counts the link as an asset' do it 'counts the link as an asset' do
is_expected.to eq(1 + Releases::Source::FORMATS.count) is_expected.to eq(1 + Releases::Source::FORMATS.count)
end end
it "excludes sources count when asked" do
assets_count = release.assets_count(except: [:sources])
expect(assets_count).to eq(1)
end
end end
end end
......
...@@ -17,7 +17,7 @@ describe ProjectPolicy do ...@@ -17,7 +17,7 @@ describe ProjectPolicy do
read_project_for_iids read_issue_iid read_label read_project_for_iids read_issue_iid read_label
read_milestone read_project_snippet read_project_member read_note read_milestone read_project_snippet read_project_member read_note
create_project create_issue create_note upload_file create_merge_request_in create_project create_issue create_note upload_file create_merge_request_in
award_emoji award_emoji read_release
] ]
end end
...@@ -26,7 +26,7 @@ describe ProjectPolicy do ...@@ -26,7 +26,7 @@ describe ProjectPolicy do
download_code fork_project create_project_snippet update_issue download_code fork_project create_project_snippet update_issue
admin_issue admin_label admin_list read_commit_status read_build admin_issue admin_label admin_list read_commit_status read_build
read_container_image read_pipeline read_environment read_deployment read_container_image read_pipeline read_environment read_deployment
read_merge_request download_wiki_code read_sentry_issue read_release read_merge_request download_wiki_code read_sentry_issue
] ]
end end
......
...@@ -52,7 +52,7 @@ describe API::Releases do ...@@ -52,7 +52,7 @@ describe API::Releases do
it 'matches response schema' do it 'matches response schema' do
get api("/projects/#{project.id}/releases", maintainer) get api("/projects/#{project.id}/releases", maintainer)
expect(response).to match_response_schema('releases') expect(response).to match_response_schema('public_api/v4/releases')
end end
end end
...@@ -69,10 +69,25 @@ describe API::Releases do ...@@ -69,10 +69,25 @@ describe API::Releases do
end end
context 'when user is a guest' do context 'when user is a guest' do
it 'responds 403 Forbidden' do let!(:release) do
create(:release,
project: project,
tag: 'v0.1',
author: maintainer,
created_at: 2.days.ago)
end
it 'responds 200 OK' do
get api("/projects/#{project.id}/releases", guest) get api("/projects/#{project.id}/releases", guest)
expect(response).to have_gitlab_http_status(:forbidden) expect(response).to have_gitlab_http_status(:ok)
end
it "does not expose tag, commit and source code" do
get api("/projects/#{project.id}/releases", guest)
expect(response).to match_response_schema('public_api/v4/release/releases_for_guest')
expect(json_response[0]['assets']['count']).to eq(release.links.count)
end end
context 'when project is public' do context 'when project is public' do
...@@ -83,6 +98,13 @@ describe API::Releases do ...@@ -83,6 +98,13 @@ describe API::Releases do
expect(response).to have_gitlab_http_status(:ok) expect(response).to have_gitlab_http_status(:ok)
end end
it "exposes tag, commit and source code" do
get api("/projects/#{project.id}/releases", guest)
expect(response).to match_response_schema('public_api/v4/releases')
expect(json_response[0]['assets']['count']).to eq(release.links.count + release.sources.count)
end
end end
end end
...@@ -135,7 +157,7 @@ describe API::Releases do ...@@ -135,7 +157,7 @@ describe API::Releases do
it 'matches response schema' do it 'matches response schema' do
get api("/projects/#{project.id}/releases/v0.1", maintainer) get api("/projects/#{project.id}/releases/v0.1", maintainer)
expect(response).to match_response_schema('release') expect(response).to match_response_schema('public_api/v4/release')
end end
it 'contains source information as assets' do it 'contains source information as assets' do
...@@ -225,6 +247,17 @@ describe API::Releases do ...@@ -225,6 +247,17 @@ describe API::Releases do
expect(response).to have_gitlab_http_status(:ok) expect(response).to have_gitlab_http_status(:ok)
end end
it "exposes tag and commit" do
create(:release,
project: project,
tag: 'v0.1',
author: maintainer,
created_at: 2.days.ago)
get api("/projects/#{project.id}/releases/v0.1", guest)
expect(response).to match_response_schema('public_api/v4/release')
end
end end
end end
end end
...@@ -306,7 +339,7 @@ describe API::Releases do ...@@ -306,7 +339,7 @@ describe API::Releases do
it 'matches response schema' do it 'matches response schema' do
post api("/projects/#{project.id}/releases", maintainer), params: params post api("/projects/#{project.id}/releases", maintainer), params: params
expect(response).to match_response_schema('release') expect(response).to match_response_schema('public_api/v4/release')
end end
it 'does not create a new tag' do it 'does not create a new tag' do
...@@ -378,7 +411,7 @@ describe API::Releases do ...@@ -378,7 +411,7 @@ describe API::Releases do
it 'matches response schema' do it 'matches response schema' do
post api("/projects/#{project.id}/releases", maintainer), params: params post api("/projects/#{project.id}/releases", maintainer), params: params
expect(response).to match_response_schema('release') expect(response).to match_response_schema('public_api/v4/release')
end end
end end
...@@ -532,7 +565,7 @@ describe API::Releases do ...@@ -532,7 +565,7 @@ describe API::Releases do
it 'matches response schema' do it 'matches response schema' do
put api("/projects/#{project.id}/releases/v0.1", maintainer), params: params put api("/projects/#{project.id}/releases/v0.1", maintainer), params: params
expect(response).to match_response_schema('release') expect(response).to match_response_schema('public_api/v4/release')
end end
context 'when user tries to update sha' do context 'when user tries to update sha' do
...@@ -624,7 +657,7 @@ describe API::Releases do ...@@ -624,7 +657,7 @@ describe API::Releases do
it 'matches response schema' do it 'matches response schema' do
delete api("/projects/#{project.id}/releases/v0.1", maintainer) delete api("/projects/#{project.id}/releases/v0.1", maintainer)
expect(response).to match_response_schema('release') expect(response).to match_response_schema('public_api/v4/release')
end end
context 'when there are no corresponding releases' do context 'when there are no corresponding releases' do
......
...@@ -378,7 +378,7 @@ describe API::Tags do ...@@ -378,7 +378,7 @@ describe API::Tags do
post api(route, user), params: { description: description } post api(route, user), params: { description: description }
expect(response).to have_gitlab_http_status(201) expect(response).to have_gitlab_http_status(201)
expect(response).to match_response_schema('public_api/v4/release') expect(response).to match_response_schema('public_api/v4/release/tag_release')
expect(json_response['tag_name']).to eq(tag_name) expect(json_response['tag_name']).to eq(tag_name)
expect(json_response['description']).to eq(description) expect(json_response['description']).to eq(description)
end end
......
...@@ -24,8 +24,7 @@ RSpec.shared_context 'ProjectPolicy context' do ...@@ -24,8 +24,7 @@ RSpec.shared_context 'ProjectPolicy context' do
download_code fork_project create_project_snippet update_issue download_code fork_project create_project_snippet update_issue
admin_issue admin_label admin_list read_commit_status read_build admin_issue admin_label admin_list read_commit_status read_build
read_container_image read_pipeline read_environment read_deployment read_container_image read_pipeline read_environment read_deployment
read_merge_request download_wiki_code read_sentry_issue read_release read_merge_request download_wiki_code read_sentry_issue read_prometheus
read_prometheus
] ]
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