fixed security tests #484

raj-chak · 2022-07-06T15:33:59Z

Signed-off-by: Raj Chakravarthi raj@icedome.ca

Issue #, if available:
#300
#329

Description of changes:

CheckList:
[ ] Commits are signed per the DCO using --signoff

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.

codecov-commenter · 2022-07-25T14:26:36Z

Codecov Report

Merging #484 (a676235) into main (6a30e6b) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main     #484   +/-   ##
=========================================
  Coverage     76.72%   76.72%           
  Complexity      176      176           
=========================================
  Files           166      166           
  Lines          8370     8370           
  Branches       1232     1232           
=========================================
  Hits           6422     6422           
+ Misses         1343     1342    -1     
- Partials        605      606    +1

Impacted Files	Coverage Δ
...rg/opensearch/alerting/BucketLevelMonitorRunner.kt	`76.15% <0.00%> (-0.77%)`	⬇️
.../kotlin/org/opensearch/alerting/core/JobSweeper.kt	`73.20% <0.00%> (-0.48%)`	⬇️
...destinationmigration/DestinationConversionUtils.kt	`70.00% <0.00%> (ø)`
...nationmigration/DestinationMigrationCoordinator.kt	`63.82% <0.00%> (+2.12%)`	⬆️
...ing/model/destination/DestinationContextFactory.kt	`78.57% <0.00%> (+3.57%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us.

lezzago · 2022-07-25T14:17:14Z

alerting/src/test/kotlin/org/opensearch/alerting/resthandler/SecureEmailAccountRestApiIT.kt

@@ -155,19 +155,15 @@ class SecureEmailAccountRestApiIT : AlertingRestTestCase() {
            deleteRoleAndRoleMapping(TEST_HR_ROLE)
        }
    }
-


Nitpick: Maintain an empty line between tests.

lezzago · 2022-07-25T14:19:17Z

alerting/src/test/kotlin/org/opensearch/alerting/resthandler/SecureMonitorRestApiIT.kt

@@ -85,7 +94,6 @@ class SecureMonitorRestApiIT : AlertingRestTestCase() {
    }

    // Create Monitor related security tests
-
    fun `test create monitor with an user with alerting role`() {

        createUserWithTestData(user, TEST_HR_INDEX, TEST_HR_ROLE, TEST_HR_BACKEND_ROLE)


Give the user ALERTING_INDEX_MONITOR_ACCESS cluster permission instead of ALERTING_FULL_ACCESS_ROLE.

Wondering if we also need separate tests for ALERTING_FULL_ACCESS_ROLE
cc: @lezzago

lezzago · 2022-07-25T14:40:44Z

alerting/src/test/kotlin/org/opensearch/alerting/resthandler/SecureMonitorRestApiIT.kt

        }
-
        // add alerting roles and search as userOne - must return 1 docs
        createUserRolesMapping(ALERTING_FULL_ACCESS_ROLE, arrayOf(user))


Use this role instead: ALERTING_SEARCH_MONITOR_ONLY_ACCESS

lezzago · 2022-07-25T14:46:32Z

alerting/src/test/kotlin/org/opensearch/alerting/resthandler/SecureMonitorRestApiIT.kt

@@ -809,29 +811,27 @@ class SecureMonitorRestApiIT : AlertingRestTestCase() {
            assertEquals("Delete monitor failed", RestStatus.OK, adminDeleteResponse.restStatus())
        } finally {
            deleteRoleAndRoleMapping(TEST_HR_ROLE)
+            deleteRoleMapping(ALERTING_FULL_ACCESS_ROLE)


Isn't this a role and not a role mapping?

it depends on how we added it. This seems to be correct

getsaurabh02 · 2022-07-25T17:05:12Z

alerting/src/test/kotlin/org/opensearch/alerting/AlertingRestTestCase.kt

+        var entity = "{\n" +
+            "\"cluster_permissions\": [\n" +
+            "\"$clusterPermissions\"\n" +
+            "],\n" +
+            "\"index_permissions\": [\n" +
+            "{\n" +
+            "\"index_patterns\": [\n" +
+            "\"$index\"\n" +
+            "],\n" +
+            "\"dls\": \"$dlsQuery\",\n" +
+            "\"fls\": [],\n" +
+            "\"masked_fields\": [],\n" +
+            "\"allowed_actions\": [\n" +
+            "\"crud\"\n" +
+            "]\n" +
+            "}\n" +
+            "],\n" +
+            "\"tenant_permissions\": []\n" +
+            "}"


I see duplicate codes for entity creation across the call. Can we abstract out the string construction logic in a helper method. It will provide consistent and simpler way for future additions.

getsaurabh02 · 2022-07-25T17:08:05Z

alerting/src/test/kotlin/org/opensearch/alerting/resthandler/SecureMonitorRestApiIT.kt

@@ -85,7 +94,6 @@ class SecureMonitorRestApiIT : AlertingRestTestCase() {
    }

    // Create Monitor related security tests
-
    fun `test create monitor with an user with alerting role`() {

        createUserWithTestData(user, TEST_HR_INDEX, TEST_HR_ROLE, TEST_HR_BACKEND_ROLE)


Wondering if we also need separate tests for ALERTING_FULL_ACCESS_ROLE
cc: @lezzago

getsaurabh02 · 2022-07-25T17:17:09Z

alerting/src/test/kotlin/org/opensearch/alerting/resthandler/SecureMonitorRestApiIT.kt

+        createUser(user, user, arrayOf(TEST_HR_BACKEND_ROLE))
+        createTestIndex(TEST_HR_INDEX)
+        createCustomIndexRoleWithDocLevelSecurity(TEST_HR_ROLE, TEST_HR_INDEX, TERM_DLS_QUERY, getClusterPermissionsFromCustomRole(ALERTING_INDEX_MONITOR_ACCESS))
+        createUserRolesMapping(TEST_HR_ROLE, arrayOf(user))


Do we also need tests to verify cross user accesses such as between UserA and UserB?

different users were used to eliminate issues during parallel execution by the test engine while running tests, probably not required

Signed-off-by: Raj Chakravarthi <raj@icedome.ca>

lezzago · 2023-01-19T18:06:14Z

@raj-chak can you please resolve the conflicts in the PR? Will take a look again after it has been resolved and get it reviewed quick

Signed-off-by: Raj Chakravarthi <raj@icedome.ca>

deshsidd · 2023-02-08T00:41:21Z

@raj-chak Can we try to fix the tests and work to either merge or close this PR. This has been open for more than 6 months now.

cc @lezzago @getsaurabh02

raj-chak · 2023-02-08T02:36:23Z

@raj-chak Can we try to fix the tests and work to either merge or close this PR. This has been open for more than 6 months now.

cc @lezzago @getsaurabh02

yes I resolved conflicts and ran integration tests against a 2.5 docker cluster, they ran successfully. There were issues with build.gradle (mockit 5.1.0 change) and PercolateQueryBuilderExt.java ( org.opensearch.common.SetOnce instead of lucene). will check again

getsaurabh02 · 2023-02-08T19:28:38Z

@raj-chak Do we know the reason for these test failures?

lezzago · 2023-02-10T18:19:04Z

@raj-chak Do we know the reason for these test failures?

The test failures are existing 3.0 failures coming from core. As the security integration tests pass and the changes are isolated to the security tests, we should know the failures are not caused by the PR.

* fixed security tests Signed-off-by: Raj Chakravarthi <raj@icedome.ca> (cherry picked from commit c51940f)

opensearch-trigger-bot · 2023-02-10T18:56:15Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-484-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 c51940f5e2d14a00262f7f4675bbba2d930d2ede
# Push it to GitHub
git push --set-upstream origin backport/backport-484-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-484-to-2.x.

* fixed security tests Signed-off-by: Raj Chakravarthi <raj@icedome.ca> (cherry picked from commit c51940f)

* Added exception check once the .opendistro-alerting-config index is b… (#650) * Added exception check once the .opendistro-alerting-config index is being created During .opendistro-alerting-config index creation, if ResourceAlreadyExists exception is being raised, the flow will check first if the index is in yellow state and then it will re-try to index monitor Signed-off-by: Stevan Buzejic <buzejic.stevan@gmail.com> * Formating of the file fixed Signed-off-by: Stevan Buzejic <buzejic.stevan@gmail.com> Signed-off-by: Stevan Buzejic <buzejic.stevan@gmail.com> * refactored DeleteMonitor Action to be synchronious (#628) (#630) * refactored DeleteMonitor Action to be synchronious Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> Co-authored-by: Petar Dzepina <petar.dzepina@gmail.com> * [Backport 2.x] QueryIndex rollover when field mapping limit is reached (#729) Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> * Mappings fix backport 2.x (#730) * Added support for "nested" mappings (#645) * example Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> * fixed updating mappings for queryIndex Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> * mappings traversal bug fix (#669) Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> * Added unwrapping exception from core; added more debug logs (#728) Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> * Add DataSources test for future backports Signed-off-by: Ashish Agrawal <ashisagr@amazon.com> * fix percolator mapping error when having field name 'type' (#726) Signed-off-by: Raj Chakravarthi <raj@icedome.ca> * [BUG] ExecuteMonitor inserting metadata doc during dry run (#758) * execute monitor bugfix Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> * added IT Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> * fixed created retval when skipIndex=true Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> --------- Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> * fix for ERROR alert state generation in doc-level monitors (#768) Signed-off-by: Subhobrata Dey <sbcd90@gmail.com> * Adjusting max field index setting dynamically for query index (#776) * added adjusting max field index setting dynamicly for query index Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> * Multiple indices support in DocLevelMonitorInput (#784) Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> * Doc transform 2.x backport (#853) * conflict resovle - backport from main to 2.x Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> * fixed module class names Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> --------- Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> * Update config index schema if needed at the start of each monitor execution (#849) * Update config index schema if needed at the start of each monitor execution Signed-off-by: Ashish Agrawal <ashisagr@amazon.com> * Mappings parsing fix (#851) * fixed mappings parsing when field name named "properties" exists in mappings Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> * message typo fix Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> --------- Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> * [Backport 2.x] Notification security fix (#861) * Notification security fix (#852) * added injecting whole user object in threadContext before calling notification APIs so that backend roles are available to notification plugin Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> * compile fix Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> * refactored user_info injection to use InjectSecurity Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> * ktlint fix Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> --------- Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> (cherry picked from commit e0b7a5a) * remove unneeded import Signed-off-by: Ashish Agrawal <ashisagr@amazon.com> --------- Signed-off-by: Ashish Agrawal <ashisagr@amazon.com> Co-authored-by: Petar Dzepina <petar.dzepina@gmail.com> Co-authored-by: Ashish Agrawal <ashisagr@amazon.com> * Fixed a bug that prevented alerts from being generated for doc level monitors that use wildcard characters in index names. (#894) Signed-off-by: AWSHurneyt <hurneyt@amazon.com> * fixed security tests (#484) (#794) * fixed security tests Signed-off-by: Raj Chakravarthi <raj@icedome.ca> (cherry picked from commit c51940f) --------- Signed-off-by: Stevan Buzejic <buzejic.stevan@gmail.com> Signed-off-by: Petar Dzepina <petar.dzepina@gmail.com> Signed-off-by: Ashish Agrawal <ashisagr@amazon.com> Signed-off-by: Raj Chakravarthi <raj@icedome.ca> Signed-off-by: Subhobrata Dey <sbcd90@gmail.com> Signed-off-by: AWSHurneyt <hurneyt@amazon.com> Co-authored-by: Stevan Buzejic <30922513+stevanbz@users.noreply.github.com> Co-authored-by: Surya Sashank Nistala <snistala@amazon.com> Co-authored-by: Petar Dzepina <petar.dzepina@gmail.com> Co-authored-by: RAJ CHAKRAVARTHI <49325334+raj-chak@users.noreply.github.com> Co-authored-by: Subhobrata Dey <sbcd90@gmail.com> Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com> Co-authored-by: AWSHurneyt <hurneyt@amazon.com> Co-authored-by: RAJ CHAKRAVARTHI <raj@icedome.ca>

raj-chak requested a review from a team July 6, 2022 15:33

lezzago requested changes Jul 25, 2022

View reviewed changes

getsaurabh02 requested changes Jul 25, 2022

View reviewed changes

raj-chak added 2 commits August 15, 2022 21:24

fixed security tests

15389bb

Signed-off-by: Raj Chakravarthi <raj@icedome.ca>

changes on comments to PR

72d9a44

Signed-off-by: Raj Chakravarthi <raj@icedome.ca>

raj-chak force-pushed the securitytest branch from 8ed88e2 to 72d9a44 Compare August 15, 2022 19:26

lezzago changed the base branch from main to 2.x November 2, 2022 20:45

lezzago changed the base branch from 2.x to main November 2, 2022 20:45

raj-chak and others added 4 commits January 31, 2023 13:46

Merge branch 'main' into securitytest

d6fd796

indentation issues after resolving conflicts

0b2238b

Signed-off-by: Raj Chakravarthi <raj@icedome.ca>

mock version and wrong import

1ce7e2c

Signed-off-by: Raj Chakravarthi <raj@icedome.ca>

changed SetOnce import to opensearch

a12dd32

Signed-off-by: Raj Chakravarthi <raj@icedome.ca>

lezzago approved these changes Feb 8, 2023

View reviewed changes

lezzago added the backport 2.x label Feb 8, 2023

lezzago mentioned this pull request Feb 8, 2023

secure tests for alerting #773

Closed

getsaurabh02 approved these changes Feb 10, 2023

View reviewed changes

lezzago merged commit c51940f into opensearch-project:main Feb 10, 2023

opensearch-trigger-bot bot mentioned this pull request Feb 10, 2023

[Backport 2.x] fixed security tests #789

Closed

opensearch-trigger-bot bot pushed a commit that referenced this pull request Feb 10, 2023

fixed security tests (#484)

03edb65

* fixed security tests Signed-off-by: Raj Chakravarthi <raj@icedome.ca> (cherry picked from commit c51940f)

lezzago added backport 2.x and removed backport 2.x labels Feb 10, 2023

raj-chak added a commit to raj-chak/alerting that referenced this pull request Feb 14, 2023

fixed security tests (opensearch-project#484)

174955f

* fixed security tests Signed-off-by: Raj Chakravarthi <raj@icedome.ca> (cherry picked from commit c51940f)

AWSHurneyt pushed a commit that referenced this pull request Feb 14, 2023

fixed security tests (#484) (#794)

3531a99

* fixed security tests Signed-off-by: Raj Chakravarthi <raj@icedome.ca> (cherry picked from commit c51940f)

lezzago pushed a commit to lezzago/alerting-opensearch that referenced this pull request Jun 8, 2023

fixed security tests (opensearch-project#484) (opensearch-project#794)

a02b933

* fixed security tests Signed-off-by: Raj Chakravarthi <raj@icedome.ca> (cherry picked from commit c51940f)

lezzago mentioned this pull request Jun 8, 2023

[Backport 2.3] Manual backports of several PRs #960

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixed security tests #484

fixed security tests #484

raj-chak commented Jul 6, 2022

codecov-commenter commented Jul 25, 2022

lezzago Jul 25, 2022

lezzago Jul 25, 2022

getsaurabh02 Jul 25, 2022

lezzago Jul 25, 2022

raj-chak Aug 15, 2022

lezzago Jul 25, 2022

raj-chak Aug 15, 2022

getsaurabh02 Jul 25, 2022

raj-chak Aug 15, 2022

getsaurabh02 Jul 25, 2022

getsaurabh02 Jul 25, 2022

raj-chak Aug 15, 2022

lezzago commented Jan 19, 2023

deshsidd commented Feb 8, 2023

raj-chak commented Feb 8, 2023

getsaurabh02 commented Feb 8, 2023

lezzago commented Feb 10, 2023

opensearch-trigger-bot bot commented Feb 10, 2023

fixed security tests #484

fixed security tests #484

Conversation

raj-chak commented Jul 6, 2022

codecov-commenter commented Jul 25, 2022

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lezzago commented Jan 19, 2023

deshsidd commented Feb 8, 2023

raj-chak commented Feb 8, 2023

getsaurabh02 commented Feb 8, 2023

lezzago commented Feb 10, 2023

opensearch-trigger-bot bot commented Feb 10, 2023