Skip to content

Commit

Permalink
Ensure compliance configuration test results are stable
Browse files Browse the repository at this point in the history 
There are some cases that use datetime that were causing code coverage
flutuations depending on when the tests are run, fixed this by adding a
date provider and new unit tests.

- Related opensearch-project#3137

Signed-off-by: Peter Nied <[email protected]>
  • Loading branch information
@peternied
peternied committed Jan 16, 2024
1 parent d734b2e commit efbc58d
Show file tree
Hide file tree
Showing 2 changed files with 127 additions and 6 deletions.
37 changes: 31 additions & 6 deletions src/main/java/org/opensearch/security/compliance/ComplianceConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.ExecutionException;
import java.util.function.Supplier;

import com.google.common.annotations.VisibleForTesting;
import com.google.common.cache.CacheBuilder;
Expand Down Expand Up @@ -73,11 +75,11 @@
@JsonInclude(JsonInclude.Include.NON_NULL)
public class ComplianceConfig {

public static Set<String> FIELDS = DefaultObjectMapper.getFields(ComplianceConfig.class);
private static final Logger log = LogManager.getLogger(ComplianceConfig.class);
public static final ComplianceConfig DEFAULT = ComplianceConfig.from(Settings.EMPTY);
private static final int CACHE_SIZE = 1000;
private static final String INTERNAL_OPENSEARCH = "internal_opensearch";
public static Set<String> FIELDS = DefaultObjectMapper.getFields(ComplianceConfig.class);

private final boolean logExternalConfig;
private final boolean logInternalConfig;
Expand All @@ -104,6 +106,7 @@ public class ComplianceConfig {
private final DateTimeFormatter auditLogPattern;
private final String auditLogIndex;
private final boolean enabled;
private final Supplier<DateTime> dateProvider;

private ComplianceConfig(
final boolean enabled,
Expand All @@ -118,7 +121,8 @@ private ComplianceConfig(
final Set<String> ignoredComplianceUsersForWrite,
final String securityIndex,
final String destinationType,
final String destinationIndex
final String destinationIndex,
final Supplier<DateTime> dateProvider
) {
this.enabled = enabled;
this.logExternalConfig = logExternalConfig;
Expand Down Expand Up @@ -148,6 +152,11 @@ private ComplianceConfig(
try {
auditLogPattern = DateTimeFormat.forPattern(destinationIndex); // throws IllegalArgumentException if no pattern
} catch (IllegalArgumentException e) {
log.warn(
"Unable to translate {} as a DateTimeFormat, will instead treat this as a static audit log index name. Error: {}",
destinationIndex,
e.getMessage()
);
// no pattern
auditLogIndex = destinationIndex;
} catch (Exception e) {
Expand All @@ -163,6 +172,8 @@ public WildcardMatcher load(String index) throws Exception {
return WildcardMatcher.from(getFieldsForIndex(index));
}
});

this.dateProvider = Optional.ofNullable(dateProvider).orElse(() -> DateTime.now(DateTimeZone.UTC));
}

@VisibleForTesting
Expand All @@ -177,6 +188,7 @@ public ComplianceConfig(
final boolean logDiffsForWrite,
final List<String> watchedWriteIndicesPatterns,
final Set<String> ignoredComplianceUsersForWrite,
final Supplier<DateTime> dateProvider,
Settings settings
) {
this(
Expand All @@ -195,7 +207,8 @@ public ComplianceConfig(
settings.get(
ConfigConstants.SECURITY_AUDIT_CONFIG_DEFAULT_PREFIX + ConfigConstants.SECURITY_AUDIT_OPENSEARCH_INDEX,
"'security-auditlog-'YYYY.MM.dd"
)
),
dateProvider
);
}

Expand Down Expand Up @@ -253,6 +266,7 @@ public static ComplianceConfig from(Map<String, Object> properties, @JacksonInje
logDiffsForWrite,
watchedWriteIndicesPatterns,
ignoredComplianceUsersForWrite,
null,
settings
);
}
Expand All @@ -263,6 +277,16 @@ public static ComplianceConfig from(Map<String, Object> properties, @JacksonInje
* @return compliance configuration
*/
public static ComplianceConfig from(Settings settings) {
return ComplianceConfig.from(settings, null);
}

/**
* Create compliance configuration from Settings defined in opensearch.yml
* @param settings settings
* @param dateProvider how the current date/time is evalated for audit logs that rollover
* @return compliance configuration
*/
public static ComplianceConfig from(Settings settings, Supplier<DateTime> dateProvider) {
final boolean logExternalConfig = settings.getAsBoolean(
ConfigConstants.OPENDISTRO_SECURITY_COMPLIANCE_HISTORY_EXTERNAL_CONFIG_ENABLED,
false
Expand Down Expand Up @@ -326,6 +350,7 @@ public static ComplianceConfig from(Settings settings) {
logDiffsForWrite,
watchedWriteIndices,
ignoredComplianceUsersForWrite,
dateProvider,
settings
);
}
Expand Down Expand Up @@ -469,7 +494,7 @@ private String getExpandedIndexName(DateTimeFormatter indexPattern, String index
if (indexPattern == null) {
return index;
}
return indexPattern.print(DateTime.now(DateTimeZone.UTC));
return indexPattern.print(dateProvider.get());
}

/**
Expand Down Expand Up @@ -507,7 +532,7 @@ public boolean writeHistoryEnabledForIndex(String index) {
* @return true/false
*/
public boolean readHistoryEnabledForIndex(String index) {
if (!this.isEnabled()) {
if (index == null || !this.isEnabled()) {
return false;
}
// if security index (internal index) check if internal config logging is enabled
Expand All @@ -529,7 +554,7 @@ public boolean readHistoryEnabledForIndex(String index) {
* @return true/false
*/
public boolean readHistoryEnabledForField(String index, String field) {
if (!this.isEnabled()) {
if (index == null || !this.isEnabled()) {
return false;
}
// if security index (internal index) check if internal config logging is enabled
Expand Down
96 changes: 96 additions & 0 deletions src/test/java/org/opensearch/security/auditlog/compliance/ComplianceConfigTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,35 @@
package org.opensearch.security.auditlog.compliance;

import java.util.Collections;
import java.util.Map;
import java.util.concurrent.atomic.AtomicReference;
import java.util.function.Consumer;

import com.google.common.collect.ImmutableSet;
import org.apache.logging.log4j.Logger;
import org.junit.Test;

import org.opensearch.common.settings.Settings;
import org.opensearch.security.compliance.ComplianceConfig;
import org.opensearch.security.support.ConfigConstants;
import org.opensearch.security.support.WildcardMatcher;

import org.joda.time.DateTime;
import org.joda.time.DateTimeZone;
import org.mockito.Mockito;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertSame;
import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.isNull;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyNoMoreInteractions;

public class ComplianceConfigTest {

Expand Down Expand Up @@ -136,4 +152,84 @@ public void testEmpty() {
assertSame(WildcardMatcher.NONE, complianceConfig.getIgnoredComplianceUsersForReadMatcher());
assertSame(WildcardMatcher.NONE, complianceConfig.getIgnoredComplianceUsersForWriteMatcher());
}

@Test
public void testLogState() {
// arrange
final var logger = Mockito.mock(Logger.class);
final ComplianceConfig complianceConfig = ComplianceConfig.from(Settings.EMPTY);
// act
complianceConfig.log(logger);
// assert: don't validate content, but ensure message's logged is generally consistant
verify(logger, times(6)).info(anyString(), anyString());
verify(logger, times(1)).info(anyString(), isNull(String.class));
verify(logger, times(1)).info(anyString(), any(Map.class));
verify(logger, times(3)).info(anyString(), any(WildcardMatcher.class));
verifyNoMoreInteractions(logger);
}

@Test
public void testReadWriteHistoryEnabledForIndex_rollingIndex() {
// arrange
final var date = new AtomicReference<DateTime>();
final Consumer<Integer> setYear = (year) -> { date.set(new DateTime(year, 1, 1, 1, 2, DateTimeZone.UTC)); };
final ComplianceConfig complianceConfig = ComplianceConfig.from(
Settings.builder()
.put(
ConfigConstants.SECURITY_AUDIT_CONFIG_DEFAULT_PREFIX + ConfigConstants.SECURITY_AUDIT_OPENSEARCH_INDEX,
"'audit-log-index'-YYYY-MM-dd"
)
.put(ConfigConstants.SECURITY_AUDIT_TYPE_DEFAULT, "internal_opensearch")
.putList(ConfigConstants.OPENDISTRO_SECURITY_COMPLIANCE_HISTORY_READ_WATCHED_FIELDS, "*")
.putList(ConfigConstants.OPENDISTRO_SECURITY_COMPLIANCE_HISTORY_WRITE_WATCHED_INDICES, "*")
.build(),
date::get
);

// act: Don't log for null indices
assertThat(complianceConfig.readHistoryEnabledForIndex(null), equalTo(false));
assertThat(complianceConfig.writeHistoryEnabledForIndex(null), equalTo(false));
// act: Don't log for the security indices
assertThat(complianceConfig.readHistoryEnabledForIndex(complianceConfig.getSecurityIndex()), equalTo(false));
assertThat(complianceConfig.writeHistoryEnabledForIndex(complianceConfig.getSecurityIndex()), equalTo(false));

// act: Don't log for the current audit log
setYear.accept(1337);
assertThat(complianceConfig.readHistoryEnabledForIndex("audit-log-index-1337-01-01"), equalTo(false));
assertThat(complianceConfig.writeHistoryEnabledForIndex("audit-log-index-1337-01-01"), equalTo(false));

// act: Log for current audit log when it does not match the date
setYear.accept(2048);
// See https://github.com/opensearch-project/security/issues/3950
// assertThat(complianceConfig.readHistoryEnabledForIndex("audit-log-index-1337-01-01"), equalTo(true));
assertThat(complianceConfig.writeHistoryEnabledForIndex("audit-log-index-1337-01-01"), equalTo(true));

// act: Log for any matching index
assertThat(complianceConfig.readHistoryEnabledForIndex("my-data"), equalTo(true));
assertThat(complianceConfig.writeHistoryEnabledForIndex("my-data"), equalTo(true));
}

@Test
public void testReadWriteHistoryEnabledForIndex_staticIndex() {
// arrange
final ComplianceConfig complianceConfig = ComplianceConfig.from(
Settings.builder()
.put(
ConfigConstants.SECURITY_AUDIT_CONFIG_DEFAULT_PREFIX + ConfigConstants.SECURITY_AUDIT_OPENSEARCH_INDEX,
"audit-log-index"
)
.put(ConfigConstants.SECURITY_AUDIT_TYPE_DEFAULT, "internal_opensearch")
.putList(ConfigConstants.OPENDISTRO_SECURITY_COMPLIANCE_HISTORY_READ_WATCHED_FIELDS, "*")
.putList(ConfigConstants.OPENDISTRO_SECURITY_COMPLIANCE_HISTORY_WRITE_WATCHED_INDICES, "*")
.build()
);

// act: Don't log for the static audit log
assertThat(complianceConfig.readHistoryEnabledForIndex(complianceConfig.getAuditLogIndex()), equalTo(false));
assertThat(complianceConfig.writeHistoryEnabledForIndex(complianceConfig.getAuditLogIndex()), equalTo(false));

// act: Log for any matching index
assertThat(complianceConfig.readHistoryEnabledForIndex("my-data"), equalTo(true));
assertThat(complianceConfig.writeHistoryEnabledForIndex("my-data"), equalTo(true));
}
}

0 comments on commit efbc58d

Please sign in to comment.