Merge lp:~cprov/canonical-identity-provider/username-edit-api into lp:canonical-identity-provider/release

[Celso Providelo]

Allow username edit via privileged API.

Both account endpoints (by openID and email) allow username editing via PATCH, which already requests privileged permissions.

SCA will be able to update an account username across the board (SSO -> LP) when it is necessary.

This is part of the journey described in https://trello.com/c/xPMusmTk/234-bug-1790874-cant-verify-accounts-with-a-dot-in-the-store-username

[Daniel Manrique (roadmr)]

LGTM, a couple of minor things below.

review: Approve

=== modified file 'src/api/v20/handlers.py'
--- src/api/v20/handlers.py	2019-01-15 17:32:45 +0000
+++ src/api/v20/handlers.py	2019-04-10 16:16:36 +0000
@@ -34,7 +34,7 @@
 from api.v20.whitelist import URL_WHITELIST
 from identityprovider import auth, emailutils
 from identityprovider.fields import FIELD_REQUIRED
-from identityprovider.forms import AccountStatusForm
+from identityprovider.forms import AccountManageForm
 from identityprovider.login import (
     AccountDeactivated,
     AccountSuspended,
@@ -60,7 +60,10 @@
 from identityprovider.signals import login_failed, login_succeeded
 from identityprovider.stats import stats
 from identityprovider.utils import redirection_url_for_token
-from webservices.launchpad import get_lp_ssh_keys
+from webservices.launchpad import (
+    LaunchpadAPIError,
+    get_lp_ssh_keys,
+)
 
 
 logger = logging.getLogger(__name__)
@@ -149,16 +152,19 @@
     @authorization_required(edit=True)
     @require_mime('json')
     @add_piston_http_patch_support
-    @validate(AccountStatusForm, 'data')
+    @validate(AccountManageForm, 'data')
     @throttle(extra_callback=get_openid_from_args)
     def patch(self, request, openid):
         account = Account.objects.active_by_openid(openid)
         if account is None:
             return errors.RESOURCE_NOT_FOUND()
 
-        account.status = request.form.cleaned_data['status']
-        account.status_comment = request.form.cleaned_data['status_comment']
-        account.save()
+        try:
+            request.form.update_account(account)
+        except LaunchpadAPIError as err:
+            return errors.INVALID_DATA(
+                new_style=True, username=[str(err)])
+
         return get_account_data(account)
 
 
@@ -593,16 +599,19 @@
     @authorization_required(edit=True)
     @require_mime('json')
     @add_piston_http_patch_support
-    @validate(AccountStatusForm, 'data')
+    @validate(AccountManageForm, 'data')
     @throttle(extra_callback=lambda request, email: valid_email(email))
     def patch(self, request, email):
         account = Account.objects.get_by_email(email)
         if account is None:
             return errors.RESOURCE_NOT_FOUND()
 
-        account.status = request.form.cleaned_data['status']
-        account.status_comment = request.form.cleaned_data['status_comment']
-        account.save()
+        try:
+            request.form.update_account(account)
+        except LaunchpadAPIError as err:
+            return errors.INVALID_DATA(
+                new_style=True, username=[str(err)])
+
         return get_account_data(account)
 
 

=== modified file 'src/api/v20/tests/test_handlers.py'
--- src/api/v20/tests/test_handlers.py	2019-01-15 17:32:45 +0000
+++ src/api/v20/tests/test_handlers.py	2019-04-10 16:16:36 +0000
@@ -28,6 +28,7 @@
 from api.v20 import handlers, whitelist
 from api.v20.utils import install_error_format_checker
 from identityprovider.auth import build_discharge_macaroon
+from identityprovider.forms import INVALID_NAME
 from identityprovider.login import PasswordPolicyError
 from identityprovider.models import (
     Account,
@@ -164,6 +165,23 @@
         self.assertEqual(json_body['code'], 'SERVER_ERROR')
         self.assertEqual(json_body['extra'], {})
 
+    def patch_lp_response(self, code=200, body=None):
+
+        class FakeResponse():
+            def __init__(self, code, body):
+                self.code = code
+                self.body = body
+
+            def read(self):
+                return self.body
+
+        if body is None:
+            body = json.dumps('not-specified')
+
+        self.patch(
+            'webservices.launchpad.http_request_with_timeout',
+            return_value=FakeResponse(code, body))
+
 
 class AccountsHandlerTestCase(BaseTestCase):
     url_name = 'api-account'
@@ -252,6 +270,53 @@
             response.get('message', ''), 'Invalid request data')
         self.assertEqual(response.get('code', ''), 'INVALID_DATA')
 
+    def test_patch_username_invalid(self):
+        body = self.do_patch(
+            {'username': 'foo.bar'}, token=self.super_token, status_code=400)
+        self.assertEqual(body['message'], 'Invalid request data')
+        self.assertEqual(body['code'], 'INVALID_DATA')
+        self.assertEqual(body['extra'], {'username': [INVALID_NAME]})
+
+    @override_settings(LP_API_URL='test.com')
+    def test_patch_username_valid(self):
+        old_status = self.account.status
+        old_comment = self.account.status_comment
+
+        self.patch_lp_response(body=json.dumps('foo-bar'))
+        self.do_patch({'username': 'foo-bar'}, token=self.super_token)
+
+        self.account.refresh_from_db()
+        self.assertTrue(self.account.need_username_refresh)
+        self.assertEqual(self.account.username, 'foo-bar')
+        self.assertEqual(self.account.status, old_status)
+        self.assertEqual(self.account.status_comment, old_comment)
+
+    @override_settings(LP_API_URL='test.com')
+    def test_patch_username_unavailable(self):
+        lp_body = 'name: foo-bar is already in use ...'
+        self.patch_lp_response(code=400, body=lp_body)
+        body = self.do_patch(
+            {'username': 'foo-bar'}, token=self.super_token, status_code=400)
+        self.assertEqual(body['message'], 'Invalid request data')
+        self.assertEqual(body['code'], 'INVALID_DATA')
+        self.assertEqual(body['extra'], {'username': [lp_body]})
+
+    @override_settings(LP_API_URL='test.com')
+    def test_patch_username_and_status(self):
+        assert self.account.status != AccountStatus.SUSPENDED
+        self.patch_lp_response(body=json.dumps('foo-bar'))
+        self.do_patch({
+            'username': 'foo-bar',
+            'status': 'SUSPENDED',
+            'status_comment': 'BAD!',
+        }, token=self.super_token)
+
+        self.account.refresh_from_db()
+        self.assertTrue(self.account.need_username_refresh)
+        self.assertEqual(self.account.username, 'foo-bar')
+        self.assertEqual(self.account.status, AccountStatus.SUSPENDED)
+        self.assertEqual(self.account.status_comment, 'BAD!')
+
     def test_authenticated_openid_no_match(self):
         assert self.account.openid_identifier != 'foo'
         url = reverse(self.url_name, args=('foo',))
@@ -853,6 +918,53 @@
             response.get('message', ''), 'Invalid request data')
         self.assertEqual(response.get('code', ''), 'INVALID_DATA')
 
