Dominus77 / yii2-advanced-start

Yii2 Start Project Advanced Template
https://dominus77.github.io/yii2-advanced-start/
MIT License
23 stars 12 forks source link

auth_key is not guaranteed to be unique #32

Closed zmoddynamics closed 4 years ago

zmoddynamics commented 4 years ago

Hello, thank you for creating this extension. In reviewing the code I see that you implement findIdentityByAccessToken in the BaseUser class.

php
// /users/models/BaseUser.php

    /**
     * @param mixed $token
     * @param mixed $type
     * @return yii\db\ActiveRecord
     */
    public static function findIdentityByAccessToken($token, $type = null)
    {
        return static::findOne(['auth_key' => $token, 'status' => self::STATUS_ACTIVE]);
    }

I assume auth_key column in the user table is meant to be used for authorization (ie. when making rest API calls to the api module). Is that the intended use ? If so, what are your thoughts on the non-uniqueness of this parameter? It appears to be generated using:

php
// /users/models/BaseUser.php
    /**
     * Generates "remember me" authentication key
     * @throws Exception
     */
    public function generateAuthKey()
    {
        /** @var yii\base\Security $security */
        $security = Yii::$app->security;
        $this->auth_key = $security->generateRandomString();
    }

Although it is unlikely, my understanding is generateRandomString() is not guaranteed to be unique and thus may present a security risk (albeit it a small one). Curious as to your thoughts?

Dominus77 commented 4 years ago

Hello! Yes, generating a not unique value is unlikely, but to ensure that it will be unique, in the rules of the User model, define attribute values as unique.

https://github.com/Dominus77/yii2-advanced-start/commit/fb3e609c927a37f9bd40e65c16658c280b8bcf1d

Dominus77 commented 4 years ago

It is also possible to create a function that checks the model attribute for uniqueness, if the generated value is not unique, generate another, until get a unique value.

zmoddynamics commented 4 years ago

Yes. I would implement both approaches you suggest for completeness. The problem with just implementing it in dB schema and rules is that it may cause validation error or database error if it is not unique when you attempt the insert. By defining a function to continuously regenerate until the fields are unique you avoid that issue.

Dominus77 commented 4 years ago

Added method generateUniqueRandomString() in BaseUser

https://github.com/Dominus77/yii2-advanced-start/blob/a1263fab6acb5812483d8600f7d7eb014735dff5/modules/users/models/BaseUser.php#L116-L131

zmoddynamics commented 4 years ago

How would you suggest I update my project to reflect these changes? composer update doesn't seem to do anything

Dominus77 commented 4 years ago

That's right, because the project was installed as a project and not an extension.

Change the following points yourself:

https://github.com/Dominus77/yii2-advanced-start/blob/a1263fab6acb5812483d8600f7d7eb014735dff5/modules/users/models/BaseUser.php#L91-L98

https://github.com/Dominus77/yii2-advanced-start/blob/a1263fab6acb5812483d8600f7d7eb014735dff5/modules/users/models/BaseUser.php#L100-L107

https://github.com/Dominus77/yii2-advanced-start/blob/a1263fab6acb5812483d8600f7d7eb014735dff5/modules/users/models/BaseUser.php#L109-L131

In order not to change it yourself, you need to fork my project into your repository, install a project for yourself from your repository, and get the latest updates from my repository for your own, then use git to update your project on the local machine.

See more: https://help.github.com/en/github/getting-started-with-github/fork-a-repo