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

PluginManager: Add Support for basic auth #12445

Merged
merged 1 commit into from Aug 5, 2015

Conversation

spinscale
Copy link
Contributor

In order to support the URL notation including a user/pass combination
(like http://user:pass@host/plugin.zip) the auth info needs to be added
manually.

@@ -472,6 +480,53 @@ public void testOfficialPluginName_ThrowsException() throws IOException {
}
}

@Test
public void testThatBasicAuthIsSupported() throws Exception {
int port = randomIntBetween(49000, 65000);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we not do this and bind to port 0 (ephemeral) instead. Otherwise this test will always randomly fail, because i'm gonna sometimes have something running on that port.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

grrr.. forget to remove. thx for the reminder. fixed!

@spinscale spinscale force-pushed the 1507-pluginmanager-basic-auth branch from a1436e4 to c39ae37 Compare August 3, 2015 11:53
@jpountz
Copy link
Contributor

jpountz commented Aug 4, 2015

LGTM

@s1monw
Copy link
Contributor

s1monw commented Aug 4, 2015

I think we should fail if we don't use SSL here - we should never share the password with the rest of the world

@spinscale
Copy link
Contributor Author

true, will change the PR accordingly... testing is going to be tricky with SSL I guess

@spinscale spinscale force-pushed the 1507-pluginmanager-basic-auth branch from c39ae37 to 8730729 Compare August 5, 2015 09:39
@spinscale
Copy link
Contributor Author

updated this PR to only support basic auth for HTTPS.

Test implementation note: I am temporarily replacing the HttpsUrlConnection SSL socket factory while running this test in order to accept a self-signed certificate - if someone has a smarter idea I am all ears.

// in case the plugin manager is its own project, this can become an authenticator
boolean isSecureProcotol = "https".equalsIgnoreCase(aSource.getProtocol());
boolean isAuthInfoSet = !Strings.isNullOrEmpty(aSource.getUserInfo());
if (isAuthInfoSet && isSecureProcotol) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we throw an exception here too if it's set but not secure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, fixed. running tests and getting it in. thx!

@s1monw
Copy link
Contributor

s1monw commented Aug 5, 2015

LGTM left one minor comment

@spinscale spinscale force-pushed the 1507-pluginmanager-basic-auth branch from 8730729 to fb9f427 Compare August 5, 2015 12:00
In order to support the URL notation including a user/pass combination
(like http://user:pass@host/plugin.zip) the auth info needs to be added
manually.
@spinscale spinscale force-pushed the 1507-pluginmanager-basic-auth branch from fb9f427 to 5a70136 Compare August 5, 2015 13:57
@spinscale spinscale merged commit 5a70136 into elastic:master Aug 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants