Netflix / dgs-framework

GraphQL for Java with Spring Boot made easy.
https://netflix.github.io/dgs
Apache License 2.0
3.08k stars 296 forks source link

bug: Reactive Security context is not available in subscriptions #1442

Closed sipe90 closed 2 months ago

sipe90 commented 1 year ago

I have a Spring Boot WebFlux application with Spring Security OAuth2 resource server authentication. When I try to access the Security Context within a subscription, null is returned instead of the context.

Spring Boot version is 3.0.2 and DGS Framework version is 6.0.0

Expected behavior

Security context (fetched with ReactiveSecurityContextHolder.getContext()) should not be null when invoked within a subscription method during an authenticated request.

Actual behavior

ReactiveSecurityContextHolder.getContext() returns null when invoked inside a subscription method.

Steps to reproduce

The security configuration looks like this:

@Configuration
@EnableWebFluxSecurity
class WebFluxSecurityConfiguration {

    @Bean
    fun springSecurityFilterChain(http: ServerHttpSecurity): SecurityWebFilterChain {
        val roles = arrayOf("role1", "role2")

        return http {
            authorizeExchange {
                authorize("/actuator/health", permitAll)
                authorize(anyExchange, hasAnyRole(*roles))
            }
            oauth2ResourceServer {
                val tokenConverter = ServerBearerTokenAuthenticationConverter()
                tokenConverter.setAllowUriQueryParameter(true) // This allows passing authentication token in a query parameter in websocket requests. eg. ws://localhost:8080/subscriptions?access_token=<token>
                bearerTokenConverter = tokenConverter
                jwt {
                    jwtAuthenticationConverter = CustomAuthenticationConverter()
                }
            }
        }
    }
}

And the subscription like this:

@DgsComponent
internal class EventsDataFetcher(private val eventService: EventService) {

    private val logger = KotlinLogging.logger {}

    @DgsSubscription(field = DgsConstants.SUBSCRIPTION.Events)
    fun events(): Flux<Event> =
        ReactiveSecurityContextHolder.getContext()
            .doOnSuccess { logger.info { it } }
            .thenMany(eventService.streamEvents())

When subscribing to the events, null is logged.

With queries, the request context can be accessed:

@DgsComponent
class UserDataFetcher(private val userService: UserService) {

    @DgsQuery
    fun currentUser(): Mono<User> {
        return ReactiveSecurityContextHolder.getContext()
            .map { it.authentication as CustomAuthentication }
            .map { auth ->
                User(
                    id = auth.principal.id,
                    firstName = auth.principal.firstName,
                    lastName = auth.principal.lastName,
                    roles = auth.authorities
                        .filterIsInstance<RoleAuthority>()
                        .map(RoleAuthority::role)
                )
            }
    }
}

I also tried creating a WebSocketHandler where I also successfully managed to access the security context.

srinivasankavitha commented 1 year ago

This should have been fixed with this PR: https://github.com/Netflix/dgs-framework/pull/1313

That said, we don't use webflux internally and there could be issues with using this feature in the webflux stack. I will do some more testing to validate this. Unfortunately, due to conflicting priorities, I won't be able to get to this in the next few weeks.

On Mon, Feb 27, 2023 at 4:02 AM Pasi @.***> wrote:

I have a Spring Boot WebFlux application with Spring Security OAuth2 resource server authentication. When I try to access the Security Context within a subscription, null is returned instead of the context.

Spring Boot version is 3.0.2 and DGS Framework version is 6.0.0 Expected behavior

Security context (fetched with ReactiveSecurityContextHolder.getContext()) should not be null when invoked within a subscription method during an authenticated request. Actual behavior

ReactiveSecurityContextHolder.getContext() returns null when invoked inside a subscription method. Steps to reproduce

The security configuration looks like this:

@Configuration @EnableWebFluxSecurityclass WebFluxSecurityConfiguration {

@Bean
fun springSecurityFilterChain(http: ServerHttpSecurity): SecurityWebFilterChain {
    val roles = arrayOf("role1", "role2")

    return http {
        authorizeExchange {
            authorize("/actuator/health", permitAll)
            authorize(anyExchange, hasAnyRole(*roles))
        }
        oauth2ResourceServer {
            val tokenConverter = ServerBearerTokenAuthenticationConverter()
            tokenConverter.setAllowUriQueryParameter(true) // This allows passing authentication token in a query parameter in websocket requests. eg. ws://localhost:8080/subscriptions?access_token=<token>
            bearerTokenConverter = tokenConverter
            jwt {
                jwtAuthenticationConverter = CustomAuthenticationConverter()
            }
        }
    }
}

}

And the subscription like this:

@DgsComponentinternal class EventsDataFetcher(private val eventService: EventService) {

private val logger = KotlinLogging.logger {}

@DgsSubscription(field = DgsConstants.SUBSCRIPTION.Events)
fun events(): Flux<Event> =
    ReactiveSecurityContextHolder.getContext()
        .doOnSuccess { logger.info { it } }
        .thenMany(eventService.streamEvents())

When subscribing to the events, null is logged.

With queries, the request context can be accessed:

@DgsComponentclass UserDataFetcher(private val userService: UserService) {

@DgsQuery
fun currentUser(): Mono<User> {
    return ReactiveSecurityContextHolder.getContext()
        .map { it.authentication as CustomAuthentication }
        .map { auth ->
            User(
                id = auth.principal.id,
                firstName = auth.principal.firstName,
                lastName = auth.principal.lastName,
                roles = auth.authorities
                    .filterIsInstance<RoleAuthority>()
                    .map(RoleAuthority::role)
            )
        }
}

}

I also tried creating a WebSocketHandler where I also successfully managed to access the security context.

— Reply to this email directly, view it on GitHub https://github.com/Netflix/dgs-framework/issues/1442, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJ5JPXNH345AJTQTSYOKS5DWZSJW3ANCNFSM6AAAAAAVJJ3K64 . You are receiving this because you are subscribed to this thread.Message ID: @.***>

sipe90 commented 1 year ago

That PR doesn't seem to affect the reactive websocket implementation, which is DgsReactiveWebsocketHandler from graphql-dgs-spring-webflux-autoconfigure.

sipe90 commented 1 year ago

I've managed to come up with a workaround using a custom context:

@Component
class DgsSecurityContextBuilder : DgsReactiveCustomContextBuilderWithRequest<CustomAuthentication> {
    override fun build(
        extensions: Map<String, Any>?,
        headers: HttpHeaders?,
        serverRequest: ServerRequest?
    ): Mono<CustomAuthentication> {
        return ReactiveSecurityContextHolder.getContext().map { it.authentication as CustomAuthentication }
    }
}
@DgsSubscription(field = DgsConstants.SUBSCRIPTION.Events)
fun events(dfe: DataFetchingEnvironment): Flux<Event> {
    val auth = DgsContext.getCustomContext<CustomAuthentication>(dfe)

    if (!auth.authorities.contains(ActionAuthority(Action.STREAM_EVENTS))) {
        return Flux.error(AccessDeniedException("Forbidden"))
    }

    return eventService.streamEvents()
}

I would still prefer to be able to access the authentication from Spring's context since I use Reactive Method Security with custom AuthorizationInterceptors and annotations.

This workaround should suffice for now, but I hope this issue gets fixed soon.

Mechwv commented 1 year ago

@sipe90 Thx for your solution, this workaround works fine for subscriptions (just tested it out myself)

srinivasankavitha commented 1 year ago

Thanks for posting an update with the workaround. We will put in a fix shortly for this issue.

On Tue, Feb 28, 2023 at 7:42 AM Mechwv @.***> wrote:

@sipe90 https://github.com/sipe90 Thx for your solution, this workaround works fine for subscriptions (just tested it out myself)

— Reply to this email directly, view it on GitHub https://github.com/Netflix/dgs-framework/issues/1442#issuecomment-1448405935, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJ5JPXM2XYEOK4UWJ7OG7NDWZYMFHANCNFSM6AAAAAAVJJ3K64 . You are receiving this because you commented.Message ID: @.***>

StringKe commented 1 year ago

@sipe90 Implementing DgsReactiveCustomContextBuilderWithRequest in webflux websocket without any data.

Have you encountered this problem?

lthoulon-locala commented 1 year ago

This looks like it's still an issue, right ? It seems linked to my question here: https://github.com/Netflix/dgs-framework/discussions/1644

lthoulon-locala commented 1 year ago

@sipe90 Implementing DgsReactiveCustomContextBuilderWithRequest in webflux websocket without any data.

Have you encountered this problem?

I got the same issue here. Workaround doesn't work for me. It doesn't seem logical that it would fix the problem either as it is trying to get the authentication from the context that is not filled. 🤷

lthoulon-locala commented 1 year ago

Actually the workaround works when the authorization header is provided on the handshake request.

srinivasankavitha commented 1 year ago

@lthoulon-locala - does setting the auth header in the handshake request unblock you?

lthoulon-locala commented 1 year ago

@srinivasankavitha Here is was I was able to do with the auth header in the handshake request:

@Component
class DgsSecurityContextBuilder : DgsReactiveCustomContextBuilderWithRequest<Authentication> {
    override fun build(
        extensions: Map<String, Any>?,
        headers: HttpHeaders?,
        serverRequest: ServerRequest?
    ): Mono<Authentication> = ReactiveSecurityContextHolder.getContext().map { it.authentication }
}

@DgsComponent
class AggregateSubscriptions(val handler: AggregateSubscriptionsHandler) {
    @DgsSubscription
    fun aggregateNotifications(
        @InputArgument id: UUID,
        @InputArgument notificationTypes: List<NotificationType>?,
        dfe: DataFetchingEnvironment
    ) = let {
        val auth = DgsContext.getCustomContext<Authentication>(dfe)
        handler.aggregateNotifications(id, notificationTypes, auth)
    }
}

@Component
class AggregateSubscriptionsHandler {
    // I put the 2 annotations that I tested but of course one should only use one at the time.
    @PreAuthorize("hasAuthority('$AGGREGATE_READ')") // Rincewind: THAT DOESN'T WORK
    @PreAuthorize("@myPermissionEvaluator.hasAuthority(#auth, '$AGGREGATE_READ')")
    fun aggregateNotifications(id: UUID, notificationTypes: List<NotificationType>?, auth: Authentication) =
        Flux.interval(Duration.ofMillis(1000))
            .map {
                Notification.Builder()
                    .withAggregateId(id.toString())
                    .withNotificationType(NotificationType.AdvertiserAdded)
                    .withPayload(listOf(KeyValue.Builder().withKey("t").withValue(it.toString()).build()))
                    .build()
            }
}

So yes, this unblocks me although it's not ideal because i'll have different code in my queries and subscriptions. Also it means I can't use GraphiQL to test my subscriptions as it sends the authz parameters as part of the connection_init message. Hopefully Postman's GraphQL client uses headers still.

Any chance we could get a fix soon on this matter ? Thanks

lthoulon-locala commented 1 year ago

Referencing this feature request: #450

srinivasankavitha commented 1 year ago

Thanks for the context @lthoulon-locala. As mentioned in the linked issue, we won't be able to prioritize this anytime soon, since Webflux stack is not internally used as much. We do accept contributions, in case you are interested or need this sooner.

paulbakker commented 2 months ago

WebFlux support is now coming from Spring GraphQL. See https://netflix.github.io/dgs/spring-graphql-integration/. Closing this issue since it's not actionable anymore.