Skip to content
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

Passwordless login #14351

Merged
merged 7 commits into from
Sep 1, 2019
Merged

Passwordless login #14351

merged 7 commits into from
Sep 1, 2019

Conversation

wuuti
Copy link
Contributor

@wuuti wuuti commented Feb 8, 2019

What does it do?

This adds the functionality of sending of a magic login link (like slack or notions do) to get you logged in to the manager instead of username/password for login.

image

image

Why is it needed?

To log in with username and password has some drawbacks:

  • you need to have a (unique) username
  • users need to choose a password or we need to generate one and communicate/send that
  • passwords need to satisfy some (custom) limits, which makes choosing the right password an errorprone process for users
  • you need to store those passwords, making this db potentially a hacking target (regardless of the hashing/salting of passwords)
  • users often don't know or don't use the password recovery function, thus producing more support effort for you.

The magic login link solution solves these problems without introducing less security (everything depends on the security of the users mail account, as with password recovery currently). Instead, we get an even better security level, as we don't need to store/exchange passwords any more and do not force our users to 'create' and remember a difficult password.

How does it work?

The function is activated by a system setting. Once you have that activated, the login screen changes to an email-only input field. After submit a one-time login link is emailed to the users mailbox if there is an account with that email address in the manager.
Internally the code just "reused" the existing password recovery functionality, so no new processors are needed here. Every time a valid login link is used, we set a temporary, random password and directly use that password internally to login via the existing processor. The links are usable only once, and have a small, configurable expiration time (default: 3600s).

Possible improvements

  • if the function is activated, do not show any password related fields in the user profile / management fields (because they are useless and confusing then)
  • maybe add a permission which re-adds a password login for certain user (groups). (Not send an email after entering the address on login, but showing a second input field for the password then).
  • if multiple users have the same email address (yes, this is possible and even configurable by a legacy setting), send out a login link to all those user accounts (resulting in multiple login links coming into the same mailbox) OR don't allow to use the method if multiple users for an email exists (Discussion needed?)

Note: this is an outcome of the MODX meetup in Engelberg@Grünenwald. It started as a POC, but we realized quickly that we don't need to change a lot of the existing code, as everything was already there in the recovery functionality.

@wuuti
Copy link
Contributor Author

wuuti commented Feb 9, 2019

I fear I can't work on that failing 7.2 test... seems not to have something to do with my code changes?

@JoshuaLuckers
Copy link
Contributor

Travis uses PHPUnit 8 for PHP 7.2, that's why it's failing. We should merge the Travis config from 2.x into 3.x

@JoshuaLuckers JoshuaLuckers added area-core pr/review-needed Pull request requires review and testing. labels Feb 9, 2019
@wuuti
Copy link
Contributor Author

wuuti commented Feb 10, 2019

Added screenshots of the changed login screen and magic link email...

$this->setPlaceholder('onManagerLoginFormPrerender', $eventInfo);

$hour = date('H') + (int)$this->modx->getOption('server_offset');
if ($hour > 18) {
$greeting = $this->modx->lexicon('login_greeting_evening');
}
elseif ($hour > 12) {
} else if ($hour > 12) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did you add these code style changes? Seems unnecessary (and not an improvement, but I know code styles are very subjective)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@wuuti Please use elseif instead of else if, since you could write:

        if ($a) {
            // code
        } elseif ($b) {
            // code
        }

instead of:

        if ($a) {
            // code
        } else {
            if ($b) {
                // code
            }
        }

If we use an autoformatter (with some strict rules) in 3.x, the second code would be generated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted any code style change (accidentally autoformatted by my IDE).

Copy link
Collaborator

@Mark-H Mark-H left a comment

Choose a reason for hiding this comment

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

I like the idea, but the implementation isn't quite ready yet. Based on a code review I've left a bunch of inline comments on specific things that should be addressed. Some of my comments may come from a lack of understanding/misunderstanding on my part in which case please do correct me.

As an aside, should this be called a "magic" login link? I like magic, but that doesn't seem like the kind of terminology the MODX core tends to use, and this is a very in-your-face type functionality that users interact with, so it's worth discussing what the exact terms we use to describe things. I'd personally drop the word "magic" and use only "login link".

manager/controllers/default/security/login.class.php Outdated Show resolved Hide resolved
manager/controllers/default/security/login.class.php Outdated Show resolved Hide resolved
manager/controllers/default/security/login.class.php Outdated Show resolved Hide resolved
manager/controllers/default/security/login.class.php Outdated Show resolved Hide resolved
manager/controllers/default/security/login.class.php Outdated Show resolved Hide resolved
manager/controllers/default/security/login.class.php Outdated Show resolved Hide resolved
manager/controllers/default/security/login.class.php Outdated Show resolved Hide resolved
manager/controllers/default/security/login.class.php Outdated Show resolved Hide resolved
} else {
$this->setPlaceholder('success_message', $this->modx->lexicon('login_user_err_nf_email'));
}
} else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This else is exposing an enumeration vulnerability allowing valid (registered) emails to be bruteforced.

To avoid that vulnerability, the response has to be identical for users that exist and users that don't. Ideally it also needs to account for the processing that happens when a user exists (parsing a message, sending an email), ensuring that timing can't be used to distinguish between a successful/failed attempt.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally it also needs to account for the processing that happens when a user exists (parsing a message, sending an email), ensuring that timing can't be used to distinguish between a successful/failed attempt.

I guess the same should apply for regular logins and password resets?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's not necessary for logins as there's either a failure or success, plus user-level brute force protection, and IIRC we addressed the difference between "user does not exist" and "password for user is incorrect" in 2.6 or so.

For password resets, yes, same protections need to apply there, but IIRC that was already addressed for 3.x when the password reset was reworked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both cases now return the same message to the login frontend.
Regarding the timing, how would you expect that to be implemented? Timing difference is minimal and processing varies a lot here. It might be possible to sleep the processing so that both e.g. take the same 1s to respond. But is that what we want here? Or just add random 200-500ms to the process? Still it depends on the connectivity when sending out the mail... it might take longer to send out an email (shouldnt, but...)

Copy link
Member

Choose a reason for hiding this comment

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

@wuuti Here's an article that deals with the varying ways to prevent timing attacks: https://blog.ircmaxell.com/2014/11/its-all-about-time.html#An-Actual-Delay-That-Works

manager/controllers/default/security/login.class.php Outdated Show resolved Hide resolved
@Jako Jako added this to the v3.0.0-alpha milestone Feb 21, 2019
@Ruslan-Aleev
Copy link
Collaborator

Ruslan-Aleev commented Mar 1, 2019

I tested and have a few comments:

  • The item "passwordless_activated" in the settings should be turned off by default, as it seems to me.

The “magic” link is sent to the mail, and everything works, it seems, but there are errors with the login:

  • I turned off this item "passwordless_activated" in the settings and log out from the manager, but I could not log in again, because passwords was changed.
  • I tried to recover the password in the standard way (Forgot your password?), the link came to the email, but the password is not reset - "The activation key does not match!", see the picture below.

login_error

And now I can not enter the manager panel in the standard way :)

@Mark-H Mark-H self-requested a review September 1, 2019 13:33
@Mark-H Mark-H self-assigned this Sep 1, 2019
Mark-H added a commit that referenced this pull request Sep 1, 2019
Merge remote-tracking branch 'upstream/pr/14351' into 3.x

* upstream/pr/14351:
  Changed registry topic.
  Removed unused code. Refactored. Adjusted lexicons.
  Remove legacy code.
  Reverted unwanted code formatting.
  Changed email string.
  Make magic login link work.
  Added first system settings for magic-login-links.
@Mark-H Mark-H merged commit 4c368c3 into modxcms:3.x Sep 1, 2019
@Mark-H
Copy link
Collaborator

Mark-H commented Sep 1, 2019

Mucho gracias, @wuuti. Love this functionality.

In the merge I've resolved some conflicts and made a couple of small tweaks.

  • To fix the issue reported by @Ruslan-Aleev, I've made sure that the magic login and password reset write to a different registry topic. That was getting mixed up, causing the reset to not find the hash.
  • In the magic login handling, it was setting a new hash to the registry with a comment indicating it was for security. However, as reading the message from the registry causes it to get removed, adding a new hash to the registry actually makes it more insecure because there's now a magic login link available that shouldn't be there.
  • Updated the settings to the new model names since the refactor.
  • Two settings were unused; removed those.
  • Added lexicons for the settings.
  • Disabled passwordless login by default. Would be a bit annyoing to have it enabled by default before email configuration is set up.
  • Slightly changed the lexicons for the user-facing bits; it's no longer a "magic", but rather a "one-time" login link, per my earlier comment.

Mark-H added a commit to Mark-H/revolution that referenced this pull request Sep 1, 2019
Mark-H added a commit to Mark-H/revolution that referenced this pull request Sep 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core pr/review-needed Pull request requires review and testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants