All commit authors signed the Contributor License Agreement.

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

The problem is that "SMTPAuthenticationError" does not mean "bad password". Other sorts of errors can cause that error to be raised, which is why we try other auth methods when we get it. I agree that that can result in confusing results though. Maybe you can figure out how we could differentiate bad credentials from other auth errors, and introduce an SMTPBadCredentials error that could cause a fail fast?

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

Hello

Thanks for the review, unsure on the SMTPBadCredentials error. Why multiply entities while by definition the software can't clear this problem (RFC 4954 states that the user should do the work here)

OK, I've looked at the response codes in the RFC again (it has been years since I last did and I didn't actually write the code in question, though I was the one that committed it). It looks like 454 (temporary failure) and 535 (bad credentials) are both things that should cause an immediate exit. But there are others (specifically 534, but arguably 432 and 500 as well) where we should try other auth methods, as we currently do.

So, looking at the existing code in more detail, what I suggest we do here is to add the response code to the SMTPAuthenticationError as an attribute, and then check that attribute in the login method and do an immediate exit if it is 454 or 535, and keep the existing logic otherwise. Or flip that and only continue if the code is 534, 432, or 500. I'd be OK with either of those. Either list should be made a module constant, I think, for easy modification in the future.

Note that I'm not really happy with the existing logic: if we ever end the loop without either a success or an error, last_exception will be uninitialized. We also only show the last exception even if there were more than one, and that isn't optimal either. We should really show all of them. There might be an example elsewhere in the standard library where we do something like that, but I can't remember where (or maybe it was one of my own projects where I did that...can't remember, unfortunately). If we don't have an accepted way of returning multiple exceptions I'd just leave that part of the code the way it is, but we should fix the loop-exit bug.
Raise an SMTPAuthenticationError that says "no auth method succeeded", perhaps?

Note that your PR will need tests and other bits (news entry...I forget if there is anything else) before it is complete.

Actually, I wonder if it would be better to retry on 454 as well? What if the temp failure only applies to one auth method supported by the server? I'm not sure it would ever matter in practice, though, so an early exit there would be fine with me. What do you think? (Thanks for working on this, by the way.)

If I understand your correctly, you'd prefer that it was more along the lines of
if code in (535, 454): raise e
So not test the extended code at all - contrary to your first advice that I tried to follow by adding
e.smtp_error[:5] == b'5.7.8

About the wisdom of aborting on temporary error, I have no personal experience of this kind of error and no considerate opinion. IMO 99,73% of errors in this loop will come of user errors (bad password). Cosmic rays happens but they are dwarfed by human errors. And the other failure case that is likely to happen, bad server configuration, another human error, is not a temporary condition - at a computer time scale at last. On one hand, aborting on a temporary condition can lead to failures in the field that could have been avoided. On the other hand, fail fast, fast early is a very sound general principle.

About 'add the response code to the SMTPAuthenticationError as an attribute' I don't understand at all, when I write e.smtp_code in the patch, e is a SMTPAuthenticationError object, and smtp_code is an attribute, so what's to add exactly ?

About returning an uninitialized object, it seems very unlikely to happen, since the auth method ends like this:
if code in (235, 503):
return (code, resp)
raise SMTPAuthenticationError(code, resp)
so if you want an 'other case' exception, it should rather be SMTPProgrammingError in my opinion.

If we can agree on a solution, I'm game for the tests (words that I could regret...)

About the news, IMO adding 'fixing a very minor bug in smtp authentication' is a waste of good electrons in a news file; from what I see, another solution is to set a label (skip_news IIRC) on the issue to say that this does not need a news entry, but I don't have the rights to do that.