From d4fff1dd912e01b9829ab031c5043df256a29d05 Mon Sep 17 00:00:00 2001 From: Guillaume Poirier-Morency Date: Wed, 28 Jan 2026 16:27:23 -0800 Subject: [PATCH] More improvements and fixes for the ACL linter Add a strategy for resolving parent ACL identity. The interface will be added to gsec, but we need it now for clarity. Fix Hibernate.getClass() called incorrectly on a class. Support for linting parent of assays and samples, looking up both datasets and subsets. In the latter case, the assay must have the source experiment as parent in the ACL structure. Improvements for ACL advice --- .../security/authorization/acl/AclAdvice.java | 82 ++++--------------- .../acl/AclLinterServiceImpl.java | 1 + .../acl/ParentIdentityRetrievalStrategy.java | 21 ----- .../ParentIdentityRetrievalStrategyImpl.java | 1 + .../gemma/applicationContext-security.xml | 13 ++- pom.xml | 2 +- 6 files changed, 30 insertions(+), 90 deletions(-) delete mode 100644 gemma-core/src/main/java/ubic/gemma/core/security/authorization/acl/ParentIdentityRetrievalStrategy.java diff --git a/gemma-core/src/main/java/ubic/gemma/core/security/authorization/acl/AclAdvice.java b/gemma-core/src/main/java/ubic/gemma/core/security/authorization/acl/AclAdvice.java index 87a7659425..910b7ed098 100644 --- a/gemma-core/src/main/java/ubic/gemma/core/security/authorization/acl/AclAdvice.java +++ b/gemma-core/src/main/java/ubic/gemma/core/security/authorization/acl/AclAdvice.java @@ -19,8 +19,9 @@ package ubic.gemma.core.security.authorization.acl; import gemma.gsec.acl.BaseAclAdvice; +import gemma.gsec.acl.ObjectTransientnessRetrievalStrategy; +import gemma.gsec.acl.ParentIdentityRetrievalStrategy; import gemma.gsec.acl.domain.AclService; -import gemma.gsec.model.GroupAuthority; import gemma.gsec.model.Securable; import gemma.gsec.model.User; import gemma.gsec.model.UserGroup; @@ -28,23 +29,16 @@ import org.apache.commons.logging.LogFactory; import org.hibernate.SessionFactory; import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.security.acls.model.*; -import org.springframework.security.core.GrantedAuthority; -import org.springframework.security.core.authority.SimpleGrantedAuthority; +import org.springframework.security.acls.model.ObjectIdentityRetrievalStrategy; import org.springframework.stereotype.Component; import ubic.gemma.model.analysis.Investigation; -import ubic.gemma.model.analysis.SingleExperimentAnalysis; import ubic.gemma.model.common.auditAndSecurity.AuditTrail; import ubic.gemma.model.common.auditAndSecurity.curation.CurationDetails; import ubic.gemma.model.expression.arrayDesign.ArrayDesign; import ubic.gemma.model.expression.bioAssay.BioAssay; -import ubic.gemma.model.expression.experiment.BioAssaySet; import ubic.gemma.model.expression.experiment.ExpressionExperiment; import ubic.gemma.persistence.util.Pointcuts; -import javax.annotation.Nullable; -import java.util.Collection; - /** * For permissions modification to be triggered, the method name must match certain patterns, which include "create", or * "remove". These patterns are defined in the {@link Pointcuts}. Other methods that would require @@ -58,14 +52,17 @@ public class AclAdvice extends BaseAclAdvice { private static final Log log = LogFactory.getLog( AclAdvice.class ); @Autowired - public AclAdvice( AclService aclService, SessionFactory sessionFactory, ObjectIdentityRetrievalStrategy objectIdentityRetrievalStrategy ) { - super( aclService, sessionFactory, objectIdentityRetrievalStrategy ); + public AclAdvice( AclService aclService, SessionFactory sessionFactory, + ObjectIdentityRetrievalStrategy objectIdentityRetrievalStrategy, + ParentIdentityRetrievalStrategy parentIdentityRetrievalStrategy, + ObjectTransientnessRetrievalStrategy objectTransientnessRetrievalStrategy ) { + super( aclService, sessionFactory, objectIdentityRetrievalStrategy, parentIdentityRetrievalStrategy, + objectTransientnessRetrievalStrategy ); } @Override protected boolean canSkipAclCheck( Object object ) { - return AuditTrail.class.isAssignableFrom( object.getClass() ) || CurationDetails.class - .isAssignableFrom( object.getClass() ); + return object instanceof AuditTrail || object instanceof CurationDetails; } @Override @@ -75,7 +72,7 @@ protected boolean canSkipAssociationCheck( Object object, String propertyName ) * If this is an expression experiment, don't go down the data vectors - it has no securable associations and * would be expensive to traverse. */ - if ( ExpressionExperiment.class.isAssignableFrom( object.getClass() ) + if ( object instanceof ExpressionExperiment && ( propertyName.equals( "rawExpressionDataVectors" ) || propertyName.equals( "processedExpressionDataVectors" ) || propertyName.equals( "singleCellExpressionDataVectors" ) ) ) { @@ -87,7 +84,7 @@ protected boolean canSkipAssociationCheck( Object object, String propertyName ) /* * Array design has some non (directly) securable associations that would be expensive to load */ - if ( ArrayDesign.class.isAssignableFrom( object.getClass() ) && propertyName.equals( "compositeSequences" ) ) { + if ( object instanceof ArrayDesign && propertyName.equals( "compositeSequences" ) ) { if ( AclAdvice.log.isTraceEnabled() ) AclAdvice.log.trace( "Skipping checking acl on probes on " + object ); return true; @@ -97,60 +94,13 @@ protected boolean canSkipAssociationCheck( Object object, String propertyName ) } @Override - protected void createOrUpdateAclSpecialCases( MutableAcl acl, @Nullable Acl parentAcl, Sid sid, Securable object ) { - - // Treating Analyses as special case. It'll inherit ACL from ExpressionExperiment - // If aclParent is passed to this method we overwrite it. - if ( SingleExperimentAnalysis.class.isAssignableFrom( object.getClass() ) ) { - SingleExperimentAnalysis experimentAnalysis = ( SingleExperimentAnalysis ) object; - - BioAssaySet bioAssaySet = experimentAnalysis.getExperimentAnalyzed(); - ObjectIdentity oi_temp = this.makeObjectIdentity( bioAssaySet ); - - parentAcl = this.getAclService().readAclById( oi_temp ); - if ( parentAcl == null ) { - // This is possible if making an EESubSet is part of the transaction. - parentAcl = this.getAclService().createAcl( oi_temp ); - } - acl.setEntriesInheriting( true ); - acl.setParent( parentAcl ); - //noinspection UnusedAssignment //Owner of the experiment owns analyses even if administrator ran them. - sid = parentAcl.getOwner(); - } - - } - - @Override - protected GrantedAuthority getUserGroupGrantedAuthority( Securable object ) { - Collection authorities = ( ( UserGroup ) object ).getAuthorities(); - assert authorities.size() == 1; - return new SimpleGrantedAuthority( authorities.iterator().next().getAuthority() ); - } - - @Override - protected String getUserName( Securable user ) { - return ( ( User ) user ).getUserName(); - } - - @Override - protected boolean objectIsUser( Securable object ) { - return User.class.isAssignableFrom( object.getClass() ); - } - - @Override - protected boolean objectIsUserGroup( Securable object ) { - return UserGroup.class.isAssignableFrom( object.getClass() ); - } - - @Override - protected boolean specialCaseForAssociationFollow( Object object, String property ) { - return BioAssay.class.isAssignableFrom( object.getClass() ) && ( property.equals( "sampleUsed" ) || property - .equals( "arrayDesignUsed" ) ); + protected boolean canFollowAssociation( Object object, String property ) { + return object instanceof BioAssay && ( property.equals( "sampleUsed" ) || property.equals( "arrayDesignUsed" ) ); } @Override - protected boolean specialCaseToKeepPrivateOnCreation( Securable object ) { - return super.specialCaseToKeepPrivateOnCreation( object ) + protected boolean isKeepPrivateOnCreation( Securable object ) { + return super.isKeepPrivateOnCreation( object ) || object instanceof UserGroup || object instanceof User || object instanceof Investigation; diff --git a/gemma-core/src/main/java/ubic/gemma/core/security/authorization/acl/AclLinterServiceImpl.java b/gemma-core/src/main/java/ubic/gemma/core/security/authorization/acl/AclLinterServiceImpl.java index fe157d6975..eedb70d090 100644 --- a/gemma-core/src/main/java/ubic/gemma/core/security/authorization/acl/AclLinterServiceImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/core/security/authorization/acl/AclLinterServiceImpl.java @@ -1,5 +1,6 @@ package ubic.gemma.core.security.authorization.acl; +import gemma.gsec.acl.ParentIdentityRetrievalStrategy; import gemma.gsec.acl.domain.AclGrantedAuthoritySid; import gemma.gsec.acl.domain.AclObjectIdentity; import gemma.gsec.acl.domain.AclService; diff --git a/gemma-core/src/main/java/ubic/gemma/core/security/authorization/acl/ParentIdentityRetrievalStrategy.java b/gemma-core/src/main/java/ubic/gemma/core/security/authorization/acl/ParentIdentityRetrievalStrategy.java deleted file mode 100644 index af21fce693..0000000000 --- a/gemma-core/src/main/java/ubic/gemma/core/security/authorization/acl/ParentIdentityRetrievalStrategy.java +++ /dev/null @@ -1,21 +0,0 @@ -package ubic.gemma.core.security.authorization.acl; - -import org.springframework.security.acls.model.ObjectIdentity; - -import javax.annotation.Nullable; - -/** - * Strategy for locating parent ACL identities. - * - * @author poirigui - */ -public interface ParentIdentityRetrievalStrategy { - - /** - * Obtain the parent ACL identity for the given ACL identity. - * - * @return the parent ACL identity if it can be determined, null otherwise - */ - @Nullable - ObjectIdentity getParentIdentity( ObjectIdentity aoi ); -} diff --git a/gemma-core/src/main/java/ubic/gemma/core/security/authorization/acl/ParentIdentityRetrievalStrategyImpl.java b/gemma-core/src/main/java/ubic/gemma/core/security/authorization/acl/ParentIdentityRetrievalStrategyImpl.java index 9b5b817f89..98cff604a8 100644 --- a/gemma-core/src/main/java/ubic/gemma/core/security/authorization/acl/ParentIdentityRetrievalStrategyImpl.java +++ b/gemma-core/src/main/java/ubic/gemma/core/security/authorization/acl/ParentIdentityRetrievalStrategyImpl.java @@ -1,5 +1,6 @@ package ubic.gemma.core.security.authorization.acl; +import gemma.gsec.acl.ParentIdentityRetrievalStrategy; import gemma.gsec.acl.domain.AclObjectIdentity; import lombok.extern.apachecommons.CommonsLog; import org.hibernate.Hibernate; diff --git a/gemma-core/src/main/resources/ubic/gemma/applicationContext-security.xml b/gemma-core/src/main/resources/ubic/gemma/applicationContext-security.xml index 35483f33c4..677108f449 100644 --- a/gemma-core/src/main/resources/ubic/gemma/applicationContext-security.xml +++ b/gemma-core/src/main/resources/ubic/gemma/applicationContext-security.xml @@ -81,8 +81,17 @@ - + + + diff --git a/pom.xml b/pom.xml index bf7e70f900..6411294100 100644 --- a/pom.xml +++ b/pom.xml @@ -875,7 +875,7 @@ - 0.0.22 + 0.0.23-SNAPSHOT 3.2.18.RELEASE 3.2.10.RELEASE 2.25.1