Implements end-to-end integration tests for Authorization in Rest Layer #3226

DarshitChanpura · 2023-08-21T21:17:21Z

Description

This change test Authorization in Rest Layer feature by testing NamedRoutes and legacy Routes. It does so by adding two plugins in security plugin's test framework with one registering NamedRoutes and other registering legacy Routes.

Category: Enhancement

Issues Resolved

Resolves Implement end-to-end testing of the new named routes API and ensure backwards compatibility. #2928

Testing

Integration tests

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.

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

…outes Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura · 2023-08-21T21:53:38Z

~~Not sure why Codecov/project shows 27% decrease when this PR adds more tests.~~

Update: Seems to have resolved on its own.

DarshitChanpura · 2023-08-22T13:56:30Z

Backport is currently blocked by: #3230

stephen-crawford

Great job @DarshitChanpura! Just a few comments.

src/integrationTest/java/org/opensearch/security/rest/AuthZinRestLayerTests.java

src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java

src/integrationTest/java/org/opensearch/test/framework/audit/AuditMessagePredicate.java

...grationTest/java/org/opensearch/test/framework/testplugins/dummy/CustomLegacyTestPlugin.java

...ationTest/java/org/opensearch/test/framework/testplugins/dummy/dummyaction/DummyRequest.java

.../java/org/opensearch/test/framework/testplugins/dummyprotected/dummyaction/DummyRequest.java

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

peternied

Really thorough testing on these components great work. I'm having trouble knowing reading if scenarios are covered or not based on conventions around the naming conventions.

src/integrationTest/java/org/opensearch/security/rest/AuthZinRestLayerTests.java

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

codecov · 2023-08-22T18:34:56Z

Codecov Report

Merging #3226 (fd84d3a) into main (07406d6) will increase coverage by 0.01%.
Report is 1 commits behind head on main.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #3226      +/-   ##
============================================
+ Coverage     62.43%   62.45%   +0.01%     
+ Complexity     3351     3350       -1     
============================================
  Files           254      254              
  Lines         19748    19748              
  Branches       3334     3334              
============================================
+ Hits          12330    12333       +3     
+ Misses         5789     5785       -4     
- Partials       1629     1630       +1

see 3 files with indirect coverage changes

peternied

Reviewed and commented on several test names

src/integrationTest/java/org/opensearch/security/rest/AuthZinRestLayerTests.java

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura · 2023-08-23T00:18:41Z

No sure why the codecov/project/plugin report shows decrease of .01% when upon clicking the link to codecov it shows an increase of .01%

peternied · 2023-08-23T20:54:36Z

No sure why the codecov/project/plugin report shows decrease of .01% when upon clicking the link to codecov it shows an increase of .01%

Congrats you've found another source of non-deterministic test results - src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java

I've updated #3137, and you can update the codecov.yml to prevent false positives if you'd like

peternied

Thanks for the renames - much more approachable!

RyanL1997

Hi @DarshitChanpura, great work for putting the tests together! In generally this is looking good to me. I just left some minor questions and a naming related comment.

RyanL1997 · 2023-08-24T04:57:13Z

src/integrationTest/java/org/opensearch/security/rest/WhoAmITests.java

@@ -179,26 +186,6 @@ private void assertResponse(TestRestClient.HttpResponse response, int expectedSt
        assertThat(response.getBody(), equalTo(expectedBody));
    }

