From 241ba4be7989547b3bc3f9a1a20b8dee7a4e9a0c Mon Sep 17 00:00:00 2001
From: Krasimir Angelov <kangelov@gitlab.com>
Date: Fri, 3 May 2019 13:29:20 +0000
Subject: 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 bdee9e8412d.
- 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.
---
 .../releases/components/release_block.vue          |  4 +-
 app/helpers/projects_helper.rb                     |  5 ++-
 app/models/release.rb                              |  7 +++-
 app/policies/project_policy.rb                     |  2 +-
 .../56838-allow-guest-access-to-releases.yml       |  5 +++
 lib/api/entities.rb                                | 19 +++++++--
 lib/api/releases.rb                                | 16 ++++---
 .../api/schemas/public_api/v4/release.json         | 35 ++++++++++++----
 .../public_api/v4/release/release_for_guest.json   | 22 ++++++++++
 .../public_api/v4/release/releases_for_guest.json  |  4 ++
 .../schemas/public_api/v4/release/tag_release.json | 12 ++++++
 .../api/schemas/public_api/v4/releases.json        |  4 ++
 spec/fixtures/api/schemas/public_api/v4/tag.json   |  2 +-
 spec/models/release_spec.rb                        |  5 +++
 spec/policies/project_policy_spec.rb               |  4 +-
 spec/requests/api/releases_spec.rb                 | 49 ++++++++++++++++++----
 spec/requests/api/tags_spec.rb                     |  2 +-
 .../policies/project_policy_shared_context.rb      |  3 +-
 18 files changed, 162 insertions(+), 38 deletions(-)
 create mode 100644 changelogs/unreleased/56838-allow-guest-access-to-releases.yml
 create mode 100644 spec/fixtures/api/schemas/public_api/v4/release/release_for_guest.json
 create mode 100644 spec/fixtures/api/schemas/public_api/v4/release/releases_for_guest.json
 create mode 100644 spec/fixtures/api/schemas/public_api/v4/release/tag_release.json
 create mode 100644 spec/fixtures/api/schemas/public_api/v4/releases.json

diff --git a/app/assets/javascripts/releases/components/release_block.vue b/app/assets/javascripts/releases/components/release_block.vue
index 7ed1b407ddd..0958b9fa926 100644
--- a/app/assets/javascripts/releases/components/release_block.vue
+++ b/app/assets/javascripts/releases/components/release_block.vue
@@ -86,7 +86,7 @@ export default {
       </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"
       >
         <b>
@@ -103,7 +103,7 @@ export default {
           </li>
         </ul>
 
-        <div v-if="assets.sources.length" class="dropdown">
+        <div v-if="assets.sources && assets.sources.length" class="dropdown">
           <button
             type="button"
             class="btn btn-link"
diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb
index 2ac90eb8d9f..8977ccaa9d8 100644
--- a/app/helpers/projects_helper.rb
+++ b/app/helpers/projects_helper.rb
@@ -318,8 +318,9 @@ module ProjectsHelper
   def get_project_nav_tabs(project, current_user)
     nav_tabs = [:home]
 
-    if !project.empty_repo? && can?(current_user, :download_code, project)
-      nav_tabs << [:files, :commits, :network, :graphs, :forks, :releases]
+    unless project.empty_repo?
+      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
 
     if project.repo_exists? && can?(current_user, :read_merge_request, project)
diff --git a/app/models/release.rb b/app/models/release.rb
index 0f9e94373c7..7bbeb3c9976 100644
--- a/app/models/release.rb
+++ b/app/models/release.rb
@@ -31,8 +31,11 @@ class Release < ApplicationRecord
     actual_tag.nil?
   end
 
-  def assets_count
-    links.count + sources.count
+  def assets_count(except: [])
+    links_count = links.count
+    sources_count = except.include?(:sources) ? 0 : sources.count
+
+    links_count + sources_count
   end
 
   def sources
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index ba38af9c529..76544249688 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -186,6 +186,7 @@ class ProjectPolicy < BasePolicy
     enable :read_cycle_analytics
     enable :award_emoji
     enable :read_pages_content
+    enable :read_release
   end
 
   # These abilities are not allowed to admins that are not members of the project,
@@ -212,7 +213,6 @@ class ProjectPolicy < BasePolicy
     enable :read_deployment
     enable :read_merge_request
     enable :read_sentry_issue
-    enable :read_release
     enable :read_prometheus
   end
 
diff --git a/changelogs/unreleased/56838-allow-guest-access-to-releases.yml b/changelogs/unreleased/56838-allow-guest-access-to-releases.yml
new file mode 100644
index 00000000000..701a015b9ac
--- /dev/null
+++ b/changelogs/unreleased/56838-allow-guest-access-to-releases.yml
@@ -0,0 +1,5 @@
+---
+title: Allow guests users to access project releases
+merge_request: 27247
+author:
+type: changed
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index ee8480122c4..a228614f684 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -1156,22 +1156,33 @@ module API
       end
     end
 
-    class Release < TagRelease
+    class Release < Grape::Entity
       expose :name
+      expose :tag, as: :tag_name, if: lambda { |_, _| can_download_code? }
+      expose :description
       expose :description_html do |entity|
         MarkupHelper.markdown_field(entity, :description)
       end
       expose :created_at
       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_count, as: :count
-        expose :sources, using: Entities::Releases::Source
+        expose :assets_count, as: :count do |release, _|
+          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|
           release.links.sorted
         end
       end
+
+      private
+
+      def can_download_code?
+        Ability.allowed?(options[:current_user], :download_code, object.project)
+      end
     end
 
     class Tag < Grape::Entity
diff --git a/lib/api/releases.rb b/lib/api/releases.rb
index cb85028f22c..6b17f4317db 100644
--- a/lib/api/releases.rb
+++ b/lib/api/releases.rb
@@ -23,7 +23,7 @@ module API
       get ':id/releases' do
         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
 
       desc 'Get a single project release' do
@@ -34,9 +34,9 @@ module API
         requires :tag_name, type: String, desc: 'The name of the tag', as: :tag
       end
       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
 
       desc 'Create a new release' do
@@ -63,7 +63,7 @@ module API
           .execute
 
         if result[:status] == :success
-          present result[:release], with: Entities::Release
+          present result[:release], with: Entities::Release, current_user: current_user
         else
           render_api_error!(result[:message], result[:http_status])
         end
@@ -86,7 +86,7 @@ module API
           .execute
 
         if result[:status] == :success
-          present result[:release], with: Entities::Release
+          present result[:release], with: Entities::Release, current_user: current_user
         else
           render_api_error!(result[:message], result[:http_status])
         end
@@ -107,7 +107,7 @@ module API
           .execute
 
         if result[:status] == :success
-          present result[:release], with: Entities::Release
+          present result[:release], with: Entities::Release, current_user: current_user
         else
           render_api_error!(result[:message], result[:http_status])
         end
@@ -135,6 +135,10 @@ module API
         authorize! :destroy_release, release
       end
 
+      def authorize_download_code!
+        authorize! :download_code, release
+      end
+
       def release
         @release ||= user_project.releases.find_by_tag(params[:tag])
       end
diff --git a/spec/fixtures/api/schemas/public_api/v4/release.json b/spec/fixtures/api/schemas/public_api/v4/release.json
index 6612c2a9911..6ea0781c1ed 100644
--- a/spec/fixtures/api/schemas/public_api/v4/release.json
+++ b/spec/fixtures/api/schemas/public_api/v4/release.json
@@ -1,12 +1,33 @@
 {
   "type": "object",
-  "required" : [
-    "tag_name",
-    "description"
-  ],
-  "properties" : {
-    "tag_name": { "type": ["string", "null"] },
-    "description": { "type": "string" }
+  "required": ["name", "tag_name", "commit"],
+  "properties": {
+    "name": { "type": "string" },
+    "tag_name": { "type": "string" },
+    "description": { "type": "string" },
+    "description_html": { "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
 }
diff --git a/spec/fixtures/api/schemas/public_api/v4/release/release_for_guest.json b/spec/fixtures/api/schemas/public_api/v4/release/release_for_guest.json
new file mode 100644
index 00000000000..e78398ad1d5
--- /dev/null
+++ b/spec/fixtures/api/schemas/public_api/v4/release/release_for_guest.json
@@ -0,0 +1,22 @@
+{
+  "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
+}
diff --git a/spec/fixtures/api/schemas/public_api/v4/release/releases_for_guest.json b/spec/fixtures/api/schemas/public_api/v4/release/releases_for_guest.json
new file mode 100644
index 00000000000..c13966b28e9
--- /dev/null
+++ b/spec/fixtures/api/schemas/public_api/v4/release/releases_for_guest.json
@@ -0,0 +1,4 @@
+{
+  "type": "array",
+  "items": { "$ref": "release_for_guest.json" }
+}
diff --git a/spec/fixtures/api/schemas/public_api/v4/release/tag_release.json b/spec/fixtures/api/schemas/public_api/v4/release/tag_release.json
new file mode 100644
index 00000000000..6612c2a9911
--- /dev/null
+++ b/spec/fixtures/api/schemas/public_api/v4/release/tag_release.json
@@ -0,0 +1,12 @@
+{
+  "type": "object",
+  "required" : [
+    "tag_name",
+    "description"
+  ],
+  "properties" : {
+    "tag_name": { "type": ["string", "null"] },
+    "description": { "type": "string" }
+  },
+  "additionalProperties": false
+}
diff --git a/spec/fixtures/api/schemas/public_api/v4/releases.json b/spec/fixtures/api/schemas/public_api/v4/releases.json
new file mode 100644
index 00000000000..e26215707fe
--- /dev/null
+++ b/spec/fixtures/api/schemas/public_api/v4/releases.json
@@ -0,0 +1,4 @@
+{
+  "type": "array",
+  "items": { "$ref": "release.json" }
+}
diff --git a/spec/fixtures/api/schemas/public_api/v4/tag.json b/spec/fixtures/api/schemas/public_api/v4/tag.json
index 10d4edb7ffb..5713ea1f526 100644
--- a/spec/fixtures/api/schemas/public_api/v4/tag.json
+++ b/spec/fixtures/api/schemas/public_api/v4/tag.json
@@ -14,7 +14,7 @@
     "release": {
       "oneOf": [
         { "type": "null" },
-        { "$ref": "release.json" }
+        { "$ref": "release/tag_release.json" }
       ]
     }
   },
diff --git a/spec/models/release_spec.rb b/spec/models/release_spec.rb
index 0b19a4f8efc..7c106ce6b85 100644
--- a/spec/models/release_spec.rb
+++ b/spec/models/release_spec.rb
@@ -49,6 +49,11 @@ RSpec.describe Release do
       it 'counts the link as an asset' do
         is_expected.to eq(1 + Releases::Source::FORMATS.count)
       end
+
+      it "excludes sources count when asked" do
+        assets_count = release.assets_count(except: [:sources])
+        expect(assets_count).to eq(1)
+      end
     end
   end
 
diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb
index 42f8bf3137b..8075fcade5f 100644
--- a/spec/policies/project_policy_spec.rb
+++ b/spec/policies/project_policy_spec.rb
@@ -17,7 +17,7 @@ describe ProjectPolicy do
       read_project_for_iids read_issue_iid read_label
       read_milestone read_project_snippet read_project_member read_note
       create_project create_issue create_note upload_file create_merge_request_in
-      award_emoji
+      award_emoji read_release
     ]
   end
 
@@ -26,7 +26,7 @@ describe ProjectPolicy do
       download_code fork_project create_project_snippet update_issue
       admin_issue admin_label admin_list read_commit_status read_build
       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
 
diff --git a/spec/requests/api/releases_spec.rb b/spec/requests/api/releases_spec.rb
index 71ec091c42c..8603fa2a73d 100644
--- a/spec/requests/api/releases_spec.rb
+++ b/spec/requests/api/releases_spec.rb
@@ -52,7 +52,7 @@ describe API::Releases do
       it 'matches response schema' do
         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
 
@@ -69,10 +69,25 @@ describe API::Releases do
     end
 
     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)
 
-        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
 
       context 'when project is public' do
@@ -83,6 +98,13 @@ describe API::Releases do
 
           expect(response).to have_gitlab_http_status(:ok)
         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
 
@@ -135,7 +157,7 @@ describe API::Releases do
       it 'matches response schema' do
         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
 
       it 'contains source information as assets' do
@@ -225,6 +247,17 @@ describe API::Releases do
 
             expect(response).to have_gitlab_http_status(:ok)
           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
@@ -306,7 +339,7 @@ describe API::Releases do
     it 'matches response schema' do
       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
 
     it 'does not create a new tag' do
@@ -378,7 +411,7 @@ describe API::Releases do
           it 'matches response schema' do
             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
 
@@ -532,7 +565,7 @@ describe API::Releases do
     it 'matches response schema' do
       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
 
     context 'when user tries to update sha' do
@@ -624,7 +657,7 @@ describe API::Releases do
     it 'matches response schema' do
       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
 
     context 'when there are no corresponding releases' do
diff --git a/spec/requests/api/tags_spec.rb b/spec/requests/api/tags_spec.rb
index fffe878ddbd..d898319e709 100644
--- a/spec/requests/api/tags_spec.rb
+++ b/spec/requests/api/tags_spec.rb
@@ -378,7 +378,7 @@ describe API::Tags do
         post api(route, user), params: { description: description }
 
         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['description']).to eq(description)
       end
diff --git a/spec/support/shared_context/policies/project_policy_shared_context.rb b/spec/support/shared_context/policies/project_policy_shared_context.rb
index ee5cfcd850d..54d9f5b15f2 100644
--- a/spec/support/shared_context/policies/project_policy_shared_context.rb
+++ b/spec/support/shared_context/policies/project_policy_shared_context.rb
@@ -24,8 +24,7 @@ RSpec.shared_context 'ProjectPolicy context' do
       download_code fork_project create_project_snippet update_issue
       admin_issue admin_label admin_list read_commit_status read_build
       read_container_image read_pipeline read_environment read_deployment
-      read_merge_request download_wiki_code read_sentry_issue read_release
-      read_prometheus
+      read_merge_request download_wiki_code read_sentry_issue read_prometheus
     ]
   end
 
-- 
cgit v1.2.1