Ryzom

Posted: **Sun May 08, 2011 11:00 pm**

Si je peux me permettre quelques critiques au niveau du code lui même après avoir fait un très rapide tour (juste ouvert 2 ou 3 fichiers) :

Code: Select all

$_SERVER["DOCUMENT_ROOT"]

Si tu veux faire du cli c'est mort, ceci n'a un sens que dans le cadre de l'utilisation avec un serveur web. Tu devrais te baser sur __DIR__ à la place. Soit dit en passant on en avais déjà parlé sur #linux de ça

Code: Select all

session_start();

$xml_config = simplexml_load_file('config.xml');

include_once($_SERVER["DOCUMENT_ROOT"].$xml_config->roster_root_directory.'/technical_class/authentification/User.php');
[...]

Et ce dans beaucoup de fichiers... il n'y a pas à dire c'est à re-desiner. Encapsule donc le système de gestion des sessions, encapsule donc également le changement de la conf, tout le monde rangé dans son coin avec une interface claire ça t'évitera de très grosses emmerdes en cas de changement futur.

Et le HTML recopié dans chaque page c'est pire que tout. À encapsuler également, et de manière urgente. Si un changement dans la gestion des sessions est relativement probable, une modif de la charte graphique par contre l'est... et aller modif chaque fichier un à un pour faire la même chose, autant se tirer une balle.

Code: Select all

    if (!User::is_user_authorize(User::ADMIN_GROUP_ID)){
        header('Location: unauthorized_access.php');
    }

/!\ ATTENTION /!\ Risque concernant la sécurité.

La fonction header() ne termine pas l'exécution du script, ce dernier va continuer bien que l'utilisateur n'ai pas les accès requis. Ainsi, n'importe quel utilisateur dispose des accès d'administrateur. Dans certains fichiers des doubles-vérifications empêchent d'effectuer les actions d'adminsitrations, mais on ne devrais pas avoir à vérifier deux fois la même chose.

Code: Select all

return $salt.sha1($salt.$pass.$site_salt);

Ou comment réinventer la roue carré comme énormément de monde. crypt() est ton amie, d'accord elle ne fais pas du sha1 mais du sha256 ou 512 c'est tellement mieux

Je te donne une astuce pour générer le sel aléatoire à lui passer en paramètre :

Code: Select all

function to64($nb)
  {
    $chrs = './0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz';
    $buff = '';

    for ($i = 0; $i < 4; $i++)
      {
         $buff .= $chrs[$nb & 0x3f];
         $nb >>= 6;
      }
    return $buff;
  }

function randStr($len)
  {
    $buff = '';
    while (strlen($buff) < $len)
      $buff .= to64(mt_rand());
    return substr($buff, 0, $len);
  }

function salt_md5()
  {
    return '$1$' . randStr(8) . '$';
  }

function salt_sha256()
  {
    return '$5$' . randStr(16) . '$';
  }

function salt_sha512()
  {
    return '$6$' . randStr(16) . '$';
  }

C'est de cette manière que PHP génère les sel aléatoires pour crypt (avec du md5 par défaut), donc on ne perd rien de ce coté.

Code: Select all

    protected function execute_sql_command($sql_cmd, $is_insert){
        try {
            $db = $this->create_db_connexion();

Help ! À chaque requête SQL tu re-créé une connexion, et en plus avant ça tu recharge intégralement un fichier xml de configuration... *glups*
Le pire c'est qu'avec PDO la connexion se ferme dès que l'objet est détruit... donc vu la manière dont tu utilises tout ça tu vas vraiment créer et fermer une connexion pour (presque) chaque requête SQL ! Pour limiter les dégâts essaye au moins d'autoriser l'utilisation des connexions persistantes, mais ça ne réglera pas le problème du chargement de fichier xml à chaque requête.

Ryzom

Tim'Roster

Re: Tim'Roster