[Refactor] Adopt request builder patterns for SecurityRestApiActions for consistency and clarity #3123

willyborankin · 2023-08-08T15:27:42Z

Description

REST API clean up continuation.

Main differences are:

Using functional approach to handle requests instead of inheritance which (IMHO) simplify code support and reading
All checks and verification stay the same, I only changed names.
PATCH uses the same validation rules as PUT and DELETE methods

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.

willyborankin · 2023-08-08T15:28:27Z

@peternied and @cwperks as i promised. So far all methods aka CRUD. PATCH will be added a bit later.

codecov · 2023-08-08T15:31:23Z

Codecov Report

Merging #3123 (c5cbd7e) into main (d7eabcf) will increase coverage by 0.63%.
The diff coverage is 83.72%.

@@             Coverage Diff              @@
##               main    #3123      +/-   ##
============================================
+ Coverage     62.51%   63.15%   +0.63%     
- Complexity     3403     3449      +46     
============================================
  Files           259      263       +4     
  Lines         20055    20023      -32     
  Branches       3370     3341      -29     
============================================
+ Hits          12538    12646     +108     
+ Misses         5866     5748     -118     
+ Partials       1651     1629      -22

Files Changed	Coverage Δ
...ecurityconf/impl/SecurityDynamicConfiguration.java	`75.24% <0.00%> (+0.99%)`	⬆️
...earch/security/dlic/rest/api/MigrateApiAction.java	`7.84% <5.45%> (+3.12%)`	⬆️
...earch/security/dlic/rest/api/TenantsApiAction.java	`41.17% <23.07%> (+13.39%)`	⬆️
...arch/security/dlic/rest/api/ValidateApiAction.java	`18.42% <28.57%> (+9.33%)`	⬆️
...ch/security/dlic/rest/api/FlushCacheApiAction.java	`65.38% <55.55%> (+1.74%)`	⬆️
...security/dlic/rest/api/InternalUsersApiAction.java	`75.00% <76.54%> (+10.92%)`	⬆️
...g/opensearch/security/dlic/rest/support/Utils.java	`60.97% <77.77%> (+6.32%)`	⬆️
...security/dlic/rest/api/SecuritySSLCertsAction.java	`76.11% <80.43%> (+9.45%)`	⬆️
...g/opensearch/security/dlic/rest/api/Responses.java	`81.57% <81.57%> (ø)`
...nsearch/security/dlic/rest/api/RolesApiAction.java	`80.43% <86.36%> (+3.16%)`	⬆️
... and 20 more

... and 4 files with indirect coverage changes

willyborankin · 2023-08-09T13:54:15Z

I fixed current failures

peternied

Added some comments around this draft - overall a big fan of this direction.

build.gradle

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

cwperks

Thank you @willyborankin ! I took a first pass at a review for this PR and left a few comments. Overall, I like the introduction of higher level abstractions and creating a common class for common Responses.

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

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

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

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

cwperks · 2023-08-14T17:49:37Z

@willyborankin I got an email about a comment, but cannot see it any longer:

Renamed methods:
getResourceName -> getConfigurationName
getConfigName -> getConfigurationType

I like the getConfigurationType (or getConfigType) name as it connects to CType well.

Likewise getConfigEntryByName could work for getResourceName, but I am also fine with getResourceName as well. What should we call a single entry in one of the sections of the security config types (internal_users, roles, roles_mappings, etc.)? Should these generally be referred to as ConfigEntries or as resources?

willyborankin · 2023-08-15T13:30:58Z

@willyborankin I got an email about a comment, but cannot see it any longer:

Renamed methods:
getResourceName -> getConfigurationName
getConfigName -> getConfigurationType

I like the getConfigurationType (or getConfigType) name as it connects to CType well.

Likewise getConfigEntryByName could work for getResourceName, but I am also fine with getResourceName as well. What should we call a single entry in one of the sections of the security config types (internal_users, roles, roles_mappings, etc.)? Should these generally be referred to as ConfigEntries or as resources?

@cwperks Yes I reverted the commit. found a bug.
Names closer to your suggestion. getConfigName -> getConfigType. A signle entity name I use so far entityName :-) .

willyborankin · 2023-08-16T18:03:04Z

Hi, @peternied and @cwperks.

I finished and address changes we discussed.

Changes:

added PATCH
removed all dead code
removed spotless on/off

Added a common EndpointValidator class which uses for all HTTP methods and validate configuration before it will be processed by HTTP response. So PATCH now uses the same set of validation rules as for PUT and DELETE methods.

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

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

cwperks

@willyborankin This PR looks good to me. I like the change this introduces to reduce the amount of notImplemented around the concrete APIs.

What work is remaining before marking this ready for review?

willyborankin · 2023-08-19T16:40:48Z

@willyborankin This PR looks good to me. I like the change this introduces to reduce the amount of notImplemented around the concrete APIs.

What work is remaining before marking this ready for review?

I'm going to add 1 test and open it as ready for review

Signed-off-by: Andrey Pleskach <ples@aiven.io>

- extract configuration validation into separate class EndpointValidator - SecurityConfiguration is a companion class now - Renamed getConfigName -> getConfigType Signed-off-by: Andrey Pleskach <ples@aiven.io>

Signed-off-by: Andrey Pleskach <ples@aiven.io>

- Moved resourceName to EndpointValidator - Added SecurityConfigurationTest Signed-off-by: Andrey Pleskach <ples@aiven.io>

Changes: - Changed some names fir better readability - Added test for validations Signed-off-by: Andrey Pleskach <ples@aiven.io>

Signed-off-by: Andrey Pleskach <ples@aiven.io>

opensearch-trigger-bot · 2023-08-29T16:37:11Z

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

…for consistency and clarity (opensearch-project#3123) Main differences are: - Using functional approach to handle requests instead of inheritance which (IMHO) simplify code support and reading - All checks and verification stay the same, I only changed names. - PATCH uses the same validation rules as PUT and DELETE methods Signed-off-by: Andrey Pleskach <ples@aiven.io> (cherry picked from commit 0338cdd)

…RestApiActions for consistency and clarity (#3261) Backport 0338cdd from #3123

) The resent refactoring of the REST APIs: #3123 introduce a regression in how roles-mapping verification has worked before. The old solution verified only hidden roles both for internal users and roles mapping, while new was too strict and forbid to do it for both. This PR fixes the problem and uses the same logic as it was before. - In case of roles-mapping it verifies only a role associated with it that the role is not hidden. - In case of internal users it verifies that a role is not hidden and roles-mapping associated with the role is mutable So verification was split and added to the corresponding ActionApi class which is more convenient as it was before. Signed-off-by: Andrey Pleskach <ples@aiven.io>

) The resent refactoring of the REST APIs: #3123 introduce a regression in how roles-mapping verification has worked before. The old solution verified only hidden roles both for internal users and roles mapping, while new was too strict and forbid to do it for both. This PR fixes the problem and uses the same logic as it was before. - In case of roles-mapping it verifies only a role associated with it that the role is not hidden. - In case of internal users it verifies that a role is not hidden and roles-mapping associated with the role is mutable So verification was split and added to the corresponding ActionApi class which is more convenient as it was before. Signed-off-by: Andrey Pleskach <ples@aiven.io> (cherry picked from commit 53f64b9) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

willyborankin force-pushed the clean-up-rest-api-classes-2 branch from 14ee156 to d838305 Compare August 8, 2023 15:30

willyborankin force-pushed the clean-up-rest-api-classes-2 branch from d838305 to 68bff72 Compare August 9, 2023 13:53

willyborankin force-pushed the clean-up-rest-api-classes-2 branch 2 times, most recently from cd21190 to ca43ee0 Compare August 9, 2023 15:03

peternied reviewed Aug 9, 2023

View reviewed changes

build.gradle Outdated Show resolved Hide resolved

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

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

cwperks reviewed Aug 9, 2023

View reviewed changes

willyborankin force-pushed the clean-up-rest-api-classes-2 branch 3 times, most recently from 0e2c1f1 to ec30ef3 Compare August 13, 2023 20:34

willyborankin mentioned this pull request Aug 14, 2023

[FEATURE] Auto-code formatting in some case gives not nice results #3178

Closed

willyborankin force-pushed the clean-up-rest-api-classes-2 branch from 2fcc3ee to 24c23c3 Compare August 15, 2023 19:43

willyborankin mentioned this pull request Aug 16, 2023

Exclude sensitive info from the jackson serialization stacktraces #3195

Merged

3 tasks

willyborankin force-pushed the clean-up-rest-api-classes-2 branch 3 times, most recently from 7b37f5c to 584cc26 Compare August 16, 2023 17:33

willyborankin force-pushed the clean-up-rest-api-classes-2 branch from 584cc26 to ac93e34 Compare August 17, 2023 15:59

cwperks reviewed Aug 18, 2023

View reviewed changes

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

cwperks reviewed Aug 18, 2023

View reviewed changes

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

cwperks reviewed Aug 18, 2023

View reviewed changes

willyborankin force-pushed the clean-up-rest-api-classes-2 branch from ac93e34 to 1c75d36 Compare August 19, 2023 19:42

willyborankin marked this pull request as ready for review August 19, 2023 19:42

willyborankin requested review from cliu123 and DarshitChanpura as code owners August 19, 2023 19:42

willyborankin added 8 commits August 27, 2023 11:42

replace handlePut with new implementation

7342c46

Signed-off-by: Andrey Pleskach <ples@aiven.io>

move configureRequestHandlers into RequestHandler.RequestHandlersBuilder

b1cb35d

Signed-off-by: Andrey Pleskach <ples@aiven.io>

Address comments:

60094cf

- extract configuration validation into separate class EndpointValidator - SecurityConfiguration is a companion class now - Renamed getConfigName -> getConfigType Signed-off-by: Andrey Pleskach <ples@aiven.io>

replace PatchableResourceApiAction with new implementation

66f957e

Signed-off-by: Andrey Pleskach <ples@aiven.io>

final clean up

7620bad

Signed-off-by: Andrey Pleskach <ples@aiven.io>

remove spotless on/off

caf8b20

Signed-off-by: Andrey Pleskach <ples@aiven.io>

Misc changes:

be226f6

- Moved resourceName to EndpointValidator - Added SecurityConfigurationTest Signed-off-by: Andrey Pleskach <ples@aiven.io>

Address comments

6b86556

Changes: - Changed some names fir better readability - Added test for validations Signed-off-by: Andrey Pleskach <ples@aiven.io>

willyborankin force-pushed the clean-up-rest-api-classes-2 branch from 95f509a to 6b86556 Compare August 27, 2023 09:42

This was referenced Aug 27, 2023

[FEATURE] Add additional validations for role mappings #3244

Open

[FEATURE] Replace JsonPath library with Jackson JsonPointer #3245

Closed

willyborankin dismissed peternied’s stale review via 63ed510 August 27, 2023 19:26

Removed SomeRestRequest and use FakeRestRequest instead

c5cbd7e

Signed-off-by: Andrey Pleskach <ples@aiven.io>

willyborankin force-pushed the clean-up-rest-api-classes-2 branch from 63ed510 to c5cbd7e Compare August 28, 2023 09:08

peternied approved these changes Aug 28, 2023

View reviewed changes

willyborankin added the v2.10.0 Issues targeting release v2.10.0 label Aug 29, 2023

cwperks approved these changes Aug 29, 2023

View reviewed changes

peternied merged commit 0338cdd into opensearch-project:main Aug 29, 2023

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

willyborankin mentioned this pull request Aug 29, 2023

[Backport 2.x] [Refactor] Adopt request builder patterns for SecurityRestApiActions for consistency and clarity #3261

Merged

This was referenced Aug 31, 2023

Skip role immutable check for Roles Mappings APIs #3272

Closed

[BUG] Add a test to ensure that using PATCH API to update Roles Mappings works on a static role #3276

Closed

willyborankin mentioned this pull request Aug 31, 2023

[BUG] Fix roles verification for roles mapping and internal users #3278

Merged

3 tasks

peternied removed the backport-failed label Aug 31, 2023

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

[Backport 2.x] [Refactor] Adopt request builder patterns for Security…

1d6b5bf

…RestApiActions for consistency and clarity (#3261) Backport 0338cdd from #3123

peternied mentioned this pull request Sep 13, 2023

Add backend filtering for legacy internal user and service accounts #2786

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Refactor] Adopt request builder patterns for SecurityRestApiActions for consistency and clarity #3123

[Refactor] Adopt request builder patterns for SecurityRestApiActions for consistency and clarity #3123

willyborankin commented Aug 8, 2023 •

edited

Loading

willyborankin commented Aug 8, 2023

codecov bot commented Aug 8, 2023 •

edited

Loading

willyborankin commented Aug 9, 2023

peternied left a comment

cwperks left a comment

cwperks commented Aug 14, 2023

willyborankin commented Aug 15, 2023 •

edited

Loading

willyborankin commented Aug 16, 2023 •

edited

Loading

cwperks left a comment

willyborankin commented Aug 19, 2023

opensearch-trigger-bot bot commented Aug 29, 2023

[Refactor] Adopt request builder patterns for SecurityRestApiActions for consistency and clarity #3123

[Refactor] Adopt request builder patterns for SecurityRestApiActions for consistency and clarity #3123

Conversation

willyborankin commented Aug 8, 2023 • edited Loading

Description

Check List

willyborankin commented Aug 8, 2023

codecov bot commented Aug 8, 2023 • edited Loading

Codecov Report

willyborankin commented Aug 9, 2023

peternied left a comment

Choose a reason for hiding this comment

cwperks left a comment

Choose a reason for hiding this comment

cwperks commented Aug 14, 2023

willyborankin commented Aug 15, 2023 • edited Loading

willyborankin commented Aug 16, 2023 • edited Loading

cwperks left a comment

Choose a reason for hiding this comment

willyborankin commented Aug 19, 2023

opensearch-trigger-bot bot commented Aug 29, 2023

willyborankin commented Aug 8, 2023 •

edited

Loading

codecov bot commented Aug 8, 2023 •

edited

Loading

willyborankin commented Aug 15, 2023 •

edited

Loading

willyborankin commented Aug 16, 2023 •

edited

Loading