Update user attributes XContent parsing logic

[markdboyd]

Description

A previous PR, #827, introduced a change to write user custom attributes to a custom_attributes property when serializing a User to XContent.

However, this change has an adverse affect on many downstream plugins which do not expect a custom_attributes property for a user in their mappings: https://github.com/search?q=org%3Aopensearch-project+custom_attribute_names+language%3AJSON&type=code&l=JSON

To avoid breaking changes for downstream plugins while still representing the custom attributes in the XContent, this PR writes custom attributes to the custom_attribute_names property of XContent in the form of key=value. While it is admittedly confusing to have a property named custom_attribute_names that actually contains both custom attribute names and values, it is a worthwhile approach to avoid the breaking changes on downstream plugins.

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.

[dbwiddis]

To avoid breaking changes for downstream plugins while still representing the custom attributes in the XContent, this PR writes custom attributes to the custom_attribute_names property of XContent in the form of key=value. While it is admittedly confusing to have a property named custom_attribute_names that actually contains both custom attribute names and values, it is a worthwhile approach to avoid the breaking changes on downstream plugins.

@markdboyd @cwperks This PR appears to be a breaking change to downstream plugins. See opensearch-project/flow-framework#1235

(For clarity: just tests making wrong assumptions!)

[dbwiddis]

@markdboyd @cwperks and anyone else coming here, looks like I need to update the tests to match what they should have been all along. Sample fix: https://github.com/opensearch-project/security-analytics/pull/1583/files

[cwperks]

Thank you @dbwiddis! I should have referenced that PR in a comment in case anyone was looking. I know ISM and AD have had to consume the change as well.

Initially, we thought custom_attribute_names was only supposed to be the attribute names (as the name implies), but the alerting plugin had a test that showed key=value in this field: https://github.com/opensearch-project/alerting/blob/3.2/alerting/src/test/kotlin/org/opensearch/alerting/TestHelpers.kt#L667

The user attributes were never serialized by the security plugin, so we found that we have many plugins with existing system index mappings that have custom_attribute_names and they just sit empty without attributes.

@markdboyd made a series of changes to introduce a feature flag that allows a cluster admin to turn on user attribute serialization to start using this field. Its solving a bug in the Alerting repo and I suspect the fix will solve the same problem in other plugins as well.

This PR does enforce that each entry of custom_attribute_names is of the format key=value so plugins that create test users that do not conform to this format would get an error. It should be a quick update and its great to see this field now being plumbed and utilized.

At one point, we contemplated adding to existing system index mappings (see opensearch-project/index-management#1484), but turns out it was possible to re-use the existing custom_attribute_names since it was never connected from security to downstream plugins.

Thank you to @markdboyd for the fix.

Here's a collection of the PRs that went into the bugfix:

• Add user custom attributes to thread context security#5491
• Inject user custom attributes security#5560
• Update user attributes XContent parsing logic #878
• Update user parsing to include custom attributes #827
• Use custom attributes alerting#1917

[dbwiddis]

All good and thanks for the clarity @cwperks . This looks like a great change.

I was mostly amused that my repo broke on something specifically talking about avoiding breaking downstream repos... but only ones that were already broken. Just lazy with tests.

Appreciate the pointer to the quick fix.