Thanks Deepak!
Jacques
Le 09/11/2017 à 06:47, Deepak Dixit a écrit :
Done at r#1814704
Thanks & Regards
--
Deepak Dixit
www.hotwaxsystems.com
www.hotwax.co
On Wed, Nov 8, 2017 at 10:15 AM, Deepak Dixit <
deepak.di...@hotwaxsystems.com> wrote:
Thanks Jacques and Jacopo for details,
Sure Jacques, I'll do necessary changes.
Thanks & Regards
--
Deepak Dixit
www.hotwaxsystems.com
www.hotwax.co
On Tue, Nov 7, 2017 at 11:29 PM, Jacques Le Roux <
jacques.le.r...@les7arts.com> wrote:
Hi Jacopo,
Thanks for the explanation about CryptoException, this was really not
obvious to me, nice piece of code BTW.
Then I think we should revert my change and document about the
CryptoException to clarify in case another RuntimeException is crossed.
Will you handle it Deepak?
Jacques
Le 07/11/2017 à 15:12, Jacopo Cappellato a écrit :
Hi Jacques,
the pattern/best practice you are describing is definitely a valid one,
thanks for your explanations.
However, in this specific case, re-throwing the RuntimeException would
not
work because when the field is encrypted with the old algorithm (3-DES),
the new Shiro code will fail to decrypt it (using AES) and then it will
throw an org.apache.shiro.crypto.CryptoException [*] that is a Runtime
Exception. For backward compatibility we want instead to catch the
exception and decrypt the code using the old algorithm.
So we should at least manage the CryptoException or use the code
originally
committed by Deepak (just in case that there are other RuntimeExceptions
that may be thrown under similar circumstances).
As I said earlier, this only applies to this specific use case: the best
practice you have mentioned is still valid in general.
Kind regards,
Jacopo
[*]
https://shiro.apache.org/static/1.2.3/apidocs/org/apache/shi
ro/crypto/CryptoException.html
On Tue, Nov 7, 2017 at 2:10 PM, Jacques Le Roux <
jacques.le.r...@les7arts.com> wrote:
Hi Deepak,
The problem as I see it is that if a RuntimeException is thrown by the
1st
version of doDecrypt() then maybe the 2nd version of doDecrypt() will do
so. Maybe I'm wrong here, I did not check each method body.
Actually I think it's more a theoretical issue than a real one because
among the possible the possible RuntimeException subclasses[1], even if
I
did not check them all, I hardly see one being thrown here. But maybe a
NPE
in some cases? In another word is more to follow a pattern and use it
everywhere in our code than about this peculiar issue. You may revert if
you want but beware of NPE ;)
Jacques
[1] https://docs.oracle.com/javase/7/docs/api/java/lang/RuntimeE
xception.html
Le 07/11/2017 à 11:19, Deepak Dixit a écrit :
Hi Jacques,
I think we don't need to handle the RuntimeException, as doDecrypt
fails
to
decrypt then it will try with old pattern, I think this is for backward
compatibility.
Here is the comment from code
try using the old/bad hex encoding approach; this is another path the
code
may take, ie if there is an exception thrown in decrypt
Thanks & Regards
--
Deepak Dixit
www.hotwaxsystems.com
www.hotwax.co
On Fri, Nov 3, 2017 at 5:15 PM, Jacques Le Roux <
jacques.le.r...@les7arts.com> wrote:
Hi Deepak,
Actually the rule is quite simple. If, for a reason, you decide to
catch
a
java.lang.Exception, you need to catch before the
java.lang.RuntimeException.
This is because java.lang.RuntimeException is a java.lang.Exception so
you
would hide possible runtime issues by only catching Exception
https://www.tutorialspoint.com/java/images/exceptions1.jpg
HTH
Jacques
Le 03/11/2017 à 11:17, Deepak Dixit a écrit :
Thanks Jacques,
I was engage in other work so did not get a chance to test your
suggestion... :)
Thanks & Regards
--
Deepak Dixit
www.hotwaxsystems.com
www.hotwax.co
On Fri, Nov 3, 2017 at 3:19 PM, Jacques Le Roux <
jacques.le.r...@les7arts.com> wrote:
Done at r1814155
Jacques
Le 01/11/2017 à 14:08, Jacques Le Roux a écrit :
Hi Deepak,
It's minor, but instead of hiding a possible RuntimeException by
catching
Exception here I'd rather follow this FindBugs advice
------------------------------
This method uses a try-catch block that catches Exception objects,
but
Exception is not thrown within the try block, and RuntimeException
is
not
explicitly caught. It is a common bug pattern to say try
{ ... } catch (Exception e) { something } as a shorthand for
catching a
number of types of exception each of whose catch blocks is
identical,
but
this construct also accidentally catches RuntimeException as well,
masking
potential bugs.
A better approach is to either explicitly catch the specific
exceptions
that are thrown, or to explicitly catch RuntimeException exception,
rethrow
it, and then catch all non-Runtime Exceptions, as shown below:
try { ... } catch (RuntimeException e) { throw e; } catch (Exception
e) {
... deal with all ...}
------------------------------
I suggest to use this late solution, as it has for example been done
for
GroovyUtil.java in r1812059
http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/
framework/base/src/main/java/org/apache/ofbiz/base/util/
GroovyUtil.java?r1=1812059&r2=1812058&pathrev=1812059
Thanks
Jacques
Le 01/11/2017 à 11:43, dee...@apache.org a écrit :
Author: deepak
Date: Wed Nov 1 10:43:14 2017
New Revision: 1813964
URL: http://svn.apache.org/viewvc?rev=1813964&view=rev
Log:
Fixed: doDecrypt method may return ClassNotFoundException,
BadPaddingException,so instead of handling GeneralException used
Exception
class to handle all exception
Modified:
ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/
org/apache/ofbiz/entity/util/EntityCrypto.java
Modified: ofbiz/ofbiz-framework/trunk/fr
amework/entity/src/main/java/
org/apache/ofbiz/entity/util/EntityCrypto.java
URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/fra
mework/entity/src/main/java/org/apache/ofbiz/entity/util/
EntityCrypto.java?rev=1813964&r1=1813963&r2=1813964&view=diff
============================================================
==================
--- ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/
org/apache/ofbiz/entity/util/EntityCrypto.java (original)
+++ ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/
org/apache/ofbiz/entity/util/EntityCrypto.java Wed Nov 1 10:43:14
2017
@@ -124,7 +124,7 @@ public final class EntityCrypto {
public Object decrypt(String keyName, EncryptMethod
encryptMethod,
String encryptedString) throws EntityCryptoException {
try {
return doDecrypt(keyName, encryptMethod,
encryptedString,
handlers[0]);
- } catch (GeneralException e) {
+ } catch (Exception e) {
Debug.logInfo("Decrypt with DES key from standard
key
name
hash failed, trying old/funny variety of key name hash", module);
for (int i = 1; i < handlers.length; i++) {
try {
