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

[Backport 2.x] Rest admin permissions (#2411) #2466

Merged
merged 3 commits into from
Feb 27, 2023

Conversation

willyborankin
Copy link
Collaborator

Backport of d676716

@willyborankin
Copy link
Collaborator Author

willyborankin commented Feb 22, 2023

ConfigurationRepository.java:383 compilation failed

@cwperks
Copy link
Member

cwperks commented Feb 22, 2023

@willyborankin There was a commit merged yesterday to fix the build in 2.x: d86dc3f. Can you rebase with the latest from 2.x?

@willyborankin
Copy link
Collaborator Author

@willyborankin There was a commit merged yesterday to fix the build in 2.x: d86dc3f. Can you rebase with the latest from 2.x?

@cwperks hmmm

From github.com:opensearch-project/security
 * branch              2.x        -> FETCH_HEAD
Current branch 2.x is up to date.

I can fix manually. Is it okay?

@stephen-crawford
Copy link
Collaborator

@willyborankin There was a commit merged yesterday to fix the build in 2.x: d86dc3f. Can you rebase with the latest from 2.x?

@cwperks hmmm

From github.com:opensearch-project/security
 * branch              2.x        -> FETCH_HEAD
Current branch 2.x is up to date.

I can fix manually. Is it okay?

If it is easier to manually swap stuff over to match the changes instead of rebasing that is fine. You will just want to double check you get all the differences so that your backport is the only diffs being merged with your PR.

Thank you for taking the time. It has been a busy couple days with the release so your changes got caught in the middle. We appreciate your contribution greatly.

@willyborankin
Copy link
Collaborator Author

willyborankin commented Feb 22, 2023

@willyborankin There was a commit merged yesterday to fix the build in 2.x: d86dc3f. Can you rebase with the latest from 2.x?

@cwperks hmmm

From github.com:opensearch-project/security
 * branch              2.x        -> FETCH_HEAD
Current branch 2.x is up to date.

I can fix manually. Is it okay?

If it is easier to manually swap stuff over to match the changes instead of rebasing that is fine. You will just want to double check you get all the differences so that your backport is the only diffs being merged with your PR.

Thank you for taking the time. It has been a busy couple days with the release so your changes got caught in the middle. We appreciate your contribution greatly.

@willyborankin There was a commit merged yesterday to fix the build in 2.x: d86dc3f. Can you rebase with the latest from 2.x?

@cwperks hmmm

From github.com:opensearch-project/security
 * branch              2.x        -> FETCH_HEAD
Current branch 2.x is up to date.

I can fix manually. Is it okay?

If it is easier to manually swap stuff over to match the changes instead of rebasing that is fine. You will just want to double check you get all the differences so that your backport is the only diffs being merged with your PR.

Thank you for taking the time. It has been a busy couple days with the release so your changes got caught in the middle. We appreciate your contribution greatly.

@cwperks
All good let's finish it :-). In any case I'm going to open 2 new PRs with bugs which I found.

I think I got it compilation error is:

/home/ples/workspace/security/src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java:383: error: incompatible types: XContentType cannot be converted to ToXContent
            fields.put(configurationType.toLCString(), Strings.toString(XContentType.JSON, retVal.get(configurationType)));
                                                                                    ^
/home/ples/workspace/security/src/main/java/org/opensearch/security/action/whoami/WhoAmIResponse.java:109: error: incompatible types: XContentType cannot be converted to ToXContent
        return Strings.toString(XContentType.JSON,this, true, true);
                                            ^
/home/ples/workspace/security/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java:213: error: incompatible types: XContentType cannot be converted to ToXContent
                        sb.append(Strings.toString(XContentType.JSON, searchRequest.source()) + System.lineSeparator());
                                                               ^
/home/ples/workspace/security/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java:216: error: incompatible types: XContentType cannot be converted to ToXContent
                    sb.append(Strings.toString(XContentType.JSON, af) + System.lineSeparator());
                                                           ^
/home/ples/workspace/security/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java:226: error: incompatible types: XContentType cannot be converted to ToXContent
                            + (searchRequest.source() == null ? "<NULL>" : Strings.toString(XContentType.JSON, searchRequest.source())));
                                                                                                        ^
/home/ples/workspace/security/src/main/java/org/opensearch/security/tools/SecurityAdmin.java:1002: error: incompatible types: XContentType cannot be converted to ToXContent
                        sb.append(Strings.toString(XContentType.JSON, nir, true, true));

the branch has this commit d86dc3f, but it complains about MediaType vs ToXContent. AFAIK it was added in OpenSearch 2.6 and hasn't been backported to 2.x yet.
#2451 here is the fix ... but @reta told me that t was reverted ... puzzled :-)

@DarshitChanpura
Copy link
Member

#2451 here is the fix ... but @reta told me that t was reverted ... puzzled :-)

As of current, #2451 should fix the CI failures related to XContentType for this PR.

@cwperks
Copy link
Member

cwperks commented Feb 24, 2023

#2451 was just merged. @willyborankin Could you merge in the latest changes from 2.x? 2.x is stable now.

Permissions for REST admin user

Added granular permissions for all REST API actions in OpenSearch to be individually assigned.

Permissions are:
    - 'restapi:admin/actiongroups' - allow full access to actiongroups
    - 'restapi:admin/allowlist' - allow full access to allowlist
    - 'restapi:admin/internalusers'- allow full access to internalusers
    - 'restapi:admin/nodesdn'- allow full access to nodesdn
    - 'restapi:admin/roles' - allow full access to roles
    - 'restapi:admin/rolesmapping' - allow full access to roles mappings
    - 'restapi:admin/ssl/certs/info' - allow full access to certs info
    - 'restapi:admin/ssl/certs/reload' - allow full access to certs reload
    - 'restapi:admin/tenants' - allow full access to tenants

Adds tests for these permissions.

Signed-off-by: Andrey Pleskach <ples@aiven.io>
@codecov-commenter
Copy link

codecov-commenter commented Feb 24, 2023

Codecov Report

Merging #2466 (9f4273c) into 2.x (76aa785) will increase coverage by 0.14%.
The diff coverage is 76.20%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##                2.x    #2466      +/-   ##
============================================
+ Coverage     61.01%   61.15%   +0.14%     
- Complexity     3248     3303      +55     
============================================
  Files           258      258              
  Lines         18137    18262     +125     
  Branches       3232     3250      +18     
============================================
+ Hits          11066    11168     +102     
- Misses         5494     5509      +15     
- Partials       1577     1585       +8     
Impacted Files Coverage Δ
...earch/security/dlic/rest/api/AccountApiAction.java 81.15% <0.00%> (-1.20%) ⬇️
...curity/dlic/rest/api/AuthTokenProcessorAction.java 53.33% <0.00%> (-3.81%) ⬇️
...ch/security/dlic/rest/api/FlushCacheApiAction.java 61.29% <0.00%> (-2.05%) ⬇️
...earch/security/dlic/rest/api/MigrateApiAction.java 4.12% <0.00%> (-0.05%) ⬇️
...earch/security/dlic/rest/api/TenantsApiAction.java 36.36% <0.00%> (-3.64%) ⬇️
...arch/security/dlic/rest/api/ValidateApiAction.java 10.25% <0.00%> (-0.27%) ⬇️
...pensearch/security/securityconf/ConfigModelV6.java 0.00% <0.00%> (ø)
...security/dlic/rest/api/SecuritySSLCertsAction.java 71.71% <71.71%> (ø)
.../security/dlic/rest/api/RolesMappingApiAction.java 82.85% <76.92%> (-3.51%) ⬇️
...dlic/rest/api/RestApiAdminPrivilegesEvaluator.java 78.33% <78.33%> (ø)
... and 20 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@RyanL1997
Copy link
Collaborator

I'm updating the branch right now

@DarshitChanpura
Copy link
Member

CI will be fixed once #2484 is merged

Copy link
Collaborator

@stephen-crawford stephen-crawford left a comment

Choose a reason for hiding this comment

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

Thank you for all your work!

@peternied peternied merged commit 076715d into opensearch-project:2.x Feb 27, 2023
Rishikesh1159 added a commit to Rishikesh1159/security that referenced this pull request Mar 15, 2023
willyborankin added a commit to willyborankin/security that referenced this pull request Apr 3, 2023
willyborankin added a commit to willyborankin/security that referenced this pull request Apr 3, 2023
willyborankin added a commit to willyborankin/security that referenced this pull request Apr 3, 2023
…project#2466)"

This reverts commit 076715d.

Signed-off-by: Andrey Pleskach <ples@aiven.io>
peternied pushed a commit that referenced this pull request Apr 4, 2023
This reverts commit 076715d.

Signed-off-by: Andrey Pleskach <ples@aiven.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants