> On 2011-09-06 18:12:46, rajith attapattu wrote: > > /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java, > > line 38 > > <https://reviews.apache.org/r/1608/diff/1/?file=34085#file34085line38> > > > > I believe you forgot to add ClientConnectionDelegate ? > > I can't seem to find this in the current source tree. > > > > Perhaps some of the missing code from ClientDelegate is moved to this > > class? > > Robbie Gemmell wrote: > Well damn, thatll teach me to say something looks fine without looking at > the diff to make sure its actually what I looked it ;( > > Pretty much all of your comments stem from this one issue as the missing > code will indeed be in the ClientConnectionDelegate, which is used in place > of ClientDelegate for actual client use now as can be seen further down the > class. ClientDelegate is really mainly used in ConnectionTest after this (the > same way ServerDelegate was really only used in ConnectionTest, but the real > broker uses the ServerConnectionDelegate subclass). The diff posted to > ReviewBoard is basically incomplete for some reason. I dont have a copy of > the original patch that I actually looked over a couple weeks ago and Keith > is on holiday just now, but I'm sure he will post the corrected version on > his return next week...until then, no need to test what wont evne compile... > :S
You are both quite correct, I accidentally omitted the new class and the removal of now redundant files from the udiff. Apologies. I have now updated the udiff and believe this should answer Rajith's questions. > On 2011-09-06 18:12:46, rajith attapattu wrote: > > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java, > > line 60 > > <https://reviews.apache.org/r/1608/diff/1/?file=34089#file34089line60> > > > > Why is this code (related to GSSAPI) is removed ? > > > > I don't see this code moved elsewhere either? This will break existing > > functionality :( Moved to ClientConnectionDelegate > On 2011-09-06 18:12:46, rajith attapattu wrote: > > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java, > > line 127 > > <https://reviews.apache.org/r/1608/diff/1/?file=34089#file34089line127> > > > > Removing this without a suitable replacement will break existing > > functionality. > > > > Is this check performed else where? Could you pls help me located it? > > > > This was in place to ensure the client to throw an exception if it's > > not configured to support any of the mechanisms supported by the broker. It > > used to be that we just ignored SASL all together if no matching mechs were > > found. > > > > I'd argue that this is an important check. Could you please explain > > your reasons behind the removal (if the same check is not performed > > elsewhere)? Check now performed with ClientConnectionDelegate. > On 2011-09-06 18:12:46, rajith attapattu wrote: > > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java, > > line 143 > > <https://reviews.apache.org/r/1608/diff/1/?file=34089#file34089line143> > > > > Lines 138 to 141 were required for SASL encryption support. Why is this > > removed ? > > > > Is this taken care of elsewhere? If so my apologies (but could you > > point me to the relevant code?). Moved to ClientConnectionDelegate > On 2011-09-06 18:12:46, rajith attapattu wrote: > > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java, > > line 215 > > <https://reviews.apache.org/r/1608/diff/1/?file=34089#file34089line215> > > > > The user identity when using GSSAPI and External are crucial for ACL > > support. > > > > We have existing customers relying on this feature. > > > > Why was this removed? (Again is there a replacement for this some > > where?) Moved to ClientConnectionDelegate > On 2011-09-06 18:12:46, rajith attapattu wrote: > > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java, > > line 304 > > <https://reviews.apache.org/r/1608/diff/1/?file=34089#file34089line304> > > > > Again this method was used to retrieved the kerberos identity of the > > user. Moved to ClientConnectionDelegate - Keith ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1608/#review1767 ----------------------------------------------------------- On 2011-09-12 12:05:21, Keith Wall wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1608/ > ----------------------------------------------------------- > > (Updated 2011-09-12 12:05:21) > > > Review request for qpid and rajith attapattu. > > > Summary > ------- > > This patch changes the 0-10 code path to create the SASL callback handler > using the CallbackHandlerRegistry. This allows the 0-10 code path to > support SASL mechanisms requiring other callback handlers, such as > CRAM-MD5-HASHED. Support for the sasl_mechs client connection option has > been retained and now applies to the 0-8..0-9-1 code paths too. > > If the user *specifies* a sasl_mechs client connection option the behaviour > of the code is unchanged from the previous version: it restricts the list of > SASL mechanisms in use. > > If the user *does not specify* a sasl_mechs client connection option, the old > code used a hardcoded PLAIN default. This is no longer the case. Now the > client will use the first SASL mechanism from the list > CallbackHandlerRegistry.properties that is also available on the server. > > Removed dead code and strengthen unit tests. > > > > This addresses bug QPID-3415. > https://issues.apache.org/jira/browse/QPID-3415 > > > Diffs > ----- > > > /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/AMQConnectionDelegate_0_10.java > 1169685 > > /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/handler/ConnectionStartMethodHandler.java > 1169685 > > /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.java > 1169685 > > /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/security/CallbackHandlerRegistry.properties > 1169685 > > /trunk/qpid/java/client/src/main/java/org/apache/qpid/client/transport/ClientConnectionDelegate.java > PRE-CREATION > > /trunk/qpid/java/client/src/test/java/org/apache/qpid/client/security/CallbackHandlerRegistryTest.java > PRE-CREATION > > /trunk/qpid/java/common/src/main/java/org/apache/qpid/security/AMQPCallbackHandler.java > 1169685 > > /trunk/qpid/java/common/src/main/java/org/apache/qpid/security/UsernamePasswordCallbackHandler.java > 1169685 > > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ClientDelegate.java > 1169685 > > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/Connection.java > 1169685 > > /trunk/qpid/java/common/src/main/java/org/apache/qpid/transport/ConnectionSettings.java > 1169685 > > /trunk/qpid/java/common/src/test/java/org/apache/qpid/transport/ConnectionTest.java > 1169685 > > /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/client/connection/ConnectionTest.java > 1169685 > > /trunk/qpid/java/systests/src/main/java/org/apache/qpid/test/unit/message/UTF8Test.java > 1169685 > > Diff: https://reviews.apache.org/r/1608/diff > > > Testing > ------- > > Improved unit testing. Ran java, cpp and cpp.ssl profiles. I am not able to > test GSSAPI locally. > > > Thanks, > > Keith > >
