spring-projects / spring-security

Spring Security
http://spring.io/projects/spring-security
Apache License 2.0
8.72k stars 5.87k forks source link

PreAuthorize not working on Services with an Interface, and also still not working on Kotlin Co-routines #15367

Open dreamstar-enterprises opened 2 months ago

dreamstar-enterprises commented 2 months ago

Much background is here:

https://stackoverflow.com/questions/78698990/spring-webflux-spring-security-preauthorize-not-working-work-using-kotlin/78699482?noredirect=1#comment138758490_78699482

I realise this had been opened in the past as an issue, but all similar issues raised have been closed off for some reason..?

I did some further tests, and I can absolutely confirm:

Does not work with Co-routines

If I do this, in the controller, on Spring Security 6.3.1,

    @GetMapping("/users/{uid}")
    @PreAuthorize("denyAll()")
    suspend fun getUser(
        @PathVariable("uid") uidString: String
    ): ResponseEntity<UserDTO> {
        // validate user ID
        val uid = userService.validateUserId(uidString)
        // get from database inside this co-routine
        return withContext(Dispatchers.IO) {
            ResponseEntity.ok(userGetService.getUserSecured(uid).awaitSingleOrNull())
        }
    }

I get this:

"message": "The returnType class java.lang.Object on public java.lang.Object com.example.timesheetapi.controllers.UserController.getUser(java.lang.String,kotlin.coroutines.Continuation) must return an instance of org.reactivestreams.Publisher (for example, a Mono or Flux) in order to support Reactor Context",

It really does not like Kotlin Co-routines...

There is a 3 year old issue here, I found: https://stackoverflow.com/questions/65507792/enablereactivemethodsecurity-for-a-reactive-kotlin-application-with-coroutines But the issue was closed for some reason...

Does not work with Methods on Services with Interfaces

Two separate issues here:

Issue 1.

If I do this, on a method in the service, it doesn't work

    @PreAuthorize("denyAll()")
    override fun getUserSecured(uid: ObjectId): Mono<UserDTO> {
        return mono {
            getUser(uid)
        }
    }

Issue 2 If I put it on the method in the interface of the service, it gives me this

internal interface UserGetService {

     suspend fun getUser(
        uid: ObjectId
     ): UserDTO

    @PreAuthorize("permitAll()")
     fun getUserSecured(
        uid: ObjectId
     ): Mono<UserDTO>

}

I get this

message": "Found more than one annotation of type interface org.springframework.security.access.prepost.PreAuthorize attributed to public reactor.core.publisher.Mono com.example.timesheetapi.services.users.UserServiceImpl.getUserSecured(org.bson.types.ObjectId) Please remove the duplicate annotations and publish a bean to handle your authorization logic.",

I found out that this is because I'm using the pattern:

@Service
internal class UserServiceImpl(
    @Autowired
    private val userRepo: UserRepositoryImpl,
    private val userDTOEntityMapMapper: UserDTOEntityMapMapperImpl,
    private val userMapKeyRenamer: UserMapKeyRenamerImpl,
    private val userUtilityService: UserUtilityServiceDelegate,
) : UserPostService by UserPostServiceDelegate(userRepo, userDTOEntityMapMapper),
    UserGetService by UserGetServiceDelegate(userRepo, userDTOEntityMapMapper, userMapKeyRenamer),
    UserPutService by UserPutServiceDelegate(userRepo, userDTOEntityMapMapper, userUtilityService),
    UserDeleteService by UserDeleteServiceDelegate(userRepo),
    UserUtilityService by UserUtilityServiceDelegate(),

And then calling just UserServiceImpl from the controller (so I can call / get access to any method from any of the above services)

