Navigation Menu

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, password reset via email & 2FA support #2923

Merged
merged 5 commits into from Nov 17, 2020

Conversation

lukasbestle
Copy link
Member

@lukasbestle lukasbestle commented Nov 9, 2020

Describe the PR

This PR adds the logic and UI for creating and verifying login codes to log in to the Panel, which results in the following features:

  • Passwordless login to the Panel via email
  • Password reset via email
  • 2FA (or actually two-step auth) via password and an email code

Login methods

There are now three ways to get into the Panel that can be configured with the panel.login.methods option:

  1. Passwordless login (code method)
    Kind of like Notion: The user enters their email address, gets a code and logs in.
  2. Password reset (password-reset method)
    The user enters the email address, gets a code and logs in with it. After logging in, the user will be redirected to the password reset form. After confirming the new password, the user is redirected to the Panel dashboard.

The third and default method is password, which will use the same UI and features as previously (i.e. the passwordless login form is disabled). This is because not everyone needs/wants password reset and also because the email configuration needs to be set up and working.

For the password method, 2fa can be enabled like this:

<?php

return [
  'panel.login.methods' => [
    'password' => ['2fa' => true]
  ]
];

It is also possible to combine the methods:

<?php

return [
  'panel.login.methods' => ['password', 'password-reset']
];
<?php

return [
  'panel.login.methods' => ['code', 'password']
];

The first method in the list will be the default.

Other syntax variants that are supported:

<?php

return [
  // just a string
  'panel.login.methods' => 'password',

  // boolean
  'panel.login.methods' => ['password' => true]
];

Other configuration options

The login codes expire after 10 minutes by default. This can be configured with the panel.login.timeout option.

The email that gets sent can be fully customized with panel.login.email.from, panel.login.email.fromName, panel.login.email.subject and email body templates in site/templates/emails/auth/login[.html].php and site/templates/emails/auth/password-reset[.html].php.

Usage from site templates & plugins

With the $kirby->auth()->createChallenge(), verifyChallenge() and login2fa() methods, users can use the feature themselves for frontend login forms etc.

Implementation

The implementation was designed according to OWASP recommendations, except that:

  • our implementation uses login codes instead of URLs (for best compatibility and to avoid that the link opens in a different default browser – note that the code needs to be entered in the same browser where it was created) and that
  • the user gets automatically logged in after (or actually before) resetting the password (contrary to what OWASP says, that actually made the implementation less complex in this case).

Regarding the UI: I have copied over the user info styles @bastianallgeier has written for the password reset form to the login code form. Is it intended that the styles are now duplicated? If not, please move them over to where they belong.

Following PR

This PR already prepares plugin extensions for auth challenges (SMS, TOTP, hardware tokens...) that will be implemented in a second PR.

Related issues

Ready?

  • Added unit tests for fixed bug/feature
  • Passing all unit tests
  • Fixed code style issues with CS fixer and composer fix
  • Added in-code documentation (if needed)

@lukasbestle lukasbestle added the type: feature 🎉 Adds a feature (requests → feedback.getkirby.com) label Nov 9, 2020
@lukasbestle lukasbestle added this to the 3.5.0 milestone Nov 9, 2020
@lukasbestle lukasbestle self-assigned this Nov 9, 2020
@bastianallgeier
Copy link
Member

bastianallgeier commented Nov 10, 2020

Here are my first thoughts:

  1. THIS IS AWESOME!!!! ❤️
  2. I kind of forgot about all our discussions, but my initial feeling was that there should also be a mode in which login via password is completely blocked. But it's quite possible that I forgot a part of our discussion in which we already covered that.
  3. Everything seems super clean to me, except the template names for the emails. Would anything speak against: /templates/email/auth/login.html.php and /templates/email/auth/reset-password.html.php?
  4. I'm quite happy with the look of the links in the forms to be honest. I think they work great so far and a more general redesign of the forms can still follow later.
  5. I wonder if the Auth code format would even be more accessible/readable with a dash instead of a space. 123-456

