[Security][Notifier] Added integration of Login Link with the Notifier component #38552

wouterj · 2020-10-13T15:51:54Z

Q	A
Branch?	5.x (5.2 hopefully?)
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets	-
License	MIT
Doc PR	-

This adds a LoginLinkNotification that uses the NotificationEmail and integrates with the notifier component. This makes it much easier to use the login link functionality, as it provides a default email and sms implementation.

class AuthController extends AbstractController
{
    /** @Route("/login", name="login") */
    public function login(LoginLinkHandlerInterface $loginLinkHandler, UserRepository $userRepository, Request $request, NotifierInterface $notifier)
    {
        if (!$request->isMethod('POST')) {
            return $this->redirect('/');
        }

        $user = $userRepository->findOneBy(['email' => $request->get('email')]);
        if (!$user) {
            return new Response('User not found');
        }

        $loginLink = $loginLinkHandler->createLoginLink($user);
        $notifier->send(new LoginLinkNotification($loginLink, 'Welcome to ACME!'), new Recipient($user->getEmail()));

        return new Response('Login link send!');
    }

    /** @Route("/login/check", name="check_login") */
    public function loginCheck()
    {
        throw new \BadMethodCallException();
    }
}

The NotificationEmail is slightly changed, to allow bypassing the logging-related functionality. Also, @weaverryan suggested to remove the "created by Symfony" footer as this email is meant to be sent to all users of a service.

src/Symfony/Component/Security/Http/LoginLink/LoginLinkNotification.php

src/Symfony/Bridge/Twig/Resources/views/Email/zurb_2/notification/body.html.twig

weaverryan

This is awesome!

Maybe some tests also? 😇

src/Symfony/Bridge/Twig/Mime/NotificationEmail.php

src/Symfony/Bridge/Twig/Resources/views/Email/zurb_2/notification/body.html.twig

weaverryan · 2020-10-13T18:20:28Z

src/Symfony/Component/Security/Http/LoginLink/LoginLinkNotification.php

+            $durationString = floor($hours).' hour'.($hours >= 2 ? 's' : '');
+        }
+
+        return sprintf('Click on the '.$target.' to confirm you want to sign in. This link will expire in %s.', $durationString);


Maybe just this?

Click on the $target to sign in. This link..

This text is based on the Auth0 passwordless authentication email: https://auth0.com/docs/connections/passwordless/guides/email-magic-link

I thought adding "confirm you were the person actually requesting to login" might be a good security feature to add to the mail. Do you have any suggestions on this?

Ok, good for me then 👍

I thought adding "confirm you were the person actually requesting to login"

What do you mean? It might be a good idea (like they do in the Auth0 email) to say something like:

If you did not request this, you can simply ignore this email.

Auth 0 says "Hit reply to contact us"... but (like with a reset password email), there is nobody preventing a "login link" from being emailed to you... so receiving an email doesn't seem like a "problem".

Yeah, it's not a big problem. But getting such email without you requesting it indicates something is a bit wrong. And given email are quite badly protected, it might indicate that someone has now logged in as you.

I like the idea of adding the "If you did not request this, you can simply ignore this email.".

wouterj · 2020-10-13T20:04:44Z

Thanks for the reviews! PR updated (apart from the 2 comments I replied on)

Maybe some tests also? 😇

Both classes in this PR are rather basic data objects. I've added some tests on the NotificationEmail to extend its current tests. Other Notification objects doesn't seem to have tests as well, and doing so will require a huge amount of extra packages in security-http's require-dev (see https://github.com/symfony/symfony-docs/pull/14389/files#diff-ff3b2d12d83cc148d47afab48db9d6e1c3e52574b4da40dedd17440d678f0c45R275-R277 )

weaverryan

This should be ready! It would be great to have another set of eyes on any wording, but I would love to have this for the login link system @fabpot

src/Symfony/Component/Security/Http/LoginLink/LoginLinkNotification.php

…r component

fabpot · 2020-10-14T18:49:48Z

Thank you @wouterj.

carsonbot added Status: Needs Review Security Notifier Feature labels Oct 13, 2020

noniagriconomie reviewed Oct 13, 2020

View reviewed changes

src/Symfony/Component/Security/Http/LoginLink/LoginLinkNotification.php Outdated Show resolved Hide resolved

noniagriconomie reviewed Oct 13, 2020

View reviewed changes

src/Symfony/Bridge/Twig/Resources/views/Email/zurb_2/notification/body.html.twig Outdated Show resolved Hide resolved

nicolas-grekas added this to the 5.x milestone Oct 13, 2020

weaverryan reviewed Oct 13, 2020

View reviewed changes

wouterj mentioned this pull request Oct 13, 2020

[Security] Added login link documentation symfony/symfony-docs#14389

Merged

wouterj force-pushed the security/loginlink-notifier-integration branch from 8ce2b91 to e4b42d2 Compare October 13, 2020 20:02

weaverryan approved these changes Oct 14, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 14, 2020

fabpot requested changes Oct 14, 2020

View reviewed changes

src/Symfony/Component/Security/Http/LoginLink/LoginLinkNotification.php Show resolved Hide resolved

src/Symfony/Component/Security/Http/LoginLink/LoginLinkNotification.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Reviewed labels Oct 14, 2020

wouterj force-pushed the security/loginlink-notifier-integration branch from e4b42d2 to 8bb7bac Compare October 14, 2020 18:17

wouterj added Status: Needs Review and removed Status: Needs Work labels Oct 14, 2020

fabpot approved these changes Oct 14, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 14, 2020

[Security][Notifier] Added integration of Login Link with the Notifie…

04ef565

…r component

fabpot force-pushed the security/loginlink-notifier-integration branch from 8bb7bac to 04ef565 Compare October 14, 2020 18:49

fabpot mentioned this pull request Oct 14, 2020

[Security][Notifier] Added integration of Login Link with the Notifier … symfony/symfony-docs#14413

Closed

fabpot merged commit a428b01 into symfony:5.x Oct 14, 2020

wouterj deleted the security/loginlink-notifier-integration branch October 14, 2020 18:50

fabpot mentioned this pull request Oct 14, 2020

Release v5.2.0-BETA2 #38573

Merged

Uh oh!

[Security][Notifier] Added integration of Login Link with the Notifier component #38552

[Security][Notifier] Added integration of Login Link with the Notifier component #38552

Uh oh!

Conversation

wouterj commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

weaverryan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

weaverryan Oct 13, 2020

Choose a reason for hiding this comment

Uh oh!

wouterj Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weaverryan Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

wouterj Oct 14, 2020

Choose a reason for hiding this comment

Uh oh!

wouterj commented Oct 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weaverryan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fabpot commented Oct 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

wouterj commented Oct 13, 2020 •

edited

Loading

wouterj Oct 13, 2020 •

edited

Loading

wouterj commented Oct 13, 2020 •

edited

Loading