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

yweber · 2024-04-17T14:41:42+02:00

yweber commented

2024-04-17 14:41:42 +02:00

Je ne suis vraiment pas certain de ne pas m'être embarqué sur une mauvaise piste d'implémentation

yweber added 1 commit 2024-04-17 14:41:43 +02:00

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

yweber force-pushed wip/89456-api-client-password from 308fd38115 to 1f734b3ab6

2024-04-17 14:42:18 +02:00

Compare

yweber force-pushed wip/89456-api-client-password from 1f734b3ab6 to b9f51f7c2a

2024-04-17 14:44:59 +02:00

Compare

bdauvergne requested changes 2024-04-17 14:58:43 +02:00

bdauvergne left a comment

Ça m'a l'air ok, juste un souci pour la migration, il faudrait créer d'abord ton nouveau champ sous le nom new_password. Faire une migration de donnée pour prendre les valeurs en clair, les hasher, les déplacer dans le nouveau champ et à la fin supprimer l'ancien champ et renommer le nouveau.

Ensuite je ne sais pas dans quel mesure on peut reprendre le code de User.check_password() qui s'occupe aussi de mettre à jour le hachage utilisé régulièrement pendant la validation (par exemple Django augmente régulièrement le nombre de round de hachage).

    def check_password(self, raw_password):
        """
        Return a boolean of whether the raw_password was correct. Handles
        hashing formats behind the scenes.
        """
        def setter(raw_password):
            self.set_password(raw_password)
            # Password hash upgrades shouldn't be considered password changes.
            self._password = None
            self.save(update_fields=["password"])
        return check_password(raw_password, self.password, setter)

Ça m'a l'air ok, juste un souci pour la migration, il faudrait créer d'abord ton nouveau champ sous le nom new_password. Faire une migration de donnée pour prendre les valeurs en clair, les hasher, les déplacer dans le nouveau champ et à la fin supprimer l'ancien champ et renommer le nouveau. Ensuite je ne sais pas dans quel mesure on peut reprendre le code de User.check_password() qui s'occupe aussi de mettre à jour le hachage utilisé régulièrement pendant la validation (par exemple Django augmente régulièrement le nombre de round de hachage). ``` def check_password(self, raw_password): """ Return a boolean of whether the raw_password was correct. Handles hashing formats behind the scenes. """ def setter(raw_password): self.set_password(raw_password) # Password hash upgrades shouldn't be considered password changes. self._password = None self.save(update_fields=["password"]) return check_password(raw_password, self.password, setter) ```

src/authentic2/api_views.py Outdated

						
				@ -1472,2 +1472,3 @@

				        try:

				            api_client = APIClient.objects.get(identifier=identifier, password=password)

				            api_client = APIClient.objects.get(identifier=identifier)

				            if not api_client.check_password(password):

bdauvergne commented

2024-04-17 14:53:36 +02:00

Tu peux mettre or not api_client.check_password() or ans le if plus loin.

Tu peux mettre `or not api_client.check_password() or` ans le `if` plus loin.

yweber commented

2024-04-17 15:25:37 +02:00

Bien vu, merci !

yweber marked this conversation as resolved

src/authentic2/authentication.py Outdated

						
				@ -73,2 +73,3 @@

				        try:

				            return APIClient.objects.get(identifier=userid, password=password), None

				            api_client = APIClient.objects.get(identifier=userid)

				            if not api_client.check_password(password):

bdauvergne commented

2024-04-17 14:51:17 +02:00

Juste faire if check_password(): return api_cient, None non ? Pas la peine de nettoyer la variable api_client.

Juste faire `if check_password(): return api_cient, None` non ? Pas la peine de nettoyer la variable api_client.

yweber commented

2024-04-17 15:25:58 +02:00

Complètement ! merci :)

yweber marked this conversation as resolved

src/authentic2/models.py Outdated

						
				@ -759,2 +765,4 @@

				        return False

				    def check_password(self, password):

				        del self.password

bdauvergne commented

2024-04-17 14:48:59 +02:00

Là j'ai un peu du mal à comprendre.

yweber commented

2024-04-17 15:14:35 +02:00

Ouai, c'est un peu bizarre (et ça mérite clairement un commentaire).

Au moment ou on créé une instance avec APIClient.objects.create(..., password='toto') le champ s'occupe bien de hasher le mot de passe avant de l'insérer en DB, mais l'attribut password vaudra encore la valeur 'toto' non hashé.

Du coup le del de l'attribut force à recharger la valeur de l'attribut depuis la DB et self.password aura bien en valeur le mot de passe hashé.

Je ne suis pas certain que cette situation se produise dans un cas "réel", mais pour certain tests oui. Peut être qu'une meilleure solution serait de modifier les tests en question ?

Ouai, c'est un peu bizarre (et ça mérite clairement un commentaire). Au moment ou on créé une instance avec `APIClient.objects.create(..., password='toto')` le champ s'occupe bien de hasher le mot de passe avant de l'insérer en DB, mais l'attribut `password` vaudra encore la valeur `'toto'` non hashé. Du coup le `del` de l'attribut force à recharger la valeur de l'attribut depuis la DB et `self.password` aura bien en valeur le mot de passe hashé. ---- Je ne suis pas certain que cette situation se produise dans un cas "réel", mais pour certain tests oui. Peut être qu'une meilleure solution serait de modifier les tests en question ?

yweber marked this conversation as resolved

yweber commented

2024-04-17 15:19:52 +02:00

Ça m'a l'air ok, juste un souci pour la migration, il faudrait créer d'abord ton nouveau champ sous le nom new_password. Faire une migration de donnée pour prendre les valeurs en clair, les hasher, les déplacer dans le nouveau champ et à la fin supprimer l'ancien champ et renommer le nouveau.

Ah oui merci !

Ensuite je ne sais pas dans quel mesure on peut reprendre le code de User.check_password() qui s'occupe aussi de mettre à jour le hachage utilisé régulièrement pendant la validation (par exemple Django augmente régulièrement le nombre de round de hachage).

Très bonne idée, merci, je vais essayer de faire ça !

edit : Je ne sais pas s'il y avait une raison particulière pour découper en plusieurs migrations, mais comme il y a un moyen de le faire en une seule je propose d'abord cette solution.

> Ça m'a l'air ok, juste un souci pour la migration, il faudrait créer d'abord ton nouveau champ sous le nom new_password. Faire une migration de donnée pour prendre les valeurs en clair, les hasher, les déplacer dans le nouveau champ et à la fin supprimer l'ancien champ et renommer le nouveau. Ah oui merci ! > Ensuite je ne sais pas dans quel mesure on peut reprendre le code de User.check_password() qui s'occupe aussi de mettre à jour le hachage utilisé régulièrement pendant la validation (par exemple Django augmente régulièrement le nombre de round de hachage). > Très bonne idée, merci, je vais essayer de faire ça ! ---- edit : Je ne sais pas s'il y avait une raison particulière pour découper en plusieurs migrations, mais comme il y a un moyen de le faire en une seule je propose d'abord cette solution.

yweber force-pushed wip/89456-api-client-password from b9f51f7c2a to 8f137a709a

2024-04-17 15:25:23 +02:00

Compare

yweber force-pushed wip/89456-api-client-password from 8f137a709a to 153727ad3b

2024-04-17 16:17:12 +02:00

Compare

yweber force-pushed wip/89456-api-client-password from 153727ad3b to 01e8fb310c

2024-04-17 16:22:23 +02:00

Compare

yweber force-pushed wip/89456-api-client-password from 01e8fb310c to 36d7f0ff0d

2024-04-17 16:40:52 +02:00

Compare

yweber changed title from ~~WIP: api: change client's password handling (#89456)~~ to api: change client's password handling (#89456)

2024-04-17 16:49:40 +02:00

yweber requested review from bdauvergne 2024-04-17 16:49:45 +02:00

yweber force-pushed wip/89456-api-client-password from 36d7f0ff0d to 0c24f87890

2024-04-17 16:54:49 +02:00

Compare

yweber force-pushed wip/89456-api-client-password from 0c24f87890 to 34e6cd3712

2024-04-17 16:57:44 +02:00

Compare

yweber reviewed 2024-04-17 16:58:10 +02:00

yweber force-pushed wip/89456-api-client-password from 34e6cd3712 to 9c73efa703

2024-04-17 17:01:43 +02:00

Compare

yweber reviewed 2024-04-17 17:07:31 +02:00

src/authentic2/migrations/0051_alter_apiclient_password.py Outdated

						
				@ -0,0 +16,4 @@

				def fake_hash_reverse(apps, schema_editor):

				    APIClient = apps.get_model('authentic2', 'APIClient')

				    for dummy in APIClient.objects.all():

				        raise NotImplementedError('Not reversible')

yweber commented

2024-04-17 17:07:31 +02:00

Je ne suis pas certain que ça soit la meilleur idée, mais j'ai pas trouvé mieux pour faire comme si la migration était reversible pour permettre les tests de migration (lors des tests il n'y a rien à "reverse" donc ça ne raise pas).

bdauvergne commented

2024-04-26 15:37:24 +02:00

Ça ira.

bdauvergne commented

2024-04-17 18:24:33 +02:00

edit : Je ne sais pas s'il y avait une raison particulière pour découper en plusieurs migrations, mais comme il y a un moyen de le faire en une seule je propose d'abord cette solution.

Oui, je décrivais juste le déroulé si ça passe dans la même migration pas de soucis.

> edit : Je ne sais pas s'il y avait une raison particulière pour découper en plusieurs migrations, mais comme il y a un moyen de le faire en une seule je propose d'abord cette solution. Oui, je décrivais juste le déroulé si ça passe dans la même migration pas de soucis.

bdauvergne requested changes 2024-04-26 16:01:07 +02:00

bdauvergne left a comment

Je ne suis pas certain que le système PasswordField / PasswordInput va fonctionner, il n'y a pas de test qui teste le formulaire d'édition sans toucher au champ mot de passe mais je suppose que le widget est initialisé avec la valeur hashée du mot de passe et en sauvant ça va rehasher cette chaîne. Comme de toute façon on a pas envie que le mot de passe haché ou pas se retrouve dans le HTML de l'interface, je serais plus à l'aise si on reprenait aussi le fonctionnement de django.contrib.auth avec une fonction set_password() et des formulaire d'éditions / ajout qui excluent explicitement le champ "password" du modèle via ModelForm.Meta.exclude et une gestion de la modification mise en place du premier mot de passe dans Form.clean(), ça retirera le besoin du del self.password et on retombera sur du code quasi identique à ce qui se fait pour django.contrib.auth.models.User.

Je ne suis pas certain que le système PasswordField / PasswordInput va fonctionner, il n'y a pas de test qui teste le formulaire d'édition sans toucher au champ mot de passe mais je suppose que le widget est initialisé avec la valeur hashée du mot de passe et en sauvant ça va rehasher cette chaîne. Comme de toute façon on a pas envie que le mot de passe haché ou pas se retrouve dans le HTML de l'interface, je serais plus à l'aise si on reprenait aussi le fonctionnement de django.contrib.auth avec une fonction set_password() et des formulaire d'éditions / ajout qui excluent explicitement le champ "password" du modèle via ModelForm.Meta.exclude et une gestion de la modification mise en place du premier mot de passe dans Form.clean(), ça retirera le besoin du `del self.password` et on retombera sur du code quasi identique à ce qui se fait pour django.contrib.auth.models.User.

src/authentic2/manager/forms.py Outdated

						
				@ -212,6 +212,7 @@ class UserEditForm(LimitQuerysetFormMixin, CssClass, BaseUserForm):

				    class Meta:

				        model = User

				        widgets = {'password': forms.PasswordInput()}

bdauvergne commented

2024-04-26 16:00:59 +02:00

Plus haut ajouter un Field explicite "password" mais pas besoin d'un PasswordInput, on va devoir noter ce mot de passe dans un coin pour le filer au client plus tard de toute façon, un help text pour dire de le noter parce que c'est la dernière fois qu'on le verra en clair ce serait bien.

En édition (si self.instance.id n'est pas None) si le champ reste de vide, on conserve la valeur existante en appelant pas set_password().

Plus haut ajouter un Field explicite "password" mais pas besoin d'un PasswordInput, on va devoir noter ce mot de passe dans un coin pour le filer au client plus tard de toute façon, un help text pour dire de le noter parce que c'est la dernière fois qu'on le verra en clair ce serait bien. En édition (si self.instance.id n'est pas None) si le champ reste de vide, on conserve la valeur existante en appelant pas set_password().

yweber commented

2024-04-30 17:50:40 +02:00

Merci !

En plus je me suis rendu compte que j'étais pas sur le bon formulaire -__-

Merci ! En plus je me suis rendu compte que j'étais pas sur le bon formulaire -__-

yweber marked this conversation as resolved

src/authentic2/manager/forms.py

						
				@ -213,3 +213,4 @@

				    class Meta:

				        model = User

				        widgets = {'password': forms.PasswordInput()}

				        exclude = (

bdauvergne commented

2024-04-26 15:55:13 +02:00

Ici on ajoute password.

yweber marked this conversation as resolved

src/authentic2/models.py Outdated

						
				@ -53,6 +53,12 @@ from .utils.misc import ServiceAccessDenied

				from .utils.sms import create_sms_code

				class PasswordField(models.CharField):

bdauvergne commented

2024-04-26 15:53:56 +02:00

À mon avis on a pas besoin d'un nouveau champ de modèle, ça obscurcit un peu le code.

yweber commented

2024-04-30 16:35:04 +02:00

Oui, d'autant que comme tu l'as bien noté au dessus il va entraîner des soucis de re-hash de mot de passe déjà hashé : pas bien pratique...

yweber marked this conversation as resolved

yweber force-pushed wip/89456-api-client-password from 9c73efa703 to 787607e6ab

2024-04-30 16:32:05 +02:00

Compare

yweber changed title from ~~api: change client's password handling (#89456)~~ to WIP: api: change client's password handling (#89456)

2024-04-30 16:37:38 +02:00

yweber force-pushed wip/89456-api-client-password from 787607e6ab to 1208553533

2024-04-30 16:42:15 +02:00

Compare

yweber force-pushed wip/89456-api-client-password from 1208553533 to b2f1e45887

2024-04-30 16:42:54 +02:00

Compare

yweber changed title from ~~WIP: api: change client's password handling (#89456)~~ to api: change client's password handling (#89456)

2024-04-30 17:49:23 +02:00

yweber commented

2024-04-30 17:57:56 +02:00

Merci pour ces remarques qui m'ont bien aidés !

Du coup, au final, en me basant sur ce qui était fait dans django, j'ai ajouté un APIClientManager permettant de créer un APIClient avec sa méthode create_user() .

Par rapport à l'implémentation du manager j'ai préféré me baser sur ce qui était fait dans django (et dans custom_user de a2) et implémenter la méthode create_user plutôt que surcharger la méthode create (ce que j'aurais fait spontanément).

Je me demandais aussi dans quel mesure le code relatif à APIClient ne serait pas mieux dans dans custom_user qui dispose d'un module managers avec déjà le manager custom pour les utilisateurs custom de a2 (si j'ai bien compris) ?

Merci pour ces remarques qui m'ont bien aidés ! Du coup, au final, en me basant sur ce qui était fait dans django, j'ai ajouté un APIClientManager permettant de créer un APIClient avec sa méthode create_user() . Par rapport à l'implémentation du manager j'ai préféré me baser sur ce qui était fait dans django (et dans custom_user de a2) et implémenter la méthode create_user plutôt que surcharger la méthode create (ce que j'aurais fait spontanément). Je me demandais aussi dans quel mesure le code relatif à APIClient ne serait pas mieux dans dans custom_user qui dispose d'un module managers avec déjà le manager custom pour les utilisateurs custom de a2 (si j'ai bien compris) ?

yweber requested review from bdauvergne 2024-04-30 17:58:28 +02:00

gitea/authentic/pipeline/head This commit looks good

Details

This pull request can be merged automatically.

This branch is out-of-date with the base branch

You are not authorized to merge this pull request.

You can also view command line instructions.

Step 1:

From your project repository, check out a new branch and test the changes.

git checkout -b wip/89456-api-client-password main

git pull origin wip/89456-api-client-password

Step 2:

Merge the changes and update on Gitea.

git checkout main

git merge --no-ff wip/89456-api-client-password

git push origin main

Sign in to join this conversation.

No reviewers

No Label

No Milestone

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: entrouvert/authentic#291