Code review comments

Signed-off-by: Derek Ho <[email protected]>
derek-ho · Dec 16, 2024 · 58cf8ca · 58cf8ca
1 parent 3fb4137
commit 58cf8ca
Show file tree

Hide file tree

Showing 4 changed files with 30 additions and 10 deletions.
diff --git a/src/main/java/org/opensearch/security/action/apitokens/ApiToken.java b/src/main/java/org/opensearch/security/action/apitokens/ApiToken.java
@@ -86,6 +86,23 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
         }
     }
 
+    /**
+     * Class represents an API token.
+     * Expected class structure
+     * {
+     *   name: "token_name",
+     *   jti: "encrypted_token",
+     *   creation_time: 1234567890,
+     *   cluster_permissions: ["cluster_permission1", "cluster_permission2"],
+     *   index_permissions: [
+     *     {
+     *       index_pattern: ["index_pattern1", "index_pattern2"],
+     *       allowed_actions: ["allowed_action1", "allowed_action2"]
+     *     }
+     *   ],
+     *   expiration: 1234567890
+     * }
+     */
     public static ApiToken fromXContent(XContentParser parser) throws IOException {
         String name = null;
         String jti = null;

diff --git a/src/main/java/org/opensearch/security/action/apitokens/ApiTokenIndexHandler.java b/src/main/java/org/opensearch/security/action/apitokens/ApiTokenIndexHandler.java
@@ -145,6 +145,7 @@ public Boolean apiTokenIndexExists() {
     }
 
     public void createApiTokenIndexIfAbsent() {
+        // TODO: Decide if this should be done at bootstrap
         if (!apiTokenIndexExists()) {
             final var originalUserAndRemoteAddress = Utils.userAndRemoteAddressFrom(client.threadPool().getThreadContext());
             try (final ThreadContext.StoredContext ctx = client.threadPool().getThreadContext().stashContext()) {

diff --git a/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java b/src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java
@@ -93,7 +93,7 @@ public abstract class AbstractAuditLog implements AuditLog {
     private final Settings settings;
     private volatile AuditConfig.Filter auditConfigFilter;
     private final String securityIndex;
-    private final WildcardMatcher securityOrApiTokensIndexMatcher;
+    private final WildcardMatcher securityIndicesMatcher;
     private volatile ComplianceConfig complianceConfig;
     private final Environment environment;
     private AtomicBoolean externalConfigLogged = new AtomicBoolean();
@@ -127,7 +127,7 @@ protected AbstractAuditLog(
             ConfigConstants.OPENDISTRO_SECURITY_DEFAULT_CONFIG_INDEX
         );
         // TODO: support custom api tokens index?
-        this.securityOrApiTokensIndexMatcher = WildcardMatcher.from(
+        this.securityIndicesMatcher = WildcardMatcher.from(
             List.of(
                 settings.get(ConfigConstants.SECURITY_CONFIG_INDEX_NAME, ConfigConstants.OPENDISTRO_SECURITY_DEFAULT_CONFIG_INDEX),
                 ConfigConstants.OPENSEARCH_API_TOKENS_INDEX
@@ -486,7 +486,7 @@ public void logDocumentRead(String index, String id, ShardId shardId, Map<String
             return;
         }
 
-        AuditCategory category = securityOrApiTokensIndexMatcher.test(index)
+        AuditCategory category = securityIndicesMatcher.test(index)
             ? AuditCategory.COMPLIANCE_INTERNAL_CONFIG_READ
             : AuditCategory.COMPLIANCE_DOC_READ;
 
@@ -519,7 +519,7 @@ public void logDocumentRead(String index, String id, ShardId shardId, Map<String
                         log.error(e.toString());
                     }
                 } else {
-                    if (securityOrApiTokensIndexMatcher.test(index) && !"tattr".equals(id)) {
+                    if (securityIndicesMatcher.test(index) && !"tattr".equals(id)) {
                         try {
                             Map<String, String> map = fieldNameValues.entrySet()
                                 .stream()
@@ -553,7 +553,7 @@ public void logDocumentWritten(ShardId shardId, GetResult originalResult, Index
             return;
         }
 
-        AuditCategory category = securityOrApiTokensIndexMatcher.test(shardId.getIndexName())
+        AuditCategory category = securityIndicesMatcher.test(shardId.getIndexName())
             ? AuditCategory.COMPLIANCE_INTERNAL_CONFIG_WRITE
             : AuditCategory.COMPLIANCE_DOC_WRITE;
 
@@ -582,7 +582,7 @@ public void logDocumentWritten(ShardId shardId, GetResult originalResult, Index
             try {
                 String originalSource = null;
                 String currentSource = null;
-                if (securityOrApiTokensIndexMatcher.test(shardId.getIndexName())) {
+                if (securityIndicesMatcher.test(shardId.getIndexName())) {
                     try (
                         XContentParser parser = XContentHelper.createParser(
                             NamedXContentRegistry.EMPTY,
@@ -638,7 +638,7 @@ public void logDocumentWritten(ShardId shardId, GetResult originalResult, Index
         }
 
         if (!complianceConfig.shouldLogWriteMetadataOnly()) {
-            if (securityOrApiTokensIndexMatcher.test(shardId.getIndexName())) {
+            if (securityIndicesMatcher.test(shardId.getIndexName())) {
                 // current source, normally not null or empty
                 try (
                     XContentParser parser = XContentHelper.createParser(

diff --git a/src/main/java/org/opensearch/security/compliance/ComplianceConfig.java b/src/main/java/org/opensearch/security/compliance/ComplianceConfig.java
@@ -107,6 +107,7 @@ public class ComplianceConfig {
     private final String auditLogIndex;
     private final boolean enabled;
     private final Supplier<DateTime> dateProvider;
+    private final WildcardMatcher securityIndicesMatcher;
 
     private ComplianceConfig(
         final boolean enabled,
@@ -174,6 +175,7 @@ public WildcardMatcher load(String index) throws Exception {
         });
 
         this.dateProvider = Optional.ofNullable(dateProvider).orElse(() -> DateTime.now(DateTimeZone.UTC));
+        this.securityIndicesMatcher = WildcardMatcher.from(securityIndex, ConfigConstants.OPENSEARCH_API_TOKENS_INDEX);
     }
 
     @VisibleForTesting
@@ -509,7 +511,7 @@ public boolean writeHistoryEnabledForIndex(String index) {
         }
         // if security index (internal index) check if internal config logging is enabled
         // TODO: Add support for custom api token index?
-        if (securityIndex.equals(index) || ConfigConstants.OPENSEARCH_API_TOKENS_INDEX.equals(index)) {
+        if (this.securityIndicesMatcher.test(index)) {
             return logInternalConfig;
         }
         // if the index is used for audit logging, return false
@@ -537,7 +539,7 @@ public boolean readHistoryEnabledForIndex(String index) {
             return false;
         }
         // if security index (internal index) check if internal config logging is enabled
-        if (securityIndex.equals(index) || ConfigConstants.OPENSEARCH_API_TOKENS_INDEX.equals(index)) {
+        if (securityIndicesMatcher.test(index)) {
             return logInternalConfig;
         }
         try {
@@ -559,7 +561,7 @@ public boolean readHistoryEnabledForField(String index, String field) {
             return false;
         }
         // if security index (internal index) check if internal config logging is enabled
-        if (securityIndex.equals(index) || ConfigConstants.OPENSEARCH_API_TOKENS_INDEX.equals(index)) {
+        if (securityIndicesMatcher.test(index)) {
             return logInternalConfig;
         }
         WildcardMatcher matcher;