SEC-2083: Create a MethodSecurityExpressionHandler that can handle fixed-sized collections

[spring-projects-issues]

Mattias Severson (Migrated from SEC-2083) said:

When using annotations to filter collections based, e.g. @PostFilter("hasPermission(filterObject, 'SOME_PERMISSION')"), the DefaultMethodSecurityExpressionHandler.filter() gets called. The problem with this method is that if the filterTarget is an immutable list or an immutable set, an exception will thrown (because collection.clear() is called before the elements in the retainList are added back to the collection).

One solution to overcome this problem is to implement an "ImmutableMethodSecurityExpressionHandler" by subclassing the DefaultMethodSecurityExpressionHandler, override the filter() method if the filterTarget is of type List, Set, or SortedSet, do the filtering as before, but instead of clearing the existing collection, returning the retainList wrapped in Collections.unmodifiableList(), Collections.unmodifiableSet() or Collections.unmodifiableSortedSet() respectively.

UPDATE: We should also support Arrays.asList which is a fixed size collection

[spring-projects-issues]

Mattias Severson said:

Are you considering filtering of immutable collections? I have just submitted a pull reqiest to give you some idea to what I have in mind.

[spring-projects-issues]

Rob Winch said:

Thanks for the submission. I think I would prefer a strategy style pattern rather than inheritance. This would allow handling of custom types. One issue I see w/ the pull request is it does not seem to consider that some users may use specific types of collections. For example, if the method signature were a LinkedList this solution does not look like it would work since LinkedList is a List and the solution would use an ArrayList. A PR that would be merged would certainly need some tests to demonstrate these use cases working.

[gcorsaro]

I was better looking to this issue and I think it's misleading. it should be considered as a bug, not enhancement. As showed in my ticket #8427 the issue regards also mutable lists. This means that currently it's not possible to use the postfilter on any collection. Changing the scope could give different prioritization to this ticket I think.

[rwinch]

It works fine with collections that allow for changing the size. For example, LinkedList and ArrayList work just fine. There are tests to back this up.

[dbouclier]

In 2023, this issue is still problematic, here some code, I hope it could help people who fall in the trap like me

the controller using @PostFilter or @PostAuthorize with an immutable collection

@RestController
@RequiredArgsConstructor
@RequestMapping("/my")
public class MyController {

	private final MyService myService;	

    @PostMapping("/search")
    @PostFilter("hasPermission(filterObject, 'SOME_READ_PERMISSION')")
    public List<SomeStuff> search() {
        return myService.fetchSomeStuff()
        .stream()
        .map(someMapping())
        // my collection is immutable now !
        .toList();
    }
}

configuration of the MethodSecurityExpressionHandler using a Custom DefaultMethodSecurityExpressionHandler

@Configuration
@EnableMethodSecurity
@RequiredArgsConstructor
@Slf4j
public class AbacMethodSecurityConfiguration {

    private final AbacPermissionEvaluator abacPermissionEvaluator;

    @Bean
    protected MethodSecurityExpressionHandler createExpressionHandler() {
        LOG.info("Configuration: ABAC Permission evaluator");
        final var expressionHandler = new CustomMethodSecurityExpressionHandler();
        expressionHandler.setPermissionEvaluator(abacPermissionEvaluator);
        return expressionHandler;
    }
}

configuration of a Custom DefaultMethodSecurityExpressionHandler

public class CustomMethodSecurityExpressionHandler extends DefaultMethodSecurityExpressionHandler {

    // apply on filterObject in @PostFilter
    // example usage: @PostAuthorize("hasPermission(returnObject, 'SOME_PERMISSION')")
    @Override
    public Object filter(final Object filterTarget, final Expression filterExpression, final EvaluationContext ctx) {
        if (filterTarget instanceof final Collection<?> targetCollection) {
            return super.filter(new ArrayList<>(targetCollection), filterExpression, ctx);
        }
        return super.filter(filterTarget, filterExpression, ctx);
    }

    // apply on returnObject in @PostAuthorize
    // example usage:  @PostAuthorize("hasPermission(returnObject, 'SOME_PERMISSION')")
    @Override
    public void setReturnObject(final Object returnObject, final EvaluationContext ctx) {
          if (returnObject instanceof final Collection<?> returnCollection) {
            super.setReturnObject(new ArrayList<>(returnCollection), ctx);
        } else {
            super.setReturnObject(returnObject, ctx);
        }
    }
}

PermissionEvaluator implementation ...

@Component
@RequiredArgsConstructor
@Slf4j
public class AbacPermissionEvaluator implements PermissionEvaluator {

  
    @Override
    public boolean hasPermission(final Authentication authentication, final Object targetDomainObject, final Object permissionString) {
           
      // load permissions once !  
      var acls = fetchAcl();

        final Collection<SecuredObject> securedObjects = (Collection<SecuredObject>) targetDomainObjects;

        // black magic here, filtering the returned object collection (that why we need a mutable collection ...)
        securedObjects.removeIf(securedObject ->
          // evaluate the permission against loaded acls
             hasPermission(securedObject, acls)
         ));

    }
    
}