-
Notifications
You must be signed in to change notification settings - Fork 261
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Configure masking algorithm default #4336
Configure masking algorithm default #4336
Conversation
Signed-off-by: Terry Quigley <terry.quigley@sas.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4336 +/- ##
==========================================
- Coverage 66.02% 66.00% -0.02%
==========================================
Files 302 302
Lines 21758 21762 +4
Branches 3522 3523 +1
==========================================
- Hits 14366 14365 -1
- Misses 5625 5630 +5
Partials 1767 1767
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this update @terryquigleysas. Lets shift the logic around so the MaskedField uses an algorithm provider. As masked field should not need to know which implementation is being used, but just that it is using the algorithm selected for use by the plugin.
This will pave the way for different kinds of providers to by swapped, such as a specific FIPS compliant set vs others.
@peternied Thank you for your comments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@terryquigleysas What do you think about passing the default in as a constructor arg to MaskedField in this call to avoid passing it into the mask
function on every call?
@cwperks Thank you for the review. I have changed the code to this effect. |
src/main/java/org/opensearch/security/configuration/DlsFlsFilterLeafReader.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates @terryquigleysas looks much cleaner, the refactor to a provider/factory doesn't seem a relevant with these changes.
src/main/java/org/opensearch/security/dlic/rest/api/RolesApiAction.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for filing this PR @terryquigleysas. I left some clarifying comments from my end.
src/main/java/org/opensearch/security/configuration/DlsFlsFilterLeafReader.java
Show resolved
Hide resolved
@terryquigleysas last request, can you sign all of the commits on this PR? The DCO check is failing: https://github.com/opensearch-project/security/pull/4336/checks?check_run_id=24958472355 |
@cwperks I have followed the process at https://github.com/opensearch-project/security/pull/4336/checks?check_run_id=24958472355 but things do not appear to have changed here. Is there another way to do this? I'm new to this specific process. UPDATE: That appears to have worked now for my commits. Hopefully none of the other commits pulled in are affected. |
7a38cfc
to
16edc79
Compare
Signed-off-by: Terry Quigley <terry.quigley@sas.com>
Signed-off-by: Terry Quigley <terry.quigley@sas.com>
Signed-off-by: Terry Quigley <terry.quigley@sas.com>
…ject#4334) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Signed-off-by: Terry Quigley <terry.quigley@sas.com>
16edc79
to
62960d7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you @terryquigleysas !
@willyborankin any concerns with this PR using the older test mechanism? This PR looks good to me. I think it can be followed-up with a change to the |
@cwperks If fine with. |
@terryquigleysas Did you create a documentation PR to accompany this change? Can you update the PR description to include a link to the companion PR for the documentation-website? |
Yes, it can be found at opensearch-project/documentation-website#7162 |
Signed-off-by: Terry Quigley <terry.quigley@sas.com> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit d19a8ba) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
Issues Resolved
Resolves #4213
Testing
New tests added.
Tested locally against a running cluster.
Documentation
I will be raising an issue to add documentation to https://opensearch.org/docs/latest/security/access-control/field-masking
Check List
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.