+    def test_patch_username_invalid(self):
+        body = self.do_patch(
+            {'username': 'foo.bar'}, token=self.super_token, status_code=400)
+        self.assertEqual(body['message'], 'Invalid request data')
+        self.assertEqual(body['code'], 'INVALID_DATA')
+        self.assertEqual(body['extra'], {'username': [INVALID_NAME]})
+
+    @override_settings(LP_API_URL='test.com')
+    def test_patch_username_valid(self):
+        old_status = self.account.status
+        old_comment = self.account.status_comment
+
+        self.patch_lp_response(body=json.dumps('foo-bar'))
+        self.do_patch({'username': 'foo-bar'}, token=self.super_token)
+
+        self.account.refresh_from_db()
+        self.assertTrue(self.account.need_username_refresh)
+        self.assertEqual(self.account.username, 'foo-bar')
+        self.assertEqual(self.account.status, old_status)
+        self.assertEqual(self.account.status_comment, old_comment)
+
+    @override_settings(LP_API_URL='test.com')
+    def test_patch_username_unavailable(self):
+        lp_body = 'name: foo-bar is already in use ...'
+        self.patch_lp_response(code=400, body=lp_body)
+        body = self.do_patch(
+            {'username': 'foo-bar'}, token=self.super_token, status_code=400)
+        self.assertEqual(body['message'], 'Invalid request data')
+        self.assertEqual(body['code'], 'INVALID_DATA')
+        self.assertEqual(body['extra'], {'username': [lp_body]})
+
+    @override_settings(LP_API_URL='test.com')
+    def test_patch_username_and_status(self):
+        assert self.account.status != AccountStatus.SUSPENDED
+        self.patch_lp_response(body=json.dumps('foo-bar'))
+        self.do_patch({
+            'username': 'foo-bar',
+            'status': 'SUSPENDED',
+            'status_comment': 'BAD!',
+        }, token=self.super_token)
+
+        self.account.refresh_from_db()
+        self.assertTrue(self.account.need_username_refresh)
+        self.assertEqual(self.account.username, 'foo-bar')
+        self.assertEqual(self.account.status, AccountStatus.SUSPENDED)
+        self.assertEqual(self.account.status_comment, 'BAD!')
+
     def test_can_get_others_if_elevated_permission(self):
         other_email = self.factory.make_account().preferredemail
         self.factory.add_permission_to_account(

=== modified file 'src/identityprovider/forms.py'
--- src/identityprovider/forms.py	2019-04-03 16:52:44 +0000
+++ src/identityprovider/forms.py	2019-04-10 16:16:36 +0000
@@ -653,15 +653,58 @@
     name = fields.CharField()
 
 
-class AccountStatusForm(Form):
-    status = fields.ChoiceField(choices=[(attr, attr)
-                                for attr in AccountStatus.enum()])
+class AccountManageForm(Form):
+
+    username = UsernameField(required=False)
+    status = fields.ChoiceField(
+        choices=[(attr, attr) for attr in AccountStatus.enum()])
     status_comment = fields.CharField()
 
+    def __init__(self, *args, **kwargs):
+        super(AccountManageForm, self).__init__(*args, **kwargs)
+        # Allow `username` to be modified in isolation.
+        if 'username' in self.data.keys():
+            self.fields['status'].required = False
+            self.fields['status_comment'].required = False
+        if 'status' in self.data.keys():
+            self.fields['status_comment'].required = True
+
+    def clean_username(self):
+        if 'username' not in self.cleaned_data:
+            return None
+        username = self.cleaned_data['username'].strip()
+        if not username:
+            return ''
+        if not VALID_NAME_RE.match(username):
+            raise forms.ValidationError(INVALID_NAME)
+        return username
+
     def clean_status(self):
         status = self.cleaned_data['status']
+        if not status:
+            return None
         return getattr(AccountStatus, status)
 
+    def update_account(self, account):
+        """Helper for API handlers to update the account and LP.
+
+        Raises `LaunchpadAPIError` in case of LP error or failure.
+        """
+        status = self.cleaned_data.get('status')
+        if status:
+            account.status = status
+
+        status_comment = self.cleaned_data.get('status_comment')
+        if status_comment:
+            account.status_comment = status_comment
+
+        username = self.cleaned_data.get('username')
+        if username and username != account.person_name:
+            set_lp_username(account.openid_identifier, username)
+            account.need_username_refresh = True
+
+        account.save()
+
 
 class MacaroonRequestForm(Form):
     """A form object for user control over requesting discharge macaroons."""

=== modified file 'src/identityprovider/tests/test_forms.py'
--- src/identityprovider/tests/test_forms.py	2019-04-03 16:52:44 +0000
+++ src/identityprovider/tests/test_forms.py	2019-04-10 16:16:36 +0000
@@ -37,7 +37,7 @@
 from identityprovider.forms import (
     INVALID_NAME,
     VALID_NAME_RE,
-    AccountStatusForm,
+    AccountManageForm,
     DeviceRenameForm,
     EditAccountForm,
     GenericEmailForm,
@@ -1367,16 +1367,16 @@
         self.assertEqual(form.cleaned_data['name'], 'bar')
 
 
-class AccountStatusFormTestCase(FormBaseTestCase):
-    fields = ['status', 'status_comment']
+class AccountManageFormTestCase(FormBaseTestCase):
+    fields = ['status', 'status_comment', 'username']
 
     def get_form(self, **kwargs):
-        return AccountStatusForm(**kwargs)
+        return AccountManageForm(**kwargs)
 
-    def test_required_fields(self):
-        form = self.get_form()
-        for field in self.fields:
-            self.assertTrue(form.fields[field].required)
+    def test_comment_required_when_status_given(self):
+        data = {'status': 'ACTIVE', 'status_comment': 'Wow!'}
+        form = self.get_form(data=data)
+        self.assertTrue(form.fields['status_comment'].required)
 
     def test_status_field_valid(self):
         values = [(attr, 'Test comment.') for attr in AccountStatus.enum()]
@@ -1396,6 +1396,29 @@
             form = self.get_form(data=data)
             self.assertFalse(form.is_valid())
 
+    def test_username_field_empty(self):
+        empty_values = (
+            {'username': ''},
+            {'username': ' '},
+            {'username': None},
+            {'status': 'ACTIVE', 'status_comment': 'Wow!'},
+        )
+        for data in empty_values:
+            form = self.get_form(data=data)
+            self.assertTrue(form.is_valid(), form.errors)
+            self.assertEqual(form.cleaned_data['username'], '')
+
+    def test_username_field_valid(self):
+        data = {'username': 'foo-bar'}
+        form = self.get_form(data=data)
+        self.assertTrue(form.is_valid())
+
+    def test_username_field_invalid(self):
+        data = {'username': 'no+pluses'}
+        form = self.get_form(data=data)
+        self.assertFalse(form.is_valid())
+        self.assertEqual(form.errors['username'], [INVALID_NAME])
+
 
 class MacaroonRequestFormTestCase(SSOBaseTestCase):