Skip to content

Commit 35ba24d

Browse files
arteymixarteymix
committed
Improvements for ACL advice
1 parent 3258c11 commit 35ba24d

10 files changed

Lines changed: 258 additions & 162 deletions

File tree

‎gemma-core/src/main/java/ubic/gemma/core/security/authorization/acl/AclAdvice.java‎

Lines changed: 16 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -19,32 +19,26 @@
1919
package ubic.gemma.core.security.authorization.acl;
2020

2121
import gemma.gsec.acl.BaseAclAdvice;
22+
import gemma.gsec.acl.ObjectTransientnessRetrievalStrategy;
23+
import gemma.gsec.acl.ParentIdentityRetrievalStrategy;
2224
import gemma.gsec.acl.domain.AclService;
23-
import gemma.gsec.model.GroupAuthority;
2425
import gemma.gsec.model.Securable;
2526
import gemma.gsec.model.User;
2627
import gemma.gsec.model.UserGroup;
2728
import org.apache.commons.logging.Log;
2829
import org.apache.commons.logging.LogFactory;
2930
import org.hibernate.SessionFactory;
3031
import org.springframework.beans.factory.annotation.Autowired;
31-
import org.springframework.security.acls.model.*;
32-
import org.springframework.security.core.GrantedAuthority;
33-
import org.springframework.security.core.authority.SimpleGrantedAuthority;
32+
import org.springframework.security.acls.model.ObjectIdentityRetrievalStrategy;
3433
import org.springframework.stereotype.Component;
3534
import ubic.gemma.model.analysis.Investigation;
36-
import ubic.gemma.model.analysis.SingleExperimentAnalysis;
3735
import ubic.gemma.model.common.auditAndSecurity.AuditTrail;
3836
import ubic.gemma.model.common.auditAndSecurity.curation.CurationDetails;
3937
import ubic.gemma.model.expression.arrayDesign.ArrayDesign;
4038
import ubic.gemma.model.expression.bioAssay.BioAssay;
41-
import ubic.gemma.model.expression.experiment.BioAssaySet;
4239
import ubic.gemma.model.expression.experiment.ExpressionExperiment;
4340
import ubic.gemma.persistence.util.Pointcuts;
4441

45-
import javax.annotation.Nullable;
46-
import java.util.Collection;
47-
4842
/**
4943
* For permissions modification to be triggered, the method name must match certain patterns, which include "create", or
5044
* "remove". These patterns are defined in the {@link Pointcuts}. Other methods that would require
@@ -58,14 +52,17 @@ public class AclAdvice extends BaseAclAdvice {
5852
private static final Log log = LogFactory.getLog( AclAdvice.class );
5953

6054
@Autowired
61-
public AclAdvice( AclService aclService, SessionFactory sessionFactory, ObjectIdentityRetrievalStrategy objectIdentityRetrievalStrategy ) {
62-
super( aclService, sessionFactory, objectIdentityRetrievalStrategy );
55+
public AclAdvice( AclService aclService, SessionFactory sessionFactory,
56+
ObjectIdentityRetrievalStrategy objectIdentityRetrievalStrategy,
57+
ParentIdentityRetrievalStrategy parentIdentityRetrievalStrategy,
58+
ObjectTransientnessRetrievalStrategy objectTransientnessRetrievalStrategy ) {
59+
super( aclService, sessionFactory, objectIdentityRetrievalStrategy, parentIdentityRetrievalStrategy,
60+
objectTransientnessRetrievalStrategy );
6361
}
6462

6563
@Override
6664
protected boolean canSkipAclCheck( Object object ) {
67-
return AuditTrail.class.isAssignableFrom( object.getClass() ) || CurationDetails.class
68-
.isAssignableFrom( object.getClass() );
65+
return object instanceof AuditTrail || object instanceof CurationDetails;
6966
}
7067

7168
@Override
@@ -75,7 +72,7 @@ protected boolean canSkipAssociationCheck( Object object, String propertyName )
7572
* If this is an expression experiment, don't go down the data vectors - it has no securable associations and
7673
* would be expensive to traverse.
7774
*/
78-
if ( ExpressionExperiment.class.isAssignableFrom( object.getClass() )
75+
if ( object instanceof ExpressionExperiment
7976
&& ( propertyName.equals( "rawExpressionDataVectors" )
8077
|| propertyName.equals( "processedExpressionDataVectors" )
8178
|| propertyName.equals( "singleCellExpressionDataVectors" ) ) ) {
@@ -87,7 +84,7 @@ protected boolean canSkipAssociationCheck( Object object, String propertyName )
8784
/*
8885
* Array design has some non (directly) securable associations that would be expensive to load
8986
*/
90-
if ( ArrayDesign.class.isAssignableFrom( object.getClass() ) && propertyName.equals( "compositeSequences" ) ) {
87+
if ( object instanceof ArrayDesign && propertyName.equals( "compositeSequences" ) ) {
9188
if ( AclAdvice.log.isTraceEnabled() )
9289
AclAdvice.log.trace( "Skipping checking acl on probes on " + object );
9390
return true;
@@ -97,60 +94,13 @@ protected boolean canSkipAssociationCheck( Object object, String propertyName )
9794
}
9895

9996
@Override
100-
protected void createOrUpdateAclSpecialCases( MutableAcl acl, @Nullable Acl parentAcl, Sid sid, Securable object ) {
101-
102-
// Treating Analyses as special case. It'll inherit ACL from ExpressionExperiment
103-
// If aclParent is passed to this method we overwrite it.
104-
if ( SingleExperimentAnalysis.class.isAssignableFrom( object.getClass() ) ) {
105-
SingleExperimentAnalysis<?> experimentAnalysis = ( SingleExperimentAnalysis<?> ) object;
106-
107-
BioAssaySet bioAssaySet = experimentAnalysis.getExperimentAnalyzed();
108-
ObjectIdentity oi_temp = this.makeObjectIdentity( bioAssaySet );
109-
110-
parentAcl = this.getAclService().readAclById( oi_temp );
111-
if ( parentAcl == null ) {
112-
// This is possible if making an EESubSet is part of the transaction.
113-
parentAcl = this.getAclService().createAcl( oi_temp );
114-
}
115-
acl.setEntriesInheriting( true );
116-
acl.setParent( parentAcl );
117-
//noinspection UnusedAssignment //Owner of the experiment owns analyses even if administrator ran them.
118-
sid = parentAcl.getOwner();
119-
}
120-
121-
}
122-
123-
@Override
124-
protected GrantedAuthority getUserGroupGrantedAuthority( Securable object ) {
125-
Collection<? extends GroupAuthority> authorities = ( ( UserGroup ) object ).getAuthorities();
126-
assert authorities.size() == 1;
127-
return new SimpleGrantedAuthority( authorities.iterator().next().getAuthority() );
128-
}
129-
130-
@Override
131-
protected String getUserName( Securable user ) {
132-
return ( ( User ) user ).getUserName();
133-
}
134-
135-
@Override
136-
protected boolean objectIsUser( Securable object ) {
137-
return User.class.isAssignableFrom( object.getClass() );
138-
}
139-
140-
@Override
141-
protected boolean objectIsUserGroup( Securable object ) {
142-
return UserGroup.class.isAssignableFrom( object.getClass() );
143-
}
144-
145-
@Override
146-
protected boolean specialCaseForAssociationFollow( Object object, String property ) {
147-
return BioAssay.class.isAssignableFrom( object.getClass() ) && ( property.equals( "sampleUsed" ) || property
148-
.equals( "arrayDesignUsed" ) );
97+
protected boolean canFollowAssociation( Object object, String property ) {
98+
return object instanceof BioAssay && ( property.equals( "sampleUsed" ) || property.equals( "arrayDesignUsed" ) );
14999
}
150100

151101
@Override
152-
protected boolean specialCaseToKeepPrivateOnCreation( Securable object ) {
153-
return super.specialCaseToKeepPrivateOnCreation( object )
102+
protected boolean isKeepPrivateOnCreation( Securable object ) {
103+
return super.isKeepPrivateOnCreation( object )
154104
|| object instanceof UserGroup
155105
|| object instanceof User
156106
|| object instanceof Investigation;

‎gemma-core/src/main/java/ubic/gemma/core/security/authorization/acl/AclLinterServiceImpl.java‎

Lines changed: 17 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,10 @@
11
package ubic.gemma.core.security.authorization.acl;
22

3+
import gemma.gsec.acl.ParentIdentityRetrievalStrategy;
34
import gemma.gsec.acl.domain.AclGrantedAuthoritySid;
45
import gemma.gsec.acl.domain.AclObjectIdentity;
56
import gemma.gsec.acl.domain.AclService;
67
import lombok.extern.apachecommons.CommonsLog;
7-
import org.hibernate.Hibernate;
88
import org.hibernate.Query;
99
import org.hibernate.SessionFactory;
1010
import org.hibernate.dialect.function.SQLFunction;
@@ -30,7 +30,6 @@
3030
import ubic.gemma.model.expression.bioAssayData.MeanVarianceRelation;
3131
import ubic.gemma.model.expression.biomaterial.BioMaterial;
3232
import ubic.gemma.model.expression.experiment.*;
33-
import ubic.gemma.persistence.service.expression.experiment.ExpressionExperimentService;
3433

3534
import javax.annotation.Nullable;
3635
import java.lang.reflect.Modifier;
@@ -66,8 +65,13 @@ private static void addSecuredChildToParent( Class<? extends SecuredChild<?>> cl
6665
static {
6766
// FIXME: handle sub-assays and sub-biomaterials in the child to parent query, or recursively resolve parents as
6867
// a special case
69-
addSecuredChildToParent( BioAssay.class, ExpressionExperiment.class, null );
70-
addSecuredChildToParent( BioMaterial.class, ExpressionExperiment.class, null );
68+
// this cover cases where the BioAssay is attached to a EE or a subset of an EE
69+
addSecuredChildToParent( BioAssay.class, ExpressionExperiment.class,
70+
//language=HQL
71+
"select coalesce(ee.id, eess.sourceExperiment.id) from ExpressionExperiment ee join ee.bioAssays ba, ExpressionExperimentSubSet eess join eess.bioAssays eessba where ba.id = aoi.identifier or eessba.id = aoi.identifier" );
72+
addSecuredChildToParent( BioMaterial.class, ExpressionExperiment.class,
73+
//language=HQL
74+
"select coalesce(ee.id, eess.sourceExperiment.id) from ExpressionExperiment ee join ee.bioAssays ba join ba.sampleUsed bm, ExpressionExperimentSubSet eess join eess.bioAssays eessba join eessba.sampleUsed eessbm where bm.id = aoi.identifier or eessbm.id = aoi.identifier" );
7175
addSecuredChildToParent( ExpressionExperimentSubSet.class, ExpressionExperiment.class,
7276
//language=HQL
7377
"select ears.analysis.id from ExpressionAnalysisResultSet ears where ears.id = aoi.identifier" );
@@ -108,7 +112,7 @@ private static void addSecuredChildToParent( Class<? extends SecuredChild<?>> cl
108112
@Autowired
109113
private SessionFactory sessionFactory;
110114
@Autowired
111-
private ExpressionExperimentService expressionExperimentService;
115+
private ParentIdentityRetrievalStrategy parentIdentityRetrievalStrategy;
112116

113117
@Override
114118
@Transactional
@@ -205,7 +209,6 @@ private void lintAclObjectIdentityLackingSecurable( Class<? extends Securable> c
205209
}
206210
for ( AclObjectIdentity aoi : list ) {
207211
if ( config.isApplyFixes() ) {
208-
// not removing child, but we will visit it later if it's also dangling
209212
aclService.deleteAcl( aoi, true );
210213
log.info( "Deleted dangling " + aoi + "." );
211214
results.add( new LintResult( clazz, aoi.getIdentifier(), String.format( "%s has no corresponding %s entity with ID %d.", aoi, clazz.getName(), aoi.getIdentifier() ), true ) );
@@ -225,7 +228,7 @@ private void lintSecurableLackingObjectIdentity( Class<? extends Securable> claz
225228
//noinspection unchecked
226229
List<? extends Securable> list = sessionFactory.getCurrentSession()
227230
.createQuery( "select e from " + clazz.getName() + " e "
228-
+ "where e.id not in ( " + "select aoi.identifier from AclObjectIdentity aoi where aoi.type = :type" + ")" )
231+
+ "where e.id not in (select aoi.identifier from AclObjectIdentity aoi where aoi.type = :type)" )
229232
.setParameter( "type", clazz.getName() )
230233
.setReadOnly( !config.isApplyFixes() )
231234
.list();
@@ -248,7 +251,7 @@ private void lintSecurableLackingObjectIdentity( Class<? extends Securable> claz
248251
private void lintSecurableLackingObjectIdentity( Class<? extends Securable> clazz, Long identifier, AclLinterConfig config, Collection<LintResult> results ) {
249252
Securable s = ( Securable ) sessionFactory.getCurrentSession()
250253
.createQuery( "select e from " + clazz.getName() + " e "
251-
+ "where e.id = :identifier and e.id not in ( " + "select aoi.identifier from AclObjectIdentity aoi where aoi.type = :type" + ")" )
254+
+ "where e.id = :identifier and e.id not in (select aoi.identifier from AclObjectIdentity aoi where aoi.type = :type and aoi.identifier = :identifier)" )
252255
.setParameter( "identifier", identifier )
253256
.setParameter( "type", clazz.getName() )
254257
.setReadOnly( !config.isApplyFixes() )
@@ -286,7 +289,7 @@ private void lintSecuredChildWithoutParent( Class<? extends SecuredChild<?>> cla
286289
}
287290
for ( AclObjectIdentity aoi : list ) {
288291
if ( config.isApplyFixes() ) {
289-
AclObjectIdentity parentAoi = getParentAclObjectIdentity( clazz, aoi );
292+
AclObjectIdentity parentAoi = ( AclObjectIdentity ) parentIdentityRetrievalStrategy.locateParentIdentity( aoi );
290293
String p = aoi.getType() + " Id=" + aoi.getIdentifier() + " has no parent ACL identity, it should be " + parentAoi + ".";
291294
if ( parentAoi != null ) {
292295
aoi.setParentObject( parentAoi );
@@ -311,12 +314,12 @@ private void lintSecuredChildWithoutParent( Class<? extends SecuredChild<?>> cla
311314
.uniqueResult();
312315
String s = clazz.getSimpleName() + " Id=" + identifier;
313316
if ( aoi == null ) {
314-
log.info( s + " has a parent ACL identity." );
317+
log.warn( s + " does not have an ACL identity." );
315318
return;
316319
}
317320
String p = s + " has no parent ACL identity.";
318321
if ( config.isApplyFixes() ) {
319-
AclObjectIdentity parentAoi = getParentAclObjectIdentity( clazz, aoi );
322+
AclObjectIdentity parentAoi = ( AclObjectIdentity ) parentIdentityRetrievalStrategy.locateParentIdentity( aoi );
320323
if ( parentAoi != null ) {
321324
aoi.setParentObject( parentAoi );
322325
results.add( new LintResult( clazz, aoi.getIdentifier(), p, true ) );
@@ -351,7 +354,7 @@ private void lintSecuredChildWithIncorrectParent( Class<? extends SecuredChild<?
351354
String p = String.format( "%s Id=%d does not have a parent ACL identity of type %s: %s.", aoi.getType(),
352355
aoi.getIdentifier(), expectedParentClass.getSimpleName(), currentParentAoi );
353356
if ( config.isApplyFixes() ) {
354-
AclObjectIdentity parentAoi = getParentAclObjectIdentity( clazz, aoi );
357+
AclObjectIdentity parentAoi = ( AclObjectIdentity ) parentIdentityRetrievalStrategy.locateParentIdentity( aoi );
355358
if ( parentAoi != null ) {
356359
aoi.setParentObject( parentAoi );
357360
results.add( new LintResult( clazz, aoi.getIdentifier(), p, true ) );
@@ -380,15 +383,15 @@ private void lintSecuredChildWithIncorrectParent( Class<? extends SecuredChild<?
380383
.uniqueResult();
381384
String s = clazz.getSimpleName() + " Id=" + identifier;
382385
if ( row == null ) {
383-
log.info( s + " has no parent ACL identity identity." );
386+
log.info( s + " has no parent ACL identity." );
384387
return;
385388
}
386389
AclObjectIdentity aoi = ( AclObjectIdentity ) row[0];
387390
AclObjectIdentity currentParentAoi = ( AclObjectIdentity ) row[1];
388391
String problem = String.format( "%s does not have a parent ACL identity of type %s: %s.", s,
389392
expectedParentClass.getSimpleName(), currentParentAoi );
390393
if ( config.isApplyFixes() ) {
391-
AclObjectIdentity parentAoi = getParentAclObjectIdentity( clazz, aoi );
394+
AclObjectIdentity parentAoi = ( AclObjectIdentity ) parentIdentityRetrievalStrategy.locateParentIdentity( aoi );
392395
if ( parentAoi != null ) {
393396
aoi.setParentObject( parentAoi );
394397
results.add( new LintResult( clazz, aoi.getId(), problem, true ) );
@@ -400,82 +403,6 @@ private void lintSecuredChildWithIncorrectParent( Class<? extends SecuredChild<?
400403
}
401404
}
402405

403-
@Nullable
404-
private AclObjectIdentity getParentAclObjectIdentity( Class<? extends SecuredChild<?>> clazz, AclObjectIdentity aoi ) {
405-
Class<? extends Securable> parentType;
406-
Long parentIdentifier;
407-
if ( ExperimentalFactor.class.isAssignableFrom( clazz ) ) {
408-
parentType = ExpressionExperiment.class;
409-
ExperimentalFactor factor = ( ExperimentalFactor ) sessionFactory.getCurrentSession()
410-
.get( ExperimentalFactor.class, aoi.getIdentifier() );
411-
ExpressionExperiment ee = expressionExperimentService.findByFactor( factor );
412-
parentIdentifier = ee != null ? ee.getId() : null;
413-
} else if ( FactorValue.class.isAssignableFrom( clazz ) ) {
414-
parentType = ExpressionExperiment.class;
415-
FactorValue factor = ( FactorValue ) sessionFactory.getCurrentSession()
416-
.get( FactorValue.class, aoi.getIdentifier() );
417-
ExpressionExperiment ee = expressionExperimentService.findByFactorValue( factor );
418-
parentIdentifier = ee != null ? ee.getId() : null;
419-
} else if ( BioAssay.class.isAssignableFrom( clazz ) ) {
420-
parentType = ExpressionExperiment.class;
421-
BioAssay ba = ( BioAssay ) sessionFactory.getCurrentSession()
422-
.get( BioAssay.class, aoi.getIdentifier() );
423-
ExpressionExperiment ee = expressionExperimentService.findByBioAssay( ba );
424-
parentIdentifier = ee != null ? ee.getId() : null;
425-
} else if ( BioMaterial.class.isAssignableFrom( clazz ) ) {
426-
parentType = ExpressionExperiment.class;
427-
BioMaterial bm = ( BioMaterial ) sessionFactory.getCurrentSession()
428-
.get( BioMaterial.class, aoi.getIdentifier() );
429-
Collection<ExpressionExperiment> ees = expressionExperimentService.findByBioMaterial( bm );
430-
if ( ees.size() == 1 ) {
431-
parentIdentifier = ees.iterator().next().getId();
432-
} else if ( ees.size() > 1 ) {
433-
log.warn( "More than one ExpressionExperiment refer to " + bm + ", cannot pick its parent ACL identity." );
434-
parentIdentifier = null;
435-
} else {
436-
parentIdentifier = null;
437-
}
438-
} else if ( MeanVarianceRelation.class.isAssignableFrom( clazz ) ) {
439-
MeanVarianceRelation mvr = ( MeanVarianceRelation ) sessionFactory.getCurrentSession()
440-
.get( MeanVarianceRelation.class, aoi.getIdentifier() );
441-
ExpressionExperiment ee = expressionExperimentService.findByMeanVarianceRelation( mvr );
442-
parentType = ExpressionExperiment.class;
443-
parentIdentifier = ee != null ? ee.getId() : null;
444-
} else {
445-
// try automated!
446-
SecuredChild<?> sc = ( SecuredChild<?> ) sessionFactory.getCurrentSession()
447-
.get( clazz, aoi.getIdentifier() );
448-
if ( sc != null && sc.getSecurityOwner() != null ) {
449-
//noinspection unchecked
450-
parentType = Hibernate.getClass( sc.getSecurityOwner() );
451-
parentIdentifier = sc.getSecurityOwner().getId();
452-
} else if ( sc == null ) {
453-
log.warn( "Cannot resolve ACL identity for " + clazz.getSimpleName() + " Id=" + aoi.getIdentifier() + "." );
454-
parentType = null;
455-
parentIdentifier = null;
456-
} else {
457-
log.warn( "Cannot resolve parent ACL identity for " + aoi.getType() + "." );
458-
parentType = null;
459-
parentIdentifier = null;
460-
}
461-
}
462-
if ( parentIdentifier != null ) {
463-
AclObjectIdentity parentAoi = ( AclObjectIdentity ) sessionFactory.getCurrentSession()
464-
.createQuery( "select aoi from AclObjectIdentity aoi where aoi.type = :type and aoi.identifier = :identifier" )
465-
.setParameter( "type", parentType.getName() )
466-
.setParameter( "identifier", parentIdentifier )
467-
.uniqueResult();
468-
if ( parentAoi != null ) {
469-
return parentAoi;
470-
} else {
471-
log.warn( "Could not resolve ACL identity for " + parentType.getSimpleName() + " Id=" + parentIdentifier + "." );
472-
return null;
473-
}
474-
} else {
475-
return null;
476-
}
477-
}
478-
479406
/**
480407
* Lint for securable entities that are explicitly not children, but have a parent object set in their ACL identity.
481408
* <p>

0 commit comments

Comments
 (0)