Fix Conan package presenter

Conan::PackagePresenter now checks package
files to ensure the correct conan_package_reference
files are being returned in requests fixing a bug
where the first file by name was returned no matter
what request was made.

changelogs/unreleased/208830-download-urls-conan-reference.yml

+---
+title: Fix Conan package download_urls and snapshot to return files based on requested
+  conan_package_reference
+merge_request: 27250
+author:
+type: fixed

ee/app/presenters/packages/conan/package_presenter.rb

@@ -6,10 +6,13 @@ module Packages
       include API::Helpers::RelatedResourcesHelpers
       include Gitlab::Utils::StrongMemoize
 
-      def initialize(recipe, user, project)
+      attr_reader :params
+
+      def initialize(recipe, user, project, params = {})
         @recipe = recipe
         @user = user
         @project = project
+        @params = params
       end
 
       def recipe_urls
@@ -26,13 +29,17 @@ module Packages
 
       def package_urls
         map_package_files do |package_file|
-          build_package_file_url(package_file) if package_file.conan_file_metadatum.package_file?
+          next unless package_file.conan_file_metadatum.package_file? && matching_reference?(package_file)
+
+          build_package_file_url(package_file)
         end
       end
 
       def package_snapshot
         map_package_files do |package_file|
-          package_file.file_md5 if package_file.conan_file_metadatum.package_file?
+          next unless package_file.conan_file_metadatum.package_file? && matching_reference?(package_file)
+
+          package_file.file_md5
         end
       end
 
@@ -89,6 +96,14 @@ module Packages
           @project.packages.with_name(name).with_version(version).order_created.last
         end
       end
+
+      def matching_reference?(package_file)
+        package_file.conan_file_metadatum.conan_package_reference == conan_package_reference
+      end
+
+      def conan_package_reference
+        params[:conan_package_reference]
+      end
     end
   end
 end

ee/lib/api/conan_packages.rb

@@ -97,11 +97,19 @@ module API
         desc 'Package Snapshot' do
           detail 'This feature was introduced in GitLab 12.5'
         end
+        params do
+          requires :conan_package_reference, type: String, desc: 'Conan package ID'
+        end
         route_setting :authentication, job_token_allowed: true
         get 'packages/:conan_package_reference' do
           authorize!(:read_package, project)
 
-          presenter = ::Packages::Conan::PackagePresenter.new(recipe, current_user, project)
+          presenter = ::Packages::Conan::PackagePresenter.new(
+            recipe,
+            current_user,
+            project,
+            conan_package_reference: params[:conan_package_reference]
+          )
 
           present presenter, with: EE::API::Entities::ConanPackage::ConanPackageSnapshot
         end
@@ -126,6 +134,9 @@ module API
         desc 'Package Digest' do
           detail 'This feature was introduced in GitLab 12.5'
         end
+        params do
+          requires :conan_package_reference, type: String, desc: 'Conan package ID'
+        end
         route_setting :authentication, job_token_allowed: true
         get 'packages/:conan_package_reference/digest' do
           present_package_download_urls
@@ -148,6 +159,9 @@ module API
         desc 'Package Download Urls' do
           detail 'This feature was introduced in GitLab 12.5'
         end
+        params do
+          requires :conan_package_reference, type: String, desc: 'Conan package ID'
+        end
         route_setting :authentication, job_token_allowed: true
         get 'packages/:conan_package_reference/download_urls' do
           present_package_download_urls

ee/lib/api/helpers/packages/conan/api_helpers.rb

@@ -8,7 +8,12 @@ module API
           def present_download_urls(entity)
             authorize!(:read_package, project)
 
-            presenter = ::Packages::Conan::PackagePresenter.new(recipe, current_user, project)
+            presenter = ::Packages::Conan::PackagePresenter.new(
+              recipe,
+              current_user,
+              project,
+              conan_package_reference: params[:conan_package_reference]
+            )
 
             render_api_error!("No recipe manifest found", 404) if yield(presenter).empty?
 

ee/spec/presenters/packages/conan/package_presenter_spec.rb

@@ -3,8 +3,9 @@
 require 'spec_helper'
 
 describe ::Packages::Conan::PackagePresenter do
-  let(:user) { create(:user) }
-  let(:project) { create(:project) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:project) { create(:project) }
+  let_it_be(:conan_package_reference) { '123456789'}
 
   describe '#recipe_urls' do
     subject { described_class.new(recipe, user, project).recipe_urls }
@@ -55,7 +56,13 @@ describe ::Packages::Conan::PackagePresenter do
   end
 
   describe '#package_urls' do
-    subject { described_class.new(recipe, user, project).package_urls }
+    let(:reference) { conan_package_reference }
+
+    subject do
+      described_class.new(
+        recipe, user, project, conan_package_reference: reference
+      ).package_urls
+    end
 
     context 'no existing package' do
       let(:recipe) { "my-pkg/v1.0.0/#{project.full_path}/stable" }
@@ -69,18 +76,62 @@ describe ::Packages::Conan::PackagePresenter do
 
       let(:expected_result) do
         {
-          "conaninfo.txt" => "#{Settings.build_base_gitlab_url}/api/v4/packages/conan/v1/files/#{package.conan_recipe_path}/0/package/123456789/0/conaninfo.txt",
-          "conanmanifest.txt" => "#{Settings.build_base_gitlab_url}/api/v4/packages/conan/v1/files/#{package.conan_recipe_path}/0/package/123456789/0/conanmanifest.txt",
-          "conan_package.tgz" => "#{Settings.build_base_gitlab_url}/api/v4/packages/conan/v1/files/#{package.conan_recipe_path}/0/package/123456789/0/conan_package.tgz"
+          "conaninfo.txt" => "#{Settings.build_base_gitlab_url}/api/v4/packages/conan/v1/files/#{package.conan_recipe_path}/0/package/#{conan_package_reference}/0/conaninfo.txt",
+          "conanmanifest.txt" => "#{Settings.build_base_gitlab_url}/api/v4/packages/conan/v1/files/#{package.conan_recipe_path}/0/package/#{conan_package_reference}/0/conanmanifest.txt",
+          "conan_package.tgz" => "#{Settings.build_base_gitlab_url}/api/v4/packages/conan/v1/files/#{package.conan_recipe_path}/0/package/#{conan_package_reference}/0/conan_package.tgz"
+        }
+      end
+
+      it { is_expected.to eq(expected_result) }
+
+      context 'multiple packages with different references' do
+        let(:info_file) { create(:conan_package_file, :conan_package_info, package: package) }
+        let(:manifest_file) { create(:conan_package_file, :conan_package_manifest, package: package) }
+        let(:package_file) { create(:conan_package_file, :conan_package, package: package) }
+        let(:alternative_reference) { 'abcdefghi' }
+
+        before do
+          [info_file, manifest_file, package_file].each do |file|
+            file.conan_file_metadatum.conan_package_reference = alternative_reference
+            file.save
+          end
+        end
+
+        it { is_expected.to eq(expected_result) }
+
+        context 'requesting the alternative reference' do
+          let(:reference) { alternative_reference }
+
+          let(:expected_result) do
+            {
+              "conaninfo.txt" => "#{Settings.build_base_gitlab_url}/api/v4/packages/conan/v1/files/#{package.conan_recipe_path}/0/package/#{alternative_reference}/0/conaninfo.txt",
+              "conanmanifest.txt" => "#{Settings.build_base_gitlab_url}/api/v4/packages/conan/v1/files/#{package.conan_recipe_path}/0/package/#{alternative_reference}/0/conanmanifest.txt",
+              "conan_package.tgz" => "#{Settings.build_base_gitlab_url}/api/v4/packages/conan/v1/files/#{package.conan_recipe_path}/0/package/#{alternative_reference}/0/conan_package.tgz"
             }
           end
 
           it { is_expected.to eq(expected_result) }
         end
+
+        it 'returns empty if the reference does not exist' do
+          result = described_class.new(
+            recipe, user, project, conan_package_reference: 'doesnotexist'
+          ).package_urls
+
+          expect(result).to eq({})
+        end
+      end
+    end
   end
 
   describe '#package_snapshot' do
-    subject { described_class.new(recipe, user, project).package_snapshot }
+    let(:reference) { conan_package_reference }
+
+    subject do
+      described_class.new(
+        recipe, user, project, conan_package_reference: reference
+      ).package_snapshot
+    end
 
     context 'no existing package' do
       let(:recipe) { "my-pkg/v1.0.0/#{project.full_path}/stable" }
@@ -101,6 +152,12 @@ describe ::Packages::Conan::PackagePresenter do
       end
 
       it { is_expected.to eq(expected_result) }
+
+      context 'when requested with invalid reference' do
+        let(:reference) { 'invalid' }
+
+        it { is_expected.to eq({}) }
+      end
     end
   end
 end

ee/spec/requests/api/conan_packages_spec.rb

@@ -240,7 +240,8 @@ describe API::ConanPackages do
           .with(
             'aa/bb@%{project}/ccc' % { project: ::Packages::ConanMetadatum.package_username_from(full_path: project.full_path) },
             user,
-            project
+            project,
+            any_args
           ).and_return(presenter)
         allow(presenter).to receive(:recipe_snapshot) { {} }
         allow(presenter).to receive(:package_snapshot) { {} }
@@ -296,7 +297,7 @@ describe API::ConanPackages do
 
     before do
       allow(::Packages::Conan::PackagePresenter).to receive(:new)
-        .with(package.conan_recipe, user, package.project)
+        .with(package.conan_recipe, user, package.project, any_args)
         .and_return(presenter)
     end