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

[NEW] Two Factor authentication via email #15949

Merged
merged 74 commits into from
Mar 19, 2020
Merged

[NEW] Two Factor authentication via email #15949

merged 74 commits into from
Mar 19, 2020

Conversation

rodrigok
Copy link
Member

@rodrigok rodrigok commented Dec 10, 2019

Documentation:

https://github.com/RocketChat/docs/pull/1554

Depends on

#15979

Todo

  • Generate code and send it via email
  • Save the code hashed (bcrypt) in the user's record
  • Intercept login as we do for the current 2FA and send the email or validate the code
  • Expire codes after 1 hour
  • Limit saved tokens to 5
  • Check email 2FA only for users with verified emails
  • Setting to define the expiration time?
  • Setting to enable/disable the Email 2FA system-wide
  • Migration to disable Email 2FA for existant installations
  • Add the 2FA setting to the wizard
  • Profile setting to enable/disable Email 2FA
  • Move code to specific files to be reused
  • Improve the 2FA modal to reflect the new source of code
  • Reuse codes
  • Method and Button to send a new code
  • Wrapper to process 2FA on UI transparently
  • Require 2FA for REST
  • Check for plain text email Plain Text Only Option for Email Notifications feature-requests#187
  • Finish translations
  • Rest endpoints to enable/disable personal setting for 2FA
  • Use the same name for REST and Methods to require 2FA
  • Return object in error details
    • Return available methods
    • Return if the token exists already and lifetime
    • Return if the token was generated and sent
  • Generate new token even if there are tokens with less than X minutes
  • Accept method to check from UI
  • Fallback to password verification when no 2FA method available
  • Remeber 2FA for x seconds (5min default)
  • Setting to define if accounts from external services will have the email verified

extras

  • Two Factor bypass option for personal access tokens
    • Require 2fa to create/delete personal access tokens
    • Disable 2fa when the option is enabled, and the REST request was made with that token
    • Disable 2fa when the option is enabled, and the DDP request was made with that token

@reetp
Copy link

reetp commented Dec 11, 2019

Email 2FA system-wide (enabled by default for new installations)

Please, don't. Not by default.

Does this mean the admin has to use 2FA before they can even login??

You have the related issue here:
#15880

Systems where email is either not used, or is broken on setup etc etc.

And in any event, isn't the goal of Rocket.Chat to REPLACE email entirely?? This is completely at odds with that policy (I have no idea on the solution, but this isn't it)

@rodrigok
Copy link
Member Author

@reetp thanks for the feedback, lets make some points clear.

  1. It will be enabled only for new installations.
  2. It will be possible to disable it on the wizard.
  3. Only users with verified emails will have the 2FA required.
  4. Admins will not need to enter the 2FA on the first login or setup because the emails will not be verified.
  5. Users without email will not trigger the 2FA.
  6. Users will be able to disable his Email 2FA on his profile (if it's not required by the admin).
  7. We are using the email as 2FA to make our product secure easily.

@rodrigok rodrigok changed the base branch from develop to enable-ts December 14, 2019 14:03
@reetp
Copy link

reetp commented Dec 16, 2019

@reetp thanks for the feedback, lets make some points clear.

1. It will be enabled only for new installations.

Isn't that the administrators choice, not yours? By all means add an option, but please stop making these things compulsory and let admins decide how they want to manage their servers

7. We are using the email as 2FA to make our product secure easily.

Except you make a hard dependency on email for your 'security' when your clear direction was to replace email.

Take a look at your own home page on your website. I am clearly missing something here.

Rocket.Chat is free, unlimited and open source. Replace email, HipChat & Slack with the ultimate team chat software solution.

Am I blind? Or maybe you need to change your website?

Screenshot_2019-12-16_02-05-24

It is totally illogical. Forcing these sorts of things don't make Rocket any more secure because it will just get turned off by those who don't want it. Or are you going to remove that choice as well next?

That then goes along with the issues in #15880 where an admin cannot set password that he wants.

You are making email completely essential when you are selling a product that is supposed to make email redundant.

@reetp
Copy link

reetp commented Dec 16, 2019

Ahh - and this needs implementing too because there are plenty of users who block ALL html mail and your current emails insist on being html only.

RocketChat/feature-requests#187

So no text mails means no mentions, no password resets, no 2FA.....

@rodrigok
Copy link
Member Author

rodrigok commented Dec 16, 2019

@reetp let me explain it again.

Isn't that the administrator's choice, not yours? By all means add an option, but please stop making these things compulsory and let admins decide how they want to manage their servers

It's an admin choice! There will be an option to disable it on the admin and even on the wizard as you can see in my last comment and the issue description!

Except you make a hard dependency on email for your 'security' when your clear direction was to replace email.

We don't want to kill email but replace it in the communication area, it's important for other reasons, and one of those reasons is security, you use it to reset your password, to receive notifications of profile changes, etc. If you enable 2fa via google authenticator you will not need to use email for that. If you don't want the 2fa you can disable it, it's up to you and your administrator. I hope it makes clear our relationship with the future of communication and emails.

It is totally illogical. Forcing these sorts of things don't make Rocket any more secure because it will just get turned off by those who don't want it. Or are you going to remove that choice as well next?

We always provide choices for the users, it's really hard to find cases where we don't, be enabled by default (for new installations) doesn't mean it will be enforced, just suggested as good practice. And yes, it will help people to make their installations more secure, if they don't want that feature it's their choice to actively disable it.

That then goes along with the issues in #15880 where an admin cannot set password that he wants.

We are solving this in another way, be able to set the plain password can be a good thing for a few ones but a huge security problem for the majority. Since we value our security we focus on the majority without leaving the minority alone, we will provide a way to create personal access tokens for bots soon.

You are making email completely essential when you are selling a product that is supposed to make email redundant.

We are not! Again, you can disable it, use another 2FA solution or just do not verify yours email. We will have other additions on top of this one, like 2FA via push notifications, via SMS, etc. This is the basis for the future.

Ahh - and this needs implementing too because there are plenty of users who block ALL html mail and your current emails insist on being html only.

This is a good feedback, thanks, lets fix this, I didn't even know it was an issue.

Thanks.

@rodrigok rodrigok changed the base branch from enable-ts to develop December 16, 2019 17:22
# Conflicts:
#	.eslintrc
#	package-lock.json
#	package.json
@engelgabriel engelgabriel modified the milestones: 3.0.0, 3.1.0 Feb 19, 2020
# Conflicts:
#	app/api/server/api.js
#	client/components/setupWizard/steps/SettingsBasedStep.js
#	package-lock.json
#	server/startup/migrations/index.js
#	server/startup/migrations/v172.js
Copy link
Member

@sampaiodiego sampaiodiego left a comment

Choose a reason for hiding this comment

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

changing something on profile page requires a password or a 2fa code to be provided.. if the wrong password or 2fa code is provided, the following error is shown:
image

providing the wrong password when trying to disable two factor by email on My account > security shows "undefined" as error:
https://recordit.co/EF5s0GzdSLa

packages/rocketchat-i18n/i18n/pt-BR.i18n.json Outdated Show resolved Hide resolved
client/components/setupWizard/steps/SettingsBasedStep.js Outdated Show resolved Hide resolved
app/api/server/v1/users.js Show resolved Hide resolved
@sampaiodiego
Copy link
Member

created a PR to our docs regarding the new param of the generate token endpoint https://github.com/RocketChat/docs/pull/1649

sampaiodiego
sampaiodiego previously approved these changes Mar 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants