Reactor - Flux and Mono can't get them to work at all #692
Comments
|
Same here. Mutation returns following error Query returns Monojust to string instead of unwrapped value |
|
After adding KotlinDataFetcherFactoryProvider Bean explicitly. Mono started working. After adding below mono started working. BTW, @thunderbird preAuhorize is failing for me. Any idea ? |
|
I thought I had nailed the spring security side, but as soon as I ran some tests it failed for me as well. I don't fancy reading all the spring code, but I am looking into it once I solve the mono problem. I'm very new to graphql. I'm still a big skeptic, seems very stateful and non-scalable to me in comparison to rest (which I'd have done my api in far less time). But I am going to keep at it because apparently the benefits outweigh the negatives. |
So having a look at the code here, the only reason it starts working is the fact that the data loader converts a mono to a completable future. I suppose it works. |
|
@thunderbird Have similar requirements, trying to keep parity b/w REST and Graphql QL. |
|
At the moment, this is what I am using: @Component
class ReactiveSecurityContextFactory: GraphQLContextFactory<GraphQLSecurityContext> {
@ExperimentalCoroutinesApi
override suspend fun generateContext(request: ServerHttpRequest, response: ServerHttpResponse): GraphQLSecurityContext {
val reactorContext = coroutineContext[ReactorContext]?.context ?: throw RuntimeException("reactor context unavailable")
val securityContext = reactorContext.getOrDefault<Mono<SecurityContext>>(SecurityContext::class.java, Mono.empty())
return GraphQLSecurityContext(securityContext = securityContext)
}
}And class GraphQLSecurityContext(val securityContext: Mono<SecurityContext>) : GraphQLContext |
|
Surprisingly given that expedia are maintaining the library, I would have thought a ton of wiring for reactive security, but maybe that is just not incorporated into the library. I'm bundling everything I learn / write to fix my issues into an auto configuration library. I'll look at publishing it on my repo in case it helps anyone. Does seem like a LOT of faffing to get this working. It's a young library though I suppose. |
|
@thunderbird Yes sharing the whole repo would help, if you have customized some parts on security config towards JWT validation. |
|
I’m getting there. Will require a directive. Not intuitive. If I can’t sort it. I’m going back to rest. I’ll have an update tonight. Will need to separate the module out of my project though. |
|
Problem is here in PrePostAdviceReactiveMethodInterceptor: @Override
public Object invoke(final MethodInvocation invocation) {
Method method = invocation.getMethod();
Class<?> returnType = method.getReturnType();
if (!Publisher.class.isAssignableFrom(returnType)) {
throw new IllegalStateException("The returnType " + returnType + " on " + method + " must return an instance of org.reactivestreams.Publisher (i.e. Mono / Flux) in order to support Reactor Context");
}
Class<?> targetClass = invocation.getThis().getClass();
Collection<ConfigAttribute> attributes = this.attributeSource
.getAttributes(method, targetClass);
PreInvocationAttribute preAttr = findPreInvocationAttribute(attributes);
Mono<Authentication> toInvoke = ReactiveSecurityContextHolder.getContext()
.map(SecurityContext::getAuthentication)
.defaultIfEmpty(this.anonymous)
.filter( auth -> this.preInvocationAdvice.before(auth, invocation, preAttr))
.switchIfEmpty(Mono.defer(() -> Mono.error(new AccessDeniedException("Denied"))));Specifically: ReactiveSecurityContextHolder.getContext() <- this is not going to have anything in it. So there are two choices. 1. Try and figure out how to populate it within the current context. Or go another route and use a directive with a custom "authorised" annotation. Which appears at first glance to be the way forward as I've spent enough time on it. I'm not sure what it's going to break, but hey. |
|
Another way is to override replace or wrap this: ContextWebFilter which has an order value of 0 and and spit a security context into the mono subscriber context |
|
Hello @thunderbird and @senthilsivanath. This library tries to take a generic approach to the implementation details, so out of the box we do not handle However these can easily be supported using two approaches: We recommend using coroutines where needed and then calling the functions with proper As for webfilter ordering, we have set the |
|
It looks like you may need to set the config for the security web filter order: spring-projects/spring-boot#1640
|
I'll give it a go |
|
Feel free to continue your discussions here or use our slack channel, however I am going to close this issue as it is a question about integrations outside of https://github.com/ExpediaGroup/graphql-kotlin#-contact |
|
Changing order didn't help for me. @smyrick Can future with ReactiveContext create a problem ? spring-projects/spring-security#5690 |
|
A directive might be the easiest way. By the time we hit PreAuth the securitycontext authentication is anonymous |
|
@thunderbird directive would work only with graphql. Happy to be corrected. We are looking for both graphql and rest with same controller. |
|
spring-security annotations currently won't work with GraphQL server. AFAIK they rely on having some reactive chain in order to work. Currently we are relying on Proper way of doing this is to:
|
That's where I was headed pretty much. I have a custom authorise annotation ready to go. |
|
So in a nut shell, without reimplementing half of spring security into directives. I can't secure my apis. |
Not necessarily, other users have managed to get spring security context with this directive implementation: #663 And this is just an underlying issue that we run on top of graphql-java which doesn't use project reactor to integrate easily with how Spring Security is implemented. But there are many other ways you can add security to a server. |
Yes I did notice that, however @PreAuthoze allows the use of expressions. I'd have to do a lot of wiring to make it fully functional. Nonetheless I have implemented a simple directive that does a similar thing on #663 for now and I will have to enhance it as I go.
I have noticed they recently added webflux support... I agree that there are other ways to add security to a server, but half the power of spring security is that it takes care of a lot of the hassle. |
|
Anyway, thanks for the help. Here is the code for anyone that wants it. Obviously a todo will be to add expression support: class AuthorisedDataFetcher(private val originDataFetcher: DataFetcher<Any?>, val roles: Array<out String>) : DataFetcher<Any?> {
val logger = LoggerFactory.getLogger(AuthorisedDataFetcher::class.java)
@Throws(AccessDeniedException::class)
override fun get(environment: DataFetchingEnvironment): Any? {
val securityContext = environment.getContext<GraphQLSecurityContext>().securityContext
val authentication = securityContext.authentication
if (!authentication.isAuthenticated || (roles.isNotEmpty() && authentication.authorities.filter {
roles.contains(it.authority) }.none())) {
throw AccessDeniedException("Access denied") // TODO Make sure this gets logged for audit purposes
}
return originDataFetcher.get(environment)
}
}@Component
class ReactiveDataFactoryProvider(val dataFetcherFactory: ReactiveDataFetcherFactory, val objectMapper: ObjectMapper) :
SimpleKotlinDataFetcherFactoryProvider(objectMapper) {
override fun functionDataFetcherFactory(target: Any?, kFunction: KFunction<*>) = DataFetcherFactory {
val authorised = kFunction.findAnnotation<Authorised>()
val reactiveDataFetcher = ReactiveFunctionDataFetcher(
target = target,
fn = kFunction,
objectMapper = objectMapper)
when {
authorised != null -> AuthorisedDataFetcher(reactiveDataFetcher, authorised.roles)
else -> reactiveDataFetcher
}
}
override fun propertyDataFetcherFactory(kClass: KClass<*>, kProperty: KProperty<*>): DataFetcherFactory<Any?> =
if (kProperty.isLateinit) {
dataFetcherFactory
} else {
super.propertyDataFetcherFactory(kClass, kProperty)
}
}@Component
class ReactiveDataFetcherFactory: DataFetcherFactory<Any?>, BeanFactoryAware {
private lateinit var beanFactory: BeanFactory
override fun setBeanFactory(beanFactory: BeanFactory) {
this.beanFactory = beanFactory
}
@Suppress("UNCHECKED_CAST")
override fun get(environment: DataFetcherFactoryEnvironment?): DataFetcher<Any?> {
// Strip out possible `Input` and `!` suffixes added to by the SchemaGenerator
val targetedTypeName = environment?.fieldDefinition?.type?.deepName?.removeSuffix("!")?.removeSuffix("Input")
return beanFactory.getBean("${targetedTypeName}DataFetcher") as DataFetcher<Any?>
}
}class ReactiveFunctionDataFetcher(target: Any?, fn: KFunction<*>, objectMapper: ObjectMapper): FunctionDataFetcher(target, fn, objectMapper) {
override fun get(environment: DataFetchingEnvironment): Any? = when (val result = super.get(environment)) {
is Mono<*> -> result.toFuture()
else -> result
}
}Edit: Forgot the annotation @GraphQLDirective(
name = "authorised",
description = "Used to check authorisation"
)
annotation class Authorised(vararg val roles: String)Example: @Authorised()
suspend fun currentUserProfile(context: GraphQLSecurityContext): UserProfile? {
logger.debug("Query executed")
return mono { context.securityContext }.flatMap { sc ->
val details = sc.authentication.details as UserCredential
userIdentityService.findById(details.id).map { UserProfile.fromUserIdentity(it) }
}.awaitFirst()
} |
GraphQL Java does not support Webflux. We are not using any of their extension libraries like We want to make using GraphQL APIs easier for everyone, so if there is some extension or directives that you add that will work with any spring security implementation we would be happy to reference that in our documentation. However, currently there is nothing we can change about the library that would make this work out of the box, unless we do a pure kotlin implementation rewrite of graphql-java or migrate to an existing one. That would be a very large effort. |
|
@thunderbird Good one. Do you still need mono -> future. As there is no PreAuthorize, we can move to suspend, in that case Mono would go away. |
I've read a lot of code today, understandably it would be a big effort. It would also be interesting to see if there is another way of wrapping graphql-java to support this. But the directive (code in a previous comment) I wrote is free for anyone to use, as and when the need arises (which I have a feeling will be soon), I will put together something more generic. I might separate my library out and push it into a public github repo for use in the next few days as I will enhance it going forwards. |
Mono will work fine. There is a datafetcher that does the mono.toFuture conversion anyway. My entire backend is reactive and heavily integrated with reactive kafka, so Fluxes and monos are a must for me. |
|
Had to maintain parity between REST and GraphQL along with expression support of spring. Neither a clean nor efficient. This workaround can be removed,
@Retention(AnnotationRetention.RUNTIME)
@Target(AnnotationTarget.FUNCTION)
annotation class Authorize(val value: String)@Aspect
class AuthorizationAspect {
private var attributeSource: MethodSecurityMetadataSource
private var preInvocationAdvice: PreInvocationAuthorizationAdvice
private var postAdvice: PostInvocationAuthorizationAdvice
private var attributeFactory: ExpressionBasedAnnotationAttributeFactory
constructor(attributeSource: MethodSecurityMetadataSource, preInvocationAdvice: PreInvocationAuthorizationAdvice,
postInvocationAdvice: PostInvocationAuthorizationAdvice,
attributeFactory: ExpressionBasedAnnotationAttributeFactory) {
this.attributeSource = attributeSource
this.preInvocationAdvice = preInvocationAdvice
this.postAdvice = postInvocationAdvice
this.attributeFactory = attributeFactory
}
@Around("@annotation(Authorize)")
@Throws(Throwable::class)
@SuppressWarnings
fun checkAuthorization(joinPoint: ProceedingJoinPoint): Any? {
val method: Method = (joinPoint.signature as MethodSignature).method
val methodInvocation = ReflectionUtils.findField(joinPoint::class.java,"methodInvocation")?.let {
ReflectionUtils.makeAccessible(it)
it.get(joinPoint) as MethodInvocation
}
val authAttributeValue = method.getAnnotation(Authorize::class.java).value
val myContext = (joinPoint.args[0] as MyGraphQLContext)
val authentication = myContext.securityContext.authentication
val preAttr = attributeFactory.createPreInvocationAttribute(null,null,authAttributeValue)
val output = preInvocationAdvice.before(authentication, methodInvocation, preAttr) ?: false
if(output){
return joinPoint.proceed()
}
else{
throw AccessDeniedException("Denied")
}
}
}class MyGraphQLContext(val securityContext: SecurityContext, val userName: String) : GraphQLContextMyGraphQLContextFactory Normalizes REST and Graphql at this point and injects MyGraphQLContext class. @Component
class MyGraphQLContextFactory : GraphQLContextFactory<MyGraphQLContext>, HandlerMethodArgumentResolver {
private fun extractUserSeqIdFromJwtToken(context: Mono<SecurityContext>): Mono<User> {
return context.filter { c: SecurityContext -> Objects.nonNull(c.authentication) }
.map { s: SecurityContext -> s.authentication.principal }
.cast(User::class.java) // Cast to your own object
}
override suspend fun generateContext(request: ServerHttpRequest, response: ServerHttpResponse): MyGraphQLContext {
return getGraphQLContext()
}
override fun supportsParameter(parameter: MethodParameter): Boolean {
return parameter.parameterType == MyGraphQLContext::class.java
}
override fun resolveArgument(parameter: MethodParameter, bindingContext: BindingContext, exchange: ServerWebExchange): Mono<Any> {
return mono {
getGraphQLContext()
}
}
private suspend fun getGraphQLContext() : MyGraphQLContext {
val context = ReactiveSecurityContextHolder.getContext()
val data = extractUserSeqIdFromJwtToken(context).awaitSingle()
return MyGraphQLContext(securityContext = context.awaitSingle(), userName = data.username)
}
}@Bean
fun authorizationAspectProvider(source: AbstractMethodSecurityMetadataSource, handler: MethodSecurityExpressionHandler): AuthorizationAspect {
val postAdvice = ExpressionBasedPostInvocationAdvice(
handler)
val preAdvice = ExpressionBasedPreInvocationAdvice()
preAdvice.setExpressionHandler(handler)
val attributeFactory = ExpressionBasedAnnotationAttributeFactory(handler)
return AuthorizationAspect(source, preAdvice, postAdvice, attributeFactory)
}Usage //@PreAuthorize("hasRole('ADMIN')")
@PostMapping("users")
@Authorize("hasRole('ADMIN')") // Instead of PreAuthorize; Spring security expressions are supported.
suspend fun createUser(context: MyGraphQLContext, @RequestBody createUser: CreateUser): User {
return Mono.just(User(createUser.userId, createUser.displayName, createUser.email, createUser.phoneNumber, createUser.groups)).awaitFirst()
} |
Great work. Not a fan of aspects (too much magic for me). Have you managed to bind the security errors into the graphql responses? The more I read on the subject, the more this is a must. I've started making a point of exposing the error directly into the response and maintaining the horrendous 200 OK that GraphQL insists on, when I get more time I'm going to ensure it gets included on query by query basis, so that there isn't just one big generic error. |
|
@thunderbird Custom aspect (via AspectJ) can be removed, provided suspend function support is not required. But allow spring bean override would be required. Mono -> Future conversion is required, refer code from @thunderbird @Bean
fun securityMethodInterceptor(source: AbstractMethodSecurityMetadataSource, handler: MethodSecurityExpressionHandler): PrePostAdviceReactiveMethodInterceptor {
val postAdvice = ExpressionBasedPostInvocationAdvice(
handler)
val preAdvice = ExpressionBasedPreInvocationAdvice()
preAdvice.setExpressionHandler(handler)
val attributeFactory = ExpressionBasedAnnotationAttributeFactory(handler)
return PrePostGraphqlReactiveMethodInterceptor(source, preAdvice, postAdvice, attributeFactory)
}Replica from PrePostAdviceReactiveMethodInterceptor except for context. Few protected methods could have avoided duplicated code. class PrePostGraphqlReactiveMethodInterceptor : PrePostAdviceReactiveMethodInterceptor {
private val anonymous: Authentication = AnonymousAuthenticationToken("key", "anonymous",
AuthorityUtils.createAuthorityList("ROLE_ANONYMOUS"))
private var attributeSource: MethodSecurityMetadataSource
private var preInvocationAdvice: PreInvocationAuthorizationAdvice
private var postAdvice: PostInvocationAuthorizationAdvice
private var attributeFactory: ExpressionBasedAnnotationAttributeFactory
constructor(attributeSource: MethodSecurityMetadataSource, preInvocationAdvice: PreInvocationAuthorizationAdvice,
postInvocationAdvice: PostInvocationAuthorizationAdvice,
attributeFactory: ExpressionBasedAnnotationAttributeFactory) : super(attributeSource,preInvocationAdvice,postInvocationAdvice) {
this.attributeSource = attributeSource
this.preInvocationAdvice = preInvocationAdvice
this.postAdvice = postInvocationAdvice
this.attributeFactory = attributeFactory
}
override fun invoke(invocation: MethodInvocation?): Any {
val method = invocation!!.method
val reflectiveMethodInvocation = invocation as ReflectiveMethodInvocation
// Read context from input args.
val graphQLContext = reflectiveMethodInvocation.arguments[0] as MyGraphQLContext
val returnType = method.returnType
check(Publisher::class.java.isAssignableFrom(returnType)) { "The returnType $returnType on $method must return an instance of org.reactivestreams.Publisher (i.e. Mono / Flux) in order to support Reactor Context" }
val targetClass: Class<*> = invocation!!.getThis()!!.javaClass
val attributes = attributeSource
.getAttributes(method, targetClass)
val preAttr = findPreInvocationAttribute(attributes)
// Modified portion start
val toInvoke = graphQLContext.securityContext
.map { obj: SecurityContext -> obj.authentication }
.defaultIfEmpty(this.anonymous)
.filter { auth: Authentication? -> preInvocationAdvice.before(auth, invocation, preAttr) }
.switchIfEmpty {
throw AccessDeniedException("")
} // Modified portion end
val attr = findPostInvocationAttribute(attributes)
if (Mono::class.java.isAssignableFrom(returnType)) {
return toInvoke
.flatMap { auth: Authentication? ->
proceed<Mono<*>>(invocation!!)
.map { r: Any? -> if (attr == null) r else postAdvice.after(auth, invocation, attr, r) }
}
}
return if (Flux::class.java.isAssignableFrom(returnType)) {
toInvoke
.flatMapMany { auth: Authentication? ->
proceed<Flux<*>>(invocation!!)
.map { r: Any? -> if (attr == null) r else postAdvice.after(auth, invocation, attr, r) }
}
} else toInvoke
.flatMapMany { auth: Authentication? ->
Flux.from(proceed<Publisher<*>>(invocation!!))
.map { r: Any? -> if (attr == null) r else postAdvice.after(auth, invocation, attr, r) }
}
}
private fun <T : Publisher<*>?> proceed(invocation: MethodInvocation): T {
return try {
invocation.proceed() as T
} catch (throwable: Throwable) {
throw Exceptions.propagate(throwable)
}
}
private fun findPostInvocationAttribute(
config: Collection<ConfigAttribute>): PostInvocationAttribute? {
for (attribute in config) {
if (attribute is PostInvocationAttribute) {
return attribute
}
}
return null
}
private fun findPreInvocationAttribute(
config: Collection<ConfigAttribute>): PreInvocationAttribute? {
for (attribute in config) {
if (attribute is PreInvocationAttribute) {
return attribute
}
}
return null
}
}You can use the original annotation, in this approach. @PreAuthorize("isAuthenticated()")
@PostMapping("users")
//@Authorize("hasRole('ADMIN')")
fun createUser(context: MyGraphQLContext, @RequestBody createUser: CreateUser): Mono<User> {
return Mono.just(User(createUser.userId, createUser.displayName, createUser.email, createUser.phoneNumber, createUser.groups))
}I think, @PreAuthorize is still an aspect via spring-aop |
|
Hi, thanks for providing the examples, it is indeed very helpful. Regarding the code from @thunderbird do I understand correctly, the Now I annotated a field on the type the query returns, like follows: Expected behaviour would be that a user without the role 'Manager' could not retrieve the field 'secret'. EDIT: One approach that works for me is to go the same way on the PropertyDataFetcherFactory: |
So I've been hammering away at this for a day and I can't seem to get even basic queries to work with mono or flux.
Below is an example of a query to a user profile from the identity store. If I convert this into a future it works out of the box, but the moment I use a mono i get all my fields as null
{"data":{"currentUserProfile":{"firstname":null,"lastname":null,"username":null,"emailAddress":null}}}And the query code:
I don't seem to have a problem generating schema with mono. I have registered a monad hook, but it seems to do nothing.
Is there something I'm doing wrong? I know it does call the query method, but I can't seem to figure out I get a null response.
The text was updated successfully, but these errors were encountered: