You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by aledsage <gi...@git.apache.org> on 2017/08/03 16:44:47 UTC
[GitHub] brooklyn-server pull request #790: Adds HttpFeed.preemptiveBasicAuth
GitHub user aledsage opened a pull request:
https://github.com/apache/brooklyn-server/pull/790
Adds HttpFeed.preemptiveBasicAuth
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aledsage/brooklyn-server add-HttpFeed-preemptiveBasicAuth
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/brooklyn-server/pull/790.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #790
----
commit 6b9c6ac172a5e1ddfbc07f624324a9a589a7d7bc
Author: Aled Sage <al...@gmail.com>
Date: 2017-08-03T16:38:41Z
Adds HttpFeed.preemptiveBasicAuth
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #790: Adds HttpFeed.preemptiveBasicAuth
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/brooklyn-server/pull/790
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #790: Adds HttpFeed.preemptiveBasicAuth
Posted by duncangrant <gi...@git.apache.org>.
Github user duncangrant commented on the issue:
https://github.com/apache/brooklyn-server/pull/790
Could you update the description to explain what this does in a bit more detail please?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #790: Adds HttpFeed.preemptiveBasicAuth
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/790#discussion_r131352449
--- Diff: core/src/main/java/org/apache/brooklyn/feed/http/HttpFeed.java ---
@@ -228,6 +235,25 @@ public Builder httpExecutor(HttpExecutor val) {
this.httpExecutor = val;
return this;
}
+ public Map<String, String> buildBaseHeaders() {
+ if (Boolean.TRUE.equals(preemptiveBasicAuth)) {
+ Credentials creds = credentials;
+ if (creds == null) {
+ throw new IllegalArgumentException("Must not enable preemptiveBasicAuth when there are no credentials, in feed for "+baseUri);
+ }
+ String username = checkNotNull(creds.getUserPrincipal().getName(), "username");
--- End diff --
I think we leave the name/password as-is. If we want to trim, or warn on extra white space, that should be done earlier (e.g. at the level of the config key's validation rules?).
Good point about colon in the username. I'll change it to fail-fast if it has a colon.
For the record, from RFC 7617 section 2:
```
Furthermore, a user-id containing a colon character is invalid, as
the first colon in a user-pass string separates user-id and password
from one another; text after the first colon is part of the password.
User-ids containing colons cannot be encoded in user-pass strings.
Note that many user agents produce user-pass strings without checking
that user-ids supplied by users do not contain colons; recipients
will then treat part of the username input as part of the password.
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #790: Adds HttpFeed.preemptiveBasicAuth
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/790#discussion_r131344099
--- Diff: core/src/main/java/org/apache/brooklyn/feed/http/HttpFeed.java ---
@@ -228,6 +235,25 @@ public Builder httpExecutor(HttpExecutor val) {
this.httpExecutor = val;
return this;
}
+ public Map<String, String> buildBaseHeaders() {
+ if (Boolean.TRUE.equals(preemptiveBasicAuth)) {
+ Credentials creds = credentials;
+ if (creds == null) {
+ throw new IllegalArgumentException("Must not enable preemptiveBasicAuth when there are no credentials, in feed for "+baseUri);
+ }
+ String username = checkNotNull(creds.getUserPrincipal().getName(), "username");
+ String password = creds.getPassword();
+ String toencode = username + (password == null ? "" : ":"+password);
--- End diff --
Don't drop the `:` if the password is null (RFC 7617 section 2).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #790: Adds HttpFeed.preemptiveBasicAuth
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/790
Thanks @duncangrant - merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #790: Adds HttpFeed.preemptiveBasicAuth
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/790#discussion_r131338828
--- Diff: core/src/main/java/org/apache/brooklyn/feed/http/HttpFeed.java ---
@@ -228,6 +235,25 @@ public Builder httpExecutor(HttpExecutor val) {
this.httpExecutor = val;
return this;
}
+ public Map<String, String> buildBaseHeaders() {
+ if (Boolean.TRUE.equals(preemptiveBasicAuth)) {
+ Credentials creds = credentials;
+ if (creds == null) {
+ throw new IllegalArgumentException("Must not enable preemptiveBasicAuth when there are no credentials, in feed for "+baseUri);
+ }
+ String username = checkNotNull(creds.getUserPrincipal().getName(), "username");
--- End diff --
Should this be trimmed, or is it guaranteed to come trimmed by `getName`? Hm I guess by the time we get here we should rely on just the value of `getName` and not do any more munging. What do you think? However it might be worth verifying in here that the username doesn't contain a colon.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #790: Adds HttpFeed.preemptiveBasicAuth
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/790#discussion_r131354907
--- Diff: core/src/main/java/org/apache/brooklyn/feed/http/HttpFeed.java ---
@@ -228,6 +235,25 @@ public Builder httpExecutor(HttpExecutor val) {
this.httpExecutor = val;
return this;
}
+ public Map<String, String> buildBaseHeaders() {
+ if (Boolean.TRUE.equals(preemptiveBasicAuth)) {
+ Credentials creds = credentials;
+ if (creds == null) {
+ throw new IllegalArgumentException("Must not enable preemptiveBasicAuth when there are no credentials, in feed for "+baseUri);
--- End diff --
I tend to find the more-situation-specific Exceptions most useful from the developer point of view, as it can be handy to add breakpoints on specific types when debugging; if you want to break when the exception is thrown it can sometimes be a bother to set it on the more generic type, as it may get thrown in other places.
I agree there's no compelling reason to change the `IllegalArgumentException` here, however. A separate discussion could be had about possible more useful base exceptions. `IllegalBrooklynConfigurationException`, anyone? :-)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #790: Adds HttpFeed.preemptiveBasicAuth
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/790#discussion_r131346980
--- Diff: core/src/test/java/org/apache/brooklyn/feed/http/HttpFeedTest.java ---
@@ -360,7 +364,90 @@ public void testPollsMultiClearsOnSubsequentFailure() throws Exception {
server.shutdown();
}
+
+ @Test
+ public void testPreemptiveBasicAuth() throws Exception {
+ final String username = "brooklyn";
+ final String password = "hunter2";
+
+ feed = HttpFeed.builder()
+ .entity(entity)
+ .baseUrl(server.getUrl("/"))
+ .credentials(username, password)
+ .preemptiveBasicAuth(true)
+ .poll(new HttpPollConfig<Integer>(SENSOR_INT)
+ .period(100)
+ .onSuccess(HttpValueFunctions.responseCode())
+ .onException(Functions.constant(-1)))
+ .build();
+
+ EntityAsserts.assertAttributeEqualsEventually(entity, SENSOR_INT, 200);
+ RecordedRequest req = server.takeRequest();
+ String headerVal = req.getHeader("Authorization");
+ String expectedVal = getBasicAuthHeaderVal(username, password);
+ assertEquals(headerVal, expectedVal);
+ }
+
+ @Test
+ public void testPreemptiveBasicAuthFailsIfNoCredentials() throws Exception {
+ try {
+ feed = HttpFeed.builder()
+ .entity(entity)
+ .baseUrl(new URL("http://shouldNeverBeCalled.org"))
+ .preemptiveBasicAuth(true)
+ .poll(new HttpPollConfig<Integer>(SENSOR_INT)
+ .period(100)
+ .onSuccess(HttpValueFunctions.responseCode())
+ .onException(Functions.constant(-1)))
+ .build();
+ Asserts.shouldHaveFailedPreviously();
+ } catch (IllegalArgumentException e) {
+ Asserts.expectedFailureContains(e, "Must not enable preemptiveBasicAuth when there are no credentials");
+ }
+ }
+ // Expected behaviour of o.a.http.client is that it first sends the request without credentials,
+ // and then when given a challenge for basic-auth it re-sends the request with the basic-auth header.
+ @Test
+ public void testNonPreemptiveBasicAuth() throws Exception {
+ final String username = "brooklyn";
+ final String password = "hunter2";
+
+ if (server != null) server.shutdown();
+ server = BetterMockWebServer.newInstanceLocalhost();
+ server.enqueue(new MockResponse()
+ .setResponseCode(401)
+ .addHeader("WWW-Authenticate", "Basic"));
+ server.enqueue(new MockResponse()
+ .setResponseCode(200)
+ .setBody("Hello World"));
+ server.play();
+
+ feed = HttpFeed.builder()
+ .entity(entity)
+ .baseUrl(server.getUrl("/"))
+ .credentials(username, password)
+ .poll(new HttpPollConfig<Integer>(SENSOR_INT)
+ .period(Duration.ONE_MINUTE) // so only dealing with first request
+ .onSuccess(HttpValueFunctions.responseCode())
+ .onException(Functions.constant(-1)))
+ .build();
+
+ EntityAsserts.assertAttributeEqualsEventually(entity, SENSOR_INT, 200);
+ RecordedRequest req = server.takeRequest();
+ assertEquals(req.getHeader("Authorization"), null);
+
+ RecordedRequest req2 = server.takeRequest();
+ String headerVal = req2.getHeader("Authorization");
+ String expected = getBasicAuthHeaderVal(username, password);
+ assertEquals(headerVal, expected);
+ }
+
+ public static String getBasicAuthHeaderVal(String username, String password) {
+ String toencode = username + (password == null ? "" : ":"+password);
--- End diff --
same comment about not dropping the `:`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #790: Adds HttpFeed.preemptiveBasicAuth
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/790
Thanks @geomacy @duncangrant - comments addressed (in a second commit).
The jenkins build failure was real - I've also fixed that; it's building again.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #790: Adds HttpFeed.preemptiveBasicAuth
Posted by duncangrant <gi...@git.apache.org>.
Github user duncangrant commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/790#discussion_r131342918
--- Diff: core/src/test/java/org/apache/brooklyn/feed/http/HttpFeedTest.java ---
@@ -360,7 +364,90 @@ public void testPollsMultiClearsOnSubsequentFailure() throws Exception {
server.shutdown();
}
+
+ @Test
+ public void testPreemptiveBasicAuth() throws Exception {
+ final String username = "brooklyn";
+ final String password = "hunter2";
+
+ feed = HttpFeed.builder()
+ .entity(entity)
+ .baseUrl(server.getUrl("/"))
+ .credentials(username, password)
+ .preemptiveBasicAuth(true)
+ .poll(new HttpPollConfig<Integer>(SENSOR_INT)
+ .period(100)
+ .onSuccess(HttpValueFunctions.responseCode())
+ .onException(Functions.constant(-1)))
+ .build();
+
+ EntityAsserts.assertAttributeEqualsEventually(entity, SENSOR_INT, 200);
+ RecordedRequest req = server.takeRequest();
+ String headerVal = req.getHeader("Authorization");
+ String expectedVal = getBasicAuthHeaderVal(username, password);
+ assertEquals(headerVal, expectedVal);
+ }
+
+ @Test
+ public void testPreemptiveBasicAuthFailsIfNoCredentials() throws Exception {
+ try {
+ feed = HttpFeed.builder()
+ .entity(entity)
+ .baseUrl(new URL("http://shouldNeverBeCalled.org"))
+ .preemptiveBasicAuth(true)
+ .poll(new HttpPollConfig<Integer>(SENSOR_INT)
+ .period(100)
+ .onSuccess(HttpValueFunctions.responseCode())
+ .onException(Functions.constant(-1)))
+ .build();
+ Asserts.shouldHaveFailedPreviously();
+ } catch (IllegalArgumentException e) {
+ Asserts.expectedFailureContains(e, "Must not enable preemptiveBasicAuth when there are no credentials");
+ }
+ }
+ // Expected behaviour of o.a.http.client is that it first sends the request without credentials,
+ // and then when given a challenge for basic-auth it re-sends the request with the basic-auth header.
+ @Test
+ public void testNonPreemptiveBasicAuth() throws Exception {
+ final String username = "brooklyn";
+ final String password = "hunter2";
+
+ if (server != null) server.shutdown();
--- End diff --
The mock server stuff in this file is in a bit of a mess. I'd suggest we clean it up but not in this PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #790: Adds HttpFeed.preemptiveBasicAuth
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/790#discussion_r131351541
--- Diff: core/src/main/java/org/apache/brooklyn/feed/http/HttpFeed.java ---
@@ -228,6 +235,25 @@ public Builder httpExecutor(HttpExecutor val) {
this.httpExecutor = val;
return this;
}
+ public Map<String, String> buildBaseHeaders() {
+ if (Boolean.TRUE.equals(preemptiveBasicAuth)) {
+ Credentials creds = credentials;
+ if (creds == null) {
+ throw new IllegalArgumentException("Must not enable preemptiveBasicAuth when there are no credentials, in feed for "+baseUri);
+ }
+ String username = checkNotNull(creds.getUserPrincipal().getName(), "username");
+ String password = creds.getPassword();
+ String toencode = username + (password == null ? "" : ":"+password);
--- End diff --
Ah, thanks!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #790: Adds HttpFeed.preemptiveBasicAuth
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/790#discussion_r131351303
--- Diff: core/src/main/java/org/apache/brooklyn/feed/http/HttpFeed.java ---
@@ -228,6 +235,25 @@ public Builder httpExecutor(HttpExecutor val) {
this.httpExecutor = val;
return this;
}
+ public Map<String, String> buildBaseHeaders() {
+ if (Boolean.TRUE.equals(preemptiveBasicAuth)) {
+ Credentials creds = credentials;
+ if (creds == null) {
+ throw new IllegalArgumentException("Must not enable preemptiveBasicAuth when there are no credentials, in feed for "+baseUri);
--- End diff --
My personal preference is to create new exception types when it is useful to the user. But saying that, if it extends `IllegalArgumentException` then there's not much difference from their perspective.
If we created things as specific as `MisconfiguredPreemptiveAuthException`, I worry that this leads towards the extreme of an exception per config key!
Maybe a middle ground is to have an `IllegalConfigurationException` or some such?
I'm personally ok with us asserting exception message snippets in unit tests (they change rarely, and it confirms the message does appear at the top level rather than only being buried far down in the "caused by").
However, the text in this particular example is far too long!
Perhaps we should discuss this separately, and figure out our approach to such exception across-the-board.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #790: Adds HttpFeed.preemptiveBasicAuth
Posted by duncangrant <gi...@git.apache.org>.
Github user duncangrant commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/790#discussion_r131340473
--- Diff: core/src/main/java/org/apache/brooklyn/feed/http/HttpFeed.java ---
@@ -228,6 +235,25 @@ public Builder httpExecutor(HttpExecutor val) {
this.httpExecutor = val;
return this;
}
+ public Map<String, String> buildBaseHeaders() {
+ if (Boolean.TRUE.equals(preemptiveBasicAuth)) {
+ Credentials creds = credentials;
+ if (creds == null) {
+ throw new IllegalArgumentException("Must not enable preemptiveBasicAuth when there are no credentials, in feed for "+baseUri);
--- End diff --
We do this a lot in brooklyn and I'm not sure why. Given that we're using a typed language why not create a MisconfiguredPreemptiveAuthException? We can check the type of the exception thrown in tests and a tweak to the error text doesn't break the tests.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---