From 5c46fcbba02acd63dcf4355a6e81181d9c90a1c0 Mon Sep 17 00:00:00 2001 From: Philip Starritt Date: Sat, 17 Apr 2021 17:34:05 +0200 Subject: [PATCH 1/5] Configure subscription pre-auth security --- .../AuthenticationConnectionListener.java | 54 +++++++++++++++++++ ...uthoritiesAuthenticationDetailsSource.java | 16 +----- .../security/GrantedAuthorityFactory.java | 26 +++++++++ .../security/GraphQLSecurityConfig.java | 2 +- .../bank/mutation/BankAccountMutation.java | 26 +++++---- .../subscription/BankAccountSubscription.java | 10 ++++ 6 files changed, 104 insertions(+), 30 deletions(-) create mode 100644 src/main/java/com/learn/graphql/config/security/AuthenticationConnectionListener.java create mode 100644 src/main/java/com/learn/graphql/config/security/GrantedAuthorityFactory.java diff --git a/src/main/java/com/learn/graphql/config/security/AuthenticationConnectionListener.java b/src/main/java/com/learn/graphql/config/security/AuthenticationConnectionListener.java new file mode 100644 index 0000000..94db3e5 --- /dev/null +++ b/src/main/java/com/learn/graphql/config/security/AuthenticationConnectionListener.java @@ -0,0 +1,54 @@ +package com.learn.graphql.config.security; + +import graphql.kickstart.execution.subscriptions.SubscriptionSession; +import graphql.kickstart.execution.subscriptions.apollo.ApolloSubscriptionConnectionListener; +import graphql.kickstart.execution.subscriptions.apollo.OperationMessage; +import java.util.Map; +import lombok.extern.slf4j.Slf4j; +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.security.web.authentication.preauth.PreAuthenticatedAuthenticationToken; +import org.springframework.stereotype.Component; + +@Slf4j +@Component +public class AuthenticationConnectionListener implements ApolloSubscriptionConnectionListener { + + /** + * Chapter 33: Subscriptions Spring Security Pre-Auth When using pre-auth, you must ensure that + * all the graphql requests have been previously authorized/authenticated by an upstream service. + * For example, all ingress traffic to this graphql server must bypass an upstream proxy node that + * will validate the request's JWT token. This code alone performs no authorization. Read more + * about Pre-auth before using this. + */ + @Override + public void onConnect(SubscriptionSession session, OperationMessage message) { + log.info("onConnect with payload {}", message.getPayload()); + + var payload = (Map) message.getPayload(); + + // Get the user id, roles (or JWT etc) + var userId = payload.get(GraphQLSecurityConfig.USER_ID_PRE_AUTH_HEADER); + var userRoles = payload.get(GraphQLSecurityConfig.USER_ROLES_PRE_AUTH_HEADER); + var grantedAuthorities = GrantedAuthorityFactory.getAuthoritiesFrom(userRoles); + + var token = new PreAuthenticatedAuthenticationToken(userId, null, grantedAuthorities); + SecurityContextHolder.getContext().setAuthentication(token); + } + + @Override + public void onStart(SubscriptionSession session, OperationMessage message) { + log.info("onStart with payload {}", message.getPayload()); + } + + @Override + public void onStop(SubscriptionSession session, OperationMessage message) { + log.info("onStop with payload {}", message.getPayload()); + // Don't clear the context as this callback is executing on a different thread than onConnect/onStart + } + + @Override + public void onTerminate(SubscriptionSession session, OperationMessage message) { + log.info("onTerminate with payload {}", message.getPayload()); + } + +} diff --git a/src/main/java/com/learn/graphql/config/security/GrantedAuthoritiesAuthenticationDetailsSource.java b/src/main/java/com/learn/graphql/config/security/GrantedAuthoritiesAuthenticationDetailsSource.java index 6d453a6..eca5d3a 100644 --- a/src/main/java/com/learn/graphql/config/security/GrantedAuthoritiesAuthenticationDetailsSource.java +++ b/src/main/java/com/learn/graphql/config/security/GrantedAuthoritiesAuthenticationDetailsSource.java @@ -2,14 +2,8 @@ import static com.learn.graphql.config.security.GraphQLSecurityConfig.USER_ROLES_PRE_AUTH_HEADER; -import java.util.List; -import java.util.Set; -import java.util.stream.Collectors; import javax.servlet.http.HttpServletRequest; -import org.apache.commons.lang3.StringUtils; import org.springframework.security.authentication.AuthenticationDetailsSource; -import org.springframework.security.core.GrantedAuthority; -import org.springframework.security.core.authority.SimpleGrantedAuthority; import org.springframework.security.web.authentication.preauth.PreAuthenticatedGrantedAuthoritiesWebAuthenticationDetails; public class GrantedAuthoritiesAuthenticationDetailsSource implements @@ -19,16 +13,8 @@ public class GrantedAuthoritiesAuthenticationDetailsSource implements public PreAuthenticatedGrantedAuthoritiesWebAuthenticationDetails buildDetails( HttpServletRequest request) { var userRoles = request.getHeader(USER_ROLES_PRE_AUTH_HEADER); - var authorities = StringUtils.isBlank(userRoles) ? List.of() : - getAuthorities(userRoles); + var authorities = GrantedAuthorityFactory.getAuthoritiesFrom(userRoles); return new PreAuthenticatedGrantedAuthoritiesWebAuthenticationDetails(request, authorities); } - private List getAuthorities(String userRoles) { - return Set.of(userRoles.split(",")) - .stream() - .map(SimpleGrantedAuthority::new) - .collect(Collectors.toList()); - } - } diff --git a/src/main/java/com/learn/graphql/config/security/GrantedAuthorityFactory.java b/src/main/java/com/learn/graphql/config/security/GrantedAuthorityFactory.java new file mode 100644 index 0000000..45e8eb7 --- /dev/null +++ b/src/main/java/com/learn/graphql/config/security/GrantedAuthorityFactory.java @@ -0,0 +1,26 @@ +package com.learn.graphql.config.security; + +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; +import lombok.AccessLevel; +import lombok.NoArgsConstructor; +import org.apache.commons.lang3.StringUtils; +import org.springframework.security.core.GrantedAuthority; +import org.springframework.security.core.authority.SimpleGrantedAuthority; + +@NoArgsConstructor(access = AccessLevel.PRIVATE) +public class GrantedAuthorityFactory { + + public static List getAuthoritiesFrom(String userRoles) { + if (StringUtils.isBlank(userRoles)) { + return List.of(); + } + + return Set.of(userRoles.split(",")) + .stream() + .map(SimpleGrantedAuthority::new) + .collect(Collectors.toList()); + } + +} diff --git a/src/main/java/com/learn/graphql/config/security/GraphQLSecurityConfig.java b/src/main/java/com/learn/graphql/config/security/GraphQLSecurityConfig.java index 09c9f48..4f58939 100644 --- a/src/main/java/com/learn/graphql/config/security/GraphQLSecurityConfig.java +++ b/src/main/java/com/learn/graphql/config/security/GraphQLSecurityConfig.java @@ -77,7 +77,7 @@ public void configure(WebSecurity web) { .antMatchers("/actuator/health") // Permit playground for development .antMatchers("/playground", "/vendor/playground/**") - // Disable security for subscription example + // Subscription are secured via AuthenticationConnectionListener .antMatchers("/subscriptions"); } diff --git a/src/main/java/com/learn/graphql/resolver/bank/mutation/BankAccountMutation.java b/src/main/java/com/learn/graphql/resolver/bank/mutation/BankAccountMutation.java index cec9ec0..9a1cf87 100644 --- a/src/main/java/com/learn/graphql/resolver/bank/mutation/BankAccountMutation.java +++ b/src/main/java/com/learn/graphql/resolver/bank/mutation/BankAccountMutation.java @@ -29,9 +29,20 @@ public class BankAccountMutation implements GraphQLMutationResolver { */ public BankAccount createBankAccount(@Valid CreateBankAccountInput input) { log.info("Creating bank account for {}", input); + return getBankAccount(UUID.randomUUID()); + } + + /** + * Schema Directive Validation (Chapter 32) + */ + public BankAccount updateBankAccount(UUID id, String name, int age) { + log.info("Updating bank account for {}. Name: {}, age: {}", id, name, age); + return getBankAccount(id); + } + private BankAccount getBankAccount(UUID id) { var bankAccount = BankAccount.builder() - .id(UUID.randomUUID()) + .id(id) .currency(Currency.USD) .createdAt(ZonedDateTime.now(clock)) .createdOn(LocalDate.now(clock)) @@ -45,17 +56,4 @@ public BankAccount createBankAccount(@Valid CreateBankAccountInput input) { return bankAccount; } - /** - * Schema Directive Validation (Chapter 32) - */ - public BankAccount updateBankAccount(UUID id, String name, int age) { - log.info("Updating bank account for {}. Name: {}, age: {}", id, name, age); - return BankAccount.builder() - .id(UUID.randomUUID()) - .currency(Currency.USD) - .createdAt(ZonedDateTime.now(clock)) - .createdOn(LocalDate.now(clock)) - .build(); - } - } diff --git a/src/main/java/com/learn/graphql/resolver/bank/subscription/BankAccountSubscription.java b/src/main/java/com/learn/graphql/resolver/bank/subscription/BankAccountSubscription.java index f132333..80b0fe5 100644 --- a/src/main/java/com/learn/graphql/resolver/bank/subscription/BankAccountSubscription.java +++ b/src/main/java/com/learn/graphql/resolver/bank/subscription/BankAccountSubscription.java @@ -5,23 +5,33 @@ import graphql.kickstart.tools.GraphQLSubscriptionResolver; import java.util.UUID; import lombok.RequiredArgsConstructor; +import lombok.extern.slf4j.Slf4j; import org.reactivestreams.Publisher; +import org.springframework.security.access.prepost.PreAuthorize; +import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.stereotype.Component; /** * Subscription (Chapter 33) */ +@Slf4j @Component @RequiredArgsConstructor public class BankAccountSubscription implements GraphQLSubscriptionResolver { private final BankAccountPublisher bankAccountPublisher; + @PreAuthorize("hasAuthority('get:bank_account')") public Publisher bankAccounts() { return bankAccountPublisher.getBankAccountPublisher(); } + @PreAuthorize("hasAuthority('get:bank_account')") public Publisher bankAccount(UUID id) { + // Access the user id, user roles etc + var context = SecurityContextHolder.getContext(); + log.info("Creating bank account publisher for {}", context.getAuthentication().getPrincipal()); + return bankAccountPublisher.getBankAccountPublisherFor(id); } From ec396d114a7f042623f7862b378ad9c0dddbb00d Mon Sep 17 00:00:00 2001 From: Philip Starritt Date: Sat, 17 Apr 2021 18:24:51 +0200 Subject: [PATCH 2/5] version 34 --- .../config/security/AuthenticationConnectionListener.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/learn/graphql/config/security/AuthenticationConnectionListener.java b/src/main/java/com/learn/graphql/config/security/AuthenticationConnectionListener.java index 94db3e5..3b637a3 100644 --- a/src/main/java/com/learn/graphql/config/security/AuthenticationConnectionListener.java +++ b/src/main/java/com/learn/graphql/config/security/AuthenticationConnectionListener.java @@ -14,7 +14,7 @@ public class AuthenticationConnectionListener implements ApolloSubscriptionConnectionListener { /** - * Chapter 33: Subscriptions Spring Security Pre-Auth When using pre-auth, you must ensure that + * Chapter 34: Subscriptions Spring Security Pre-Auth When using pre-auth, you must ensure that * all the graphql requests have been previously authorized/authenticated by an upstream service. * For example, all ingress traffic to this graphql server must bypass an upstream proxy node that * will validate the request's JWT token. This code alone performs no authorization. Read more From de5ee068a8935fa2bb3d9286fe37fb118c5a8342 Mon Sep 17 00:00:00 2001 From: Philip Starritt Date: Sun, 18 Apr 2021 10:42:32 +0200 Subject: [PATCH 3/5] Subscription method level auth --- .../AuthenticationConnectionListener.java | 37 ++++++++++++++++--- .../RequestLoggingInstrumentation.java | 6 +-- .../subscription/BankAccountSubscription.java | 18 ++++++--- 3 files changed, 48 insertions(+), 13 deletions(-) diff --git a/src/main/java/com/learn/graphql/config/security/AuthenticationConnectionListener.java b/src/main/java/com/learn/graphql/config/security/AuthenticationConnectionListener.java index 3b637a3..27d2515 100644 --- a/src/main/java/com/learn/graphql/config/security/AuthenticationConnectionListener.java +++ b/src/main/java/com/learn/graphql/config/security/AuthenticationConnectionListener.java @@ -5,6 +5,7 @@ import graphql.kickstart.execution.subscriptions.apollo.OperationMessage; import java.util.Map; import lombok.extern.slf4j.Slf4j; +import org.springframework.security.core.Authentication; import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.security.web.authentication.preauth.PreAuthenticatedAuthenticationToken; import org.springframework.stereotype.Component; @@ -13,8 +14,10 @@ @Component public class AuthenticationConnectionListener implements ApolloSubscriptionConnectionListener { + public static final String AUTHENTICATION = "AUTHENTICATION"; + /** - * Chapter 34: Subscriptions Spring Security Pre-Auth When using pre-auth, you must ensure that + * Chapter 34: Subscriptions Spring Security Pre-Auth. When using pre-auth, you must ensure that * all the graphql requests have been previously authorized/authenticated by an upstream service. * For example, all ingress traffic to this graphql server must bypass an upstream proxy node that * will validate the request's JWT token. This code alone performs no authorization. Read more @@ -22,28 +25,52 @@ public class AuthenticationConnectionListener implements ApolloSubscriptionConne */ @Override public void onConnect(SubscriptionSession session, OperationMessage message) { - log.info("onConnect with payload {}", message.getPayload()); + log.info("onConnect with payload {}");//, message.getPayload()); var payload = (Map) message.getPayload(); - // Get the user id, roles (or JWT etc) + // Get the user id, roles (or JWT etc) and perform authentication / rejection here var userId = payload.get(GraphQLSecurityConfig.USER_ID_PRE_AUTH_HEADER); var userRoles = payload.get(GraphQLSecurityConfig.USER_ROLES_PRE_AUTH_HEADER); var grantedAuthorities = GrantedAuthorityFactory.getAuthoritiesFrom(userRoles); + /** + + Q: Why do not set the token/Authentication inside Spring Security SecurityContextHolder here? + + If the start frame is not sent directly with the connection_init then the two frames may be serviced on different threads. + The thread servicing the connection_init frame will check the websocket for any further inbound frames, + if false the thread will move onto another websocket. Another thread is then free to service the following start frame. + In this case, that thread not have the security context of the correct session/thread. + + Same scenario happens for onStop. (Message can be executed on different thread). + + This seems to be why some users are reporting intermittent failures with spring security. + E.g. https://github.com/graphql-java-kickstart/graphql-java-servlet/discussions/134#discussioncomment-225980 + + With the NIO connector, a small number of threads will check sessions for new frames. + If a session has a frame available, the session will be passed to another thread pool which will read frame, execute it, check for another frame, execute it (loop). + The session will be released when there are no further frames available. With this, we know that at most one thread will concurrently access one socket, + therefore frames will be read sequentially. We can therefore extract the auth credentials from onConnect and add them to the session.getUserProperties(). + These properties are available in the onStart and onStop callbacks. Inside these callbacks, we can add the token to the SecurityContextHolder if we decide to use method level security, + or simply access the credentials inside the subscription resolver via DataFetchingEnvironment. + + */ + var token = new PreAuthenticatedAuthenticationToken(userId, null, grantedAuthorities); - SecurityContextHolder.getContext().setAuthentication(token); + session.getUserProperties().put(AUTHENTICATION, token); } @Override public void onStart(SubscriptionSession session, OperationMessage message) { log.info("onStart with payload {}", message.getPayload()); + var authentication = (Authentication) session.getUserProperties().get(AUTHENTICATION); + SecurityContextHolder.getContext().setAuthentication(authentication); } @Override public void onStop(SubscriptionSession session, OperationMessage message) { log.info("onStop with payload {}", message.getPayload()); - // Don't clear the context as this callback is executing on a different thread than onConnect/onStart } @Override diff --git a/src/main/java/com/learn/graphql/instrumentation/RequestLoggingInstrumentation.java b/src/main/java/com/learn/graphql/instrumentation/RequestLoggingInstrumentation.java index 3c74aac..2aa36a6 100644 --- a/src/main/java/com/learn/graphql/instrumentation/RequestLoggingInstrumentation.java +++ b/src/main/java/com/learn/graphql/instrumentation/RequestLoggingInstrumentation.java @@ -29,13 +29,13 @@ public InstrumentationContext beginExecution( // Add the correlation ID to the NIO thread MDC.put(CORRELATION_ID, parameters.getExecutionInput().getExecutionId().toString()); - log.info("Query: {} with variables: {}", parameters.getQuery(), parameters.getVariables()); + // log.info("Query: {} with variables: {}", parameters.getQuery(), parameters.getVariables()); return SimpleInstrumentationContext.whenCompleted((executionResult, throwable) -> { var duration = Duration.between(start, Instant.now(clock)); if (throwable == null) { - log.info("Completed successfully in: {}", duration); + // log.info("Completed successfully in: {}", duration); } else { - log.warn("Failed in: {}", duration, throwable); + // log.warn("Failed in: {}", duration, throwable); } // If we have async resolvers, this callback can occur in the thread-pool and not the NIO thread. // In that case, the LoggingListener will be used as a fallback to clear the NIO thread. diff --git a/src/main/java/com/learn/graphql/resolver/bank/subscription/BankAccountSubscription.java b/src/main/java/com/learn/graphql/resolver/bank/subscription/BankAccountSubscription.java index 80b0fe5..6de2d9d 100644 --- a/src/main/java/com/learn/graphql/resolver/bank/subscription/BankAccountSubscription.java +++ b/src/main/java/com/learn/graphql/resolver/bank/subscription/BankAccountSubscription.java @@ -1,14 +1,17 @@ package com.learn.graphql.resolver.bank.subscription; +import com.learn.graphql.config.security.AuthenticationConnectionListener; import com.learn.graphql.domain.bank.BankAccount; import com.learn.graphql.publisher.BankAccountPublisher; +import graphql.kickstart.servlet.context.GraphQLWebSocketContext; import graphql.kickstart.tools.GraphQLSubscriptionResolver; +import graphql.schema.DataFetchingEnvironment; import java.util.UUID; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.reactivestreams.Publisher; import org.springframework.security.access.prepost.PreAuthorize; -import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.security.core.Authentication; import org.springframework.stereotype.Component; /** @@ -27,12 +30,17 @@ public Publisher bankAccounts() { } @PreAuthorize("hasAuthority('get:bank_account')") - public Publisher bankAccount(UUID id) { - // Access the user id, user roles etc - var context = SecurityContextHolder.getContext(); - log.info("Creating bank account publisher for {}", context.getAuthentication().getPrincipal()); + public Publisher bankAccount(UUID id, DataFetchingEnvironment e) { + GraphQLWebSocketContext context = getContext(e); + var authentication = (Authentication) context.getSession().getUserProperties() + .get(AuthenticationConnectionListener.AUTHENTICATION); + log.info("Creating bank account publisher for {}", authentication.getPrincipal()); return bankAccountPublisher.getBankAccountPublisherFor(id); } + private T getContext(DataFetchingEnvironment e) { + return e.getContext(); + } + } From c6efee6a7c405d50f8bc26cec53242fb50c4fcba Mon Sep 17 00:00:00 2001 From: Philip Starritt Date: Sun, 18 Apr 2021 10:50:46 +0200 Subject: [PATCH 4/5] Add logging --- .../instrumentation/RequestLoggingInstrumentation.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/com/learn/graphql/instrumentation/RequestLoggingInstrumentation.java b/src/main/java/com/learn/graphql/instrumentation/RequestLoggingInstrumentation.java index 2aa36a6..3c74aac 100644 --- a/src/main/java/com/learn/graphql/instrumentation/RequestLoggingInstrumentation.java +++ b/src/main/java/com/learn/graphql/instrumentation/RequestLoggingInstrumentation.java @@ -29,13 +29,13 @@ public InstrumentationContext beginExecution( // Add the correlation ID to the NIO thread MDC.put(CORRELATION_ID, parameters.getExecutionInput().getExecutionId().toString()); - // log.info("Query: {} with variables: {}", parameters.getQuery(), parameters.getVariables()); + log.info("Query: {} with variables: {}", parameters.getQuery(), parameters.getVariables()); return SimpleInstrumentationContext.whenCompleted((executionResult, throwable) -> { var duration = Duration.between(start, Instant.now(clock)); if (throwable == null) { - // log.info("Completed successfully in: {}", duration); + log.info("Completed successfully in: {}", duration); } else { - // log.warn("Failed in: {}", duration, throwable); + log.warn("Failed in: {}", duration, throwable); } // If we have async resolvers, this callback can occur in the thread-pool and not the NIO thread. // In that case, the LoggingListener will be used as a fallback to clear the NIO thread. From 85fb5b43e7efdfa62af94a677f071971c2a1d840 Mon Sep 17 00:00:00 2001 From: Philip Starritt Date: Sun, 18 Apr 2021 12:22:59 +0200 Subject: [PATCH 5/5] comments --- .../config/security/AuthenticationConnectionListener.java | 2 +- .../bank/subscription/BankAccountSubscription.java | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/learn/graphql/config/security/AuthenticationConnectionListener.java b/src/main/java/com/learn/graphql/config/security/AuthenticationConnectionListener.java index 27d2515..e890dfe 100644 --- a/src/main/java/com/learn/graphql/config/security/AuthenticationConnectionListener.java +++ b/src/main/java/com/learn/graphql/config/security/AuthenticationConnectionListener.java @@ -25,7 +25,7 @@ public class AuthenticationConnectionListener implements ApolloSubscriptionConne */ @Override public void onConnect(SubscriptionSession session, OperationMessage message) { - log.info("onConnect with payload {}");//, message.getPayload()); + log.info("onConnect with payload {}", message.getPayload()); var payload = (Map) message.getPayload(); diff --git a/src/main/java/com/learn/graphql/resolver/bank/subscription/BankAccountSubscription.java b/src/main/java/com/learn/graphql/resolver/bank/subscription/BankAccountSubscription.java index 6de2d9d..68da882 100644 --- a/src/main/java/com/learn/graphql/resolver/bank/subscription/BankAccountSubscription.java +++ b/src/main/java/com/learn/graphql/resolver/bank/subscription/BankAccountSubscription.java @@ -12,6 +12,7 @@ import org.reactivestreams.Publisher; import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.security.core.Authentication; +import org.springframework.security.core.context.SecurityContextHolder; import org.springframework.stereotype.Component; /** @@ -31,10 +32,15 @@ public Publisher bankAccounts() { @PreAuthorize("hasAuthority('get:bank_account')") public Publisher bankAccount(UUID id, DataFetchingEnvironment e) { + log.info("Creating bank account publisher for user Id: {}", + SecurityContextHolder.getContext().getAuthentication().getPrincipal()); + + // As an alternative to spring-security, you can access the authentication via the DataFetchingEnvironment GraphQLWebSocketContext context = getContext(e); var authentication = (Authentication) context.getSession().getUserProperties() .get(AuthenticationConnectionListener.AUTHENTICATION); - log.info("Creating bank account publisher for {}", authentication.getPrincipal()); + log.info("Creating bank account publisher for user Id: {}", + authentication.getPrincipal()); return bankAccountPublisher.getBankAccountPublisherFor(id); }