Merge lp:~maxiberta/canonical-identity-provider/drop_honeypot_from_delete_account into lp:canonical-identity-provider

Proposed by Maximiliano Bertacchini on 2019-07-05
Status: Merged
Approved by: Maximiliano Bertacchini on 2019-07-05
Approved revision: 1694
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: lp:~maxiberta/canonical-identity-provider/drop_honeypot_from_delete_account
Merge into: lp:canonical-identity-provider
Diff against target: 56 lines (+19/-3)
2 files modified
src/identityprovider/middleware/honeypot.py (+1/-1)
src/identityprovider/tests/test_middleware.py (+18/-2)
To merge this branch: bzr merge lp:~maxiberta/canonical-identity-provider/drop_honeypot_from_delete_account
Reviewer Review Type Date Requested Status
Daniel Manrique 2019-07-05 Approve on 2019-07-05
Review via email: mp+369777@code.launchpad.net

Commit message

Exclude the delete account view from honeypot checks.

Description of the change

This fixes account deletion on browsers with overeager/buggy password managers (such as Chrome/Chromium). The web form still includes the honeypot field ("openid.usernamesecret") to help reproduce the issue, but it's ignored server-side so that the processing can proceed regardless.

The affected URL: /+delete.

Additionally, fixed `HoneypotWhitelistedTestCase`'s setup to actually use the honeypot middleware, otherwise tests would pass unconditionally.

To post a comment you must log in.
Daniel Manrique (roadmr) wrote :

LGTM, thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/identityprovider/middleware/honeypot.py'
2--- src/identityprovider/middleware/honeypot.py 2018-08-02 15:51:07 +0000
3+++ src/identityprovider/middleware/honeypot.py 2019-07-05 15:47:40 +0000
4@@ -15,7 +15,7 @@
5 )
6
7 HONEYPOT_EXEMPT_URLS = re.compile(
8- '^/api/|^/[A-Za-z_]+/\+openid|^/\+openid|^/\+saml|^/saml')
9+ '^/api/|^/[A-Za-z_]+/\+openid|^/\+openid|^/\+saml|^/saml|^/\+delete$')
10
11
12 class HoneypotMiddleware(upstream_honeypot_middleware):
13
14=== modified file 'src/identityprovider/tests/test_middleware.py'
15--- src/identityprovider/tests/test_middleware.py 2018-08-02 16:26:42 +0000
16+++ src/identityprovider/tests/test_middleware.py 2019-07-05 15:47:40 +0000
17@@ -20,7 +20,7 @@
18 from identityprovider.middleware.useraccount import (
19 UserAccountConversionMiddleware)
20 from identityprovider.models import Account, OpenIDRPConfig
21-from identityprovider.models.const import TokenScope
22+from identityprovider.models.const import AccountStatus, TokenScope
23 from identityprovider.oops_config import ErrorReporter
24 from identityprovider.tests import DEFAULT_USER_PASSWORD
25 from identityprovider.tests.utils import SSOBaseTestCase
26@@ -519,7 +519,10 @@
27 self.assertContains(response, "<!DOCTYPE html>")
28
29
30-class HoneypotWhitelistedTestCase(TestCase):
31+@modify_settings(MIDDLEWARE_CLASSES={
32+ 'append': 'identityprovider.middleware.honeypot.HoneypotMiddleware'
33+})
34+class HoneypotWhitelistedTestCase(SSOBaseTestCase):
35
36 def assert_honeypot_not_required(self, url, **kwargs):
37 response = self.client.post(url, **kwargs)
38@@ -555,6 +558,19 @@
39 self.assert_honeypot_not_required(
40 url, data={'SAMLRequest': 'foo', 'RelayState': 'bar'})
41
42+ def test_post_to_delete_account_not_required_to_have_honeypot(self):
43+ account = self.factory.make_account()
44+ login_successful = self.client.login(
45+ username=account.user.email, password=DEFAULT_USER_PASSWORD)
46+ assert login_successful
47+
48+ response = self.client.post(
49+ '/+delete', {'password': DEFAULT_USER_PASSWORD})
50+
51+ self.assertNotContains(response, 'Bad bot')
52+ account.refresh_from_db()
53+ self.assertEqual(account.status, AccountStatus.DELETED)
54+
55
56 class SetRemoteAddrFromForwardedForMiddlewareTestCase(SSOBaseTestCase):
57