Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eliminate dependency on email address as a reliable handle for user #523

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

tblanchard
Copy link

Not all my users have email, some have more than one email. Since an email is just a bit of contact info, I don't have it as a field in user. I have it as a related table so users can have multiple. This keeps confide from being useful to me and anyone else who didn't tie email address to user identity.

The most egregious evidence of this is

    public function userByResetPasswordToken($token)
    {
        $email = $this->passService->getEmailByToken($token);
        if ($email) {
            return $this->repo->getUserByEmail($email);
        }
        return false;
    }

because I can't look up a user by email. This change set adds the primary key for the user to the password reset table.

    public function requestChangePassword(RemindableInterface $user)
    {
        $token = $this->generateToken();

        $values = array(
            'user_id' => $user->getAuthIdentifier(),
            'email'=> $user->getReminderEmail(),
            'token'=> $token,
            'created_at'=> new DateTime
        );

        $table = $this->getTable();

        $this->app['db']
            ->connection($user->getConnectionName())
            ->table($table)
            ->insert($values);

        $this->sendEmail($user, $token);

        return $token;
    }

and so now the user is looked up by primary key instead of by email. There are a few more changes around making email optional in the database when generating controllers and database tables and there is more support for using username (although this isn't necessarily unique in my system either).

@coveralls
Copy link

Coverage Status

Coverage decreased (-23.51%) to 74.83% when pulling bbfa55f on tblanchard:master into f9c3d09 on Zizaco:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants