Skip to content

Added "remember me" feature in the login form #1356

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

Merged
merged 1 commit into from
Nov 16, 2022
Merged

Added "remember me" feature in the login form #1356

merged 1 commit into from
Nov 16, 2022

Conversation

artyuum
Copy link
Contributor

@artyuum artyuum commented Aug 24, 2022

"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?

@ihmels
Copy link
Contributor

ihmels commented Sep 12, 2022

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');
-        }

        // ...
    }

    // ...
}

@artyuum
Copy link
Contributor Author

artyuum commented Sep 28, 2022

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

@ihmels
Copy link
Contributor

ihmels commented Sep 28, 2022

@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.

@artyuum
Copy link
Contributor Author

artyuum commented Sep 28, 2022

Apparently the IS_AUTHENTICATED_FULLY attribute is only used there:

#[IsGranted('IS_AUTHENTICATED_FULLY')]

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.

// if user is already logged in, don't display the login page again

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?

@javiereguiluz javiereguiluz merged commit 5a37aa1 into symfony:main Nov 16, 2022
@javiereguiluz
Copy link
Member

Thank you @artyuum for contributing this feature.

At the end, I opted for using IS_AUTHENTICATED as you said in #1356 (comment) so I did that change while merging.

@artyuum artyuum deleted the ft/remember-me branch November 16, 2022 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants