You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Kirk Lund <ki...@gmail.com> on 2016/03/04 20:28:57 UTC

Review Request 44402: GEODE-949: fix serialization of GemFireSecurityExceptions to improve debugging

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44402/
-----------------------------------------------------------

Review request for geode, Jens Deppe and Jinmei Liao.


Bugs: GEODE-949
    https://issues.apache.org/jira/browse/GEODE-949


Repository: geode


Description
-------

GEODE-949: fix serialization of GemFireSecurityExceptions to improve debugging

* add workarounds to security exceptions for unserializable fields (inspired by similiar code in Spring ldap)
* add cause for security exceptions in test code instead of eating exceptions
* cleanup javadocs of security exceptions
* introduce unit tests
* replace hardcoded class strings with class getName in security test code to help facilitate repackaging


Diffs
-----

  geode-core/src/main/java/com/gemstone/gemfire/security/GemFireSecurityException.java 1f97420 
  geode-core/src/main/java/com/gemstone/gemfire/security/NotAuthorizedException.java c6165a6 
  geode-core/src/test/java/com/gemstone/gemfire/security/GemFireSecurityExceptionTest.java PRE-CREATION 
  geode-core/src/test/java/com/gemstone/gemfire/security/NotAuthorizedExceptionTest.java PRE-CREATION 
  geode-core/src/test/java/security/AuthzCredentialGenerator.java e15a60a 
  geode-core/src/test/java/security/CredentialGenerator.java 7a430f1 
  geode-core/src/test/java/security/DummyAuthzCredentialGenerator.java 7e40d13 
  geode-core/src/test/java/security/DummyCredentialGenerator.java 86b26a7 
  geode-core/src/test/java/security/LdapUserCredentialGenerator.java 12bcb62 
  geode-core/src/test/java/security/PKCSCredentialGenerator.java 24c0100 
  geode-core/src/test/java/security/SSLCredentialGenerator.java 29a1a30 
  geode-core/src/test/java/security/UserPasswordWithExtraPropsAuthInit.java a41f73a 
  geode-core/src/test/java/security/XmlAuthzCredentialGenerator.java 929eafb 
  geode-core/src/test/java/templates/security/DummyAuthenticator.java 5d33f22 
  geode-core/src/test/java/templates/security/DummyAuthorization.java fe8e908 
  geode-core/src/test/java/templates/security/FunctionSecurityPrmsHolder.java 76827bb 
  geode-core/src/test/java/templates/security/LdapUserAuthenticator.java db55219 
  geode-core/src/test/java/templates/security/PKCSAuthInit.java d43b78e 
  geode-core/src/test/java/templates/security/PKCSAuthenticator.java d3610c4 
  geode-core/src/test/java/templates/security/PKCSPrincipal.java 563689b 
  geode-core/src/test/java/templates/security/PKCSPrincipalTest.java PRE-CREATION 
  geode-core/src/test/java/templates/security/UserPasswordAuthInit.java f4b6eec 
  geode-core/src/test/java/templates/security/UsernamePrincipal.java 739dd52 
  geode-core/src/test/java/templates/security/UsernamePrincipalTest.java PRE-CREATION 
  geode-core/src/test/java/templates/security/XmlAuthorization.java 1ed0142 
  geode-core/src/test/java/templates/security/XmlErrorHandler.java 5da8e09 
  geode-core/src/test/resources/com/gemstone/gemfire/codeAnalysis/sanctionedSerializables.txt f3c1c5d 

Diff: https://reviews.apache.org/r/44402/diff/


Testing
-------

precheckin


Thanks,

Kirk Lund

Re: Review Request 44402: GEODE-949: fix serialization of GemFireSecurityExceptions to improve debugging

Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44402/#review122351
-----------------------------------------------------------


Ship it!




Ship It!

- Jinmei Liao


On March 4, 2016, 7:28 p.m., Kirk Lund wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44402/
> -----------------------------------------------------------
> 
> (Updated March 4, 2016, 7:28 p.m.)
> 
> 
> Review request for geode, Jens Deppe and Jinmei Liao.
> 
> 
> Bugs: GEODE-949
>     https://issues.apache.org/jira/browse/GEODE-949
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-949: fix serialization of GemFireSecurityExceptions to improve debugging
> 
> * add workarounds to security exceptions for unserializable fields (inspired by similiar code in Spring ldap)
> * add cause for security exceptions in test code instead of eating exceptions
> * cleanup javadocs of security exceptions
> * introduce unit tests
> * replace hardcoded class strings with class getName in security test code to help facilitate repackaging
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/com/gemstone/gemfire/security/GemFireSecurityException.java 1f97420 
>   geode-core/src/main/java/com/gemstone/gemfire/security/NotAuthorizedException.java c6165a6 
>   geode-core/src/test/java/com/gemstone/gemfire/security/GemFireSecurityExceptionTest.java PRE-CREATION 
>   geode-core/src/test/java/com/gemstone/gemfire/security/NotAuthorizedExceptionTest.java PRE-CREATION 
>   geode-core/src/test/java/security/AuthzCredentialGenerator.java e15a60a 
>   geode-core/src/test/java/security/CredentialGenerator.java 7a430f1 
>   geode-core/src/test/java/security/DummyAuthzCredentialGenerator.java 7e40d13 
>   geode-core/src/test/java/security/DummyCredentialGenerator.java 86b26a7 
>   geode-core/src/test/java/security/LdapUserCredentialGenerator.java 12bcb62 
>   geode-core/src/test/java/security/PKCSCredentialGenerator.java 24c0100 
>   geode-core/src/test/java/security/SSLCredentialGenerator.java 29a1a30 
>   geode-core/src/test/java/security/UserPasswordWithExtraPropsAuthInit.java a41f73a 
>   geode-core/src/test/java/security/XmlAuthzCredentialGenerator.java 929eafb 
>   geode-core/src/test/java/templates/security/DummyAuthenticator.java 5d33f22 
>   geode-core/src/test/java/templates/security/DummyAuthorization.java fe8e908 
>   geode-core/src/test/java/templates/security/FunctionSecurityPrmsHolder.java 76827bb 
>   geode-core/src/test/java/templates/security/LdapUserAuthenticator.java db55219 
>   geode-core/src/test/java/templates/security/PKCSAuthInit.java d43b78e 
>   geode-core/src/test/java/templates/security/PKCSAuthenticator.java d3610c4 
>   geode-core/src/test/java/templates/security/PKCSPrincipal.java 563689b 
>   geode-core/src/test/java/templates/security/PKCSPrincipalTest.java PRE-CREATION 
>   geode-core/src/test/java/templates/security/UserPasswordAuthInit.java f4b6eec 
>   geode-core/src/test/java/templates/security/UsernamePrincipal.java 739dd52 
>   geode-core/src/test/java/templates/security/UsernamePrincipalTest.java PRE-CREATION 
>   geode-core/src/test/java/templates/security/XmlAuthorization.java 1ed0142 
>   geode-core/src/test/java/templates/security/XmlErrorHandler.java 5da8e09 
>   geode-core/src/test/resources/com/gemstone/gemfire/codeAnalysis/sanctionedSerializables.txt f3c1c5d 
> 
> Diff: https://reviews.apache.org/r/44402/diff/
> 
> 
> Testing
> -------
> 
> precheckin
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 44402: GEODE-949: fix serialization of GemFireSecurityExceptions to improve debugging

Posted by Kirk Lund <ki...@gmail.com>.

> On March 7, 2016, 6:33 p.m., Jinmei Liao wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/security/GemFireSecurityException.java, line 38
> > <https://reviews.apache.org/r/44402/diff/1/?file=1281204#file1281204line38>
> >
> >     Probably I don't understand the purpose of this yet. Why we would need to have a cause variable? Is the cuase in the parent class not sufficient?
> 
> Kirk Lund wrote:
>     Adding a cause variable with custom serialization is how Spring works around the NamingException (cause) with an unserializable field (resolvedObj which is frequently LdapCtxt which is unserializable). You can't override the way in which super.cause is serialized, so the Exception has to have its own cause field -- super.cause remains null. During serialization, if cause is a NamingException and if it has an unserializable field in resolvedObj, then the Exception temporarily nulls it out and immediately restores the field after serializing itself.
>     
>     I copied the approach from Spring Ldap which wraps all NamingExceptions, but implemented it a little differently specific to our need.
> 
> Jinmei Liao wrote:
>     I see. Thanks! If this is used to handle serialization problem, would GemfireException be a better place for it? I saw it has some commented out code about "cause".....

I took a look at GemFireException. That dead code is from early GemFire when the JDK Throwable didn't support cause. We should delete the deadcode.

I'd prefer to keep the custom serialization for NamingException as close to the code that needs it as possible. It's only needed by our security exceptions that wrap ldap NamingException. If we ever find ourselves using non-security exceptions to wrap NamingException, then we could always move the custom cause to a different exception class. I think that if security is the only code section that needs this then we should solve the problem there.


- Kirk


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44402/#review122325
-----------------------------------------------------------


On March 4, 2016, 7:28 p.m., Kirk Lund wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44402/
> -----------------------------------------------------------
> 
> (Updated March 4, 2016, 7:28 p.m.)
> 
> 
> Review request for geode, Jens Deppe and Jinmei Liao.
> 
> 
> Bugs: GEODE-949
>     https://issues.apache.org/jira/browse/GEODE-949
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-949: fix serialization of GemFireSecurityExceptions to improve debugging
> 
> * add workarounds to security exceptions for unserializable fields (inspired by similiar code in Spring ldap)
> * add cause for security exceptions in test code instead of eating exceptions
> * cleanup javadocs of security exceptions
> * introduce unit tests
> * replace hardcoded class strings with class getName in security test code to help facilitate repackaging
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/com/gemstone/gemfire/security/GemFireSecurityException.java 1f97420 
>   geode-core/src/main/java/com/gemstone/gemfire/security/NotAuthorizedException.java c6165a6 
>   geode-core/src/test/java/com/gemstone/gemfire/security/GemFireSecurityExceptionTest.java PRE-CREATION 
>   geode-core/src/test/java/com/gemstone/gemfire/security/NotAuthorizedExceptionTest.java PRE-CREATION 
>   geode-core/src/test/java/security/AuthzCredentialGenerator.java e15a60a 
>   geode-core/src/test/java/security/CredentialGenerator.java 7a430f1 
>   geode-core/src/test/java/security/DummyAuthzCredentialGenerator.java 7e40d13 
>   geode-core/src/test/java/security/DummyCredentialGenerator.java 86b26a7 
>   geode-core/src/test/java/security/LdapUserCredentialGenerator.java 12bcb62 
>   geode-core/src/test/java/security/PKCSCredentialGenerator.java 24c0100 
>   geode-core/src/test/java/security/SSLCredentialGenerator.java 29a1a30 
>   geode-core/src/test/java/security/UserPasswordWithExtraPropsAuthInit.java a41f73a 
>   geode-core/src/test/java/security/XmlAuthzCredentialGenerator.java 929eafb 
>   geode-core/src/test/java/templates/security/DummyAuthenticator.java 5d33f22 
>   geode-core/src/test/java/templates/security/DummyAuthorization.java fe8e908 
>   geode-core/src/test/java/templates/security/FunctionSecurityPrmsHolder.java 76827bb 
>   geode-core/src/test/java/templates/security/LdapUserAuthenticator.java db55219 
>   geode-core/src/test/java/templates/security/PKCSAuthInit.java d43b78e 
>   geode-core/src/test/java/templates/security/PKCSAuthenticator.java d3610c4 
>   geode-core/src/test/java/templates/security/PKCSPrincipal.java 563689b 
>   geode-core/src/test/java/templates/security/PKCSPrincipalTest.java PRE-CREATION 
>   geode-core/src/test/java/templates/security/UserPasswordAuthInit.java f4b6eec 
>   geode-core/src/test/java/templates/security/UsernamePrincipal.java 739dd52 
>   geode-core/src/test/java/templates/security/UsernamePrincipalTest.java PRE-CREATION 
>   geode-core/src/test/java/templates/security/XmlAuthorization.java 1ed0142 
>   geode-core/src/test/java/templates/security/XmlErrorHandler.java 5da8e09 
>   geode-core/src/test/resources/com/gemstone/gemfire/codeAnalysis/sanctionedSerializables.txt f3c1c5d 
> 
> Diff: https://reviews.apache.org/r/44402/diff/
> 
> 
> Testing
> -------
> 
> precheckin
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 44402: GEODE-949: fix serialization of GemFireSecurityExceptions to improve debugging

Posted by Jinmei Liao <ji...@pivotal.io>.

> On March 7, 2016, 6:33 p.m., Jinmei Liao wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/security/GemFireSecurityException.java, line 38
> > <https://reviews.apache.org/r/44402/diff/1/?file=1281204#file1281204line38>
> >
> >     Probably I don't understand the purpose of this yet. Why we would need to have a cause variable? Is the cuase in the parent class not sufficient?
> 
> Kirk Lund wrote:
>     Adding a cause variable with custom serialization is how Spring works around the NamingException (cause) with an unserializable field (resolvedObj which is frequently LdapCtxt which is unserializable). You can't override the way in which super.cause is serialized, so the Exception has to have its own cause field -- super.cause remains null. During serialization, if cause is a NamingException and if it has an unserializable field in resolvedObj, then the Exception temporarily nulls it out and immediately restores the field after serializing itself.
>     
>     I copied the approach from Spring Ldap which wraps all NamingExceptions, but implemented it a little differently specific to our need.

I see. Thanks! If this is used to handle serialization problem, would GemfireException be a better place for it? I saw it has some commented out code about "cause".....


- Jinmei


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44402/#review122325
-----------------------------------------------------------


