Clean up REST API (Part 1) #2900

willyborankin · 2023-06-23T15:20:28Z

Description

The aim of this PR is to start cleaning code in REST API since with the current implementation is difficult to understand and support.

Changes:

Implemented new RequestConetnValidator class which uses the same validation logic as AbstractConfigurationValidator
Removed all redundant AbstractConfigurationValidator extensions

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

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.

codecov · 2023-06-23T15:54:00Z

Codecov Report

Merging #2900 (a336e88) into main (8063e1b) will increase coverage by 0.05%.
The diff coverage is 82.89%.

@@             Coverage Diff              @@
##               main    #2900      +/-   ##
============================================
+ Coverage     62.38%   62.44%   +0.05%     
+ Complexity     3379     3351      -28     
============================================
  Files           267      254      -13     
  Lines         19774    19735      -39     
  Branches       3355     3334      -21     
============================================
- Hits          12336    12323      -13     
+ Misses         5797     5779      -18     
+ Partials       1641     1633       -8

Files Changed	Coverage Δ
...curity/dlic/rest/api/AuthTokenProcessorAction.java	`53.33% <0.00%> (ø)`
...security/dlic/rest/api/SecuritySSLCertsAction.java	`66.66% <ø> (ø)`
...earch/security/dlic/rest/api/TenantsApiAction.java	`27.77% <0.00%> (-13.89%)`	⬇️
...arch/security/dlic/rest/api/ValidateApiAction.java	`9.09% <0.00%> (-0.91%)`	⬇️
...earch/security/dlic/rest/api/MigrateApiAction.java	`4.71% <10.00%> (+0.90%)`	⬆️
...ch/security/dlic/rest/api/FlushCacheApiAction.java	`63.63% <33.33%> (+1.13%)`	⬆️
...nsearch/security/dlic/rest/api/RolesApiAction.java	`77.27% <70.00%> (-16.07%)`	⬇️
...rch/security/dlic/rest/api/AllowlistApiAction.java	`73.68% <75.00%> (-0.61%)`	⬇️
...h/security/dlic/rest/api/SecurityConfigAction.java	`75.00% <75.00%> (-0.76%)`	⬇️
...earch/security/dlic/rest/api/NodesDnApiAction.java	`86.00% <80.00%> (-0.96%)`	⬇️
... and 11 more

... and 6 files with indirect coverage changes

stephen-crawford

Hi @willyborankin looks pretty good. I left a couple nitpicky things but I also wanted to know what you felt was the best way to handle tests for the validator you added. Theoretically, it is always tested by everything that uses it, but perhaps we can add some explicit unit tests for the RequestContentValidator you added.

In theory, I think something could break and we would not have as straightforward of a time isolating the changes to the Validator as we could if you added some explicit tests on the class itself.

Let me know what you think.

src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java

src/main/java/org/opensearch/security/dlic/rest/api/AccountApiAction.java

src/main/java/org/opensearch/security/dlic/rest/api/AuthTokenProcessorAction.java

src/main/java/org/opensearch/security/dlic/rest/api/PatchableResourceApiAction.java

src/main/java/org/opensearch/security/dlic/rest/api/ValidateApiAction.java

src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java

willyborankin · 2023-06-27T12:28:18Z

Hi @willyborankin looks pretty good. I left a couple nitpicky things but I also wanted to know what you felt was the best way to handle tests for the validator you added. Theoretically, it is always tested by everything that uses it, but perhaps we can add some explicit unit tests for the RequestContentValidator you added.

In theory, I think something could break and we would not have as straightforward of a time isolating the changes to the Validator as we could if you added some explicit tests on the class itself.

Let me know what you think.

Agree I added a new RequestContentValidatorTest test

stephen-crawford · 2023-07-03T20:44:07Z

@opensearch-project/security can we get another review here to move this PR forward? Thank you!

stephen-crawford · 2023-07-07T14:10:37Z

@opensearch-project/security bumping this PR again for a second review.

src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java

cwperks

Thank you @willyborankin. This is cleaner than the existing implementation and I'm looking forward to see what Part II entails. I took a first pass and left a few comments.

cwperks · 2023-07-10T13:38:18Z

