Saeven / zf3-circlical-user

Turnkey Authentication, Identity, and RBAC for Laminas and Zend Framework 3. Supports Doctrine and Middleware.
Mozilla Public License 2.0
36 stars 15 forks source link

Make Auto-Login Optional After User Create #16

Closed Tigerman55 closed 7 years ago

Saeven commented 7 years ago

First, thanks for taking some of your personal time to create this PR, I really appreciate the contribution!

There's a few things we should consider at this point before we merge in:

In AuthenticationService:

1: There are two elements that should be wrapped in the "condition"

$this->setSessionCookies($auth);
$this->setIdentity($user);

The cookies and the identity essentially go hand in hand.

2: Adding the modifications caused tests to fail. It's always good to run phpunit or phpspec tests before pushing code up to make sure that your changes don't break tests. Travis is reporting errors - if you click the 'details' link it highlights the failure at https://travis-ci.org/Saeven/zf3-circlical-user/builds/173076427 - this gives you a chance to fix it (moot point though, read on!).

The stuff above is the easy stuff to fix, the one I'm struggling with is...

One function with two semantics

I worry about/hate introducing two ways to do things through a single function, it always ends up with users breaking things down the road (think of an admin panel that doesn't RTFM and ends up logging out each time they create a user or some such). They'll blame the lib and open issues; in the end, they wouldn't be totally wrong to raise a stink because the problem is in the code.

I'm thinking we should introduce two calls, and maybe clear up the semantic. Maybe we give them more verbose names:

public function create(User $user, $username, $password): AuthenticationRecordInterface

vs.

public function registerAuthentication(User $user, $username, $password): AuthenticationRecordInterface

You'd end up with something like this:

/**
 * Register a new user into the auth tables, and, log them in.
 *
 * @param User   $user
 * @param string $username
 * @param string $password
 *
 * @return AuthenticationRecordInterface
 * @throws EmailUsernameTakenException
 * @throws MismatchedEmailsException
 * @throws UsernameTakenException
 */
public function create(User $user, string $username, string $password): AuthenticationRecordInterface
{
    $auth = $this->registerAuthenticationRecord($user, $username, $password);
    $this->setSessionCookies($auth);
    $this->setIdentity($user);

    return $auth;
}

/**
 * Very similar to create, except that it won't log the user in.  This was created to satisfy circumstances where
 * you are creating users from an admin panel for example.  This function is also used by create.
 *
 * @param User   $user
 * @param string $username
 * @param string $password
 *
 * @return AuthenticationRecordInterface
 * @throws EmailUsernameTakenException
 * @throws MismatchedEmailsException
 * @throws UsernameTakenException
 */
private function registerAuthenticationRecord(User $user, string $username, string $password): AuthenticationRecordInterface
{
    if ($this->authenticationProvider->findByUsername($username)) {
        throw new UsernameTakenException();
    }

    if (filter_var($username, FILTER_VALIDATE_EMAIL)) {
        if ($user->getEmail() != $username) {
            throw new MismatchedEmailsException();
        }

        if ($emailUser = $this->userProvider->findByEmail($username)) {
            if ($emailUser != $user) {
                throw new EmailUsernameTakenException();
            }
        }
    }

    $hash = password_hash($password, PASSWORD_DEFAULT);
    $auth = $this->authenticationProvider->create(
        $user->getId(),
        $username,
        $hash,
        KeyFactory::generateEncryptionKey()->getRawKeyMaterial()
    );
    $this->authenticationProvider->save($auth);

    return $auth;
}

If we stick to this recipe, we won't break tests. Coverage will decrease but I can deal with that later unless you feel like writing tests.

Saeven commented 7 years ago

Hey Tiger! Was guessing you didn't have a chance to fix! I went ahead and did it -- thanks again for contributing this idea to the lib. Makes a ton of sense.

https://github.com/Saeven/zf3-circlical-user/pull/17