On March 4, 2016, 7:28 p.m., Kirk Lund wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44402/
> -----------------------------------------------------------
> 
> (Updated March 4, 2016, 7:28 p.m.)
> 
> 
> Review request for geode, Jens Deppe and Jinmei Liao.
> 
> 
> Bugs: GEODE-949
>     https://issues.apache.org/jira/browse/GEODE-949
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-949: fix serialization of GemFireSecurityExceptions to improve debugging
> 
> * add workarounds to security exceptions for unserializable fields (inspired by similiar code in Spring ldap)
> * add cause for security exceptions in test code instead of eating exceptions
> * cleanup javadocs of security exceptions
> * introduce unit tests
> * replace hardcoded class strings with class getName in security test code to help facilitate repackaging
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/com/gemstone/gemfire/security/GemFireSecurityException.java 1f97420 
>   geode-core/src/main/java/com/gemstone/gemfire/security/NotAuthorizedException.java c6165a6 
>   geode-core/src/test/java/com/gemstone/gemfire/security/GemFireSecurityExceptionTest.java PRE-CREATION 
>   geode-core/src/test/java/com/gemstone/gemfire/security/NotAuthorizedExceptionTest.java PRE-CREATION 
>   geode-core/src/test/java/security/AuthzCredentialGenerator.java e15a60a 
>   geode-core/src/test/java/security/CredentialGenerator.java 7a430f1 
>   geode-core/src/test/java/security/DummyAuthzCredentialGenerator.java 7e40d13 
>   geode-core/src/test/java/security/DummyCredentialGenerator.java 86b26a7 
>   geode-core/src/test/java/security/LdapUserCredentialGenerator.java 12bcb62 
>   geode-core/src/test/java/security/PKCSCredentialGenerator.java 24c0100 
>   geode-core/src/test/java/security/SSLCredentialGenerator.java 29a1a30 
>   geode-core/src/test/java/security/UserPasswordWithExtraPropsAuthInit.java a41f73a 
>   geode-core/src/test/java/security/XmlAuthzCredentialGenerator.java 929eafb 
>   geode-core/src/test/java/templates/security/DummyAuthenticator.java 5d33f22 
>   geode-core/src/test/java/templates/security/DummyAuthorization.java fe8e908 
>   geode-core/src/test/java/templates/security/FunctionSecurityPrmsHolder.java 76827bb 
>   geode-core/src/test/java/templates/security/LdapUserAuthenticator.java db55219 
>   geode-core/src/test/java/templates/security/PKCSAuthInit.java d43b78e 
>   geode-core/src/test/java/templates/security/PKCSAuthenticator.java d3610c4 
>   geode-core/src/test/java/templates/security/PKCSPrincipal.java 563689b 
>   geode-core/src/test/java/templates/security/PKCSPrincipalTest.java PRE-CREATION 
>   geode-core/src/test/java/templates/security/UserPasswordAuthInit.java f4b6eec 
>   geode-core/src/test/java/templates/security/UsernamePrincipal.java 739dd52 
>   geode-core/src/test/java/templates/security/UsernamePrincipalTest.java PRE-CREATION 
>   geode-core/src/test/java/templates/security/XmlAuthorization.java 1ed0142 
>   geode-core/src/test/java/templates/security/XmlErrorHandler.java 5da8e09 
>   geode-core/src/test/resources/com/gemstone/gemfire/codeAnalysis/sanctionedSerializables.txt f3c1c5d 
> 
> Diff: https://reviews.apache.org/r/44402/diff/
> 
> 
> Testing
> -------
> 
> precheckin
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 44402: GEODE-949: fix serialization of GemFireSecurityExceptions to improve debugging

Posted by Kirk Lund <ki...@gmail.com>.

> On March 7, 2016, 6:33 p.m., Jinmei Liao wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/security/GemFireSecurityException.java, line 38
> > <https://reviews.apache.org/r/44402/diff/1/?file=1281204#file1281204line38>
> >
> >     Probably I don't understand the purpose of this yet. Why we would need to have a cause variable? Is the cuase in the parent class not sufficient?

Adding a cause variable with custom serialization is how Spring works around the NamingException (cause) with an unserializable field (resolvedObj which is frequently LdapCtxt which is unserializable). You can't override the way in which super.cause is serialized, so the Exception has to have its own cause field -- super.cause remains null. During serialization, if cause is a NamingException and if it has an unserializable field in resolvedObj, then the Exception temporarily nulls it out and immediately restores the field after serializing itself.

I copied the approach from Spring Ldap which wraps all NamingExceptions, but implemented it a little differently specific to our need.


- Kirk


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44402/#review122325
-----------------------------------------------------------


