Open musou1500 opened 5 years ago
I think you should simplify this idea/suggestion because it's not clear what proposed changes your trying to make. It's already understood that email verification at the moment needs authentication, which isn't ideal.
My current fix for this is to override the verify
method on the controller and auto logout/login the user if necessary so that verification works.
namespace App\Http\Controllers\Auth;
use Auth;
use Illuminate\Http\Request;
use App\Http\Controllers\Controller;
use Illuminate\Foundation\Auth\VerifiesEmails;
class VerificationController extends Controller
{
/*
|--------------------------------------------------------------------------
| Email Verification Controller
|--------------------------------------------------------------------------
|
| This controller is responsible for handling email verification for any
| user that recently registered with the application. Emails may also
| be re-sent if the user didn't receive the original email message.
|
*/
use VerifiesEmails {
verify as parentVerify;
}
/**
* Where to redirect users after verification.
*
* @var string
*/
protected $redirectTo = '/';
/**
* Create a new controller instance.
*
* @return void
*/
public function __construct()
{
$this->middleware('auth')->except('verify');
$this->middleware('signed')->only('verify');
$this->middleware('throttle:6,1')->only('verify', 'resend');
}
/**
* Mark the authenticated user's email address as verified.
*
* @param \Illuminate\Http\Request $request
*
* @return \Illuminate\Http\Response
*
* @throws \Illuminate\Auth\Access\AuthorizationException
*/
public function verify(Request $request)
{
if ($request->user() && $request->user() != $request->route('id')) {
Auth::logout();
}
if (! $request->user()) {
Auth::loginUsingId($request->route('id'), true);
}
return $this->parentVerify($request);
}
}
As far as I can see, there aren't any security implications in doing this as the URL signature has already been verified and any tampering with it (including attempting to login as someone else) will be met with a 403 error. But if anyone can see any issues with this implementation then please let me know.
@garygreen thank you.
I think you should simplify this idea/suggestion because it's not clear what proposed changes your trying to make.
Is it means that it's better to provide a more concrete way to how to improve it? if so, I will be able to add some code.
@garygreen
Sorry. I am wondering why I wrote customizable response
section. I simplified the issue.
And thank you for posting your code. I checked it. I think to introduce guard factory ($this->guard()
) to support multi-auth may be better.
I also disabled auth on my Application. Since the Signup is for a App there is no need to ever login to a website. So there is also no need to Build a Login-Area for App-Users just for that case if they want to verify their e-mail... Imho a Signed URL with expiredate (which is already configurable) is enough.
This should be configurable so it can be turned off via a simple config setting, rather than having to override the verify
method.
I wrote draft implementation based on this code. https://gist.github.com/musou1500/5f6d2ff3b6e9344d2df21f04abb8267a
I don‘t like the idea of signing in someone with loginById. This “could” be a potential risk if someone really “guesses” the correct url. I personally think the E-Mail should just get verified no matter if you’re logged in or not. (I.e. only remove the auth middleware)
Wouldn't it make more sense to just do something like below? If a user is logged in and tries to verify a different account, or if no user is logged in and no UserProvider can be found, it'll throw an AuthorizationException, otherwise it will verify the user. The login state of the user won't be changed this way.
/**
* Mark the authenticated user's email address as verified.
*
* @param \Illuminate\Http\Request $request
* @param \Illuminate\Auth\AuthManager $auth
* @return \Illuminate\Http\Response
* @throws \Illuminate\Auth\Access\AuthorizationException
*/
public function verify(Request $request, AuthManager $auth)
{
if (
($request->user() && $request->route('id') != $request->user()->getKey()) ||
(! $request->user() && ! $auth->createUserProvider())
) {
throw new AuthorizationException;
}
$provider = $auth->createUserProvider();
$user = $request->user() ?: $provider->retrieveById($request->route('id'))
if ($user->hasVerifiedEmail()) {
return redirect($this->redirectPath());
}
if ($user->markEmailAsVerified()) {
event(new Verified($user));
}
return redirect($this->redirectPath())->with('verified', true);
}
@riyuk
I don‘t like the idea of signing in someone with
loginById
. This “could” be a potential risk if someone really “guesses” the correct URL.
certainly, force switching account by loginById
may be unexpected behavior for a user and debatable.
on the other hand, in terms of security, I didn't worry about "potential risk" (I thought HMAC is secure against known-plaintext attacks). I'm not a security expert, so it needs a review.
@CupOfTea696
How is the following code?
This uses onceUsingId
and guard
factory function.
public function verify(Request $request)
{
if (! $this->guard()->onceUsingId($request->route('id'))) {
throw new AuthorizationException;
}
$user = $this->guard()->user();
if ($user->hasVerifiedEmail()) {
return redirect($this->redirectPath());
}
if ($user->markEmailAsVerified()) {
event(new Verified($user));
}
return redirect($this->redirectPath())->with('verified', true);
}
This would allow a user to verify another account, though. But I guess that's not that much of a security issue as they could log out before following the verification link.
I personally think we should allow 3 different scenarios, all of them configurable:
default-verify
)guest-verify
) *crossaccount-verify
) ** As a supplementary extra, I think we should allow forcing login under the verified account, too (also a configurable option autologin
). From a user's experience perspective, it's highly frustrating being able to verify an account but yet you are prompted to login (I'm sure you've all experienced this with some sites?)
@garygreen
can you explain the use case of default-verify
? (is it for backward compatibility?)
Allow verification only logged in under the same credentials (as it is, currently - this will remain the default-verify)
I think having a signed URL is enough to prove that the user is the same as a person that requested email verification.
Just come up with a simpler option which uses a middleware to auto login/login before verification. Starting to think maybe this should live in userland?
namespace App\Http\Middleware;
use Illuminate\Http\Request;
class AutoLoginVerifyMiddleware
{
/**
* Automatically logout/login user being verified.
*
* @param \Illuminate\Http\Request $request
* @param Closure $next
* @return mixed
*/
public function handle(Request $request, $next)
{
$verifyUserId = $request->route('id');
// Support cross-user verification.
if ($request->user() && $request->user() != $verifyUserId) {
$request->guard()->logout();
}
// Support guest verification.
if (! $request->user()) {
$request->guard()->loginUsingId($verifyUserId, true);
}
return $next($request);
}
}
@garygreen thank you. I like this middleware. certainly, this middleware enables cross-user/guest verification in userland. but I believe that it's worth putting in the framework(Apart from how to achieve it).
* As a supplementary extra, I think we should allow forcing login under the verified account, too (also a configurable option
autologin
). From a user's experience perspective, it's highly frustrating being able to verify an account but yet you are prompted to login (I'm sure you've all experienced this with some sites?)
I think most sites require a user to login (again) if the user is not already logged in (in that browser). i still don't think it's a "good practice" to login a user just by a token... (i haven't actually seen one site doing this ... ever)
Guess the best way would be to allow anonymous verification (example for this are Apps where laravel is 'just' the backend/API) and - by default - authorized verification (where laravel is also the backend/website which has a members area) of the e-mail address.
For your use-case (autologin a user by a verification e-mail) you should use a custom middleware to do so - as you do.
@riyuk I think this middleware supports anonymous verification. what is the use case that is not covered by this middleware? https://github.com/laravel/ideas/issues/1632#issuecomment-491527352
BTW, I am glad if I can make a PR.
Could you let me know the process to propose an idea on laravel/idea
to Laravel's main repository?
I starting to think a discussion about the implementation detail may have to do on the laravel/framework
, if we have an agreement that "the feature is necessary" at least.
@riyuk I think this middleware supports anonymous verification. what is the use case that is not covered by this middleware? #1632 (comment)
This middleware actively logs users out, which is not something I would want to do.
@CupOfTea696 IMO, in many cases, the user wants to use the application as a newly registered user after email verification. but it may be better to provide a choice about changing the auth state or not.
I mean, personally I'm not even sure if I'd want to allow a signed in user to verify another user.
@CupOfTea696 How is the following code? This uses
onceUsingId
andguard
factory function.public function verify(Request $request) { if (! $this->guard()->onceUsingId($request->route('id'))) { throw new AuthorizationException; } $user = $this->guard()->user(); if ($user->hasVerifiedEmail()) { return redirect($this->redirectPath()); } if ($user->markEmailAsVerified()) { event(new Verified($user)); } return redirect($this->redirectPath())->with('verified', true); }
I have tried to use it adding $this->middleware('autoLoginVerify')->only('verify'); above $this->middleware('signed')->only('verify'); but get an Exception "Method Illuminate\Http\Request::guard does not exist." Someone had this problem?
Logging users automatically can create some security vulnerability. Instead of this just verify an email address and in case something went wrong only an email will be verified.
My solution is to override the verify method in VerificationController. Not checking if a user logged in only a signature.
<?php
namespace App\Http\Controllers\Auth;
use App\Http\Controllers\Controller;
use Illuminate\Foundation\Auth\User;
use Illuminate\Foundation\Auth\VerifiesEmails;
use Illuminate\Http\Request;
use Illuminate\Auth\Events\Verified;
use Illuminate\Auth\Access\AuthorizationException;
class VerificationController extends Controller
{
/*
|--------------------------------------------------------------------------
| Email Verification Controller
|--------------------------------------------------------------------------
|
| This controller is responsible for handling email verification for any
| user that recently registered with the application. Emails may also
| be re-sent if the user didn't receive the original email message.
|
*/
use VerifiesEmails{
verify as public traitVerify;
}
/**
* Where to redirect users after verification.
*
* @var string
*/
protected $redirectTo = RoutePath::REDIRECT_AFTER_LOGIN;
/**
* Create a new controller instance.
*
* @return void
*/
public function __construct()
{
$this->middleware('auth')->only('show', 'resend');
$this->middleware('signed')->only('verify');
$this->middleware('throttle:6,1')->only('verify', 'resend');
}
public function verify(Request $request)
{
$user = User::findOrFail($request->route('id'));
if ($user->hasVerifiedEmail()) {
return redirect($this->redirectPath());
}
if ($user->markEmailAsVerified()) {
event(new Verified($user));
}
return redirect($this->redirectPath())->with('verified', true);
}
}
For a project I'm currently working on I was also having some issues with this. When a user registers I do want to send him the verification mail. I do NOT want to get him logged in after registration! Let him verify the ownership of the mail adres, and then let him log in by himself.
I've not rewritten the registration/verification controller yet, but I'm going the same way as how password resets are being taken care of by default. This will will not introduce a new vulnerability, and in my opinion will be even a little more secure because nowhere the userid will be visible. I hope to find the time for it tomorrow. When I'm done I will share what I have done exactly.
@SuidingNL what do you think about my solution?
The proposed solution seems decent, but I would like to make sure the url can't be manipulated that easily. Like you said; "in case something went wrong only an email will be verified". I would like to limit the risks, and want to really make sure it was the user itself that verified it.
On registration, you often find a ToS which you need to agree with. This is where I want to limit risks.
Let's say, I want to trick someone else into some ToS. I would need to register myself first. In your solution I would receive a mail with the URL containing the userid. Quickly after I register myself, I register my 'target'. Based on the id I received earlier for verification, I can try some numbers above it. Depending on the popularity and registrations per minute, this has a high change of only 1 try to be successful (for startups/least populair sites).
I would like to come back on your solution. I totally forgot about the signed middleware. Having this with it does make your proposal secure!
However, I'm still not a fan of putting the userid into the url.
Thanks for sharing your thoughts. You are putting encrypted user id, right? I think it should be ok.
My solution is to override the verify method in VerificationController
in order to add the user to the Request
:
<?php
namespace App\Http\Controllers\Auth;
use App\Http\Controllers\Helpers\BaseController;
use App\Models\User;
use App\Providers\RouteServiceProvider;
use Illuminate\Auth\Events\Verified;
use Illuminate\Foundation\Auth\VerifiesEmails;
use Illuminate\Http\Request;
/**
* Verification.
*
* @see https://laravel.com/docs/7.x/verification
* @see https://medium.com/@pran.81/how-to-implement-laravels-must-verify-email-feature-in-the-api-registration-b531608ecb99
* @see https://github.com/laravel/ideas/issues/1632#issuecomment-575903992
*/
class VerificationController extends BaseController
{
/*
|--------------------------------------------------------------------------
| Email Verification Controller
|--------------------------------------------------------------------------
|
| This controller is responsible for handling email verification for any
| user that recently registered with the application. Emails may also
| be re-sent if the user didn't receive the original email message.
|
*/
use VerifiesEmails {
verify as public traitVerify;
}
/**
* Where to redirect users after verification.
*
* @var string
*/
protected $redirectTo = RouteServiceProvider::HOME;
/**
* Create a new controller instance.
*
* @return void
*/
public function __construct()
{
$this->middleware('auth')->only('show', 'resend');
$this->middleware('signed')->only('verify', 'resend');
$this->middleware('throttle:6,1')->only('verify', 'resend');
}
public function verify(Request $request)
{
$user = User::findOrFail($request->route('id'));
$request->setUserResolver(function () use ($user) {
return $user;
});
return $this->traitVerify($request);
}
}
After lots of headaches, this is how I made it work.
First make a new custom request type with php artisan make:request SignedEmailVerificationRequest
app/Http/Requests/SignedEmailVerificationRequest.php
<?php
namespace App\Http\Requests;
use Illuminate\Foundation\Auth\EmailVerificationRequest;
/**
* Class SignedEmailVerificationRequest
*
* A request that authorizes the user based on the route signature.
*
* @package App\Http\Requests
*/
class SignedEmailVerificationRequest extends EmailVerificationRequest
{
/**
* Determine if the user is authorized to make this request.
*
* @return bool
*/
public function authorize()
{
if (! $this->hasValidSignature()) {
return false;
}
auth()->loginUsingId($this->route('id'));
return parent::authorize();
}
}
routes/web.php
Your verification.verify
route which the user will visit in the browser should look something like this:
<?php
use Illuminate\Support\Facades\Route;
Route::get('email/verify/{id}/{hash}', function(\App\Http\Requests\SignedEmailVerificationRequest $request) {
$request->fulfill();
return 'Email verified.';
})->name('verification.verify')->middleware(['signed']);
@DanielSchetritt Good to hear that you could solve that. But, the loginUsingId now must use Auth instead of auth(). Other than that, I think in my case the code still not work. Everytime I try to verify my registration, I am always redirected to login route due to the app failed to log me in.
Logging users automatically can create some security vulnerability. Instead of this just verify an email address and in case something went wrong only an email will be verified.
My solution is to override the verify method in VerificationController. Not checking if a user logged in only a signature.
<?php namespace App\Http\Controllers\Auth; use App\Http\Controllers\Controller; use Illuminate\Foundation\Auth\User; use Illuminate\Foundation\Auth\VerifiesEmails; use Illuminate\Http\Request; use Illuminate\Auth\Events\Verified; use Illuminate\Auth\Access\AuthorizationException; class VerificationController extends Controller { /* |-------------------------------------------------------------------------- | Email Verification Controller |-------------------------------------------------------------------------- | | This controller is responsible for handling email verification for any | user that recently registered with the application. Emails may also | be re-sent if the user didn't receive the original email message. | */ use VerifiesEmails{ verify as public traitVerify; } /** * Where to redirect users after verification. * * @var string */ protected $redirectTo = RoutePath::REDIRECT_AFTER_LOGIN; /** * Create a new controller instance. * * @return void */ public function __construct() { $this->middleware('auth')->only('show', 'resend'); $this->middleware('signed')->only('verify'); $this->middleware('throttle:6,1')->only('verify', 'resend'); } public function verify(Request $request) { $user = User::findOrFail($request->route('id')); if ($user->hasVerifiedEmail()) { return redirect($this->redirectPath()); } if ($user->markEmailAsVerified()) { event(new Verified($user)); } return redirect($this->redirectPath())->with('verified', true); } }
This works fine, but there is one issue with this if user requested another verification link then can verify email with previous email verification link.
I propose that enable Email Verification without authentication. this issue is related to https://github.com/laravel/framework/issues/28454.
Problem
Email Verification requires authentication(see following code)
https://github.com/laravel/framework/blob/5.8/src/Illuminate/Foundation/Auth/VerifiesEmails.php#L35
This can be a problem in a specific use case. For example, in a typical mobile application, users will take the following steps to register.
then, users will be redirected to the login page because the web browser has not any credentials. it may be a bother for a user.
Improvement
I think just checking the signature is sufficient. if my understanding is correct, HMAC(Signed URL uses it under the hood) is secure against known-plaintext attacks.
Signed URL documentation also describes a similar use case for example.
here is a draft implementation. https://gist.github.com/musou1500/5f6d2ff3b6e9344d2df21f04abb8267a
middleware style solution is also posted. https://github.com/laravel/ideas/issues/1632#issuecomment-491503943