You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Jinmei Liao <ji...@pivotal.io> on 2016/08/05 15:38:32 UTC
Review Request 50855: Geode-1569: require principal to be
Serializable so that post process can happen in a remote server
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50855/
-----------------------------------------------------------
Review request for geode, Grace Meilen, Kevin Duling, and Kirk Lund.
Repository: geode
Description
-------
Geode-1569: require principal to be Serializable so that post process can happen in a remote server
Diffs
-----
geode-core/src/main/java/com/gemstone/gemfire/internal/security/GeodeSecurityUtil.java a9c21629489251a336ad975edc28d221a089f7d1
geode-core/src/main/java/com/gemstone/gemfire/internal/security/IntegratedSecurityService.java 84a0c320728899fb4cf92e786cdec4b86380438b
geode-core/src/main/java/com/gemstone/gemfire/internal/security/SecurityService.java 9629ba3ad81aff24668565a3772ce334f794b242
geode-core/src/main/java/com/gemstone/gemfire/internal/security/shiro/CustomAuthRealm.java edb0790f871251798f3f53482acec98bc2ab0efd
geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/commands/DataCommands.java ae87b72018de414e0d37941f56f38d110fd34b94
geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/domain/DataCommandRequest.java 92d4579153017cdce3694007410352c30d4fdb2d
geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/functions/DataCommandFunction.java e6482c1b34349c3559c3596a0bef5522aa155523
geode-core/src/main/java/org/apache/geode/security/PostProcessor.java 1a0e5ded09b57407a254d06e31bdf37c25ae8971
geode-core/src/main/java/org/apache/geode/security/SecurityManager.java 273f2f139c20d478e896da31698f6da4aecaeeac
geode-core/src/main/java/org/apache/geode/security/templates/SamplePostProcessor.java ce87bf8ae45287125890767824036bed43a83510
geode-core/src/main/java/org/apache/geode/security/templates/SampleSecurityManager.java 83ac50e740f39cca9584dc3da9e905c62f33a7f7
geode-core/src/test/java/com/gemstone/gemfire/security/AbstractSecureServerDUnitTest.java 4f0e26032e1124f6ad7c831e8570347cd3e00a34
geode-core/src/test/java/com/gemstone/gemfire/security/NoShowValue1PostProcessor.java b42814423db5421d1b8007ae87171617c6c06421
geode-core/src/test/java/com/gemstone/gemfire/security/PDXGfshPostProcessorOnRemoteServerTest.java PRE-CREATION
geode-core/src/test/java/com/gemstone/gemfire/security/PDXPostProcessor.java 5609a21f94eb00edd0276cfd649355b0977174e1
geode-core/src/test/java/com/gemstone/gemfire/security/SpySecurityManager.java 6d04a0db7feae9f9af59d2e39c45435cd7a0c653
Diff: https://reviews.apache.org/r/50855/diff/
Testing
-------
Thanks,
Jinmei Liao
Re: Review Request 50855: Geode-1569: require principal to be
Serializable so that post process can happen in a remote server
Posted by Jinmei Liao <ji...@pivotal.io>.
On Aug. 5, 2016, 4:32 p.m., Jinmei Liao wrote:
> > In your comment you said that you wanted to require that the principal was Serializable. But did you also intend to not require it to be an instanceof Principal?
Yes, It does not need to be an instance of Principal. It could be a String identifying the user like what Shiro would return.
- Jinmei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50855/#review144943
-----------------------------------------------------------
On Aug. 5, 2016, 3:38 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50855/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2016, 3:38 p.m.)
>
>
> Review request for geode, Grace Meilen, Kevin Duling, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Geode-1569: require principal to be Serializable so that post process can happen in a remote server
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/security/GeodeSecurityUtil.java a9c21629489251a336ad975edc28d221a089f7d1
> geode-core/src/main/java/com/gemstone/gemfire/internal/security/IntegratedSecurityService.java 84a0c320728899fb4cf92e786cdec4b86380438b
> geode-core/src/main/java/com/gemstone/gemfire/internal/security/SecurityService.java 9629ba3ad81aff24668565a3772ce334f794b242
> geode-core/src/main/java/com/gemstone/gemfire/internal/security/shiro/CustomAuthRealm.java edb0790f871251798f3f53482acec98bc2ab0efd
> geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/commands/DataCommands.java ae87b72018de414e0d37941f56f38d110fd34b94
> geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/domain/DataCommandRequest.java 92d4579153017cdce3694007410352c30d4fdb2d
> geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/functions/DataCommandFunction.java e6482c1b34349c3559c3596a0bef5522aa155523
> geode-core/src/main/java/org/apache/geode/security/PostProcessor.java 1a0e5ded09b57407a254d06e31bdf37c25ae8971
> geode-core/src/main/java/org/apache/geode/security/SecurityManager.java 273f2f139c20d478e896da31698f6da4aecaeeac
> geode-core/src/main/java/org/apache/geode/security/templates/SamplePostProcessor.java ce87bf8ae45287125890767824036bed43a83510
> geode-core/src/main/java/org/apache/geode/security/templates/SampleSecurityManager.java 83ac50e740f39cca9584dc3da9e905c62f33a7f7
> geode-core/src/test/java/com/gemstone/gemfire/security/AbstractSecureServerDUnitTest.java 4f0e26032e1124f6ad7c831e8570347cd3e00a34
> geode-core/src/test/java/com/gemstone/gemfire/security/NoShowValue1PostProcessor.java b42814423db5421d1b8007ae87171617c6c06421
> geode-core/src/test/java/com/gemstone/gemfire/security/PDXGfshPostProcessorOnRemoteServerTest.java PRE-CREATION
> geode-core/src/test/java/com/gemstone/gemfire/security/PDXPostProcessor.java 5609a21f94eb00edd0276cfd649355b0977174e1
> geode-core/src/test/java/com/gemstone/gemfire/security/SpySecurityManager.java 6d04a0db7feae9f9af59d2e39c45435cd7a0c653
>
> Diff: https://reviews.apache.org/r/50855/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 50855: Geode-1569: require principal to be
Serializable so that post process can happen in a remote server
Posted by Darrel Schneider <ds...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50855/#review144943
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/security/PostProcessor.java (line 41)
<https://reviews.apache.org/r/50855/#comment211098>
typo: implemented
In your comment you said that you wanted to require that the principal was Serializable. But did you also intend to not require it to be an instanceof Principal?
- Darrel Schneider
On Aug. 5, 2016, 8:38 a.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50855/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2016, 8:38 a.m.)
>
>
> Review request for geode, Grace Meilen, Kevin Duling, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Geode-1569: require principal to be Serializable so that post process can happen in a remote server
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/security/GeodeSecurityUtil.java a9c21629489251a336ad975edc28d221a089f7d1
> geode-core/src/main/java/com/gemstone/gemfire/internal/security/IntegratedSecurityService.java 84a0c320728899fb4cf92e786cdec4b86380438b
> geode-core/src/main/java/com/gemstone/gemfire/internal/security/SecurityService.java 9629ba3ad81aff24668565a3772ce334f794b242
> geode-core/src/main/java/com/gemstone/gemfire/internal/security/shiro/CustomAuthRealm.java edb0790f871251798f3f53482acec98bc2ab0efd
> geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/commands/DataCommands.java ae87b72018de414e0d37941f56f38d110fd34b94
> geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/domain/DataCommandRequest.java 92d4579153017cdce3694007410352c30d4fdb2d
> geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/functions/DataCommandFunction.java e6482c1b34349c3559c3596a0bef5522aa155523
> geode-core/src/main/java/org/apache/geode/security/PostProcessor.java 1a0e5ded09b57407a254d06e31bdf37c25ae8971
> geode-core/src/main/java/org/apache/geode/security/SecurityManager.java 273f2f139c20d478e896da31698f6da4aecaeeac
> geode-core/src/main/java/org/apache/geode/security/templates/SamplePostProcessor.java ce87bf8ae45287125890767824036bed43a83510
> geode-core/src/main/java/org/apache/geode/security/templates/SampleSecurityManager.java 83ac50e740f39cca9584dc3da9e905c62f33a7f7
> geode-core/src/test/java/com/gemstone/gemfire/security/AbstractSecureServerDUnitTest.java 4f0e26032e1124f6ad7c831e8570347cd3e00a34
> geode-core/src/test/java/com/gemstone/gemfire/security/NoShowValue1PostProcessor.java b42814423db5421d1b8007ae87171617c6c06421
> geode-core/src/test/java/com/gemstone/gemfire/security/PDXGfshPostProcessorOnRemoteServerTest.java PRE-CREATION
> geode-core/src/test/java/com/gemstone/gemfire/security/PDXPostProcessor.java 5609a21f94eb00edd0276cfd649355b0977174e1
> geode-core/src/test/java/com/gemstone/gemfire/security/SpySecurityManager.java 6d04a0db7feae9f9af59d2e39c45435cd7a0c653
>
> Diff: https://reviews.apache.org/r/50855/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 50855: Geode-1569: require principal to be
Serializable so that post process can happen in a remote server
Posted by Jinmei Liao <ji...@pivotal.io>.
> On Aug. 5, 2016, 4:51 p.m., Kirk Lund wrote:
> > geode-core/src/main/java/org/apache/geode/security/PostProcessor.java, line 52
> > <https://reviews.apache.org/r/50855/diff/1/?file=1464417#file1464417line52>
> >
> > Don't we want this to be a new org.apache.geode.security.SerializablePrincipal?
> >
> > If not, then I'd restore it back to Principal and just modify the javadoc to say that the implementation of Principal must be Serializable.
It does not need to be a Principal object. Keep in mind the PostProcessor should also work if customer initialized security using shiro.ini configuration files. Shiro's Subject.getPrincipal() simply returns an Object. In the simples cases, it's only a String. So the only requirement for the Principal the SecurityManager should return is Serializable if we want to to send it across the wire.
> On Aug. 5, 2016, 4:51 p.m., Kirk Lund wrote:
> > geode-core/src/main/java/org/apache/geode/security/PostProcessor.java, line 24
> > <https://reviews.apache.org/r/50855/diff/1/?file=1464417#file1464417line24>
> >
> > This should be reworded to not use the term "customer". Maybe use "user" like SecurityManager javadocs mention.
> >
> > We should also modify the javadocs on both PostProcessor and SecurityManager to actually show what/how/why a user would actually use these classes.
>
> Kirk Lund wrote:
> Note: this can be done under a separate Jira ticket and changeset (doesn't need to be as part of this one)
I will update the javadoc.
- Jinmei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50855/#review144948
-----------------------------------------------------------
On Aug. 5, 2016, 3:38 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50855/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2016, 3:38 p.m.)
>
>
> Review request for geode, Grace Meilen, Kevin Duling, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Geode-1569: require principal to be Serializable so that post process can happen in a remote server
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/security/GeodeSecurityUtil.java a9c21629489251a336ad975edc28d221a089f7d1
> geode-core/src/main/java/com/gemstone/gemfire/internal/security/IntegratedSecurityService.java 84a0c320728899fb4cf92e786cdec4b86380438b
> geode-core/src/main/java/com/gemstone/gemfire/internal/security/SecurityService.java 9629ba3ad81aff24668565a3772ce334f794b242
> geode-core/src/main/java/com/gemstone/gemfire/internal/security/shiro/CustomAuthRealm.java edb0790f871251798f3f53482acec98bc2ab0efd
> geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/commands/DataCommands.java ae87b72018de414e0d37941f56f38d110fd34b94
> geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/domain/DataCommandRequest.java 92d4579153017cdce3694007410352c30d4fdb2d
> geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/functions/DataCommandFunction.java e6482c1b34349c3559c3596a0bef5522aa155523
> geode-core/src/main/java/org/apache/geode/security/PostProcessor.java 1a0e5ded09b57407a254d06e31bdf37c25ae8971
> geode-core/src/main/java/org/apache/geode/security/SecurityManager.java 273f2f139c20d478e896da31698f6da4aecaeeac
> geode-core/src/main/java/org/apache/geode/security/templates/SamplePostProcessor.java ce87bf8ae45287125890767824036bed43a83510
> geode-core/src/main/java/org/apache/geode/security/templates/SampleSecurityManager.java 83ac50e740f39cca9584dc3da9e905c62f33a7f7
> geode-core/src/test/java/com/gemstone/gemfire/security/AbstractSecureServerDUnitTest.java 4f0e26032e1124f6ad7c831e8570347cd3e00a34
> geode-core/src/test/java/com/gemstone/gemfire/security/NoShowValue1PostProcessor.java b42814423db5421d1b8007ae87171617c6c06421
> geode-core/src/test/java/com/gemstone/gemfire/security/PDXGfshPostProcessorOnRemoteServerTest.java PRE-CREATION
> geode-core/src/test/java/com/gemstone/gemfire/security/PDXPostProcessor.java 5609a21f94eb00edd0276cfd649355b0977174e1
> geode-core/src/test/java/com/gemstone/gemfire/security/SpySecurityManager.java 6d04a0db7feae9f9af59d2e39c45435cd7a0c653
>
> Diff: https://reviews.apache.org/r/50855/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 50855: Geode-1569: require principal to be
Serializable so that post process can happen in a remote server
Posted by Kirk Lund <ki...@gmail.com>.
> On Aug. 5, 2016, 4:51 p.m., Kirk Lund wrote:
> > geode-core/src/main/java/org/apache/geode/security/PostProcessor.java, line 24
> > <https://reviews.apache.org/r/50855/diff/1/?file=1464417#file1464417line24>
> >
> > This should be reworded to not use the term "customer". Maybe use "user" like SecurityManager javadocs mention.
> >
> > We should also modify the javadocs on both PostProcessor and SecurityManager to actually show what/how/why a user would actually use these classes.
Note: this can be done under a separate Jira ticket and changeset (doesn't need to be as part of this one)
- Kirk
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50855/#review144948
-----------------------------------------------------------
On Aug. 5, 2016, 3:38 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50855/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2016, 3:38 p.m.)
>
>
> Review request for geode, Grace Meilen, Kevin Duling, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Geode-1569: require principal to be Serializable so that post process can happen in a remote server
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/security/GeodeSecurityUtil.java a9c21629489251a336ad975edc28d221a089f7d1
> geode-core/src/main/java/com/gemstone/gemfire/internal/security/IntegratedSecurityService.java 84a0c320728899fb4cf92e786cdec4b86380438b
> geode-core/src/main/java/com/gemstone/gemfire/internal/security/SecurityService.java 9629ba3ad81aff24668565a3772ce334f794b242
> geode-core/src/main/java/com/gemstone/gemfire/internal/security/shiro/CustomAuthRealm.java edb0790f871251798f3f53482acec98bc2ab0efd
> geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/commands/DataCommands.java ae87b72018de414e0d37941f56f38d110fd34b94
> geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/domain/DataCommandRequest.java 92d4579153017cdce3694007410352c30d4fdb2d
> geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/functions/DataCommandFunction.java e6482c1b34349c3559c3596a0bef5522aa155523
> geode-core/src/main/java/org/apache/geode/security/PostProcessor.java 1a0e5ded09b57407a254d06e31bdf37c25ae8971
> geode-core/src/main/java/org/apache/geode/security/SecurityManager.java 273f2f139c20d478e896da31698f6da4aecaeeac
> geode-core/src/main/java/org/apache/geode/security/templates/SamplePostProcessor.java ce87bf8ae45287125890767824036bed43a83510
> geode-core/src/main/java/org/apache/geode/security/templates/SampleSecurityManager.java 83ac50e740f39cca9584dc3da9e905c62f33a7f7
> geode-core/src/test/java/com/gemstone/gemfire/security/AbstractSecureServerDUnitTest.java 4f0e26032e1124f6ad7c831e8570347cd3e00a34
> geode-core/src/test/java/com/gemstone/gemfire/security/NoShowValue1PostProcessor.java b42814423db5421d1b8007ae87171617c6c06421
> geode-core/src/test/java/com/gemstone/gemfire/security/PDXGfshPostProcessorOnRemoteServerTest.java PRE-CREATION
> geode-core/src/test/java/com/gemstone/gemfire/security/PDXPostProcessor.java 5609a21f94eb00edd0276cfd649355b0977174e1
> geode-core/src/test/java/com/gemstone/gemfire/security/SpySecurityManager.java 6d04a0db7feae9f9af59d2e39c45435cd7a0c653
>
> Diff: https://reviews.apache.org/r/50855/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jinmei Liao
>
>
Re: Review Request 50855: Geode-1569: require principal to be
Serializable so that post process can happen in a remote server
Posted by Kirk Lund <ki...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50855/#review144948
-----------------------------------------------------------
geode-core/src/main/java/org/apache/geode/security/PostProcessor.java (line 24)
<https://reviews.apache.org/r/50855/#comment211101>
This should be reworded to not use the term "customer". Maybe use "user" like SecurityManager javadocs mention.
We should also modify the javadocs on both PostProcessor and SecurityManager to actually show what/how/why a user would actually use these classes.
geode-core/src/main/java/org/apache/geode/security/PostProcessor.java (line 52)
<https://reviews.apache.org/r/50855/#comment211100>
Don't we want this to be a new org.apache.geode.security.SerializablePrincipal?
If not, then I'd restore it back to Principal and just modify the javadoc to say that the implementation of Principal must be Serializable.
geode-core/src/main/java/org/apache/geode/security/SecurityManager.java (line 52)
<https://reviews.apache.org/r/50855/#comment211103>
Same as before. I don't think we should replace Principal with Serializable. Keep it as Principal and just javadoc the fact that it must be serializable.
- Kirk Lund
On Aug. 5, 2016, 3:38 p.m., Jinmei Liao wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50855/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2016, 3:38 p.m.)
>
>
> Review request for geode, Grace Meilen, Kevin Duling, and Kirk Lund.
>
>
> Repository: geode
>
>
> Description
> -------
>
> Geode-1569: require principal to be Serializable so that post process can happen in a remote server
>
>
> Diffs
> -----
>
> geode-core/src/main/java/com/gemstone/gemfire/internal/security/GeodeSecurityUtil.java a9c21629489251a336ad975edc28d221a089f7d1
> geode-core/src/main/java/com/gemstone/gemfire/internal/security/IntegratedSecurityService.java 84a0c320728899fb4cf92e786cdec4b86380438b
> geode-core/src/main/java/com/gemstone/gemfire/internal/security/SecurityService.java 9629ba3ad81aff24668565a3772ce334f794b242
> geode-core/src/main/java/com/gemstone/gemfire/internal/security/shiro/CustomAuthRealm.java edb0790f871251798f3f53482acec98bc2ab0efd
> geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/commands/DataCommands.java ae87b72018de414e0d37941f56f38d110fd34b94
> geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/domain/DataCommandRequest.java 92d4579153017cdce3694007410352c30d4fdb2d
> geode-core/src/main/java/com/gemstone/gemfire/management/internal/cli/functions/DataCommandFunction.java e6482c1b34349c3559c3596a0bef5522aa155523
> geode-core/src/main/java/org/apache/geode/security/PostProcessor.java 1a0e5ded09b57407a254d06e31bdf37c25ae8971
> geode-core/src/main/java/org/apache/geode/security/SecurityManager.java 273f2f139c20d478e896da31698f6da4aecaeeac
> geode-core/src/main/java/org/apache/geode/security/templates/SamplePostProcessor.java ce87bf8ae45287125890767824036bed43a83510
> geode-core/src/main/java/org/apache/geode/security/templates/SampleSecurityManager.java 83ac50e740f39cca9584dc3da9e905c62f33a7f7
> geode-core/src/test/java/com/gemstone/gemfire/security/AbstractSecureServerDUnitTest.java 4f0e26032e1124f6ad7c831e8570347cd3e00a34
> geode-core/src/test/java/com/gemstone/gemfire/security/NoShowValue1PostProcessor.java b42814423db5421d1b8007ae87171617c6c06421
> geode-core/src/test/java/com/gemstone/gemfire/security/PDXGfshPostProcessorOnRemoteServerTest.java PRE-CREATION
> geode-core/src/test/java/com/gemstone/gemfire/security/PDXPostProcessor.java 5609a21f94eb00edd0276cfd649355b0977174e1
> geode-core/src/test/java/com/gemstone/gemfire/security/SpySecurityManager.java 6d04a0db7feae9f9af59d2e39c45435cd7a0c653
>
> Diff: https://reviews.apache.org/r/50855/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Jinmei Liao
>
>