On Nov 25, 2009, at 6:05 PM, Alexander Lamaison wrote: > 2009/11/25 Dave McCaldon <[email protected]>: >> The patch adds an error code, LIBSSH2_ERROR_AUTHORIZATION_FAILED (which is >> actually an alias for LIBSSH2_ERROR_PUBLICKEY_UNVERIFIED, returned in >> libssh2_userauth_publickey_fromfile() to maintain backward compatibility) >> which is returned in the case of SSH_MSG_USERAUTH_FAILURE (this is the same >> as the logic in libssh2_userauth_publickey_fromfile() that handles an >> unrecognized public key). > > Something about the idea of aliasing the error codes bothers me. > Currently, they uniquely identify the type of error that occurred and > so can be used as an index into, for instance, a lookup table of error > messages.
I understand, this is how I did it the first time I submitted it! > Or are you suggesting that all authentication failures (as opposed to > other errors such as sending failures) should share an error code > regardless of the authentication method used? As I understood it, this was Peter's suggestion. > If so, I may agree with > this as I see the logic that these errors are the authentication > results and therefore of the same 'type'. However, in this case we > should remove all references to LIBSSH2_ERROR_PUBLICKEY_UNVERIFIED, > except the #define for backward-compatibility, and replace them with > LIBSSH2_ERROR_AUTHORIZATION_FAILED. Also, > userauth_keyboard_interactive() needs a similar else block adding and > maybe LIBSSH2_ERROR_PUBLICKEY_UNRECOGNISED needs replacing as well. I'm not sure of the differences in the semantics of LIBSSH2_ERROR_PUBLICKEY_UNVERIFIED and LIBSSH2_ERROR_PUBLICKEY_UNRECOGNISED, which is why I had initially created a new "login failed" error. I'm wary of changing error codes like this, I don't want to break any existing code out there that expects to know the difference between these two. To be honest, the original patch was better in that it was more fine-grained and you could tell the difference between the errors -- maybe the calling application would like to be able to tell you "login failed -- invalid username/password" vs. "login failed -- invalid public key". Perhaps the best (and hopefully final!) solution would be to simply use the original patch and add the exact same else block for userauth_keyboard_interactive() (I haven't used or looked at this function), with the same new error. In any case, if I can get a final opinion from the group on this, I'm happy to implement it. Thanks. _______________________________________________ libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel
