api: change client's password handling (#89456)

src/authentic2/api_views.py

@@ -1470,12 +1470,12 @@ class CheckAPIClientAPI(BaseRpcView):
         ou = serializer.validated_data.get('ou', None)
         api_client = None
         try:
-            api_client = APIClient.objects.get(identifier=identifier, password=password)
+            api_client = APIClient.objects.get(identifier=identifier)
         except APIClient.DoesNotExist:
             pass
 
         result = {}
-        if api_client is None or ou and ou != api_client.ou:
+        if api_client is None or not api_client.check_password(password) or ou and ou != api_client.ou:
             result['err'] = 1
             result['err_desc'] = 'api client not found'
         else:

src/authentic2/authentication.py

@@ -71,7 +71,9 @@ class Authentic2Authentication(BasicAuthentication):
             pass
 
         try:
-            return APIClient.objects.get(identifier=userid, password=password), None
+            api_client = APIClient.objects.get(identifier=userid)
+            if api_client.check_password(password):
+                return api_client, None
         except APIClient.DoesNotExist:
             pass
 

src/authentic2/manager/forms.py

@@ -212,6 +212,7 @@ class UserEditForm(LimitQuerysetFormMixin, CssClass, BaseUserForm):
 
     class Meta:
         model = User
+        widgets = {'password': forms.PasswordInput()}
         exclude = (
             'is_staff',
             'groups',

src/authentic2/manager/templates/authentic2/manager/api_client_detail.html

@@ -22,7 +22,6 @@
     <h3>{% trans "Parameters" %}</h3>
     <ul>
       <li>{% blocktrans with identifier=api_client.identifier %}Identifier: {{ identifier }}{% endblocktrans %}</li>
-      <li>{% blocktrans with password=api_client.password %}Password: {{ password }}{% endblocktrans %}</li>
       <li>{% blocktrans with ou=api_client.ou.name %}Organizational unit: {{ ou }}{% endblocktrans %}</li>
       {% if api_client.apiclient_roles.count %}
         <li>{% trans "Roles:" %}

src/authentic2/migrations/0051_alter_apiclient_password.py

@@ -0,0 +1,34 @@
+# Generated by Django 3.2.18 on 2024-04-17 09:54
+
+from django.contrib.auth.hashers import make_password
+from django.db import migrations
+
+import authentic2.models
+
+
+def hash_existing(apps, schema_editor):
+    APIClient = apps.get_model('authentic2', 'APIClient')
+    for client in APIClient.objects.all():
+        client.password = make_password(client.password)
+        client.save()
+
+
+def fake_hash_reverse(apps, schema_editor):
+    APIClient = apps.get_model('authentic2', 'APIClient')
+    for dummy in APIClient.objects.all():
+        raise NotImplementedError('Not reversible')
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ('authentic2', '0050_initialize_users_advanced_configuration'),
+    ]
+
+    operations = [
+        migrations.RunPython(hash_existing, fake_hash_reverse),
+        migrations.AlterField(
+            model_name='apiclient',
+            name='password',
+            field=authentic2.models.PasswordField(max_length=256, verbose_name='Password'),
+        ),
+    ]

src/authentic2/models.py

@@ -53,6 +53,12 @@ from .utils.misc import ServiceAccessDenied
 from .utils.sms import create_sms_code
 
 
+class PasswordField(models.CharField):
+    def pre_save(self, model_instance, add):
+        value = super().pre_save(model_instance, add)
+        return auth.hashers.make_password(value)
+
+
 class UserExternalId(models.Model):
     user = models.ForeignKey(settings.AUTH_USER_MODEL, verbose_name=_('user'), on_delete=models.CASCADE)
     source = models.CharField(max_length=256, verbose_name=_('source'))
@@ -707,7 +713,7 @@ class APIClient(models.Model):
     name = models.CharField(max_length=128, verbose_name=_('Name'))
     description = models.TextField(verbose_name=_('Description'), blank=True)
     identifier = models.CharField(max_length=256, verbose_name=_('Identifier'))