On March 4, 2016, 7:28 p.m., Kirk Lund wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44402/
> -----------------------------------------------------------
> 
> (Updated March 4, 2016, 7:28 p.m.)
> 
> 
> Review request for geode, Jens Deppe and Jinmei Liao.
> 
> 
> Bugs: GEODE-949
>     https://issues.apache.org/jira/browse/GEODE-949
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-949: fix serialization of GemFireSecurityExceptions to improve debugging
> 
> * add workarounds to security exceptions for unserializable fields (inspired by similiar code in Spring ldap)
> * add cause for security exceptions in test code instead of eating exceptions
> * cleanup javadocs of security exceptions
> * introduce unit tests
> * replace hardcoded class strings with class getName in security test code to help facilitate repackaging
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/com/gemstone/gemfire/security/GemFireSecurityException.java 1f97420 
>   geode-core/src/main/java/com/gemstone/gemfire/security/NotAuthorizedException.java c6165a6 
>   geode-core/src/test/java/com/gemstone/gemfire/security/GemFireSecurityExceptionTest.java PRE-CREATION 
>   geode-core/src/test/java/com/gemstone/gemfire/security/NotAuthorizedExceptionTest.java PRE-CREATION 
>   geode-core/src/test/java/security/AuthzCredentialGenerator.java e15a60a 
>   geode-core/src/test/java/security/CredentialGenerator.java 7a430f1 
>   geode-core/src/test/java/security/DummyAuthzCredentialGenerator.java 7e40d13 
>   geode-core/src/test/java/security/DummyCredentialGenerator.java 86b26a7 
>   geode-core/src/test/java/security/LdapUserCredentialGenerator.java 12bcb62 
>   geode-core/src/test/java/security/PKCSCredentialGenerator.java 24c0100 
>   geode-core/src/test/java/security/SSLCredentialGenerator.java 29a1a30 
>   geode-core/src/test/java/security/UserPasswordWithExtraPropsAuthInit.java a41f73a 
>   geode-core/src/test/java/security/XmlAuthzCredentialGenerator.java 929eafb 
>   geode-core/src/test/java/templates/security/DummyAuthenticator.java 5d33f22 
>   geode-core/src/test/java/templates/security/DummyAuthorization.java fe8e908 
>   geode-core/src/test/java/templates/security/FunctionSecurityPrmsHolder.java 76827bb 
>   geode-core/src/test/java/templates/security/LdapUserAuthenticator.java db55219 
>   geode-core/src/test/java/templates/security/PKCSAuthInit.java d43b78e 
>   geode-core/src/test/java/templates/security/PKCSAuthenticator.java d3610c4 
>   geode-core/src/test/java/templates/security/PKCSPrincipal.java 563689b 
>   geode-core/src/test/java/templates/security/PKCSPrincipalTest.java PRE-CREATION 
>   geode-core/src/test/java/templates/security/UserPasswordAuthInit.java f4b6eec 
>   geode-core/src/test/java/templates/security/UsernamePrincipal.java 739dd52 
>   geode-core/src/test/java/templates/security/UsernamePrincipalTest.java PRE-CREATION 
>   geode-core/src/test/java/templates/security/XmlAuthorization.java 1ed0142 
>   geode-core/src/test/java/templates/security/XmlErrorHandler.java 5da8e09 
>   geode-core/src/test/resources/com/gemstone/gemfire/codeAnalysis/sanctionedSerializables.txt f3c1c5d 
> 
> Diff: https://reviews.apache.org/r/44402/diff/
> 
> 
> Testing
> -------
> 
> precheckin
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 44402: GEODE-949: fix serialization of GemFireSecurityExceptions to improve debugging

Posted by Jinmei Liao <ji...@pivotal.io>.

> On March 7, 2016, 6:33 p.m., Jinmei Liao wrote:
> > geode-core/src/main/java/com/gemstone/gemfire/security/GemFireSecurityException.java, line 38
> > <https://reviews.apache.org/r/44402/diff/1/?file=1281204#file1281204line38>
> >
> >     Probably I don't understand the purpose of this yet. Why we would need to have a cause variable? Is the cuase in the parent class not sufficient?
> 
> Kirk Lund wrote:
>     Adding a cause variable with custom serialization is how Spring works around the NamingException (cause) with an unserializable field (resolvedObj which is frequently LdapCtxt which is unserializable). You can't override the way in which super.cause is serialized, so the Exception has to have its own cause field -- super.cause remains null. During serialization, if cause is a NamingException and if it has an unserializable field in resolvedObj, then the Exception temporarily nulls it out and immediately restores the field after serializing itself.
>     
>     I copied the approach from Spring Ldap which wraps all NamingExceptions, but implemented it a little differently specific to our need.
> 
> Jinmei Liao wrote:
>     I see. Thanks! If this is used to handle serialization problem, would GemfireException be a better place for it? I saw it has some commented out code about "cause".....
> 
> Kirk Lund wrote:
>     I took a look at GemFireException. That dead code is from early GemFire when the JDK Throwable didn't support cause. We should delete the deadcode.
>     
>     I'd prefer to keep the custom serialization for NamingException as close to the code that needs it as possible. It's only needed by our security exceptions that wrap ldap NamingException. If we ever find ourselves using non-security exceptions to wrap NamingException, then we could always move the custom cause to a different exception class. I think that if security is the only code section that needs this then we should solve the problem there.

Sure. Thanks!


- Jinmei


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44402/#review122325
-----------------------------------------------------------


On March 4, 2016, 7:28 p.m., Kirk Lund wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44402/
> -----------------------------------------------------------
> 
> (Updated March 4, 2016, 7:28 p.m.)
> 
> 
> Review request for geode, Jens Deppe and Jinmei Liao.
> 
> 
> Bugs: GEODE-949
>     https://issues.apache.org/jira/browse/GEODE-949
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-949: fix serialization of GemFireSecurityExceptions to improve debugging
> 
> * add workarounds to security exceptions for unserializable fields (inspired by similiar code in Spring ldap)
> * add cause for security exceptions in test code instead of eating exceptions
> * cleanup javadocs of security exceptions
> * introduce unit tests
> * replace hardcoded class strings with class getName in security test code to help facilitate repackaging
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/com/gemstone/gemfire/security/GemFireSecurityException.java 1f97420 
>   geode-core/src/main/java/com/gemstone/gemfire/security/NotAuthorizedException.java c6165a6 
>   geode-core/src/test/java/com/gemstone/gemfire/security/GemFireSecurityExceptionTest.java PRE-CREATION 
>   geode-core/src/test/java/com/gemstone/gemfire/security/NotAuthorizedExceptionTest.java PRE-CREATION 
>   geode-core/src/test/java/security/AuthzCredentialGenerator.java e15a60a 
>   geode-core/src/test/java/security/CredentialGenerator.java 7a430f1 
>   geode-core/src/test/java/security/DummyAuthzCredentialGenerator.java 7e40d13 
>   geode-core/src/test/java/security/DummyCredentialGenerator.java 86b26a7 
>   geode-core/src/test/java/security/LdapUserCredentialGenerator.java 12bcb62 
>   geode-core/src/test/java/security/PKCSCredentialGenerator.java 24c0100 
>   geode-core/src/test/java/security/SSLCredentialGenerator.java 29a1a30 
>   geode-core/src/test/java/security/UserPasswordWithExtraPropsAuthInit.java a41f73a 
>   geode-core/src/test/java/security/XmlAuthzCredentialGenerator.java 929eafb 
>   geode-core/src/test/java/templates/security/DummyAuthenticator.java 5d33f22 
>   geode-core/src/test/java/templates/security/DummyAuthorization.java fe8e908 
>   geode-core/src/test/java/templates/security/FunctionSecurityPrmsHolder.java 76827bb 
>   geode-core/src/test/java/templates/security/LdapUserAuthenticator.java db55219 
>   geode-core/src/test/java/templates/security/PKCSAuthInit.java d43b78e 
>   geode-core/src/test/java/templates/security/PKCSAuthenticator.java d3610c4 
>   geode-core/src/test/java/templates/security/PKCSPrincipal.java 563689b 
>   geode-core/src/test/java/templates/security/PKCSPrincipalTest.java PRE-CREATION 
>   geode-core/src/test/java/templates/security/UserPasswordAuthInit.java f4b6eec 
>   geode-core/src/test/java/templates/security/UsernamePrincipal.java 739dd52 
>   geode-core/src/test/java/templates/security/UsernamePrincipalTest.java PRE-CREATION 
>   geode-core/src/test/java/templates/security/XmlAuthorization.java 1ed0142 
>   geode-core/src/test/java/templates/security/XmlErrorHandler.java 5da8e09 
>   geode-core/src/test/resources/com/gemstone/gemfire/codeAnalysis/sanctionedSerializables.txt f3c1c5d 
> 
> Diff: https://reviews.apache.org/r/44402/diff/
> 
> 
> Testing
> -------
> 
> precheckin
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 44402: GEODE-949: fix serialization of GemFireSecurityExceptions to improve debugging

Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44402/#review122325
-----------------------------------------------------------




geode-core/src/main/java/com/gemstone/gemfire/security/GemFireSecurityException.java (line 37)
<https://reviews.apache.org/r/44402/#comment184283>

    Probably I don't understand the purpose of this yet. Why we would need to have a cause variable? Is the cuase in the parent class not sufficient?


- Jinmei Liao


On March 4, 2016, 7:28 p.m., Kirk Lund wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44402/
> -----------------------------------------------------------
> 
> (Updated March 4, 2016, 7:28 p.m.)
> 
> 
> Review request for geode, Jens Deppe and Jinmei Liao.
> 
> 
> Bugs: GEODE-949
>     https://issues.apache.org/jira/browse/GEODE-949
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-949: fix serialization of GemFireSecurityExceptions to improve debugging
> 
> * add workarounds to security exceptions for unserializable fields (inspired by similiar code in Spring ldap)
> * add cause for security exceptions in test code instead of eating exceptions
> * cleanup javadocs of security exceptions
> * introduce unit tests
> * replace hardcoded class strings with class getName in security test code to help facilitate repackaging
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/com/gemstone/gemfire/security/GemFireSecurityException.java 1f97420 
>   geode-core/src/main/java/com/gemstone/gemfire/security/NotAuthorizedException.java c6165a6 
>   geode-core/src/test/java/com/gemstone/gemfire/security/GemFireSecurityExceptionTest.java PRE-CREATION 
>   geode-core/src/test/java/com/gemstone/gemfire/security/NotAuthorizedExceptionTest.java PRE-CREATION 
>   geode-core/src/test/java/security/AuthzCredentialGenerator.java e15a60a 
>   geode-core/src/test/java/security/CredentialGenerator.java 7a430f1 
>   geode-core/src/test/java/security/DummyAuthzCredentialGenerator.java 7e40d13 
>   geode-core/src/test/java/security/DummyCredentialGenerator.java 86b26a7 
>   geode-core/src/test/java/security/LdapUserCredentialGenerator.java 12bcb62 
>   geode-core/src/test/java/security/PKCSCredentialGenerator.java 24c0100 
>   geode-core/src/test/java/security/SSLCredentialGenerator.java 29a1a30 
>   geode-core/src/test/java/security/UserPasswordWithExtraPropsAuthInit.java a41f73a 
>   geode-core/src/test/java/security/XmlAuthzCredentialGenerator.java 929eafb 
>   geode-core/src/test/java/templates/security/DummyAuthenticator.java 5d33f22 
>   geode-core/src/test/java/templates/security/DummyAuthorization.java fe8e908 
>   geode-core/src/test/java/templates/security/FunctionSecurityPrmsHolder.java 76827bb 
>   geode-core/src/test/java/templates/security/LdapUserAuthenticator.java db55219 
>   geode-core/src/test/java/templates/security/PKCSAuthInit.java d43b78e 
>   geode-core/src/test/java/templates/security/PKCSAuthenticator.java d3610c4 
>   geode-core/src/test/java/templates/security/PKCSPrincipal.java 563689b 
>   geode-core/src/test/java/templates/security/PKCSPrincipalTest.java PRE-CREATION 
>   geode-core/src/test/java/templates/security/UserPasswordAuthInit.java f4b6eec 
>   geode-core/src/test/java/templates/security/UsernamePrincipal.java 739dd52 
>   geode-core/src/test/java/templates/security/UsernamePrincipalTest.java PRE-CREATION 
>   geode-core/src/test/java/templates/security/XmlAuthorization.java 1ed0142 
>   geode-core/src/test/java/templates/security/XmlErrorHandler.java 5da8e09 
>   geode-core/src/test/resources/com/gemstone/gemfire/codeAnalysis/sanctionedSerializables.txt f3c1c5d 
> 
> Diff: https://reviews.apache.org/r/44402/diff/
> 
> 
> Testing
> -------
> 
> precheckin
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 44402: GEODE-949: fix serialization of GemFireSecurityExceptions to improve debugging

Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44402/#review122352
-----------------------------------------------------------


Ship it!




Ship It!

- Jinmei Liao


On March 4, 2016, 7:28 p.m., Kirk Lund wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44402/
> -----------------------------------------------------------
> 
> (Updated March 4, 2016, 7:28 p.m.)
> 
> 
> Review request for geode, Jens Deppe and Jinmei Liao.
> 
> 
> Bugs: GEODE-949
>     https://issues.apache.org/jira/browse/GEODE-949
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-949: fix serialization of GemFireSecurityExceptions to improve debugging
> 
> * add workarounds to security exceptions for unserializable fields (inspired by similiar code in Spring ldap)
> * add cause for security exceptions in test code instead of eating exceptions
> * cleanup javadocs of security exceptions
> * introduce unit tests
> * replace hardcoded class strings with class getName in security test code to help facilitate repackaging
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/com/gemstone/gemfire/security/GemFireSecurityException.java 1f97420 
>   geode-core/src/main/java/com/gemstone/gemfire/security/NotAuthorizedException.java c6165a6 
>   geode-core/src/test/java/com/gemstone/gemfire/security/GemFireSecurityExceptionTest.java PRE-CREATION 
>   geode-core/src/test/java/com/gemstone/gemfire/security/NotAuthorizedExceptionTest.java PRE-CREATION 
>   geode-core/src/test/java/security/AuthzCredentialGenerator.java e15a60a 
>   geode-core/src/test/java/security/CredentialGenerator.java 7a430f1 
>   geode-core/src/test/java/security/DummyAuthzCredentialGenerator.java 7e40d13 
>   geode-core/src/test/java/security/DummyCredentialGenerator.java 86b26a7 
>   geode-core/src/test/java/security/LdapUserCredentialGenerator.java 12bcb62 
>   geode-core/src/test/java/security/PKCSCredentialGenerator.java 24c0100 
>   geode-core/src/test/java/security/SSLCredentialGenerator.java 29a1a30 
>   geode-core/src/test/java/security/UserPasswordWithExtraPropsAuthInit.java a41f73a 
>   geode-core/src/test/java/security/XmlAuthzCredentialGenerator.java 929eafb 
>   geode-core/src/test/java/templates/security/DummyAuthenticator.java 5d33f22 
>   geode-core/src/test/java/templates/security/DummyAuthorization.java fe8e908 
>   geode-core/src/test/java/templates/security/FunctionSecurityPrmsHolder.java 76827bb 
>   geode-core/src/test/java/templates/security/LdapUserAuthenticator.java db55219 
>   geode-core/src/test/java/templates/security/PKCSAuthInit.java d43b78e 
>   geode-core/src/test/java/templates/security/PKCSAuthenticator.java d3610c4 
>   geode-core/src/test/java/templates/security/PKCSPrincipal.java 563689b 
>   geode-core/src/test/java/templates/security/PKCSPrincipalTest.java PRE-CREATION 
>   geode-core/src/test/java/templates/security/UserPasswordAuthInit.java f4b6eec 
>   geode-core/src/test/java/templates/security/UsernamePrincipal.java 739dd52 
>   geode-core/src/test/java/templates/security/UsernamePrincipalTest.java PRE-CREATION 
>   geode-core/src/test/java/templates/security/XmlAuthorization.java 1ed0142 
>   geode-core/src/test/java/templates/security/XmlErrorHandler.java 5da8e09 
>   geode-core/src/test/resources/com/gemstone/gemfire/codeAnalysis/sanctionedSerializables.txt f3c1c5d 
> 
> Diff: https://reviews.apache.org/r/44402/diff/
> 
> 
> Testing
> -------
> 
> precheckin
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>

