Merge lp:~tartley/canonical-identity-provider/rm-deactivated-check into lp:canonical-identity-provider/release

Proposed by Jonathan Hartley
Status: Merged
Approved by: Jonathan Hartley
Approved revision: no longer in the source branch.
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: lp:~tartley/canonical-identity-provider/rm-deactivated-check
Merge into: lp:canonical-identity-provider/release
Prerequisite: lp:~tartley/canonical-identity-provider/rm-no-verified-address-para
Diff against target: 99 lines (+20/-30)
2 files modified
src/api/v20/handlers.py (+13/-17)
src/api/v20/tests/test_handlers.py (+7/-13)
To merge this branch: bzr merge lp:~tartley/canonical-identity-provider/rm-deactivated-check
Reviewer Review Type Date Requested Status
Matias Bordese (community) Approve
Review via email: mp+377387@code.launchpad.net

Commit message

Unconditionally error if "not account.can_reset_password"

Don't try to special-case "deactivated" accounts, since we cannot reach
this code with that account.status value.

Don't special case "suspended" accounts, since that is the only
status value which can reach this bit of code.

Instead, just unconditionally refuse to send the password reset
and return an error.

There was a test of special-case for "deactivated" accounts,
but to reach that code, the test monkey-patched to create an
impossible situation. Without that patching, the test now
demonstrates what actually happens (both before this change
and after it) - i.e. a deactivated account can send a password
reset email.

Add a comment about test assertions using mock_logging,
which actually do nothing.

Fix a few comment typos.

To post a comment you must log in.
Revision history for this message
Jonathan Hartley (tartley) wrote :

diff comments for reviewers

Revision history for this message
Matias Bordese (matiasb) wrote :

LGTM (don't forget to change your commit message!).

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/api/v20/handlers.py'
2--- src/api/v20/handlers.py 2020-01-09 19:14:26 +0000
3+++ src/api/v20/handlers.py 2020-01-09 19:14:26 +0000
4@@ -221,7 +221,7 @@
5 lp_account = get_lp_account_by_openid(identifier)
6 except LaunchpadAPIError as e:
7 # It's only fine to return the raw LP error here because
8- # the endpoint alredy requires privileged access.
9+ # the endpoint already requires privileged access.
10 return errors.SERVER_ERROR(lp_error=str(e))
11
12 # This condition will evolve as LP gets more permissive about renames
13@@ -285,20 +285,16 @@
14 return response
15
16 if not account.can_reset_password:
17- if account.status == AccountStatus.SUSPENDED:
18- # log why email was not sent
19- condition = ("account '%s' is not active" %
20- account.displayname)
21- logger.debug("PasswordResetTokenHandler.create: email was "
22- "not sent out because %s" % condition)
23- return errors.ACCOUNT_SUSPENDED()
24- elif account.status == AccountStatus.DEACTIVATED:
25- # log why email was not sent
26- condition = ("account '%s' is not active" %
27- account.displayname)
28- logger.debug("PasswordResetTokenHandler.create: email was "
29- "not sent out because %s" % condition)
30- return errors.ACCOUNT_DEACTIVATED()
31+ # i.e. If account.status in {Suspended, Deleted}
32+ logger.debug(
33+ "PasswordResetTokenHandler.create: email was not sent out "
34+ "because account '%s' is '%s'"
35+ % (account.displayname, account.status)
36+ )
37+ # Assume account must be Suspended.
38+ # It would be impossible to get here with Deleted accounts
39+ # since they have no email addresses.
40+ return errors.ACCOUNT_SUSPENDED()
41
42 # account_email is populated from our database.
43 # It is safe to use to generate tokens, send emails, etc.
44@@ -311,7 +307,7 @@
45 account_email = matching_validated.first().email
46 else:
47 # provided_email doesn't match any of this account's validated
48- # email addresses. So get an account email address from our db.
49+ # email addresses. So get an account email address from our DB.
50 # This getter falls back through preferred, validated, new.
51 preferred = account.preferredemail
52 if preferred:
53@@ -320,7 +316,7 @@
54 if not account_email:
55 # This should be impossible. We found this account using
56 # 'provided_email', but now we're finding the account has no
57- # useable email addresses.
58+ # usable email addresses.
59 return errors.CAN_NOT_RESET_PASSWORD()
60
61 tokens = AuthToken.objects.filter(
62
63=== modified file 'src/api/v20/tests/test_handlers.py'
64--- src/api/v20/tests/test_handlers.py 2020-01-09 19:14:26 +0000
65+++ src/api/v20/tests/test_handlers.py 2020-01-09 19:14:26 +0000
66@@ -1751,6 +1751,10 @@
67 'extra': {}})
68
69 condition = "account '%s' is not active" % self.account.displayname
70+ # XXX jhartley 2020-01-09 The following call does nothing.
71+ # (If it worked, it would fail. This logging has changed.)
72+ # It just calls a non-existant method on a Mock. The same seems true
73+ # of all other assertions using 'mock_logger' in this module.
74 self.mock_logger.debug("PasswordResetTokenHandler.create: email was "
75 "not sent out because %s" % condition)
76
77@@ -1758,19 +1762,9 @@
78 self.account.status = AccountStatus.DEACTIVATED
79 self.account.save()
80
81- name = 'identityprovider.models.account.Account.can_reset_password'
82- with patch(name, False):
83- content = self.do_post(data=self.data, status_code=403)
84-
85- self.assertEqual(content, {
86- 'message': 'Your account has been deactivated. To reactivate it, '
87- 'please reset your password',
88- 'code': 'ACCOUNT_DEACTIVATED',
89- 'extra': {}})
90-
91- condition = "account '%s' is not active" % self.account.displayname
92- self.mock_logger.debug("PasswordResetTokenHandler.create: email was "
93- "not sent out because %s" % condition)
94+ result = self.do_post(data=self.data, status_code=201)
95+
96+ self.assert_email_used(result, self.account.preferredemail.email)
97
98 def assert_email_used(self, result, expected_email):
99 # Then the response contains the user's stored email.