Skip to content

Remove PowerMock from the Slack GCF sample. #3186

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

Merged
merged 4 commits into from
Jun 18, 2020
Merged

Remove PowerMock from the Slack GCF sample. #3186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c7aec4e
Remove PowerMock from the Slack GCF sample.
eamonnmcmanus May 7, 2020
a870f53
fix lint
saturnism Jun 17, 2020
f2c07d3
Fix the tests
saturnism Jun 18, 2020
cd34ee8
Merge branch 'master' into eaamon-mockito3
saturnism Jun 18, 2020
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
19 changes: 0 additions & 19 deletions functions/slack/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
</parent>

<properties>
<powermock.version>2.0.7</powermock.version>
<maven.compiler.target>11</maven.compiler.target>
<maven.compiler.source>11</maven.compiler.source>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
Expand Down Expand Up @@ -101,24 +100,6 @@
<version>29.0-jre</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.powermock</groupId>
<artifactId>powermock-core</artifactId>
<version>${powermock.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.powermock</groupId>
<artifactId>powermock-module-junit4</artifactId>
<version>${powermock.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.powermock</groupId>
<artifactId>powermock-api-mockito2</artifactId>
<version>${powermock.version}</version>
<scope>test</scope>
</dependency>
</dependencies>

<!-- Disable tests during GCF builds (from parent POM) -->
Expand Down
38 changes: 21 additions & 17 deletions functions/slack/src/main/java/functions/SlackSlashCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@
import java.io.IOException;
import java.net.HttpURLConnection;
import java.security.GeneralSecurityException;
import java.util.HashMap;
import java.util.List;
import java.util.Optional;
import java.util.logging.Logger;
import java.util.stream.Collectors;

Expand All @@ -43,14 +42,24 @@ public class SlackSlashCommand implements HttpFunction {
private static final String SLACK_SECRET = getenv("SLACK_SECRET");
private static final Gson gson = new Gson();

private Kgsearch kgClient;
private SlackSignature.Verifier verifier;
private final String apiKey;
private final Kgsearch kgClient;
private final SlackSignature.Verifier verifier;

public SlackSlashCommand() throws IOException, GeneralSecurityException {
kgClient = new Kgsearch.Builder(
GoogleNetHttpTransport.newTrustedTransport(), new JacksonFactory(), null).build();
this(new SlackSignature.Verifier(new SlackSignature.Generator(SLACK_SECRET)));
}

SlackSlashCommand(SlackSignature.Verifier verifier) throws IOException, GeneralSecurityException {
this(verifier, API_KEY);
}

verifier = new SlackSignature.Verifier(new SlackSignature.Generator(SLACK_SECRET));
SlackSlashCommand(SlackSignature.Verifier verifier, String apiKey)
throws IOException, GeneralSecurityException {
this.verifier = verifier;
this.apiKey = apiKey;
this.kgClient = new Kgsearch.Builder(
GoogleNetHttpTransport.newTrustedTransport(), new JacksonFactory(), null).build();
}

// Avoid ungraceful deployment failures due to unset environment variables.
Expand All @@ -73,18 +82,13 @@ private static String getenv(String name) {
* @return true if the provided request came from Slack, false otherwise
*/
boolean isValidSlackWebhook(HttpRequest request, String requestBody) {

// Check for headers
HashMap<String, List<String>> headers = new HashMap(request.getHeaders());
if (!headers.containsKey("X-Slack-Request-Timestamp")
|| !headers.containsKey("X-Slack-Signature")) {
Optional<String> maybeTimestamp = request.getFirstHeader("X-Slack-Request-Timestamp");
Optional<String> maybeSignature = request.getFirstHeader("X-Slack-Signature");
if (!maybeTimestamp.isPresent() || !maybeSignature.isPresent()) {
return false;
}
return verifier.isValid(
headers.get("X-Slack-Request-Timestamp").get(0),
requestBody,
headers.get("X-Slack-Signature").get(0),
1L);
return verifier.isValid(maybeTimestamp.get(), requestBody, maybeSignature.get(), 1L);
}
// [END functions_verify_webhook]

Expand Down Expand Up @@ -158,7 +162,7 @@ String formatSlackMessage(JsonObject kgResponse, String query) {
JsonObject searchKnowledgeGraph(String query) throws IOException {
Kgsearch.Entities.Search kgRequest = kgClient.entities().search();
kgRequest.setQuery(query);
kgRequest.setKey(API_KEY);
kgRequest.setKey(apiKey);

return gson.fromJson(kgRequest.execute().toString(), JsonObject.class);
}
Expand Down
50 changes: 17 additions & 33 deletions functions/slack/src/test/java/functions/SlackSlashCommandTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@
package functions;

import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertThrows;
import static org.mockito.Mockito.any;
import static org.mockito.Mockito.anyLong;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.powermock.api.mockito.PowerMockito.mock;
import static org.powermock.api.mockito.PowerMockito.when;
import static org.mockito.Mockito.when;

import com.github.seratch.jslack.app_backend.SlackSignature;
import com.google.api.client.googleapis.json.GoogleJsonResponseException;
Expand All @@ -34,15 +36,12 @@
import java.io.StringWriter;
import java.net.HttpURLConnection;
import java.security.GeneralSecurityException;
import java.util.Arrays;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import org.junit.Before;
import org.junit.Test;
import org.mockito.ArgumentMatchers;
import org.mockito.Mock;
import org.powermock.reflect.Whitebox;
import org.mockito.MockitoAnnotations;

public class SlackSlashCommandTest {

Expand All @@ -58,37 +57,27 @@ public class SlackSlashCommandTest {

@Before
public void beforeTest() throws IOException {
request = mock(HttpRequest.class);
when(request.getReader()).thenReturn(new BufferedReader(new StringReader("")));
MockitoAnnotations.initMocks(this);

response = mock(HttpResponse.class);
when(request.getReader()).thenReturn(new BufferedReader(new StringReader("")));

responseOut = new StringWriter();

writerOut = new BufferedWriter(responseOut);
when(response.getWriter()).thenReturn(writerOut);

alwaysValidVerifier = mock(SlackSignature.Verifier.class);
when(alwaysValidVerifier.isValid(
ArgumentMatchers.any(),
ArgumentMatchers.any(),
ArgumentMatchers.any(),
ArgumentMatchers.anyLong())
).thenReturn(true);
when(alwaysValidVerifier.isValid(any(), any(), any(), anyLong())).thenReturn(true);

// Construct valid header list
String validSlackSignature = System.getenv("SLACK_TEST_SIGNATURE");
String timestamp = "0"; // start of Unix epoch

Map<String, List<String>> validHeaders = Map.of(
"X-Slack-Signature", Arrays.asList(validSlackSignature),
"X-Slack-Request-Timestamp", Arrays.asList(timestamp)
);
"X-Slack-Signature", List.of(validSlackSignature),
"X-Slack-Request-Timestamp", List.of(timestamp));

when(request.getHeaders()).thenReturn(validHeaders);

// Reset knowledge graph API key
Whitebox.setInternalState(SlackSlashCommand.class, "API_KEY", System.getenv("KG_API_KEY"));
when(request.getFirstHeader(any())).thenCallRealMethod();
}

@Test
Expand Down Expand Up @@ -126,20 +115,19 @@ public void recognizesValidSlackTokenTest() throws IOException, GeneralSecurityE
verify(response, times(1)).setStatusCode(HttpURLConnection.HTTP_BAD_REQUEST);
}

@Test(expected = GoogleJsonResponseException.class)
@Test
public void handlesSearchErrorTest() throws IOException, GeneralSecurityException {
String jsonStr = gson.toJson(Map.of("text", "foo"));
StringReader requestReadable = new StringReader(jsonStr);

when(request.getReader()).thenReturn(new BufferedReader(requestReadable));
when(request.getMethod()).thenReturn("POST");

SlackSlashCommand functionInstance = new SlackSlashCommand();
Whitebox.setInternalState(functionInstance, "verifier", alwaysValidVerifier);
Whitebox.setInternalState(SlackSlashCommand.class, "API_KEY", "gibberish");
SlackSlashCommand functionInstance = new SlackSlashCommand(alwaysValidVerifier, "gibberish");

// Should throw a GoogleJsonResponseException (due to invalid API key)
functionInstance.service(request, response);
assertThrows(
GoogleJsonResponseException.class, () -> functionInstance.service(request, response));
}

@Test
Expand All @@ -150,9 +138,7 @@ public void handlesEmptyKgResultsTest() throws IOException, GeneralSecurityExcep
when(request.getReader()).thenReturn(new BufferedReader(requestReadable));
when(request.getMethod()).thenReturn("POST");

SlackSlashCommand functionInstance = new SlackSlashCommand();
Whitebox.setInternalState(functionInstance, "verifier", alwaysValidVerifier);

SlackSlashCommand functionInstance = new SlackSlashCommand(alwaysValidVerifier);

functionInstance.service(request, response);

Expand All @@ -168,9 +154,7 @@ public void handlesPopulatedKgResultsTest() throws IOException, GeneralSecurityE
when(request.getReader()).thenReturn(new BufferedReader(requestReadable));
when(request.getMethod()).thenReturn("POST");

SlackSlashCommand functionInstance = new SlackSlashCommand();
Whitebox.setInternalState(functionInstance, "verifier", alwaysValidVerifier);

SlackSlashCommand functionInstance = new SlackSlashCommand(alwaysValidVerifier);

functionInstance.service(request, response);

Expand Down