bpo-29750: support non-ASCII passwords in smtplib by Windsooon · Pull Request #8938 · python/cpython (original) (raw)
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation19 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that Unicode with UTF-16 surrogate-pair characters, e.g. '\ud800', cannot be encoded in UTF-8. IMO we should ensure that this code fails with an appropriate, informative error on such passwords.
| @@ -631,7 +631,7 @@ def testLineTooLong(self): |
|---|
| 'Mrs.C@somewhereesle.com':'Ruth C', |
| } |
| sim_auth = ('Mr.A@somewhere.com', 'somepassword') |
| sim_auth = ('Mr.A@somewhere.com', 'somepassword密码') |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should keep the existing test case with an ASCII-only password, and add at least one more case with additional ones.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E.g., add the following after sim_auth = ..., and then modify the test code to try them each in a loop (using with self.subTest() with an informative description).
sim_auth_nonascii = ('Mrs.C@somewhereelse.com', 'contraseña') sim_auth_nonlatin1 = ('Ms.D@example.com', 'p\u03B1sswor\u03B4')
This should reference bpo-29750, rather than bpo-33741 which is a duplicate.
Please use the proper format for the pull request title: it should begin with "bpo-29750: ..."
This does indeed need some more tests, and a NEWS entry.
Thank you @taleinat . Maybe we should do something like
try:
response = encode_base64(initial_response.encode('utf-8'), eol='')
except UnicodeEncodeError:
raise(Error Message)
in every encode method? I would add some tests later. I wonder should I close this pr and create one for bpo-29750?
fixed issue-33741 bpo-29750: support non-ASCII passwords in smtplib
@Windsooon, keep this PR, I've updated it to reference the proper issue.
Such error handling as you suggest seems appropriate.
If identical logic is used in all 3 cases, consider extracting that into a helper function/method, which would also handle errors as you suggested.
@taleinat I add some test cases and I think maybe we can make testAUTH_CRAM_MD5 testAUTH_LOGIN testAUTH_multiple test_auth_function in a for loop test.
Ping. Is there anything we can do to help get this resolved? Thanks
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This need some changes IMO, but nothing major.
| sim_auth_latin = ('Mr.A@somewhere.com', 'somepassword') |
|---|
| sim_auth_nonlatin = ('Ms.D@example.com', 'p\u03B1sswor\u03B4') |
| sim_auth_nonvalid = ('Ms.E@example.com', '\ud800\ud800') |
| sim_auth_lists = [sim_auth_latin, sim_auth_nonlatin] |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be renamed to something like "valid_sim_auths".
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean rename sim_auth_lists to valid_sim_auths?
| @@ -417,6 +417,7 @@ def getreply(self): |
|---|
| def docmd(self, cmd, args=""): |
| """Send a command, and return its response code.""" |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this empty line to maintain the convention used in this file.
| sim_auth = ('Mr.A@somewhere.com', 'somepassword') |
|---|
| sim_auth_latin = ('Mr.A@somewhere.com', 'somepassword') |
| sim_auth_nonlatin = ('Ms.D@example.com', 'p\u03B1sswor\u03B4') |
| sim_auth_nonvalid = ('Ms.E@example.com', '\ud800\ud800') |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use "invalid" rather than "nonvalid", since "invalid" is actually a word.
| @@ -719,7 +723,7 @@ def _auth_plain(self, arg=None): |
|---|
| self.push('535 Splitting response {!r} into user and password' |
| ' failed: {}'.format(logpass, e)) |
| return |
| self._authenticated(user, password == sim_auth[1]) |
| self._authenticated(user, password == self.sim_auth[1]) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using this sim_auth class attribute and changing it in test code is prone to side-effects, such as different tests affecting each other in a way that depends on the order they are run.
IMO it would be clearer if this were just a global variable.
Regardless, the tests should make sure to reset the global variable back to its default value when they are done, using a a helper method (utilizing .addCleanup()) or a context manager.
| resp = smtp.auth(mechanism, getattr(smtp, method)) |
|---|
| self.assertEqual(resp, (235, b'Authentication Succeeded')) |
| smtp.close() |
| for sim_auth in sim_auth_lists: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The with self.subTest(...) line should be in the internal loop, i.e. with self.subTest(mechanism=mechanism, sim_auth=sim_auth):
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.
Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
Also, please add a NEWS entry. You're welcome to try the new "blurb-it" tool.
Sure, I kinda forgot this PR, sorry :D
