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 all commits
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
2 changes: 1 addition & 1 deletion slack-api-client/src/main/java/com/slack/api/Slack.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,7 @@ public String issueSocketModeUrl(String appToken) throws IOException {
} catch (SlackApiException e) {
String message = "Failed to connect to the Socket Mode API endpoint. (" +
"status: " + e.getResponse().code() + ", " +
"error: " + e.getError().getError() +
"error: " + (e.getError() != null ? e.getError().getError() : "") +
Copy link
Member Author

Choose a reason for hiding this comment

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

e.getError() cannot be null in a usual situation... but, just in case, we want to improve this part not to throw NPE (I encountered with the mock proxy sever returning 502 status)

")";
throw new IOException(message, e);
}
Expand Down
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
30 changes: 20 additions & 10 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 @@ -68,11 +68,16 @@ public void connect() throws Exception {
try (SocketModeClient client = slack.socketMode(VALID_APP_TOKEN)) {
AtomicBoolean received = new AtomicBoolean(false);
client.addWebSocketMessageListener(helloListener(received));
client.addWebSocketErrorListener(error -> {});
client.addWebSocketCloseListener((code, reason) -> {});
client.addEventsApiEnvelopeListener(envelope -> {});
client.addInteractiveEnvelopeListener(envelope -> {});
client.addSlashCommandsEnvelopeListener(envelope -> {});
client.addWebSocketErrorListener(error -> {
});
client.addWebSocketCloseListener((code, reason) -> {
});
client.addEventsApiEnvelopeListener(envelope -> {
});
client.addInteractiveEnvelopeListener(envelope -> {
});
client.addSlashCommandsEnvelopeListener(envelope -> {
});

client.connect();
Thread.sleep(500L);
Expand Down Expand Up @@ -163,11 +168,16 @@ public void connect_JavaWebSocket() throws Exception {
try (SocketModeClient client = slack.socketMode(VALID_APP_TOKEN, SocketModeClient.Backend.JavaWebSocket)) {
AtomicBoolean received = new AtomicBoolean(false);
client.addWebSocketMessageListener(helloListener(received));
client.addWebSocketErrorListener(error -> {});
client.addWebSocketCloseListener((code, reason) -> {});
client.addEventsApiEnvelopeListener(envelope -> {});
client.addInteractiveEnvelopeListener(envelope -> {});
client.addSlashCommandsEnvelopeListener(envelope -> {});
client.addWebSocketErrorListener(error -> {
});
client.addWebSocketCloseListener((code, reason) -> {
});
client.addEventsApiEnvelopeListener(envelope -> {
});
client.addInteractiveEnvelopeListener(envelope -> {
});
client.addSlashCommandsEnvelopeListener(envelope -> {
});

client.connect();
Thread.sleep(500L);
Expand Down
108 changes: 108 additions & 0 deletions ...-api-client/src/test/java/test_locally/api/socket_mode/SocketModeClient_Proxies_Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
package test_locally.api.socket_mode;

import com.slack.api.Slack;
import com.slack.api.SlackConfig;
import com.slack.api.socket_mode.SocketModeClient;
import lombok.extern.slf4j.Slf4j;
import okhttp3.Credentials;
import org.eclipse.jetty.proxy.ConnectHandler;
import org.eclipse.jetty.proxy.ProxyServlet;
import org.eclipse.jetty.server.Request;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.ServerConnector;
import org.eclipse.jetty.servlet.ServletContextHandler;
import org.eclipse.jetty.servlet.ServletHolder;
import org.junit.After;
import org.junit.Before;
import org.junit.Test;
import test_with_remote_apis.AuthProxyHeadersTest;
import util.PortProvider;
import util.socket_mode.MockWebApiServer;

import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import java.io.IOException;
import java.util.Collections;
import java.util.HashMap;
import java.util.Locale;
import java.util.Map;
import java.util.concurrent.atomic.AtomicInteger;

@Slf4j
public class SocketModeClient_Proxies_Test {

static final String VALID_APP_TOKEN = "xapp-valid-123123123123123123123123123123123123";

MockWebApiServer webApiServer = new MockWebApiServer();
SlackConfig config = new SlackConfig();

static Server proxyServer = new Server();
static ServerConnector proxyServerConnector = new ServerConnector(proxyServer);
static Integer proxyServerPort;

AtomicInteger proxyCallCount = new AtomicInteger(0);

@Before
public void setUp() throws Exception {
webApiServer.start();
config.setMethodsEndpointUrlPrefix(webApiServer.getMethodsEndpointPrefix());

// https://github.com/eclipse/jetty.project/blob/jetty-9.2.30.v20200428/examples/embedded/src/main/java/org/eclipse/jetty/embedded/ProxyServer.java
proxyServerPort = PortProvider.getPort(AuthProxyHeadersTest.class.getName());
proxyServerConnector.setPort(proxyServerPort);
proxyServer.addConnector(proxyServerConnector);
ConnectHandler proxy = new ConnectHandler() {
@Override
public void handle(String target, Request br, HttpServletRequest request, HttpServletResponse res)
throws ServletException, IOException {
log.info("Proxy server handles a new connection (target: {})", target);
super.handle(target, br, request, res);
if (res.getStatus() != 407) {
proxyCallCount.incrementAndGet();
}
}

@Override
protected boolean handleAuthentication(HttpServletRequest request, HttpServletResponse response, String address) {
for (String name : Collections.list(request.getHeaderNames())) {
log.info("{}: {}", name, request.getHeader(name));
if (name.toLowerCase(Locale.ENGLISH).equals("proxy-authorization")) {
return request.getHeader(name).equals("Basic bXktdXNlcm5hbWU6bXktcGFzc3dvcmQ=");
}
}
return false;
}
};
proxyServer.setHandler(proxy);
ServletContextHandler context = new ServletContextHandler(proxy, "/", ServletContextHandler.SESSIONS);
ServletHolder proxyServlet = new ServletHolder(ProxyServlet.class);
context.addServlet(proxyServlet, "/*");
proxyServer.start();

config.setProxyUrl("http://127.0.0.1:" + proxyServerPort);

Map<String, String> proxyHeaders = new HashMap<>();
String username = "my-username";
String password = "my-password";
proxyHeaders.put("Proxy-Authorization", Credentials.basic(username, password));
config.setProxyHeaders(proxyHeaders);
}

@After
public void tearDown() throws Exception {
webApiServer.stop();

proxyServer.removeConnector(proxyServerConnector);
proxyServer.stop();
}

@Test(expected = UnsupportedOperationException.class)
public void proxyAuth() throws Exception {
SlackConfig config = new SlackConfig();
config.setMethodsEndpointUrlPrefix(webApiServer.getMethodsEndpointPrefix());
config.setProxyUrl("http://my-username:my-password@localhost:" + proxyServerPort + "/");
Slack slack = Slack.getInstance(config);
slack.socketMode(VALID_APP_TOKEN, SocketModeClient.Backend.JavaWebSocket);
}
}