mattcasters commented on code in PR #8540:
URL: https://github.com/apache/hop/pull/8540#discussion_r4083725411
##########
plugins/transforms/http/src/main/java/org/apache/hop/pipeline/transforms/http/Http.java:
##########
@@ -279,6 +286,32 @@ private String readResponseBody(HttpEntity entity) throws
IOException {
}
}
+ /**
+ * The client to execute with: the one built from the selected REST
connection, or a client
+ * configured from this transform's own fields.
+ */
+ private CloseableHttpClient httpClient(HttpHost target) {
+ if (data.restConnectionClient != null) {
+ return data.restConnectionClient;
+ }
+ return createClientBuilder(target).build();
+ }
+
+ /**
+ * Bearer, API-key and preemptive Basic authentication are request headers
rather than answers to
+ * a challenge, so the connection writes them onto every request itself.
+ */
+ private void addConnectionAuthentication(HttpGet method, URI uri) throws
HopException {
+ if (data.restConnection == null) {
+ return;
+ }
+ Map<String, String> authHeaders = new LinkedHashMap<>();
+ data.restConnection.applyAuthentication(authHeaders, uri.toString());
Review Comment:
**[bug]** `RestConnection.applyAuthentication` sets the credential origin to
this request URL before `RestAuthenticator.targetMatchesOrigin` runs, so the
check always succeeds. Preemptive Basic (the default), Bearer, API key, and
OAuth headers are then written onto every request, including an absolute URL
taken from a field or typed in place of the connection base. The REST transform
does not do this: it keeps `createClientSettings()`'s origin (the base URL) and
calls `applyRequestHeaders` directly, which is what stops those secrets being
sent to a different host.
**Suggestion:** Build one `RestAuthenticator` from `createClientSettings()`
and call `applyRequestHeaders(headers, uri.toString())`. Do not call
`applyAuthentication` for row URLs.
##########
plugins/transforms/httppost/src/main/java/org/apache/hop/pipeline/transforms/httppost/HttpPost.java:
##########
@@ -535,11 +529,124 @@ public boolean init() {
data.realcloseIdleConnectionsTime =
Const.toInt(resolve(meta.getCloseIdleConnectionsTime()), -1);
+ if (!loadRestConnection()) {
+ return false;
+ }
+
+ // Resolved here rather than on the first row: the base URL it may hang
off belongs to the
+ // connection just loaded, and every request needs it, including the
first.
+ data.realUrl =
+ data.restConnection == null
+ ? resolve(meta.getUrl())
+ : RestConnection.resolveAgainstBase(
+ resolve(data.restConnection.getBaseUrl()),
resolve(meta.getUrl()));
+
return true;
}
return false;
}
+ /**
+ * Loads the selected REST connection and the client that goes with it.
Returns false when a
+ * connection is named but cannot be loaded, which has to stop the
transform: falling back to the
+ * transform's own fields would send the request somewhere else,
unauthenticated.
+ */
+ private boolean loadRestConnection() {
+ String realConnectionName = resolve(meta.getConnectionName());
+ if (Utils.isEmpty(realConnectionName)) {
+ return true;
+ }
+ try {
+ data.restConnection =
+
metadataProvider.getSerializer(RestConnection.class).load(realConnectionName);
+ if (data.restConnection == null) {
+ logError(
+ BaseMessages.getString(PKG, "HTTPPOST.Error.ConnectionNotFound",
realConnectionName));
+ return false;
+ }
+ data.restConnection.setVariables(this);
+ data.restConnectionClient =
+
RestClientFactory.createClient(data.restConnection.createClientSettings());
+ return true;
+ } catch (Exception e) {
+ // Keep the cause: a class loader split between the metadata plugin and
this transform
+ // surfaces here as a ClassCastException, which is not a missing
connection at all.
+ logError(
+ BaseMessages.getString(PKG, "HTTPPOST.Error.ConnectionNotLoaded",
realConnectionName), e);
+ return false;
+ }
+ }
+
+ @Override
+ public void dispose() {
+ if (data.restConnectionClient != null) {
+ try {
+ data.restConnectionClient.close();
+ } catch (IOException e) {
+ logError(BaseMessages.getString(PKG, "HTTPPOST.Error.ClosingClient"),
e);
+ }
+ data.restConnectionClient = null;
+ }
+ super.dispose();
+ }
+
+ /**
+ * The client to execute with: the one built from the selected REST
connection, or a client
+ * configured from this transform's own fields.
+ *
+ * <p>Credentials are registered for the host they belong to rather than for
any host at all: a
+ * proxy asking for authentication of its own used to be answered with the
web server's user name
+ * and password, because a single wildcard {@link
org.apache.hc.client5.http.auth.AuthScope}
+ * matched both challenges (issue #3440).
+ */
+ private CloseableHttpClient httpClient(HttpHost target) {
+ if (data.restConnectionClient != null) {
+ return data.restConnectionClient;
+ }
+ HttpClientManager.HttpClientBuilderFacade clientBuilder =
+ HttpClientManager.getInstance().createBuilder();
+
+ if (data.realConnectionTimeout > -1) {
+ clientBuilder.setConnectionTimeout(data.realConnectionTimeout);
+ }
+ if (data.realSocketTimeout > -1) {
+ clientBuilder.setSocketTimeout(data.realSocketTimeout);
+ }
+ if (StringUtils.isNotBlank(data.realHttpLogin)) {
+ clientBuilder.setCredentials(
+ data.realHttpLogin, data.realHttpPassword, new AuthScope(target));
+ }
+ if (StringUtils.isNotBlank(data.realProxyHost)) {
+ HttpHost proxy = new HttpHost("http", data.realProxyHost,
data.realProxyPort);
+ clientBuilder.setProxy(proxy.getHostName(), proxy.getPort(),
proxy.getSchemeName());
+ clientBuilder.setNonProxyHosts(data.realNonProxyHosts);
+ if (StringUtils.isNotBlank(data.realProxyUsername)) {
+ clientBuilder.setCredentials(
+ data.realProxyUsername, data.realProxyPassword, new
AuthScope(proxy));
+ }
+ }
+ if (meta.isIgnoreSsl()) {
+ clientBuilder.ignoreSsl(true);
+ }
+ return clientBuilder.build();
+ }
+
+ /**
+ * Bearer, API-key and preemptive Basic authentication are request headers
rather than answers to
+ * a challenge, so the connection writes them onto every request itself.
+ */
+ private void addConnectionAuthentication(
+ org.apache.hc.client5.http.classic.methods.HttpPost post, URI uri)
throws HopException {
+ if (data.restConnection == null) {
+ return;
+ }
+ Map<String, String> authHeaders = new LinkedHashMap<>();
+ data.restConnection.applyAuthentication(authHeaders, uri.toString());
Review Comment:
**[bug]** Same origin bypass as the HTTP client transform.
`applyAuthentication` rebinds the REST connection's credential origin to `uri`,
so a body posted to an absolute URL from the URL field (or any absolute URL
that is not the connection host) carries that connection's Basic, Bearer,
API-key, or OAuth credentials. `Rest.java` avoids this by leaving the origin on
the connection base URL.
**Suggestion:** Use `new
RestAuthenticator(restConnection.createClientSettings()).applyRequestHeaders(...)`
instead of `applyAuthentication`.
##########
plugins/actions/http/src/main/java/org/apache/hop/workflow/actions/http/ActionHttp.java:
##########
@@ -532,6 +501,137 @@ protected PasswordAuthentication
getPasswordAuthentication() {
return result;
}
+ /**
+ * The URL to call, as a URI HttpClient can route. Anything without a scheme
and a host is
+ * rejected here rather than further down, where it would surface as a less
obvious error.
+ */
+ private static URI toUri(String urlToUse) throws URISyntaxException {
+ URI uri = new URI(Const.NVL(urlToUse, "").trim());
+ if (uri.getScheme() == null || uri.getAuthority() == null) {
+ throw new URISyntaxException(String.valueOf(urlToUse), "No protocol or
host in the URL");
+ }
+ return uri;
+ }
+
+ /**
+ * The client for one request: proxy routing, the bypass list and both sets
of credentials.
+ *
+ * <p>Server and proxy credentials are registered for their own host only.
They used to share a
+ * single JVM-wide {@link java.net.Authenticator}, which answers a 401 from
the server and a 407
+ * from the proxy alike, so whichever asked first received the other's user
name and password.
+ */
+ private CloseableHttpClient createHttpClient(HttpHost target) {
+ HttpClientManager.HttpClientBuilderFacade builder =
+ HttpClientManager.getInstance().createBuilder();
+
+ String realProxyHost = resolve(proxyHostname);
+ if (!Utils.isEmpty(realProxyHost)) {
+ int realProxyPort = Const.toInt(resolve(proxyPort), DEFAULT_PROXY_PORT);
+ HttpHost proxy = new HttpHost("http", realProxyHost, realProxyPort);
+ builder.setProxy(proxy.getHostName(), proxy.getPort(),
proxy.getSchemeName());
+ builder.setNonProxyHosts(resolve(nonProxyHosts));
+ if (!Utils.isEmpty(resolve(proxyUsername))) {
+ builder.setCredentials(
+ resolve(proxyUsername),
+ Encr.decryptPasswordOptionallyEncrypted(resolve(proxyPassword)),
+ new AuthScope(proxy));
+ }
+ }
+
+ if (!Utils.isEmpty(resolve(username))) {
+ builder.setCredentials(
+ resolve(username),
+ Encr.decryptPasswordOptionallyEncrypted(resolve(password)),
+ new AuthScope(target));
+ }
+
+ builder.ignoreSsl(isIgnoreSsl());
+ return builder.build();
+ }
+
+ /**
+ * The REST connection this action was pointed at, or {@code null} when it
configures its own
+ * client.
+ */
+ private RestConnection loadRestConnection() throws HopException {
+ String realConnectionName = resolve(connectionName);
+ if (Utils.isEmpty(realConnectionName)) {
+ return null;
+ }
+ IHopMetadataProvider provider = getMetadataProvider();
+ if (provider == null) {
+ throw new HopException(
+ BaseMessages.getString(PKG, "ActionHTTP.Error.ConnectionNotFound",
realConnectionName));
+ }
+ try {
+ RestConnection connection =
+
provider.getSerializer(RestConnection.class).load(realConnectionName);
+ if (connection == null) {
+ throw new HopException(
+ BaseMessages.getString(PKG, "ActionHTTP.Error.ConnectionNotFound",
realConnectionName));
+ }
+ connection.setVariables(this);
+ return connection;
+ } catch (HopException e) {
+ throw e;
+ } catch (Exception e) {
+ // Keep the cause: a class loader split between the metadata plugin and
this action surfaces
+ // here as a ClassCastException, which is not a missing connection at
all.
+ throw new HopException(
+ BaseMessages.getString(PKG, "ActionHTTP.Error.ConnectionNotLoaded",
realConnectionName),
+ e);
+ }
+ }
+
+ /**
+ * Bearer, API-key and preemptive Basic authentication are request headers
rather than answers to
+ * a challenge, so the connection writes them onto every request itself.
+ */
+ private void addConnectionAuthentication(
+ ClassicHttpRequest request, RestConnection restConnection, String
urlToUse)
+ throws HopException {
+ if (restConnection == null) {
+ return;
+ }
+ Map<String, String> authHeaders = new LinkedHashMap<>();
+ restConnection.applyAuthentication(authHeaders, urlToUse);
Review Comment:
**[bug]** `applyAuthentication` overwrites the connection's auth origin with
`urlToUse`, so the origin check cannot reject a different host. With "Run for
every result row" that URL comes from a previous result, and an absolute URL is
also returned unchanged by `resolveAgainstBase`. The connection's preemptive
credentials are then sent to that host. A user-typed URL on another host has
the same effect, which the REST transform deliberately does not do.
**Suggestion:** Apply headers through a `RestAuthenticator` created from
`createClientSettings()` so the origin stays the connection base URL.
##########
plugins/actions/http/src/main/java/org/apache/hop/workflow/actions/http/ActionHttp.java:
##########
@@ -532,6 +501,137 @@ protected PasswordAuthentication
getPasswordAuthentication() {
return result;
}
+ /**
+ * The URL to call, as a URI HttpClient can route. Anything without a scheme
and a host is
+ * rejected here rather than further down, where it would surface as a less
obvious error.
+ */
+ private static URI toUri(String urlToUse) throws URISyntaxException {
+ URI uri = new URI(Const.NVL(urlToUse, "").trim());
+ if (uri.getScheme() == null || uri.getAuthority() == null) {
+ throw new URISyntaxException(String.valueOf(urlToUse), "No protocol or
host in the URL");
+ }
+ return uri;
+ }
+
+ /**
+ * The client for one request: proxy routing, the bypass list and both sets
of credentials.
+ *
+ * <p>Server and proxy credentials are registered for their own host only.
They used to share a
+ * single JVM-wide {@link java.net.Authenticator}, which answers a 401 from
the server and a 407
+ * from the proxy alike, so whichever asked first received the other's user
name and password.
+ */
+ private CloseableHttpClient createHttpClient(HttpHost target) {
+ HttpClientManager.HttpClientBuilderFacade builder =
+ HttpClientManager.getInstance().createBuilder();
+
+ String realProxyHost = resolve(proxyHostname);
+ if (!Utils.isEmpty(realProxyHost)) {
Review Comment:
**[suggestion]** When the proxy host is empty this client now always
connects directly. The previous `URLConnection` path used the JVM
`ProxySelector`, so a workflow that left the action proxy blank and relied on
`-Dhttp.proxyHost` / `java.net.useSystemProxies` will stop using that proxy.
Dropping `System.setProperty` is the right fix for #2962; reading the existing
selector is separate.
**Suggestion:** If no proxy host is set, install `SystemDefaultRoutePlanner`
(or `ProxySelector.getDefault()`) instead of leaving `DefaultRoutePlanner`,
which ignores system proxy settings.
##########
plugins/transforms/http/src/main/java/org/apache/hop/pipeline/transforms/http/HttpMeta.java:
##########
@@ -90,6 +91,17 @@ public class HttpMeta extends BaseTransformMeta<Http,
HttpData> {
private String closeIdleConnectionsTime;
/** URL / service to be called */
+ /**
+ * Optional REST connection supplying the client: proxy, credentials, TLS
and timeouts. When one
+ * is selected the transform's own authentication, proxy and SSL fields are
not read.
+ */
+ @HopMetadataProperty(
+ key = "connection_name",
+ injectionKey = "CONNECTION_NAME",
+ injectionKeyDescription = "HttpMeta.Injection.CONNECTION_NAME",
Review Comment:
**[suggestion]** The new injection descriptions
(`HttpMeta.Injection.CONNECTION_NAME`, `PROXY_USERNAME`, `PROXY_PASSWORD`,
`NON_PROXY_HOSTS`, and the matching `HTTPPOST.Injection.connectionName` /
`proxyUsername` / `proxyPassword` / `nonProxyHosts` keys) are not in the
message bundles. Metadata injection will show the unresolved key.
**Suggestion:** Add those strings to `messages_en_US.properties` for both
transforms.
##########
plugins/transforms/http/src/main/java/org/apache/hop/pipeline/transforms/http/Http.java:
##########
@@ -490,4 +555,47 @@ public boolean init() {
}
return false;
}
+
+ /**
+ * Loads the selected REST connection and the client that goes with it.
Returns false when a
+ * connection is named but cannot be loaded, which has to stop the
transform: falling back to the
+ * transform's own fields would send the request somewhere else,
unauthenticated.
+ */
+ private boolean loadRestConnection() {
+ String realConnectionName = resolve(meta.getConnectionName());
+ if (Utils.isEmpty(realConnectionName)) {
+ return true;
+ }
+ try {
+ data.restConnection =
+
metadataProvider.getSerializer(RestConnection.class).load(realConnectionName);
Review Comment:
**[suggestion]** `HttpMeta` / `HttpPostMeta` never set `classLoaderGroup =
"rest"`, unlike `ActionHttp` and `RestMeta` in this same change.
`dependencies.xml` only puts `hop-misc-rest` on this plugin's own loader, so
`RestConnection.class` here is a second copy of the metadata type. The catch
below is written for the `ClassCastException` that split produces, and the
action comment says the group is what loads the connection from the loader that
defined it.
**Suggestion:** Add `classLoaderGroup = "rest"` on both transform
`@Transform` annotations, matching the action.
##########
plugins/actions/http/src/main/java/org/apache/hop/workflow/actions/http/ActionHttpDialog.java:
##########
@@ -679,6 +753,80 @@ private Group setupAuthGroup(Composite wGeneralComp) {
return wAuthentication;
}
+ private void setupConnectionLine(int margin, Composite wGeneralComp) {
+ wConnection =
+ new MetaSelectionLine<>(
+ variables,
+ metadataProvider,
+ RestConnection.class,
+ wGeneralComp,
+ SWT.SINGLE | SWT.LEFT | SWT.BORDER,
+ BaseMessages.getString(PKG, "ActionHTTP.Connection.Label"),
+ BaseMessages.getString(PKG, "ActionHTTP.Connection.Tooltip"));
+ PropsUi.setLook(wConnection);
+ FormData fdConnection = new FormData();
+ fdConnection.left = new FormAttachment(0, 0);
+ fdConnection.top = new FormAttachment(0, margin);
+ fdConnection.right = new FormAttachment(100, 0);
+ wConnection.setLayoutData(fdConnection);
+ wConnection.addListener(
+ SWT.Selection,
+ e -> {
+ action.setChanged();
+ activateConnectionSupersededFields();
+ });
+ wConnection.addModifyListener(
+ e -> {
+ action.setChanged();
+ activateConnectionSupersededFields();
+ });
+ try {
+ wConnection.fillItems();
+ } catch (Exception e) {
+ new ErrorDialog(
+ shell,
+ BaseMessages.getString(PKG, "System.Dialog.Error.Title"),
+ "Error getting the list of REST connections",
Review Comment:
**[nit]** This dialog error is hardcoded English. The same string is copied
in `HttpDialog` and `HttpPostDialog`.
**Suggestion:** Move it to the action/transform message bundles, same as the
other dialog titles.
##########
plugins/actions/http/src/main/java/org/apache/hop/workflow/actions/http/ActionHttp.java:
##########
@@ -532,6 +501,137 @@ protected PasswordAuthentication
getPasswordAuthentication() {
return result;
}
+ /**
+ * The URL to call, as a URI HttpClient can route. Anything without a scheme
and a host is
+ * rejected here rather than further down, where it would surface as a less
obvious error.
+ */
+ private static URI toUri(String urlToUse) throws URISyntaxException {
+ URI uri = new URI(Const.NVL(urlToUse, "").trim());
Review Comment:
**[bug]** `new URI(String)` rejects URLs that `new URL(String)` used to
open. `http://example.com/a b` and `http://example.com/path?q=a b` now fail
here with `URISyntaxException` and the action errors; previously
`URLConnection` accepted them. URLs with userinfo
(`http://user:pass@host/file`) pass this check and then fail later in
HttpClient 5 with `ProtocolException: Request URI authority contains deprecated
userinfo component`.
**Suggestion:** Build the URI with the multi-argument constructor (or an
encoder) so illegal path/query characters are escaped, and strip userinfo
before creating the request, surfacing a clear error if it was being used as
credentials.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
