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

#169: Implemented email verification #222

Merged
merged 5 commits into from
Sep 11, 2017
Merged

#169: Implemented email verification #222

merged 5 commits into from
Sep 11, 2017

Conversation

samdark
Copy link
Contributor

@samdark samdark commented Sep 10, 2017

Needed for #169

@samdark samdark added this to the launch milestone Sep 10, 2017
@samdark samdark self-assigned this Sep 10, 2017
@samdark samdark requested a review from cebe September 10, 2017 22:23
@@ -32,17 +32,29 @@ public function behaviors()
'class' => AccessControl::class,
'rules' => [
[
'actions' => ['signup', 'login', 'request-password-reset', 'reset-password', 'auth'],
'actions' => [
'request-password-reset',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why allow password reset if already logged in?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Same as it was before the change. That's fine because you may want to change your password even if you're currently logged in.

if ($user) {
$user->email_verified = true;
$user->removeEmailVerificationToken();
if ($user->save()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about making this a method in User which can not fail on validation of unrelated fields?

$user->verifyEmail(); will set email_verified, remove the token and save(false);

{
if ($user) {
if (!User::isEmailVerificationTokenValid($user->email_verification_token)) {
$user->generateEmailVerificationToken();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this method should do save() internally, makes code below easier for not checking return value of save().

return $user;
}

return null;
}

private function sendEmailVerificationEmail(User $user)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is duplicated with the code from UserController, should be a single method.

$expire = Yii::$app->params['user.emailVerificationTokenExpire'];
$parts = explode('_', $token);
$timestamp = (int) end($parts);
return $timestamp + $expire >= time();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does that mean you can make a token valid again by manipulating the time?

It is not verified yet. <?= Html::a('Verify', ['user/request-email-verification']) ?>.
<?php endif ?>
</li>
<?php endif ?>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if someone has logged in via oauth there is no need to verify email. This case should be covered.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@samdark
Copy link
Contributor Author

samdark commented Sep 10, 2017

Done adjustments.

@samdark samdark merged commit 3d224d6 into master Sep 11, 2017
@samdark samdark deleted the email-verification branch September 11, 2017 09:28
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