classification
Title: smtplib: add support for arbitrary auth methods
Type: enhancement Stage: resolved
Components: email, Library (Lib) Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: barry, catalin.iacob, jesstess, python-dev, r.david.murray, sandro.tosi, zvyn
Priority: normal Keywords: patch

Created on 2012-06-06 08:11 by sandro.tosi, last changed 2014-07-03 18:55 by r.david.murray. This issue is now closed.

Files
File name Uploaded Description Edit
smtplib.patch zvyn, 2014-03-03 19:57 Implements new member functions for each auth-method in SMTP. review
smtplibAuthobj.patch zvyn, 2014-03-04 19:33 add support for arbitrary auth methods via authenticate(..., authobject) review
smtplib_auth.patch zvyn, 2014-05-28 22:25 review
smtplib_auth_060614.patch zvyn, 2014-06-06 14:49 review
smtplib_auth.patch r.david.murray, 2014-07-03 18:55 review
Messages (16)
msg162396 - (view) Author: Sandro Tosi (sandro.tosi) * (Python committer) Date: 2012-06-06 08:11
Hello,
I'm writing some tests from an MTA, and so I'm using smtplib. Sadly the login() method doesn't allow to choose the auth method to use (but it's selected from a static list compared with what's advertized from the MTA) while it would be useful to be able to choose the AUTH method to use.
msg162407 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-06-06 13:01
Yes, this is a need I also ran into, but hadn't gotten around to submitting a bug for.  If we are going to do this, though, I think we should do it right and add the ability to support arbitrary login methods.  An example of one way to do this is the imaplib authobj protocol.  

As things are, I wound up implementing XYMCOOKIE login using the even-more-primitive-than-SMTP-cmd-level operation 'do_cmd'.
msg212663 - (view) Author: Milan Oberkirch (zvyn) * Date: 2014-03-03 19:57
I implemented one approach to solve this by writing new member functions for each method (see the patch attached). Bonus: It does not change the usage of login() in any way (which uses the new functions internally).
Another option would be to make those functions private/put them into login() and provide an optional keyarg. Or auth objects, but as far as I can see only 3 methods are relevant so it might be an overkill?

So that's the easy way to fix it, would be glad if it helps!
msg212667 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-03-03 20:33
There is a lot of repeated code in those methods, which becomes a maintenance burden.  That is one reason the imaplib authobj approach seems interesting to me.  Also, going the authobj route would mean imaplib and smptlib had a consistent interface for this, and consistency is good where it makes sense.
msg212668 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-03-03 20:35
Oh, sorry.  I meant to start out that message by saying, thanks for the patch.  Your idea is indeed interesting from the standpoint of not needing to change the rest of the API, but....and then the rest of my message.

In other words, I appreciate your work and your approach, but I think the reasons I outline argue for a different approach.
msg212720 - (view) Author: Milan Oberkirch (zvyn) * Date: 2014-03-04 13:51
I looked into imaplib a bit to see how the problem is solved there; what I observed:
- login() does 'PLAIN' only (and does not use authobj but smtplib would)
- there exists an extra function login_cram_md5() for 'CRAM-MD5'

So I guess the right way to do it would be to write an authenticate() method as in imaplib and use it in new member functions of the form login_[method](). In contrast to imaplib login() could still be used to probe through the three main methods (to avoid breaking backward compatibility).

I'll try to implement this later today, so if you think it's completely dumb and read this before a patch is applied feel free to give me a quick reply to stop me from wasting my time :)
msg212726 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-03-04 15:55
Ah, I was going on imperfect memory of how imaplib worked without looking it up.  And I won't have time to do so today probably.

Probably we should decide on the exact API before you implement anything.
msg212735 - (view) Author: Milan Oberkirch (zvyn) * Date: 2014-03-04 19:33
Maybe I am a bit hyperactive, but I already was at it when receiving your message, so I finished it. Anyhow most of what I have done is completely independent of the API chosen. What I did:
1. I implemented a new authenticate(self, mechanism, authobject) as in imaplib which can be used for all methods
2. I implemented authobjects for the three most important mechanisms
3. login() has a keyarg 'preferred_auth' which is a list of auth-methods (strings)

I think 1. and 2. make sense regardless of how the API should look like in the end, because it's the only way without any code-redundancy. 3 is more a proposal.
msg212739 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-03-04 20:51
OK, that makes sense to me.  I'll take a look at it as soon as I can.
msg218707 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-05-17 16:13
OK, I've finally had time to review this, sorry for the delay.

The impalib mechanism is tailored to how imap works (that's the whole thing about "continuation response".  smtplib auth works a bit differently, and your adaptation looks OK to me.  The accompanying docstrings are a bit confusing, though, since they copy the imaplib language, which isn't entirely applicable.  Instead of talking about a 'continuation response', it should talk about a '334 challenge response', I think.

Also, both the imaplib and smptlib methods are named after the corresponding command names in the protocol.  So for imaplib we have 'authenticate', but for smtplib it should be 'auth'.

I also think we should expose the supported auth methods as part of the public API.

I think exposing the preferred auth list is premature, since if we are going to do that we need to have a way to add elements to that list for the users's own auth methods and we don't have that.  So let's confine this issue to adding the 'auth' method and using it in the login method.

The tests should include a new test of calling the auth method directly.
msg219315 - (view) Author: Milan Oberkirch (zvyn) * Date: 2014-05-28 22:25
Done.
msg219845 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-06-05 20:47
I made some review comments.

Also, we need doc updates, including a what's new entry.
msg219876 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-06-06 12:22
Ah, you are right, I wasn't looking at the full context of the diff when I made the comment about backward compatibility.  So ignore that part :)

On the other hand, exposing perferred_auth on the class would be a simple API for allowing auth extension (since one really needs to subclass to add an authobject, given that it needs access to self.user and self.password).  But, like I said before, that can be a separate issue.
msg219886 - (view) Author: Milan Oberkirch (zvyn) * Date: 2014-06-06 14:49
Here comes the patch implementing your suggestions. Changing the API to make adding new auth methods and still using login() would only require to make preferred_auth accessable as you mentioned. Using custom authobjects is possible with this patch.
msg222205 - (view) Author: Roundup Robot (python-dev) Date: 2014-07-03 18:48
New changeset 42917d774476 by R David Murray in branch 'default':
#15014: Add 'auth' command to implement auth mechanisms and use it in login.
http://hg.python.org/cpython/rev/42917d774476
msg222206 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-07-03 18:55
Thanks, Milan.

I made some tweaks...mostly documentation and code style (your code style wasn't wrong, I just have some slightly different preferences with regards to line folding).  I also eliminated the authobjects test method, since I didn't see that it added anything to duplicate the code from the login method to test them...they get tested via the login methods.  Instead I added the loop to the test for the auth method, so that we test that calling the public API with the auth objects that are documented as part of the public API work.  That is, if someone were to change login such that the method names changed but login still worked, the auth test method will throw an error because the documented method names changed.

I'm attaching the final patch here so you can look at the differences via the reitveld patch-diff function, if you want to.
History
Date User Action Args
2014-07-03 18:55:30r.david.murraysetstatus: open -> closed
files: + smtplib_auth.patch
messages: + msg222206

resolution: fixed
stage: needs patch -> resolved
2014-07-03 18:48:48python-devsetnosy: + python-dev
messages: + msg222205
2014-06-06 14:49:39zvynsetfiles: + smtplib_auth_060614.patch

messages: + msg219886
2014-06-06 12:22:40r.david.murraysetmessages: + msg219876
2014-06-05 20:47:35r.david.murraysetmessages: + msg219845
2014-05-28 22:29:06zvynsetnosy: + jesstess
2014-05-28 22:25:29zvynsetfiles: + smtplib_auth.patch

messages: + msg219315
2014-05-17 16:13:33r.david.murraysetmessages: + msg218707
2014-04-15 17:53:02r.david.murraylinkissue1521196 superseder
2014-03-04 20:51:50r.david.murraysetmessages: + msg212739
2014-03-04 19:33:38zvynsetfiles: + smtplibAuthobj.patch

messages: + msg212735
2014-03-04 15:55:53r.david.murraysetmessages: + msg212726
2014-03-04 13:51:38zvynsetmessages: + msg212720
2014-03-03 20:35:21r.david.murraysetmessages: + msg212668
2014-03-03 20:33:00r.david.murraysetmessages: + msg212667
versions: + Python 3.5, - Python 3.3
2014-03-03 19:57:50zvynsetfiles: + smtplib.patch

nosy: + zvyn
messages: + msg212663

keywords: + patch
2012-06-08 06:59:54catalin.iacobsetnosy: + catalin.iacob
2012-06-06 13:02:19r.david.murraysetnosy: + barry

components: + email
stage: needs patch
2012-06-06 13:01:56r.david.murraysettitle: smtplib: allow to choose auth method on login() -> smtplib: add support for arbitrary auth methods
nosy: + r.david.murray

messages: + msg162407

type: enhancement
2012-06-06 08:11:21sandro.tosicreate