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

[Feature-2188] AuditMessage validation in tests should be more useful / removed #3596

Merged
merged 10 commits into from
Nov 16, 2023
Merged

[Feature-2188] AuditMessage validation in tests should be more useful / removed #3596

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1d32e96
finished ticket
prabhask5 Oct 25, 2023
4c28b59
prettier changes + typo fixes
prabhask5 Oct 25, 2023
7a1f7e8
Revert "prettier changes + typo fixes"
prabhask5 Oct 26, 2023
016759d
added specific exceptions
prabhask5 Oct 26, 2023
c43c7e3
made more specific exceptions
prabhask5 Oct 27, 2023
7f83488
removed unecessary try catch
prabhask5 Oct 28, 2023
b53d3ec
Merge remote-tracking branch 'upstream/main' into pk-fix-auditvalidat…
prabhask5 Nov 3, 2023
3850e1c
fix test errors
prabhask5 Nov 9, 2023
be57002
fix spotlessApply errors
prabhask5 Nov 14, 2023
2c5d5f6
merge main
prabhask5 Nov 14, 2023
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
30 changes: 12 additions & 18 deletions src/test/java/org/opensearch/security/auditlog/AbstractAuditlogiUnitTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,34 +84,28 @@ protected void setupStarfleetIndex() {
rh.keystore = keystore;
}

protected boolean validateMsgs(final Collection<AuditMessage> msgs) {
boolean valid = true;
protected void validateMsgs(final Collection<AuditMessage> msgs) throws Exception {
for (AuditMessage msg : msgs) {
valid = validateMsg(msg) && valid;
validateMsg(msg);
DarshitChanpura marked this conversation as resolved.
Show resolved Hide resolved
}
return valid;
}

protected boolean validateMsg(final AuditMessage msg) {
return validateJson(msg.toJson()) && validateJson(msg.toPrettyString());
}

protected boolean validateJson(final String json) {
protected void validateMsg(final AuditMessage msg) throws Exception {
validateJson(msg.toJson());
validateJson(msg.toPrettyString());
prabhask5 marked this conversation as resolved.
Show resolved Hide resolved
}

protected void validateJson(final String json) throws Exception { // this function can throw either IllegalArgumentException,
// JsonMappingException
if (json == null || json.isEmpty()) {
return false;
throw new IllegalArgumentException("json is either null or empty");
}

try {
JsonNode node = DefaultObjectMapper.objectMapper.readTree(json);

if (node.get("audit_request_body") != null) {
DefaultObjectMapper.objectMapper.readTree(node.get("audit_request_body").asText());
}
JsonNode node = DefaultObjectMapper.objectMapper.readTree(json);

return true;
} catch (Exception e) {
return false;
if (node.get("audit_request_body") != null) {
DefaultObjectMapper.objectMapper.readTree(node.get("audit_request_body").asText());
}
}

Expand Down
10 changes: 5 additions & 5 deletions src/test/java/org/opensearch/security/auditlog/compliance/ComplianceAuditlogTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public void testSourceFilter() throws Exception {
assertThat(message.getRequestBody(), not(containsString("Salary")));
assertThat(message.getRequestBody(), containsString("Gender"));

Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
}

@Test
Expand Down Expand Up @@ -245,7 +245,7 @@ public void testSourceFilterMsearch() throws Exception {
assertThat(genderMsg.getRequestBody(), containsString("Gender"));
assertThat(genderMsg.getRequestBody(), not(containsString("Salary")));

Assert.assertTrue(validateMsgs(messages));
validateMsgs(messages);
}

@Test
Expand Down Expand Up @@ -302,7 +302,7 @@ public void testInternalConfig() throws Exception {
);
});

Assert.assertTrue(validateMsgs(messages));
validateMsgs(messages);
prabhask5 marked this conversation as resolved.
Show resolved Hide resolved
}

@Test
Expand Down Expand Up @@ -346,7 +346,7 @@ public void testExternalConfig() throws Exception {
assertThat(messages.get(1).getNodeId(), not(equalTo(messages.get(2).getNodeId())));
assertThat(messages.get(2).getNodeId(), not(equalTo(messages.get(3).getNodeId())));

Assert.assertTrue(validateMsgs(messages));
validateMsgs(messages);
}

@Test
Expand Down Expand Up @@ -399,7 +399,7 @@ public void testUpdate() throws Exception {
assertThat(ex2.getMissingCount(), equalTo(1));

Assert.assertTrue(TestAuditlogImpl.messages.isEmpty());
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
}

@Test
Expand Down
10 changes: 5 additions & 5 deletions src/test/java/org/opensearch/security/auditlog/compliance/RestApiComplianceAuditlogTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public void testRestApiRolesEnabled() throws Exception {
Assert.assertFalse(TestAuditlogImpl.sb.toString().contains("COMPLIANCE_INTERNAL_CONFIG_READ"));
Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("COMPLIANCE_INTERNAL_CONFIG_WRITE"));
Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("UPDATE"));
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
}

@Test
Expand Down Expand Up @@ -89,7 +89,7 @@ public void testRestApiRolesDisabled() throws Exception {
Assert.assertFalse(TestAuditlogImpl.sb.toString().contains("COMPLIANCE_INTERNAL_CONFIG_READ"));
Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("COMPLIANCE_INTERNAL_CONFIG_WRITE"));
Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("UPDATE"));
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
}

@Test
Expand Down Expand Up @@ -122,7 +122,7 @@ public void testRestApiRolesDisabledGet() throws Exception {
Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("COMPLIANCE_INTERNAL_CONFIG_READ"));
Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("COMPLIANCE_INTERNAL_CONFIG_WRITE"));
Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("UPDATE"));
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
}

@Test
Expand All @@ -147,7 +147,7 @@ public void testAutoInit() throws Exception {
Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("audit_request_effective_user"));
Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("COMPLIANCE_INTERNAL_CONFIG_WRITE"));
Assert.assertTrue(TestAuditlogImpl.sb.toString().contains("COMPLIANCE_EXTERNAL_CONFIG"));
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
}

@Test
Expand Down Expand Up @@ -208,7 +208,7 @@ public void testRestInternalConfigRead() throws Exception {
Assert.assertTrue(auditLogImpl.contains("COMPLIANCE_INTERNAL_CONFIG_READ"));
Assert.assertFalse(auditLogImpl.contains("COMPLIANCE_INTERNAL_CONFIG_WRITE"));
Assert.assertFalse(auditLogImpl.contains("UPDATE"));
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
}

@Test
Expand Down
38 changes: 19 additions & 19 deletions src/test/java/org/opensearch/security/auditlog/integration/BasicAuditlogTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ public void testSSLPlainText() throws Exception {
Assert.assertEquals(AuditCategory.SSL_EXCEPTION, message.getCategory());
Assert.assertTrue(message.getExceptionStackTrace().contains("not an SSL/TLS record"));
});
Assert.assertTrue(validateMsgs(messages));
validateMsgs(messages);
prabhask5 marked this conversation as resolved.
Show resolved Hide resolved
}

@Test
Expand Down Expand Up @@ -182,7 +182,7 @@ public void testTaskId() throws Exception {
TestAuditlogImpl.messages.get(1).getAsMap().get(AuditMessage.TASK_ID),
TestAuditlogImpl.messages.get(1).getAsMap().get(AuditMessage.TASK_ID)
);
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
}

@Test
Expand Down Expand Up @@ -211,7 +211,7 @@ public void testDefaultsRest() throws Exception {
Assert.assertTrue(auditLogImpl.contains("\"audit_request_effective_user\" : \"admin\""));
Assert.assertTrue(auditLogImpl.contains("REST"));
Assert.assertFalse(auditLogImpl.toLowerCase().contains("authorization"));
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
}

@Test
Expand Down Expand Up @@ -317,7 +317,7 @@ public void testWrongUser() throws Exception {
Assert.assertTrue(TestAuditlogImpl.sb.toString(), TestAuditlogImpl.sb.toString().contains(AuditMessage.UTC_TIMESTAMP));
Assert.assertFalse(TestAuditlogImpl.sb.toString(), TestAuditlogImpl.sb.toString().contains("AUTHENTICATED"));
Assert.assertEquals(1, TestAuditlogImpl.messages.size());
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
}

public void testUnknownAuthorization() throws Exception {
Expand All @@ -329,7 +329,7 @@ public void testUnknownAuthorization() throws Exception {
Assert.assertTrue(TestAuditlogImpl.sb.toString().contains(AuditMessage.UTC_TIMESTAMP));
Assert.assertFalse(TestAuditlogImpl.sb.toString().contains("AUTHENTICATED"));
Assert.assertEquals(1, TestAuditlogImpl.messages.size());
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
}

public void testUnauthenticated() throws Exception {
Expand All @@ -345,15 +345,15 @@ public void testUnauthenticated() throws Exception {
Assert.assertTrue(auditLogImpl.contains("/_search"));
Assert.assertTrue(auditLogImpl.contains(AuditMessage.UTC_TIMESTAMP));
Assert.assertFalse(auditLogImpl.contains("AUTHENTICATED"));
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);

}

public void testJustAuthenticated() throws Exception {
HttpResponse response = rh.executeGetRequest("", encodeBasicHeader("admin", "admin"));
Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode());
Assert.assertEquals(0, TestAuditlogImpl.messages.size());
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
}

