Add user custom attributes to thread context #5491

markdboyd · 2025-07-18T20:52:29Z

Description

Category: Bug fix
Why these changes are required?

In opensearch-project/alerting#1829 (comment), I was informed that the the user serialization into the threadcontext does not handle custom attributes, which breaks DLS for alert monitors.

What is the old behavior before changes and new behavior after changes?

Without these changes, the serialized user in the thread context does not include custom attribute names. With these changes, the serialized user in the thread context does include custom attribute names.

Related Issues

Related to opensearch-project/alerting#1829

Check List

New functionality includes testing.
New functionality has been documented.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

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.

Testing

I have not done any testing because I was unsure how to implement it or run it

Check List

New functionality includes testing
New functionality has been documented
New Roles/Permissions have a corresponding security dashboards plugin PR
API changes companion pull request created
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.

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java

cwperks · 2025-07-21T00:39:21Z

@markdboyd I think your IDE may have some conflicts with the spotless rules in this repo. Make sure to run ./gradlew spotlessApply to apply the spotless rules prior to push.

cwperks · 2025-07-21T00:52:57Z

@markdboyd FYI for serialization, I was thinking something like this: cwperks#59

i.e. let's base64 encode it to serialize.

I'm thinking that its safe to base64 encode as its done elsewhere (example) and won't clash with the pipe-delimited format.

markdboyd · 2025-07-21T14:04:29Z

@cwperks OK, I applied your feedback on base64 encoding and ran ./gradlew spotlessApply. Let me know if I need to do anything else.

cwperks · 2025-07-21T14:35:30Z

Thank you @markdboyd1 Can you also make sure to sign the commits? (See DCO check failure) Each repo has a DCO check that requires contributors to sign commits as a Developer Certificate of Origin: https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin

I think we should also add an entry in the CHANGELOG for this PR.

markdboyd · 2025-07-21T15:19:36Z

@cwperks Sorry for the amount of commits here. I had to try a couple times to get the rebasing + commit signing to work correctly. But it should be ready now, including a CHANGELOG entry

Also, if you have any suggestions on how to setup git to do automatic signoff of future commits, I would welcome that

cwperks · 2025-07-21T15:41:05Z

Also, if you have any suggestions on how to setup git to do automatic signoff of future commits, I would welcome that

I've gotten in the habit of doing git commit -m "message here" --signoff, but I think other contributors may create an alias like this: https://serverfault.com/a/433639

CHANGELOG.md

cwperks · 2025-07-21T15:48:23Z

FYI tests are failing bc common-utils changes will need to be merged first.

They are failing on User.parse here: https://github.com/opensearch-project/security/blob/main/sample-resource-plugin/src/main/java/org/opensearch/sample/resource/actions/transport/CreateResourceTransportAction.java#L58

cwperks · 2025-07-21T15:50:02Z

Other test error is bc ImmutableMap is not Serializable:

» org.opensearch.OpenSearchException: Instance {} of class class com.google.common.collect.RegularImmutableMap is not serializable
» at org.opensearch.security.support.Base64Helper.serializeObject(Base64Helper.java:84) ~[?:?]
» at org.opensearch.security.privileges.PrivilegesEvaluator.setUserInfoInThreadContext(PrivilegesEvaluator.java:298) ~[?:?]
» at org.opensearch.security.privileges.PrivilegesEvaluator.evaluate(PrivilegesEvaluator.java:397) ~[?:?]

markdboyd · 2025-07-21T15:56:06Z

@cwperks

OK. I'll fix the CHANGELOG.

Any suggestions re: Serializable and ImmutableMap? Base64Helper.serializeObject wouldn't work on user.getCustomAttributesMap() without Serializable. I took the idea from this code.

Also the common-utils PR is up and ready for review as well if that needs to go first: opensearch-project/common-utils#827

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java

shikharj05 · 2025-07-21T16:07:29Z

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java

            }
+
+            joiner.add(Base64Helper.serializeObject((Serializable) user.getCustomAttributesMap()));


This can lead to performance issues depending on size of custom attributes - should this be made selective using a role configuration or authz configuration?

Sure. Can you provide any guidance as to how that would work? I'm a novice Java developer with minimal overall knowledge of the OpenSearch codebase

@shikharj05 would a feature flag be useful here? IMO all attributes should be serialized bc user role definitions can change at any time.

