Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build trusted publisher filter #5237

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion app/models/concerns/rubygem_searchable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,8 @@ module RubygemSearchable
suggest: { type: "completion", contexts: { name: "yanked", type: "category" } },
yanked: { type: "boolean" },
downloads: { type: "long" },
updated: { type: "date" }
updated: { type: "date" },
trusted_publisher: { type: "boolean" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this mapping change need full re-indexing?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I would not think so, since there is no change to the database source schema, but I am not sure. It is my first time committing to this repo and I do not have full context, so I will wait for others to weigh in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any new mappings do require a reindex, which is easy to do but needs some coordination when deployed.

}
}
scope :search_import, -> { includes(:linkset, :gem_download, :most_recent_version, :versions, :latest_version) }
Expand All @@ -43,6 +44,7 @@ def search_data # rubocop:disable Metrics/MethodLength
version_downloads: latest_version&.downloads_count,
platform: latest_version&.platform,
authors: latest_version&.authors,
trusted_publisher: latest_version&.pushed_by_trusted_publisher?,
info: latest_version&.info,
licenses: latest_version&.licenses,
metadata: latest_version&.metadata,
Expand Down
4 changes: 4 additions & 0 deletions app/models/version.rb
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,10 @@ def gem_file_name
"#{full_name}.gem"
end

def pushed_by_trusted_publisher?
pusher_api_key&.trusted_publisher? ? true : false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't trusted_publisher? already return true/false? Is there any reason for explicit ? true: false?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The safe navigator means a nil value can be returned if the version has no pusher_api_key.

Copy link
Member

@simi simi Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would pusher_api_key? && pusher_api_key.trusted_publisher? work also?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but pusher_api_key.present? && pusher_api_key.trusted_publisher? would work. It is a matter of preference here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe pusher_api_key&.trusted_publisher? || false. Yes, just a nitpick indeed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I the only one that prefers !! to force boolean?

end

private

def update_prerelease
Expand Down
3 changes: 3 additions & 0 deletions app/views/searches/advanced.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,8 @@

<dt><%= label_tag :updated, t(".updated"), class: "form__label" %></dt>
<dd><%= text_field_tag :updated, "", placeholder: ">#{1.week.ago.strftime('%F')}", class: "form__input", pattern: "\\s*(<=?|>=?)?[\\d-]+\\s*", data: { search_target: "attribute", action: "input->search#input keydown.enter->search#submit" } %></dd>

<dt><%= label_tag :trusted_publisher, t(".trusted_publisher"), class: "form__label" %></dt>
<dd><%= text_field_tag :trusted_publisher, "", placeholder: "true", class: "form__input", pattern: "true|false", data: { search_target: "attribute", action: "input->search#input keydown.enter->search#submit" } %></dd>
</dl>
</div>
1 change: 1 addition & 0 deletions config/locales/de.yml
Original file line number Diff line number Diff line change
Expand Up @@ -844,6 +844,7 @@ de:
summary:
description:
downloads:
trusted_publisher:
updated:
yanked:
show:
Expand Down
1 change: 1 addition & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -757,6 +757,7 @@ en:
summary: Summary
description: Description
downloads: Downloads
trusted_publisher: Trusted Publisher
updated: Updated
yanked: Yanked
show:
Expand Down
1 change: 1 addition & 0 deletions config/locales/es.yml
Original file line number Diff line number Diff line change
Expand Up @@ -870,6 +870,7 @@ es:
summary: Resumen
description: Descripción
downloads: Descargas
trusted_publisher:
updated: Actualizada
yanked: Borrada
show:
Expand Down
1 change: 1 addition & 0 deletions config/locales/fr.yml
Original file line number Diff line number Diff line change
Expand Up @@ -794,6 +794,7 @@ fr:
summary: Sommaire
description: Description
downloads: Téléchargements
trusted_publisher:
updated: Mis à jour
yanked:
show:
Expand Down
1 change: 1 addition & 0 deletions config/locales/ja.yml
Original file line number Diff line number Diff line change
Expand Up @@ -755,6 +755,7 @@ ja:
summary: 概要
description: 説明
downloads: ダウンロード数
trusted_publisher:
updated: 更新日
yanked: ヤンク済み
show:
Expand Down
1 change: 1 addition & 0 deletions config/locales/nl.yml
Original file line number Diff line number Diff line change
Expand Up @@ -748,6 +748,7 @@ nl:
summary:
description:
downloads:
trusted_publisher:
updated:
yanked:
show:
Expand Down
1 change: 1 addition & 0 deletions config/locales/pt-BR.yml
Original file line number Diff line number Diff line change
Expand Up @@ -772,6 +772,7 @@ pt-BR:
summary:
description:
downloads:
trusted_publisher:
updated:
yanked:
show:
Expand Down
1 change: 1 addition & 0 deletions config/locales/zh-CN.yml
Original file line number Diff line number Diff line change
Expand Up @@ -760,6 +760,7 @@ zh-CN:
summary: 概要
description: 描述
downloads: 下载数
trusted_publisher:
updated: 更新
yanked: 撤回
show:
Expand Down
1 change: 1 addition & 0 deletions config/locales/zh-TW.yml
Original file line number Diff line number Diff line change
Expand Up @@ -750,6 +750,7 @@ zh-TW:
summary: 摘要
description: 描述
downloads: 下載數
trusted_publisher:
updated: 更新於
yanked: 移除於
show:
Expand Down
8 changes: 8 additions & 0 deletions test/factories/version.rb
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,14 @@
metadata { { "rubygems_mfa_required" => "true" } }
end

