developer.jelix.org is not used any more and exists only for history. Post new tickets on the Github account.
developer.jelix.org n'est plus utilisée, et existe uniquement pour son historique. Postez les nouveaux tickets sur le compte github.

Opened 13 years ago

Closed 13 years ago

#229 closed enhancement (fixed)

jAuth : Ajout d'une fonctionnalité "Se souvenir de moi"

Reported by: antoinefr Owned by: antoinefr
Priority: normal Milestone: Jelix 1.0beta3
Component: jelix:auth Version: 1.0 beta2.1
Severity: normal Keywords:
Cc: Blocked By:
Blocking: Documentation needed:
Hosting Provider: Php version:

Description

Il serait intéressant d'avoir une fonctionnalité "Se souvenir de moi" pour jAuth, permettant à l'utilisateur qui le souhaite d'éviter d'avoir à se re-connecter lors des prochaines visites du site.

Voir fil sur le forum : http://jelix.org/forums/read.php?5,1121

Attachments (3)

patch_ade_040807.diff (20.7 KB) - added by antoinefr 13 years ago.
Patch relatif à l'évol #229
patch_ade_060807.diff (19.3 KB) - added by antoinefr 13 years ago.
Nouvelle version du patch, corrigé
patch_ade_070807.diff (21.0 KB) - added by antoinefr 13 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 13 years ago by antoinefr

  • Owner set to antoinefr
  • Status changed from new to assigned

Changed 13 years ago by antoinefr

Patch relatif à l'évol #229

comment:2 Changed 13 years ago by laurentj

Index: lib/jelix/plugins/coord/auth/auth.coord.php

+            global $gJCoord;
+            $plugin = $gJCoord->getPlugin('auth');
+            if($plugin === null){
+                throw new jException('jelix~auth.error.plugin.missing');
+            }
+            $authConfig = & $plugin->config;

Pourquoi faire ça ? Tu es déjà dans le plugin que tu récupères. et la configuration est dans $this->config.

+            if($_COOKIE[$cookieName]){

Ça provoque une notice ce genre de test. Vaut mieux faire un isset.

+                if(!jCrypt::isMcryptInstalled()){

Je ne suis pas d'accord avec ça. L'utilisateur de jCrypt ne devrait pas avoir à tester si mcryp est installé ou pas. jCrypt devrait comporter juste deux méthodes (encrypt et decrypt) et c'est jcrypt qui se débrouille d'utiliser la meilleur façon d'encrypter. Sinon jCrypt n'aurait plus trop d'interêt.

+                    $user=substr($cookieValue,0,strpos($cookieValue,"="));
+                    $encryptedPassword=substr(strstr($cookieValue,"="),1);

Plutôt que d'avoir à faire ce genre de manipulation (coûteuse au niveau perf), vaut peut être mieux stocker le login et le mot de passe dans deux cookies différents, en utilisant les tableaux par exemple : setcookie ("toto[login]", $login) et setcookie("toto[pwd]", $login) (où toto est la valeur de $persistant_cookie_name), et tu récupère les cookies dans $_COOKIEtoto?login? et $_COOKIEtoto?pwd? ;-).

+                    $encryptedPassword=base64_decode($encryptedPassword);

Pourquoi encoder/decoder le mot de passe crypté en base 64 ?

Index: lib/jelix/utils/jCrypt.class.php

+    public static function isMcryptInstalled(){

Non, je préfère des méthodes génériques qui devinent toutes seules la méthode de cryptage à utiliser

+    public static function mcryptGenerateIV($mcryptAlgo=MCRYPT_XTEA,$mcrypMode=MCRYPT_MODE_ECB){

Pourquoi avoir ça ? mcryptEncrypt et mcryptDecrypt ne peuvent pas appeler elle même mcrypt_module_open &co ?

+    public static function mcryptEncrypt($string,$iv,$key,$mcryptAlgo=MCRYPT_XTEA,$mcryptMode=MCRYPT_MODE_ECB){
+    public static function simpleEncrypt($str,$key){

Ces deux méthodes devraient être en protected, et une seule nouvelle méthode publique, "encrypt". Enlever aussi ces paramètres $mcryptAlgo, $mcryptMode : jCrypt devrait utiliser des valeurs par défaut, non redéfinissable. Si vraiment l'utilisateur a besoin d'autres types de cryptage, il utilisera mcrypt directement. Gardons jCrypt simple d'utilisation.

+    public static function mcryptDecrypt($string,$iv, $key,$mcrypAlgo=MCRYPT_XTEA,$mcryptMode=MCRYPT_MODE_ECB){
+    public static function simpleDecrypt($string,$key){

idem que précédent, avec une nouvelle méthode publique "decrypt".

Index: lib/jelix/auth/jAuth.class.php

+                if($config['persistant_enable']){

Il est préférable de faire aussi un isset, pour éviter les notices (le user ne définit pas forcément tout, ou ne met pas forcément à jour son fichier de conf quand il migre vers une version plus récente. Cette remarque est valable pour toutes les autres paramètres de config un peu partout dans le patch.

J'attend une nouvelle version du patch :-)

comment:3 Changed 13 years ago by laurentj

désolé, dans le commentaire précédent il faut lire :

 $_COOKIE['toto']['login'] et $_COOKIE['toto']['pwd']

comment:4 Changed 13 years ago by antoinefr

Je prend bonne note de tes remarques, je me remet au travail pour corriger ces points :-)

Pour récapituler :

  • Je corrige la façon de récupérer les params de config dans le plugin coord ($this->config)
  • Je corrige mes tests avec des isset
  • J'utilise deux cookies, un pour le password, un pour le login
  • Je passe en protected mcryptEncrypt, mcryptDecrypt, simpleEncrypt et simpleDecrypt, je crée deux methodes publiques decrypt et encrypt qui se chargent d'encrypter/decrypter la chaine (en determinant automatiquement si mcrypt est présent ou non).
  • Je choisit en dur l'algo et le mode mcrypt utilisé pour crypter.

Par contre, quelques remarques :

  • Les algo de cryptage mcrypt générent des données binaires, que j'encode en base 64 pour stocker sous forme de string dans le cookie. Sinon risque de problèmes avec des caractères speciaux.
  • Le vecteur d'initialisation (VI) utilisé par mcrypt pour (de)crypter les chaines est créé de manière aléatoire à chaque cryptage (dans mon cas). J'ai donc besoin de le stocker dans le cookie pour le réutiliser lors du décryptage du password. Je passe par une autre méthode (mcryptGenerateIV) pour créer le VI car je souhaitais que mcryptEncrypt me retourne uniquement la chaine cryptée. Si mcryptEncrypt crée elle-même le VI (ce qui est plus simple d'utilisation, en effet), la méthode devrait retourner le password ET le VI (j'ai besoin de récupéré ce VI !). C'était simplement pour une 'cohérence' de l'API de jCrypt (entre simpleEncrypt et mcryptEncrypt) que j'ai choisi cette solution. Dans le cas contraire, mcryptEncrypt me retournera un tableau avec la chaine cryptée et le VI...

Et par contre mcryptDecrypt recevra toujours en paramètre ce VI !

  • Je serai toujours obligé de découper la chaine contenant le password dans le cas d'un crytage par mcrypt pour récupérer ce VI. A moins de passer par un 3ème cookie qui contient le vecteur, et dans ce cas la methode publique jCrypt::decrypt devra, si elle détérmine que mcrypt est présent, récupérer ce dernier cookie... je trouve pas ça top... a moins finalement d'utiliser tout le temps le même vecteur d'initialisation (puisque le choix de l'algo n'est plus libre), mais où le stocker ?

Je suis tout à fait d'accord avec toi pour garder jCrypt simple d'utilisation. Pour ma défense ;-), je souhaitais quand même laissé une certaine souplesse à l'utilisateur au niveau du paramètrage (au niveau du choix de l'algo par exemple, tous ne sont peut être pas disponible sur toutes les plateformes, certains sont plus sûr/lent que d'autres...), et souhaitais que jCrypt puisse être utilisé dans d'autres cas d'utilisation que les cookies.

comment:5 Changed 13 years ago by laurentj

  • Les algo de cryptage mcrypt générent des données binaires, que j'encode en base 64 pour stocker sous forme de string dans le cookie. Sinon risque de problèmes avec des caractères speciaux.

dans ce cas là, pourquoi cet encodage en base 64 n'est pas fait directement dans la classe jCrypt ? ;-) Ou il n'y a pas d'autres algo qui génèrent une chaine non binaire ?

Le vecteur d'initialisation (VI) utilisé par mcrypt pour (de)crypter les chaines est créé de manière aléatoire à chaque cryptage (dans mon cas). J'ai donc besoin de le stocker dans le cookie pour le réutiliser lors du décryptage du password.

Ok je comprends.. Il n'y a pas toutefois un moyen d'intégrer ce VI dans la chaine retournée, genre "valeurVI:mot de passe crypté", et mcryptDecrypt s'attendrait à ce format de chaine, et donc pourrait découper la chaine et ainsi récupérer les deux valeurs ? ainsi ça resterait transparent dans l'utilisation de jCrypt (peut être qu'il faut choisir un caractère séparateur autre que ":", mais l'idée est là). Ou encore, n'y a t'il pas d'autres algo dans mcrypt qui n'a pas besoin de ce VI ?

  • Je serai toujours obligé de découper la chaine contenant le password dans le cas d'un crytage par mcrypt pour récupérer ce VI.

oui c'est ce que je propose. mais au moins c'est transparent d'un point de vu utilisation.

A moins de passer par un 3ème cookie qui contient le vecteur, et dans ce cas la methode publique jCrypt::decrypt devra, si elle détérmine que mcrypt est présent, récupérer ce dernier cookie

non non, parce que la chaine cryptée peut être utilisée dans un autre contexte, stockage dans une base, dans un fichier par ex, donc il n'y aurait pas de cookie là :-)

je souhaitais quand même laissé une certaine souplesse à l'utilisateur au niveau du paramètrage (au niveau du choix de l'algo par exemple, tous ne sont peut être pas disponible sur toutes les plateformes, certains sont plus sûr/lent que d'autres...)

Oui mais adieu l'API unifiée, à moins d'inclure les algo en PHP... faut voir.

Changed 13 years ago by antoinefr

Nouvelle version du patch, corrigé

comment:6 Changed 13 years ago by antoinefr

J'ai trouvé un algo mcrypt permettant d'éviter d'avoir à utiliser un vecteur d'initialisation, ce qui simplifie l'utilisation de la classe jCrypt, effectivement !

Sauf erreur de ma part, j'ai corrigé les problèmes que tu avais remontés... j'attend tes remarques ;-)

comment:7 Changed 13 years ago by chris

En cours de test de ce nouveau patch (que je dois à chaque fois modifier pour enlever les références à build/manifests/jelix-lib.mn, et lib/jelix/core-modules/jelix/locales/en_{EN,US}/auth.UTF-8.properties qui n'existent pas dans la nightly), j'ai régulièrement un warning :

[warning 1] mdecrypt_generic() [<a href='function.mdecrypt-generic'>function.mdecrypt-generic</a>]: An empty string was passed /var/www/jelix/lib/jelix/utils/jCrypt.class.php 79

En particulier quand j'avais l'ancien cookie, et quand je me déloggue de mon appli. Je n'ai pas encore trouvé la raison (à peine regardé).

comment:8 Changed 13 years ago by antoinefr

En particulier quand j'avais l'ancien cookie, et quand je me déloggue de mon appli. Je n'ai pas encore trouvé la raison (à peine regardé).

Ce qui est sûr c'est que ça ne peux pas marcher avec les cookies créés par la version précédente du patch (le format des cookies à changé) donc il faut supprimer tes cookies avant d'utiliser ton appli avec cette version du patch.

Pour ma part lors de mes tests je n'ai pas rencontré ce problème, par contre ce message d'erreur peut survenir lorsque tu essaye de te connecter sans mot de passe (mcrypt essaye de chiffrer une 'empty string'), il y a peut etre un test à rajouter pour éviter l'affichage de cette erreur...

Je vais continuer à regarder d'où cela peut venir. Peux-tu tester de ton côté (en supprimant tes anciens cookies) et me décrire un scénario qui permet de reproduire cette erreur ?

comment:9 Changed 13 years ago by antoinefr

OK, je viens de reproduire ce bug, ce qui est étrange c'est que j'ai fait exactement les mêmes tests chez moi hier (sous Linux) ça marche sans problème et là (sous Windows) j'ai bien l'erreur que tu décris, elle survient systèmatiquement à la deconnexion lorsque la persistance de la session est activée dans la config. Je fais la correction tout de suite mais je ne pourrai pas fournir la version corrigée du patch avant ce soir !

comment:10 follow-up: Changed 13 years ago by antoinefr

Pour info, l'erreur est dans la méthode

public function beforeAction ($params)

de la classe AuthCoordPlugin? (jelix\plugins\coord\auth\auth.coord.php).

Lors de la deconnection, les cookies ('login' et 'passwd') sont vidés, et lors de l'execution de l'action suivant la deconnexion, dans le plugin coord on récupère le contenu de ces cookies vides pour tenter une connection, ce qui provoque une erreur mcrypt (decryptage d'une chaine vide). Une solution consiste à faire un test dans la méthode beforeAction pour vérifier que le cookie contenant le password n'est pas vide...

J'attache une nouvelle version du patch corrigé ce soir... desolé pour cette erreur bête, mais sur ma plateforme de dev l'erreur ne se produisait pas :-)

comment:11 Changed 13 years ago by laurentj

Index: lib/jelix/plugins/coord/auth/auth.coord.php

+        if(jAuth::isPersistant() && !jAuth::isConnected() && isset($this->config['persistant_cookie_name']) && isset($this->config['persistant_crypt_key'])){

l'appel de jAuth::isPersistant() est un peu overkill. En effet cet méthode récupère le présent plugin pour tester dans sa config. Pour des raisons de performances, je pense qu'il est préférable de tester directement ici 'persistant_enable' dans le $this->config ;-). De plus la ligne est un peu longue, il faudrait y introduire un saut de ligne.

Je pense aussi qu'il est préférable de séparer les tests en deux, et de tester la presence de persistant_cookie_name et de persistant_crypt_key, et si ils ne sont pas présent, générer une erreur. Car ici, si le developpeur oublie ces paramètres, la persistance ne fonctionne pas mais elle reste silencieuse ! Le dev risque de passer un bout de temp à chercher pourquoi. Je ferais plutôt donc un truc comme :

    if (isset($this->config['persistant_enable']) && $this->config['persistant_enable'] && !jAuth::isConnected()) {
        if (isset($this->config['persistant_cookie_name']) && isset($this->config['persistant_crypt_key'])) {
            // le reste du code
        } else {
           throw new jException("...."); // erreur indiquant qu'il manque des options
        }
     }

Dans mcryptEncrypt et mcryptDecrypt, il faudrait tester la longueur de la clé, et générer une erreur si elle est trop courte ou vide, comme dans simpleCrypt.

+    protected static function simpleCrypt($str,$key){
+        if($key=='')
+            return $str;

Peut être que l'on pourrait améliorer ça en générant une erreur quand la clé est vide plutôt que de retourner la chaine non crypté. Parce que si la clé est vide (par mégarde), il n'y a plus de cryptage, ce qui constitue un trou de sécurité.

Index: lib/jelix/auth/jAuth.class.php

+            if($persistant){
+                if(isset($config['persistant_enable']) && isset($config['persistant_crypt_key']) && isset($config['persistant_cookie_name'])){
+	                if($config['persistant_enable']){

Je pense que l'on peut améliorer ces tests en :

            if($persistant && isset($config['persistant_enable']) && $config['persistant_enable']) { // si la persistance est activée
                if(!isset($config['persistant_crypt_key']) || !isset($config['persistant_cookie_name'])){ // vérification que l'on a tout
                      throw new jException()...// même erreur que dans le plugin
                }
                $cookieDuration=24*3600;
                //etc...
             }
+        if(isset($config['persistant_enable']) && isset($config['persistant_cookie_name'])){
+            if($config['persistant_enable']){
+                setcookie($config['persistant_cookie_name']."[login]");
+                setcookie($config['persistant_cookie_name']."[passwd]");
+            }
+        }

même réorganisation des tests comme précédement : on test d'abord si c'est enable. Si oui, on teste si on a toutes les informations et si ce n'est pas le cas, erreur.

À part ces quelques remarques, le reste est bon (modulo le bug que vous avez trouvé :-) )

comment:12 Changed 13 years ago by antoinefr

OK pour réorganiser les tests et lever des exceptions si la config n'est pas correcte, ou si la clé n'est pas valide.

Pour ce qui est de jAuth::isPersistant(), elle a bien sûr plus vocation à être appelée depuis les controllers que depuis le plugin. Je vais donc récupérer l'info directement dans la conf.

comment:13 in reply to: ↑ 10 Changed 13 years ago by chris

Replying to antoinefr:

desolé pour cette erreur bête, mais sur ma plateforme de dev l'erreur ne se produisait pas :-)

Il n'y a pas de mal voyons. Le but étant de chasser les bugs ;-)

Changed 13 years ago by antoinefr

comment:14 Changed 13 years ago by antoinefr

Nouvelle version du patch attachée.

Une petite précision concernant ta remarque Laurent :

Dans mcryptEncrypt et mcryptDecrypt, il faudrait tester la longueur de la clé, et générer une erreur si elle est trop courte ou vide, comme dans simpleCrypt.

L'algo utilisé par mcrypt ne nécessite pas une longueur de clé minimale, donc je vérifie simplement qu'elle ne soit pas vide.

comment:15 Changed 13 years ago by laurentj

  • Milestone set to Jelix 1.0beta3

comment:16 Changed 13 years ago by laurentj

  • Resolution set to fixed
  • Status changed from assigned to closed

patch ok. Inclus dans le trunk. Merci !

Note: See TracTickets for help on using tickets.