From dbd2e9342f7839b54b595e41cc2f818e10feac37 Mon Sep 17 00:00:00 2001
From: Domenico Francesco Bruscino <brusdev@apache.org>
Date: Thu, 28 Nov 2024 10:21:49 +0100
Subject: [PATCH] ARTEMIS-5187 Fix management authorization checks after
 authentication failures

When the ArtemisRbacMBeanServerBuilder class is used for the RBAC management,
a clash of authentication cache keys between clients failing authentication and
web console authenticated users can cause web console authenticated users to
receive authorization errors, blank screens and similar issues after successful
login to the console.
---
 .../core/security/impl/SecurityStoreImpl.java | 10 ++--
 .../security/impl/SecurityStoreImplTest.java  | 52 +++++++++++++++++++
 2 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/artemis-server/src/main/java/org/apache/activemq/artemis/core/security/impl/SecurityStoreImpl.java b/artemis-server/src/main/java/org/apache/activemq/artemis/core/security/impl/SecurityStoreImpl.java
index ec7b50ca8ab..abec6465866 100644
--- a/artemis-server/src/main/java/org/apache/activemq/artemis/core/security/impl/SecurityStoreImpl.java
+++ b/artemis-server/src/main/java/org/apache/activemq/artemis/core/security/impl/SecurityStoreImpl.java
@@ -442,16 +442,16 @@ private void authenticationFailed(String user, RemotingConnection connection) th
     * @return the authenticated Subject with all associated role principals
     */
    private Subject getSubjectForAuthorization(SecurityAuth auth, ActiveMQSecurityManager5 securityManager) {
-      String authnCacheKey = createAuthenticationCacheKey(auth.getUsername(), auth.getPassword(), auth.getRemotingConnection());
-      Pair<Boolean, Subject> cached = getAuthenticationCacheEntry(authnCacheKey);
-
-      if (cached == null && auth.getUsername() == null && auth.getPassword() == null && auth.getRemotingConnection() instanceof ManagementRemotingConnection) {
+      if (auth.getUsername() == null && auth.getPassword() == null && auth.getRemotingConnection() instanceof ManagementRemotingConnection) {
          AccessControlContext accessControlContext = AccessController.getContext();
          if (accessControlContext != null) {
-            cached = new Pair<>(true, Subject.getSubject(accessControlContext));
+            return Subject.getSubject(accessControlContext);
          }
       }
 
+      String authnCacheKey = createAuthenticationCacheKey(auth.getUsername(), auth.getPassword(), auth.getRemotingConnection());
+      Pair<Boolean, Subject> cached = getAuthenticationCacheEntry(authnCacheKey);
+
       /*
        * We don't need to worry about the cached boolean being false as users always have to
        * successfully authenticate before requesting authorization for anything.
diff --git a/artemis-server/src/test/java/org/apache/activemq/artemis/core/security/impl/SecurityStoreImplTest.java b/artemis-server/src/test/java/org/apache/activemq/artemis/core/security/impl/SecurityStoreImplTest.java
index 016aa944015..5dc808e07c1 100644
--- a/artemis-server/src/test/java/org/apache/activemq/artemis/core/security/impl/SecurityStoreImplTest.java
+++ b/artemis-server/src/test/java/org/apache/activemq/artemis/core/security/impl/SecurityStoreImplTest.java
@@ -18,10 +18,15 @@
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.fail;
 
 import javax.security.auth.Subject;
+import java.security.PrivilegedExceptionAction;
 import java.util.Set;
 
+import org.apache.activemq.artemis.api.core.ActiveMQSecurityException;
+import org.apache.activemq.artemis.api.core.SimpleString;
+import org.apache.activemq.artemis.core.management.impl.ManagementRemotingConnection;
 import org.apache.activemq.artemis.core.security.CheckType;
 import org.apache.activemq.artemis.core.security.Role;
 import org.apache.activemq.artemis.core.security.SecurityAuth;
@@ -32,6 +37,8 @@
 import org.apache.activemq.artemis.spi.core.security.jaas.UserPrincipal;
 import org.apache.activemq.artemis.utils.RandomUtil;
 import org.junit.jupiter.api.Test;
+import org.mockito.ArgumentMatchers;
+import org.mockito.Mockito;
 
 public class SecurityStoreImplTest {
 
@@ -113,4 +120,49 @@ public void getCaller() throws Exception {
       assertEquals("joe", securityStore.getCaller("joe", subject));
    }
 
+   @Test
+   public void testManagementAuthorizationAfterNullAuthenticationFailure() throws Exception {
+      ActiveMQSecurityManager5 securityManager = Mockito.mock(ActiveMQSecurityManager5.class);
+      Mockito.when(securityManager.authorize(ArgumentMatchers.any(Subject.class),
+          ArgumentMatchers.isNull(),
+          ArgumentMatchers.any(CheckType.class),
+          ArgumentMatchers.anyString())).
+          thenReturn(true);
+
+      SecurityStoreImpl securityStore = new SecurityStoreImpl(
+          new HierarchicalObjectRepository<>(),
+          securityManager,
+          10000,
+          true,
+          "",
+          null,
+          null,
+          1000,
+          1000);
+
+      try {
+         securityStore.authenticate(null, null, Mockito.mock(RemotingConnection.class), null);
+         fail("Authentication must fail");
+      } catch (Throwable t) {
+         assertEquals(ActiveMQSecurityException.class, t.getClass());
+      }
+
+      SecurityAuth session = Mockito.mock(SecurityAuth.class);
+      Mockito.when(session.getRemotingConnection()).thenReturn(new ManagementRemotingConnection());
+
+      Subject viewSubject = new Subject();
+      viewSubject.getPrincipals().add(new UserPrincipal("v"));
+      viewSubject.getPrincipals().add(new RolePrincipal("viewers"));
+
+      Object checkResult = Subject.doAs(viewSubject, (PrivilegedExceptionAction<Object>) () -> {
+         try {
+            securityStore.check(SimpleString.of("test"), CheckType.VIEW, session);
+            return true;
+         } catch (Exception ignore) {
+            return false;
+         }
+      });
+
+      assertEquals(true, checkResult);
+   }
 }