@lukasbestle
Copy link
Member Author

I kind of forgot about all our discussions, but my initial feeling was that there should also be a mode in which login via password is completely blocked. But it's quite possible that I forgot a part of our discussion in which we already covered that.

I don't think we discussed this already, but I may also have missed it.

What would be the use-case for such a mode that only allows login without password?

Everything seems super clean to me, except the template names for the emails. Would anything speak against: /templates/email/auth/login.html.php and /templates/email/auth/reset-password.html.php?

Good idea, I will change that!

I wonder if the Auth code format would even be more accessible/readable with a dash instead of a space. 123-456

To be honest I haven't seen this format anywhere so far, I personally prefer the space. Is there a specific reason why you prefer the dash?

@bastianallgeier
Copy link
Member

What would be the use-case for such a mode that only allows login without password?

I was thinking of larger teams where a lot of users would go for weak passwords. I've seen this in client projects a lot. You can set rules, but then people still stick to their birthday and their last name 😂 It's a bit of a risky mode as it has to be clear that email transport is working properly, but then it's also a lot safer in such situations.

To be honest I haven't seen this format anywhere so far, I personally prefer the space. Is there a specific reason why you prefer the dash?

It was just an initial thought. I got a couple codes via email afterwards and it totally works fine with the space. Just forget about it. There's not really a need to change it.

@lukasbestle
Copy link
Member Author

I was thinking of larger teams where a lot of users would go for weak passwords. I've seen this in client projects a lot. You can set rules, but then people still stick to their birthday and their last name 😂 It's a bit of a risky mode as it has to be clear that email transport is working properly, but then it's also a lot safer in such situations.

Good point! But instead of preventing login with password, wouldn't it be better to introduce an option (maybe in the user blueprint) that prevents users from setting a password? Then the admins could set a password and login with it, while the editors would only be able to login via email.

@bastianallgeier
Copy link
Member

Oh, that's even better!

@bastianallgeier
Copy link
Member

But we can do that in a different PR and maybe even later. Let's keep the modes like they are.

@lukasbestle
Copy link
Member Author

lukasbestle commented Nov 10, 2020

I thought about it again: Maybe we should offer both options. Some devs may want to disable passwords completely, some may want to disable them for specific roles.

We could have the modes email, email-password, password, password-reset and password+2fa.
Because the email mode would do a different thing in this setup as in the current implementation, we can only decide about it now.

The option to disable setting passwords per role could indeed come later.

@bastianallgeier
Copy link
Member

I haven't thought this through, but could something like this work instead? The modes seem to get a bit complicated to understand.

return [
  'panel' => [
    'login' => [
        'via' => ['email', 'password'],
        '2fa' => true
    ],
    'reset-password' => true
  ]
];

@bastianallgeier
Copy link
Member

Obviously, password-reset: true would need to be disabled automatically as soon as login via password is disabled. I'm not sure if this actually ends up to be more clear. But in the end we might also have something like this later (i.e. via plugins) and we might want to plan ahead for that:

return [
  'panel' => [
    'login' => [
        'via' => ['email', 'password', 'github'],
        '2fa' => true
    ],
    'reset-password' => true
  ]
];

@lukasbestle
Copy link
Member Author

lukasbestle commented Nov 10, 2020

To be honest I think this syntax is too complex. It's easy to understand from the user's perspective, but we would need to test every combination of option values. Some combinations are impossible, for example both 2FA and password reset (you could circumvent the 2FA via password reset).

The fixed mode strings ensure that only those combinations are possible that are secure and supported. We will list and explain the modes in the docs, so it should be pretty simple to pick one.

Regarding completely different login methods like GitHub: It's really difficult to integrate these into the core, even when they are defined by plugins. For example GitHub login would be just a button that takes you to GitHub, but that button would need to be defined by a Vue component. We already allow replacing the login form, which would need to be done for that.

I'm going to think about this again. Maybe one of us comes up with a good solution for the way forward.

@bastianallgeier
Copy link
Member

I think you are right. Maybe we need to work on the mode names instead. It's really just about understanding it without the docs. Here's a first try. What trips me up the most is the use of email and password that feels like I will disable or enable those fields with the setting. Maybe something like email-code vs. password could make clearer that this is the method of authentication and not the field, which will be enabled/disabled. Not 100% sure yet.

email-code
email-code+password
password
password+reset
password+2fa

@lukasbestle
Copy link
Member Author

You are right, it isn't very semantic and easy to understand. To be honest, I think your variant isn't much better.

However I think a variant of your original suggestion with the array could work. I will try implementing something around that.

@lukasbestle
Copy link
Member Author

What if we actually allow to configure the forms:

<?php

return [
  'panel.login' => [
    'forms' => ['email+password', 'email', 'email-pwReset'],
    'defaultForm' => 'email+password'
    '2fa' => true/false
  ]
];

If both email and email-pwReset are defined, only email-pwReset will be used (as they are two variants of the same thing).

If 2fa is set to true, the two email forms will be removed from the list.

This setup is a bit more complex, but a lot more flexible and extensible. What do you think?

@bastianallgeier
Copy link
Member

@lukasbestle to be honest, I don't think it's very intuitive either. Maybe we could use some outside help here. @afbora @distantnative what do you think? Any ideas? I'm a bit lost to be honest. It all feels like it's almost there, but not quite.

@lukasbestle
Copy link
Member Author

One thing to keep in mind: The mode option should be neutral about the code challenge that is used (e.g. email, TOTP, SMS...). The challenge will be determined using a different mechanism.

So this is only about whether login with any kind of code is enabled at all and about the UI that is displayed.

@afbora
Copy link
Member

afbora commented Nov 11, 2020

I do not have very good knowledge but maybe I can give examples of social provider logins as an idea.
Each provider can be adjusted individually. Thus, I think that new modes/providers can be easily added or modified with plugins.

It may seem confusing, but it will be understandable if we show all the modes/providers in the documents as lists or tables.

return [
    'panel' => [
        'login' => [
            // Default login mode
            'mode' => 'email-password',

            // Available login modes/providers
            'providers' => [
                'email-password' => [
                    'name' => 'Email-Password',
                    '2fa' => true,
                    'reset-password' => true,
                ],
                'single-auth-code' => [
                    'name' => 'Single Auth Code',
                    '2fa' => false,
                    'reset-password' => false,
                ],
                'github' => [
                    'name' => 'GitHub',
                    '2fa' => false,
                    'reset-password' => false,
                    'options' => [
                        'api_key' => 'XXX',
                        'secret_key' => 'XXX',
                    ]
                ],
            ],
        ]
    ]
];

@lukasbestle
Copy link
Member Author

You are right about plugin configuration!

However I think it's very verbose for our core modes. 😕

@lukasbestle
Copy link
Member Author

lukasbestle commented Nov 11, 2020

A variation of @afbora's suggestion:

<?php

return [
  'panel.login' => [
    'methods' => [
      // core method with options
      'password' => [
        '2fa' => true,

        // the first method in the list that has this set to `true` will be the default;
        // maybe we even go without this (see the open question below)
        'default' => true
      ],
      
      // core methods that are just enabled without options
      // (automatic translation of array values to array keys when the key is numeric)
      'password-reset',
      'code',

      // also supported syntax variants:
      // 'password-reset' => true
      // 'password-reset' => []

      // third-party methods (not yet supported, for future expansion)
      'github' => [
        'apiKey' => 'xxx',
        'secretKey' => 'yyy'
      ]
    ]
  ]
];

A very simple config could look like this:

<?php

return [
  'panel.login' => [
    'methods' => ['password', 'password-reset']
  ]
];

