Integ test cases for field-level security by Swiddis · Pull Request #5008 · opensearch-project/sql

Swiddis · 2025-12-30T18:58:20Z

Description

Adds some tests to validate various scenarios for document-level security (DLS), since there was an internal scare for a bit on if DLS is working with background IO. Turns out we're protected implicitly by ContextPreservingRunnable. Just pushing some work from that to keep verifying this in the future.

Related Issues

N/A

Check List

New functionality includes testing.
New functionality has been documented.
New functionality has javadoc added.
New functionality has a user manual doc added.
New PPL command checklist all confirmed.
API changes companion pull request created.
Commits are signed per the DCO using --signoff or -s.
Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

coderabbitai · 2025-12-30T18:58:26Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Summary by CodeRabbit

Tests
- Added comprehensive FGAC integration tests covering index-, field-, and row-level security across large datasets and multiple engine configurations
- Introduced a shared security-focused test base with utilities for role/user setup, DLS/FLS, bulk data population, and authenticated query execution
- Updated existing permission tests to use the new security-centric test base
Chores
- Added junit-jupiter-params test dependency

Walkthrough

Adds a SecurityTestBase providing role/user/DLS/FLS helpers, authenticated query and bulk utilities; introduces FGACIndexScanningIT to test index-, field-, and row-level FGAC with large datasets across two engine modes; refactors PPLPermissionsIT to use the new base; and updates integ-test build deps for JUnit parameterized tests.

Changes

Cohort / File(s)	Summary
Test build config `integ-test/build.gradle`	Adds `org.junit.jupiter:junit-jupiter-params:5.9.3` to `testImplementation`.
Security test base `integ-test/src/test/java/org/opensearch/sql/security/SecurityTestBase.java`	New abstract test base adding helpers for creating roles (index access, permissions, DLS, FLS), creating users and role-mappings, executing authenticated PPL queries (Basic Auth), building/performing bulk inserts, creating indices with mappings, and an inner `BulkDocumentBuilder`.
New FGAC integration test `integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java`	New parameterized integration test that creates indices, roles, and users; bulk-populates large datasets; and verifies index-, field-, and row-level FGAC behaviors across users and engine modes.
Permissions tests refactor `integ-test/src/test/java/org/opensearch/sql/security/PPLPermissionsIT.java`	Class now extends `SecurityTestBase` (was `PPLIntegTestCase`); removes in-class password/user creation and legacy helpers; replaces role/user/index setup with `SecurityTestBase` utilities like `createRoleWithIndexAccess` and `createRoleWithPermissions`.

Sequence Diagram(s)

sequenceDiagram
    actor Test
    participant Base as SecurityTestBase
    participant OpenSearch
    participant Auth as Auth/Authorization

    rect rgba(230,240,255,0.5)
        Note over Test,Base: Setup Phase
        Test->>Base: createRoleWithIndexAccess(role, indexPattern)
        Base->>OpenSearch: HTTP role creation (index/DLS/FLS/permissions)
        OpenSearch-->>Base: 200 OK
        Test->>Base: createUser(username, role)
        Base->>OpenSearch: HTTP user creation & role-mapping
        OpenSearch-->>Base: 200 OK
    end

    rect rgba(240,255,230,0.5)
        Note over Test,Base: Bulk Load Phase
        Test->>Base: performBulkInsert(bulkBody)
        Base->>OpenSearch: Bulk API request (with refresh)
        OpenSearch-->>Base: Bulk response
    end

    rect rgba(255,240,230,0.5)
        Note over Test,Base: Authenticated Query Phase
        Test->>Base: executeQueryAsUser(query, username)
        Base->>Base: createBasicAuthHeader(username, password)
        Base->>OpenSearch: PPL query + Basic Auth header
        OpenSearch->>Auth: Validate credentials & evaluate roles
        Auth-->>OpenSearch: Authorization decision
        OpenSearch-->>Base: Query results (filtered by FGAC)
        Base-->>Test: JSONObject response
    end

    rect rgba(245,235,255,0.5)
        Note over Test: Verification
        Test->>Test: Assert results match expected FGAC behavior
    end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Fix the filter of integTestWithSecurity #5098 — Modifies integ-test security tests and PPLPermissionsIT setup, directly related to the refactor to SecurityTestBase.

