Merge lp:~maxiberta/canonical-identity-provider/disable-user-registration-api into lp:canonical-identity-provider/release

[Maximiliano Bertacchini]

Disable user registration via API v1/v2 (via USER_REGISTRATION_API_ENABLED switch, disabled by default).

It was agreed that account registration via API should be disabled/not allowed; leaving the web UI as the only means of registration.

User registration via web internally POSTs to http://localhost/api/v2/accounts (from `settings.API_HOST = "http://localhost"`), so we need to unconditionally allow "internal" requests to this endpoint. These internal requests do not contain the X-Forwarded-For header.

The presence of the X-Forwarded-For header is used to mark a request as coming from an external client, instead of `request.environ['REMOTE_ADDR']` which is basically the 1st item of `request.environ['HTTP_X_FORWARDED_FOR']` (see src/identityprovider/middleware/http.py), and easily spoofable (should be fixed separately).

[Maximiliano Bertacchini (maxiberta)]

Some stats from sso-app/4 in PROD in the last 120 days (details: https://pastebin.canonical.com/p/JXcXz6yyY7/):

- POST /api/1.0/registration: 731 hits (broken due to dead reCaptcha v1).
- POST /api/1.1/registration: 0 hits (broken due to dead reCaptcha v1).
- POST /api/v2/accounts: 80848 hits total, 64199 successful (returning 201), of which only 424 *not* coming from 127.0.0.1.

In summary: only 424 successful account registrations via API in the last 4 months (in this host out of 4 units total).

[Natalia Bidart (nataliabidart)]

18:47 < nessita> maxiberta, thank you, would you have any user agent or any info about those 424 attempts?
18:47 < nessita> maxiberta, are they "ubuntuone clients" for example?
18:49 < maxiberta> nessita: 59 are "Mozilla/5.0", 365 are "-"

[Natalia Bidart (nataliabidart)]

+1 with a minor nitpick

review: Approve

[Natalia Bidart (nataliabidart)]

+1 with a small nitpick

review: Approve

=== modified file 'src/api/v10/handlers.py'
--- src/api/v10/handlers.py	2018-10-12 21:03:41 +0000
+++ src/api/v10/handlers.py	2018-10-22 14:58:19 +0000
@@ -17,6 +17,7 @@
 from django.shortcuts import render
 from django.utils.timezone import now
 from django.utils.translation import ugettext_lazy as _
+from gargoyle import gargoyle
 from oauth_backend.models import Token
 from piston.emitters import Emitter
 from piston.handler import BaseHandler
@@ -178,6 +179,13 @@
 
     @named_operation
     def register(self, request):
+        if not gargoyle.is_active('USER_REGISTRATION_API_ENABLED', request):
+            # The presence X-Forwarded-For marks an external client's request.
+            # Internal requests (e.g. from the new_account view) are allowed.
+            if 'HTTP_X_FORWARDED_FOR' in request.environ:
+                return api_error(HttpResponseForbidden,
+                                 "User registration via API is disabled.")
+
         data = request.data
         data['remote_ip'] = request.environ['REMOTE_ADDR']
         form = WebserviceCreateAccountForm(data)

=== modified file 'src/api/v10/tests/test_handlers.py'
--- src/api/v10/tests/test_handlers.py	2018-10-12 21:03:41 +0000
+++ src/api/v10/tests/test_handlers.py	2018-10-22 14:58:19 +0000
@@ -59,6 +59,8 @@
 
 class RegistrationHandlerTestCase(AnonAPITestCase):
 
+    url = reverse('api-10-registration')
+
     def test_authtoken_without_requester_is_handled_properly(self):
         authtoken = self.factory.make_authtoken(
             email="[e-mail]",
@@ -380,13 +382,81 @@
 
         with self.assertRaises(Exception):
             self.client.post(
-                '/api/1.0/registration',
+                self.url,
                 {'ws.op': 'register',
                  'email': email_address,
                  'password': 'MySecretPassword1',
                  'captcha_solution': 'foobar',
                  'captcha_id': 'id'})
 
+    @switches(USER_REGISTRATION_API_ENABLED=False)
+    def test_feature_flag_disabled_with_internal_client(self):
+        # Request has *no* X-Forwarded-For, which is treated as an internal IP.
+        email = self.factory.make_email_address()
+        data = {
+            'ws.op': 'register', 'displayname': 'Test User', 'email': email,
+            'password': 'MySecretPassword1',
+            'captcha_solution': 'foobar', 'captcha_id': 'id',
+        }
+
+        response = self.client.get(self.url, data)
+
+        self.assertEqual(response.status_code, 200)
+        account = Account.objects.get_by_email(email)
+        self.assertEqual(account.displayname, 'Test User')
+        self.assert_new_user_email_sent(email)
+
+    @switches(USER_REGISTRATION_API_ENABLED=False)
+    def test_feature_flag_disabled_with_external_client(self):
+        # Request has X-Forwarded-For, which is treated as an *external* IP.
+        email = self.factory.make_email_address()
+        data = {
+            'ws.op': 'register', 'displayname': 'Test User', 'email': email,
+            'password': 'MySecretPassword1',
+            'captcha_solution': 'foobar', 'captcha_id': 'id',
+        }
+        headers = {'HTTP_X_FORWARDED_FOR': '127.0.0.1'}
+
+        response = self.client.get(self.url, data, **headers)
+
+        self.assertEqual(response.status_code, 403)
+        self.assertIsNone(Account.objects.get_by_email(email))
+
+    @switches(USER_REGISTRATION_API_ENABLED=True)
+    def test_feature_flag_enabled_with_internal_client(self):
+        # Request has *no* X-Forwarded-For, which is treated as an internal IP.
+        email = self.factory.make_email_address()
+        data = {
+            'ws.op': 'register', 'displayname': 'Test User', 'email': email,
+            'password': 'MySecretPassword1',
+            'captcha_solution': 'foobar', 'captcha_id': 'id',
+        }
+
+        response = self.client.get(self.url, data)
+
+        self.assertEqual(response.status_code, 200)
+        account = Account.objects.get_by_email(email)
+        self.assertEqual(account.displayname, 'Test User')
+        self.assert_new_user_email_sent(email)
+
+    @switches(USER_REGISTRATION_API_ENABLED=True)
+    def test_feature_flag_enabled_with_external_client(self):
+        # Request has X-Forwarded-For, which is treated as an *external* IP.
+        email = self.factory.make_email_address()
+        data = {
+            'ws.op': 'register', 'displayname': 'Test User', 'email': email,
+            'password': 'MySecretPassword1',
+            'captcha_solution': 'foobar', 'captcha_id': 'id',
+        }
+        headers = {'HTTP_X_FORWARDED_FOR': '127.0.0.1'}
+
+        response = self.client.get(self.url, data, **headers)
+
+        self.assertEqual(response.status_code, 200)
+        account = Account.objects.get_by_email(email)
+        self.assertEqual(account.displayname, 'Test User')
+        self.assert_new_user_email_sent(email)
+
 
 class AuthenticationTestCase(SSOBaseTestCase):
 

=== modified file 'src/api/v20/handlers.py'
--- src/api/v20/handlers.py	2018-08-17 18:38:29 +0000
+++ src/api/v20/handlers.py	2018-10-22 14:58:19 +0000
@@ -282,6 +282,12 @@
     @throttle()
     def create(self, request):
         """Create/register a new account."""
+        if not gargoyle.is_active('USER_REGISTRATION_API_ENABLED', request):
+            # The presence X-Forwarded-For marks an external client's request.
+            # Internal requests (e.g. from the new_account view) are allowed.
+            if 'HTTP_X_FORWARDED_FOR' in request.environ:
+                return errors.FEATURE_DISABLED()
+
         data = request.data
 
         try:

=== modified file 'src/api/v20/tests/test_handlers.py'
--- src/api/v20/tests/test_handlers.py	2018-09-13 20:19:21 +0000
+++ src/api/v20/tests/test_handlers.py	2018-10-22 14:58:19 +0000
@@ -616,6 +616,33 @@
         self.assert_bad_request(
             data=self.data, extra={'email': ['Invalid email']})
 
+    @switches(USER_REGISTRATION_API_ENABLED=False)
+    def test_feature_flag_disabled_with_internal_client(self):
+        # Request has *no* X-Forwarded-For, which is treated as an internal IP.
+        json_body = self.do_post(self.data, status_code=201)
+        self.assert_correct_account_information(json_body)
+
+    @switches(USER_REGISTRATION_API_ENABLED=False)
+    def test_feature_flag_disabled_with_external_client(self):
+        # Request has X-Forwarded-For, which is treated as an *external* IP.
+        headers = {'HTTP_X_FORWARDED_FOR': '127.0.0.1'}
+        json_body = self.do_post(self.data, status_code=403, **headers)
+        self.assertEqual(json_body['code'], "FEATURE_DISABLED")
+        self.assertEqual("Feature disabled.", json_body['message'])
+
+    @switches(USER_REGISTRATION_API_ENABLED=True)
+    def test_feature_flag_enabled_with_internal_client(self):
+        # Request has *no* X-Forwarded-For, which is treated as an internal IP.
+        json_body = self.do_post(self.data, status_code=201)
+        self.assert_correct_account_information(json_body)
+
+    @switches(USER_REGISTRATION_API_ENABLED=True)
+    def test_feature_flag_enabled_with_external_client(self):
+        # Request has X-Forwarded-For, which is treated as an *external* IP.
+        headers = {'HTTP_X_FORWARDED_FOR': '127.0.0.1'}
+        json_body = self.do_post(self.data, status_code=201, **headers)
+        self.assert_correct_account_information(json_body)
+
 
 @override_settings(CAPTCHA_PUBLIC_KEY='public', CAPTCHA_PRIVATE_KEY='private')
 class AnonymousAccountRegistrationWithCaptchaHandlerTestCase(BaseTestCase):

=== modified file 'src/api/v20/utils.py'
--- src/api/v20/utils.py	2018-05-28 17:22:45 +0000
+++ src/api/v20/utils.py	2018-10-22 14:58:19 +0000
@@ -67,6 +67,7 @@
 class ErrorCode:
     ACCOUNT_NOT_READY = 'account-not-ready'
     BAD_REQUEST = 'bad-request'
+    FEATURE_DISABLED = 'feature-disabled'
     INTERNAL_ERROR = 'internal-server-error'
     INVALID_DATA = 'invalid-data'
     INVALID_CREDENTIALS = 'invalid-credentials'
@@ -141,6 +142,10 @@
         403, _("Insufficient permissions."),
         ErrorCode.PERMISSION_REQUIRED),
 
+    FEATURE_DISABLED=(
+        403, _("Feature disabled."),
+        ErrorCode.FEATURE_DISABLED),
+
     RESOURCE_NOT_FOUND=(
         404, _("The resource requested was not found."),
         ErrorCode.RESOURCE_NOT_FOUND),