bpo-29750: support non-ASCII passwords in smtplib #8938

Windsooon · 2018-08-26T07:29:07Z

I'm not sure should I add an extra test for the patch?

https://bugs.python.org/issue29750

taleinat

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.

taleinat · 2018-08-26T07:38:02Z

Lib/test/test_smtplib.py

@@ -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密码')


IMO we should keep the existing test case with an ASCII-only password, and add at least one more case with additional ones.

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')

taleinat · 2018-08-26T07:43:14Z

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.

Windsooon · 2018-08-26T08:52:46Z

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?

taleinat · 2018-08-26T14:13:13Z

@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.

Windsooon · 2018-09-02T12:14:43Z

@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.

william-gr · 2019-01-07T13:26:54Z

Ping. Is there anything we can do to help get this resolved? Thanks

taleinat

This need some changes IMO, but nothing major.

taleinat · 2019-01-09T20:37:20Z

Lib/test/test_smtplib.py

+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]


This should be renamed to something like "valid_sim_auths".

Did you mean rename sim_auth_lists to valid_sim_auths?

taleinat · 2019-01-09T20:38:12Z

Lib/smtplib.py

@@ -417,6 +417,7 @@ def getreply(self):

    def docmd(self, cmd, args=""):
        """Send a command, and return its response code."""
+


Please remove this empty line to maintain the convention used in this file.

taleinat · 2019-01-09T20:38:57Z

Lib/test/test_smtplib.py

-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')


Use "invalid" rather than "nonvalid", since "invalid" is actually a word.

taleinat · 2019-01-09T20:53:10Z

Lib/test/test_smtplib.py

@@ -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])


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.

taleinat · 2019-01-09T20:56:12Z

Lib/test/test_smtplib.py

-                resp = smtp.auth(mechanism, getattr(smtp, method))
-                self.assertEqual(resp, (235, b'Authentication Succeeded'))
-                smtp.close()
+                for sim_auth in sim_auth_lists:


The with self.subTest(...) line should be in the internal loop, i.e. with self.subTest(mechanism=mechanism, sim_auth=sim_auth):

bedevere-bot · 2019-01-09T20:57:49Z

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.

taleinat · 2019-01-09T20:58:36Z

Also, please add a NEWS entry. You're welcome to try the new "blurb-it" tool.

csabella · 2019-05-31T11:09:14Z

@Windsooon, please address the last review from @taleinat. Thanks!

Windsooon · 2019-05-31T11:17:39Z

Sure, I kinda forgot this PR, sorry :D

Windsooon requested a review from a team as a code owner August 26, 2018 07:29

the-knights-who-say-ni added the CLA signed label Aug 26, 2018

bedevere-bot added the awaiting review label Aug 26, 2018

taleinat reviewed Aug 26, 2018

View reviewed changes

fsrrt approved these changes Aug 26, 2018

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Aug 26, 2018

taleinat changed the title ~~fixed issue-33741~~ bpo-29750: support non-ASCII passwords in smtplib Aug 26, 2018

bpo-29750: support non-ASCII passwords in smtplib

58e0345

Windsooon force-pushed the bpo-33741 branch from 73ef102 to 58e0345 Compare September 2, 2018 12:09

taleinat requested changes Jan 9, 2019

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Jan 9, 2019

Windsooon closed this Aug 1, 2019

Windsooon deleted the bpo-33741 branch August 1, 2019 04:03

Windsooon mentioned this pull request Aug 1, 2019

bpo-29750: support non-ASCII passwords in smtplib #15064

Closed

david mannequin mentioned this pull request Apr 30, 2022

smtplib doesn't handle unicode passwords #73936

Open

		@@ -417,6 +417,7 @@ def getreply(self):

		def docmd(self, cmd, args=""):
		"""Send a command, and return its response code."""

Uh oh!

bpo-29750: support non-ASCII passwords in smtplib #8938

bpo-29750: support non-ASCII passwords in smtplib #8938

Uh oh!

Conversation

Windsooon commented Aug 26, 2018 • edited by taleinat Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taleinat left a comment

Choose a reason for hiding this comment

Uh oh!

taleinat Aug 26, 2018

Choose a reason for hiding this comment

Uh oh!

taleinat Aug 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

taleinat commented Aug 26, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Windsooon commented Aug 26, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taleinat commented Aug 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Windsooon commented Sep 2, 2018

Uh oh!

william-gr commented Jan 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

taleinat left a comment

Choose a reason for hiding this comment

Uh oh!

taleinat Jan 9, 2019

Choose a reason for hiding this comment

Uh oh!

Windsooon Jan 26, 2019

Choose a reason for hiding this comment

Uh oh!

taleinat Jan 9, 2019

Choose a reason for hiding this comment

Uh oh!

taleinat Jan 9, 2019

Choose a reason for hiding this comment

Uh oh!

taleinat Jan 9, 2019

Choose a reason for hiding this comment

Uh oh!

taleinat Jan 9, 2019

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jan 9, 2019

Uh oh!

taleinat commented Jan 9, 2019

Uh oh!

csabella commented May 31, 2019

Uh oh!

Windsooon commented May 31, 2019

Uh oh!

Uh oh!

Windsooon commented Aug 26, 2018 •

edited by taleinat

Loading

taleinat Aug 26, 2018 •

edited

Loading

taleinat commented Aug 26, 2018 •

edited by bedevere-bot

Loading

Windsooon commented Aug 26, 2018 •

edited by bedevere-bot

Loading

taleinat commented Aug 26, 2018 •

edited

Loading

william-gr commented Jan 7, 2019 •

edited

Loading