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