[GitHub] geode pull request #596: GEODE-2920 - GEODE-2925: Finer Grained Security

[PurelyApplied]

Github user PurelyApplied closed the pull request at:

https://github.com/apache/geode/pull/596


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #596: GEODE-2920 - GEODE-2925: Finer Grained Security

[PurelyApplied]

Github user PurelyApplied commented on a diff in the pull request:

https://github.com/apache/geode/pull/596#discussion_r123832520
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ClientCommands.java
 ---
@@ -109,12 +107,10 @@ public Result listClient() {
   }
 
   String memberSeparator = ";  ";
-  Iterator> it = 
clientServerMap.entrySet().iterator();
 
-  while (it.hasNext()) {
-Map.Entry pairs = (Map.Entry) it.next();
-String client = (String) pairs.getKey();
-List servers = (List) pairs.getValue();
+  for (Entry pairs : clientServerMap.entrySet()) 
{
+String client = pairs.getKey();
+List servers = pairs.getValue();
 StringBuilder serverListForClient = new StringBuilder();
--- End diff --

Added in commit `b4bb08b`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #596: GEODE-2920 - GEODE-2925: Finer Grained Security

[PurelyApplied]

Github user PurelyApplied commented on a diff in the pull request:

https://github.com/apache/geode/pull/596#discussion_r123832527
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/FunctionCommands.java
 ---
@@ -130,31 +125,8 @@ public Result executeFunction(
 return result;
   }
 
-  if (onRegion != null && onMember != null && onGroups != null) {
-ErrorResultData errorResultData =
-
ResultBuilder.createErrorResultData().setErrorCode(ResultBuilder.ERRORCODE_DEFAULT)
-.addLine(CliStrings.EXECUTE_FUNCTION__MSG__OPTIONS);
-result = ResultBuilder.buildResult(errorResultData);
-return result;
-  } else if (onRegion != null && onMember != null) {
-ErrorResultData errorResultData =
-
ResultBuilder.createErrorResultData().setErrorCode(ResultBuilder.ERRORCODE_DEFAULT)
-.addLine(CliStrings.EXECUTE_FUNCTION__MSG__OPTIONS);
-result = ResultBuilder.buildResult(errorResultData);
-return result;
-  } else if (onMember != null && onGroups != null) {
-ErrorResultData errorResultData =
-
ResultBuilder.createErrorResultData().setErrorCode(ResultBuilder.ERRORCODE_DEFAULT)
-.addLine(CliStrings.EXECUTE_FUNCTION__MSG__OPTIONS);
-result = ResultBuilder.buildResult(errorResultData);
-return result;
-  } else if (onRegion != null && onGroups != null) {
-ErrorResultData errorResultData =
-
ResultBuilder.createErrorResultData().setErrorCode(ResultBuilder.ERRORCODE_DEFAULT)
-.addLine(CliStrings.EXECUTE_FUNCTION__MSG__OPTIONS);
-result = ResultBuilder.buildResult(errorResultData);
-return result;
-  } else if (onRegion != null && onMember != null && onGroups != null) 
{
+  if (isMoreThanOneIsTrue(onRegion != null, onMember != null, onGroups 
!= null)) {
--- End diff --

Good call.  Added in commit `b4bb08b`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #596: GEODE-2920 - GEODE-2925: Finer Grained Security

[PurelyApplied]

Github user PurelyApplied commented on a diff in the pull request:

https://github.com/apache/geode/pull/596#discussion_r123830383
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBean.java
 ---
@@ -148,7 +148,12 @@ public long getInitialImageTime() {
 
   @Override
   public int getInitialImagesInProgres() {
--- End diff --

This is our internal implementation of 
`GEODE/geode-core/src/main/java/org/apache/geode/management/MemberMXBean.java`. 
 I was hesitant to remove the typo'd version from the public-facing interface 
without at least one version with it `@Deprecated`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #596: GEODE-2920 - GEODE-2925: Finer Grained Security

[PurelyApplied]

Github user PurelyApplied commented on a diff in the pull request:

https://github.com/apache/geode/pull/596#discussion_r123829882
  
--- Diff: 
geode-core/src/test/java/org/apache/geode/management/internal/security/DiskStoreMXBeanSecurityJUnitTest.java
 ---
@@ -57,7 +56,48 @@ public void setUp() throws Exception {
   }
 
   @Test
-  @ConnectionConfiguration(user = "data-admin", password = "1234567")
+  @ConnectionConfiguration(user = "clusterRead", password = "clusterRead")
+  public void testClusterReadAccess() throws Exception {
+assertThatThrownBy(() -> 
bean.flush()).hasMessageContaining(TestCommand.diskManage.toString());
--- End diff --

Thank you regex.  Done and done in commit `97de2f3`.
For what it's worth / future reference, `s/\(\) -> 
(\w+)\.(\w+)\(\)\)/$1::$2\)` would make 3,232 test replacements, but hits no 
production files, so that's nice.  Maybe something to add to spotless down the 
line.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #596: GEODE-2920 - GEODE-2925: Finer Grained Security

[jaredjstewart]

Github user jaredjstewart commented on a diff in the pull request:

https://github.com/apache/geode/pull/596#discussion_r123822645
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/FunctionCommands.java
 ---
@@ -130,31 +125,8 @@ public Result executeFunction(
 return result;
   }
 
-  if (onRegion != null && onMember != null && onGroups != null) {
-ErrorResultData errorResultData =
-
ResultBuilder.createErrorResultData().setErrorCode(ResultBuilder.ERRORCODE_DEFAULT)
-.addLine(CliStrings.EXECUTE_FUNCTION__MSG__OPTIONS);
-result = ResultBuilder.buildResult(errorResultData);
-return result;
-  } else if (onRegion != null && onMember != null) {
-ErrorResultData errorResultData =
-
ResultBuilder.createErrorResultData().setErrorCode(ResultBuilder.ERRORCODE_DEFAULT)
-.addLine(CliStrings.EXECUTE_FUNCTION__MSG__OPTIONS);
-result = ResultBuilder.buildResult(errorResultData);
-return result;
-  } else if (onMember != null && onGroups != null) {
-ErrorResultData errorResultData =
-
ResultBuilder.createErrorResultData().setErrorCode(ResultBuilder.ERRORCODE_DEFAULT)
-.addLine(CliStrings.EXECUTE_FUNCTION__MSG__OPTIONS);
-result = ResultBuilder.buildResult(errorResultData);
-return result;
-  } else if (onRegion != null && onGroups != null) {
-ErrorResultData errorResultData =
-
ResultBuilder.createErrorResultData().setErrorCode(ResultBuilder.ERRORCODE_DEFAULT)
-.addLine(CliStrings.EXECUTE_FUNCTION__MSG__OPTIONS);
-result = ResultBuilder.buildResult(errorResultData);
-return result;
-  } else if (onRegion != null && onMember != null && onGroups != null) 
{
+  if (isMoreThanOneIsTrue(onRegion != null, onMember != null, onGroups 
!= null)) {
--- End diff --

I like the readability introduced by this `isMoreThanOneIsTrue` method.  It 
might make sense to take the abstraction one step further: 
```
if (isMoreThanOneNonNull(onRegion, onMember, onGroup) {
```

```
  private boolean isMoreThanOneNonNull(Object... values) {
return Stream.of(values).filter(Objects::nonNull).count() > 1;
  }
```




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #596: GEODE-2920 - GEODE-2925: Finer Grained Security

[jaredjstewart]

Github user jaredjstewart commented on a diff in the pull request:

https://github.com/apache/geode/pull/596#discussion_r123820699
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/management/internal/cli/commands/ClientCommands.java
 ---
@@ -109,12 +107,10 @@ public Result listClient() {
   }
 
   String memberSeparator = ";  ";
-  Iterator> it = 
clientServerMap.entrySet().iterator();
 
-  while (it.hasNext()) {
-Map.Entry pairs = (Map.Entry) it.next();
-String client = (String) pairs.getKey();
-List servers = (List) pairs.getValue();
+  for (Entry pairs : clientServerMap.entrySet()) 
{
+String client = pairs.getKey();
+List servers = pairs.getValue();
 StringBuilder serverListForClient = new StringBuilder();
--- End diff --

Since we're already cleaning up this section of the code, it might be worth 
considering replacing 114-123 with a one liner: 

```
String serverListForClient = 
servers.stream().collect(Collectors.joining(memberSeparator));
```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #596: GEODE-2920 - GEODE-2925: Finer Grained Security

[jaredjstewart]

Github user jaredjstewart commented on a diff in the pull request:

https://github.com/apache/geode/pull/596#discussion_r123819260
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/management/internal/beans/MemberMBean.java
 ---
@@ -148,7 +148,12 @@ public long getInitialImageTime() {
 
   @Override
   public int getInitialImagesInProgres() {
--- End diff --

We ended up with two copies of this method, one with the correct spelling 
and one with the incorrect spelling.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #596: GEODE-2920 - GEODE-2925: Finer Grained Security

[jaredjstewart]

Github user jaredjstewart commented on a diff in the pull request:

https://github.com/apache/geode/pull/596#discussion_r123818520
  
--- Diff: 
geode-core/src/test/java/org/apache/geode/management/internal/security/DiskStoreMXBeanSecurityJUnitTest.java
 ---
@@ -57,7 +56,48 @@ public void setUp() throws Exception {
   }
 
   @Test
-  @ConnectionConfiguration(user = "data-admin", password = "1234567")
+  @ConnectionConfiguration(user = "clusterRead", password = "clusterRead")
+  public void testClusterReadAccess() throws Exception {
+assertThatThrownBy(() -> 
bean.flush()).hasMessageContaining(TestCommand.diskManage.toString());
--- End diff --

A bunch of tests in this change set can be simplified by using method 
references in place of lambda expressions (when the lambdas take no parameters 
and invoke a no-arg method on the target):
```

assertThatThrownBy(bean::flush).hasMessageContaining(TestCommand.diskManage.toString());
```
in place of 
```
assertThatThrownBy(() -> 
bean.flush()).hasMessageContaining(TestCommand.diskManage.toString());
```



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #596: GEODE-2920 - GEODE-2925: Finer Grained Security

[jaredjstewart]

Github user jaredjstewart commented on a diff in the pull request:

https://github.com/apache/geode/pull/596#discussion_r123817037
  
--- Diff: 
geode-core/src/main/java/org/apache/geode/management/CacheServerMXBean.java ---
@@ -60,48 +61,48 @@
   /**
* Returns the port on which this CacheServer listens for clients.
*/
-  public int getPort();
+  int getPort();
--- End diff --

I think this should be ok actually, since CacheServerMXBean is an interface 
and not a class.  (So all methods will be public by default.)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] geode pull request #596: GEODE-2920 - GEODE-2925: Finer Grained Security

[PurelyApplied]

GitHub user PurelyApplied opened a pull request:

https://github.com/apache/geode/pull/596

GEODE-2920 - GEODE-2925: Finer Grained Security

Due to the size of this commit and for your convenience of review, I have 
not yet squashed my commits.  Do note that I have not individually tested each 
individual commit for stability and each internal commit is meant only for ease 
of review.

The commit message to be included in the final, squashed version of this PR 
is present in the `8fe19ca`... commit, and reproduced below.

TODO: 
[ ] Is your initial contribution a single, squashed commit?

-

This commit represents the actual Finer Grained Security changes.
GEODE-2920 - GEODE-2925: Migration of security from DATA:MANAGE
* DATA:MANAGE -> CLUSTER:MANAGE
*
* configure pdx
* import cluster-configuration
* LockServiceMXBean.becomeLockGrantor
*
* DATA:MANAGE -> CLUSTER:MANAGE:DISK
*
* compact disk-store
* create disk-store
* destroy disk-store
* revoke missing-disk-store
* DiskStoreMXBean.forceRoll
* DiskStoreMXBean.forceCompaction
* DiskStoreMXBean.flush
* DiskStoreMXBean.setDiskUsageWarningPercentage
* DiskStoreMXBean.setDiskUsageCriticalPercentage
* DistributedSystemMXBean.revokeMissingDiskStores
* MemberMXBean.compactAllDistStores
*
* DATA:MANAGE -> CLUSTER:MANAGE:GATEWAY
*
* create gateway-receiver
* create gateway-sender
* destroy gateway-sender
* load-balance gateway-sender
* pause gateway-sender
* resume gateway-sender
* start gateway-receiver
* start gateway-sender
* stop gateway-receiver
* stop gateway-sender
* GatewayReceiverMXBean.start
* GatewayReceiverMXBean.stop
* GatewaySenderMXBean.start
* GatewaySenderMXBean.stop
* GatewaySenderMXBean.pause
* GatewaySenderMXBean.resume
* GatewaySenderMXBean.rebalance
*
* DATA:MANAGE -> CLUSTER:MANAGE:JAR
*
* create async-event-queue (Requires CLUSTER:WRITE:DISK if persistent)
* destroy function
* undeploy
*
* DATA:MANAGE -> CLUSTER:MANAGE:QUERY
*
* clear defined indexes
* close durable-client
* close durable-cq
* create defined indexes
* stop continuous-query
* CacheServerMXBean.closeAllContinuousQuery
* CacheServerMXBean.closeContinuousQuery
* DistributedSystemMXBean.setQueryResultSetLimit
* DistributedSystemMXBean.setQueryCollectionsDepth
*
* DATA:READ -> CLUSTER:READ
*
* list region
*
* DATA:MANAGE -> [None]
*
* pdx rename
*
* DATA:READ -> DATA:READ and CLUSTER:WRITE:DISK
*
* backup disk-store
* DistributedSystemMXBean.backupAllMembers
*
* DATA:MANAGE:RegionName -> CLUSTER:MANAGE:QUERY
*
* create index
* create lucene index (also requires CLUSTER:WRITE:DISK)
* define index
* destroy lucene index
*
* DATA:MANAGE, DATA:WRITE, CLUSTER:MANAGE, and CLUSTER:WRITE -> 
CLUSTER:MANAGE:JAR
*
* deploy
*
* DATA:MANAGE or DATA:MANAGE:RegionName -> CLUSTER:MANAGE:QUERY
*
* destroy index
*
* CLUSTER:READ -> CLUSTER:READ:QUERY
*
* describe lucene index
* list index
* list lucene indexes
*
* DATA:WRITE -> DATA:READ:Region
*
* search lucene index
*
* DATA:MANAGE -> DATA:MANAGE and CLUSTER:WRITE:DISK if persistent
*
* create region

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/PurelyApplied/geode geode-2924

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/geode/pull/596.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #596


commit 8b14112094fd49bfc7638cc952f8d322d5bd50e7
Author: Patrick Rhomberg 
Date:   2017-06-21T17:51:32Z

For ease of viewing, this commit covers all necessary imports.

commit 8fe19ca6ff3e51aed601537afb8e23c3e85569a1
Author: Patrick Rhomberg 
Date:   2017-06-21T18:59:13Z

This commit represents the actual Finer Grained Security changes.

GEODE-2920 - GEODE-2925: Migration of security from DATA:MANAGE
* DATA:MANAGE -> CLUSTER:MANAGE
*
* configure pdx
* import cluster-configuration
* LockServiceMXBean.becomeLockGrantor
*
* DATA:MANAGE -> CLUSTER:MANAGE:DISK
*
* compact disk-store
* create disk-store
* destroy disk-store
* revoke missing-disk-store
* DiskStoreMXBean.forceRoll
* DiskStoreMXBean.forceCompaction
* DiskStoreMXBean.flush
* DiskStoreMXBean.setDiskUsageWarningPercentage
* DiskStoreMXBean.setDiskUsageCriticalPercentage
* DistributedSystemMXBean.revokeMissingDiskStores
* MemberMXBean.compactAllDistStores
*
* DATA:MANAGE -> CLUSTER:MANAGE:GATEWAY
*
*