Re: Review Request 44402: GEODE-949: fix serialization of GemFireSecurityExceptions to improve debugging

Posted by Jinmei Liao <ji...@pivotal.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44402/#review122353
-----------------------------------------------------------


Ship it!




Ship It!

- Jinmei Liao


On March 4, 2016, 7:28 p.m., Kirk Lund wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44402/
> -----------------------------------------------------------
> 
> (Updated March 4, 2016, 7:28 p.m.)
> 
> 
> Review request for geode, Jens Deppe and Jinmei Liao.
> 
> 
> Bugs: GEODE-949
>     https://issues.apache.org/jira/browse/GEODE-949
> 
> 
> Repository: geode
> 
> 
> Description
> -------
> 
> GEODE-949: fix serialization of GemFireSecurityExceptions to improve debugging
> 
> * add workarounds to security exceptions for unserializable fields (inspired by similiar code in Spring ldap)
> * add cause for security exceptions in test code instead of eating exceptions
> * cleanup javadocs of security exceptions
> * introduce unit tests
> * replace hardcoded class strings with class getName in security test code to help facilitate repackaging
> 
> 
> Diffs
> -----
> 
>   geode-core/src/main/java/com/gemstone/gemfire/security/GemFireSecurityException.java 1f97420 
>   geode-core/src/main/java/com/gemstone/gemfire/security/NotAuthorizedException.java c6165a6 
>   geode-core/src/test/java/com/gemstone/gemfire/security/GemFireSecurityExceptionTest.java PRE-CREATION 
>   geode-core/src/test/java/com/gemstone/gemfire/security/NotAuthorizedExceptionTest.java PRE-CREATION 
>   geode-core/src/test/java/security/AuthzCredentialGenerator.java e15a60a 
>   geode-core/src/test/java/security/CredentialGenerator.java 7a430f1 
>   geode-core/src/test/java/security/DummyAuthzCredentialGenerator.java 7e40d13 
>   geode-core/src/test/java/security/DummyCredentialGenerator.java 86b26a7 
>   geode-core/src/test/java/security/LdapUserCredentialGenerator.java 12bcb62 
>   geode-core/src/test/java/security/PKCSCredentialGenerator.java 24c0100 
>   geode-core/src/test/java/security/SSLCredentialGenerator.java 29a1a30 
>   geode-core/src/test/java/security/UserPasswordWithExtraPropsAuthInit.java a41f73a 
>   geode-core/src/test/java/security/XmlAuthzCredentialGenerator.java 929eafb 
>   geode-core/src/test/java/templates/security/DummyAuthenticator.java 5d33f22 
>   geode-core/src/test/java/templates/security/DummyAuthorization.java fe8e908 
>   geode-core/src/test/java/templates/security/FunctionSecurityPrmsHolder.java 76827bb 
>   geode-core/src/test/java/templates/security/LdapUserAuthenticator.java db55219 
>   geode-core/src/test/java/templates/security/PKCSAuthInit.java d43b78e 
>   geode-core/src/test/java/templates/security/PKCSAuthenticator.java d3610c4 
>   geode-core/src/test/java/templates/security/PKCSPrincipal.java 563689b 
>   geode-core/src/test/java/templates/security/PKCSPrincipalTest.java PRE-CREATION 
>   geode-core/src/test/java/templates/security/UserPasswordAuthInit.java f4b6eec 
>   geode-core/src/test/java/templates/security/UsernamePrincipal.java 739dd52 
>   geode-core/src/test/java/templates/security/UsernamePrincipalTest.java PRE-CREATION 
>   geode-core/src/test/java/templates/security/XmlAuthorization.java 1ed0142 
>   geode-core/src/test/java/templates/security/XmlErrorHandler.java 5da8e09 
>   geode-core/src/test/resources/com/gemstone/gemfire/codeAnalysis/sanctionedSerializables.txt f3c1c5d 
> 
> Diff: https://reviews.apache.org/r/44402/diff/
> 
> 
> Testing
> -------
> 
> precheckin
> 
> 
> Thanks,
> 
> Kirk Lund
> 
>