[Refactor] Remove json-path from deps and use JsonPointer instead #3262

willyborankin · 2023-08-29T17:40:44Z

Description

After RFC 6901 was introduced
and the implementation was added to Jackson,
there is no need to keep the com.jayway.jsonpath:json-path library in our source code,
so we can replace current validation with Jackson's JsonPointer class.

Besides added missing tests for:

RoleRequestContentValidator
AuditRequestContentValidator

Issues Resolved

#3245

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-08-29T17:44:35Z

Codecov Report

Merging #3262 (385061f) into main (0338cdd) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #3262      +/-   ##
============================================
+ Coverage     63.15%   63.23%   +0.07%     
- Complexity     3448     3451       +3     
============================================
  Files           263      263              
  Lines         20024    20026       +2     
  Branches       3341     3339       -2     
============================================
+ Hits          12647    12664      +17     
+ Misses         5748     5737      -11     
+ Partials       1629     1625       -4

Files Changed	Coverage Δ
...nsearch/security/dlic/rest/api/AuditApiAction.java	`91.17% <100.00%> (ø)`
...nsearch/security/dlic/rest/api/RolesApiAction.java	`95.83% <100.00%> (+15.39%)`	⬆️

... and 2 files with indirect coverage changes

cwperks · 2023-08-29T18:28:39Z

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

@@ -137,7 +136,7 @@ public class AuditApiAction extends AbstractApiAction {
    private final List<String> readonlyFields;

    public static class AuditRequestContentValidator extends RequestContentValidator {
-        private static final Set<AuditCategory> DISABLED_REST_CATEGORIES = ImmutableSet.of(
+        public static final Set<AuditCategory> DISABLED_REST_CATEGORIES = Set.of(


Why switch these from ImmutableSet?

This is a good question. List.of, Map.of, Set.of were introduced in JDK 9 and I think guava sooner or later will replace ImmutableSet and ImmutableList with JDK implementation how it was done for generics it is just matter of time. It does not mean that we need to change everything immediately, but if it is possible I'm for vanilla implementation of immutable objects.

cwperks · 2023-08-29T18:36:12Z

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

            try {
-                new MaskedField(mf, SALT).isValid();
+                new MaskedField(maskedFieldNode.asText(), SALT).isValid();


Which of these methods throws an exception? It seems odd that this calls a function called isValid() and expects it to throw an error if its invalid.

asText does not throw any exception. Docs:
Method that will return a valid String representation of the container value, if the node is a value node (method isValueNode returns true), otherwise empty String.
MaskedFieild throws exception in case the value is null. This validation triggers after we check on null values in array so I do not think that null will appear in this case. If you think that we need an addition check here something like:
maskedFieldNode.isValueNode()
I will add no problem.

Thanks for checking. I'm not as familiar with the MaskedField class and can't find any good tests around it.

This looks fine to me, but it took me a second to understand the change with returning a Pair or null vs prior to this change it returned true or false.

Ahhh sorry for that, but the previous solution checks masked fields only ones and after that you need to repeat an attempt. So in case you have more than one invalid masked fields you have to submit PUT/PATCH multiple times. Which is not nice since in other places we try to validate (e.g. validateDataType) all values in the request body.

Since there is a behavior change here it might be more obvious with a unit test that confirms

Oops looks like when I applied a patch I missed tests which I added. I was sure that tests are part of PR :-D. Which I mentioned in the description.

@peternied added missed tests

@cwperks I forgot to add tests when I applied the patch. I think now it should be better to understand MaskedField behave.

After RFC 6901 was introduced and the implementation was added to Jackson, there is no need to keep the com.jayway.jsonpath:json-path library in our source code, so we can replace current validation with Jackson's JsonPointer class. Signed-off-by: Andrey Pleskach <ples@aiven.io>

stephen-crawford

Looks good to me! Thanks @willyborankin

opensearch-trigger-bot · 2023-08-31T21:07:13Z

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

…ensearch-project#3262) ### Description After RFC 6901 was introduced and the implementation was added to Jackson, there is no need to keep the `com.jayway.jsonpath:json-path` library in our source code, so we can replace current validation with Jackson's `JsonPointer` class. Besides added missing tests for: - `RoleRequestContentValidator` - `AuditRequestContentValidator` ### Issues Resolved opensearch-project#3245 ### 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: Andrey Pleskach <ples@aiven.io> (cherry picked from commit 14574dd)

…ter instead #3262 (#3284) Backport 14574dd from #3262

### Description Selective manual backport of #3262. The original PR cannot be backported as is because of the changes to how the AbstractAPI is written between the 2.x and 1.x line. This PR resolves the build error seen on #3979. ### 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 <cwperx@amazon.com>

willyborankin marked this pull request as ready for review August 29, 2023 17:41

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

willyborankin added the backport 2.x backport to 2.x branch label Aug 29, 2023

cwperks reviewed Aug 29, 2023

View reviewed changes

willyborankin force-pushed the remove-json-path-library branch from c5c6733 to 385061f Compare August 29, 2023 20:36

peternied approved these changes Aug 29, 2023

View reviewed changes

stephen-crawford approved these changes Aug 31, 2023

View reviewed changes

stephen-crawford merged commit 14574dd into opensearch-project:main Aug 31, 2023

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

peternied mentioned this pull request Aug 31, 2023

[RELEASE] Release version 2.10.0 #3028

Closed

31 tasks

willyborankin mentioned this pull request Sep 1, 2023

[Backport 2.x] [Refactor] Remove json-path from deps and use JsonPointer instead #3262 #3284

Merged

cwperks pushed a commit that referenced this pull request Sep 1, 2023

[Backport 2.x] [Refactor] Remove json-path from deps and use JsonPoin…

4c9e6eb

…ter instead #3262 (#3284) Backport 14574dd from #3262

This was referenced Jan 24, 2024

Update json-path to address CVE-2023-51074 #3979

Closed

[1.3] Remove json-smart and json-path dependencies #3982

Merged

DarshitChanpura removed the backport-failed label Jul 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Refactor] Remove json-path from deps and use JsonPointer instead #3262

[Refactor] Remove json-path from deps and use JsonPointer instead #3262

willyborankin commented Aug 29, 2023

codecov bot commented Aug 29, 2023 •

edited

Loading

cwperks Aug 29, 2023

willyborankin Aug 29, 2023 •

edited

Loading

cwperks Aug 29, 2023 •

edited

Loading

willyborankin Aug 29, 2023 •

edited

Loading

cwperks Aug 29, 2023 •

edited

Loading

willyborankin Aug 29, 2023 •

edited

Loading

peternied Aug 29, 2023

willyborankin Aug 29, 2023

willyborankin Aug 29, 2023

willyborankin Aug 29, 2023 •

edited

Loading

stephen-crawford left a comment

opensearch-trigger-bot bot commented Aug 31, 2023

[Refactor] Remove json-path from deps and use JsonPointer instead #3262

[Refactor] Remove json-path from deps and use JsonPointer instead #3262

Conversation

willyborankin commented Aug 29, 2023

Description

Issues Resolved

Check List

codecov bot commented Aug 29, 2023 • edited Loading

Codecov Report

cwperks Aug 29, 2023

Choose a reason for hiding this comment

willyborankin Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

cwperks Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

willyborankin Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

cwperks Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

willyborankin Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

peternied Aug 29, 2023

Choose a reason for hiding this comment

willyborankin Aug 29, 2023

Choose a reason for hiding this comment

willyborankin Aug 29, 2023

Choose a reason for hiding this comment

willyborankin Aug 29, 2023 • edited Loading

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 31, 2023

codecov bot commented Aug 29, 2023 •

edited

Loading

willyborankin Aug 29, 2023 •

edited

Loading

cwperks Aug 29, 2023 •

edited

Loading

willyborankin Aug 29, 2023 •

edited

Loading

cwperks Aug 29, 2023 •

edited

Loading

willyborankin Aug 29, 2023 •

edited

Loading

willyborankin Aug 29, 2023 •

edited

Loading