Reverse order of setUserInfoInThreadContext and addSecurityRoles to r…

…esolve ConcurrentModificationException on bulk request (opensearch-project#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

opensearch-project#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 <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
(cherry picked from commit cd699bb)

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) {