@cwperks do you think it's possible to look at role definition, identify the dynamic parameter substitution required and just serilalize this specific parameter?

I think the feature flag approach would be simpler. We could also add another setting in the future for additional controls for an allowlist of which attributes to specifically include when serializing instead of serializing all.

cwperks · 2025-07-21T17:01:28Z

@markdboyd for an example of a feature flag see: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java#L1365-L1373

Essentially, we'd add a setting that an admin can place in opensearch.yml to enable/disable serialization of user attributes.

i.e.

plugins.security.user_attribute_serialization.enabled: true

cwperks · 2025-07-21T18:17:44Z

To resolve the serialization issue with ImmutableMap, we need to add that as a Safe class here: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/support/SafeSerializationUtils.java#L41-L54

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java

codecov · 2025-07-22T00:08:18Z

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 72.75%. Comparing base (3121d88) to head (c90eae9).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
...earch/security/privileges/PrivilegesEvaluator.java	50.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5491      +/-   ##
==========================================
+ Coverage   72.72%   72.75%   +0.02%     
==========================================
  Files         397      397              
  Lines       24589    24597       +8     
  Branches     3740     3742       +2     
==========================================
+ Hits        17882    17895      +13     
+ Misses       4881     4872       -9     
- Partials     1826     1830       +4

Files with missing lines	Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java	`85.01% <100.00%> (+0.03%)`	⬆️
...g/opensearch/security/support/ConfigConstants.java	`95.23% <ø> (ø)`
...earch/security/support/SafeSerializationUtils.java	`86.66% <ø> (ø)`
...earch/security/privileges/PrivilegesEvaluator.java	`74.57% <50.00%> (-0.29%)`	⬇️

... and 5 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

DarshitChanpura

@markdboyd Thank you for adding this fix! I've left some comments and couple of clarification questions.

...esource-plugin/src/integrationTest/java/org/opensearch/sample/SampleResourcePluginTests.java

...ain/java/org/opensearch/sample/resource/actions/transport/CreateResourceTransportAction.java

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java

DarshitChanpura · 2025-07-23T21:30:02Z

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java

            }
+
+            if (this.isUserAttributeSerializationEnabled()) {
+                joiner.add(Base64Helper.serializeObject((Serializable) user.getCustomAttributesMap()));


Base64 encoding might still raise performance concerns if the user has thousands of attributes. Not a blocker for this PR, but we should test that scenario and add a fix in a follow-up PR.

cwperks · 2025-07-24T15:31:21Z

Note: integ tests for the sample plugin will fail until opensearch-project/common-utils#827 is merged.

attempt reading injected attributes in RolesInjector add custom attributes to user information in thread context add more logging to injectUserAndRoles add debugging statements update serialization to include only custom attribute name revert changes to RolesInjector revert changes to UserInjector revert changes to SecurityFilter Signed-off-by: Mark Boyd <mark.boyd@gsa.gov> update setUserInfoInThreadContext to base64 encode the serialized map of user custom attributes Signed-off-by: Mark Boyd <mark.boyd@gsa.gov> add null tenant to user context when none is available Signed-off-by: Mark Boyd <mark.boyd@gsa.gov> Immutable user object (opensearch-project#5212) Signed-off-by: Nils Bandener <nils.bandener@eliatra.com> Signed-off-by: Mark Boyd <mark.boyd@gsa.gov> apply spotless formatting Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

…en requested tenant is empty Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

…Utils Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

…control whether user attributes are serialized Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

…k if user attribute serialization is enabled before adding serialized user attributes to the thread context Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

…t consistent Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

…text Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

…te file Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

DarshitChanpura

Thanks @markdboyd ! LGTM. lets keep this feature behind feature flag for now.

markdboyd requested review from cwperks, DarshitChanpura, derek-ho, nibix, peternied, RyanL1997, reta, shikharj05 and willyborankin as code owners July 18, 2025 20:52

markdboyd mentioned this pull request Jul 18, 2025

[BUG] Alerting does not work when the last person to create/update has a role with parameter substitution in the DLS opensearch-project/alerting#1829

Open

markdboyd commented Jul 18, 2025

View reviewed changes

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java Outdated Show resolved Hide resolved

markdboyd force-pushed the add-user-attributes-thread-context branch 4 times, most recently from a614db2 to 6d44822 Compare July 21, 2025 15:17

cwperks reviewed Jul 21, 2025

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

markdboyd force-pushed the add-user-attributes-thread-context branch from c07b657 to d4887ff Compare July 21, 2025 16:00

shikharj05 reviewed Jul 21, 2025

View reviewed changes

markdboyd force-pushed the add-user-attributes-thread-context branch from 125ea6b to 2ec314c Compare July 21, 2025 16:38

cwperks reviewed Jul 21, 2025

View reviewed changes

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java Outdated Show resolved Hide resolved

cwperks reviewed Jul 21, 2025

View reviewed changes

src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java Outdated Show resolved Hide resolved

markdboyd force-pushed the add-user-attributes-thread-context branch from 4bd2d5c to c90eae9 Compare July 21, 2025 19:10

DarshitChanpura reviewed Jul 23, 2025

View reviewed changes

cwperks mentioned this pull request Jul 24, 2025

Update user parsing to include custom attributes opensearch-project/common-utils#827

Open

5 tasks

markdboyd added 21 commits July 24, 2025 15:20

re-add base64 encoding of serialized user custom attributes & null wh…

d4bc906

…en requested tenant is empty Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

add entry to CHANGELOG

453070c

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

remove extraneous edits to CHANGELOG

0935b4d

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

remove change to add null requested tenant

b70264a

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

add ImmutableMap to SAFE_ASSIGNABLE_FROM_CLASSES in SafeSerialization…

a84479f

…Utils Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

add plugins.security.user_attribute_serialization.enabled setting to …

5c21dbf

…control whether user attributes are serialized Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

add PrivilegesEvaluator.isUserAttributeSerializationEnabled() to chec…

a2cd321

…k if user attribute serialization is enabled before adding serialized user attributes to the thread context Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

apply feedback and spotless formatting

e1c337f

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

re-add null requested tenant to keep number of items in thread contex…

9611bba

…t consistent Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

fix typo

d96a9a9

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

update TestSecurityConfig.User to have custom user attributes

0092da1

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

log parsed user in CreateResourceTransportAction.doExecute

b3fbe92

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

add SampleResourcePluginTests.testUserSerializationAndDeserialization

963fffa

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

update SafeSerializationUtils.SAFE_CLASS_NAMES

a2e2407

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

update common_utils_version for sample-resource-plugin

641ce35

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

add integrationTestImplementation for sample-resource-plugin

8becb7f

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

apply spotless formatting

003585c

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

refactor code for handling requested tenant in setUserInfoInThreadCon…

7f8ece8

…text Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

move user serialization/deserialization integration tests to a separa…

03d0be6

…te file Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

remove debugging output

4f0ee5b

Signed-off-by: Mark Boyd <mark.boyd@gsa.gov>

markdboyd force-pushed the add-user-attributes-thread-context branch from 8971760 to 4f0ee5b Compare July 24, 2025 19:20

DarshitChanpura approved these changes Jul 24, 2025

View reviewed changes

		}

		joiner.add(Base64Helper.serializeObject((Serializable) user.getCustomAttributesMap()));

Add user custom attributes to thread context #5491

Are you sure you want to change the base?

Add user custom attributes to thread context #5491

Uh oh!

Conversation

markdboyd commented Jul 18, 2025

Description

Related Issues

Check List

Testing

Check List

Uh oh!

Uh oh!

cwperks commented Jul 21, 2025

Uh oh!

cwperks commented Jul 21, 2025

Uh oh!

markdboyd commented Jul 21, 2025

Uh oh!

cwperks commented Jul 21, 2025

Uh oh!

markdboyd commented Jul 21, 2025

Uh oh!

cwperks commented Jul 21, 2025

Uh oh!

Uh oh!

cwperks commented Jul 21, 2025

Uh oh!

cwperks commented Jul 21, 2025

Uh oh!

markdboyd commented Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

shikharj05 Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

markdboyd Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

cwperks Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

shikharj05 Jul 21, 2025

Choose a reason for hiding this comment

Uh oh!

cwperks Jul 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwperks commented Jul 21, 2025

Uh oh!

cwperks commented Jul 21, 2025

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jul 22, 2025

Codecov Report

Uh oh!

DarshitChanpura left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DarshitChanpura Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

cwperks commented Jul 24, 2025

Uh oh!

DarshitChanpura left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

markdboyd commented Jul 21, 2025 •

edited

Loading

cwperks Jul 21, 2025 •

edited

Loading