public void testSecurityIndexAttempt() throws Exception {
Expand All @@ -366,7 +366,7 @@ public void testSecurityIndexAttempt() throws Exception {
Assert.assertTrue(TestAuditlogImpl.sb.toString().contains(AuditMessage.UTC_TIMESTAMP));
Assert.assertFalse(TestAuditlogImpl.sb.toString().contains("AUTHENTICATED"));
Assert.assertEquals(2, TestAuditlogImpl.messages.size());
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
}

public void testBadHeader() throws Exception {
Expand All @@ -381,7 +381,7 @@ public void testBadHeader() throws Exception {
Assert.assertTrue(TestAuditlogImpl.sb.toString(), TestAuditlogImpl.sb.toString().contains("BAD_HEADERS"));
Assert.assertTrue(TestAuditlogImpl.sb.toString(), TestAuditlogImpl.sb.toString().contains("_opendistro_security_bad"));
Assert.assertEquals(TestAuditlogImpl.sb.toString(), 1, TestAuditlogImpl.messages.size());
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
}

public void testMissingPriv() throws Exception {
Expand All @@ -395,7 +395,7 @@ public void testMissingPriv() throws Exception {
Assert.assertTrue(TestAuditlogImpl.sb.toString().contains(AuditMessage.UTC_TIMESTAMP));
Assert.assertFalse(TestAuditlogImpl.sb.toString().contains("AUTHENTICATED"));
Assert.assertEquals(1, TestAuditlogImpl.messages.size());
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
}

public void testMsearch() throws Exception {
Expand All @@ -419,7 +419,7 @@ public void testMsearch() throws Exception {
Assert.assertTrue(auditLogImpl.contains("audit_trace_task_id"));
Assert.assertEquals(auditLogImpl, 4, TestAuditlogImpl.messages.size());
Assert.assertFalse(auditLogImpl.toLowerCase().contains("authorization"));
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
}

public void testBulkAuth() throws Exception {
Expand Down Expand Up @@ -458,7 +458,7 @@ public void testBulkAuth() throws Exception {
Assert.assertTrue(auditLogImpl.contains("audit_trace_task_id"));
// may vary because we log shardrequests which are not predictable here
Assert.assertTrue(TestAuditlogImpl.messages.size() >= 17);
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
}

public void testBulkNonAuth() throws Exception {
Expand Down Expand Up @@ -496,7 +496,7 @@ public void testBulkNonAuth() throws Exception {
Assert.assertTrue(auditLogImpl.contains("IndexRequest"));
// may vary because we log shardrequests which are not predictable here
Assert.assertTrue(TestAuditlogImpl.messages.size() >= 7);
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
}

public void testUpdateSettings() throws Exception {
Expand All @@ -521,7 +521,7 @@ public void testUpdateSettings() throws Exception {
Assert.assertTrue(auditLogImpl.contains(expectedRequestBodyLog));
// may vary because we log may hit cluster manager directly or not
Assert.assertTrue(TestAuditlogImpl.messages.size() > 1);
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
}

@Test
Expand Down Expand Up @@ -620,7 +620,7 @@ public void testAliases() throws Exception {
Assert.assertTrue(auditLogImpl.contains("starfleet"));
Assert.assertTrue(auditLogImpl.contains("sf"));
Assert.assertEquals(2, TestAuditlogImpl.messages.size());
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
}

@Test
Expand Down Expand Up @@ -682,7 +682,7 @@ public void testScroll() throws Exception {
Assert.assertTrue(auditLogImpl.contains("InternalScrollSearchRequest"));
Assert.assertTrue(auditLogImpl.contains("MISSING_PRIVILEGES"));
Assert.assertTrue(TestAuditlogImpl.messages.size() > 2);
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
}

@Test
Expand Down Expand Up @@ -718,7 +718,7 @@ public void testAliasResolution() throws Exception {
Assert.assertTrue(auditLogImpl.contains("audit_trace_resolved_indices"));
Assert.assertTrue(auditLogImpl.contains("vulcangov"));
Assert.assertEquals(1, TestAuditlogImpl.messages.size());
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
TestAuditlogImpl.clear();
}

Expand Down Expand Up @@ -747,7 +747,7 @@ public void testAliasBadHeaders() throws Exception {
Assert.assertTrue(auditLogImpl.contains("BAD_HEADERS"));
Assert.assertTrue(auditLogImpl.contains("xxx"));
Assert.assertEquals(1, TestAuditlogImpl.messages.size());
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
TestAuditlogImpl.clear();
}

Expand Down Expand Up @@ -780,7 +780,7 @@ public void testIndexCloseDelete() throws Exception {
Assert.assertTrue(auditLogImpl.contains("indices:admin/close"));
Assert.assertTrue(auditLogImpl.contains("indices:admin/delete"));
Assert.assertTrue(auditLogImpl, TestAuditlogImpl.messages.size() >= 2);
Assert.assertTrue(validateMsgs(TestAuditlogImpl.messages));
validateMsgs(TestAuditlogImpl.messages);
}

@Test
Expand Down