My thoughts behind this:

  • Renamed providers to login methods as that's a less technical term that is easier to understand by users.
  • 2FA only works with some methods (password and third-party methods), so it can only be configured for these methods.
  • Password reset was moved to its own separate method so it can be configured independently.
  • Options are optional as some methods don't have any options by default. All syntax variants (simple array values, true and an empty [] options array) are automatically normalized.
  • The default was moved to the method options to make the configuration less scattered.

Open questions:

  • Maybe the default option can be omitted completely. The first configured method would then automatically be the default. Would this reduce flexibility when defining the methods or would it be fine?
  • There would still need to be a bit of logic magic behind this that checks which modes are compatible (e.g. no password-reset or code when password with 2fa = true was defined; no code, when password-reset was defined). I don't think there is a way around that if we want to protect users from configuring incompatible or even insecure combinations.

@bastianallgeier
Copy link
Member

I feel like we are getting somewhere here! I like the methods configuration and especially the option to keep it super short.

I wonder though if we should take password-reset out of it. It feels weird to include that as a login method. I know that it's technically working in the same way, but from a user perspective it's a different type of feature.

@lukasbestle
Copy link
Member Author

I think it makes sense if you think of the login methods as "ways to get into the Panel":

If you have your password, you can get into the Panel by entering it.
If you don't, you can get into the Panel by resetting it.

In that context I think it could be more confusing to split the configuration up and define the password reset separately – you wouldn't be able to see at one glance which variants are active. Splitting up the config would also make the implementation more complex.

So my vote is to keep it like this.

What do you think about the two open questions above?

@bastianallgeier
Copy link
Member

Ha, I think that sentence really just made it click for me: think of the login methods as "ways to get into the Panel"

I'm totally fine with this as long as we communicate it that way in the docs later.

Open questions:

  1. I like the idea that the first one is the default one. That way we also don't have to check for duplicate "default: true" options
  2. I agree that we should add checks to avoid invalid fixes. Maybe we can have some nice error handling in the login screen in that case?

@lukasbestle
Copy link
Member Author

@bastianallgeier What do you mean with nice error handling in the login screen?

@bastianallgeier
Copy link
Member

@lukasbestle In case you misconfigure the combination of login methods, we could show that in the login screen in debug mode. As a default behaviour we would probably just remove the login methods that should not be allowed in combination, right? But in debug mode we could explain it like "you cannot combine this and that for security reasons"

@lukasbestle
Copy link
Member Author

Ah, I see. Yes, by default we would remove the conflicting options.

We could throw an Exception if in debug mode and then display that Exception in the frontend.

@lukasbestle lukasbestle changed the title Passwordless login & password reset via email Passwordless login, password reset via email & 2FA support Nov 14, 2020
@lukasbestle
Copy link
Member Author

lukasbestle commented Nov 14, 2020

The discussed changes + further improvements are now implemented:

  • Renamed email templates to emails/auth/login and emails/auth/password-reset
  • New configuration syntax (mode string option → methods array option), which brings:
    • Support for disabling login with password by just configuring code-based methods
    • Support for custom default forms by reordering the configured methods
  • Refactored backend logic that is even more secure and more consistent with the other Auth methods
  • The default email sender address now respects the url option

You can find the full list of changes compared to the original version of this PR in this diff.

I have also decided to already implement the 2FA feature as the foundation of the code challenge feature is already pretty much reviewed and final. You can find the implementation in a separate, fifth commit for easier review.

The first post of this PR is updated with the latest information.

@bastianallgeier
Copy link
Member

Sorry, I probably merged the other PR too soon. Could you fix the conflict or should I do it? Otherwise, I'm super happy with the changes! <3

@lukasbestle
Copy link
Member Author

Will do so tonight. :)

@lukasbestle
Copy link
Member Author

@bastianallgeier Should be ready for merging now. :)

@bastianallgeier bastianallgeier merged commit 200a81f into features Nov 17, 2020
@bastianallgeier bastianallgeier deleted the feature/passwordless branch November 17, 2020 08:38
@lukasbestle lukasbestle mentioned this pull request Dec 8, 2020
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature 🎉 Adds a feature (requests → feedback.getkirby.com)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants