Cluster permissions evaluation logic will now include `index_template` type action #1885

DarshitChanpura · 2022-06-13T19:59:01Z

Description

Title

Category : Bug Fix

Issues Resolved

Resolves [BUG] index_template action type is not part of cluster-permissions check even though template is. #1884

Testing

A test is written to evaluate this

Check List

New functionality includes testing
~~- [ ] New functionality has been documented~~
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.

DarshitChanpura · 2022-06-14T02:21:09Z

@opensearch-project/security Seems like the test failures are related to audit-log changes. Need some help fixing this.

cliu123 · 2022-06-14T06:00:35Z

@DarshitChanpura CI failed. Would you please take a look?

src/test/java/org/opensearch/security/IndexTemplateClusterPermissionsCheckTest.java

DarshitChanpura · 2022-06-14T15:20:54Z

@DarshitChanpura CI failed. Would you please take a look?

It failed due to errors unrelated to the changes in this PR, example look here. Have we seen this error before?

davidlago · 2022-06-14T15:24:21Z

src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java

-        } else {
+        } else if (request instanceof PutComponentTemplateAction.Request) {
+            // do nothing
+        }else {


nitpick: space before else (do we have a linting rule for this?)

davidlago · 2022-06-14T15:26:41Z

Could you please add a description to either this PR or the associated issue explaining what the implications of index_template not being initially included? i.e., what is the behavior change after fixing this issue?

DarshitChanpura · 2022-06-14T15:43:19Z

Could you please add a description to either this PR or the associated issue explaining what the implications of index_template not being initially included? i.e., what is the behavior change after fixing this issue?

done... description added to the referenced issue

src/test/java/org/opensearch/security/test/helper/rest/RestHelper.java

codecov-commenter · 2022-06-14T20:43:36Z

Codecov Report

Merging #1885 (260f7bb) into main (001d73f) will increase coverage by 0.00%.
The diff coverage is 50.00%.

@@            Coverage Diff            @@
##               main    #1885   +/-   ##
=========================================
  Coverage     61.01%   61.02%           
  Complexity     3232     3232           
=========================================
  Files           256      256           
  Lines         18085    18087    +2     
  Branches       3222     3224    +2     
=========================================
+ Hits          11034    11037    +3     
+ Misses         5469     5466    -3     
- Partials       1582     1584    +2

Impacted Files	Coverage Δ
...earch/security/resolver/IndexResolverReplacer.java	`65.34% <0.00%> (-0.19%)`	⬇️
...earch/security/privileges/PrivilegesEvaluator.java	`72.02% <100.00%> (+0.09%)`	⬆️
...security/auditlog/sink/ExternalOpenSearchSink.java	`59.25% <0.00%> (-2.47%)`	⬇️
...nsearch/security/dlic/rest/api/AuditApiAction.java	`68.08% <0.00%> (+4.25%)`	⬆️
...ecurity/configuration/StaticResourceException.java	`25.00% <0.00%> (+25.00%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 001d73f...260f7bb. Read the comment docs.

peternied · 2022-06-14T19:11:42Z

src/test/java/org/opensearch/security/test/helper/rest/RestHelper.java

-					final String pathEntry = jsonPathScanner.next();
+					String pathEntry = jsonPathScanner.next();
+					// if pathEntry is an array lookup
+					boolean isArrayLookup = pathEntry.contains("[");


Much cleaner to use a regex, here is an example https://ideone.com/wEcFot

'test[12]' Name was: test, index position: 12 'result[2]' Name was: result, index position: 2 'res[2da]' No Match '[a2da]' No Match 'sitename' No Match

If you have a match, immediately assign currentNode = currentNode.get(arrayEntryIdx); and then call continue to go back to the top of the loop

good find.. this is much better.. Slight change, I can't immediately continue once I find the match, as I first have to check whether the current node on the requested path is an array and then try to get the request index.

…te permission type Signed-off-by: Darshit Chanpura <[email protected]>

…mission and fixes existing broken tests Signed-off-by: Darshit Chanpura <[email protected]>

peternied · 2022-06-15T17:36:23Z

Great work @DarshitChanpura

…endistro-1.11 Signed-off-by: Rutuja Surve <[email protected]> (cherry picked from commit 702c81a)

…endistro-1.10 Signed-off-by: Rutuja Surve <[email protected]> (cherry picked from commit 702c81a)

…endistro-1.2 Signed-off-by: Rutuja Surve <[email protected]> (cherry picked from commit 702c81a)

…endistro-1.11 Signed-off-by: Rutuja Surve <[email protected]> (cherry picked from commit 702c81a)

Signed-off-by: Rutuja Surve <[email protected]> (cherry picked from commit 702c81a)

Signed-off-by: Rutuja Surve <[email protected]>

… to opendistro-1.11 (opensearch-project#1986)" This reverts commit ec80578.

… to opendistro-1.11 (opensearch-project#1986)" This reverts commit ec80578. Signed-off-by: Rutuja Surve <[email protected]>

Signed-off-by: Rutuja Surve <[email protected]>

…ch-project#1885 Signed-off-by: Rutuja Surve <[email protected]>

…project#1885 Signed-off-by: Rutuja Surve <[email protected]>

…` type action (opensearch-project#1885) * Updates the cluster permission check function to include index_template permission type Signed-off-by: Darshit Chanpura <[email protected]> Signed-off-by: Stephen Crawford <[email protected]>

…` type action (opensearch-project#1885) * Updates the cluster permission check function to include index_template permission type Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura requested a review from a team June 13, 2022 19:59

DarshitChanpura force-pushed the cluster-perm-check branch 3 times, most recently from e5c6b46 to eb73447 Compare June 13, 2022 20:11

cliu123 previously approved these changes Jun 14, 2022

View reviewed changes

peternied reviewed Jun 14, 2022

View reviewed changes

src/test/java/org/opensearch/security/IndexTemplateClusterPermissionsCheckTest.java Outdated Show resolved Hide resolved

src/test/java/org/opensearch/security/IndexTemplateClusterPermissionsCheckTest.java Show resolved Hide resolved

davidlago reviewed Jun 14, 2022

View reviewed changes

DarshitChanpura dismissed cliu123’s stale review via 6c8cfdc June 14, 2022 15:25

DarshitChanpura force-pushed the cluster-perm-check branch from eb73447 to 6c8cfdc Compare June 14, 2022 15:25

DarshitChanpura force-pushed the cluster-perm-check branch from 6c8cfdc to f2ea0c2 Compare June 14, 2022 15:28

DarshitChanpura force-pushed the cluster-perm-check branch 2 times, most recently from 6d92a32 to cbc53e5 Compare June 14, 2022 17:56

cliu123 reviewed Jun 14, 2022

View reviewed changes

src/test/java/org/opensearch/security/test/helper/rest/RestHelper.java Outdated Show resolved Hide resolved

DarshitChanpura force-pushed the cluster-perm-check branch from 827286e to 260f7bb Compare June 14, 2022 20:11

cliu123 previously approved these changes Jun 14, 2022

View reviewed changes

peternied requested changes Jun 15, 2022

View reviewed changes

DarshitChanpura added 2 commits June 15, 2022 12:55

Updates the cluster permission check function to include index_templa…

3bef20c

…te permission type Signed-off-by: Darshit Chanpura <[email protected]>

Adds test to verify that index-template update is a cluster level per…

c91015d

…mission and fixes existing broken tests Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura dismissed cliu123’s stale review via c91015d June 15, 2022 16:55

DarshitChanpura force-pushed the cluster-perm-check branch from 260f7bb to c91015d Compare June 15, 2022 16:55

cliu123 approved these changes Jun 15, 2022

View reviewed changes

peternied approved these changes Jun 15, 2022

View reviewed changes

peternied merged commit e40d935 into opensearch-project:main Jun 15, 2022

rutuja-amazon mentioned this pull request Aug 2, 2022

backport cluster evaluation permissions #1885 to opendistro-1.12 #1985

Merged

3 tasks

rutuja-amazon added a commit to rutuja-amazon/security that referenced this pull request Aug 2, 2022

backport cluster evaluation permissions opensearch-project#1885 to op…

b90c262

…endistro-1.11 Signed-off-by: Rutuja Surve <[email protected]> (cherry picked from commit 702c81a)

rutuja-amazon mentioned this pull request Aug 2, 2022

backport cluster evaluation permissions #1885 to opendistro-1.11 #1986

Merged

3 tasks

rutuja-amazon added a commit to rutuja-amazon/security that referenced this pull request Aug 2, 2022

backport cluster evaluation permissions opensearch-project#1885 to op…

4f64435

…endistro-1.10 Signed-off-by: Rutuja Surve <[email protected]> (cherry picked from commit 702c81a)

rutuja-amazon mentioned this pull request Aug 2, 2022

backport cluster evaluation permissions #1885 to opendistro-1.10 #1987

Merged

3 tasks

rutuja-amazon added a commit to rutuja-amazon/security that referenced this pull request Aug 2, 2022

backport cluster evaluation permissions opensearch-project#1885 to op…

4f9ddbb

…endistro-1.2 Signed-off-by: Rutuja Surve <[email protected]> (cherry picked from commit 702c81a)

rutuja-amazon added a commit to rutuja-amazon/security that referenced this pull request Aug 2, 2022

backport cluster evaluation permissions opensearch-project#1885 to op…

b5b6305

…endistro-1.2 Signed-off-by: Rutuja Surve <[email protected]> (cherry picked from commit 702c81a)

rutuja-amazon added a commit to rutuja-amazon/security that referenced this pull request Aug 2, 2022

backport cluster evaluation permissions opensearch-project#1885 to op…

35973ec

…endistro-1.11 Signed-off-by: Rutuja Surve <[email protected]> (cherry picked from commit 702c81a)

peternied pushed a commit that referenced this pull request Aug 8, 2022

backport cluster evaluation permissions #1885 to opendistro-1.10 (#1987)

ca12873

Signed-off-by: Rutuja Surve <[email protected]> (cherry picked from commit 702c81a)

peternied pushed a commit that referenced this pull request Aug 8, 2022

backport cluster evaluation permissions #1885 to opendistro-1.11 (#1986)

ec80578

Signed-off-by: Rutuja Surve <[email protected]> (cherry picked from commit 702c81a)

peternied pushed a commit that referenced this pull request Aug 8, 2022

backport cluster evaluation permissions #1885 to opendistro-1.2 (#1985)

434f237

Signed-off-by: Rutuja Surve <[email protected]> (cherry picked from commit 702c81a)

rutuja-amazon added a commit to rutuja-amazon/security that referenced this pull request Aug 9, 2022

Revert backport of opensearch-project#1885

3bebce1

Signed-off-by: Rutuja Surve <[email protected]>

rutuja-amazon added a commit to rutuja-amazon/security that referenced this pull request Aug 10, 2022

rollback changes for backport opensearch-project#1885

9d9c43d

Signed-off-by: Rutuja Surve <[email protected]>

rutuja-amazon mentioned this pull request Aug 10, 2022

rollback changes for backport #1885 #2008

Closed

rutuja-amazon added a commit to rutuja-amazon/security that referenced this pull request Aug 10, 2022

Revert "backport cluster evaluation permissions opensearch-project#1885…

fda4ef8

… to opendistro-1.11 (opensearch-project#1986)" This reverts commit ec80578.

rutuja-amazon mentioned this pull request Aug 10, 2022

Revert "backport cluster evaluation permissions #1885 to opendistro-1.11" #2009

Closed

3 tasks

rutuja-amazon added a commit to rutuja-amazon/security that referenced this pull request Aug 10, 2022

fix resthelper for backport of opensearch-project#1885

b88e764

Signed-off-by: Rutuja Surve <[email protected]>

rutuja-amazon mentioned this pull request Aug 10, 2022

Fixes RestHelper and IndexTemplateClusterPermissionsCheckTest for backport of #1885 : opendistro-1.11 #2011

Closed

3 tasks

rutuja-amazon added a commit to rutuja-amazon/security that referenced this pull request Aug 10, 2022

fix resthelper and indextemplatecluster test for backport of opensear…

d6911e3

…ch-project#1885 Signed-off-by: Rutuja Surve <[email protected]>

rutuja-amazon added a commit to rutuja-amazon/security that referenced this pull request Aug 10, 2022

fix resthelper and indextemplatecluster test for backport opensearch-…

0754d1d

…project#1885 Signed-off-by: Rutuja Surve <[email protected]>

rutuja-amazon mentioned this pull request Aug 10, 2022

Fixes RestHelper and IndexTemplateClusterPermissionsCheckTest for backport of #1885 : opendistro-1.13 #2012

Closed

3 tasks

rutuja-amazon added a commit to rutuja-amazon/security that referenced this pull request Aug 10, 2022

fix resthelper and indextemplatecluster test for backport opensearch-…

f1be1c6

…project#1885 Signed-off-by: Rutuja Surve <[email protected]>

rutuja-amazon mentioned this pull request Aug 10, 2022

Fixes RestHelper and IndexTemplateCluster test for backport #1885: opendistro-1.12 #2013

Closed

3 tasks

rutuja-amazon added a commit to rutuja-amazon/security that referenced this pull request Aug 10, 2022

fix resthelper and indextemplatecluster test for backport opensearch-…

73b41a2

…project#1885 Signed-off-by: Rutuja Surve <[email protected]>

rutuja-amazon mentioned this pull request Aug 10, 2022

Fixes RestHelper and IndexTemplateClusterPermissionsCheckTest for backport of #1885 : opendistro-1.10 #2014

Closed

3 tasks

rutuja-amazon added a commit to rutuja-amazon/security that referenced this pull request Aug 10, 2022

fix resthelper and indextemplatecluster test for backport opensearch-…

b0758c0

…project#1885 Signed-off-by: Rutuja Surve <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cluster permissions evaluation logic will now include `index_template` type action #1885

Cluster permissions evaluation logic will now include `index_template` type action #1885

DarshitChanpura commented Jun 13, 2022 •

edited

Loading

DarshitChanpura commented Jun 14, 2022

cliu123 commented Jun 14, 2022

DarshitChanpura commented Jun 14, 2022

davidlago Jun 14, 2022

DarshitChanpura Jun 14, 2022

davidlago commented Jun 14, 2022

DarshitChanpura commented Jun 14, 2022 •

edited

Loading

codecov-commenter commented Jun 14, 2022

peternied Jun 14, 2022

DarshitChanpura Jun 15, 2022

peternied commented Jun 15, 2022

Cluster permissions evaluation logic will now include index_template type action #1885

Cluster permissions evaluation logic will now include index_template type action #1885

Conversation

DarshitChanpura commented Jun 13, 2022 • edited Loading

Description

Issues Resolved

Testing

Check List

DarshitChanpura commented Jun 14, 2022

cliu123 commented Jun 14, 2022

DarshitChanpura commented Jun 14, 2022

davidlago Jun 14, 2022

Choose a reason for hiding this comment

DarshitChanpura Jun 14, 2022

Choose a reason for hiding this comment

davidlago commented Jun 14, 2022

DarshitChanpura commented Jun 14, 2022 • edited Loading

codecov-commenter commented Jun 14, 2022

Codecov Report

peternied Jun 14, 2022

Choose a reason for hiding this comment

DarshitChanpura Jun 15, 2022

Choose a reason for hiding this comment

peternied commented Jun 15, 2022

Cluster permissions evaluation logic will now include `index_template` type action #1885

Cluster permissions evaluation logic will now include `index_template` type action #1885

DarshitChanpura commented Jun 13, 2022 •

edited

Loading

DarshitChanpura commented Jun 14, 2022 •

edited

Loading