-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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: Make the HTTP-Client use pre-emptive authentication #7255
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…rd authentication scenario
Marked draft pending inclusion of improvements from #7268 (adding configurability for hosted suppressions and support for Bearer Authentication + exposure of credential configuration for hostedSuppressions) |
Might be better to do the remainder in a separate PR, marking this one as ready for review/merge. PR for extending auth to various integrations and enabling bearer auth to follow in a separate PR. |
final String artifactId = xpath.evaluate( | ||
"/org.sonatype.nexus.rest.model.NexusArtifact/artifactId", | ||
try { | ||
// JSON would be more elegant, but there's not currently a dependency |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not that you have to change this - but we do have this dependency:
Lines 1231 to 1235 in 486ca94
<dependency> | |
<groupId>org.glassfish</groupId> | |
<artifactId>javax.json</artifactId> | |
<version>${org.glassfish.javax.json.version}</version> | |
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it was very tempting to change its behavior while sifting through the code, but thought it would be better to first cleanly fix authentication and leave the TODO-ish comment in as is for another time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@aikebah see my one comment - but you don't have to change anything. I'm fine if you just want to merge the PR. |
Make the HTTP-Client use pre-emptive authentication for configured server credentials and extend HTTPClient usage to Nexus search
Fixes #7108
Fixes #7253
Description of Change
Use pre-emptive authentication for the servers for which credentials are configured.
And add usage of the Apache HTTPClient for the web requests from Nexus search.
Related issues
Fixes #7108
Fixes #7253
Have test cases been added to cover the new functionality?
no;
Nexus search locally manually tested against private instances of Nexus v2 and Nexus v3 including access via a local squid proxy container.