Spring Method Security does not seem to work correctly.

[Nick-Wunderdog]

Spring Method Security does not seem to work correctly.

[Nick-Wunderdog]

And let me say: Thanks for great project and all the hard work on it.

[paulcwarren]

I see. Thanks for the report @Nick-Wunderdog. Appreciate the detailed report.

The project is definitely missing test cases like this so I am not surprised there are inconsistencies here. Spring Content doesn't do too much to make this work, actually. It's mostly all Spring Security. But it looks like we're missing a hook somewhere. Will look into soonest.

[Nick-Wunderdog]

Thank you once again. It is very stange as it kind of works, but not correctly.

Debugging ObservationAuthorizationManager.check() with
curl -X PUT -H 'Content-Type:text/plain' -d 'Hello Spring Content World!' http://localhost:8080/attachments/1/content

1. it checks with RequestMatcherDelegatingAuthorizationManager for servlet security mapping (for there is just .authenticated() returning granted=true

2. Then it checks PreAuthorizeAuMa for AttachmentRepository.findbyId() class level security which evaluates to granted =true

3. Then it checks PreAuthorizeAuMa for AttachmentRepository.findbyId() class method security which evaluates to granted =true

4. Then it checks PostAuthorizeAuMa for AttachmentRepository.findbyId() class level security which evaluates to granted =true

5. Then it checks PostAuthorizeAuMa for AttachmentRepository.findbyId() class method security which evaluates to granted =true

6. then again RequestMatcherDelegatingAuthorizationManager which evaluates to granted =true

7. then again RequestMatcherDelegatingAuthorizationManager which evaluates to granted =true

At no point is there Class or method level Pre- or Post-Authorize checks for the AttachmentContentStore class.

I am also looking at StoreRestController and how it retries the Stores from Factory class. StoreRestController putContent and getContent Pre and PostAuthorize seucirity is checked but not the underlying AttachmentContentStore .

[paulcwarren]

I've debugged this a little bit today and I cant reproduce some of your findings thus far. Moreover, Repository and ContentStore behavior appear identical to me.

I'm playing around with a repository as follows:

@PreAuthorize("false")
public interface FileRepository extends JpaRepository<File, Long> {

    @Override
    @PreAuthorize("true")
    public Optional<File> findById(Long id);

    @Override
    @PreAuthorize("true")
    public File save(File entity);
}

And a ContentStore as follows:

@PreAuthorize("false")
public interface FileContentStore extends ContentStore<File, String> {

	@PreAuthorize("true")
	@Override
	Resource getResource(File entity, PropertyPath propertyPath, GetResourceParams params);

	@PreAuthorize("true")
	@Override
	File setContent(File entity, PropertyPath propertyPath, InputStream content, long contentLen);
}

And, for example, when I POST, or PUT to /files/ and /files/{id}. Or GET from /files/{id} it is also allowed. i.e. to the Spring Data repository. Likewise when I PUT or GET to /files/{id}/content; i.e. the content store, it is also allowed. In all cases the AuthorizationManagerBeforeMethodInterceptor is used to perform the auth checks.

If I flip the PreAuthorize's (class == true, findById & getResource == false) then GET /files/{id} and GET /files/{id}/content then both return 403.

I'll continue to try and reproduce your findings but one point to note is that Spring Data REST and Spring Content REST layers both perform findById calls to find the entity that is being acted on. In addition SCR performs a getResource call and then potentially a setContent and a repository save. I noticed in my own testing it is easy to accidentally lock operations down. Just a thought.

[Nick-Wunderdog]

Strange.
I am using Temurin 17 Java, latest build + Spring Boot 3.0.5 parent

Running the app and doing HTTP PUT to existing "attachment" parent entity.

curl -X PUT "{host}/api/attachments/{id}/content" -H "Content-Type: text/plain" -d "Hello"

For classes identical to ones you posted, evept I have overriden all methods in ContentStore

@PreAuthorize("false")
@StoreRestResource
public interface AttachmentContentStore extends ContentStore<Attachment, String> {

// all methods overidden with 
@PreAuthorize("true")

}

Debugging that call in ObservationAuthorizationManager.check() method the authorization flow is :

1. RequestMatcherDelegatingAuthrizationManager.check() = true
2. RequestMatcherDelegatingAuthrizationManager.check() = true
3. PreAuthorizeAuthrizationManager.check() = true // from JPA repo findById() METHOD PreAuth
4. PreAuthorizeAuthrizationManager.check() = false // from Store CLASS PreAuth! which should not happen, like it does not happen for Repo-class with identical annotations.

So the difference for me is that while identically annotated JPA repoclass method level annotation overrrides the class level annotation and the method level @PerAuthorize in evaluated instead. In ContentStore it always seem to evaluate the Class level annotation.

If I remove the Class level annotation from ContentStore, then it works and checks the method level annotation and the PUT works. This could be somewhat acceptable solution, albeit not secure. But now without class level security annotation the method level security annotation is never called for GET

curl -X GET "{host}/api/attachments/{id}/content" -H "Content-Type: text/plain"

GET does return the content, even if the methods are annotated @PreAuthorize("false") so in my environment that is not working either.

The app is pretty vanilla Spring Boot the Data REST + Security only with Keyclock auth integration :

@Configuration
@EnableWebSecurity
@EnableMethodSecurity
public class SecurityConfig {

   @Autowired
    public SecurityConfig(JwtAuthConverter jwtAuthConverter) {
        this.jwtAuthConverter = jwtAuthConverter; // just a converter for JWT token ans roles to Spring Security Principal
    }
    @Bean
    public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Exception {
        return http
                .anyRequest().authenticated().and()
                .oauth2ResourceServer().jwt().jwtAuthenticationConverter(jwtAuthConverter).and().and()
                .sessionManagement().sessionCreationPolicy(SessionCreationPolicy.STATELESS).and()
                .build();
    }

[Nick-Wunderdog]

I create simple debugging Service class to help debugging annotation on interface :

@Service
public class DebugService {
    public boolean debug(Object o) {
        log.debug("****** DEBUG: {} ******", o);
        AuthorizationManagerAfterMethodInterceptor
        return true;
    }
}    

I also set org.springframework.security.authorization.method package log level to DEBUG, it has good debug logging.

I annotated :

@RepositoryRestResource
@PreAuthorize("@debugService.debug('AttachmentRepository Class level security') AND false")
public interface AttachmentRepository extends CrudRepository<Attachment, Long> {
    @Override
    @PreAuthorize("@debugService.debug('AttachmentRepository findById method security') AND isFullyAuthenticated()")
    Optional<Attachment> findById(Long id);

and

@StoreRestResource
// NO class level securuty annotation 
public interface AttachmentContentStore extends ContentStore<Attachment, String> {
    @Override
    @PreAuthorize("@debugService.debug('Store setContent1 security')")
// All methods 

so PUT /attachmetns/{id}/content -d hello logs :

2023-04-19 16:00:07,790 DEBUG o.s.s.a.m.AuthorizationManagerBeforeMethodInterceptor Authorizing method invocation ReflectiveMethodInvocation: public abstract java.util.Optional AttachmentRepository.findById(java.lang.Long); target is of class [jdk.proxy4]
2023-04-19 16:00:07,792 DEBUG DebugService ****** DEBUG: AttachmentRepository findById method security ******
2023-04-19 16:00:07,793 DEBUG o.s.s.a.m.AuthorizationManagerBeforeMethodInterceptor Authorized method invocation ReflectiveMethodInvocation: public abstract java.util.Optional AttachmentRepository.findById(java.lang.Long); target is of class [jdk.proxy4]
2023-04-19 16:00:07,819 DEBUG o.s.s.a.m.AuthorizationManagerAfterMethodInterceptor [] Authorizing method invocation ReflectiveMethodInvocation: public abstract java.util.Optional xxx.repository.AttachmentRepository.findById(java.lang.Long); target is of class [jdk.proxy4.$Proxy204]
2023-04-19 16:00:07,820 DEBUG o.s.s.a.m.AuthorizationManagerAfterMethodInterceptor [] Authorized method invocation ReflectiveMethodInvocation: public abstract java.util.Optional AttachmentRepository.findById(java.lang.Long); target is of class [jdk.proxy4]

[edit]
It does also call method level annotation on PUT

     @Override
    @PreAuthorize("@debugService.debug('Store setContent3 method level')")
    Attachment setContent(Attachment attachment, PropertyPath propertyPath, InputStream inputStream, long l);

```Logging just did not work as your software uses jcl and I have slf4j on app.

But it does not call the method level security on GET. 

[Nick-Wunderdog]

@paulcwarren I am trying to work around this problem. Can you confirm that there are no direct HTTP access to the content ?
That there is no "/content/{content_id} mapping or something to access the content without internally first retrieving the JPA parent entity "/files/{jpa_entity_id}/content" ?

That would defacto secure the content access via the parent entity JPA repository security.

[paulcwarren]

Still debugging. I added your debug service to my app and ran a test that gets the entity, gets entity content and set entity content and I get this output. Note, its is note always exactly the same - but it does show method and class level PreAuthorizes being called.

# these calls are direct to the JAVA API
****** DEBUG: {FileRepository save security} ******
****** DEBUG: {FileContentStore setContent security} ******
****** DEBUG: {FileRepository save security} ******

# these calls a via SDR/SCR
****** DEBUG: {FileContentStore class level security} ******
****** DEBUG: {FileRepository class level security} ******
****** DEBUG: {FileRepository class level security} ******
****** DEBUG: {FileContentStore class level security} ******
****** DEBUG: {FileRepository findById security} ******
****** DEBUG: {FileContentStore class level security} ******
****** DEBUG: {FileRepository class level security} ******
****** DEBUG: {FileRepository class level security} ******
****** DEBUG: {FileContentStore class level security} ******
****** DEBUG: {FileRepository findById security} ******
****** DEBUG: {FileContentStore getResource security} ******
****** DEBUG: {FileContentStore class level security} ******
****** DEBUG: {FileRepository class level security} ******
****** DEBUG: {FileRepository class level security} ******
****** DEBUG: {FileContentStore class level security} ******
****** DEBUG: {FileRepository findById security} ******
****** DEBUG: {FileContentStore getResource security} ******
****** DEBUG: {FileContentStore class level security} ******
****** DEBUG: {FileRepository save security} ******

One thing I noted whilst debugging was that if I miss-specified the overidden method in the store interface (or I suspect Respsitory interface) - for example I omitted @Override or accidentally used one of the other overloaded methods (i.e. one that the REST api does not call) then the authorization check can accidentally find the class level @PreAutorize

Anyway, I'm going to create a sample app and share it with you.

Regards the direct REST APIs. No SCR does not offer any direct to content API for ContentStores.

[Nick-Wunderdog]

I used IDE(A) automation to override all the methods form ContentStore and I added @PreAuthorize on all of them.
I rewritten the tests several time and I consistently get same result that the method level @PreAuthorize("true") is never called in ContentStore class if there is class level @PreAuthorize("false") or vice versa. There is something really strange that we are getting differnet results.
I sent you an email with my contact details, in case you want to have a call and see this live.

Here is my pom.xml

<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">
    <modelVersion>4.0.0</modelVersion>
    <parent>
        <groupId>org.springframework.boot</groupId>
        <artifactId>spring-boot-starter-parent</artifactId>
        <version>3.0.5</version>
        <relativePath/> <!-- lookup parent from repository -->
    </parent>
    <groupId>com.example</groupId>
    <artifactId>example</artifactId>
    <version>0.0.1-SNAPSHOT</version>
    <name>Example</name>
    <description>Example to reproduce Spring security problem</description>
    <properties>
        <java.version>17</java.version>
        <file.encoding>UTF-8</file.encoding>
        <skip.npm.build>false</skip.npm.build>
    </properties>
    <dependencyManagement>
        <dependencies>
            <dependency>
                <groupId>org.springframework.boot</groupId>
                <artifactId>spring-boot-starter</artifactId>
                <exclusions>
                    <exclusion>
                        <groupId>org.springframework.boot</groupId>
                        <artifactId>spring-boot-starter-logging</artifactId>
                    </exclusion>
                </exclusions>
            </dependency>
            <dependency>
                <groupId>org.springframework.data</groupId>
                <artifactId>spring-data-bom</artifactId>
                <version>2022.0.2</version>
                <scope>import</scope>
                <type>pom</type>
            </dependency>
            <dependency>
               <groupId>com.github.paulcwarren</groupId>
                <artifactId>spring-content-jpa</artifactId>
                <exclusions>
                    <exclusion>
                        <groupId>commons-logging</groupId>
                        <artifactId>commons-logging</artifactId>
                    </exclusion>
                </exclusions>
            </dependency>
        </dependencies>
    </dependencyManagement>
    <dependencies>

        <!-- Spring boot -->
        <dependency>
            <groupId>org.springframework.boot</groupId>
            <artifactId>spring-boot-starter-actuator</artifactId>
        </dependency>
        <dependency>
            <groupId>org.springframework.boot</groupId>
            <artifactId>spring-boot-starter-data-jpa</artifactId>
        </dependency>
        <dependency>
            <groupId>org.springframework.data</groupId>
            <artifactId>spring-data-jpa</artifactId>
        </dependency>
        <dependency>
            <groupId>org.springframework.data</groupId>
            <artifactId>spring-data-envers</artifactId>
        </dependency>
        <dependency>
            <groupId>org.springframework.boot</groupId>
            <artifactId>spring-boot-starter-data-rest</artifactId>
        </dependency>
        <dependency>
            <groupId>org.springframework.boot</groupId>
            <artifactId>spring-boot-starter-hateoas</artifactId>
        </dependency>
        <dependency>
            <groupId>org.springframework.boot</groupId>
            <artifactId>spring-boot-starter-web</artifactId>
        </dependency>
        <dependency>
            <groupId>org.springframework.boot</groupId>
            <artifactId>spring-boot-starter-security</artifactId>
        </dependency>
        <dependency>
            <groupId>org.springframework.boot</groupId>
            <artifactId>spring-boot-starter-oauth2-resource-server</artifactId>
        </dependency>

        <!-- DB -->
        <dependency>
            <groupId>com.h2database</groupId>
            <artifactId>h2</artifactId>
        </dependency>
        <dependency>
            <groupId>org.hibernate.validator</groupId>
            <artifactId>hibernate-validator</artifactId>
            <version>8.0.0.Final</version>
        </dependency>
        <dependency>
            <groupId>org.flywaydb</groupId>
            <artifactId>flyway-core</artifactId>
        </dependency>
        <dependency>
            <groupId>com.github.paulcwarren</groupId>
            <artifactId>spring-content-jpa-boot-starter</artifactId>
            <version>3.0.1</version>
            <exclusions>
                <exclusion>
                    <groupId>commons-logging</groupId>
                    <artifactId>commons-logging</artifactId>
                </exclusion>
                <exclusion>
                    <groupId>jakarta.servlet</groupId>
                    <artifactId>jakarta.servlet-api</artifactId>
                </exclusion>
            </exclusions>
        </dependency>
        <dependency>
            <groupId>com.github.paulcwarren</groupId>
            <artifactId>spring-content-rest-boot-starter</artifactId>
            <version>3.0.1</version>
            <exclusions>
                <exclusion>
                    <groupId>commons-logging</groupId>
                    <artifactId>commons-logging</artifactId>
                </exclusion>
                <exclusion>
                    <groupId>jakarta.servlet</groupId>
                    <artifactId>jakarta.servlet-api</artifactId>
                </exclusion>
            </exclusions>
        </dependency>
<!--
        <dependency>
            <groupId>com.github.paulcwarren</groupId>
            <artifactId>spring-content-fs-boot-starter</artifactId>
            <version>3.0.1</version>
        </dependency>
-->

        <!-- Log -->
        <dependency>
            <groupId>org.springframework.boot</groupId>
            <artifactId>spring-boot-starter-log4j2</artifactId>
        </dependency>
        <dependency>
            <groupId>com.fasterxml.jackson.dataformat</groupId>
            <artifactId>jackson-dataformat-yaml</artifactId>
            <version>2.14.2</version>
        </dependency>

        <!-- Tooling -->
        <dependency>
            <groupId>org.springframework.data</groupId>
            <artifactId>spring-data-rest-hal-explorer</artifactId>
        </dependency>

        <dependency>
            <groupId>org.springframework.boot</groupId>
            <artifactId>spring-boot-devtools</artifactId>
            <scope>runtime</scope>
            <optional>true</optional>
        </dependency>
        <dependency>
            <groupId>org.springframework.boot</groupId>
            <artifactId>spring-boot-configuration-processor</artifactId>
            <optional>true</optional>
        </dependency>

        <!-- Test -->
        <dependency>
            <groupId>org.springframework.boot</groupId>
            <artifactId>spring-boot-starter-test</artifactId>
            <scope>test</scope>
        </dependency>
        <dependency>
            <groupId>org.springframework.security</groupId>
            <artifactId>spring-security-test</artifactId>
            <scope>test</scope>
        </dependency>

    </dependencies>

    <build>
        <plugins>
            <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-compiler-plugin</artifactId>
                <configuration>
                    <source>16</source>
                    <target>16</target>
                </configuration>
            </plugin>

            <plugin>
                <groupId>org.springframework.boot</groupId>
                <artifactId>spring-boot-maven-plugin</artifactId>
                <executions>
                    <execution>
                        <goals>
                            <goal>repackage</goal>
                        </goals>
                    </execution>
                </executions>
                <configuration>
                    <imageName>${project.groupId}/${project.artifactId}:${project.version}</imageName>
                </configuration>
            </plugin>

        </plugins>
    </build>

    
</project>

[paulcwarren]

OK. I added a sample project that shows the method level preauthorize do get called.

To try it yourself you should just need to git clone it and run the GettingStartedTest. Modify as needed to test the different scenarios.

Debugging using this I can see that method-level authorizations are called on both repositories and stores when they can be found on the repository (or store) proxy. If an annotated method is not found though, it will look for and use the class level authorization.

[Nick-Wunderdog]

@paulcwarren I found the cause of (my) problems in your : `DefaultEntityResolver.findOne()" method code.

Starting point :
Content parent Entity repository ".findById()" work fine and is Secured with class and method level @PreAuthorize and @PostAuthorize rules. Evaluating to false in the test case for example just @PreAuthorize("false").

When used via Content Store there is NPE thrown from the `DefaultEntityResolver.findOne()" method from the actually AccessDenied - exception to POST (/files/1/content) HTTP request.

public class DefaultEntityResolver implements EntityResolver {
...
  public Object findOne(Repositories repositories, StoreInfo info, Class<?> domainObjClass, String repository, Serializable id)
            throws HttpRequestMethodNotSupportedException {

        Optional<Object> domainObj = null; //TODO: = Optional.empty(); 

        if (ROOT_RESOURCE_INFORMATION_CLASS_PRESENT) {

            RepositoryInvoker invoker;
            try {
                invoker = resolveRootResourceInformation(info, repository, id, new ModelAndViewContainer(), new FakeWebBinderFactory());
                if (invoker != null) {
                    domainObj = invoker.invokeFindById(id);
                }
            } catch (ConverterNotFoundException e) {
                domainObj = findOneByReflection(repositories, domainObjClass, id);
//TODO:             } catch (org.springframework.security.access.AccessDeniedException  e) { throw e;   

            
            } catch (Exception e) {
                e.printStackTrace(); //TODO: log.error("A helpful information message of the unrecoverable error ", e.cause()); and return or throw something   
            }
            
        } else {

            domainObj = findOneByReflection(repositories, domainObjClass, id);
        }
        return domainObj.orElseThrow(ResourceNotFoundException::new);
    }

Problem 1) : Optional object is set null, which is not "legal"
Optional<Object> domainObj = null; // Optional.empty()

Simply Optional<Object> domainObj = Optinal.empty(); might fix all problems.

Problem 2.1): your catch-all block catched AccessDenied exception and only vomits stack trace. Leaving the domainObj == null so the method return calls .orElseThrow() from null.

Problem 2.2): Catch-all block supresses the AccessDenied - exception, which I think should be thrown depending if desired outcome is ResourceNotFoundException or AccessDeniedException. Add explicit catch for ADE.

Problem 2.3): [nitpicking] e.printStackTrace(); is security anti-pattern just like SOut, it leaks information without any control, as opposed to log.error("", e.cause()) is controllable, as it is specific level and specific logger which can have specific security settings, like more more restricted read access.

[Nick-Wunderdog]

I added an PR:
#1400

Logging and catching ACE are optional, but just initializing the optional as empty and not as null would resolve the two NPE conditions.

[Nick-Wunderdog]

I guess the RP did not make to 3.0.2 ?

[paulcwarren]

It did not but it sounds like it resolves your issue? If so I can cut another release for you.

[Nick-Wunderdog]

I have not tried 3.0.2 yet. It was released day before my PR was merged. That is why I asked.

…

On Tue, 9 May 2023, 11:59 Paul Warren, ***@***.***> wrote: It did not but it sounds like it resolves your issue? If so I can cut another release for you. — Reply to this email directly, view it on GitHub <#1374 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A6UFVFRYVBPGNXTBSOA7OQDXFIBQLANCNFSM6AAAAAAXBVHWYA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[paulcwarren]

The fix is not in 3.0.2 so that wont work. I was asking if the PR represents a completed fix for you? If so I will cut a 3.0.3 release for you.

[paulcwarren]

Hi @Nick-Wunderdog , I cut 3.0.3 release for you yesterday. I haven't created the github release yet but its out there on maven central.

[paulcwarren]

Fixed in 3.0.3