@RestController
internal class UserController(
    @Autowired
    private val userService: UserServiceImpl,
) {

But Spring is getting confused with the @PreAuthorize on the UserGetService interface, and says there are duplicate annotations (where there is only one) (another bug?)

What did work:

What did eventually work was this:


@PreAuthorize("permitAll()")
fun getUserSecured(uid: ObjectId): Mono<UserDTO> {
        return mono {
            getUser(uid)
        },

Putting it a completely separate service w/o an interface, not putting that service in a master service (like I did with UserServiceImpl), and injecting all the extra security services in the controller

@RestController
internal class UserController(
    @Autowired
    private val userService: UserServiceImpl, (has all functions in one from several delegates)
    private val userGetSecurityServiceImp: UserGetServiceSecurityDelegate,
    private val userPostSecurityServiceImp: UserPostServiceSecurityDelegate,
    private val userPutSecurityServiceImp: UserPutServiceSecurityDelegate,
   .....
) {

This is the only way I get this to work.

I would say the bugs / requests for enhancements would be, for @PreAuthorize:

(i) No interface support on services (requires creating separate services w/o an interface) (ii) Spring Boot gets confused and complaining about multiple bean annotations for @PreAuthorize, when there is only one (in my example on one method in the UserGetService interface). But Spring / Spring Security doesn't like the UserServiceImpl pattern (which I use to group several several classes, into one master service class, called UserServiceImpl, so I can just inject that one Service the controller) (ii) No Kotlin Co-routines support. I'm forced to create a mono function

Maybe there are better workarounds to the above. Sorry, I'm a novice!

Describe the bug A clear and concise description of what the bug is.

To Reproduce Steps to reproduce the behavior.

Expected behavior A clear and concise description of what you expected to happen.

Sample

A link to a GitHub repository with a minimal, reproducible sample.

Reports that include a sample will take priority over reports that do not. At times, we may require a sample, so it is good to try and include a sample up front.

jzheaux commented 2 months ago

Hi, @dreamstar-enterprises, thanks for all the detail. Are you able to produce a minimal GitHub sample that reproduces the issue?

spring-projects-issues commented 1 month ago

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

dreamstar-enterprises commented 1 month ago

Hi sorry for the delay.

Here is my code:

@RestController
@RequestMapping("/api/resource")
internal class UserController(
    @Autowired
    private val userService: UserServiceImpl,
    private val userServiceSecure: UserServiceSecureImpl,
) {

    @GetMapping("/users/{uid}")
    final suspend fun getUser(
        @PathVariable("uid") uidString: String
    ): ResponseEntity<UserDTO> {
        // validate user ID
        val uid = userService.validateUserId(uidString)
        // get from database inside this co-routine
        return withContext(Dispatchers.IO) {
            ResponseEntity.ok(userServiceSecure.getUserSecured(uid).awaitSingleOrNull())
        }
    }
}
@Service
internal class UserGetServiceDelegate(
    @Autowired
    private val userRepo: UserRepositoryImpl,
    private val userDTOEntityMapMapper: UserDTOEntityMapMapperImpl,
) : UserGetService {

    @PreAuthorize("denyAll()") 
    final override suspend fun getUser(uid: ObjectId): UserDTO {

        //** SIGNIFICANT DATABASE STEP **//
        // attempt to get user from database
        val userEntity = userRepo.getUser(uid)
                 ?: throw ErrorManager.getException("service-user-does-not-exist")

        // return retrieved user
        return userDTOEntityMapMapper.fromEntity(userEntity)
    }
}
internal class UserGetRepositoryDelegate(
    private val reactiveMongoTemplate: ReactiveMongoTemplate
) : UserGetRepository {

    override suspend fun getUser(uid: ObjectId): UserEntity? {

        /*************************/
        /* MONGO DB CALL       */
        /*************************/
        try {
            return UserEntityCaster.castToType(
                reactiveMongoTemplate.findById(uid, UserEntity::class.java).awaitSingleOrNull()
            )
        } catch (ex: Exception){
            throw ErrorManager.getException("repository-user-not-gotten",
                ErrorContext(data = mapOf("ex" to ex))
            )
        }
    }
}

I kept getting the below when I put @PreAuthorize("denyAll()") in the service or controller - example above shown being applied to the service:

"message": "The returnType class java.lang.Object on public java.lang.Object com.example.timesheetapi.controllers.UserController.getUser(java.lang.String,kotlin.coroutines.Continuation) must return an instance of org.reactivestreams.Publisher (for example, a Mono or Flux) in order to support Reactor Context"