Expected Behavior

SecurityExpressionRoot<T extends Authentication>

Current Behavior

SecurityExpressionRoot

Context

When extending SecurityExpressionRoot to enrich security DSL, I have to cast the returned value from super.getAuthentication(), which should not happen as the actual authentication type is known per SecurityExpressionRoot constructors.

Comment From: ch4mpy

For a minimal sample where I have to do such a cast: https://github.com/ch4mpy/spring-addons/blob/ef9b921480ee99a69a658e62237384ebcd767568/samples/tutorials/resource-server_with_specialized_oidcauthentication/src/main/java/com/c4soft/springaddons/tutorials/WebSecurityConfig.java#L96

As you can see, the actual Authentication impl is important the next line, where we access "getProxies()" which does not exist on Authentication interface.

Comment From: sjohnr

Hi @ch4mpy! I'm curious if you've tried using the bean approach, as suggested in this comment? I believe you can even pass the Authentication instance if you need to use it (though it's nice to stay decoupled from Spring Security if you can). You can also make your @Bean take the specific authentication type. For example:

    @Bean
    public MyExpressionHandler myExpressionHandler() {
        return new MyExpressionHandler();
    }

    public class MyExpressionHandler {
        public boolean hasProxy(MyAuthentication authentication, String subject, String permission) {
            ...
        }
    }

which can be used like this:

@PreAuthorize("@myExpressionHandler.hasProxy(authentication, #subject, 'read')")

In any case, adding a parameterized type to this class would be an unnecessary (possibly breaking) change since there are other ways to accomplish what you're looking to do.

With that in mind, I'm going to close this issue, but again feel free to add comments if you need to discuss this issue more and we can create a new issue if necessary.

Comment From: ch4mpy

I thought, we were about to have a brand new version of the framework with breaking changes.

I dream of a strongly typed Java world. Like Authentication<C, D, P> were credentials, details and principal would not be just Object, or accessors returning the same type of object as the one I provide to constructor (and not just one of the interfaces it implements)

@sjohnr , to answer your question, I can make a difference between 

@PreAuthorize("@myExpressionHandler.hasProxy(authentication, #subject, 'read')")

and

@PreAuthorize("hasProxy(#subject, 'read')")

The difference increases with expression complexity:

@PreAuthorize("@myExpressionHandler.isAgent(authentication) or @myExpressionHandler.hasProxy(authentication, #subject, 'read')")

VS

@PreAuthorize("isAgent() or hasProxy(#subject, 'read')")

More important, people from business who validate security rules and do not know about coding, make a difference too.

Comment From: sjohnr

Thanks for your feedback, @ch4mpy.

I thought, we were about to have a brand new version of the framework with breaking changes.

We do still want to keep breaking changes to a minimum, and unexpected and/or significant changes slow and complicate adoption of the new major version. APIs will be the part of the framework that is least likely to have breaking changes.

More important, people from business who validate security rules and do not know about coding, make a difference too.

You can create meta-annotations in this case to make the code more readable. I'd also recommend one or more integration tests for each meta-annotation to increase confidence in the code.

Comment From: ch4mpy

@sjohnr creating meta annotations for each and every security expression is viable solution only when there are a few different expressions. When there are many, it's pretty hard to keep expressive meta annotations.

Comment From: ch4mpy

@sjohnr what I achive wtih a generic copy of MethodSecurityExpressionRoot is just more convenient than the proposed @Bean alternative

I'll keep using your framework that way for now:

@PreAuthorize("isNice() or onBehalfOf(#otherSubject).can('greet')")

With just:

    @Bean
    public MethodSecurityExpressionHandler methodSecurityExpressionHandler() {
        return new GenericMethodSecurityExpressionHandler<>(MyMethodSecurityExpressionRoot::new);
    }

    static final class MyMethodSecurityExpressionRoot extends GenericMethodSecurityExpressionRoot<MyAuthentication> {
        public MyMethodSecurityExpressionRoot() {
            super(MyAuthentication.class);
        }

        public Proxy onBehalfOf(String proxiedUserSubject) {
            return getAuth().getProxyFor(proxiedUserSubject);
        }

        public boolean isNice() {
            return hasAnyAuthority("ROLE_NICE_GUY", "SUPER_COOL");
        }
    }

Even if that means duplicating some code from spring's MethodSecurityExpressionRoot and MethodSecurityExpressionHandler, which I'd surely prefer to avoid.

P.S. if you think I lack spring-security backround to take the right decision, please consider how security auto-configuration happens in the tutorial linked above (and also where @WithMyAuth( authorities = { "AUTHOR" }, claims = @OpenIdClaims(sub = "greeter", preferredUsername = "Tonton Pirate"), proxies = { @Proxy(onBehalfOf = "greeted", can = { "greet" }) }) comes from in the tests)