On 6 July 2015 at 18:14, Andrew Stitcher <astitc...@redhat.com> wrote: > On Mon, 2015-07-06 at 17:48 +0100, Robbie Gemmell wrote: >> ... >> The old toggle only used to define whether sasl was required or not >> (which it historically was once you enabled the sasl layer, and the >> toggle was never implemented in proton-j), whereas IIRC the new >> 'requireAuth' governs that but also whether ANONYMOUS is allowed or >> not when a SASL layer is used, is that correct? > > That is true, but I think it actually more useful to be able to select > authenticated or not compared to using SASL or not (because ANONYMOUS is > unauthenticated but uses SASL). >
I wasn't arguing one way or another what is better here, I was just pointing out the difference given its important to this change-right-before-release and that proton-j doesn't have an updated SASL API/Impl to go along with it. > The C implementation does the actual enforcement when it reads the AMQP > header, which would obviously be a significant change to the Java > implementation, but I really do think gives a more satisfactory user > result. > >> Given the older SASL >> API means much of the SASL work including mechansim selection happens >> outside/before proton-j and you get as far as making a connection, >> isnt that going to be a bit of a pain to implement and use? > > Well I obviously disagree, else I would have implemented the C SASL > support like this! If it wasnt clear above, I only meant it might be pain to implement and use isAuthenticated right now *given the older SASL API/Impl* that currently still exists in proton-j and presumably isnt going away before the next release. > > TBH, I think that using the same API in Java would make sense along with > an extra "plugin API" to allow implementing mechanisms (assuming there > is no external Java library like Cyrus SASL that can do the work for > us). > > But yes that is new work, probably not going to happen before Proton > 0.11. >> > >> Might it be simpler to use allowSkip() until such time the wider SASL >> API is actually implemented, keep the default behaviour as it was >> previously (so people will be less likely to need to use the behaviour >> toggle and less likely to care what it is called), and update >> whichever reactor tests that needed the change to configure it that >> way in the meantime? > > I'm agnostic on this point really, I'd just like to ensure that the C > and Java APIs actually converge in the shortest time scale. My comment above was coming from an impression that the change here wouldn't be close to fully aligning them, meaning further change and probable breakage would be required down the line. If thats the case I would tend towards leaving the existing default behaviour until such time a viable attempt at converging them is actually being made, in which case such change and breakage would be better warranted and rewarded. > The > alternative would be to stop testing them with the same test code - > there are already far too many specific tests for Java or Windows in the > tests. > >From my impression jsut as many of the specific tests are C or Linux specific given they often [now] test things that simply haven't been implemented elsewhere yet :) I'll admit I have writetn some Java specific tests in actual Java on occasion....sometimes because I burned a lot of time first trying to write it in python and found the test shim stuff entirely masked the problem so I actually couldn't, and others just because it was a known handful of minutes as a direct pure unit test versus indeterminate time working out how the to do it far less directly via the python tests that I simply justify at the time.
