Task #8861 (closed)
Opened 12 years ago
Closed 12 years ago
Bug: group:-1 causes NPE on load
| Reported by: | jamoore | Owned by: | jamoore |
|---|---|---|---|
| Priority: | critical | Milestone: | OMERO-4.4 |
| Component: | Security | Version: | n.a. |
| Keywords: | testing,phase1 | Cc: | omero-team@… |
| Resources: | n.a. | Referenced By: | n.a. |
| References: | n.a. | Remaining Time: | 0.0d |
| Sprint: | 2012-06-19 (17) |
Description
Carlos was seeing permissions issues caused by the EMPTY permissions being used in the "-1" call context. This I fixed with the commit:
commit 71b2414e01ca4f3f4b0598c3fc3e2ecae884df85
Author: jmoore <josh@glencoesoftware.com>
Date: Tue May 15 14:24:15 2012 +0200
Load group for every logic in BasicACLVoter.allowLoad (Fix #8798)
Since the current group is ID=-1 when in the AllGroupsSecurityFilter,
the current group permissions are EMPTY. Therefore, we now pass the
session into all ACLVoter implementations in order to allow loading
the requested object's group.
Then he started seeing NPEs (see below). I've provided Carlos with a workaround on his branch, but if anyone else starts seeing NPEs, the patch (also below) should be applied to the mainline.
Change History (7)
comment:1 Changed 12 years ago by jmoore
comment:2 Changed 12 years ago by jmoore
Workaround:
commit f5576b6e5e0c087d2489a6f64c4090a4f7552264
Author: jmoore <josh@glencoesoftware.com>
Date: Thu May 17 18:25:40 2012 +0200
Force loading of group permissions when null!
This is a nasty workaround. For every object, it's
hitting the database to find out the permissions. If
we can't figure out why these permissions are null in
the first place, then the better solution will be to
introduce a GrouPermissionsCache which keeps all of this
nice and tidy.
diff --git a/components/server/resources/ome/services/sec-primitives.xml b/components/server/resources/ome/services/sec-primitives.xml
index 4dffef0..8b5bd6f 100644
--- a/components/server/resources/ome/services/sec-primitives.xml
+++ b/components/server/resources/ome/services/sec-primitives.xml
@@ -53,6 +53,7 @@
</bean>
<bean id="AllGroupsSecurityFilter" class="ome.security.basic.AllGroupsSecurityFilter">
+ <constructor-arg ref="simpleSqlAction"/>
<constructor-arg ref="roles"/>
</bean>
diff --git a/components/server/src/ome/security/basic/AbstractSecurityFilter.java b/components/server/src/ome/security/basic/AbstractSecurityFilter.java
index f4cdc2b..5b202f6 100644
--- a/components/server/src/ome/security/basic/AbstractSecurityFilter.java
+++ b/components/server/src/ome/security/basic/AbstractSecurityFilter.java
@@ -8,18 +8,8 @@
package ome.security.basic;
import java.util.Collection;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
import java.util.Properties;
-import org.hibernate.Filter;
-import org.hibernate.Session;
-import org.springframework.beans.factory.FactoryBean;
-import org.springframework.orm.hibernate3.FilterDefinitionFactoryBean;
-
-import ome.conditions.InternalException;
import ome.model.internal.Details;
import ome.model.internal.Permissions;
import ome.model.internal.Permissions.Right;
@@ -28,6 +18,12 @@ import ome.security.SecurityFilter;
import ome.system.EventContext;
import ome.system.Roles;
+import org.apache.commons.logging.Log;
+import org.apache.commons.logging.LogFactory;
+import org.hibernate.Session;
+import org.springframework.beans.factory.FactoryBean;
+import org.springframework.orm.hibernate3.FilterDefinitionFactoryBean;
+
/**
* overrides {@link FilterDefinitionFactoryBean} in order to construct our
* security filter in code and not in XML. This allows us to make use of the
@@ -42,7 +38,9 @@ import ome.system.Roles;
public abstract class AbstractSecurityFilter extends FilterDefinitionFactoryBean
implements SecurityFilter {
- protected final Roles roles;
+ protected final Log log = LogFactory.getLog(getClass());
+
+ protected final Roles roles;
/**
* default constructor which calls all the necessary setters for this
diff --git a/components/server/src/ome/security/basic/AllGroupsSecurityFilter.java b/components/server/src/ome/security/basic/AllGroupsSecurityFilter.java
index 2374756..8749370 100644
--- a/components/server/src/ome/security/basic/AllGroupsSecurityFilter.java
+++ b/components/server/src/ome/security/basic/AllGroupsSecurityFilter.java
@@ -7,6 +7,11 @@
package ome.security.basic;
+import static ome.model.internal.Permissions.Right.READ;
+import static ome.model.internal.Permissions.Role.GROUP;
+import static ome.model.internal.Permissions.Role.USER;
+import static ome.model.internal.Permissions.Role.WORLD;
+
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
@@ -14,12 +19,6 @@ import java.util.List;
import java.util.Map;
import java.util.Properties;
-import org.hibernate.Filter;
-import org.hibernate.Session;
-import org.springframework.beans.factory.FactoryBean;
-import org.springframework.orm.hibernate3.FilterDefinitionFactoryBean;
-
-import ome.conditions.InternalException;
import ome.model.internal.Details;
import ome.model.internal.Permissions;
import ome.model.internal.Permissions.Right;
@@ -28,9 +27,13 @@ import ome.model.meta.ExperimenterGroup;
import ome.security.SecurityFilter;
import ome.system.EventContext;
import ome.system.Roles;
+import ome.util.SqlAction;
+
+import org.hibernate.Filter;
+import org.hibernate.Session;
-import static ome.model.internal.Permissions.Right.*;
-import static ome.model.internal.Permissions.Role.*;
+import org.springframework.beans.factory.FactoryBean;
+import org.springframework.orm.hibernate3.FilterDefinitionFactoryBean;
/**
@@ -70,7 +73,9 @@ public class AllGroupsSecurityFilter extends AbstractSecurityFilter {
+ "\n)"
+ "\n", isGranted(USER, READ), isGranted(GROUP, READ),
isGranted(WORLD, READ));
-
+
+ final SqlAction sql;
+
/**
* default constructor which calls all the necessary setters for this
* {@link FactoryBean}. Also constructs the {@link #defaultFilterCondition }
@@ -82,12 +87,13 @@ public class AllGroupsSecurityFilter extends AbstractSecurityFilter {
* @see FilterDefinitionFactoryBean#setParameterTypes(Properties)
* @see FilterDefinitionFactoryBean#setDefaultFilterCondition(String)
*/
- public AllGroupsSecurityFilter() {
- this(new Roles());
+ public AllGroupsSecurityFilter(SqlAction sql) {
+ this(sql, new Roles());
}
- public AllGroupsSecurityFilter(Roles roles) {
+ public AllGroupsSecurityFilter(SqlAction sql, Roles roles) {
super(roles);
+ this.sql = sql;
}
public String getDefaultCondition() {
@@ -130,8 +136,16 @@ public class AllGroupsSecurityFilter extends AbstractSecurityFilter {
// ticket:8798 - load permissions for group of object regardless.
final ExperimenterGroup group = (ExperimenterGroup) session.get(ExperimenterGroup.class, g);
- org.hibernate.Hibernate.initialize(group);
- final Permissions p = group.getDetails().getPermissions();
+ Permissions p = group.getDetails().getPermissions();
+
+ if (p == null) {
+ // Don't know why this is happening, but must do something to
+ // force reloading.
+ p = ome.util.Utils.toPermissions(sql.getGroupPermissions(g));
+ group.getDetails().setPermissions(p);
+ log.warn(String.format(
+ "Forced to reload permissions for group %s: %s", g, p));
+ }
if (share || admin) {
return true;
diff --git a/components/server/src/ome/security/basic/BasicSecuritySystem.java b/components/server/src/ome/security/basic/BasicSecuritySystem.java
index b1203ca..bc9b32b 100644
--- a/components/server/src/ome/security/basic/BasicSecuritySystem.java
+++ b/components/server/src/ome/security/basic/BasicSecuritySystem.java
@@ -121,7 +121,7 @@ public class BasicSecuritySystem implements SecuritySystem,
Roles roles = new Roles();
SecurityFilterHolder holder = new SecurityFilterHolder(
cd, new OneGroupSecurityFilter(roles),
- new AllGroupsSecurityFilter(roles));
+ new AllGroupsSecurityFilter(null, roles));
BasicSecuritySystem sec = new BasicSecuritySystem(oi, st, cd, sm,
roles, sf, new TokenHolder(), holder);
return sec;
commit bbe5408ee11a353f2d92697a6dc4886fc5d9b7c8
Author: Carlos Neves <carlos@glencoesoftware.com>
Date: Thu May 17 12:40:56 2012 +0100
josh's Possible fix for NPE in AllGroupsSecurityFilter (See #8798)
diff --git a/components/server/src/ome/security/basic/AllGroupsSecurityFilter.java b/components/server/src/ome/security/basic/AllGroupsSecurityFilter.java
index ce2e741..2374756 100644
--- a/components/server/src/ome/security/basic/AllGroupsSecurityFilter.java
+++ b/components/server/src/ome/security/basic/AllGroupsSecurityFilter.java
@@ -129,8 +129,9 @@ public class AllGroupsSecurityFilter extends AbstractSecurityFilter {
final Long g = d.getGroup().getId();
// ticket:8798 - load permissions for group of object regardless.
- final Permissions p = ((ExperimenterGroup) session.get(ExperimenterGroup.class, g))
- .getDetails().getPermissions();
+ final ExperimenterGroup group = (ExperimenterGroup) session.get(ExperimenterGroup.class, g);
+ org.hibernate.Hibernate.initialize(group);
+ final Permissions p = group.getDetails().getPermissions();
if (share || admin) {
return true;
comment:3 Changed 12 years ago by jmoore
Applied to sprint16-bugfixes:
commit f93f93e3ee9ae0a599b07d932e02c02e1bad74df
Author: Carlos Neves <carlos@glencoesoftware.com>
Date: Thu May 17 12:40:56 2012 +0100
Force loading of group permissions when null! (See #8798, #8861)
This is a nasty workaround. For every object, it's
hitting the database to find out the permissions. If
we can't figure out why these permissions are null in
the first place, then the better solution will be to
introduce a GrouPermissionsCache which keeps all of this
nice and tidy.
comment:4 Changed 12 years ago by atarkowska
- Keywords testing phase1 added
comment:5 Changed 12 years ago by jmoore <josh@…>
(In [f93f93e3ee9ae0a599b07d932e02c02e1bad74df/ome.git] on branch develop) Force loading of group permissions when null! (See #8798, #8861)
This is a nasty workaround. For every object, it's
hitting the database to find out the permissions. If
we can't figure out why these permissions are null in
the first place, then the better solution will be to
introduce a GrouPermissionsCache? which keeps all of this
nice and tidy.
comment:6 Changed 12 years ago by jburel
- Sprint changed from 2012-06-05 (16) to 2012-06-19 (17)
Moved from sprint 2012-06-05 (16)
comment:7 Changed 12 years ago by jmoore
- Remaining Time changed from 0.5 to 0
- Resolution set to fixed
- Status changed from new to closed
Closing since have #8991 for a review.
Note: See
TracTickets for help on using
tickets.
You may also have a look at Agilo extensions to the ticket.
NPE:
====================================================================== ERROR: testUsers (__main__.UserTest) ---------------------------------------------------------------------- Traceback (most recent call last): File "/tmp/_chmod/components/tools/OmeroPy/build/lib/omero/gateway/scripts/testdb_create.py", line 61, in setUp self.prepTestDB() File "/tmp/_chmod/components/tools/OmeroPy/build/lib/omero/gateway/scripts/testdb_create.py", line 144, in prepTestDB dbhelpers.bootstrap() File "/tmp/_chmod/components/tools/OmeroPy/build/lib/omero/gateway/scripts/dbhelpers.py", line 467, in bootstrap p = p.create() File "/tmp/_chmod/components/tools/OmeroPy/build/lib/omero/gateway/scripts/dbhelpers.py", line 264, in create p = self.get(client) File "/tmp/_chmod/components/tools/OmeroPy/build/lib/omero/gateway/scripts/dbhelpers.py", line 255, in get for p in client.listProjects(): File "/tmp/_chmod/components/tools/OmeroPy/build/lib/omero/gateway/__init__.py", line 2500, in getObjects result = self.getQueryService().findAllByQuery(query, params, self.CONFIG['SERVICE_OPTS']) File "/tmp/_chmod/components/tools/OmeroPy/build/lib/omero/gateway/__init__.py", line 3249, in __call__ return self.handle_exception(e, *args, **kwargs) File "/tmp/_chmod/components/tools/OmeroPy/build/lib/omero/gateway/__init__.py", line 3246, in __call__ return self.f(*args, **kwargs) File "/tmp/_chmod/components/tools/OmeroPy/build/lib/omero_api_IQuery_ice.py", line 139, in findAllByQuery return _M_omero.api.IQuery._op_findAllByQuery.invoke(self, ((query, params), _ctx)) InternalException: exception ::omero::InternalException { serverStackTrace = ome.conditions.InternalException: Wrapped Exception: (java.lang.NullPointerException): null at ome.security.basic.AllGroupsSecurityFilter.passesFilter(AllGroupsSecurityFilter.java:141) at ome.security.SecurityFilterHolder.passesFilter(SecurityFilterHolder.java:83) at ome.security.basic.BasicACLVoter.allowLoad(BasicACLVoter.java:103) at ome.security.CompositeACLVoter.allowLoad(CompositeACLVoter.java:72) at ome.security.ACLEventListener.onPostLoad(ACLEventListener.java:103) at org.hibernate.engine.TwoPhaseLoad.initializeEntity(TwoPhaseLoad.java:250) at org.hibernate.loader.Loader.initializeEntitiesAndCollections(Loader.java:898) at org.hibernate.loader.Loader.doQuery(Loader.java:773) at org.hibernate.loader.Loader.doQueryAndInitializeNonLazyCollections(Loader.java:270) at org.hibernate.loader.Loader.doList(Loader.java:2449) at org.hibernate.loader.Loader.listIgnoreQueryCache(Loader.java:2192) at org.hibernate.loader.Loader.list(Loader.java:2187) at org.hibernate.loader.hql.QueryLoader.list(QueryLoader.java:452) at org.hibernate.hql.ast.QueryTranslatorImpl.list(QueryTranslatorImpl.java:363) at org.hibernate.engine.query.HQLQueryPlan.performList(HQLQueryPlan.java:196) at org.hibernate.impl.SessionImpl.list(SessionImpl.java:1260) at org.hibernate.impl.QueryImpl.list(QueryImpl.java:102) at ome.services.query.Query.doInHibernate(Query.java:244) at org.springframework.orm.hibernate3.HibernateTemplate.doExecute(HibernateTemplate.java:406) at org.springframework.orm.hibernate3.HibernateTemplate.execute(HibernateTemplate.java:339) at ome.logic.QueryImpl.execute(QueryImpl.java:144) at ome.logic.QueryImpl.findAllByQuery(QueryImpl.java:394) at sun.reflect.GeneratedMethodAccessor345.invoke(Unknown Source) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at org.springframework.aop.support.AopUtils.invokeJoinpointUsingReflection(AopUtils.java:307) at org.springframework.aop.framework.ReflectiveMethodInvocation.invokeJoinpoint(ReflectiveMethodInvocation.java:183) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:150) at ome.security.basic.EventHandler.invoke(EventHandler.java:165) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172) at org.springframework.orm.hibernate3.HibernateInterceptor.invoke(HibernateInterceptor.java:111) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172) at org.springframework.transaction.interceptor.TransactionInterceptor.invoke(TransactionInterceptor.java:108) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172) at ome.tools.hibernate.ProxyCleanupFilter$Interceptor.invoke(ProxyCleanupFilter.java:236) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172) at ome.services.util.ServiceHandler.invoke(ServiceHandler.java:116) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172) at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:202) at $Proxy75.findAllByQuery(Unknown Source) at sun.reflect.GeneratedMethodAccessor345.invoke(Unknown Source) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at org.springframework.aop.support.AopUtils.invokeJoinpointUsingReflection(AopUtils.java:307) at org.springframework.aop.framework.ReflectiveMethodInvocation.invokeJoinpoint(ReflectiveMethodInvocation.java:183) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:150) at ome.security.basic.BasicSecurityWiring.invoke(BasicSecurityWiring.java:98) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172) at ome.services.blitz.fire.AopContextInitializer.invoke(AopContextInitializer.java:43) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172) at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:202) at $Proxy75.findAllByQuery(Unknown Source) at sun.reflect.GeneratedMethodAccessor468.invoke(Unknown Source) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at ome.services.blitz.util.IceMethodInvoker.invoke(IceMethodInvoker.java:179) at ome.services.throttling.Callback.run(Callback.java:56) at ome.services.throttling.InThreadThrottlingStrategy.callInvokerOnRawArgs(InThreadThrottlingStrategy.java:56) at ome.services.blitz.impl.AbstractAmdServant.callInvokerOnRawArgs(AbstractAmdServant.java:137) at ome.services.blitz.impl.QueryI.findAllByQuery_async(QueryI.java:66) at sun.reflect.GeneratedMethodAccessor467.invoke(Unknown Source) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:597) at org.springframework.aop.support.AopUtils.invokeJoinpointUsingReflection(AopUtils.java:307) at org.springframework.aop.framework.ReflectiveMethodInvocation.invokeJoinpoint(ReflectiveMethodInvocation.java:183) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:150) at omero.cmd.CallContext.invoke(CallContext.java:59) at org.springframework.aop.framework.ReflectiveMethodInvocation.proceed(ReflectiveMethodInvocation.java:172) at org.springframework.aop.framework.JdkDynamicAopProxy.invoke(JdkDynamicAopProxy.java:202) at $Proxy76.findAllByQuery_async(Unknown Source) at omero.api._IQueryTie.findAllByQuery_async(_IQueryTie.java:92) at omero.api._IQueryDisp.___findAllByQuery(_IQueryDisp.java:366) at omero.api._IQueryDisp.__dispatch(_IQueryDisp.java:496) at IceInternal.Incoming.invoke(Incoming.java:159) at Ice.ConnectionI.invokeAll(ConnectionI.java:2037) at Ice.ConnectionI.message(ConnectionI.java:972) at IceInternal.ThreadPool.run(ThreadPool.java:577) at IceInternal.ThreadPool.access$100(ThreadPool.java:12) at IceInternal.ThreadPool$EventHandlerThread.run(ThreadPool.java:971) serverExceptionClass = ome.conditions.InternalException message = Wrapped Exception: (java.lang.NullPointerException): null } ---------------------------------------------------------------------- Ran 5 tests in 218.435s FAILED (errors=4)