Skip to content

Commit

Permalink
Fix Security Tests After Changes to Permissions Requirements (#1308)
Browse files Browse the repository at this point in the history
This PR addresses errors in security tests caused by recent changes in opensearch-project/security#4719. Previously, users needed both AD full access and source index permissions to fully utilize anomaly detection (AD). AD full access has already included all alias and mapping permissions.  it was inconsistent not to include index search permission, which would otherwise force users to create an additional role. The change in the referenced PR aimed to simplify user management.

Due to this change, existing security tests that relied on a user having AD full access but lacking data search permission would no longer trigger the expected search permission exception. This PR addresses that issue by creating a new user role with only AD read permission (note we didn't change ad read access permission in the referenced PR) and without source index search permission, ensuring the tests correctly validate the lack of search permissions.

Testing Done:
* Verified that previously failing security tests now pass

Signed-off-by: Kaituo Li <[email protected]>
  • Loading branch information
kaituo authored Sep 11, 2024
1 parent b84dbfe commit 0aebc6d
Showing 1 changed file with 17 additions and 11 deletions.
28 changes: 17 additions & 11 deletions src/test/java/org/opensearch/ad/rest/SecureADRestIT.java
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ public class SecureADRestIT extends AnomalyDetectorRestTestCase {
RestClient lionClient;
private String indexAllAccessRole = "index_all_access";
private String indexSearchAccessRole = "index_all_search";
String oceanUser = "ocean";
RestClient oceanClient;

/**
* Create an unguessable password. Simple password are weak due to https://tinyurl.com/383em9zk
Expand Down Expand Up @@ -156,7 +158,13 @@ public void setupSecureTests() throws IOException {
.setSocketTimeout(60000)
.build();

createRoleMapping("anomaly_read_access", new ArrayList<>(Arrays.asList(bobUser)));
String oceanPassword = generatePassword(oceanUser);
createUser(oceanUser, elkPassword, new ArrayList<>(Arrays.asList("odfe")));
oceanClient = new SecureRestClientBuilder(getClusterHosts().toArray(new HttpHost[0]), isHttps(), oceanUser, oceanPassword)
.setSocketTimeout(60000)
.build();

createRoleMapping("anomaly_read_access", new ArrayList<>(Arrays.asList(bobUser, oceanUser)));
createRoleMapping("anomaly_full_access", new ArrayList<>(Arrays.asList(aliceUser, catUser, dogUser, elkUser, fishUser, goatUser)));
createRoleMapping(indexAllAccessRole, new ArrayList<>(Arrays.asList(aliceUser, bobUser, catUser, dogUser, fishUser, lionUser)));
createRoleMapping(indexSearchAccessRole, new ArrayList<>(Arrays.asList(goatUser)));
Expand All @@ -172,6 +180,7 @@ public void deleteUserSetup() throws IOException {
fishClient.close();
goatClient.close();
lionClient.close();
oceanClient.close();
deleteUser(aliceUser);
deleteUser(bobUser);
deleteUser(catUser);
Expand All @@ -180,6 +189,7 @@ public void deleteUserSetup() throws IOException {
deleteUser(fishUser);
deleteUser(goatUser);
deleteUser(lionUser);
deleteUser(oceanUser);
}

public void testCreateAnomalyDetectorWithWriteAccess() throws IOException {
Expand Down Expand Up @@ -416,8 +426,8 @@ public void testCreateAnomalyDetectorWithNoReadPermissionOfIndex() throws IOExce
AnomalyDetector anomalyDetector = createRandomAnomalyDetector(false, false, aliceClient);
// User elk has AD full access, but has no read permission of index
String indexName = anomalyDetector.getIndices().get(0);
Exception exception = expectThrows(IOException.class, () -> { createRandomAnomalyDetector(false, false, indexName, elkClient); });
Assert.assertTrue(exception.getMessage().contains("no permissions for [indices:data/read/search]"));
Exception exception = expectThrows(IOException.class, () -> { createRandomAnomalyDetector(false, false, indexName, oceanClient); });
Assert.assertTrue("actual: " + exception.getMessage(), exception.getMessage().contains("Unauthorized"));
}

public void testCreateAnomalyDetectorWithCustomResultIndex() throws IOException {
Expand Down Expand Up @@ -496,12 +506,8 @@ public void testPreviewAnomalyDetectorWithNoReadPermissionOfIndex() throws IOExc
);
enableFilterBy();
// User elk has no read permission of index
Exception exception = expectThrows(Exception.class, () -> { previewAnomalyDetector(aliceDetector.getId(), elkClient, input); });
Assert
.assertTrue(
"actual msg: " + exception.getMessage(),
exception.getMessage().contains("no permissions for [indices:data/read/search]")
);
Exception exception = expectThrows(Exception.class, () -> { previewAnomalyDetector(aliceDetector.getId(), oceanClient, input); });
Assert.assertTrue("actual msg: " + exception.getMessage(), exception.getMessage().contains("Unauthorized"));
}

public void testValidateAnomalyDetectorWithWriteAccess() throws IOException {
Expand Down Expand Up @@ -530,8 +536,8 @@ public void testValidateAnomalyDetectorWithNoReadPermissionOfIndex() throws IOEx
AnomalyDetector detector = TestHelpers.randomAnomalyDetector(null, Instant.now());
enableFilterBy();
// User elk has no read permission of index, can't validate detector
Exception exception = expectThrows(Exception.class, () -> { validateAnomalyDetector(detector, elkClient); });
Assert.assertTrue(exception.getMessage().contains("no permissions for [indices:data/read/search]"));
Exception exception = expectThrows(Exception.class, () -> { validateAnomalyDetector(detector, oceanClient); });
Assert.assertTrue("actual: " + exception.getMessage(), exception.getMessage().contains("Unauthorized"));
}

public void testValidateAnomalyDetectorWithNoBackendRole() throws IOException {
Expand Down

0 comments on commit 0aebc6d

Please sign in to comment.