You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by GitBox <gi...@apache.org> on 2020/04/21 00:14:28 UTC
[GitHub] [geode] demery-pivotal opened a new pull request #4977: GEODE-7851: Pulse refreshes expired access tokens
demery-pivotal opened a new pull request #4977:
URL: https://github.com/apache/geode/pull/4977
If a user's access token expires, Pulse attempts to refresh it. If the
refresh fails, Pulse logs the user out and redirects the browser to
/pulse/clusterLogout.
Changes in Repository:
- When OAuth is configured, before returning the user's cluster,
getCluster() checks whether the user's access token has expired.
- If the access token has expired, the repository attempts to refresh
it. If the refresh succeeds, the repository reconnects the user's
cluster to JMX and returns it.
- If the refresh fails, the repository disconnects the user's cluster
from JMX, removes the cluster from the repository, and throws an
authentication or authorization exception.
Changes in PulseController:
- If the service call throws an authentication or authorization
exception, PulseController. getPulseUpdate() returns a 401 status.
Changes in pulsescript/common.js:
- If a Pulse ajax call returns a 401 status, ajaxPost() redirects the
browser to /pulse/clusterLogout to log the user out and request
re-authorization.
Co-authored-by: Joris Melchior <jo...@gmail.com>
Co-authored-by: Dale Emery <de...@pivotal.io>
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [X] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [X] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [X] Is your initial contribution a single, squashed commit?
- [X] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #4977: GEODE-7851: Pulse refreshes expired access tokens
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #4977:
URL: https://github.com/apache/geode/pull/4977#discussion_r413160879
##########
File path: geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/Repository.java
##########
@@ -80,8 +88,95 @@ public Repository(OAuth2AuthorizedClientService authorizedClientService,
this.clusterFactory = clusterFactory;
}
+ /**
+ * this will return a cluster already connected to the geode jmx manager for the user in the
+ * request
+ * <p>
+ * But for multi-user connections to gemfireJMX, i.e pulse that uses gemfire integrated security,
+ * we will need to get the username from the context
+ */
+ public Cluster getCluster() {
+ Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
+ if (authentication == null) {
+ return null;
+ }
+
+ if (authentication instanceof OAuth2AuthenticationToken) {
+ return getClusterWithAuthenticationToken((OAuth2AuthenticationToken) authentication);
+ }
+
+ return getClusterWithUserNameAndPassword(authentication.getName(), null);
+ }
+
+ public Cluster getClusterWithUserNameAndPassword(String userName, String password) {
+ String[] credentials = {userName, password};
+ return getClusterWithCredentials(userName, credentials);
+ }
+
+ public Cluster getClusterWithCredentials(String userName, Object credentials) {
+ synchronized (clusterMap) {
+ Cluster cluster = clusterMap.get(userName);
+ if (cluster == null) {
+ logger.info(resourceBundle.getString("LOG_MSG_CREATE_NEW_THREAD") + " : " + userName);
+ cluster = clusterFactory.create(host, port, userName, resourceBundle, this);
+ // Assign name to thread created
+ cluster.setName(PulseConstants.APP_NAME + "-" + host + ":" + port + ":" + userName);
+ cluster.connectToGemFire(credentials);
+ if (cluster.isConnectedFlag()) {
+ clusterMap.put(userName, cluster);
+ }
+ }
+ return cluster;
+ }
+ }
+
+ /**
+ * Returns the cluster for the user associated with the given authentication. If the user's
+ * access token is expired, it is refreshed and the cluster is reconnected to JMX using the fresh
+ * token. If the refresh fails, the user's cluster is disconnected from JMX and removed from the
+ * repository.
+ */
+ private Cluster getClusterWithAuthenticationToken(OAuth2AuthenticationToken authentication) {
+ OAuth2AuthorizedClient authorizedClient = getAuthorizedClient(authentication);
+
+ if (isExpired(authorizedClient.getAccessToken())) {
Review comment:
wondering which flow is better. Using the following might get rid of the reconnectToGemfire method in `Cluster`.
```suggestion
String userName = authorizedClient.getPrincipalName();
if (isExpired(authorizedClient.getAccessToken())) {
logoutUser(userName);
authorizedClient = refreshExpiredClient(authentication, authorizedClient);
}
userName = authorizedClient.getPrincipalName();
String credentials = authorizedClient.getAccessToken().getTokenValue();
return getClusterWithCredentials(userName, credentials);
```
##########
File path: geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/Repository.java
##########
@@ -80,8 +88,95 @@ public Repository(OAuth2AuthorizedClientService authorizedClientService,
this.clusterFactory = clusterFactory;
}
+ /**
+ * this will return a cluster already connected to the geode jmx manager for the user in the
+ * request
+ * <p>
+ * But for multi-user connections to gemfireJMX, i.e pulse that uses gemfire integrated security,
+ * we will need to get the username from the context
+ */
+ public Cluster getCluster() {
+ Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
+ if (authentication == null) {
+ return null;
+ }
+
+ if (authentication instanceof OAuth2AuthenticationToken) {
+ return getClusterWithAuthenticationToken((OAuth2AuthenticationToken) authentication);
+ }
+
+ return getClusterWithUserNameAndPassword(authentication.getName(), null);
+ }
+
+ public Cluster getClusterWithUserNameAndPassword(String userName, String password) {
+ String[] credentials = {userName, password};
+ return getClusterWithCredentials(userName, credentials);
+ }
+
+ public Cluster getClusterWithCredentials(String userName, Object credentials) {
Review comment:
It would be a good idea to document the the Object `credentials` here is the object that will be put in the jmx environment for connection: jmx.remote.credentials
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #4977: GEODE-7851: Pulse refreshes expired access tokens
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #4977:
URL: https://github.com/apache/geode/pull/4977#discussion_r413403957
##########
File path: geode-pulse/build.gradle
##########
@@ -50,7 +50,9 @@ dependencies {
// Needed to fully use log4j instead of commons-logging.
implementation('org.apache.logging.log4j:log4j-jcl')
implementation('org.apache.logging.log4j:log4j-api')
-// implementation('org.apache.logging.log4j:log4j-core')
+ implementation('org.apache.logging.log4j:log4j-slf4j-impl') {
Review comment:
probably it would be good to do this in a separate PR so that we can backport this to support branches.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] demery-pivotal commented on a change in pull request #4977: GEODE-7851: Pulse refreshes expired access tokens
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on a change in pull request #4977:
URL: https://github.com/apache/geode/pull/4977#discussion_r413431615
##########
File path: geode-pulse/build.gradle
##########
@@ -50,7 +50,9 @@ dependencies {
// Needed to fully use log4j instead of commons-logging.
implementation('org.apache.logging.log4j:log4j-jcl')
implementation('org.apache.logging.log4j:log4j-api')
-// implementation('org.apache.logging.log4j:log4j-core')
+ implementation('org.apache.logging.log4j:log4j-slf4j-impl') {
Review comment:
Done: https://github.com/apache/geode/pull/4988
And I rebased this PR on that one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] demery-pivotal commented on a change in pull request #4977: GEODE-7851: Pulse refreshes expired access tokens
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on a change in pull request #4977:
URL: https://github.com/apache/geode/pull/4977#discussion_r413302656
##########
File path: geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/Repository.java
##########
@@ -80,8 +88,95 @@ public Repository(OAuth2AuthorizedClientService authorizedClientService,
this.clusterFactory = clusterFactory;
}
+ /**
+ * this will return a cluster already connected to the geode jmx manager for the user in the
+ * request
+ * <p>
+ * But for multi-user connections to gemfireJMX, i.e pulse that uses gemfire integrated security,
+ * we will need to get the username from the context
+ */
+ public Cluster getCluster() {
+ Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
+ if (authentication == null) {
+ return null;
+ }
+
+ if (authentication instanceof OAuth2AuthenticationToken) {
+ return getClusterWithAuthenticationToken((OAuth2AuthenticationToken) authentication);
+ }
+
+ return getClusterWithUserNameAndPassword(authentication.getName(), null);
+ }
+
+ public Cluster getClusterWithUserNameAndPassword(String userName, String password) {
+ String[] credentials = {userName, password};
+ return getClusterWithCredentials(userName, credentials);
+ }
+
+ public Cluster getClusterWithCredentials(String userName, Object credentials) {
+ synchronized (clusterMap) {
+ Cluster cluster = clusterMap.get(userName);
+ if (cluster == null) {
+ logger.info(resourceBundle.getString("LOG_MSG_CREATE_NEW_THREAD") + " : " + userName);
+ cluster = clusterFactory.create(host, port, userName, resourceBundle, this);
+ // Assign name to thread created
+ cluster.setName(PulseConstants.APP_NAME + "-" + host + ":" + port + ":" + userName);
+ cluster.connectToGemFire(credentials);
+ if (cluster.isConnectedFlag()) {
+ clusterMap.put(userName, cluster);
+ }
+ }
+ return cluster;
+ }
+ }
+
+ /**
+ * Returns the cluster for the user associated with the given authentication. If the user's
+ * access token is expired, it is refreshed and the cluster is reconnected to JMX using the fresh
+ * token. If the refresh fails, the user's cluster is disconnected from JMX and removed from the
+ * repository.
+ */
+ private Cluster getClusterWithAuthenticationToken(OAuth2AuthenticationToken authentication) {
+ OAuth2AuthorizedClient authorizedClient = getAuthorizedClient(authentication);
+
+ if (isExpired(authorizedClient.getAccessToken())) {
Review comment:
`logoutUser()` discards any data cached in the `Cluster`, including all of the trends stored in circular buffers. That seems like a harsh thing to do when the token refreshes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] demery-pivotal commented on a change in pull request #4977: GEODE-7851: Pulse refreshes expired access tokens
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on a change in pull request #4977:
URL: https://github.com/apache/geode/pull/4977#discussion_r413431474
##########
File path: geode-pulse/src/main/java/org/apache/geode/tools/pulse/internal/data/Repository.java
##########
@@ -80,8 +88,95 @@ public Repository(OAuth2AuthorizedClientService authorizedClientService,
this.clusterFactory = clusterFactory;
}
+ /**
+ * this will return a cluster already connected to the geode jmx manager for the user in the
+ * request
+ * <p>
+ * But for multi-user connections to gemfireJMX, i.e pulse that uses gemfire integrated security,
+ * we will need to get the username from the context
+ */
+ public Cluster getCluster() {
+ Authentication authentication = SecurityContextHolder.getContext().getAuthentication();
+ if (authentication == null) {
+ return null;
+ }
+
+ if (authentication instanceof OAuth2AuthenticationToken) {
+ return getClusterWithAuthenticationToken((OAuth2AuthenticationToken) authentication);
+ }
+
+ return getClusterWithUserNameAndPassword(authentication.getName(), null);
+ }
+
+ public Cluster getClusterWithUserNameAndPassword(String userName, String password) {
+ String[] credentials = {userName, password};
+ return getClusterWithCredentials(userName, credentials);
+ }
+
+ public Cluster getClusterWithCredentials(String userName, Object credentials) {
Review comment:
Done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] demery-pivotal commented on a change in pull request #4977: GEODE-7851: Pulse refreshes expired access tokens
Posted by GitBox <gi...@apache.org>.
demery-pivotal commented on a change in pull request #4977:
URL: https://github.com/apache/geode/pull/4977#discussion_r413425777
##########
File path: geode-pulse/build.gradle
##########
@@ -50,7 +50,9 @@ dependencies {
// Needed to fully use log4j instead of commons-logging.
implementation('org.apache.logging.log4j:log4j-jcl')
implementation('org.apache.logging.log4j:log4j-api')
-// implementation('org.apache.logging.log4j:log4j-core')
+ implementation('org.apache.logging.log4j:log4j-slf4j-impl') {
Review comment:
Okay. And probably it should be `runtimeOnly` instead of `implementation`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org