Classe D'utilisateur PHP (login/logout/signup)

a commencé à expérimenter avec des classes de construction, et j'ai commencé en convertissant Mon inscription/connexion utilisateur en une classe unique. Voulais arrêter et demander des commentaires avant de monter trop loin. 

class UserService
{
    private $_email;
    private $_password;

    public function login($email, $password)
    {
        $this->_email = mysql_real_escape_string($email);
        $this->_password = mysql_real_escape_string($password);

        $user_id = $this->_checkCredentials();
        if($user_id){
            $_SESSION['user_id'] = $user_id;
            return $user_id;
        }
        return false;
    }

    protected function _checkCredentials()
    {
        $query = "SELECT *
                    FROM users
                    WHERE email = '$this->_email'";
        $result = mysql_query($query);
        if(!empty($result)){
            $user = mysql_fetch_assoc($result);
            $submitted_pass = sha1($user['salt'] . $this->_password);
            if($submitted_pass == $user['password']){
                return $user['id'];
            }
        }
        return false;
    }   
}

L'une des questions que j'ai liées à ma classe est: si je la construis comme ceci: 

$User = new UserService();
$User->login($_POST['email'], $_POST['password']);

où la méthode login appelle automatiquement la méthode _checkCredentials. Ou devrait-il être construit comme: 

$User = new UserService();
$UserId = $User->checkCredentials($_POST['email'], $_POST['password']);
$User->login($UserId);

autre que que j'ai amour quelques conseils sur la façon de restructurer ceci et s'il vous plaît pointez tout ce que je fais de mal!

merci les gars

23
demandé sur Gumbo 2011-01-16 21:03:00

4 réponses

je pense que votre idée principale était de séparer le traitement de l'utilisateur (session) de la requête de la base de données , ce qui est une bonne chose à mon avis.

cependant, ce n'est pas le cas avec votre mise en œuvre réelle, parce que login échappe aux données à envoyer à la base de données, même si le reste de la méthode n'a rien à voir avec les bases de données. Ne pas dire que votre requête de base de données dépend d'une ressource globale pour fonctionner . Pendant que j'y suis, je vous suggérerai aussi D'utiliser L'AOP.

Aussi, vos propriétés $_email et $_password sont dans le privé, mais sont accessibles par une méthode protégée. Cela peut causer des problèmes. les propriétés et la méthode doivent avoir une visibilité équivalente .

maintenant, je peux voir que votre UserService nécessite trois choses : un gestionnaire de base de données, un courriel et un mot de passe. Il du sens mettre dans un constructeur .

Voici comment j'allais le faire: 

class UserService
{
    protected $_email;    // using protected so they can be accessed
    protected $_password; // and overidden if necessary

    protected $_db;       // stores the database handler
    protected $_user;     // stores the user data

    public function __construct(PDO $db, $email, $password) 
    {
       $this->_db = $db;
       $this->_email = $email;
       $this->_password = $password;
    }

    public function login()
    {
        $user = $this->_checkCredentials();
        if ($user) {
            $this->_user = $user; // store it so it can be accessed later
            $_SESSION['user_id'] = $user['id'];
            return $user['id'];
        }
        return false;
    }

    protected function _checkCredentials()
    {
        $stmt = $this->_db->prepare('SELECT * FROM users WHERE email=?');
        $stmt->execute(array($this->email));
        if ($stmt->rowCount() > 0) {
            $user = $stmt->fetch(PDO::FETCH_ASSOC);
            $submitted_pass = sha1($user['salt'] . $this->_password);
            if ($submitted_pass == $user['password']) {
                return $user;
            }
        }
        return false;
    }

    public function getUser()
    {
        return $this->_user;
    }
}

puis l'utiliser comme tel: 

$pdo = new PDO('mysql:dbname=mydb', 'myuser', 'mypass');

$userService = new UserService($pdo, $_POST['email'], $_POST['password']);
if ($user_id = $userService->login()) {
    echo 'Logged it as user id: '.$user_id;
    $userData = $userService->getUser();
    // do stuff
} else {
    echo 'Invalid login';
}
36
répondu netcoder 2012-08-20 22:07:50

j'ai dit beaucoup de choses sur stackoverflow avant, mais ce que je pense que vous faites mal est que vous à nouveau essayez de créer un système de connexion(Même Jeff Atwood est d'accord avec moi sur ce point) qui va probablement être dangereux. Juste pour nommer quelques choses qui pourraient aller mal :

  • vous ne faites pas d'authentification via une connexion sécurisée (https) ce qui signifie que votre nom d'utilisateur / mot de passe ça pourrait être reniflé par le fil.
  • il pourrait avoir XSS-hole.
  • les mots de passe ne sont pas stockés de façon sécuritaire dans la base de données en raison d'une utilisation incorrecte du sel. Vous n'êtes 't un expert en sécurité donc je ne pense pas que vous devriez même stocker des informations sensibles dans votre base de données de toute façon!
  • il a un CSRF - trou.

alors il y a l'ennui que nous reste à créer un autre compte sur votre serveur. Vous pourriez Et devriez éviter ces tracas en utilisant l'une des alternatives librement disponibles qui ont été testées pour les vulnérabilités de sécurité par des experts:

  • openid = > Lightopenid est une bibliothèque très facile à utiliser/intégrer. Même stackoverflow / jeff atwood l'utilise parce qu'il sait qu'il est difficile d'obtenir le login-système correctement. Même si vous êtes un expert en sécurité.
  • Google friend connect.
  • Facebook connect.
  • twitter single sign-in.

donc vous-même en sécurité le temps de concevoir à nouveau un autre système de connexion et à la place d'utiliser par exemple la bibliothèque lightopenid vraiment simple et laissez les utilisateurs s'y connecter avec compte google. L'extrait ci-dessous est le seul code dont vous avez besoin pour le faire fonctionner: 

<?php
# Logging in with Google accounts requires setting special identity, so this example shows how to do it.
require 'openid.php';
try {
    $openid = new LightOpenID;
    if(!$openid->mode) {
        if(isset($_GET['login'])) {
            $openid->identity = 'https://www.google.com/accounts/o8/id';
            header('Location: ' . $openid->authUrl());
        }
?>
<form action="?login" method="post">
    <button>Login with Google</button>
</form>
<?php
    } elseif($openid->mode == 'cancel') {
        echo 'User has canceled authentication!';
    } else {
        echo 'User ' . ($openid->validate() ? $openid->identity . ' has ' : 'has not ') . 'logged in.';
    }
} catch(ErrorException $e) {
    echo $e->getMessage();
}
14
répondu Alfred 2017-05-23 11:47:23

cela dépend de votre conception et de la notion d'une solution plus simple, et comment vous voulez organiser votre code et le garder plus simple, plus petit et le rendre maintenable en même temps.

demandez-vous pourquoi vous appelleriez checkCredentials , puis login et peut-être une autre méthode. Avez-vous une bonne raison de le faire, est-ce une bonne conception, ce que je veux réaliser avec cette.

appelant juste login , et exécutant chaque opération de connexion est beaucoup plus simple et élégant. Lui donner un autre nom tout en faisant ainsi est beaucoup plus compréhensible et aussi plus maintenable.

si vous voulez mon avis, j'utiliserais un constructeur.

1
répondu Secko 2011-01-16 18:24:25

la réponse dépend vraiment d'autres considérations d'architecture, Comme la question de savoir si la méthode checkCredentials() doit être disponible en dehors de la portée de la classe pour les autres éléments du système. Si son seul but est d'être appelé par la méthode login (), envisagez de combiner les deux en une seule méthode.

une autre recommandation que je pourrais avoir est d'utiliser des verbes dans les méthodes de nommage, ce qui signifie que 'login' peut être trop général pour comprendre ses effets à première vue.

0
répondu Dennis Kreminsky 2018-08-30 14:41:46