trait :has_trusted_publisher do
pusher_api_key { association(:api_key, :trusted_publisher, key: SecureRandom.hex(24)) }
end

trait :has_untrusted_publisher do
pusher_api_key { association(:api_key, key: SecureRandom.hex(24)) }
end

after(:create) do |version|
if version.info_checksum.blank?
checksum = GemInfo.new(version.rubygem.name).info_checksum
Expand Down
16 changes: 16 additions & 0 deletions test/integration/search_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -115,4 +115,20 @@ class SearchTest < SystemTest
refute page.has_content? "Search reverse dependencies Gems…"
assert page.has_content? "This gem has no reverse dependencies"
end

test "filtering by trusted publishers" do
rubygem1 = create(:rubygem, name: "LDAP", number: "1.0.0")
rubygem2 = create(:rubygem, name: "LDAP-Shady-Gem", number: "1.0.0")
create(:version, :has_trusted_publisher, rubygem: rubygem1, indexed: false)
create(:version, :has_untrusted_publisher, rubygem: rubygem2, indexed: false)
import_and_refresh

visit search_path

fill_in "query", with: "LDAP&trusted_publisher=true"
click_button "search_submit"

assert page.has_content? "LDAP"
assert page.has_no_content? "LDAP-Shady-Gem"
end
end
1 change: 1 addition & 0 deletions test/models/pusher_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -636,6 +636,7 @@ def two_cert_chain(signing_key:, root_not_before: Time.current, cert_not_before:
"version_downloads" => 0,
"platform" => "ruby",
"authors" => "Joe User",
"trusted_publisher" => false,
"info" => "Some awesome gem",
"licenses" => "MIT",
"metadata" => { "foo" => "bar" },
Expand Down
32 changes: 32 additions & 0 deletions test/models/version_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,38 @@ class VersionTest < ActiveSupport::TestCase
end
end

context "#pushed_by_trusted_publisher?" do
context "with a trusted publisher api key association" do
setup do
@version = build(:version, :has_trusted_publisher)
end

should "return true" do
assert_predicate @version, :pushed_by_trusted_publisher?
end
end

context "with an untrusted publisher api key association" do
setup do
@version = build(:version, :has_untrusted_publisher)
end

should "return false" do
refute_predicate @version, :pushed_by_trusted_publisher?
end
end

context "with no api key association" do
setup do
@version = build(:version)
end

should "return false" do
refute_predicate @version, :pushed_by_trusted_publisher?
end
end
end

context "updated gems" do
setup do
@existing_gem = create(:rubygem)
Expand Down
3 changes: 2 additions & 1 deletion test/system/advanced_search_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ class AdvancedSearchTest < ApplicationSystemTestCase
fill_in "description", with: "foo"
fill_in "downloads", with: ">69"
fill_in "updated", with: ">2021-05-05"
fill_in "trusted_publisher", with: "true"

assert has_field? "Search Gems…", with: "name: hello summary: world description: foo downloads: >69 updated: >2021-05-05"
assert has_field? "Search Gems…", with: "name: hello summary: world description: foo downloads: >69 updated: >2021-05-05 trusted_publisher: true"
end
end