Skip to content

Conversation

@kucherenko
Copy link

What does it do?

New feature added to strapi-plugin-users-permissions

Why is it needed?

One of the secure and easiest way to login is a link send to email

@strapi-cla
Copy link

strapi-cla commented Apr 2, 2021

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Apr 2, 2021

Codecov Report

Merging #9944 (bb847c3) into master (b97fb68) will decrease coverage by 24.15%.
The diff coverage is n/a.

❗ Current head bb847c3 differs from pull request most recent head cd81741. Consider uploading reports for the commit cd81741 to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##           master    #9944       +/-   ##
===========================================
- Coverage   60.06%   35.90%   -24.16%     
===========================================
  Files         183     1342     +1159     
  Lines        5702    14815     +9113     
  Branches     1077     1478      +401     
===========================================
+ Hits         3425     5320     +1895     
- Misses       1817     8575     +6758     
- Partials      460      920      +460     
Flag Coverage Δ
front 27.27% <ø> (∅)
unit 55.57% <ø> (-4.50%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ions/admin/src/containers/Providers/utils/forms.js 0.00% <ø> (ø)
...t-type-builder/controllers/validation/component.js 62.96% <0.00%> (-29.63%) ⬇️
packages/strapi-admin/services/permission.js 71.64% <0.00%> (-28.36%) ⬇️
packages/strapi-plugin-graphql/services/utils.js 25.00% <0.00%> (-22.62%) ⬇️
...ges/strapi-plugin-graphql/services/data-loaders.js 28.00% <0.00%> (-13.18%) ⬇️
packages/strapi-utils/lib/policy.js 75.71% <0.00%> (-4.29%) ⬇️
...i-database/lib/queries/relations-counts-queries.js 26.08% <0.00%> (-3.08%) ⬇️
packages/strapi-admin/controllers/role.js 47.16% <0.00%> (-2.84%) ⬇️
...ntent-type-builder/controllers/validation/types.js 52.27% <0.00%> (-2.28%) ⬇️
...plugin-content-manager/controllers/single-types.js 76.54% <0.00%> (-1.62%) ⬇️
... and 1285 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b97fb68...cd81741. Read the comment docs.

@SorinGFS
Copy link
Contributor

SorinGFS commented Apr 3, 2021

One of the secure and easiest way to login is a link send to email

Without even checking the user's password?

@kucherenko
Copy link
Author

One of the secure and easiest way to login is a link send to email

Without even checking the user's password?

Yes, if user has access to his mailbox he can login, otherwise - no access. On our side we don't store password in this case at all

@SorinGFS
Copy link
Contributor

SorinGFS commented Apr 4, 2021

Yes, if user has access to his mailbox he can login, otherwise - no access. On our side we don't store password in this case at all

Well, this is a major security breach:

  1. I can make a script that will trigger your server to send 1 million emails in one hour, and in less than 24 hours your server will be blacklisted everywhere
  2. Accessing email links leaves traces in many places (like logs). Did you know that the login to Srapi is valid for a month?

@kucherenko
Copy link
Author

Yes, if user has access to his mailbox he can login, otherwise - no access. On our side we don't store password in this case at all

Well, this is a major security breach:

  1. I can make a script that will trigger your server to send 1 million emails in one hour, and in less than 24 hours your server will be blacklisted everywhere
  2. Accessing email links leaves traces in many places (like logs). Did you know that the login to Srapi is valid for a month?
  1. The same thing you can do with classic flow with email confirmation.
  2. loginToken for email login valid one time, that is why we can don't care about links in log

I'm not inventor of the login method, more information you can find at different places like https://fusionauth.io/docs/v1/tech/guides/passwordless/ or in Auth0 documentation

@SorinGFS
Copy link
Contributor

SorinGFS commented Apr 4, 2021

1. The same thing you can do with classic flow with email confirmation.
2. `loginToken` for email login valid one time, that is why we can don't care about links in log
  1. If with classic flow with email confirmation can do the same thing that would be another problem, not a justification to do the same mistake. Anyway, I think the email confirmation has a rate limit protection.
  2. Did you tried to use the login link the second time? Maybe I'm wrong, I ask because I didn't see the loginToken invalidation part in your code,...
  3. Passwordless or magic link authentication in fact are not... paswordless! You have to login into email account using... a password! So, IMHO this looks more like a second factor authentication, in which case I would imagine a plugin sending emails or text messages with a token to be entered in a box in order to complete the login process.
  4. Auth0 is not an authority in cybersecurity, is just a collection of codes gathered from here and there and improved by trial and error in some cases even at the expense of those who used them. It is enough to study their history of security bugs to realize that I am right. You know the saying: Your cybersecurity is as strong as its weakest link. Well, to that I add trust no one and use your own brain! If someone would hack my app I would rather prefer to be my fault, not the fault of an email provider I haven't even heard of...

Anyway, I'm just a regular Strapi user, their team may have a different opinion.

@SorinGFS
Copy link
Contributor

SorinGFS commented Apr 5, 2021

Even more:

By far the most dangerous security flow introduced by this PR would be the imposibility to use CSRF Protection. When user clicks a link inside it's own email account the access is automatically cross origin. CSRF Protection does not neccesarily block cross-origin access, but it ensures that allows access only to requests having a valid CSRF token sent in regular HTTP headers. Well, since there is impossible to get and set a CSRF token inside an email account the only way arround this would be to instruct the user to copy the login link and to paste it in the same window from where the login request was initiated. More abut login CSRF.
Easy enough to guess: no one will do this, especially on mobile!

@kucherenko
Copy link
Author

kucherenko commented Apr 5, 2021

1. The same thing you can do with classic flow with email confirmation.
2. `loginToken` for email login valid one time, that is why we can don't care about links in log
  1. If with classic flow with email confirmation can do the same thing that would be another problem, not a justification to do the same mistake. Anyway, I think the email confirmation has a rate limit protection.
  2. Did you tried to use the login link the second time? Maybe I'm wrong, I ask because I didn't see the loginToken invalidation part in your code,...
  3. Passwordless or magic link authentication in fact are not... paswordless! You have to login into email account using... a password! So, IMHO this looks more like a second factor authentication, in which case I would imagine a plugin sending emails or text messages with a token to be entered in a box in order to complete the login process.
  4. Auth0 is not an authority in cybersecurity, is just a collection of codes gathered from here and there and improved by trial and error in some cases even at the expense of those who used them. It is enough to study their history of security bugs to realize that I am right. You know the saying: Your cybersecurity is as strong as its weakest link. Well, to that I add trust no one and use your own brain! If someone would hack my app I would rather prefer to be my fault, not the fault of an email provider I haven't even heard of...

Anyway, I'm just a regular Strapi user, their team may have a different opinion.

  1. Rate limit protection should be introduced to the system on global level , if we are going to follow SOLID base on single responsibility we should not implemented it on service level. (as I know strapi developers already implement the rate limit protection)
  2. https://github.com/strapi/strapi/pull/9944/files#diff-81ba2718e479b40f49b3620f34da4d78df2889e56cbc07dc6bb2e83c69bcad71R616 - here is line with token invalidation
  3. Passwordless is common practice, and yes you are right in actual it is not really passwordless, sometimes in is called One-time password - it is mean that your password every time changed.
  4. Auth0 is one of the services implemented the passwordless, you can find a tons of examples base on other services and products including google, aws, microsoft and other:

https://firebase.google.com/docs/auth/web/email-link-auth
https://aws.amazon.com/blogs/mobile/implementing-passwordless-email-authentication-with-amazon-cognito/
https://www.microsoft.com/en-us/security/business/identity-access-management/passwordless-authentication
https://www.okta.com/passwordless-authentication/
https://medium.com/profusion-engineering/keycloak-passwordless-authentication-550ca095c0bf

As summary I would say that the PR is the first step to create more secure application with strapi, base on the suggested changes we have ability to move to two-factor authentication and integration with services like Google Authenticator.

@SorinGFS
Copy link
Contributor

SorinGFS commented Apr 5, 2021

1. Rate limit protection should be introduced to the system on global level

No, I dissagree, Rate limit protection should not be introduced to the system on global level, Rate Limit comes with a high cost and the use should not be abused, it should only be used on routes with sensitive data (e.g. /admin/*).

In fact, I believe that Strapi should consider a scenario where access is managed entirely by another downstream application. This means that the Rate Limit should be optional, because if it is applied in a cascade, the delay time will be multiplied unnecessarily.

@kucherenko
Copy link
Author

1. Rate limit protection should be introduced to the system on global level

No, I dissagree, Rate limit protection should not be introduced to the system on global level, Rate Limit comes with a high cost and the use should not be abused, it should only be used on routes with sensitive data (e.g. /admin/*).

https://forum.strapi.io/t/rate-limiting-for-routes/953

Based on the answer you can see that rate limit protection introduced on plugin level (not global, you are right).

@kucherenko
Copy link
Author

In fact, I believe that Strapi should consider a scenario where access is managed entirely by another downstream application. This means that the Rate Limit should be optional, because if it is applied in a cascade, the delay time will be multiplied unnecessarily.

plugins::users-permissions.ratelimit is configurable parameter in strapi, you can disable it if you want

@SorinGFS
Copy link
Contributor

SorinGFS commented Apr 5, 2021

plugins::users-permissions.ratelimit is configurable parameter in strapi, you can disable it if you want

I want to disable in admin panel, I avoid to cusomize Strapi in any way for now because Strapi is like quicksand, changing fast and I dont want to set to myself a concern to follow the changes in order to update my customization. So, for me is simple: if I can't see it in admin panel.... it doesn't exist!

@kucherenko
Copy link
Author

plugins::users-permissions.ratelimit is configurable parameter in strapi, you can disable it if you want

I want to disable in admin panel, I avoid to cusomize Strapi in any way for now because Strapi is like quicksand, changing fast and I dont want to set to myself a concern to follow the changes in order to update my customization. So, for me is simple: if I can't see it in admin panel.... it doesn't exist!

I think you can create an issue or PR with adding rate limit to the admin panel. It is part of users-permissions plugin.

@kucherenko kucherenko changed the title Add ability to login with link send to email Add ability to login with link send to email (passwordless login) Apr 5, 2021
@SorinGFS
Copy link
Contributor

SorinGFS commented Apr 5, 2021

I think you can create an issue or PR with adding rate limit to the admin panel. It is part of users-permissions plugin.

I don't think Strapi and I share the same vision about modular design, fallback system, and so on. I've tried before, that's enough. Strapi for a reason I don't understand prefers that some things can't be done from the Administration Panel, at least that's how I see it.

@MostafaYehia
Copy link

@kucherenko kucherenko
I have read your PR and the whole discussion, Good one and I think it will be applicable for some use cases especially after apply the DKIM , so I think it should be configurable from the Admin panel

@derrickmehaffy
Copy link
Member

@SorinGFS

I don't think Strapi and I share the same vision about modular design, fallback system, and so on. I've tried before, that's enough. Strapi for a reason I don't understand prefers that some things can't be done from the Administration Panel, at least that's how I see it.

Anything saved in files (minus model structures, IE the content-type-builder) cannot be configured in the admin panel due to the need for node to restart which should not happen in production. Most configuration options are done via files to follow standard source control and automation (see ansible) type deployments.

Anything that is configurable in the admin panel is saved to the database.

@derrickmehaffy
Copy link
Member

On that note, I'm going to lock this PR from further comments until the team can review it, it's unlikely we would merge something like this due the plans we have in mind but for further discussion I suggest you move this over to our forum: https://forum.strapi.io

@strapi strapi locked as off-topic and limited conversation to collaborators May 10, 2021
@alexandrebodin alexandrebodin added flag: don't merge This PR should not be merged at the moment issue: feature request Issue suggesting a new feature labels May 11, 2021
@derrickmehaffy
Copy link
Member

This pull request has been mentioned on Strapi Community Forum. There might be relevant details there:

https://forum.strapi.io/t/passwordless-login-for-strapi/4893/1

@derrickmehaffy
Copy link
Member

Hi 👋
We now have released the v4. As of now, the v3 is in maintenance mode for 6 months. We will only be fixing critical and security issues on v3 from now on.
I will go ahead and close this PR. Thank you for contributing to Strapi 🔥

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

flag: don't merge This PR should not be merged at the moment issue: feature request Issue suggesting a new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants