bpo-29750: support non-ASCII passwords in smtplib by Windsooon · Pull Request #8938

bpo-29750: support non-ASCII passwords in smtplib by Windsooon · Pull Request #8938 · python/cpython

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.

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?

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

Aug 26, 2018

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


		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.

		' 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

david mannequin mentioned this pull request

Apr 30, 2022