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/ >> shiro/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 { >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >
