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.

Sign up for GitHub

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

add Maven credential metadata to the URLs it searches for POM files #5884

Merged
merged 2 commits into from
Oct 17, 2022
Merged

add Maven credential metadata to the URLs it searches for POM files #5884

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e562c98
add maven credential metadata to the urls it searches for pom files
jakecoffman Oct 13, 2022
f05e712
Merge branch 'main' into jakecoffman/maven-creds-settings-xml
jakecoffman Oct 17, 2022
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 maven/lib/dependabot/maven/file_parser/property_value_finder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,9 @@ class PropertyValueFinder

DOT_SEPARATOR_REGEX = %r{\.(?!\d+([.\/_\-]|$)+)}.freeze

def initialize(dependency_files:)
def initialize(dependency_files:, credentials: [])
@dependency_files = dependency_files
@credentials = credentials
end

def property_details(property_name:, callsite_pom:)
Expand Down Expand Up @@ -119,6 +120,7 @@ def repositories_finder
@repositories_finder ||=
RepositoriesFinder.new(
dependency_files: dependency_files,
credentials: @credentials,
evaluate_properties: false
)
end
Expand Down
13 changes: 10 additions & 3 deletions maven/lib/dependabot/maven/file_parser/repositories_finder.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@ class RepositoriesFinder
CENTRAL_REPO_URL = "https://repo.maven.apache.org/maven2"
SUPER_POM = { url: CENTRAL_REPO_URL, id: "central" }

def initialize(dependency_files:, evaluate_properties: true)
def initialize(dependency_files:, credentials: [], evaluate_properties: true)
@dependency_files = dependency_files
@credentials = credentials

# We need the option not to evaluate properties so as not to have a
# circular dependency between this class and the PropertyValueFinder
Expand All @@ -39,7 +40,7 @@ def initialize(dependency_files:, evaluate_properties: true)
def repository_urls(pom:, exclude_inherited: false)
entries = gather_repository_urls(pom: pom, exclude_inherited: exclude_inherited)
ids = Set.new
entries.map do |entry|
urls_from_credentials + entries.map do |entry|
next if entry[:id] && ids.include?(entry[:id])

ids.add(entry[:id]) unless entry[:id].nil?
Expand Down Expand Up @@ -119,7 +120,7 @@ def internal_dependency_poms
end

def fetch_remote_parent_pom(group_id, artifact_id, version, repo_urls)
(repo_urls + [CENTRAL_REPO_URL]).uniq.each do |base_url|
(urls_from_credentials + repo_urls + [CENTRAL_REPO_URL]).uniq.each do |base_url|
url = remote_pom_url(group_id, artifact_id, version, base_url)

@maven_responses ||= {}
Expand Down Expand Up @@ -155,6 +156,12 @@ def remote_pom_url(group_id, artifact_id, version, base_repo_url)
"#{artifact_id}-#{version}.pom"
end

def urls_from_credentials
@credentials.
select { |cred| cred["type"] == "maven_repository" }.
Copy link
Contributor

Choose a reason for hiding this comment

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

I had been thinking we'd only do this for creds with replaces-base: true. I can see now that you might want this but still allow a public registry though. Is there any potential for breaking existing jobs if we start to automatically move the registry sources to the front?

Alternatives might be:

  1. Introducing a new "global" key on the registry to indicate it comes from a global configuration not checked into the repo so we should insert it
  2. Use replaces-base which would also remove automatic usage of the default registry but allow the default registry to added back via inclusion in dependabot.yml registries.

But if it doesn't seem likely this would break anything we could try this out first.

Copy link
Member Author

@jakecoffman jakecoffman Oct 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

My thought was replaces-base would remove the default registry if it is true in any of the maven creds. I was going to follow this up by adding that in the next bite-sized PR.

This approach does allow fallback to Maven Central. For users of GitHub Registry that's probably what they want. It's also possible GHR users would want to check Central first to avoid 404ing first. If we drop the username and password requirement, then they have complete control of the order by specifying in dependabot.yml.

filter_map { |cred| cred["url"]&.strip&.gsub(%r{/$}, "") }
end

def contains_property?(value)
value.match?(property_regex)
end
Expand Down
28 changes: 26 additions & 2 deletions maven/spec/dependabot/maven/file_parser/repositories_finder_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,13 @@
require "dependabot/maven/file_parser/repositories_finder"

RSpec.describe Dependabot::Maven::FileParser::RepositoriesFinder do
let(:finder) { described_class.new(dependency_files: dependency_files) }

let(:finder) do
described_class.new(
dependency_files: dependency_files,
credentials: credentials
)
end
let(:credentials) { [] }
let(:dependency_files) { [base_pom] }
let(:base_pom) do
Dependabot::DependencyFile.new(
Expand Down Expand Up @@ -51,6 +56,25 @@
end
end

context "with credentials" do
let(:base_pom_fixture_name) { "basic_pom.xml" }
let(:credentials) do
[
{ "type" => "maven_repository", "url" => "https://example.com" },
{ "type" => "git_source", "url" => "https://github.com" } # ignored since it's not maven
]
end

it "adds the credential urls first" do
expect(repository_urls).to eq(
%w(
https://example.com
https://repo.maven.apache.org/maven2
)
)
end
end

context "that use properties" do
let(:base_pom_fixture_name) { "property_repo_pom.xml" }

Expand Down