Skip to content

Commit

Permalink
General cleanup
Browse files Browse the repository at this point in the history 
Signed-off-by: Derek Ho <[email protected]>
  • Loading branch information
@derek-ho
derek-ho committed Dec 11, 2024
1 parent 16c1615 commit dad767b
Show file tree
Hide file tree
Showing 6 changed files with 24 additions and 44 deletions.
2 changes: 1 addition & 1 deletion src/main/java/org/opensearch/security/action/apitokens/ApiTokenAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ List<ApiToken.IndexPermission> extractIndexPermissions(Map<String, Object> reque
}

/**
* Creates a single RoleV7.Index permission from a permission map
* Creates a single index permission from a permission map
*/
ApiToken.IndexPermission createIndexPermission(Map<String, Object> indexPerm) {
List<String> indexPatterns;
Expand Down
8 changes: 5 additions & 3 deletions src/main/java/org/opensearch/security/action/apitokens/ApiTokenIndexHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@
import org.opensearch.search.builder.SearchSourceBuilder;
import org.opensearch.security.support.ConfigConstants;

import static org.opensearch.security.action.apitokens.ApiToken.NAME_FIELD;

public class ApiTokenIndexHandler {

private final Client client;
Expand All @@ -54,7 +56,7 @@ public ApiTokenIndexHandler(Client client, ClusterService clusterService) {
this.clusterService = clusterService;
}

public String indexToken(ApiToken token) {
public String indexTokenPayload(ApiToken token) {
try (final ThreadContext.StoredContext ctx = client.threadPool().getThreadContext().stashContext()) {

XContentBuilder builder = XContentFactory.jsonBuilder();
Expand All @@ -81,7 +83,7 @@ public String indexToken(ApiToken token) {
public void deleteToken(String name) throws ApiTokenException {
try (final ThreadContext.StoredContext ctx = client.threadPool().getThreadContext().stashContext()) {
DeleteByQueryRequest request = new DeleteByQueryRequest(ConfigConstants.OPENSEARCH_API_TOKENS_INDEX).setQuery(
QueryBuilders.matchQuery("description", name)
QueryBuilders.matchQuery(NAME_FIELD, name)
).setRefresh(true);

BulkByScrollResponse response = client.execute(DeleteByQueryAction.INSTANCE, request).actionGet();
Expand All @@ -95,7 +97,7 @@ public void deleteToken(String name) throws ApiTokenException {
}
}

public Map<String, ApiToken> getApiTokens() {
public Map<String, ApiToken> getTokenPayloads() {
try (final ThreadContext.StoredContext ctx = client.threadPool().getThreadContext().stashContext()) {
SearchRequest searchRequest = new SearchRequest(ConfigConstants.OPENSEARCH_API_TOKENS_INDEX);
searchRequest.source(new SearchSourceBuilder());
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/opensearch/security/action/apitokens/ApiTokenRepository.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public String createApiToken(String name, List<String> clusterPermissions, List<
apiTokenIndexHandler.createApiTokenIndexIfAbsent();
// TODO: Implement logic of creating JTI to match against during authc/z
// TODO: Add validation on whether user is creating a token with a subset of their permissions
return apiTokenIndexHandler.indexToken(new ApiToken(name, "test-token", clusterPermissions, indexPermissions));
return apiTokenIndexHandler.indexTokenPayload(new ApiToken(name, "test-token", clusterPermissions, indexPermissions));
}

public void deleteApiToken(String name) throws ApiTokenException {
Expand All @@ -38,7 +38,7 @@ public void deleteApiToken(String name) throws ApiTokenException {

public Map<String, ApiToken> getApiTokens() {
apiTokenIndexHandler.createApiTokenIndexIfAbsent();
return apiTokenIndexHandler.getApiTokens();
return apiTokenIndexHandler.getTokenPayloads();
}

}
14 changes: 0 additions & 14 deletions src/main/java/org/opensearch/security/auth/BackendRegistry.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ public class BackendRegistry {
private Cache<AuthCredentials, User> userCache; // rest standard
private Cache<String, User> restImpersonationCache; // used for rest impersonation
private Cache<User, Set<String>> restRoleCache; //
private Cache<AuthCredentials, User> apiTokensCache;

private void createCaches() {
userCache = CacheBuilder.newBuilder()
Expand Down Expand Up @@ -136,18 +135,6 @@ public void onRemoval(RemovalNotification<User, Set<String>> notification) {
})
.build();

/* TODO: Listen in to index events on API Tokens index */
apiTokensCache = CacheBuilder.newBuilder()
.expireAfterWrite(ttlInMin, TimeUnit.MINUTES)
.removalListener(
(RemovalListener<AuthCredentials, User>) notification -> log.debug(
"Clear api token cache for {} due to {}",
notification.getKey(),
notification.getCause()
)
)
.build();

}

public BackendRegistry(
Expand Down Expand Up @@ -183,7 +170,6 @@ public void invalidateCache() {
userCache.invalidateAll();
restImpersonationCache.invalidateAll();
restRoleCache.invalidateAll();
apiTokensCache.invalidateAll();
}

@Subscribe
Expand Down
22 changes: 7 additions & 15 deletions src/test/java/org/opensearch/security/action/apitokens/ApiTokenActionTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,10 @@

public class ApiTokenActionTest {

private final ApiTokenAction apiTokenAction = new ApiTokenAction(null, null, null); // repository not needed for these tests
private final ApiTokenAction apiTokenAction = new ApiTokenAction(null, null, null);

@Test
public void testSafeStringList() {
// Valid case
List<String> result = apiTokenAction.safeStringList(Arrays.asList("test1", "test2"), "test_field");
assertThat(result, is(Arrays.asList("test1", "test2")));

Expand All @@ -45,20 +44,16 @@ public void testSafeStringList() {
public void testCreateIndexPermission() {
Map<String, Object> validPermission = new HashMap<>();
validPermission.put("index_pattern", "test-*");
validPermission.put("allowed_actions", Arrays.asList("read"));
validPermission.put("dls", "");
validPermission.put("fls", Arrays.asList("field1", "field2"));
validPermission.put("masked_fields", Arrays.asList("sensitive1"));
validPermission.put("allowed_actions", List.of("read"));

ApiToken.IndexPermission result = apiTokenAction.createIndexPermission(validPermission);

assertThat(result.getIndexPatterns(), is(Arrays.asList("test-*")));
assertThat(result.getAllowedActions(), is(Arrays.asList("read")));
assertThat(result.getIndexPatterns(), is(List.of("test-*")));
assertThat(result.getAllowedActions(), is(List.of("read")));
}

@Test
public void testValidateRequestParameters() {
// Valid case
Map<String, Object> validRequest = new HashMap<>();
validRequest.put("name", "test-token");
validRequest.put("cluster_permissions", Arrays.asList("perm1", "perm2"));
Expand All @@ -77,15 +72,14 @@ public void testValidateRequestParameters() {

@Test
public void testValidateIndexPermissionsList() {
// Valid case
Map<String, Object> validPerm = new HashMap<>();
validPerm.put("index_pattern", "test-*");
validPerm.put("allowed_actions", Arrays.asList("read"));
validPerm.put("allowed_actions", List.of("read"));
apiTokenAction.validateIndexPermissionsList(Collections.singletonList(validPerm));

// Missing index_pattern
Map<String, Object> missingPattern = new HashMap<>();
missingPattern.put("allowed_actions", Arrays.asList("read"));
missingPattern.put("allowed_actions", List.of("read"));
assertThrows(
IllegalArgumentException.class,
() -> apiTokenAction.validateIndexPermissionsList(Collections.singletonList(missingPattern))
Expand All @@ -102,7 +96,7 @@ public void testValidateIndexPermissionsList() {
// Invalid index_pattern type
Map<String, Object> invalidPattern = new HashMap<>();
invalidPattern.put("index_pattern", 123);
invalidPattern.put("allowed_actions", Arrays.asList("read"));
invalidPattern.put("allowed_actions", List.of("read"));
assertThrows(
IllegalArgumentException.class,
() -> apiTokenAction.validateIndexPermissionsList(Collections.singletonList(invalidPattern))
Expand All @@ -113,10 +107,8 @@ public void testValidateIndexPermissionsList() {
public void testExtractClusterPermissions() {
Map<String, Object> requestBody = new HashMap<>();

// Empty case
assertThat(apiTokenAction.extractClusterPermissions(requestBody), is(empty()));

// Valid case
requestBody.put("cluster_permissions", Arrays.asList("perm1", "perm2"));
assertThat(apiTokenAction.extractClusterPermissions(requestBody), is(Arrays.asList("perm1", "perm2")));
}
Expand Down
18 changes: 9 additions & 9 deletions src/test/java/org/opensearch/security/action/apitokens/ApiTokenIndexHandlerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.is;
import static org.opensearch.security.action.apitokens.ApiToken.NAME_FIELD;
import static org.junit.Assert.fail;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.RETURNS_DEEP_STUBS;
Expand Down Expand Up @@ -133,10 +134,10 @@ public void testDeleteApiTokeCallsDeleteByQueryWithSuppliedName() {
ArgumentCaptor<DeleteByQueryRequest> captor = ArgumentCaptor.forClass(DeleteByQueryRequest.class);
verify(client).execute(eq(DeleteByQueryAction.INSTANCE), captor.capture());

// Verify the captured request has the correct query parameters
// Captured request has the correct name
DeleteByQueryRequest capturedRequest = captor.getValue();
MatchQueryBuilder query = (MatchQueryBuilder) capturedRequest.getSearchRequest().source().query();
assertThat(query.fieldName(), equalTo("description"));
assertThat(query.fieldName(), equalTo(NAME_FIELD));
assertThat(query.value(), equalTo(tokenName));
}

Expand All @@ -163,7 +164,7 @@ public void testDeleteTokenThrowsExceptionWhenNoDocumentsDeleted() {
public void testDeleteTokenSucceedsWhenDocumentIsDeleted() {
when(metadata.hasConcreteIndex(ConfigConstants.OPENSEARCH_API_TOKENS_INDEX)).thenReturn(true);

// Mock response with 1 deleted document
// 1 deleted document
PlainActionFuture<BulkByScrollResponse> future = new PlainActionFuture<>();
BulkByScrollResponse response = mock(BulkByScrollResponse.class);
when(response.getDeleted()).thenReturn(1L);
Expand All @@ -179,10 +180,9 @@ public void testDeleteTokenSucceedsWhenDocumentIsDeleted() {
}

@Test
public void testIndexTokenStoresToken() {
public void testIndexTokenStoresTokenPayload() {
when(metadata.hasConcreteIndex(ConfigConstants.OPENSEARCH_API_TOKENS_INDEX)).thenReturn(true);

// Create a real ApiToken
List<String> clusterPermissions = Arrays.asList("cluster:admin/something");
List<ApiToken.IndexPermission> indexPermissions = Arrays.asList(
new ApiToken.IndexPermission(
Expand Down Expand Up @@ -216,7 +216,7 @@ public void testIndexTokenStoresToken() {
return null;
}).when(client).index(any(IndexRequest.class), listenerCaptor.capture());

indexHandler.indexToken(token);
indexHandler.indexTokenPayload(token);

// Verify the index request
ArgumentCaptor<IndexRequest> requestCaptor = ArgumentCaptor.forClass(IndexRequest.class);
Expand All @@ -225,7 +225,7 @@ public void testIndexTokenStoresToken() {
IndexRequest capturedRequest = requestCaptor.getValue();
assertThat(capturedRequest.index(), equalTo(ConfigConstants.OPENSEARCH_API_TOKENS_INDEX));

// Convert the source to a string and verify contents
// verify contents
String source = capturedRequest.source().utf8ToString();
assertThat(source, containsString("test-token-description"));
assertThat(source, containsString("test-jti"));
Expand All @@ -234,7 +234,7 @@ public void testIndexTokenStoresToken() {
}

@Test
public void testGetApiTokens() throws IOException {
public void testGetTokenPayloads() throws IOException {
when(metadata.hasConcreteIndex(ConfigConstants.OPENSEARCH_API_TOKENS_INDEX)).thenReturn(true);

// Create sample search hits
Expand Down Expand Up @@ -286,7 +286,7 @@ public void testGetApiTokens() throws IOException {
when(client.search(any(SearchRequest.class))).thenReturn(future);

// Get tokens and verify
Map<String, ApiToken> resultTokens = indexHandler.getApiTokens();
Map<String, ApiToken> resultTokens = indexHandler.getTokenPayloads();

assertThat(resultTokens.size(), equalTo(2));
assertThat(resultTokens.containsKey("token1-description"), is(true));
Expand Down

0 comments on commit dad767b

Please sign in to comment.