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

fix: 7093 add username/password properties to be able to authenticate for central.content.url and analyzer.central.url again #7169

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

drijkersbq
Copy link

@drijkersbq drijkersbq commented Nov 19, 2024

fix: add username/password properties to be able to authenticate for central.content.url and analyzer.central.url again

Fixes Issue

7093

Description of Change

Added username and password properties for authentication of both the central.content.url and analyzer.central.url:

  • analyzer.central.username
  • analyzer.central.password
  • central.content.username
  • central.content.password

This fixes the problem that the new httpclient doesn't support userinfo components in these url's anymore.

Have test cases been added to cover the new functionality?

no

… for central.content.url and analyzer.central.url again
@aikebah
Copy link
Collaborator

aikebah commented Nov 20, 2024

Thanks for the PR.

As it adds CLI commandline arguments cli/src/site/markdown/arguments.md should also be updated to make documentation reflect the new arguments.

It would be worthwhile to also add the same configurability to the maven plugin and Ant task (and the gradle plugin, which is maintained in a different repo), but I'm perfectly fine in enhancing those as separate PRs

@aikebah aikebah added this to the 11.2.0 milestone Nov 20, 2024
@boring-cyborg boring-cyborg bot added the documentation site documentation label Nov 21, 2024
@drijkersbq
Copy link
Author

I have added the two arguments that are added to the cli to the arguments.md as requested

@aikebah
Copy link
Collaborator

aikebah commented Nov 21, 2024

@drijkersbq Overlooked in my initial review, but spotted it after you added the documentation: In the settings you introduce properties to override user/pass for search as well as for content. But in the CLI arguments you only add a way to override the credentials for search. Intentional? Or accidental because credentials and host happen to be identical in your case so that defining them once makes both cases succeed for you?

If host and port are the same one set of creds would replace the other, as the credentials put in a map linked to host/port combo, which means that if both are the same for search- and content-URL it would likely go unnoticed that only one of the two was attempted to be configured (unless the proxy host was expecting different credentials for search vs content paths, in which case one of the two would still fail, but that would be an unsupported case with the current codebase anyhow given that neither URI-paths nor authentication-realms play a role in the current configs for credentials)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli changes to the cli documentation site documentation utils changes to utils
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants