PR review

Signed-off-by: Derek Ho <[email protected]>
derek-ho · Dec 19, 2024 · 2287742 · 2287742
1 parent da8cf45
commit 2287742
Show file tree

Hide file tree

Showing 10 changed files with 38 additions and 54 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
@@ -38,46 +38,36 @@ public class ApiToken implements ToXContent {
     public static final String ALLOWED_ACTIONS_FIELD = "allowed_actions";
     public static final String EXPIRATION_FIELD = "expiration";
 
-    private String name;
+    private final String name;
     private String jti;
     private final Instant creationTime;
-    private List<String> clusterPermissions;
-    private List<IndexPermission> indexPermissions;
+    private final List<String> clusterPermissions;
+    private final List<IndexPermission> indexPermissions;
     private final long expiration;
 
-    public ApiToken(String name, List<String> clusterPermissions, List<IndexPermission> indexPermissions, Long expiration) {
+    public ApiToken(String name, String jti, List<String> clusterPermissions, List<IndexPermission> indexPermissions, Long expiration) {
         this.creationTime = Instant.now();
+        this.jti = jti;
         this.name = name;
         this.clusterPermissions = clusterPermissions;
         this.indexPermissions = indexPermissions;
         this.expiration = expiration;
     }
 
-    public ApiToken(String name, List<String> clusterPermissions, List<IndexPermission> indexPermissions) {
-        this.creationTime = Instant.now();
-        this.name = name;
-        this.clusterPermissions = clusterPermissions;
-        this.indexPermissions = indexPermissions;
-        this.expiration = Long.MAX_VALUE;
-    }
-
     public ApiToken(
         String name,
+        String jti,
         List<String> clusterPermissions,
         List<IndexPermission> indexPermissions,
         Instant creationTime,
         Long expiration
     ) {
         this.name = name;
+        this.jti = jti;
         this.clusterPermissions = clusterPermissions;
         this.indexPermissions = indexPermissions;
         this.creationTime = creationTime;
         this.expiration = expiration;
-
-    }
-
-    public void setJti(String jti) {
-        this.jti = jti;
     }
 
     public static class IndexPermission implements ToXContent {
@@ -106,6 +96,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
             return builder;
         }
 
+        // TODO: look into integrating with default object mapper
         @Override
         public String toString() {
             JsonObject json = new JsonObject();
@@ -157,6 +148,7 @@ public static IndexPermission fromString(String str) {
      */
     public static ApiToken fromXContent(XContentParser parser) throws IOException {
         String name = null;
+        String jti = null;
         List<String> clusterPermissions = new ArrayList<>();
         List<IndexPermission> indexPermissions = new ArrayList<>();
         Instant creationTime = null;
@@ -173,6 +165,9 @@ public static ApiToken fromXContent(XContentParser parser) throws IOException {
                     case NAME_FIELD:
                         name = parser.text();
                         break;
+                    case JTI_FIELD:
+                        jti = parser.text();
+                        break;
                     case CREATION_TIME_FIELD:
                         creationTime = Instant.ofEpochMilli(parser.longValue());
                         break;
@@ -198,7 +193,7 @@ public static ApiToken fromXContent(XContentParser parser) throws IOException {
             }
         }
 
-        return new ApiToken(name, clusterPermissions, indexPermissions, creationTime, expiration);
+        return new ApiToken(name, jti, clusterPermissions, indexPermissions, creationTime, expiration);
     }
 
     private static IndexPermission parseIndexPermission(XContentParser parser) throws IOException {
@@ -233,10 +228,6 @@ public String getName() {
         return name;
     }
 
-    public void setName(String name) {
-        this.name = name;
-    }
-
     public Long getExpiration() {
         return expiration;
     }
@@ -254,10 +245,6 @@ public List<String> getClusterPermissions() {
         return clusterPermissions;
     }
 
-    public void setClusterPermissions(List<String> clusterPermissions) {
-        this.clusterPermissions = clusterPermissions;
-    }
-
     @Override
     public XContentBuilder toXContent(XContentBuilder xContentBuilder, ToXContent.Params params) throws IOException {
         xContentBuilder.startObject();
@@ -273,8 +260,4 @@ public XContentBuilder toXContent(XContentBuilder xContentBuilder, ToXContent.Pa
     public List<IndexPermission> getIndexPermissions() {
         return indexPermissions;
     }
-
-    public void setIndexPermissions(List<IndexPermission> indexPermissions) {
-        this.indexPermissions = indexPermissions;
-    }
 }
diff --git a/src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java b/src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java
@@ -191,7 +191,7 @@ void validateRequestParameters(Map<String, Object> requestBody) {
         if (requestBody.containsKey(EXPIRATION_FIELD)) {
             Object expiration = requestBody.get(EXPIRATION_FIELD);
             if (!(expiration instanceof Long)) {
-                throw new IllegalArgumentException(EXPIRATION_FIELD + " must be an long");
+                throw new IllegalArgumentException(EXPIRATION_FIELD + " must be a long");
             }
         }
 

diff --git a/src/main/java/org/opensearch/security/action/apitokens/ApiTokenRepository.java b/src/main/java/org/opensearch/security/action/apitokens/ApiTokenRepository.java
@@ -42,9 +42,8 @@ public String createApiToken(
     ) {
         apiTokenIndexHandler.createApiTokenIndexIfAbsent();
         // TODO: Add validation on whether user is creating a token with a subset of their permissions
-        ApiToken apiToken = new ApiToken(name, clusterPermissions, indexPermissions, expiration);
-        ExpiringBearerAuthToken token = securityTokenManager.issueApiToken(apiToken);
-        apiToken.setJti(securityTokenManager.encryptToken(token.getCompleteToken()));
+        ExpiringBearerAuthToken token = securityTokenManager.issueApiToken(name, expiration, clusterPermissions, indexPermissions);
+        ApiToken apiToken = new ApiToken(name, securityTokenManager.encryptToken(token.getCompleteToken()), clusterPermissions, indexPermissions, expiration);
         apiTokenIndexHandler.indexTokenMetadata(apiToken);
         return token.getCompleteToken();
     }

diff --git a/src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java b/src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java
@@ -177,7 +177,7 @@ public ExpiringBearerAuthToken createJwt(
 
         if (clusterPermissions != null) {
             final String listOfClusterPermissions = String.join(",", clusterPermissions);
-            claimsBuilder.claim("cp", encryptionDecryptionUtil.encrypt(listOfClusterPermissions));
+            claimsBuilder.claim("cp", encryptString(listOfClusterPermissions));
         }
 
         if (indexPermissions != null) {
@@ -186,7 +186,7 @@ public ExpiringBearerAuthToken createJwt(
                 permissionStrings.add(permission.toString());
             }
             final String listOfIndexPermissions = String.join(",", permissionStrings);
-            claimsBuilder.claim("ip", encryptionDecryptionUtil.encrypt(listOfIndexPermissions));
+            claimsBuilder.claim("ip", encryptString(listOfIndexPermissions));
         }
 
         final JWSHeader header = new JWSHeader.Builder(JWSAlgorithm.parse(signingKey.getAlgorithm().getName())).build();

diff --git a/src/main/java/org/opensearch/security/http/ApiTokenAuthenticator.java b/src/main/java/org/opensearch/security/http/ApiTokenAuthenticator.java
@@ -0,0 +1,4 @@
+package org.opensearch.security.http;
+
+public class ApiTokenAuthenticator {
+}
diff --git a/src/main/java/org/opensearch/security/identity/SecurityTokenManager.java b/src/main/java/org/opensearch/security/identity/SecurityTokenManager.java
@@ -11,6 +11,7 @@
 
 package org.opensearch.security.identity;
 
+import java.util.List;
 import java.util.Optional;
 import java.util.Set;
 import java.util.stream.Collectors;
@@ -140,7 +141,7 @@ public ExpiringBearerAuthToken issueOnBehalfOfToken(final Subject subject, final
         }
     }
 
-    public ExpiringBearerAuthToken issueApiToken(final ApiToken apiToken) {
+    public ExpiringBearerAuthToken issueApiToken(final String name, final Long expiration, final List<String> clusterPermissions, final List<ApiToken.IndexPermission> indexPermissions) {
         final User user = threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER);
         if (user == null) {
             throw new OpenSearchSecurityException("Unsupported user to generate Api Token");
@@ -149,11 +150,11 @@ public ExpiringBearerAuthToken issueApiToken(final ApiToken apiToken) {
         try {
             return apiTokenJwtVendor.createJwt(
                 cs.getClusterName().value(),
-                apiToken.getName(),
-                apiToken.getName(),
-                apiToken.getExpiration(),
-                apiToken.getClusterPermissions(),
-                apiToken.getIndexPermissions()
+                name,
+                name,
+                expiration,
+                clusterPermissions,
+                indexPermissions
             );
         } catch (final Exception ex) {
             logger.error("Error creating Api Token for " + user.getName(), ex);

diff --git a/src/main/java/org/opensearch/security/securityconf/impl/v7/ConfigV7.java b/src/main/java/org/opensearch/security/securityconf/impl/v7/ConfigV7.java
@@ -541,7 +541,7 @@ public void setEncryptionKey(String encryptionKey) {
 
         @Override
         public String toString() {
-            return "ApiTokens [ enabled=" + enabled + ", signing_key=" + signingKey + ", encryption_key=" + encryptionKey + "]";
+            return "ApiTokenSettings [ enabled=" + enabled + ", signing_key=" + signingKey + ", encryption_key=" + encryptionKey + "]";
         }
 
     }

diff --git a/src/test/java/org/opensearch/security/action/apitokens/ApiTokenIndexHandlerTest.java b/src/test/java/org/opensearch/security/action/apitokens/ApiTokenIndexHandlerTest.java
@@ -192,12 +192,12 @@ public void testIndexTokenStoresTokenPayload() {
         );
         ApiToken token = new ApiToken(
                 "test-token-description",
+                "test-token-jti",
                 clusterPermissions,
                 indexPermissions,
                 Instant.now(),
                 Long.MAX_VALUE
         );
-        token.setJti("test-token-jti");
 
         // Mock the index method with ActionListener
         @SuppressWarnings("unchecked")
@@ -245,6 +245,7 @@ public void testGetTokenPayloads() throws IOException {
         // First token
         ApiToken token1 = new ApiToken(
                 "token1-description",
+                "token1-jti",
                 Arrays.asList("cluster:admin/something"),
                 Arrays.asList(new ApiToken.IndexPermission(
                         Arrays.asList("index1-*"),
@@ -257,6 +258,7 @@ public void testGetTokenPayloads() throws IOException {
         // Second token
         ApiToken token2 = new ApiToken(
                 "token2-description",
+                "token2-jti",
                 Arrays.asList("cluster:admin/other"),
                 Arrays.asList(new ApiToken.IndexPermission(
                         Arrays.asList("index2-*"),

diff --git a/src/test/java/org/opensearch/security/action/apitokens/ApiTokenRepositoryTest.java b/src/test/java/org/opensearch/security/action/apitokens/ApiTokenRepositoryTest.java
@@ -62,7 +62,7 @@ public void testDeleteApiToken() throws ApiTokenException {
     @Test
     public void testGetApiTokens() throws IndexNotFoundException {
         Map<String, ApiToken> expectedTokens = new HashMap<>();
-        expectedTokens.put("token1", new ApiToken("token1", Arrays.asList("perm1"), Arrays.asList(), Long.MAX_VALUE));
+        expectedTokens.put("token1", new ApiToken("token1", "token1-jti", Arrays.asList("perm1"), Arrays.asList(), Long.MAX_VALUE));
         when(apiTokenIndexHandler.getTokenMetadatas()).thenReturn(expectedTokens);
 
         Map<String, ApiToken> result = repository.getApiTokens();
@@ -84,13 +84,13 @@ public void testCreateApiToken() {
         String encryptedToken = "encrypted-token";
         ExpiringBearerAuthToken bearerToken = mock(ExpiringBearerAuthToken.class);
         when(bearerToken.getCompleteToken()).thenReturn(completeToken);
-        when(securityTokenManager.issueApiToken(any())).thenReturn(bearerToken);
+        when(securityTokenManager.issueApiToken(any(), any(), any(), any())).thenReturn(bearerToken);
         when(securityTokenManager.encryptToken(completeToken)).thenReturn(encryptedToken);
 
         String result = repository.createApiToken(tokenName, clusterPermissions, indexPermissions, expiration);
 
         verify(apiTokenIndexHandler).createApiTokenIndexIfAbsent();
-        verify(securityTokenManager).issueApiToken(any(ApiToken.class));
+        verify(securityTokenManager).issueApiToken(any(), any(), any(), any());
         verify(securityTokenManager).encryptToken(completeToken);
         verify(apiTokenIndexHandler).indexTokenMetadata(
             argThat(

diff --git a/src/test/java/org/opensearch/security/authtoken/jwt/JwtVendorTest.java b/src/test/java/org/opensearch/security/authtoken/jwt/JwtVendorTest.java
@@ -274,7 +274,7 @@ public void testCreateJwtLogsCorrectly() throws Exception {
     }
 
     @Test
-    public void testCreateJwtForApiTokenLogsCorrectly() throws Exception {
+    public void testCreateJwtForApiTokenSuccess() throws Exception {
         final String issuer = "cluster_0";
         final String subject = "test-token";
         final String audience = "test-token";
@@ -286,12 +286,7 @@ public void testCreateJwtForApiTokenLogsCorrectly() throws Exception {
 
         LongSupplier currentTime = () -> (long) 100;
         String claimsEncryptionKey = "1234567890123456";
-        Settings settings = Settings.builder()
-            .put("signing_key", signingKeyB64Encoded)
-            .put("encryption_key", claimsEncryptionKey)
-            // CS-SUPPRESS-SINGLE: RegexpSingleline get Extensions Settings
-            .put(ConfigConstants.EXTENSIONS_BWC_PLUGIN_MODE, true)
-            .build();
+        Settings settings = Settings.builder().put("signing_key", signingKeyB64Encoded).put("encryption_key", claimsEncryptionKey).build();
         final JwtVendor jwtVendor = new JwtVendor(settings, Optional.of(currentTime));
         final ExpiringBearerAuthToken authToken = jwtVendor.createJwt(
             issuer,