Andrew Vant <ajv...@gmail.com> added the comment: Here's a second version of the previous patch taking into account the errors Antoine noticed and some odds and ends from the other comments. Specifically:
Comments fixed and tabs (I think...I hope...) all removed. Added explicit skip to test_nntplib when attempting to test starttls using a server that doesn't support it. Exceptions raised when issuing STARTTLS for: Authorization already done, TLS already active, server not advertising STARTTLS. Method moving: I'm not sure why the first patch seemed to think I was e.g. moving getwelcome (I wasn't, as far as I knew), but this time I put the new functions at the end of the class and it's no longer doing it. (still a bit of a mess for the code moved from __init__ to login, starttls, and readermode though) Selftest cli option: I threw out what I did before (as pointed out, it didn't work anyway) and added a -l flag to use SSL. It uses port 563 for the test only if the user hasn't asked for a non-default port. I'd like to address some of the objections other users have to the way I implemented it the first time. NNTPS and STARTTLS separation: One objection noted that nntps and STARTTLS support are really separate things and should be handled as such. Actually they're probably right. I did both at once because I view them as simply two ways of performing the same function, as someone mentioned earlier in the thread. Unnecessary Method Separation: One objection noted that separating out login and starttls was unnecessary and the calling program shouldn't have to worry about these steps. I disagree; I think the caller should be able to choose when and if to log in or engage encryption. (in fact I think usenetrc should be false by default for this reason, but I figure that would break backwards compatibility for programs that rely on it being true by default, and I'm not sure what the rules are regarding this) More significant than my own potentially newbie-ish opinion is that the RFC suggests as a valid use case the idea of a client starting up TLS or authentication in reaction to a 483 command response, rather than right off the bat. I'm pretty sure this is impossible under the current setup, where login/encryption happens only at initialization and there's no method exposed to do it later. If I'm going to get overruled on this, I guess the other option is to add a starttls argument to the constructors. If so, I'd suggest it have settings to forbid starttls, use it if it's advertised, or insist on it (by raising an exception if it can't be used). Perhaps false/none/true respectively. Existing issue for AUTHINFO and MODE READER: One objection noted there's already an issue open for them (#10287). Which is true, but the AUTHINFO and MODE READER changes were necessary to make STARTTLS work as intended. (in my opinion, I suppose) So I did them as I went along. I'm not sure if I violated custom there, but my apologies if so. At least having them functioned out will make most of what's mentioned in 10287 easier to fix, I suppose. Randomness: BTW, I've been maintaining the readermode_afterauth thing because of the comment next to it, but it sounds from that other issue like it's not supposed to be there at all...which kind of restores my faith in the universe since it smelled bad from the start to me. ---------- Added file: http://bugs.python.org/file19519/python_nntp_ssl_patch2.diff _______________________________________ Python tracker <rep...@bugs.python.org> <http://bugs.python.org/issue1926> _______________________________________ _______________________________________________ Python-bugs-list mailing list Unsubscribe: http://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com