Suggested labels

testing

Suggested reviewers

kavithacm
derek-ho
joshuali925
anirudha
penghuo
ps48
ykmr1224
yuancu
mengweieric
GumpacG
MaxKsyunz

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.98% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	Title focuses on field-level security testing, but PR primarily adds document-level security (DLS) integration tests with a new security test base class and parameterized tests across multiple security scenarios.	Clarify whether the title should emphasize DLS (the main focus per the description) or broaden to include the new SecurityTestBase utility class and comprehensive security testing framework.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description check	✅ Passed	Description clearly explains the purpose: adding DLS integration tests to verify security with background I/O, references ContextPreservingRunnable, and indicates future verification intent.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

Swiddis · 2025-12-30T19:05:11Z

@coderabbitai review

coderabbitai · 2025-12-30T19:05:17Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (4)

integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java (2)
53-64: Static initialization flag may cause issues with test isolation.

The static boolean initialized flag combined with @BeforeEach could cause issues if tests run in parallel or if the test class is instantiated multiple times. Consider using @BeforeAll with a static method instead, which is the idiomatic JUnit 5 pattern for one-time setup.
🔎 Proposed refactor to use @BeforeAll
- private static boolean initialized = false;
-
- @SneakyThrows
- @BeforeEach
- public void initialize() {
-   if (!initialized) {
-     setUpIndices(); // Initialize client if needed
-     setupTestIndices();
-     createSecurityRolesAndUsers();
-     initialized = true;
-   }
- }
+ @SneakyThrows
+ @BeforeAll
+ public static void initializeOnce() {
+   // Note: This requires making the setup methods static or 
+   // using a different initialization approach
+ }
Alternatively, if @BeforeAll isn't compatible with the parent class, document why the static flag pattern is necessary.
74-85: Consider adding test cleanup for created indices.

The test creates multiple indices (public_logs_fgac, sensitive_logs_fgac, secure_logs_fgac, employee_records_fgac) but there's no @AfterAll or @AfterEach cleanup method. While using unique _fgac suffix provides isolation, these indices will persist after tests complete.

As per coding guidelines, integration tests should validate that tests clean up resources after execution.
🔎 Consider adding cleanup
@AfterAll
public static void cleanup() throws IOException {
  // Delete test indices
  String[] indices = {PUBLIC_LOGS, SENSITIVE_LOGS, SECURE_LOGS, EMPLOYEE_RECORDS};
  for (String index : indices) {
    try {
      client().performRequest(new Request("DELETE", "/" + index));
    } catch (Exception e) {
      // Index may not exist, ignore
    }
  }
}
integ-test/src/test/java/org/opensearch/sql/security/SecurityTestBase.java (2)
56-98: Consider using JSONArray for permission arrays to avoid manual string building.

The manual StringBuilder approach for building JSON arrays works but is error-prone. Using JSONArray would be cleaner and safer.
🔎 Optional refactor using JSONArray
import org.json.JSONArray;

// In createRoleWithPermissions:
JSONArray clusterPermsArray = new JSONArray(clusterPermissions);
JSONArray indexPermsArray = new JSONArray(indexPermissions);

// Then use clusterPermsArray.toString() and indexPermsArray.toString() in the template
341-353: Remove unused BulkDocumentBuilder helper class or document its intended purpose.

The BulkDocumentBuilder is defined but never instantiated or used anywhere in the codebase. Either remove it if not needed for this PR, or add a comment documenting its intended usage in future tests.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 08be6f9 and ded19e9.

📒 Files selected for processing (4)

integ-test/build.gradle
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
integ-test/src/test/java/org/opensearch/sql/security/PPLPermissionsIT.java
integ-test/src/test/java/org/opensearch/sql/security/SecurityTestBase.java

🧰 Additional context used

📓 Path-based instructions (4)

**/*.java

📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)

**/*.java: Use PascalCase for class names (e.g., QueryExecutor)
Use camelCase for method and variable names (e.g., executeQuery)
Use UPPER_SNAKE_CASE for constants (e.g., MAX_RETRY_COUNT)
Keep methods under 20 lines with single responsibility
All public classes and methods must have proper JavaDoc
Use specific exception types with meaningful messages for error handling
Prefer Optional<T> for nullable returns in Java
Avoid unnecessary object creation in loops
Use StringBuilder for string concatenation in loops
Validate all user inputs, especially queries
Sanitize data before logging to prevent injection attacks
Use try-with-resources for proper resource cleanup in Java
Maintain Java 11 compatibility when possible for OpenSearch 2.x
Document Calcite-specific workarounds in code

Files:

integ-test/src/test/java/org/opensearch/sql/security/PPLPermissionsIT.java
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
integ-test/src/test/java/org/opensearch/sql/security/SecurityTestBase.java

⚙️ CodeRabbit configuration file

**/*.java: - Flag methods >50 lines as potentially too complex - suggest refactoring

Flag classes >500 lines as needing organization review

Check for dead code, unused imports, and unused variables

Identify code reuse opportunities across similar implementations

Assess holistic maintainability - is code easy to understand and modify?

Flag code that appears AI-generated without sufficient human review

Verify Java naming conventions (PascalCase for classes, camelCase for methods/variables)

Check for proper JavaDoc on public classes and methods

Flag redundant comments that restate obvious code

Ensure proper error handling with specific exception types

Check for Optional usage instead of null returns

Validate proper use of try-with-resources for resource management

Files:

integ-test/src/test/java/org/opensearch/sql/security/PPLPermissionsIT.java
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
integ-test/src/test/java/org/opensearch/sql/security/SecurityTestBase.java

integ-test/**/*IT.java

📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)

End-to-end scenarios need integration tests in integ-test/ module

Files:

integ-test/src/test/java/org/opensearch/sql/security/PPLPermissionsIT.java
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java

⚙️ CodeRabbit configuration file

integ-test/**/*IT.java: - Integration tests MUST use valid test data from resources

Verify test data files exist in integ-test/src/test/resources/

Check test assertions are meaningful and specific

Validate tests clean up resources after execution

Ensure tests are independent and can run in any order

Flag tests that reference non-existent indices (e.g., EMP)

Verify integration tests are in correct module (integ-test/)

Check tests can be run with ./gradlew :integ-test:integTest

Ensure proper test data setup and teardown

Validate end-to-end scenario coverage

Files:

integ-test/src/test/java/org/opensearch/sql/security/PPLPermissionsIT.java
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java

**/*IT.java

📄 CodeRabbit inference engine (.rules/REVIEW_GUIDELINES.md)

Name integration tests with *IT.java suffix in OpenSearch SQL

Files:

integ-test/src/test/java/org/opensearch/sql/security/PPLPermissionsIT.java
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java

**/test/**/*.java

⚙️ CodeRabbit configuration file

**/test/**/*.java: - Verify NULL input tests for all new functions

Check boundary condition tests (min/max values, empty inputs)

Validate error condition tests (invalid inputs, exceptions)

Ensure multi-document tests for per-document operations

Flag smoke tests without meaningful assertions

Check test naming follows pattern: test

Verify test data is realistic and covers edge cases

Verify test coverage for new business logic

Ensure tests are independent and don't rely on execution order

Validate meaningful test data that reflects real-world scenarios

Check for proper cleanup of test resources

Files:

integ-test/src/test/java/org/opensearch/sql/security/PPLPermissionsIT.java
integ-test/src/test/java/org/opensearch/sql/security/FGACIndexScanningIT.java
integ-test/src/test/java/org/opensearch/sql/security/SecurityTestBase.java

🧠 Learnings (8)

📓 Common learnings

Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Applies to **/*IT.java : Name integration tests with `*IT.java` suffix in OpenSearch SQL

Learnt from: CR
Repo: opensearch-project/sql PR: 0
File: .rules/REVIEW_GUIDELINES.md:0-0
Timestamp: 2025-12-02T17:27:55.938Z
Learning: Verify changes with `./gradlew :integ-test:integTest` before merge