concord-server: reduce Shiro's Subject usage

walmartlabs · Apr 7, 2024 · a345b7e · a345b7e
1 parent 47cd347
commit a345b7e
Show file tree

Hide file tree

Showing 21 changed files with 69 additions and 61 deletions.
diff --git a/...rc/main/java/com/walmartlabs/concord/server/boot/filters/ConcordAuthenticatingFilter.java b/...rc/main/java/com/walmartlabs/concord/server/boot/filters/ConcordAuthenticatingFilter.java
@@ -24,7 +24,7 @@
 import com.fasterxml.jackson.databind.ObjectMapper;
 import com.walmartlabs.concord.server.RequestUtils;
 import com.walmartlabs.concord.server.sdk.metrics.InjectMeter;
-import com.walmartlabs.concord.server.security.PrincipalUtils;
+import com.walmartlabs.concord.server.security.SecurityUtils;
 import com.walmartlabs.concord.server.security.apikey.ApiKey;
 import org.apache.shiro.authc.AuthenticationException;
 import org.apache.shiro.authc.AuthenticationToken;
@@ -80,7 +80,7 @@ public ConcordAuthenticatingFilter(Set<AuthenticationHandler> authenticationHand
 
     @Override
     protected AuthenticationToken createToken(ServletRequest request, ServletResponse response) {
-        Subject subject = PrincipalUtils.getSubject();
+        Subject subject = SecurityUtils.getSubject();
         if (subject != null && subject.isRemembered()) {
             AuthenticationToken t = getFirstToken(subject);
             if (t != null) {

diff --git a/server/impl/src/main/java/com/walmartlabs/concord/server/console/ConsoleService.java b/server/impl/src/main/java/com/walmartlabs/concord/server/console/ConsoleService.java
@@ -41,15 +41,14 @@
 import com.walmartlabs.concord.server.sdk.metrics.WithTimer;
 import com.walmartlabs.concord.server.sdk.rest.Resource;
 import com.walmartlabs.concord.server.sdk.validation.Validate;
-import com.walmartlabs.concord.server.security.PrincipalUtils;
+import com.walmartlabs.concord.server.security.SecurityUtils;
 import com.walmartlabs.concord.server.security.UnauthorizedException;
 import com.walmartlabs.concord.server.security.UserPrincipal;
 import com.walmartlabs.concord.server.security.apikey.ApiKeyDao;
 import com.walmartlabs.concord.server.security.ldap.LdapGroupSearchResult;
 import com.walmartlabs.concord.server.security.ldap.LdapPrincipal;
 import com.walmartlabs.concord.server.user.UserEntry;
 import com.walmartlabs.concord.server.user.UserManager;
-import org.apache.shiro.subject.Subject;
 
 import javax.inject.Inject;
 import javax.validation.constraints.Size;
@@ -142,8 +141,7 @@ public UserResponse whoami() {
     @POST
     @Path("/logout")
     public void logout() {
-        Subject subject = PrincipalUtils.getSubject();
-        subject.logout();
+        SecurityUtils.logout();
     }
 
     @GET

diff --git a/server/impl/src/main/java/com/walmartlabs/concord/server/events/TriggerProcessExecutor.java b/server/impl/src/main/java/com/walmartlabs/concord/server/events/TriggerProcessExecutor.java
@@ -33,10 +33,9 @@
 import com.walmartlabs.concord.server.sdk.ConcordApplicationException;
 import com.walmartlabs.concord.server.sdk.PartialProcessKey;
 import com.walmartlabs.concord.server.sdk.metrics.WithTimer;
-import com.walmartlabs.concord.server.security.PrincipalUtils;
 import com.walmartlabs.concord.server.security.Roles;
+import com.walmartlabs.concord.server.security.SecurityUtils;
 import com.walmartlabs.concord.server.user.UserEntry;
-import org.apache.shiro.subject.Subject;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -114,7 +113,7 @@ public List<PartialProcessKey> execute(Event event,
         return triggers.stream()
                 .filter(t -> !isRepositoryDisabled(t))
                 .map(t -> submitProcess(event, t, initiatorResolver, cfgEnricher, exclusiveResolver))
-                .collect(Collectors.toList()) // collect all "futures"
+                .toList() // collect all "futures"
                 .stream()
                 .map(TriggerProcessExecutor::resolve)
                 .filter(Objects::nonNull)
@@ -132,10 +131,8 @@ private void assertRoles(String eventName) {
             return;
         }
 
-        Subject s = PrincipalUtils.getSubject();
-
         requiredRoles.forEach((k, v) -> {
-            if (eventName.matches(k) && !s.hasRole(v)) {
+            if (eventName.matches(k) && !SecurityUtils.hasRole(v)) {
                 throw new ConcordApplicationException("'" + v + "' role is required", Response.Status.FORBIDDEN);
             }
         });

diff --git a/server/impl/src/main/java/com/walmartlabs/concord/server/process/ProcessSecurityContext.java b/server/impl/src/main/java/com/walmartlabs/concord/server/process/ProcessSecurityContext.java
@@ -25,7 +25,7 @@
 import com.walmartlabs.concord.server.process.state.ProcessStateManager;
 import com.walmartlabs.concord.server.sdk.PartialProcessKey;
 import com.walmartlabs.concord.server.sdk.ProcessKey;
-import com.walmartlabs.concord.server.security.PrincipalUtils;
+import com.walmartlabs.concord.server.security.SecurityUtils;
 import com.walmartlabs.concord.server.security.UnauthorizedException;
 import com.walmartlabs.concord.server.security.UserPrincipal;
 import com.walmartlabs.concord.server.security.internal.InternalRealm;
@@ -77,12 +77,12 @@ public byte[] serializePrincipals(PrincipalCollection src) {
                 dst.add(p, realm);
             }
         }
-        return PrincipalUtils.serialize(dst);
+        return SecurityUtils.serialize(dst);
     }
 
     // TODO: invalidate cache for processKey?
     public void storeCurrentSubject(ProcessKey processKey) {
-        Subject s = PrincipalUtils.getSubject();
+        Subject s = SecurityUtils.getSubject();
         PrincipalCollection src = s.getPrincipals();
         storeSubject(processKey, src);
     }
@@ -101,7 +101,7 @@ public PrincipalCollection getPrincipals(PartialProcessKey processKey) {
     }
 
     private PrincipalCollection doGetPrincipals(PartialProcessKey processKey) {
-        return stateManager.get(processKey, PRINCIPAL_FILE_PATH, PrincipalUtils::deserialize)
+        return stateManager.get(processKey, PRINCIPAL_FILE_PATH, SecurityUtils::deserialize)
                 .orElse(null);
     }
 

diff --git a/.../com/walmartlabs/concord/server/process/pipelines/processors/PayloadRestoreProcessor.java b/.../com/walmartlabs/concord/server/process/pipelines/processors/PayloadRestoreProcessor.java
@@ -31,7 +31,7 @@
 import com.walmartlabs.concord.server.sdk.ConcordApplicationException;
 import com.walmartlabs.concord.server.sdk.ProcessKey;
 import com.walmartlabs.concord.server.sdk.metrics.WithTimer;
-import com.walmartlabs.concord.server.security.PrincipalUtils;
+import com.walmartlabs.concord.server.security.SecurityUtils;
 import org.apache.shiro.subject.PrincipalCollection;
 
 import javax.inject.Inject;
@@ -85,7 +85,7 @@ public Payload process(Chain chain, Payload payload) {
         });
         payload = payload.putAttachments(attachments);
 
-        PrincipalCollection principals = stateManager.getInitial(processKey, "initiator", PrincipalUtils::deserialize)
+        PrincipalCollection principals = stateManager.getInitial(processKey, "initiator", SecurityUtils::deserialize)
                 .orElseThrow(() -> new ConcordApplicationException("Process initiator not found", Response.Status.INTERNAL_SERVER_ERROR));
 
         securityContext.storeSubject(processKey, principals);

diff --git a/...va/com/walmartlabs/concord/server/process/pipelines/processors/PayloadStoreProcessor.java b/...va/com/walmartlabs/concord/server/process/pipelines/processors/PayloadStoreProcessor.java
@@ -29,7 +29,7 @@
 import com.walmartlabs.concord.server.process.state.ProcessStateManager;
 import com.walmartlabs.concord.server.sdk.ProcessKey;
 import com.walmartlabs.concord.server.sdk.metrics.WithTimer;
-import com.walmartlabs.concord.server.security.PrincipalUtils;
+import com.walmartlabs.concord.server.security.SecurityUtils;
 
 import javax.inject.Inject;
 import javax.inject.Named;
@@ -83,7 +83,7 @@ public Payload process(Chain chain, Payload payload) {
 
         stateManager.tx(tx -> {
             stateManager.insertInitial(tx, processKey, "payload.json", serializedHeaders.getBytes());
-            stateManager.insertInitial(tx, processKey, "initiator", securityContext.serializePrincipals(PrincipalUtils.getSubject().getPrincipals()));
+            stateManager.insertInitial(tx, processKey, "initiator", securityContext.serializePrincipals(SecurityUtils.getSubject().getPrincipals()));
             stateManager.importPathInitial(tx, processKey, "attachments/", payload.getHeader(Payload.BASE_DIR), (path, basicFileAttributes) -> payload.getAttachments().containsValue(path));
         });
 

diff --git a/server/impl/src/main/java/com/walmartlabs/concord/server/security/Permission.java b/server/impl/src/main/java/com/walmartlabs/concord/server/security/Permission.java
@@ -21,8 +21,6 @@
  */
 
 
-import org.apache.shiro.subject.Subject;
-
 public enum Permission {
 
     /**
@@ -46,7 +44,7 @@ public enum Permission {
 
     private final String key;
 
-    private Permission(String key) {
+    Permission(String key) {
         this.key = key;
     }
 
@@ -55,7 +53,6 @@ public String getKey() {
     }
 
     public static boolean isPermitted(Permission p) {
-        Subject s = PrincipalUtils.getSubject();
-        return s.isPermitted(p.key);
+        return SecurityUtils.isPermitted(p.getKey());
     }
 }
diff --git a/server/impl/src/main/java/com/walmartlabs/concord/server/security/Roles.java b/server/impl/src/main/java/com/walmartlabs/concord/server/security/Roles.java
@@ -20,9 +20,6 @@
  * =====
  */
 
-
-import org.apache.shiro.subject.Subject;
-
 public final class Roles {
 
     /**
@@ -41,18 +38,15 @@ public final class Roles {
     public static final String SYSTEM_WRITER = "concordSystemWriter";
 
     public static boolean isAdmin() {
-        Subject s = PrincipalUtils.getSubject();
-        return s.hasRole(ADMIN);
+        return SecurityUtils.hasRole(ADMIN);
     }
 
     public static boolean isGlobalReader() {
-        Subject s = PrincipalUtils.getSubject();
-        return s.hasRole(SYSTEM_READER);
+        return SecurityUtils.hasRole(SYSTEM_READER);
     }
 
     public static boolean isGlobalWriter() {
-        Subject s = PrincipalUtils.getSubject();
-        return s.hasRole(SYSTEM_WRITER);
+        return SecurityUtils.hasRole(SYSTEM_WRITER);
     }
 
     private Roles() {

diff --git a/...ncord/server/security/PrincipalUtils.java → ...oncord/server/security/SecurityUtils.java b/...ncord/server/security/PrincipalUtils.java → ...oncord/server/security/SecurityUtils.java
@@ -35,7 +35,29 @@
 import java.util.Optional;
 import java.util.Set;
 
-public final class PrincipalUtils {
+/**
+ * Utility methods for working with Shiro's security context.
+ * Should be the only place where Shiro's API is used directly except for
+ * the security filters.
+ */
+public final class SecurityUtils {
+
+    public static void logout() {
+        Subject subject = getSubject();
+        if (subject != null) {
+            subject.logout();
+        }
+    }
+
+    public static boolean hasRole(String role) {
+        Subject s = getSubject();
+        return s.hasRole(role);
+    }
+
+    public static boolean isPermitted(String permission) {
+        Subject s = getSubject();
+        return s.isPermitted(permission);
+    }
 
     public static Subject getSubject() {
         Subject subject = ThreadContext.getSubject();
@@ -128,6 +150,6 @@ public static AuthorizationInfo toAuthorizationInfo(PrincipalCollection principa
         return i;
     }
 
-    private PrincipalUtils() {
+    private SecurityUtils() {
     }
 }
diff --git a/server/impl/src/main/java/com/walmartlabs/concord/server/security/UserPrincipal.java b/server/impl/src/main/java/com/walmartlabs/concord/server/security/UserPrincipal.java
@@ -35,11 +35,11 @@ public class UserPrincipal implements Serializable {
     private static final long serialVersionUID = 1L;
 
     public static UserPrincipal getCurrent() {
-        return PrincipalUtils.getCurrent(UserPrincipal.class);
+        return SecurityUtils.getCurrent(UserPrincipal.class);
     }
 
     public static UserPrincipal assertCurrent() {
-        return PrincipalUtils.assertCurrent(UserPrincipal.class);
+        return SecurityUtils.assertCurrent(UserPrincipal.class);
     }
 
     private final String realm;

diff --git a/server/impl/src/main/java/com/walmartlabs/concord/server/security/apikey/ApiKeyRealm.java b/server/impl/src/main/java/com/walmartlabs/concord/server/security/apikey/ApiKeyRealm.java
@@ -25,7 +25,7 @@
 import com.walmartlabs.concord.server.audit.AuditObject;
 import com.walmartlabs.concord.server.sdk.metrics.WithTimer;
 import com.walmartlabs.concord.server.sdk.security.AuthenticationException;
-import com.walmartlabs.concord.server.security.PrincipalUtils;
+import com.walmartlabs.concord.server.security.SecurityUtils;
 import com.walmartlabs.concord.server.security.UserPrincipal;
 import com.walmartlabs.concord.server.user.UserEntry;
 import com.walmartlabs.concord.server.user.UserManager;
@@ -88,6 +88,6 @@ protected AuthorizationInfo doGetAuthorizationInfo(PrincipalCollection principal
             return null;
         }
 
-        return PrincipalUtils.toAuthorizationInfo(principals);
+        return SecurityUtils.toAuthorizationInfo(principals);
     }
 }
diff --git a/server/impl/src/main/java/com/walmartlabs/concord/server/security/github/GithubKey.java b/server/impl/src/main/java/com/walmartlabs/concord/server/security/github/GithubKey.java
@@ -20,15 +20,15 @@
  * =====
  */
 
-import com.walmartlabs.concord.server.security.PrincipalUtils;
+import com.walmartlabs.concord.server.security.SecurityUtils;
 import org.apache.shiro.authc.AuthenticationToken;
 
 import java.util.UUID;
 
 public class GithubKey implements AuthenticationToken {
 
     public static GithubKey getCurrent() {
-        return PrincipalUtils.getCurrent(GithubKey.class);
+        return SecurityUtils.getCurrent(GithubKey.class);
     }
 
     private static final long serialVersionUID = 1L;

diff --git a/server/impl/src/main/java/com/walmartlabs/concord/server/security/github/GithubRealm.java b/server/impl/src/main/java/com/walmartlabs/concord/server/security/github/GithubRealm.java
@@ -22,7 +22,7 @@
 
 import com.walmartlabs.concord.server.sdk.metrics.WithTimer;
 import com.walmartlabs.concord.server.sdk.security.AuthenticationException;
-import com.walmartlabs.concord.server.security.PrincipalUtils;
+import com.walmartlabs.concord.server.security.SecurityUtils;
 import com.walmartlabs.concord.server.security.UserPrincipal;
 import com.walmartlabs.concord.server.user.UserManager;
 import org.apache.shiro.authc.AuthenticationInfo;
@@ -82,6 +82,6 @@ protected AuthorizationInfo doGetAuthorizationInfo(PrincipalCollection principal
             return null;
         }
 
-        return PrincipalUtils.toAuthorizationInfo(principals);
+        return SecurityUtils.toAuthorizationInfo(principals);
     }
 }
diff --git a/...er/impl/src/main/java/com/walmartlabs/concord/server/security/internal/InternalRealm.java b/...er/impl/src/main/java/com/walmartlabs/concord/server/security/internal/InternalRealm.java
@@ -21,7 +21,7 @@
  */
 
 import com.walmartlabs.concord.server.sdk.security.AuthenticationException;
-import com.walmartlabs.concord.server.security.PrincipalUtils;
+import com.walmartlabs.concord.server.security.SecurityUtils;
 import com.walmartlabs.concord.server.security.UserPrincipal;
 import org.apache.shiro.authc.AuthenticationInfo;
 import org.apache.shiro.authc.AuthenticationToken;
@@ -46,6 +46,6 @@ protected AuthorizationInfo doGetAuthorizationInfo(PrincipalCollection principal
             return null;
         }
 
-        return PrincipalUtils.toAuthorizationInfo(principals);
+        return SecurityUtils.toAuthorizationInfo(principals);
     }
 }
diff --git a/server/impl/src/main/java/com/walmartlabs/concord/server/security/ldap/LdapPrincipal.java b/server/impl/src/main/java/com/walmartlabs/concord/server/security/ldap/LdapPrincipal.java
@@ -20,7 +20,7 @@
  * =====
  */
 
-import com.walmartlabs.concord.server.security.PrincipalUtils;
+import com.walmartlabs.concord.server.security.SecurityUtils;
 
 import java.io.Serializable;
 import java.util.Map;
@@ -63,7 +63,7 @@ public LdapPrincipal(String username,
     }
 
     public static LdapPrincipal getCurrent() {
-        return PrincipalUtils.getCurrent(LdapPrincipal.class);
+        return SecurityUtils.getCurrent(LdapPrincipal.class);
     }
 
     public String getUsername() {

diff --git a/server/impl/src/main/java/com/walmartlabs/concord/server/security/ldap/LdapRealm.java b/server/impl/src/main/java/com/walmartlabs/concord/server/security/ldap/LdapRealm.java
@@ -26,7 +26,7 @@
 import com.walmartlabs.concord.server.cfg.LdapConfiguration;
 import com.walmartlabs.concord.server.sdk.ConcordApplicationException;
 import com.walmartlabs.concord.server.sdk.metrics.WithTimer;
-import com.walmartlabs.concord.server.security.PrincipalUtils;
+import com.walmartlabs.concord.server.security.SecurityUtils;
 import com.walmartlabs.concord.server.security.UserPrincipal;
 import com.walmartlabs.concord.server.user.UserEntry;
 import com.walmartlabs.concord.server.user.UserManager;
@@ -196,6 +196,6 @@ protected AuthorizationInfo queryForAuthorizationInfo(PrincipalCollection princi
             return null;
         }
 
-        return PrincipalUtils.toAuthorizationInfo(principals);
+        return SecurityUtils.toAuthorizationInfo(principals);
     }
 }
diff --git a/...ain/java/com/walmartlabs/concord/server/security/rememberme/ConcordRememberMeManager.java b/...ain/java/com/walmartlabs/concord/server/security/rememberme/ConcordRememberMeManager.java
@@ -21,7 +21,7 @@
  */
 
 import com.walmartlabs.concord.server.cfg.RememberMeConfiguration;
-import com.walmartlabs.concord.server.security.PrincipalUtils;
+import com.walmartlabs.concord.server.security.SecurityUtils;
 import com.walmartlabs.concord.server.security.apikey.ApiKey;
 import org.apache.shiro.authc.UsernamePasswordToken;
 import org.apache.shiro.io.SerializationException;
@@ -77,12 +77,12 @@ protected void rememberIdentity(Subject subject, PrincipalCollection src) {
     private static class PrincipalCollectionSerializer implements Serializer<PrincipalCollection> {
         @Override
         public byte[] serialize(PrincipalCollection principalCollection) throws SerializationException {
-            return PrincipalUtils.serialize(principalCollection);
+            return SecurityUtils.serialize(principalCollection);
         }
 
         @Override
         public PrincipalCollection deserialize(byte[] bytes) throws SerializationException {
-            return PrincipalUtils.deserialize(bytes).orElse(null);
+            return SecurityUtils.deserialize(bytes).orElse(null);
         }
     }
 }