-
Notifications
You must be signed in to change notification settings - Fork 280
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
Add backend filtering for legacy internal user and service accounts #2786
Add backend filtering for legacy internal user and service accounts #2786
Conversation
Signed-off-by: scosta <[email protected]>
Signed-off-by: Ryan Liang <[email protected]>
Signed-off-by: scosta <[email protected]>
src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/dlic/rest/api/UserApiTest.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Darshit Chanpura <[email protected]> Signed-off-by: Sam <[email protected]>
…ersApiAction.java Co-authored-by: Darshit Chanpura <[email protected]> Signed-off-by: Sam <[email protected]>
Co-authored-by: Darshit Chanpura <[email protected]> Signed-off-by: Sam <[email protected]>
Signed-off-by: scosta <[email protected]>
Signed-off-by: scosta <[email protected]>
Signed-off-by: scosta <[email protected]>
Signed-off-by: scosta <[email protected]>
@DarshitChanpura I had to a good bit of refactoring as I noticed that before, the beahviour in the PR was returning only the list of names of the accounts of the specified type, but the existing implementations on internalUsersAPI return the full account details. |
src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.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.
Overall looks good to me. I left a comment about tests, but I believe this should ready for final review.
@samuelcostae could you rebase against main and resolve any conflicts. |
Signed-off-by: Sam <[email protected]>
@samuelcostae Please let us know if you would require any help to move this along. |
@samuelcostae Thanks for the updates - sorry its taken a little bit to circle back around to this pull requests. |
# Conflicts: # src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java
Signed-off-by: Sam <[email protected]>
Signed-off-by: Sam <[email protected]>
This is associated with #2704 |
src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Sam <[email protected]>
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.
Looks close. Final two 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.
Last blocker in my mind is the structure of the UserService code, please iterate on @DarshitChanpura suggestion to slim down that code.
Co-authored-by: Darshit Chanpura <[email protected]> Signed-off-by: Sam <[email protected]>
Signed-off-by: Sam <[email protected]>
Signed-off-by: Sam <[email protected]>
src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.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.
Adding some feedback about the new unit tests - really close.
src/test/java/org/opensearch/security/UserServiceUnitTests.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/UserServiceUnitTests.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/UserServiceUnitTests.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/dlic/rest/api/UserApiTest.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Sam <[email protected]>
Signed-off-by: Sam <[email protected]>
Signed-off-by: Sam <[email protected]>
src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java
Show resolved
Hide resolved
src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java
Show resolved
Hide resolved
@scrawfor99 Could you take another look? |
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.
Great job Sam :)
7f6944c
into
opensearch-project:main
Signed-off-by: Ryan Liang <[email protected]>
Description
Update UserAPIAction to Filter internal accounts and Service accounts.
Dashboards Plugin changes via PR opensearch-project/security-dashboards-plugin#1502
Issues Resolved
Testing
Unit Tests created
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.