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

add metadata parameter to CodeArtifact api #109

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

add metadata parameter to CodeArtifact api #109

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c532d3e
Add metadata parameter to CodeArtifact api
seschis Nov 4, 2021
1e28f20
fix formatting and interface from PR comments
seschis Nov 17, 2021
f376c92
formatting
seschis Nov 17, 2021
08db808
fix stream pattern in test
seschis Nov 17, 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
5 changes: 5 additions & 0 deletions src/main/java/com/contrastsecurity/sdk/scan/CodeArtifact.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
* #L%
*/

import com.contrastsecurity.sdk.internal.Nullable;
import java.time.Instant;

/**
Expand All @@ -40,6 +41,10 @@ public interface CodeArtifact {
/** @return filename */
String filename();

@Nullable
/** @return metadata filename */
seschis marked this conversation as resolved.
Show resolved Hide resolved
String metadata();

/** @return time at which the code artifact was uploaded to Contrast Scan */
Instant createdTime();
}
4 changes: 3 additions & 1 deletion src/main/java/com/contrastsecurity/sdk/scan/CodeArtifactClient.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,14 @@ interface CodeArtifactClient {
*
* @param projectId ID of the project to which the code artifact belongs
* @param file the file to upload
* @param metadata the prescan metadata to upload with the file artifact. Null may be given if
* prescan data is not present.
* @return new {@link CodeArtifactInner} from Contrast API
* @throws IOException when an IO error occurs while making the request to the Contrast API
* @throws UnauthorizedException when Contrast rejects the credentials used to send the request
* @throws ResourceNotFoundException when the requested resource does not exist
* @throws HttpResponseException when Contrast rejects this request with an error code
* @throws ServerResponseException when Contrast API returns a response that cannot be understood
*/
CodeArtifactInner upload(String projectId, Path file) throws IOException;
CodeArtifactInner upload(String projectId, Path file, Path metadata) throws IOException;
seschis marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

This API asks that the user provide the metadata JSON as a file. Maybe passing the metadata as a file is the only use case we have now (i.e. the Maven plugin), but it doesn't feel like the most flexible assumption for the SDK. I would expect the SDK to accept the metadata as a Java object that describes the data structure, and I would expect the SDK to marshal that object to JSON for me.

Are the fields in the metadata JSON object well known, or is this more of an open-ended bag of key-value pairs? If it's the former, then we should define a new class for it. If it's the latter, then maybe all we need is a Map.

Copy link
Author

@seschis seschis Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

the format of the data is defined here: https://github.com/Contrast-Security-OSS/contrast-scan-prescan/blob/master/src/main/resources/schema/scan-input-metadata-schema-1.0.0.json

I don't anticipate it changing from json, but I can't say it never will. I also don't know how the content of the data will be required to change over time. The main purpose of the prescan metadata is to allow the engine to generate physical absolute paths to files in its sarif report which GitHub uses as part of preview renderings when displaying the sarif findings.
It also allows local sarif viewers, like the one in VSCode to naturally find the right file locally and show code annotated with the data flow path in the editor directly.

Copy link
Contributor

@gilday gilday Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I understand. Knowing that has a well-defined schema, I feel more strongly that we should make a corresponding Java class to hold this data, but I don't fully understand what expectation you have for users who wan to use this API. How do they know how to generate this file?

}
43 changes: 36 additions & 7 deletions src/main/java/com/contrastsecurity/sdk/scan/CodeArtifactClientImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,8 @@ final class CodeArtifactClientImpl implements CodeArtifactClient {
}

@Override
public CodeArtifactInner upload(final String projectId, final Path file) throws IOException {
public CodeArtifactInner upload(final String projectId, final Path file, final Path metadata)
throws IOException {
final String uri =
contrast.getRestApiURL()
+ new URIBuilder()
Expand All @@ -66,9 +67,9 @@ public CodeArtifactInner upload(final String projectId, final Path file) throws
"code-artifacts")
.toURIString();
final String boundary = "ContrastFormBoundary" + ThreadLocalRandom.current().nextLong();
final String header =
"--"
+ boundary
final String boundaryMarker = CRLF + "--" + boundary;
final String filenameSection =
boundaryMarker
+ CRLF
+ "Content-Disposition: form-data; name=\"filename\"; filename=\""
+ file.getFileName().toString()
Expand All @@ -80,8 +81,31 @@ public CodeArtifactInner upload(final String projectId, final Path file) throws
+ "Content-Transfer-Encoding: binary"
+ CRLF
+ CRLF;
final String footer = CRLF + "--" + boundary + "--" + CRLF;
final long contentLength = header.length() + Files.size(file) + footer.length();
final String metadataSection;
if (metadata != null) {
seschis marked this conversation as resolved.
Show resolved Hide resolved
metadataSection =
boundaryMarker
+ CRLF
+ "Content-Disposition: form-data; name=\"metadata\"; filename=\""
+ metadata.getFileName().toString()
+ '"'
+ CRLF
+ "Content-Type: "
+ determineMime(metadata)
+ CRLF
+ "Content-Transfer-Encoding: binary"
+ CRLF
+ CRLF;
} else {
metadataSection = "";
}

final String footer = boundaryMarker + "--" + CRLF;
long contentLength = filenameSection.length() + Files.size(file);
if (metadata != null) {
contentLength += metadataSection.length() + Files.size(metadata);
}
contentLength += footer.length();

final HttpURLConnection connection = contrast.makeConnection(uri, "POST");
connection.setDoOutput(true);
Expand All @@ -91,9 +115,14 @@ public CodeArtifactInner upload(final String projectId, final Path file) throws
try (OutputStream os = connection.getOutputStream();
PrintWriter writer =
new PrintWriter(new OutputStreamWriter(os, StandardCharsets.US_ASCII), true)) {
writer.append(header).flush();
writer.append(filenameSection).flush();
Files.copy(file, os);
os.flush();
if (metadata != null) {
writer.append(metadataSection).flush();
Files.copy(metadata, os);
os.flush();
}
writer.append(footer).flush();
}
final int code = connection.getResponseCode();
Expand Down
7 changes: 7 additions & 0 deletions src/main/java/com/contrastsecurity/sdk/scan/CodeArtifactImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
* #L%
*/

import com.contrastsecurity.sdk.internal.Nullable;
import java.time.Instant;
import java.util.Objects;

Expand Down Expand Up @@ -52,6 +53,12 @@ public String filename() {
return inner.filename();
}

@Override
@Nullable
gilday marked this conversation as resolved.
Show resolved Hide resolved
public String metadata() {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the metadata is a String, does that imply that the SDK user is not intended to parse this structured data; rather, they should treat it as an opaque box? Specifically, is this JSON-encoded JSON inside this JSON metadata property and it's not meant to be decoded?

Copy link
Author

@seschis seschis Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

hmm, I always considered the generation of prescan data something that was external to the SDK.... but it probably would be more user-friendly to allow the SDK user to call a function that generated prescan data for them.... or perhaps just do it transparently.
In either case, it seems like an opaque thing to the SDK user.

return inner.metadata();
}

@Override
public Instant createdTime() {
return inner.createdTime();
Expand Down
8 changes: 8 additions & 0 deletions src/main/java/com/contrastsecurity/sdk/scan/CodeArtifactInner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
* #L%
*/

import com.contrastsecurity.sdk.internal.Nullable;
import com.google.auto.value.AutoValue;
import java.time.Instant;

Expand All @@ -44,6 +45,10 @@ static Builder builder() {
/** @return filename */
abstract String filename();

@Nullable
/** @return metadata filename */
seschis marked this conversation as resolved.
Show resolved Hide resolved
abstract String metadata();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this imply that the user can retrieve the metadata filename, but not the metadata? What would the user do with the filename?


/** @return time at which the code artifact was uploaded to Contrast Scan */
abstract Instant createdTime();

Expand All @@ -63,6 +68,9 @@ abstract static class Builder {
/** @see CodeArtifactInner#filename() */
abstract Builder filename(String value);

/** @see CodeArtifactInner#metadata() */
abstract Builder metadata(String value);

/** @see CodeArtifactInner#createdTime() */
abstract Builder createdTime(Instant value);

Expand Down
3 changes: 3 additions & 0 deletions src/main/java/com/contrastsecurity/sdk/scan/CodeArtifacts.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ interface Factory {
*/
CodeArtifact upload(Path file, String name) throws IOException;

CodeArtifact upload(Path file, String name, Path metadata, String metaname) throws IOException;
Copy link
Contributor

Choose a reason for hiding this comment

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

What is metaname here? I don't think it's used.

Copy link
Author

@seschis seschis Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

metaname is analagous to the name argument the way metadata is analogous to the file argument. As to if that is even needed, I'm not sure. I don't see why we'd give the user the capability to change the "filename" on the multipart upload... but I figured it was something I didn't understand so I just mirrored the behavior when allowing the prescan metadata to be added to the multipart file upload. It's your call if you want me to take it out as I don't have a valid argument for or against it.

Copy link
Contributor

Choose a reason for hiding this comment

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

iirc the filename of the upload is reflected in the UI, and that's why we give the user the capability to set that. I don't see a use case for allowing the user to set the name of the metadata file. I'd argue we take that capability out. Also, I'm still questioning whether the metadata should be sent as a file vs as JSON, but I'll continue that discussion in another thread.

Copy link
Author

Choose a reason for hiding this comment

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

ok, I see now. I can take it out. Before I do any work on it yet though I'll wait until we resolve the fundamental API design questions you have in this area.

/**
* Transfers a file from the file system to Contrast Scan to create a new code artifact for static
* analysis.
Expand All @@ -75,4 +76,6 @@ interface Factory {
* @throws ServerResponseException when Contrast API returns a response that cannot be understood
*/
CodeArtifact upload(Path file) throws IOException;

CodeArtifact upload(Path file, Path metadata) throws IOException;
seschis marked this conversation as resolved.
Show resolved Hide resolved
}
15 changes: 14 additions & 1 deletion src/main/java/com/contrastsecurity/sdk/scan/CodeArtifactsImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,27 @@ public CodeArtifacts create(final String projectId) {
this.projectId = projectId;
}

@Override
public CodeArtifact upload(
final Path file, final String name, final Path metadata, final String metaname)
throws IOException {
final CodeArtifactInner inner = client.upload(projectId, file, metadata);
return new CodeArtifactImpl(inner);
}

@Override
public CodeArtifact upload(final Path file, final String name) throws IOException {
final CodeArtifactInner inner = client.upload(projectId, file);
final CodeArtifactInner inner = client.upload(projectId, file, null);
return new CodeArtifactImpl(inner);
}

@Override
public CodeArtifact upload(final Path file) throws IOException {
return upload(file, file.getFileName().toString());
}

@Override
public CodeArtifact upload(final Path file, final Path metadata) throws IOException {
return upload(file, file.getFileName().toString(), metadata, metadata.getFileName().toString());
}
}
1 change: 1 addition & 0 deletions src/test/java/com/contrastsecurity/sdk/scan/CodeArtifactAssert.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ public CodeArtifactAssert hasSameValuesAsInner(final CodeArtifactInner inner) {
Assertions.assertThat(actual.projectId()).isEqualTo(inner.projectId());
Assertions.assertThat(actual.organizationId()).isEqualTo(inner.organizationId());
Assertions.assertThat(actual.filename()).isEqualTo(inner.filename());
Assertions.assertThat(actual.metadata()).isEqualTo(inner.metadata());
Assertions.assertThat(actual.createdTime()).isEqualTo(inner.createdTime());
return this;
}
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/com/contrastsecurity/sdk/scan/CodeArtifactsImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ void upload(@TempDir final Path tmp) throws IOException {
final CodeArtifactClient client = mock(CodeArtifactClient.class);
final CodeArtifactInner inner = builder().build();
final Path file = tmp.resolve(inner.filename());
when(client.upload(inner.projectId(), file)).thenReturn(inner);
when(client.upload(inner.projectId(), file, null)).thenReturn(inner);

// WHEN upload file
final CodeArtifacts codeArtifacts = new CodeArtifactsImpl(client, inner.projectId());
Expand All @@ -59,7 +59,7 @@ void upload_custom_filename(@TempDir final Path tmp) throws IOException {
final CodeArtifactClient client = mock(CodeArtifactClient.class);
final CodeArtifactInner inner = builder().build();
final Path file = tmp.resolve("other-file.jar");
when(client.upload(inner.projectId(), file)).thenReturn(inner);
when(client.upload(inner.projectId(), file, null)).thenReturn(inner);

// WHEN upload file
final CodeArtifacts codeArtifacts = new CodeArtifactsImpl(client, inner.projectId());
Expand Down
88 changes: 86 additions & 2 deletions src/test/java/com/contrastsecurity/sdk/scan/CodeArtifactsPactTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,14 @@
import com.google.gson.Gson;
import java.io.FileOutputStream;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.util.HashMap;
import java.util.jar.JarOutputStream;
import java.util.zip.ZipEntry;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
Expand All @@ -53,6 +55,7 @@
final class CodeArtifactsPactTest {

private Path jar;
private Path metadataJson;

/**
* Creates a test jar for the test to upload as a code artifact
Expand All @@ -65,6 +68,10 @@ void before(@TempDir final Path tmp) throws IOException {
try (JarOutputStream jos = new JarOutputStream(new FileOutputStream(jar.toFile()))) {
jos.putNextEntry(new ZipEntry("HelloWorld.class"));
}
metadataJson = tmp.resolve("prescan.json");
try (FileOutputStream fos = new FileOutputStream(metadataJson.toFile())) {
seschis marked this conversation as resolved.
Show resolved Hide resolved
fos.write("{\"test\": \"data\" }".getBytes(StandardCharsets.UTF_8));
}
}

/** Verifies the code artifact upload behavior. */
Expand All @@ -78,7 +85,7 @@ RequestResponsePact pact(final PactDslWithProvider builder) throws IOException {
params.put("organizationId", "organization-id");
return builder
.given("Projects Exist", params)
.uponReceiving("upload new code artifact")
.uponReceiving("upload new code artifact with metadata")
seschis marked this conversation as resolved.
Show resolved Hide resolved
.method("POST")
.pathFromProviderState(
"/sast/organizations/${organizationId}/projects/${projectId}/code-artifacts",
Expand Down Expand Up @@ -115,14 +122,91 @@ void upload_code_artifact(final MockServer server) throws IOException {
.build();
final Gson gson = GsonFactory.create();
CodeArtifactClient client = new CodeArtifactClientImpl(contrast, gson, "organization-id");
final CodeArtifactInner codeArtifact = client.upload("project-id", jar);
final CodeArtifactInner codeArtifact = client.upload("project-id", jar, null);

final CodeArtifactInner expected =
CodeArtifactInner.builder()
.id("code-artifact-id")
.projectId("project-id")
.organizationId("organization-id")
.filename(jar.getFileName().toString())
.createdTime(TestDataConstants.TIMESTAMP_EXAMPLE)
.build();
assertThat(codeArtifact).isEqualTo(expected);
}
}
/** Verifies the code artifact upload with metadata behavior. */
@Nested
final class UploadCodeArtifactWithMetadata {

@Disabled("https://github.com/pact-foundation/pact-jvm/issues/668")
@Pact(consumer = "contrast-sdk")
RequestResponsePact pact(final PactDslWithProvider builder) throws IOException {

final HashMap<String, Object> params = new HashMap<>();
params.put("id", "project-id");
params.put("organizationId", "organization-id");
return builder
.given("Projects Exist", params)
.uponReceiving("upload new code artifact")
.method("POST")
.pathFromProviderState(
"/sast/organizations/${organizationId}/projects/${projectId}/code-artifacts",
"/sast/organizations/organization-id/projects/project-id/code-artifacts")
.withFileUpload(
"filename",
jar.getFileName().toString(),
"application/java-archive",
Files.readAllBytes(jar))
// BUG: https://github.com/pact-foundation/pact-jvm/issues/668. Unable to define a PACT
// request matcher that
// has multiple multipart sections.
// Consumer interface definition is:
// https://github.com/Contrast-Security-Inc/sast-api-documentation/blob/master/sast-code-artifacts.yaml#L83
.withFileUpload(
"metadata",
metadataJson.getFileName().toString(),
"application/json",
Files.readAllBytes(metadataJson))
.willRespondWith()
.status(201)
.body(
newJsonBody(
o -> {
o.stringType("id", "code-artifact-id");
o.valueFromProviderState("projectId", "${projectId}", "project-id");
o.valueFromProviderState(
"organizationId", "${organizationId}", "organization-id");
o.stringType("filename", jar.getFileName().toString());
o.stringType("metadata", metadataJson.getFileName().toString());
o.datetime(
"createdTime",
PactConstants.DATETIME_FORMAT,
TestDataConstants.TIMESTAMP_EXAMPLE);
})
.build())
.toPact();
}

@Disabled("https://github.com/pact-foundation/pact-jvm/issues/668")
@Test
void upload_code_artifact_with_metadata(final MockServer server) throws IOException {
System.out.println("running metadata test");
seschis marked this conversation as resolved.
Show resolved Hide resolved
final ContrastSDK contrast =
new ContrastSDK.Builder("test-user", "test-service-key", "test-api-key")
.withApiUrl(server.getUrl())
.build();
final Gson gson = GsonFactory.create();
CodeArtifactClient client = new CodeArtifactClientImpl(contrast, gson, "organization-id");
final CodeArtifactInner codeArtifact = client.upload("project-id", jar, metadataJson);

final CodeArtifactInner expected =
CodeArtifactInner.builder()
.id("code-artifact-id")
.projectId("project-id")
.organizationId("organization-id")
.filename(jar.getFileName().toString())
.metadata(metadataJson.getFileName().toString())
.createdTime(TestDataConstants.TIMESTAMP_EXAMPLE)
.build();
assertThat(codeArtifact).isEqualTo(expected);
Expand Down