-    password = models.CharField(max_length=256, verbose_name=_('Password'))
+    password = PasswordField(max_length=256, verbose_name=_('Password'))
     restrict_to_anonymised_data = models.BooleanField(
         verbose_name=_('Restrict to anonymised data'), default=False
     )
@@ -758,6 +764,18 @@ class APIClient(models.Model):
     def is_superuser(self):
         return False
 
+    def check_password(self, raw_password):
+        # ensure we load self.password from db and that we do not keep
+        # the value given at creation
+        del self.password
+
+        def update_password(raw_password):
+            # Will update password hash if hasher changed
+            self.password = auth.hashers.make_password(raw_password)
+            self.save(update_fields=['password'])
+
+        return auth.hashers.check_password(raw_password, self.password, setter=update_password)
+
     def has_perm(self, perm, obj=None):
         if self.is_active and self.is_superuser:
             return True

tests/api/test_all.py

@@ -61,6 +61,7 @@ def test_api_user(client):
     user.attributes.birthdate = datetime.date(2019, 2, 2)
     user.set_password('password')
     user.save()
+    assert user.password != 'password'
 
     role1 = Role.objects.create(name='Role1', ou=ou)
     role1.members.add(user)
@@ -458,6 +459,10 @@ def test_api_users_create(settings, app, api_user):
         assert AttributeValue.objects.with_owner(new_user).filter(verified=True).count() == 0
         assert AttributeValue.objects.with_owner(new_user).filter(attribute=at).exists()
         assert AttributeValue.objects.with_owner(new_user).get(attribute=at).content == payload['title']
+        # Check that password is hashed
+        assert User.objects.get(uuid=resp.json['uuid']).check_password('password')
+        assert User.objects.get(uuid=resp.json['uuid']).password != 'password'
+
         if (
             hasattr(api_user, 'oidc_client')
             and api_user.oidc_client.authorization_mode != OIDCClient.AUTHORIZATION_MODE_NONE
@@ -501,7 +506,7 @@ def test_api_users_create(settings, app, api_user):
         'last_name': 'Doe',
         'last_name_verified': True,
         'email': 'john.doe@example.net',
-        'password': 'password',
+        'password': 'secret',
         'title': 'Mr',
         'title_verified': True,
     }
@@ -541,6 +546,9 @@ def test_api_users_create(settings, app, api_user):
         assert resp.json['first_name_verified']
         assert resp.json['last_name_verified']
         assert not resp2.json['title_verified']
+        # Check that password is hashed
+        assert User.objects.get(uuid=resp.json['uuid']).check_password('secret')
+        assert User.objects.get(uuid=resp.json['uuid']).password != 'secret'
 
 
 def test_api_users_create_email_is_unique(settings, app, superuser):
@@ -633,6 +641,9 @@ def test_api_users_create_send_mail(app, settings, superuser, rf):
     assert str(app.session['_auth_user_id']) == str(user_id)
     assert not good_next_url(rf.get('/'), 'http://example.com')
     assert resp.location == 'http://example.com/'
+    # Check password is hashed
+    assert User.objects.get(username='john.doe').check_password('1234==aA')
+    assert User.objects.get(username='john.doe').password != '1234==aA'
 
 
 def test_api_users_create_force_password_reset(app, client, settings, superuser):
@@ -653,6 +664,8 @@ def test_api_users_create_force_password_reset(app, client, settings, superuser)
     resp.form.set('new_password2', '1234==aB')
     resp = resp.form.submit('Submit').follow().maybe_follow()
     assert 'Password changed' in resp
+    assert User.objects.get(username='john.doe').check_password('1234==aB')
+    assert User.objects.get(username='john.doe').password != '1234==aB'
 
 
 def test_api_drf_authentication_class(app, admin, user_ou1, oidc_client):
@@ -752,6 +765,7 @@ def test_password_change(app, ou1, admin):
     response = app.post_json(url, params=payload)
     assert response.json['result'] == 1
     assert User.objects.get(username='john.doe').check_password('password2')
+    assert User.objects.get(username='john.doe').password != 'password2'
     assert_event('manager.user.password.change', user=admin, api=True)
 
 
@@ -1072,6 +1086,7 @@ def test_api_password_change_user_delete(app, settings, admin, ou1):
     user2.delete()
     app.post_json(url, params=payload)
     assert User.objects.get(username='john.doe').check_password('password2')
+    assert User.objects.get(username='john.doe').password != 'password2'
 
 
 def test_api_users_delete(settings, app, admin, simple_user):

tests/test_api_client.py

@@ -15,7 +15,10 @@ User = get_user_model()
 
 @pytest.fixture
 def api_client(db):
-    return APIClient.objects.create(name='foo', identifier='id', password='pass')
+    api_client = APIClient.objects.create(name='foo', identifier='id', password='pass')
+    assert api_client.check_password('pass')
+    api_client.password = 'pass'  # unhash password
+    return api_client
 
 
 def test_has_perm(api_client):
@@ -197,3 +200,38 @@ def test_api_users_create(app, api_client):
     api_client.apiclient_roles.add(r)
     resp = app.post_json('/api/users/', params=payload)
     assert User.objects.get(uuid=resp.json['uuid'])
+
+
+def test_api_users_create_password(app, api_client):
+    payload = {
+        'username': 'janedoe',
+        'email': 'jane.doe@nowhere.null',
+        'first_name': 'Jane',
+        'last_name': 'Doe',
+        'password': 'secret',
+    }
+    app.authorization = ('Basic', (api_client.identifier, api_client.password))
+    resp = app.post_json('/api/users/', params=payload, status=403)
+
+    # give permissions
+    r = Role.objects.get_admin_role(
+        ContentType.objects.get_for_model(User), name='role', slug='role', operation=ADD_OP
+    )
+    api_client.apiclient_roles.add(r)
+    resp = app.post_json('/api/users/', params=payload)
+    assert User.objects.get(uuid=resp.json['uuid'])
+    assert User.objects.get(uuid=resp.json['uuid']).check_password('secret')
+    assert User.objects.get(uuid=resp.json['uuid']).password != 'secret'
+
+
+def test_api_users_hasher_change(app, api_client, settings):
+    del api_client.password  # load password hash from DB
+    original_password = api_client.password
+
+    settings.PASSWORD_HASHERS = ['authentic2.hashers.PloneSHA1PasswordHasher'] + settings.PASSWORD_HASHERS
+
+    assert api_client.check_password('foobar') is False
+    assert original_password == api_client.password
+
+    assert api_client.check_password('pass')
+    assert original_password != api_client.password

tests/test_manager_apiclient.py

@@ -140,6 +140,12 @@ def test_list_show_objects_local_admin(admin_ou1, app, ou1, ou2):
         password='bar-password',
         ou=ou2,
     )
+    del api_client_ou1.password
+    del api_client_ou2.password
+    assert api_client_ou1.password != 'foo-password'
+    assert api_client_ou1.check_password('foo-password')
+    assert api_client_ou2.password != 'bar-password'
+    assert api_client_ou2.check_password('bar-password')
     url = '/manage/api-clients/%s/' % api_client_ou1.pk
     resp = login(app, admin_ou1, 'a2-manager-api-clients')
     assert len(resp.pyquery('div.content ul.objects-list li')) == 1
@@ -190,6 +196,8 @@ def test_add(superuser, app):
     response = form.submit().follow()
     assert APIClient.objects.count() == 1
     api_client = APIClient.objects.get(name='api-client-name')
+    assert api_client.password != 'api-client-password'
+    assert api_client.check_password('api-client-password')
     assert set(api_client.apiclient_roles.all()) == {role_1, role_2}
     assert set(api_client.allowed_user_attributes.all()) == {preferred_color, phone2}
     assert urlparse(response.request.url).path == api_client.get_absolute_url()
@@ -223,6 +231,8 @@ def test_add_description_non_mandatory(superuser, app):
     response = form.submit().follow()
     assert APIClient.objects.count() == 1
     api_client = APIClient.objects.get(name='api-client-name')
+    assert api_client.check_password('api-client-password')
+    assert api_client.password != 'api-client-password'
     assert set(api_client.apiclient_roles.all()) == {role_1, role_2}
     assert urlparse(response.request.url).path == api_client.get_absolute_url()
 
@@ -242,7 +252,6 @@ def test_detail(superuser, app, phone_activated_authn):
     resp = login(app, superuser, api_client.get_absolute_url())
     assert 'foo-description' in resp.text
     assert 'Identifier: foo-identifier' in resp.text
-    assert 'Password: foo-password' in resp.text
     assert 'foo-description' in resp.text
     assert 'Restricted to anonymised data' in resp.text
     assert 'role-1' in resp.text
@@ -304,7 +313,6 @@ def test_edit(superuser, app, ou1, ou2):
     assert APIClient.objects.count() == 1
     resp = login(app, superuser, 'a2-manager-api-client-edit', kwargs={'pk': api_client.pk})
     form = resp.form
-    assert form.get('password').value == 'foo-password'
     assert set(form.get('allowed_user_attributes').value) == {str(preferred_color.id), str(phone2.id)}
     assert ('', False, '---------') in form['ou'].options
     resp.form.set('password', 'easy')
@@ -318,8 +326,9 @@ def test_edit(superuser, app, ou1, ou2):
     response = form.submit().follow()
     assert urlparse(response.request.url).path == api_client.get_absolute_url()
     assert APIClient.objects.count() == 1
-    api_client = APIClient.objects.get(password='easy')
-    assert api_client.identifier == 'foo-identifier'
+    api_client = APIClient.objects.get(identifier='foo-identifier')
+    assert api_client.check_password('easy')
+    assert api_client.password != 'easy'
     assert set(api_client.allowed_user_attributes.all()) == {phone2}
 
     resp = app.get(reverse('a2-manager-api-client-edit', kwargs={'pk': api_client.pk}))
@@ -370,7 +379,6 @@ def test_edit_local_admin(admin_ou1, app, ou1, ou2):
     )
     resp = login(app, admin_ou1, 'a2-manager-api-client-edit', kwargs={'pk': api_client_ou1.pk})
     form = resp.form
-    assert form.get('password').value == 'foo-password'
     resp.form.set('password', 'easy')
     assert ('', False, '---------') not in form['ou'].options
     with pytest.raises(KeyError):
@@ -381,13 +389,12 @@ def test_edit_local_admin(admin_ou1, app, ou1, ou2):
     form['apiclient_roles'].force_value([role_1.id, role_3.id])
     response = form.submit().follow()
     assert urlparse(response.request.url).path == api_client_ou1.get_absolute_url()
-    api_client = APIClient.objects.get(password='easy')
-    assert api_client.identifier == 'foo-description'
+    api_client = APIClient.objects.get(identifier='foo-description')
+    assert api_client.check_password('easy')
 
     role = Role.objects.get(slug='_a2-manager-of-api-clients-%s' % ou2.slug)
     admin_ou1.roles.add(role)
     resp = app.get(reverse('a2-manager-api-client-edit', kwargs={'pk': api_client_ou2.pk}))
-    assert resp.form.get('password').value == 'bar-password'
     assert ('', False, '---------') not in form['ou'].options
     resp.form.set('ou', ou1.id)
     resp.form.submit().follow()

tests/test_migrations.py

@@ -84,3 +84,18 @@ def test_migration_custom_user_0050_initialize_users_advanced_configuration(tran
     assert Setting.objects.count() == before + 1
     assert Setting.objects.filter(key__startswith='users:').count() == 1
     assert Setting.objects.get(key='users:backoffice_sidebar_template').value == ''
+
+
+def test_migration_apiclient_0051_alter_apiclient_password(transactional_db, migration):
+    old_apps = migration.before([('authentic2', '0050_initialize_users_advanced_configuration')])
+
+    old_APIClient = old_apps.get_model('authentic2', 'APIClient')
+    test_client = old_APIClient.objects.create(name='foo', password='hackmeplz')
+    test_client.save()
+
+    migration.apply([('authentic2', '0051_alter_apiclient_password')])
+    from authentic2.models import APIClient
+
+    test_client = APIClient.objects.get(name='foo')
+    assert test_client.password != 'hackmeplz'
+    assert test_client.check_password('hackmeplz')