-    private void assertExactlyOneAuthenticatedLogMessage(TestSecurityConfig.User user) {


Just wanna confirm, we are removing this since we are switching to use the audit log for capturing all the log msgs?

RyanL1997 · 2023-08-24T05:01:28Z

src/integrationTest/java/org/opensearch/test/framework/audit/AuditMessagePredicate.java

+    ) {
+        return auditPredicate(category).withLayer(Origin.TRANSPORT)
+            .withEffectiveUser(user)
+            .withPrivilege(privilege)


By reading through the previous comment, I think the privilege is the actual permissions for the user who sends out the request is that correct?

RyanL1997 · 2023-08-24T05:09:33Z

...t/java/org/opensearch/test/framework/testplugins/dummyprotected/dummyaction/DummyAction.java

+public class DummyAction extends ActionType<DummyResponse> {
+
+    public static final DummyAction INSTANCE = new DummyAction();
+    public static final String NAME = "cluster:admin/dummy_protected_plugin/dummy/get";


nit: I assume the NAME is referencing to this dummy permission? so maybe rename it to DUMMY_PERMISSION_NAME?

stephen-crawford

Looks Good To Me!

opensearch-trigger-bot · 2023-08-24T15:35:29Z

The backport to 2.x failed:

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

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/security/backport-2.x
# Create a new branch
git switch --create backport/backport-3226-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 9a7e517abd9d4d0e301f918adc65cb82b8f14d88
# Push it to GitHub
git push --set-upstream origin backport/backport-3226-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-2.x

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

peternied · 2023-08-31T21:07:14Z

@DarshitChanpura Can you make sure to include this in the everything at once backport for AuthZ in the Rest layer?

DarshitChanpura added 10 commits August 18, 2023 16:54

Base setup for dummy plugin for E2E testing

52b980e

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Adds a plugin that has protected routes

022e663

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Adds a plugin that is a legacy plugin which does not have protected r…

6a1add9

…outes Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Adds an abstract base rest handler for dummy plugins

214d276

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Adds a few tests for E2E testing

7a68bec

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Refactors plugins to be more readable

0d6ba84

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Adds two reusable methods in AuditMessagePredicate

bc87ad5

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Adds final changes to AuthZ end-to-end tests

65d53da

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

Merge remote-tracking branch 'upstream/main' into end-to-end-testing

220c4b5

Fixes WhoAmITests to be a little cleaner

eb61c9e

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura requested review from cliu123, cwperks, davidlago, peternied, RyanL1997, stephen-crawford, reta and willyborankin as code owners August 21, 2023 21:17

Fixes license headers

8c0cd13

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura mentioned this pull request Aug 21, 2023

Implement end-to-end testing of the new named routes API and ensure backwards compatibility. #2928

Closed

3 tasks

Merge branch 'main' into end-to-end-testing

8c9b9cb

DarshitChanpura added the backport 2.x backport to 2.x branch label Aug 22, 2023

stephen-crawford reviewed Aug 22, 2023

View reviewed changes

Cleans up code and addresses PR feedback

d6b69f9

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

peternied requested changes Aug 22, 2023

View reviewed changes

src/integrationTest/java/org/opensearch/security/rest/AuthZinRestLayerTests.java Outdated Show resolved Hide resolved

src/integrationTest/java/org/opensearch/security/rest/AuthZinRestLayerTests.java Outdated Show resolved Hide resolved

Renames users in Authz tests to be more intuitive

cbbbf27

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

peternied reviewed Aug 22, 2023

View reviewed changes

Renames tests to be more readable

fd84d3a

Signed-off-by: Darshit Chanpura <dchanp@amazon.com>

DarshitChanpura mentioned this pull request Aug 23, 2023

Add test that confirms when Rest and Transport authorization checks overlap #3112

Closed

4 tasks

peternied approved these changes Aug 23, 2023

View reviewed changes

RyanL1997 reviewed Aug 24, 2023

View reviewed changes

stephen-crawford approved these changes Aug 24, 2023

View reviewed changes

stephen-crawford merged commit 9a7e517 into opensearch-project:main Aug 24, 2023

opensearch-trigger-bot bot added the backport-failed label Aug 24, 2023

peternied removed backport 2.x backport to 2.x branch backport-failed labels Aug 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements end-to-end integration tests for Authorization in Rest Layer #3226

Implements end-to-end integration tests for Authorization in Rest Layer #3226

DarshitChanpura commented Aug 21, 2023

DarshitChanpura commented Aug 21, 2023 •

edited

Loading

DarshitChanpura commented Aug 22, 2023

stephen-crawford left a comment

peternied left a comment

codecov bot commented Aug 22, 2023 •

edited

Loading

peternied left a comment

DarshitChanpura commented Aug 23, 2023 •

edited

Loading

peternied commented Aug 23, 2023

peternied left a comment

RyanL1997 left a comment

RyanL1997 Aug 24, 2023

RyanL1997 Aug 24, 2023

RyanL1997 Aug 24, 2023

stephen-crawford left a comment

opensearch-trigger-bot bot commented Aug 24, 2023

peternied commented Aug 31, 2023

Implements end-to-end integration tests for Authorization in Rest Layer #3226

Implements end-to-end integration tests for Authorization in Rest Layer #3226

Conversation

DarshitChanpura commented Aug 21, 2023

Description

Issues Resolved

Testing

Check List

DarshitChanpura commented Aug 21, 2023 • edited Loading

DarshitChanpura commented Aug 22, 2023

stephen-crawford left a comment

Choose a reason for hiding this comment

peternied left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 22, 2023 • edited Loading

Codecov Report

peternied left a comment

Choose a reason for hiding this comment

DarshitChanpura commented Aug 23, 2023 • edited Loading

peternied commented Aug 23, 2023

peternied left a comment

Choose a reason for hiding this comment

RyanL1997 left a comment

Choose a reason for hiding this comment

RyanL1997 Aug 24, 2023

Choose a reason for hiding this comment

RyanL1997 Aug 24, 2023

Choose a reason for hiding this comment

RyanL1997 Aug 24, 2023

Choose a reason for hiding this comment

stephen-crawford left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Aug 24, 2023

peternied commented Aug 31, 2023

DarshitChanpura commented Aug 21, 2023 •

edited

Loading

codecov bot commented Aug 22, 2023 •

edited

Loading

DarshitChanpura commented Aug 23, 2023 •

edited

Loading