demo icon indicating copy to clipboard operation
demo copied to clipboard

Added "remember me" feature in the login form

Open artyuum opened this issue 3 years ago • 1 comments

"Remember me" is a common functionality in login forms and I noticed that the demo application does not have it.

This PR is a small change and any user can easily find that in the documentation, but I believe that this is still a relevant addition in order to make this demo a bit more "complete" and show that Symfony can provide common functionalities out of the box.

I also didn't find any discussion (issue/pr) about this functionality so I don't know whether it was intended to not have it in the demo app.

What do you think?

artyuum avatar Aug 24 '22 18:08 artyuum

Maybe we should remove the check, if the user is already logged in in the SecurityController::login() action so that users can re-enter their passwords, when IS_AUTHENTICATED_FULLY is required.

class SecurityController extends AbstractController
{
    // ...

    #[Route('/login', name: 'security_login')]
    public function login(Request $request, AuthenticationUtils $helper): Response
    {
-        // if user is already logged in, don't display the login page again
-        if ($this->getUser()) {
-            return $this->redirectToRoute('blog_index');
-        }

        // ...
    }

    // ...
}

ihmels avatar Sep 12 '22 13:09 ihmels

@ihmels I'm not sure how this is related to my PR.

artyuum avatar Sep 28 '22 17:09 artyuum

@artyuum If you add the remember me functionallity, users who are logged in only because of a remember me cookie will have IS_AUTHENTICATED_REMEMBERED, but will not have IS_AUTHENTICATED_FULLY. If you then check for the IS_AUTHENTICATED_FULLY special attribute (via AuthorizationCheckerInterface::isGranted('IS_AUTHENTICATED_FULLY')) the user will be redirected to the login form. In this case you want the user to re-enter their password. If you do not remove the check I mentioned in SecurityController::login() the user will be redirected to the blog_index route and will not get the IS_AUTHENTICATED_FULLY attribute.

ihmels avatar Sep 28 '22 18:09 ihmels

Apparently the IS_AUTHENTICATED_FULLY attribute is only used there: https://github.com/symfony/demo/blob/ecf3f8526c59ebe6cb68b2b79fac5251c9430607/src/Controller/BlogController.php#L100

I suggest replacing this attribute with IS_AUTHENTICATED and keeping the condition in the login controller to prevent the user from accessing the login page while they are already logged in. https://github.com/symfony/demo/blob/ecf3f8526c59ebe6cb68b2b79fac5251c9430607/src/Controller/SecurityController.php#L35

Unless we really want to enforce this level of security (requiring the user to re-enter their password again before submitting a comment just because they have been logged-in again thanks to the remember me cookie)...

WDYT?

artyuum avatar Sep 28 '22 18:09 artyuum

Thank you @artyuum for contributing this feature.

At the end, I opted for using IS_AUTHENTICATED as you said in https://github.com/symfony/demo/pull/1356#issuecomment-1261305666 so I did that change while merging.

javiereguiluz avatar Nov 16 '22 16:11 javiereguiluz