From 5e41865f42f23d950e9cc1daa1c71a0e4c27a6b6 Mon Sep 17 00:00:00 2001
From: Craig Perkins <cwperx@amazon.com>
Date: Mon, 7 Aug 2023 16:50:29 -0400
Subject: [PATCH] Reverse order of setUserInfoInThreadContext and
 addSecurityRoles to resolve ConcurrentModificationException on bulk request
 (#3094)

This PR changes the order of `setUserInfoInThreadContext` and
`addSecurityRoles` to ensure that `addSecurityRoles` is called before
`setUserInfoInThreadContext`. The ConcurrentModificationException would
arise in scenarios where the first thread was done with
`setUserInfoInThreadContext` and had moved onto `addSecurityRoles` to
add the mapped roles into the user's set of security roles. On the first
call to `addSecurityRoles` it may be populating the set with data, any
subsequent call would be a noop in other threads.

If simultaneously there is another thread executing
`setUserInfoInThreadContext` while the first thread is in
`addSecurityRoles` then a ConcurrentModificationException is thrown
inside the `Sets.union(...)` call.

By calling `addSecurityRoles` before `setUserInfoInThreadContext`, it
can be guaranteed that no ConcurrentModificationException could be
thrown because the user's security roles will already be set and any
thread that attempts another call will be a noop.

* Category (Enhancement, New feature, Bug fix, Test fix, Refactoring,
Maintenance, Documentation)

Bug fix

https://github.com/opensearch-project/security/issues/2263

Tested by running a python script to bulk insert 100k documents and
using tmux to run the script in multiple shells at once. Before the
change the bug is reproducible regularly, but after the change the bug
cannot be reproduced.

- [ ] New functionality includes testing
- [ ] New functionality has been documented
- [ ] 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](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

---------

Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <craig5008@gmail.com>
(cherry picked from commit cd699bb7d3a07b8919ef2fb5e8fb4ccd2e622acb)
---
 .../security/privileges/PrivilegesEvaluator.java       | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java
index 240e7bf9f2..524061338a 100644
--- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java
+++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java
@@ -99,8 +99,6 @@
 import org.opensearch.security.support.WildcardMatcher;
 import org.opensearch.security.user.User;
 
-import com.google.common.collect.Sets;
-
 import static org.opensearch.security.OpenSearchSecurityPlugin.traceAction;
 import static org.opensearch.security.support.ConfigConstants.OPENDISTRO_SECURITY_USER_INFO_THREAD_CONTEXT;
 
@@ -184,12 +182,12 @@ public boolean isInitialized() {
         return configModel !=null && configModel.getSecurityRoles() != null && dcm != null;
     }
 
-    private void setUserInfoInThreadContext(User user, Set<String> mappedRoles) {
+    private void setUserInfoInThreadContext(User user) {
         if (threadContext.getTransient(OPENDISTRO_SECURITY_USER_INFO_THREAD_CONTEXT) == null) {
             StringJoiner joiner = new StringJoiner("|");
             joiner.add(user.getName());
             joiner.add(String.join(",", user.getRoles()));
-            joiner.add(String.join(",", Sets.union(user.getSecurityRoles(), mappedRoles)));
+            joiner.add(String.join(",", user.getSecurityRoles()));
             String requestedTenant = user.getRequestedTenant();
             if (!Strings.isNullOrEmpty(requestedTenant)) {
                 joiner.add(requestedTenant);
@@ -235,7 +233,9 @@ public PrivilegesEvaluatorResponse evaluate(final User user, String action0, fin
         presponse.resolvedSecurityRoles.addAll(mappedRoles);
         final SecurityRoles securityRoles = getSecurityRoles(mappedRoles);
 
-        setUserInfoInThreadContext(user, mappedRoles);
+        // Add the security roles for this user so that they can be used for DLS parameter substitution.
+        user.addSecurityRoles(mappedRoles);
+        setUserInfoInThreadContext(user);
 
         final boolean isDebugEnabled = log.isDebugEnabled();
         if (isDebugEnabled) {