Skip to content
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

Separated DLS/FLS privilege evaluation from action privilege evaluation #4490

Merged
merged 7 commits into from
Jul 9, 2024

Conversation

nibix
Copy link
Collaborator

@nibix nibix commented Jun 26, 2024

Description

This change is in preparation for #3870 and #4380 .

This cuts off some parts from the quite big and monolithic method PrivilegesEvaluator.evaluate() into separate methods and modules.

This achieves several things:

  • Better separation of concerns, thus better modularization and thus better understandability, re-usability and testability
  • Computations of DLS/FLS privileges are done directly in the DLS/FLS valve code. Thus, if DLS/FLS valve is disabled, no DLS/FLS privileges are computed
  • The new class PrivilegesEvaluationContext combines commonly needed information for privilege evaluation and thus allows to shorten the parameter lists of many methods in this context. For this PR, only the PrivilegesEvaluator.evaluate() method itself is changed, but further adaptions will follow up when due.
  • Category - Refactoring
  • Why these changes are required?
    • For Optimized Privilege Evaluation #3870, we need to re-build the privilege evaluation code both for the normal action privilege evaluation and for DLS/FLS. Even though these are quite separate and different things, these are currently intermingled in one big monolith. The changes for Optimized Privilege Evaluation #3870 will be easier to understand if they are structured better.
  • What is the old behavior before changes and new behavior after changes? - No changes.

Issues Resolved

Contributes to #3870

Testing

  • Existing 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: Nils Bandener <[email protected]>
Signed-off-by: Nils Bandener <[email protected]>
@nibix nibix marked this pull request as ready for review June 26, 2024 09:33
Signed-off-by: Nils Bandener <[email protected]>
Copy link

codecov bot commented Jun 26, 2024

Codecov Report

Attention: Patch coverage is 89.09091% with 6 lines in your changes missing coverage. Please review.

Project coverage is 65.22%. Comparing base (9caf5cb) to head (97d8d54).
Report is 11 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4490      +/-   ##
==========================================
- Coverage   65.27%   65.22%   -0.05%     
==========================================
  Files         313      314       +1     
  Lines       22058    22090      +32     
  Branches     3563     3563              
==========================================
+ Hits        14398    14408      +10     
- Misses       5889     5906      +17     
- Partials     1771     1776       +5     
Files Coverage Δ
...ity/configuration/DlsFilterLevelActionHandler.java 57.60% <100.00%> (+0.39%) ⬆️
...rch/security/configuration/DlsFlsRequestValve.java 0.00% <ø> (ø)
...search/security/configuration/DlsFlsValveImpl.java 59.93% <100.00%> (+0.88%) ⬆️
...org/opensearch/security/filter/SecurityFilter.java 66.35% <100.00%> (+0.63%) ⬆️
...curity/privileges/PrivilegesEvaluationContext.java 100.00% <100.00%> (ø)
.../opensearch/security/OpenSearchSecurityPlugin.java 84.33% <50.00%> (+0.02%) ⬆️
...curity/privileges/PrivilegesEvaluatorResponse.java 80.95% <0.00%> (-1.66%) ⬇️
...earch/security/privileges/PrivilegesEvaluator.java 71.87% <71.42%> (-0.30%) ⬇️

... and 6 files with indirect coverage changes

nibix added 3 commits July 4, 2024 10:24
…TRO_SECURITY_INJECTED_ROLES_VALIDATION handling back to evaluate()

Signed-off-by: Nils Bandener <[email protected]>
Signed-off-by: Nils Bandener <[email protected]>
Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new changes look good. Let's capture the role re-assignment for the injected roles validation set in an issue to discuss further.

@DarshitChanpura
Copy link
Member

@cwperks @scrawfor99 this is blocked by each of your un-resolved conversations. Would you mind going through them and closing as needed?

@DarshitChanpura DarshitChanpura added the backport 2.x backport to 2.x branch label Jul 9, 2024
@cwperks cwperks merged commit dabff35 into opensearch-project:main Jul 9, 2024
42 of 43 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jul 9, 2024
…on (#4490)

Signed-off-by: Nils Bandener <[email protected]>
(cherry picked from commit dabff35)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants