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.

Sign up for GitHub

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

Improve the error caused in the #792 scenario #807

Merged
merged 2 commits into from
Aug 11, 2021
Merged

Improve the error caused in the #792 scenario #807

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3688808
Improve the error caused in the #792 scenario
seratch Aug 11, 2021
fc09e63
Fix the test
seratch Aug 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion ...k-api-client/src/main/java/com/slack/api/socket_mode/impl/SocketModeClientJavaWSImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,17 +219,29 @@ public UnderlyingWebSocketSession(URI serverUri, Map<String, String> httpHeaders
// FIXME: the proxy settings here may not work
SlackConfig slackConfig = smc.getSlack().getHttpClient().getConfig();
Map<String, String> proxyHeaders = slackConfig.getProxyHeaders();
if (proxyHeaders == null) {
Copy link
Member Author

Choose a reason for hiding this comment

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

As the getProxyHeaders() method can return null value, we have a null check like we do in the default SocketModeClientTyrusImpl

proxyHeaders = new HashMap<>();
}

String proxyUrl = slackConfig.getProxyUrl();
if (proxyUrl != null) {
if (smc.getLogger().isDebugEnabled()) {
smc.getLogger().debug("The SocketMode client's going to use an HTTP proxy: {}", proxyUrl);
}
ProxyUrlUtil.ProxyUrl parsedProxy = ProxyUrlUtil.parse(proxyUrl);
if (parsedProxy.getUsername() != null && parsedProxy.getPassword() != null) {
// see also: https://github.com/slackapi/java-slack-sdk/issues/792#issuecomment-895961176
Copy link
Member Author

Choose a reason for hiding this comment

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

see also: #806

String message = "Unfortunately, " +
"having username:password with the Java-WebSocket library is not yet supported. " +
"Consider using other implementations such SocketModeClient.Backend.Tyrus.";
throw new UnsupportedOperationException(message);
}

InetSocketAddress proxyAddress = new InetSocketAddress(parsedProxy.getHost(), parsedProxy.getPort());
this.setProxy(new Proxy(Proxy.Type.HTTP, proxyAddress));
ProxyUrlUtil.setProxyAuthorizationHeader(proxyHeaders, parsedProxy);
}
if (slackConfig.getProxyHeaders() != null) {
if (proxyHeaders != null && !proxyHeaders.isEmpty()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

for improvement; proxyHeaders can be passed for other purposes

for (Map.Entry<String, String> each : proxyHeaders.entrySet()) {
this.addHeader(each.getKey(), each.getValue());
}
Expand Down
4 changes: 2 additions & 2 deletions slack-api-client/src/main/java/com/slack/api/util/http/ProxyUrlUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,14 +48,14 @@ public static ProxyUrl parse(String proxyUrl) {
.username(userAndPassword[0])
.password(userAndPassword[1])
.host(hostAndPort[0])
.port(hostAndPort.length == 2 ? Integer.parseInt(hostAndPort[1]) : 80)
.port(hostAndPort.length == 2 ? Integer.parseInt(hostAndPort[1].replace("/", "")) : 80)
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 found this potential issue while writing unit tests

.build();
} else {
String[] hostAndPort = proxyUrl.split("://")[1].split(":");
return ProxyUrl.builder()
.schema(schema)
.host(hostAndPort[0])
.port(hostAndPort.length == 2 ? Integer.parseInt(hostAndPort[1]) : 80)
.port(hostAndPort.length == 2 ? Integer.parseInt(hostAndPort[1].replace("/", "")) : 80)
.build();
}
}
Expand Down
9 changes: 9 additions & 0 deletions slack-api-client/src/test/java/test_locally/api/socket_mode/SocketModeClientTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,15 @@ public void messageReceiver() throws Exception {
// JavaWebSocket implementation
// -------------------------------------------------

@Test(expected = UnsupportedOperationException.class)
public void proxyAuth() throws Exception {
SlackConfig config = new SlackConfig();
config.setMethodsEndpointUrlPrefix(webApiServer.getMethodsEndpointPrefix());
config.setProxyUrl("http://user:pass@localhost:9000/");
Copy link
Member Author

Choose a reason for hiding this comment

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

Using "http://localhost:9000/" still works. Also, passing static proxyHeaders also works.

Slack slack = Slack.getInstance(config);
slack.socketMode(VALID_APP_TOKEN, SocketModeClient.Backend.JavaWebSocket);
}

@Test
public void attributes_JavaWebSocket() throws Exception {
try (SocketModeClient client = slack.socketMode(VALID_APP_TOKEN, SocketModeClient.Backend.JavaWebSocket)) {
Expand Down