Re: svn commit: r1813964 - /ofbiz/ofbiz-framework/trunk/f
Locked
Re: svn commit: r1813964 - /ofbiz/ofbiz-framework/trunk/f
 
|
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 < [hidden email]> 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, [hidden email] 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/framework/entity/src/main/java/org/apache/ofbiz/entity/util/EntityCrypto.java > URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/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 { > > > > > > > |
Re: svn commit: r1813964 - /ofbiz/ofbiz-framework/trunk/f
Administrator
|
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 < > [hidden email]> 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, [hidden email] 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/framework/entity/src/main/java/org/apache/ofbiz/entity/util/EntityCrypto.java >> URL: http://svn.apache.org/viewvc/ofbiz/ofbiz-framework/trunk/framework/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 { >> >> >> >> >> >> >> |
Re: svn commit: r1813964 - /ofbiz/ofbiz-framework/trunk/f
|
|
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 < [hidden email]> 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 < >> [hidden email]> 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, [hidden email] 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/framework/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 { >>> >>> >>> >>> >>> >>> >>> >>> > |
Re: svn commit: r1813964 - /ofbiz/ofbiz-framework/trunk/f
|
Administrator
|
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/RuntimeException.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 < > [hidden email]> 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 < >>> [hidden email]> 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, [hidden email] 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/framework/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 { >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> |
Re: svn commit: r1813964 - /ofbiz/ofbiz-framework/trunk/f
|
|
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 < [hidden email]> 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 < >> [hidden email]> 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 < >>>> [hidden email]> 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, [hidden email] 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/framework/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 { >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> > |
Re: svn commit: r1813964 - /ofbiz/ofbiz-framework/trunk/f
|
Administrator
|
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 < > [hidden email]> 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 < >>> [hidden email]> 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 < >>>>> [hidden email]> 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, [hidden email] 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/framework/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 { >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> |
Re: svn commit: r1813964 - /ofbiz/ofbiz-framework/trunk/f
|
|
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 < [hidden email]> 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 < >> [hidden email]> 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 < >>>> [hidden email]> 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 < >>>>>> [hidden email]> 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, [hidden email] 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 { >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> > |
Re: svn commit: r1813964 - /ofbiz/ofbiz-framework/trunk/f
|
|
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 < [hidden email]> 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 < > [hidden email]> 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 < >>> [hidden email]> 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 < >>>>> [hidden email]> 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 < >>>>>>> [hidden email]> 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, [hidden email] 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 { >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >> > |
Re: svn commit: r1813964 - /ofbiz/ofbiz-framework/trunk/f
|
Administrator
|
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 < > [hidden email]> 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 < >> [hidden email]> 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 < >>>> [hidden email]> 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 < >>>>>> [hidden email]> 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 < >>>>>>>> [hidden email]> 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, [hidden email] 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 { >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> |
«
Return to OFBiz - Dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |