ofbiz-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Deepak Dixit <deepak.di...@hotwaxsystems.com>
Subject Re: svn commit: r1813964 - /ofbiz/ofbiz-framework/trunk/framework/entity/src/main/java/org/apache/ofbi z/entity/util/EntityCrypto.java
Date Wed, 08 Nov 2017 04:45:57 GMT
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.roux@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/
>> shiro/crypto/CryptoException.html
>>
>> On Tue, Nov 7, 2017 at 2:10 PM, Jacques Le Roux <
>> jacques.le.roux@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.roux@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.roux@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, deepak@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 {
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message