On 21/03/2013 07:56, Brian Burch wrote: > On 20/03/13 10:17, bugzi...@apache.org wrote: >> https://issues.apache.org/bugzilla/show_bug.cgi?id=54729 > > Sorry I could not reply more quickly, Mark, but I am currently in > Australia and I am probably asleep while you are working.
Not a problem. One of the reasons the ASF insists on using mailing lists rather than teleconferences is to ensure everyone in all timezones has an equal chance to participate. > Judging by the flurry of activity in this area, I conclude my change is > no longer necessary and you might want to close this bug report. You know that better than I. My understanding from reading your report was that there were a number of issues of which Base64 was just one and one that was making fixing the others look a little ugly. > Just for completeness, I'll briefly answer your comments and the later > one from Konstantin. > >> --- Comment #1 from Mark Thomas <ma...@apache.org> --- >> Regarding your points: >> 1. Clarity vs efficiency is always a trade off. It depends how much >> efficiency >> has been lost. Do you have any performance numbers? > > You were quick to write a performance test! Given more time, I would > have explained that I (like Konstantin) did not like the conversion from > MessageBytes to a StringReader, and did not like the way I had created a > second StringReader to parse the decoded the Base64 blob. However, I > felt the performance hit would be small relative to the original > implementation, although it would create extra short-lived instances. > > My earlier approach had attempted to push the MessageBytes into the new > HttpParser method, but that meant the parsing logic for BASIC became > very different to that of DIGEST. I couldn't see a way to square the > circle, so I trashed it and started again on a more harmonious path. The conversions aren't ideal but I think they are a price worth paying for not having to maintain another Base64 encoder/decoder. >> 2. I don't see a test case either. I'd rather get any bugs in the >> decoder fixed >> than put sticking plasters over other bits of code. > > I've worked with Base64 implementations a lot in the past, in many > different languages. However, the last time I looked (showing my age!) > the java class was in a sun.* package and well worth avoiding. When I > researched the apache commons version, I did not realise it had been > superseded by javax.xml.bind.DatatypeConverter. > > I've just noticed the discussion on the dev list "Use of JAXB api to > process Base64 in Tomcat 7 and 8". I understand the arguments presented > to not use this converter, but if you back out your change and reinstate > org.apache.catalina.util.Base64, then we just go back to where I started > with a broken implementation and no unit tests.... I have a plan B in mind for that. More details on that thread. >> 3. I'd prefer to do this first. That code is only still in the code base >> because it support ByteChunk / CharChunk which should allow a more >> efficient >> interface with the rest of the Tomcat code base. It is probably time >> that that >> assumption is tested. > > I understand. However, the two decode methods look superficially quite > similar to me. There must be scope for refactoring them, writing a > proper unit test suite, and fixing the bugs that I wrapped in my > suggested change to Basic Authenticator. That is a fair amount of work > to achieve little more than just copying another implementation and add > some novel method signatures. Wrappering a robust implementation feels > like a better approach to me. +1. I'm all for code re-use. >> 4. It is very unlikely we will be adding Commons Codec as a dependency to >> Tomcat. We may do an svn copy and rename much like we have done for >> FileUpload >> but the usefulness of that depends on the ByteChunk/CharChunk issues >> in 3. For >> FileUpload I am looking at replacing the decoder with the JVM >> implementation. > > I didn't follow your FileUpload change closely, especially before you > switched to the DatatypeConverter. Did you basically clone the commons > class? If yes, surely it would be better to put that code in tomcat.util > - FileUpload could use it from there and I suspect the > ByteChunk/CharChunk users would only need some pre/post processing. The Base64 decoder in FileUpload was copied from Geronimo to Commons and then I updated Tomcat's copy of FileUpload. That Base64 code has issues of it's own. Looking for a solution to that prior to the 7.0.39 release was co-incidental with you looking at Base64 for BASIC auth. > It seems to me there are enough developers (in the same time zone) > working on these issues. I don't have a lot of time to spend on tomcat > at the moment, and it is fairly disheartening to find my efforts in what > seemed to be a quiet backwater have been washed away by a flood. I'll > step back until the dust settles! Let me know if I can help. I'm sorry you feel that your efforts have been wasted. My intention was to get you a robust Base64 implementation to work with for the other improvements you had in mind for BASIC auth. If there are other improvements you have in mind for BASIC auth then your help there would be much appreciated. Hopefully, by the time you wake up the Base64 issue will have a solution everyone can agree with. Cheers, Mark --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
