SND Role Impersonation (#207)

* Use hasAllPermissions instead of hasPermission in SNDSecurityManager * Pass in roles to hasAllPermissions when doing Role Impersonation * Add better error handling in SND test
LabKey · Aug 6, 2024 · 67e94c6 · 67e94c6
1 parent a665aa6
commit 67e94c6
Show file tree

Hide file tree

Showing 3 changed files with 30 additions and 8 deletions.
diff --git a/src/org/labkey/snd/security/SNDSecurityManager.java b/src/org/labkey/snd/security/SNDSecurityManager.java
@@ -35,6 +35,8 @@
 import org.labkey.api.security.SecurityPolicy;
 import org.labkey.api.security.SecurityPolicyManager;
 import org.labkey.api.security.User;
+import org.labkey.api.security.impersonation.ImpersonationContext;
+import org.labkey.api.security.impersonation.RoleImpersonationContextFactory;
 import org.labkey.api.security.permissions.Permission;
 import org.labkey.api.security.roles.Role;
 import org.labkey.api.snd.Category;
@@ -206,7 +208,24 @@ public Map<String, Role> getAllSecurityRoles()
     private boolean hasPermission(User u, Category category, QCStateActionEnum action, QCStateEnum qcState)
     {
         Permission perm = action.getPermission(qcState);
-        return perm != null && category.hasPermission(u, perm.getClass());
+        if (perm == null)
+        {
+            return false;
+        }
+
+        Set<Role> roles = Set.of();
+
+        // SND has permissions bound to SND categories which can be assigned to packages (domains). Impersonating roles is used
+        // in automated and manual testing to verify this behavior. The behavior of role impersonation was changed in core
+        // labkey to only check for roles related to containers. This is a workaround to go back to checking all roles.
+        ImpersonationContext impersonationContext = u.getImpersonationContext();
+        if (impersonationContext instanceof RoleImpersonationContextFactory.RoleImpersonationContext context)
+        {
+            roles = context.getRoles().getRoles();
+        }
+
+        return SecurityManager.hasAllPermissions(this.getClass().getName() + ":" + category.getResourceName(),
+                category, u, Set.of(perm.getClass()), roles);
 
     }
 

diff --git a/webapp/snd/test/driver.js b/webapp/snd/test/driver.js
@@ -1113,6 +1113,9 @@
                                 else if (LABKEY.Utils.isObject(result)) {
                                     finish(undefined, result);
                                 }
+                                else if (json?.event?.exception?.message) {
+                                    finish('Test "' + test.name + '" failed. ' + json.event.exception.message);
+                                }
                                 else {
                                     finish('Test "' + test.name + '" failed. Test should specify a reason for failure or return true.');
                                 }

diff --git a/webapp/snd/test/tests.js b/webapp/snd/test/tests.js
@@ -348,7 +348,7 @@
                             return true;
                         }
 
-                        LABKEY.handleFailure(response, name + " - Stack Trace");
+                        LABKEY.handleSndFailure(response, name + " - Stack Trace");
                         return false;
                     }
                 }
@@ -563,7 +563,7 @@
                             return true;
                         }
 
-                        LABKEY.handleFailure(response, name + " - Stack Trace");
+                        LABKEY.handleSndFailure(response, name + " - Stack Trace");
                         return false;
                     }
                 }
@@ -586,7 +586,7 @@
                             return true;
                         }
 
-                        LABKEY.handleFailure(response, name + " - Stack Trace");
+                        LABKEY.handleSndFailure(response, name + " - Stack Trace");
                         return false;
                     }
                 }
@@ -630,7 +630,7 @@
                             return true;
                         }
 
-                        LABKEY.handleFailure(response, name + " - Stack Trace");
+                        LABKEY.handleSndFailure(response, name + " - Stack Trace");
                         return false;
                     }
                 }
@@ -655,7 +655,7 @@
                             return true;
                         }
 
-                        LABKEY.handleFailure(response, name + " - Stack Trace");
+                        LABKEY.handleSndFailure(response, name + " - Stack Trace");
                         return false;
                     }
                 }
@@ -713,7 +713,7 @@
                             return true;
                         }
 
-                        LABKEY.handleFailure(response, name + " - Stack Trace");
+                        LABKEY.handleSndFailure(response, name + " - Stack Trace");
                         return false;
                     }
                 }
@@ -782,7 +782,7 @@
                             return true;
                         }
 
-                        LABKEY.handleFailure(response, test + " - Stack Trace");
+                        LABKEY.handleSndFailure(response, test + " - Stack Trace");
                         return false;
                     }
                 };