src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java

            switch (request.method()) {
                case DELETE:
-                    handleDelete(channel, request, client, validator.getContentAsNode());
+                    handleDelete(channel, request, client, null);


Do handleDelete and handleGet use the JsonNode parameter anywhere?

They are not, I intentionally left them as is, the next PR will change this logic

cwperks · 2023-07-10T13:42:28Z

src/main/java/org/opensearch/security/dlic/rest/api/AccountApiAction.java

+            }
+
+            @Override
+            public Map<String, RequestContentValidator.DataType> allowedKeys() {


This is much cleaner than now. Ultimately, I'd like to see this defined with an OpenAPI spec like syntax one day

cwperks · 2023-07-10T13:43:55Z

src/main/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiAction.java

+                }
+                allowedKeys.put("allowed_actions", DataType.ARRAY);
+                allowedKeys.put("description", DataType.STRING);
+                allowedKeys.put("type", DataType.STRING);


String is overly generic here. Is there an enum for type (i.e. INDEX, CLUSTER) somewhere?

Good point! There are no enums or constants except MutitenecyActionApi. I can extract all of them as enums, and some of them have the same name in the next small PR.

cwperks · 2023-07-10T13:44:53Z

src/main/java/org/opensearch/security/dlic/rest/api/ActionGroupsApiAction.java

+                if (isSuperAdmin()) {
+                    allowedKeys.put("reserved", DataType.BOOLEAN);
+                }
+                allowedKeys.put("allowed_actions", DataType.ARRAY);


Does this go beyond defining an array? i.e. an array of action names (strings) vs an array of booleans

No it doesn't and same for DataType.OBJECT. I noticed it as well.

cwperks · 2023-07-10T13:47:11Z

src/main/java/org/opensearch/security/dlic/rest/api/RolesMappingApiAction.java

+            }
+
+            @Override
+            public Set<String> mandatoryOrKeys() {


Nice! I like this vs writing this as an if statement checking if at least one is nonNull.

cwperks · 2023-07-10T13:54:51Z

src/main/java/org/opensearch/security/dlic/rest/validation/RequestContentValidator.java

+        requestedKeys.removeAll(allowed);
+        invalidKeys.addAll(requestedKeys);
+        if (!missingMandatoryKeys.isEmpty() || !invalidKeys.isEmpty() || !missingMandatoryOrKeys.isEmpty()) {
+            this.validationError = ValidationError.INVALID_CONFIGURATION;


nit: Is CONFIGURATION the right term to use here? wdyt INVALID_PAYLOAD?

Would this message be able to be descriptive around what is invalid about the payload when responding to the caller? i.e. This request is missing required keys key1, key2, key3?

nit: Is CONFIGURATION the right term to use here? wdyt INVALID_PAYLOAD?

INVALID_PAYLOAD looks better agree originally it is INVALID_CONFIGURATION.

Would this message be able to be descriptive around what is invalid about the payload when responding to the caller? i.e. This request is missing required keys key1, key2, key3?

yes the response looks something like:

{ "status": "error" "reason": "", "invalid_keys": {"keys":"a,b,c"}, "missing_mandatory_keys": {"keys": "d,e,f"}, "specify_one_of": {"keys": "g,h"} }

willyborankin · 2023-07-18T12:11:41Z

Rebased with main

cwperks · 2023-07-18T14:24:40Z

@willyborankin Please add the backport 2.x label if you want this backported to 2.x for an upcoming release.

willyborankin · 2023-07-18T14:50:37Z

@willyborankin Please add the backport 2.x label if you want this backported to 2.x for an upcoming release.

@scrawfor99 Let's release it in 2.10; there's no rush.

Removed redundant classes for the request content validation Signed-off-by: Andrey Pleskach <[email protected]>

Request content validator code cleaning Signed-off-by: Andrey Pleskach <[email protected]>

opensearch-trigger-bot · 2023-07-27T15:27:09Z

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-2900-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 6bac470df67b16a22f496b052776e7afdd50e900
# Push it to GitHub
git push --set-upstream origin backport/backport-2900-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-2900-to-2.x.

The aim of this PR is to start cleaning code in REST API since with the current implementation is difficult to understand and support. Changes: - Implemented new `RequestConetnValidator` class which uses the same validation logic as `AbstractConfigurationValidator` - Removed all redundant `AbstractConfigurationValidator` extensions [Please provide details of testing done: unit testing, integration testing and manual testing] - [ ] 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](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin). --------- Signed-off-by: Andrey Pleskach <[email protected]> (cherry picked from commit 6bac470)

The aim of this PR is to start cleaning code in REST API since with the current implementation is difficult to understand and support. Changes: - Implemented new `RequestConetnValidator` class which uses the same validation logic as `AbstractConfigurationValidator` - Removed all redundant `AbstractConfigurationValidator` extensions [Please provide details of testing done: unit testing, integration testing and manual testing] - [ ] 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](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin). --------- Signed-off-by: Andrey Pleskach <[email protected]> (cherry picked from commit 6bac470) Signed-off-by: Andrey Pleskach <[email protected]>

@willyborankin

### Description Reacting to changes from the backport of opensearch-project/OpenSearch#9262. This reason the main branch of Security is not encountering these errors is because the affected class was removed in #2900 and the change has not been backported yet. @willyborankin are you planning to backport #2900? * Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation) Maintenance ### 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](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin). Signed-off-by: Craig Perkins <[email protected]>

The aim of this PR is to start cleaning code in REST API since with the current implementation is difficult to understand and support. Changes: - Implemented new `RequestConetnValidator` class which uses the same validation logic as `AbstractConfigurationValidator` - Removed all redundant `AbstractConfigurationValidator` extensions [Please provide details of testing done: unit testing, integration testing and manual testing] - [ ] 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](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin). --------- Signed-off-by: Andrey Pleskach <[email protected]> (cherry picked from commit 6bac470) Signed-off-by: Andrey Pleskach <[email protected]>

The aim of this PR is to start cleaning code in REST API since with the current implementation is difficult to understand and support. Changes: - Implemented new `RequestConetnValidator` class which uses the same validation logic as `AbstractConfigurationValidator` - Removed all redundant `AbstractConfigurationValidator` extensions Signed-off-by: Andrey Pleskach <[email protected]> (cherry picked from commit 6bac470) Signed-off-by: Andrey Pleskach <[email protected]>

The aim of this PR is to start cleaning code in REST API since with the current implementation is difficult to understand and support. Changes: - Implemented new `RequestConetnValidator` class which uses the same validation logic as `AbstractConfigurationValidator` - Removed all redundant `AbstractConfigurationValidator` extensions Signed-off-by: Andrey Pleskach <[email protected]> (cherry picked from commit 6bac470) Signed-off-by: Andrey Pleskach <[email protected]> Signed-off-by: Andrey Pleskach <[email protected]>

Manual backport 6bac470 from #2900 Signed-off-by: Andrey Pleskach <[email protected]>

willyborankin requested review from cliu123, cwperks, DarshitChanpura, davidlago, peternied, RyanL1997, stephen-crawford and reta as code owners June 23, 2023 15:20

willyborankin force-pushed the clean-up-rest-api-classes branch from 4eee5a7 to cfb1543 Compare June 23, 2023 15:29

willyborankin changed the title ~~Clean up rest api classes (Part 1)~~ Clean up REST API (Part 1) Jun 24, 2023

willyborankin mentioned this pull request Jun 26, 2023

Misc changes #2902

Merged

3 tasks

stephen-crawford reviewed Jun 26, 2023

View reviewed changes

willyborankin force-pushed the clean-up-rest-api-classes branch 2 times, most recently from 061d2da to 1441831 Compare June 27, 2023 12:16

willyborankin force-pushed the clean-up-rest-api-classes branch 4 times, most recently from 845dabe to a1a46e2 Compare July 1, 2023 17:50

stephen-crawford previously approved these changes Jul 3, 2023

View reviewed changes

willyborankin dismissed stephen-crawford’s stale review via 80d3e61 July 8, 2023 20:06

willyborankin force-pushed the clean-up-rest-api-classes branch from a1a46e2 to 80d3e61 Compare July 8, 2023 20:06

stephen-crawford previously approved these changes Jul 10, 2023

View reviewed changes

src/main/java/org/opensearch/security/dlic/rest/api/AbstractApiAction.java Show resolved Hide resolved

cwperks reviewed Jul 10, 2023

View reviewed changes

willyborankin dismissed stephen-crawford’s stale review via d19308f July 18, 2023 10:21

willyborankin force-pushed the clean-up-rest-api-classes branch 2 times, most recently from d19308f to a816aac Compare July 18, 2023 10:46

cwperks approved these changes Jul 18, 2023

View reviewed changes

willyborankin force-pushed the clean-up-rest-api-classes branch from a816aac to 1c45c0c Compare July 21, 2023 15:08

willyborankin added 2 commits July 26, 2023 08:29

REST API code cleaning remove classes

4ef9bcc

Removed redundant classes for the request content validation Signed-off-by: Andrey Pleskach <[email protected]>

REST API code cleaning

a336e88

Request content validator code cleaning Signed-off-by: Andrey Pleskach <[email protected]>

willyborankin force-pushed the clean-up-rest-api-classes branch from 1c45c0c to a336e88 Compare July 26, 2023 06:40

stephen-crawford approved these changes Jul 27, 2023

View reviewed changes

stephen-crawford merged commit 6bac470 into opensearch-project:main Jul 27, 2023

stephen-crawford added the backport 2.x backport to 2.x branch label Jul 27, 2023

peternied mentioned this pull request Aug 15, 2023

[Backport 2.x] React to changes in ActionListener in core #3182

Merged

3 tasks

This was referenced Aug 21, 2023

[2.x] Add [2.x] to dependabot commit message #3219

Merged

[2.x] React to Compressor changes from core from #9262 #3221

Merged

willyborankin mentioned this pull request Aug 21, 2023

[Backport 2.x] Clean up REST API (Part 1) (#2900) #3223

Merged

cwperks pushed a commit that referenced this pull request Aug 21, 2023

[Backport 2.x] Clean up REST API (Part 1) (#2900) (#3223)

1bb6df5

Manual backport 6bac470 from #2900 Signed-off-by: Andrey Pleskach <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up REST API (Part 1) #2900

Clean up REST API (Part 1) #2900

willyborankin commented Jun 23, 2023 •

edited

Loading

codecov bot commented Jun 23, 2023 •

edited

Loading

stephen-crawford left a comment

willyborankin commented Jun 27, 2023

stephen-crawford commented Jul 3, 2023

stephen-crawford commented Jul 7, 2023

cwperks left a comment

cwperks Jul 10, 2023

willyborankin Jul 11, 2023

cwperks Jul 10, 2023

cwperks Jul 10, 2023

willyborankin Jul 11, 2023

cwperks Jul 10, 2023

willyborankin Jul 11, 2023 •

edited

Loading

cwperks Jul 10, 2023

cwperks Jul 10, 2023

willyborankin Jul 11, 2023 •

edited

Loading

willyborankin commented Jul 18, 2023

cwperks commented Jul 18, 2023

willyborankin commented Jul 18, 2023

opensearch-trigger-bot bot commented Jul 27, 2023

Clean up REST API (Part 1) #2900

Clean up REST API (Part 1) #2900

Conversation

willyborankin commented Jun 23, 2023 • edited Loading

Description

Testing

Check List

codecov bot commented Jun 23, 2023 • edited Loading

Codecov Report

stephen-crawford left a comment

Choose a reason for hiding this comment

willyborankin commented Jun 27, 2023

stephen-crawford commented Jul 3, 2023

stephen-crawford commented Jul 7, 2023

cwperks left a 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

willyborankin Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willyborankin Jul 11, 2023 • edited Loading

Choose a reason for hiding this comment

willyborankin commented Jul 18, 2023

cwperks commented Jul 18, 2023

willyborankin commented Jul 18, 2023

opensearch-trigger-bot bot commented Jul 27, 2023

willyborankin commented Jun 23, 2023 •

edited

Loading

codecov bot commented Jun 23, 2023 •

edited

Loading

willyborankin Jul 11, 2023 •

edited

Loading

willyborankin Jul 11, 2023 •

edited

Loading