8000 [Security][Notifier] Added integration of Login Link with the Notifier component by wouterj · Pull Request #38552 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 14, 2020
Merged

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

merged 1 commit into from
Oct 14, 2020

Conversation

wouterj
Copy link
Member
@wouterj wouterj commented Oct 13, 2020 •
edited
Loading
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();
    }
}

image

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.

@nicolas-grekas nicolas-grekas added this to the 5.x milestone Oct 13, 2020
weaverryan
weaverryan reviewed Oct 13, 2020
View reviewed changes
Copy link
Member
@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

This is awesome!

Maybe some tests also? 😇

src/Symfony/Component/Security/Http/LoginLink/LoginLinkNotification.php Outdated
$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);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe just this?

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

Copy link
Member Author
@wouterj wouterj Oct 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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 wouterj mentioned this pull request Oct 13, 2020
Merged
@wouterj wouterj force-pushed the security/loginlink-notifier-integration branch from 8ce2b91 to e4b42d2 Compare October 13, 2020 20:02
@wouterj
Copy link
Member Author
wouterj commented Oct 13, 2020 •
edited
Loading

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
weaverryan approved these changes Oct 14, 2020
View reviewed changes
Copy link
Member
@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

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

fabpot
fabpot requested changes Oct 14, 2020
View reviewed changes
@wouterj wouterj force-pushed the security/loginlink-notifier-integration branch from e4b42d2 to 8bb7bac Compare October 14, 2020 18:17
@fabpot fabpot force-pushed the security/loginlink-notifier-integration branch from 8bb7bac to 04ef565 Compare October 14, 2020 18:49
@fabpot
Copy link
Member
fabpot commented Oct 14, 2020

Thank you @wouterj.

@fabpot fabpot mentioned this pull request Oct 14, 2020
Closed
@fabpot fabpot merged commit a428b01 into symfony:5.x Oct 14, 2020
@wouterj wouterj deleted the security/loginlink-notifier-integration branch October 14, 2020 18:50
@fabpot fabpot mentioned this pull request Oct 14, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noniagriconomie noniagriconomie noniagriconomie left review comments

@fabpot fabpot fabpot approved these changes

@weaverryan weaverryan weaverryan approved these changes

Assignees
No one assigned
Labels
Feature Notifier Security Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
6 participants
@wouterj @fabpot @weaverryan @noniagriconomie @nicolas-grekas @carsonbot
0