Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Security Tests After Changes to Permissions Requirements #1308

Merged
merged 1 commit into from
Sep 11, 2024

Conversation

kaituo
Copy link
Collaborator

@kaituo kaituo commented Sep 11, 2024

Description

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

Related Issues

#1307

Suite: Test class org.opensearch.ad.rest.SecureADRestIT

  2> Sep 11, 2024 6:07:48 AM org.apache.lucene.internal.vectorization.VectorizationProvider lookup

  2> WARNING: Java vector incubator module is not readable. For optimal vector performance, pass '--add-modules jdk.incubator.vector' to enable Vector API.

  2> REPRODUCE WITH: gradlew ':integTest' --tests "org.opensearch.ad.rest.SecureADRestIT.testValidateAnomalyDetectorWithNoReadPermissionOfIndex" -Dtests.seed=92E7F7A66E928896 -Dtests.security.manager=false -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=en-AU -Dtests.timezone=Asia/Novosibirsk -Druntime.java=21

  2> junit.framework.AssertionFailedError: Expected exception Exception but no exception was thrown

        at __randomizedtesting.SeedInfo.seed([92E7F7A66E928896:95DAB14B833EFF1B]:0)

        at org.apache.lucene.tests.util.LuceneTestCase.expectThrows(LuceneTestCase.java:2885)

        at org.apache.lucene.tests.util.LuceneTestCase.expectThrows(LuceneTestCase.java:2871)

        at org.opensearch.ad.rest.SecureADRestIT.testValidateAnomalyDetectorWithNoReadPermissionOfIndex(SecureADRestIT.java:531)

  2> REPRODUCE WITH: gradlew ':integTest' --tests "org.opensearch.ad.rest.SecureADRestIT.testPreviewAnomalyDetectorWithNoReadPermissionOfIndex" -Dtests.seed=92E7F7A66E928896 -Dtests.security.manager=false -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=en-AU -Dtests.timezone=Asia/Novosibirsk -Druntime.java=21

  2> junit.framework.AssertionFailedError: Expected exception Exception but no exception was thrown

        at __randomizedtesting.SeedInfo.seed([92E7F7A66E928896:E8BD49915378A3BB]:0)

        at org.apache.lucene.tests.util.LuceneTestCase.expectThrows(LuceneTestCase.java:2885)

        at org.apache.lucene.tests.util.LuceneTestCase.expectThrows(LuceneTestCase.java:2871)

        at org.opensearch.ad.rest.SecureADRestIT.testPreviewAnomalyDetectorWithNoReadPermissionOfIndex(SecureADRestIT.java:497)

  2> REPRODUCE WITH: gradlew ':integTest' --tests "org.opensearch.ad.rest.SecureADRestIT.testCreateAnomalyDetectorWithNoReadPermissionOfIndex" -Dtests.seed=92E7F7A66E928896 -Dtests.security.manager=false -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=en-AU -Dtests.timezone=Asia/Novosibirsk -Druntime.java=21

  2> java.lang.AssertionError

        at __randomizedtesting.SeedInfo.seed([92E7F7A66E928896:6E8988EEB6425BF1]:0)

        at org.junit.Assert.fail(Assert.java:87)

        at org.junit.Assert.assertTrue(Assert.java:42)

        at org.junit.Assert.assertTrue(Assert.java:53)

        at org.opensearch.ad.rest.SecureADRestIT.testCreateAnomalyDetectorWithNoReadPermissionOfIndex(SecureADRestIT.java:418)

  2> NOTE: leaving temporary files on disk at: C:\Users\ContainerAdministrator\tmpusmlhzan\anomaly-detection\build\testrun\integTest\temp\org.opensearch.ad.rest.SecureADRestIT_92E7F7A66E928896-002

  2> NOTE: test params are: codec=Asserting(Lucene99), sim=Asserting(RandomSimilarity(queryNorm=true): {}), locale=en-AU, timezone=Asia/Novosibirsk

  2> NOTE: Windows 10 10.0 amd64/Eclipse Adoptium 21.0.3 (64-bit)/cpus=16,threads=1,free=308185312,total=536870912

  2> NOTE: All tests run in this JVM: [DetectionResultEvalutationIT, HistoricalMissingSingleFeatureIT, MissingMultiFeatureIT, MissingSingleFeatureIT, MixedRealtimeHistoricalIT, PreviewMissingSingleFeatureIT, PreviewRuleIT, RealTimeRuleIT, AnomalyDetectorRestApiIT, DataDependentADRestApiIT, HistoricalAnalysisRestApiIT, SecureADRestIT]

Sep 11, 2024 6:08:37 AM sun.util.locale.provider.LocaleProviderAdapter <clinit>

WARNING: COMPAT locale provider will be removed in a future release

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • 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.

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]>
@amitgalitz
Copy link
Member

I see we are getting NoShardAvailableActionException again on bwc, is the same as we saw before at some point?

@vibrantvarun
Copy link
Member

CI check for BWC is failing

Copy link

codecov bot commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.44%. Comparing base (3f0fc8c) to head (7b50e44).
Report is 14 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1308      +/-   ##
============================================
+ Coverage     71.83%   77.44%   +5.61%     
- Complexity     4898     5437     +539     
============================================
  Files           518      533      +15     
  Lines         22879    23324     +445     
  Branches       2245     2311      +66     
============================================
+ Hits          16434    18064    +1630     
+ Misses         5410     4199    -1211     
- Partials       1035     1061      +26     
Flag Coverage Δ
plugin 77.44% <ø> (+5.61%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 108 files with indirect coverage changes

@kaituo kaituo merged commit 0aebc6d into opensearch-project:main Sep 11, 2024
22 of 23 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 11, 2024
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]>
(cherry picked from commit 0aebc6d)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 11, 2024
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]>
(cherry picked from commit 0aebc6d)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@kaituo
Copy link
Collaborator Author

kaituo commented Sep 11, 2024

I see we are getting NoShardAvailableActionException again on bwc, is the same as we saw before at some point?

@amitgalitz @vibrantvarun I don't recall. Should be unrelated to security test change. Will try to fix it when I see it again

kaituo pushed a commit that referenced this pull request Sep 11, 2024
…1309)

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


(cherry picked from commit 0aebc6d)

Signed-off-by: Kaituo Li <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
kaituo pushed a commit that referenced this pull request Sep 11, 2024
…1310)

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


(cherry picked from commit 0aebc6d)

Signed-off-by: Kaituo Li <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants