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

add more user based permission check in Memory #1927

Merged
merged 2 commits into from
Jan 26, 2024

Conversation

Zhangxunmt
Copy link
Collaborator

@Zhangxunmt Zhangxunmt commented Jan 25, 2024

Description

This PR is to add user based permission check that addresses concerns in #1901.

The permission checks are strictly applied to all interactions/conversations.

The PR is verified using two different login users and ensure they don't have access to each others's data

More tests cases added and Rebased to latest main branch.

Issues Resolved

[List any issues this PR will resolve]

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • 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.

Copy link

codecov bot commented Jan 25, 2024

Codecov Report

Attention: 42 lines in your changes are missing coverage. Please review.

Comparison is base (849fecf) 82.96% compared to head (fc476ac) 82.84%.

Files Patch % Lines
.../opensearch/ml/memory/index/InteractionsIndex.java 71.42% 20 Missing and 6 partials ⚠️
...onversation/UpdateConversationTransportAction.java 75.00% 4 Missing and 1 partial ⚠️
...conversation/UpdateInteractionTransportAction.java 64.28% 4 Missing and 1 partial ⚠️
...y/index/OpenSearchConversationalMemoryHandler.java 28.57% 5 Missing ⚠️
...nsearch/ml/memory/index/ConversationMetaIndex.java 90.90% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #1927      +/-   ##
============================================
- Coverage     82.96%   82.84%   -0.12%     
- Complexity     5424     5445      +21     
============================================
  Files           522      522              
  Lines         21781    21900     +119     
  Branches       2222     2226       +4     
============================================
+ Hits          18070    18144      +74     
- Misses         2811     2848      +37     
- Partials        900      908       +8     
Flag Coverage Δ
ml-commons 82.84% <70.62%> (-0.12%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@HenryL27 HenryL27 left a comment

Choose a reason for hiding this comment

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

Overall looks good, thanks for doing this.
There used to be an access test for singular get interaction - can we put it back? And maybe add some similar tests for the trace/update APIs?

@Zhangxunmt
Copy link
Collaborator Author

Zhangxunmt commented Jan 25, 2024

Overall looks good, thanks for doing this. There used to be an access test for singular get interaction - can we put it back? And maybe add some similar tests for the trace/update APIs?

Yes those tests are added. There is slightly difference for the get interaction permission check test due to the conversation id is removed after refactor. But overall this is a valid solution without adding too much latency (negligible from tests).

Signed-off-by: Xun Zhang <xunzh@amazon.com>
Copy link
Collaborator

@HenryL27 HenryL27 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for adding access tests

@Zhangxunmt Zhangxunmt merged commit cdd63b4 into opensearch-project:main Jan 26, 2024
13 of 15 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 26, 2024
* add more user based permission check in Memory

Signed-off-by: Xun Zhang <xunzh@amazon.com>

* add UT for acess denied cases

Signed-off-by: Xun Zhang <xunzh@amazon.com>

---------

Signed-off-by: Xun Zhang <xunzh@amazon.com>
(cherry picked from commit cdd63b4)
Zhangxunmt added a commit that referenced this pull request Jan 26, 2024
* add more user based permission check in Memory

Signed-off-by: Xun Zhang <xunzh@amazon.com>

* add UT for acess denied cases

Signed-off-by: Xun Zhang <xunzh@amazon.com>

---------

Signed-off-by: Xun Zhang <xunzh@amazon.com>
(cherry picked from commit cdd63b4)

Co-authored-by: Xun Zhang <xunzh@amazon.com>
Zhangxunmt added a commit to Zhangxunmt/ml-commons that referenced this pull request Jan 29, 2024
* add more user based permission check in Memory

Signed-off-by: Xun Zhang <xunzh@amazon.com>

* add UT for acess denied cases

Signed-off-by: Xun Zhang <xunzh@amazon.com>

---------

Signed-off-by: Xun Zhang <xunzh@amazon.com>
austintlee pushed a commit to austintlee/ml-commons that referenced this pull request Mar 19, 2024
* add more user based permission check in Memory

Signed-off-by: Xun Zhang <xunzh@amazon.com>

* add UT for acess denied cases

Signed-off-by: Xun Zhang <xunzh@amazon.com>

---------

Signed-off-by: Xun Zhang <xunzh@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants