InjectSecurity - inject User object in UserInfo in threadContext #396

petardz · 2023-04-11T22:24:05Z

Description

Added user_info injection of User object in InjectSecurity

Pre-requisite for opensearch-project/alerting#852

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.

codecov-commenter · 2023-04-11T22:29:46Z

Codecov Report

❗ No coverage uploaded for pull request base (main@deed4bd). Click here to learn what that means.
The diff coverage is 50.00%.

📣 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           @@
##             main     #396   +/-   ##
=======================================
  Coverage        ?   73.20%           
  Complexity      ?      700           
=======================================
  Files           ?      110           
  Lines           ?     4631           
  Branches        ?      610           
=======================================
  Hits            ?     3390           
  Misses          ?      985           
  Partials        ?      256

Impacted Files	Coverage Δ
...in/java/org/opensearch/commons/InjectSecurity.java	`71.92% <50.00%> (ø)`

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

Signed-off-by: Petar <petar.dzepina@gmail.com>

eirsep · 2023-04-13T18:41:20Z

src/main/java/org/opensearch/commons/InjectSecurity.java

+        if (user == null) {
+            return;
+        }
+        if (threadContext.getTransient(ConfigConstants.OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT) != null) {


can we log in the error message the value present in threadContext.getTransient(ConfigConstants.OPENSEARCH_SECURITY_USER_INFO_THREAD_CONTEXT)

Yeah, the error message may not be that useful.
Can you add in the comment of this method that this should only be used within plugin after stash the context and injecting user permission? I think this is the only use case of this method.

@eirsep @bowenlan-amzn

Added comments, better logging and unit test

bowenlan-amzn · 2023-04-13T18:58:46Z

src/main/java/org/opensearch/commons/InjectSecurity.java

+        StringJoiner joiner = new StringJoiner("|");
+        joiner.add(user.getName());
+        joiner.add(java.lang.String.join(",", user.getBackendRoles()));
+        joiner.add(java.lang.String.join(",", user.getRoles()));


Is there already a method to reuse for combining these?

I didn't find one. ISM has one in SecurityUtils (generateUserString)

This is the source of where this ThreadContext header is populated from the security plugin: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java#L197-L209

This is done in the PrivilegesEvaluator which is called from the SecurityFilter which is the first action filter that is applied before a transport request is executed on a node.

mappedRoles in this method is the set of roles that are resolved to as part of the roles resolution process which is calculating via a roles mapping like:

# mapping for role1 role1 { "backend_roles" : [ "starfleet", "captains", "defectors" ], "hosts" : [ "*.starfleetintranet.com" ], "users" : [ "kirk", "spock" ], "and_backend_roles": ["enterprise", "voyager"] // User must have all backend roles in this list to be mapped to role1 }

@cwperks thanks for checking on this. If there's any possible regression in the future, please help track it in some issue.

@bowenlan-amzn No regression that I can see. I saw this PR and wanted to get familiar with what it was introducing. I thought I'd leave a fly-by comment for future reference in case anyone wants to know where this threadcontext header originally comes from.

Signed-off-by: Petar <petar.dzepina@gmail.com>

bowenlan-amzn

Thanks!

Signed-off-by: Petar <petar.dzepina@gmail.com>

* Added user_info injection of User object in InjectSecurity Signed-off-by: Petar <petar.dzepina@gmail.com> (cherry picked from commit f7639aa)

… (#399) * Added user_info injection of User object in InjectSecurity Signed-off-by: Petar <petar.dzepina@gmail.com> (cherry picked from commit f7639aa) Co-authored-by: Petar Dzepina <petar.dzepina@gmail.com>

…nsearch-project#396) (opensearch-project#399) * Added user_info injection of User object in InjectSecurity Signed-off-by: Petar <petar.dzepina@gmail.com> (cherry picked from commit f7639aa) Co-authored-by: Petar Dzepina <petar.dzepina@gmail.com>

… (#399) (#406) * Added user_info injection of User object in InjectSecurity (cherry picked from commit f7639aa) Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com> Co-authored-by: Petar Dzepina <petar.dzepina@gmail.com>

… (#403) * Added user_info injection of User object in InjectSecurity Signed-off-by: Petar <petar.dzepina@gmail.com> (cherry picked from commit f7639aa) Co-authored-by: Petar Dzepina <petar.dzepina@gmail.com>

… (#401) * Added user_info injection of User object in InjectSecurity Signed-off-by: Petar <petar.dzepina@gmail.com> (cherry picked from commit f7639aa) Co-authored-by: Petar Dzepina <petar.dzepina@gmail.com>

… (#404) * Added user_info injection of User object in InjectSecurity Signed-off-by: Petar <petar.dzepina@gmail.com> (cherry picked from commit f7639aa) Co-authored-by: Petar Dzepina <petar.dzepina@gmail.com>

… (#402) * Added user_info injection of User object in InjectSecurity Signed-off-by: Petar <petar.dzepina@gmail.com> (cherry picked from commit f7639aa) Co-authored-by: Petar Dzepina <petar.dzepina@gmail.com>

… (#400) * Added user_info injection of User object in InjectSecurity Signed-off-by: Petar <petar.dzepina@gmail.com> (cherry picked from commit f7639aa) Co-authored-by: Petar Dzepina <petar.dzepina@gmail.com>

… (#398) * Added user_info injection of User object in InjectSecurity Signed-off-by: Petar <petar.dzepina@gmail.com> (cherry picked from commit f7639aa) Co-authored-by: Petar Dzepina <petar.dzepina@gmail.com>

… (#397) * Added user_info injection of User object in InjectSecurity Signed-off-by: Petar <petar.dzepina@gmail.com> (cherry picked from commit f7639aa) Co-authored-by: Petar Dzepina <petar.dzepina@gmail.com>

…nsearch-project#396) (opensearch-project#399) * Added user_info injection of User object in InjectSecurity Signed-off-by: Petar <petar.dzepina@gmail.com> (cherry picked from commit f7639aa) Co-authored-by: Petar Dzepina <petar.dzepina@gmail.com> Signed-off-by: AWSHurneyt <hurneyt@amazon.com>

petardz requested a review from a team April 11, 2023 22:24

Added user_info injection of User object in InjectSecurity

0053ee6

Signed-off-by: Petar <petar.dzepina@gmail.com>

petardz force-pushed the injectsecurity-userinfo branch from 90f1c8f to 0053ee6 Compare April 13, 2023 18:26

lezzago previously approved these changes Apr 13, 2023

View reviewed changes

lezzago mentioned this pull request Apr 13, 2023

Notification security fix opensearch-project/alerting#852

Merged

lezzago added backport 2.0 backport 2.1 backport 2.x backport 2.2 backport 2.3 backport 2.4 backport 2.5 backport 2.6 labels Apr 13, 2023

eirsep reviewed Apr 13, 2023

View reviewed changes

bowenlan-amzn reviewed Apr 13, 2023

View reviewed changes

petardz dismissed lezzago’s stale review via 67e9728 April 13, 2023 19:49

Added better logging, comments and unit test

98eb7f1

Signed-off-by: Petar <petar.dzepina@gmail.com>

petardz force-pushed the injectsecurity-userinfo branch from 67e9728 to 98eb7f1 Compare April 13, 2023 19:50

bowenlan-amzn previously approved these changes Apr 13, 2023

View reviewed changes

lezzago previously approved these changes Apr 13, 2023

View reviewed changes

lezzago self-requested a review April 13, 2023 20:29

petardz dismissed stale reviews from lezzago and bowenlan-amzn via 41890cf April 13, 2023 20:49

spotless fix

a85eba0

Signed-off-by: Petar <petar.dzepina@gmail.com>

petardz force-pushed the injectsecurity-userinfo branch from 41890cf to a85eba0 Compare April 13, 2023 20:52

eirsep approved these changes Apr 13, 2023

View reviewed changes

lezzago approved these changes Apr 13, 2023

View reviewed changes

lezzago merged commit f7639aa into opensearch-project:main Apr 13, 2023

opensearch-trigger-bot bot mentioned this pull request Apr 13, 2023

[Backport 2.0] InjectSecurity - inject User object in UserInfo in threadContext #397

Merged

opensearch-trigger-bot bot mentioned this pull request Apr 13, 2023

[Backport 2.1] InjectSecurity - inject User object in UserInfo in threadContext #398

Merged

This was referenced Apr 13, 2023

[Backport 2.x] InjectSecurity - inject User object in UserInfo in threadContext #399

Merged

[Backport 2.2] InjectSecurity - inject User object in UserInfo in threadContext #400

Merged

opensearch-trigger-bot bot mentioned this pull request Apr 13, 2023

[Backport 2.3] InjectSecurity - inject User object in UserInfo in threadContext #401

Merged

opensearch-trigger-bot bot mentioned this pull request Apr 13, 2023

[Backport 2.4] InjectSecurity - inject User object in UserInfo in threadContext #402

Merged

This was referenced Apr 13, 2023

[Backport 2.5] InjectSecurity - inject User object in UserInfo in threadContext #403

Merged

[Backport 2.6] InjectSecurity - inject User object in UserInfo in threadContext #404

Merged

petardz deleted the injectsecurity-userinfo branch April 27, 2023 08:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

InjectSecurity - inject User object in UserInfo in threadContext #396

InjectSecurity - inject User object in UserInfo in threadContext #396

Uh oh!

petardz commented Apr 11, 2023 •

edited by bowenlan-amzn

Loading

Uh oh!

codecov-commenter commented Apr 11, 2023 •

edited

Loading

Uh oh!

eirsep Apr 13, 2023

Uh oh!

bowenlan-amzn Apr 13, 2023

Uh oh!

petardz Apr 13, 2023

Uh oh!

bowenlan-amzn Apr 13, 2023

Uh oh!

petardz Apr 13, 2023

Uh oh!

cwperks Apr 27, 2023

Uh oh!

bowenlan-amzn Apr 27, 2023

Uh oh!

cwperks May 5, 2023

Uh oh!

bowenlan-amzn left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

InjectSecurity - inject User object in UserInfo in threadContext #396

InjectSecurity - inject User object in UserInfo in threadContext #396

Uh oh!

Conversation

petardz commented Apr 11, 2023 • edited by bowenlan-amzn Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

codecov-commenter commented Apr 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bowenlan-amzn left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

petardz commented Apr 11, 2023 •

edited by bowenlan-amzn

Loading

codecov-commenter commented Apr 11, 2023 •

edited

Loading