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

Fill AuthScope with host and port #10176

Merged
merged 16 commits into from Aug 5, 2019
Merged

Fill AuthScope with host and port #10176

merged 16 commits into from Aug 5, 2019

Conversation

big-guy
Copy link
Member

@big-guy big-guy commented Aug 5, 2019

Fixes #?

Context

Contributor Checklist

  • Review Contribution Guidelines
  • Make sure that all commits are signed off to indicate that you agree to the terms of Developer Certificate of Origin.
  • Check "Allow edit from maintainers" option in pull request so that additional changes can be pushed by Gradle team
  • Provide integration tests (under <subproject>/src/integTest) to verify changes from a user perspective
  • Provide unit tests (under <subproject>/src/test) to verify logic
  • Update User Guide, DSL Reference, and Javadoc for public-facing changes
  • Ensure that tests pass locally: ./gradlew <changed-subproject>:check

Gradle Core Team Checklist

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation
  • Recognize contributor in release notes

NTLMCredentials ntlmCredentials = new NTLMCredentials(passwordCredentials);
Credentials httpCredentials = new NTCredentials(ntlmCredentials.getUsername(), ntlmCredentials.getPassword(), ntlmCredentials.getWorkstation(), ntlmCredentials.getDomain());
credentialsProvider.setCredentials(new AuthScope(host, port, AuthScope.ANY_REALM, AuthSchemes.NTLM), httpCredentials);
assert host != null : "HTTP credentials and authentication require a host scope to be defined as well";
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to add a check that port != -1 here?

Copy link
Member Author

Choose a reason for hiding this comment

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

The port is going to be -1 a lot of the time because the URL doesn't specify the port explicitly. We would need to check the protocol and assume the right default (80 for http and 443 for https). That seems doable, but I think it would raise the danger of breaking someone that has their repository setup to redirect http to https and only use the http URL with authentication?

Are hosts usually distinguishable by port number for these kinds of auth schemes?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think your assessment is appropriate here.

Are hosts usually distinguishable by port number for these kinds of auth schemes?

I don't understand the question.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean, I think you could have different sets of credentials for example.com:1234 and example.com:5678, but if we didn't allow that for now, would that be a big deal?

@@ -137,12 +139,12 @@ class HttpClientConfigurerTest extends Specification {
configurer.configure(httpClientBuilder)

then:
def basicCredentials = httpClientBuilder.credentialsProvider.getCredentials(new AuthScope(AuthScope.ANY_HOST, AuthScope.ANY_PORT))
def basicCredentials = httpClientBuilder.credentialsProvider.getCredentials(new AuthScope("host", 1234))
Copy link
Contributor

Choose a reason for hiding this comment

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

You've got these "magic strings/numbers" hard coded in multiple places in the test, do you want to extract them to a variable or constants in the scope of the test or class.

big-guy and others added 2 commits August 5, 2019 15:13
Co-Authored-By: Jonathan Leitschuh <Jonathan.Leitschuh@gradle.com>
@big-guy big-guy added this to the 5.6 RC2 milestone Aug 5, 2019
@big-guy big-guy merged commit de6f4f1 into release Aug 5, 2019
@big-guy
Copy link
Member Author

big-guy commented Aug 5, 2019

@ghale I'm going to merge this into release and get a nightly kicked off. I'll follow up on any review items you have in a follow up PR.

Copy link
Contributor

@JLLeitschuh JLLeitschuh left a comment

Choose a reason for hiding this comment

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

LGTM!

@ghale
Copy link
Member

ghale commented Aug 6, 2019

@big-guy Per our conversation, looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants