From c27c46bac656b1da74f28eac1b52dfa5df76e6f2 Mon Sep 17 00:00:00 2001 From: Patrik Dufresne Date: Fri, 21 Oct 2022 12:14:40 -0400 Subject: [PATCH] Send email notification when enabling or disabling MFA --- README.md | 1 + rdiffweb/controller/page_mfa.py | 14 ++-- rdiffweb/controller/page_pref_mfa.py | 2 +- .../controller/tests/test_page_prefs_mfa.py | 70 ++++++++-------- rdiffweb/core/config.py | 2 +- rdiffweb/core/model/_user.py | 79 +++++++------------ rdiffweb/core/model/tests/test_user.py | 9 +-- rdiffweb/core/notification.py | 38 ++++++--- rdiffweb/core/tests/test_notification.py | 3 + rdiffweb/templates/email_mfa.html | 14 ++-- .../templates/email_verification_code.html | 18 +++++ tox.ini | 2 +- 12 files changed, 132 insertions(+), 120 deletions(-) create mode 100644 rdiffweb/templates/email_verification_code.html diff --git a/README.md b/README.md index be717e7b..fa1ba2f3 100644 --- a/README.md +++ b/README.md @@ -138,6 +138,7 @@ This next release focus on two-factor-authentication as a measure to increase se * Enforce better rate limit on login, mfa, password change and API [CVE-2022-3439](https://nvd.nist.gov/vuln/detail/CVE-2022-3439) [CVE-2022-3456](https://nvd.nist.gov/vuln/detail/CVE-2022-3456) * Enforce 'Origin' validation [CVE-2022-3457](https://nvd.nist.gov/vuln/detail/CVE-2022-3457) * Define idle and absolute session timeout with agressive default to protect usage on public computer [CVE-2022-3327](https://nvd.nist.gov/vuln/detail/CVE-2022-3327) +* Send email notification when enabling or disabling MFA [CVE-2022-3363](https://nvd.nist.gov/vuln/detail/CVE-2022-3363) Breaking changes: diff --git a/rdiffweb/controller/page_mfa.py b/rdiffweb/controller/page_mfa.py index 9b6981d2..017ceaaf 100644 --- a/rdiffweb/controller/page_mfa.py +++ b/rdiffweb/controller/page_mfa.py @@ -105,10 +105,10 @@ def send_code(self): "Multi-factor authentication is enabled for your account, but your account does not have a valid email address to send the verification code to. Check your account settings with your administrator." ) ) - else: - code = cherrypy.tools.auth_mfa.generate_code() - body = self.app.templates.compile_template( - "email_mfa.html", **{"header_name": self.app.cfg.header_name, 'user': userobj, 'code': code} - ) - cherrypy.engine.publish('queue_mail', to=userobj.email, subject=_("Your verification code"), message=body) - flash(_("A new verification code has been sent to your email.")) + return + code = cherrypy.tools.auth_mfa.generate_code() + body = self.app.templates.compile_template( + "email_verification_code.html", **{"header_name": self.app.cfg.header_name, 'user': userobj, 'code': code} + ) + cherrypy.engine.publish('queue_mail', to=userobj.email, subject=_("Your verification code"), message=body) + flash(_("A new verification code has been sent to your email.")) diff --git a/rdiffweb/controller/page_pref_mfa.py b/rdiffweb/controller/page_pref_mfa.py index cf4d4295..918bf8d1 100644 --- a/rdiffweb/controller/page_pref_mfa.py +++ b/rdiffweb/controller/page_pref_mfa.py @@ -126,7 +126,7 @@ def send_code(self): return code = cherrypy.tools.auth_mfa.generate_code() body = self.app.templates.compile_template( - "email_mfa.html", **{"header_name": self.app.cfg.header_name, 'user': userobj, 'code': code} + "email_verification_code.html", **{"header_name": self.app.cfg.header_name, 'user': userobj, 'code': code} ) cherrypy.engine.publish('queue_mail', to=userobj.email, subject=_("Your verification code"), message=body) flash(_("A new verification code has been sent to your email.")) diff --git a/rdiffweb/controller/tests/test_page_prefs_mfa.py b/rdiffweb/controller/tests/test_page_prefs_mfa.py index 248e53bd..97e8e532 100644 --- a/rdiffweb/controller/tests/test_page_prefs_mfa.py +++ b/rdiffweb/controller/tests/test_page_prefs_mfa.py @@ -15,7 +15,7 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . -from unittest.mock import MagicMock +from unittest.mock import ANY, MagicMock import cherrypy from parameterized import parameterized @@ -34,47 +34,49 @@ def setUp(self): userobj = UserObject.get_user(self.USERNAME) userobj.email = 'admin@example.com' userobj.add() + # Register a listener on email + self.listener = MagicMock() + cherrypy.engine.subscribe('queue_mail', self.listener.queue_email, priority=50) + + def tearDown(self): + cherrypy.engine.unsubscribe('queue_mail', self.listener.queue_email) + return super().tearDown() def _set_mfa(self, mfa): # Define mfa for user userobj = UserObject.get_user(self.USERNAME) userobj.mfa = mfa userobj.add() + # Reset mock. + self.listener.queue_email.reset_mock() + # Leave to disable mfa if mfa == UserObject.DISABLED_MFA: return # Generate a code for login if required - self.listener = MagicMock() - cherrypy.engine.subscribe('queue_mail', self.listener.queue_email, priority=50) - try: - self.getPage("/mfa/") - self.assertStatus(200) - self.assertInBody("A new verification code has been sent to your email.") - # Extract code from email between and - self.listener.queue_email.assert_called_once() - message = self.listener.queue_email.call_args[1]['message'] - code = message.split('', 1)[1].split('')[0] - # Login to MFA - self.getPage("/mfa/", method='POST', body={'code': code, 'submit': '1'}) - self.assertStatus(303) - finally: - cherrypy.engine.unsubscribe('queue_mail', self.listener.queue_email) + self.getPage("/mfa/") + self.assertStatus(200) + self.assertInBody("A new verification code has been sent to your email.") + # Extract code from email between and + self.listener.queue_email.assert_called_once() + message = self.listener.queue_email.call_args[1]['message'] + code = message.split('', 1)[1].split('')[0] + # Login to MFA + self.getPage("/mfa/", method='POST', body={'code': code, 'submit': '1'}) + self.assertStatus(303) + # Clear mock. + self.listener.queue_email.reset_mock() def _get_code(self, action): assert action in ['enable_mfa', 'disable_mfa', 'resend_code'] - # Register an email listeer to capture email send - self.listener = MagicMock() - cherrypy.engine.subscribe('queue_mail', self.listener.queue_email, priority=50) # Query MFA page to generate a code - try: - self.getPage("/prefs/mfa", method='POST', body={action: '1'}) - self.assertStatus(200) - self.assertInBody("A new verification code has been sent to your email.") - # Extract code from email between and - self.listener.queue_email.assert_called_once() - message = self.listener.queue_email.call_args[1]['message'] - return message.split('', 1)[1].split('')[0] - finally: - cherrypy.engine.unsubscribe('queue_mail', self.listener.queue_email) + self.getPage("/prefs/mfa", method='POST', body={action: '1'}) + self.assertStatus(200) + self.assertInBody("A new verification code has been sent to your email.") + # Extract code from email between and + self.listener.queue_email.assert_called_once() + message = self.listener.queue_email.call_args[1]['message'] + self.listener.queue_email.reset_mock() + return message.split('', 1)[1].split('')[0] def test_get(self): # When getting the page @@ -84,11 +86,11 @@ def test_get(self): @parameterized.expand( [ - ('enable_mfa', UserObject.DISABLED_MFA, UserObject.ENABLED_MFA), - ('disable_mfa', UserObject.ENABLED_MFA, UserObject.DISABLED_MFA), + ('enable_mfa', UserObject.DISABLED_MFA, UserObject.ENABLED_MFA, "Two-Factor Authentication turned on"), + ('disable_mfa', UserObject.ENABLED_MFA, UserObject.DISABLED_MFA, "Two-Factor Authentication turned off"), ] ) - def test_with_valid_code(self, action, initial_mfa, expected_mfa): + def test_with_valid_code(self, action, initial_mfa, expected_mfa, expected_subject): # Define mfa for user self._set_mfa(initial_mfa) # Given a user with email requesting a code @@ -99,8 +101,10 @@ def test_with_valid_code(self, action, initial_mfa, expected_mfa): self.assertStatus(200) userobj = UserObject.get_user(self.USERNAME) self.assertEqual(userobj.mfa, expected_mfa) - # Then no email get sent + # Then no verification code get sent self.assertNotInBody("A new verification code has been sent to your email.") + # Then an email confirmation get send + self.listener.queue_email.assert_called_once_with(to=ANY, subject=expected_subject, message=ANY) # Then next page request is still working. self.getPage('/') self.assertStatus(200) diff --git a/rdiffweb/core/config.py b/rdiffweb/core/config.py index b560ab86..6b2cbc7e 100644 --- a/rdiffweb/core/config.py +++ b/rdiffweb/core/config.py @@ -159,7 +159,7 @@ def get_parser(): '--emailsendchangednotification', help='True to send notification when sensitive information get change in user profile.', action='store_true', - default=False, + default=True, ) parser.add_argument( diff --git a/rdiffweb/core/model/_user.py b/rdiffweb/core/model/_user.py index d2bbcd09..d3dc7da6 100644 --- a/rdiffweb/core/model/_user.py +++ b/rdiffweb/core/model/_user.py @@ -24,7 +24,7 @@ from sqlalchemy import Column, Integer, SmallInteger, String, and_, event, inspect, or_ from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.hybrid import hybrid_property -from sqlalchemy.orm import deferred, relationship +from sqlalchemy.orm import deferred, relationship, validates from zxcvbn import zxcvbn import rdiffweb.tools.db # noqa @@ -74,9 +74,9 @@ class UserObject(Base): PATTERN_USERNAME = r"[a-zA-Z0-9_.\-]+$" userid = Column('UserID', Integer, primary_key=True) - _username = Column('Username', String, nullable=False, unique=True) + username = Column('Username', String, nullable=False, unique=True) hash_password = Column('Password', String, nullable=False, default="") - _user_root = Column('UserRoot', String, nullable=False, default="") + user_root = Column('UserRoot', String, nullable=False, default="") _is_admin = deferred( Column( 'IsAdmin', @@ -86,7 +86,7 @@ class UserObject(Base): doc="DEPRECATED This column is replaced by 'role'", ) ) - _email = Column('UserEmail', String, nullable=False, default="") + email = Column('UserEmail', String, nullable=False, default="") restore_format = deferred( Column( 'RestoreFormat', @@ -96,7 +96,7 @@ class UserObject(Base): doc="DEPRECATED This column is not used anymore", ) ) - _role = Column('role', SmallInteger, nullable=False, server_default=str(USER_ROLE)) + role = Column('role', SmallInteger, nullable=False, server_default=str(USER_ROLE), default=USER_ROLE) fullname = Column('fullname', String, nullable=False, default="") mfa = Column('mfa', SmallInteger, nullable=False, default=DISABLED_MFA) repo_objs = relationship( @@ -129,7 +129,7 @@ def create_admin_user(cls, default_username, default_password): userobj.add() @classmethod - def add_user(cls, username, password=None, **attrs): + def add_user(cls, username, password=None, role=USER_ROLE, **attrs): """ Used to add a new user with an optional password. """ @@ -143,6 +143,7 @@ def add_user(cls, username, password=None, **attrs): userobj = UserObject( username=username, hash_password=hash_password(password) if password else '', + role=role, **attrs, ).add() # Raise event @@ -383,51 +384,11 @@ def set_password(self, password): def __eq__(self, other): return type(self) == type(other) and inspect(self).key == inspect(other).key - @hybrid_property - def username(self): - return self._username - - @username.setter - def username(self, value): - oldvalue = self._username - self._username = value - if oldvalue != value: - cherrypy.engine.publish('user_attr_changed', self, {'username': (oldvalue, value)}) - - @hybrid_property - def role(self): - if self._role is None: - return self.USER_ROLE - return self._role - - @role.setter - def role(self, value): - oldvalue = self._role - self._role = value - if oldvalue != value: - cherrypy.engine.publish('user_attr_changed', self, {'role': (oldvalue, value)}) - - @hybrid_property - def email(self): - return self._email - - @email.setter - def email(self, value): - oldvalue = self._email - self._email = value - if oldvalue != value: - cherrypy.engine.publish('user_attr_changed', self, {'email': (oldvalue, value)}) - - @hybrid_property - def user_root(self): - return self._user_root - - @user_root.setter - def user_root(self, value): - oldvalue = self._user_root - self._user_root = value - if oldvalue != value: - cherrypy.engine.publish('user_attr_changed', self, {'user_root': (oldvalue, value)}) + @validates('username') + def validates_username(self, key, value): + if self.username: + raise ValueError('Username cannot be modified.') + return value def validate_access_token(self, token): """ @@ -460,3 +421,19 @@ def user_after_delete(mapper, connection, target): Publish event when user is deleted. """ cherrypy.engine.publish('user_deleted', target.username) + + +@event.listens_for(UserObject, 'after_update') +def user_attr_changed(mapper, connection, target): + changes = {} + state = inspect(target) + for attr in state.attrs: + if attr.key in ['user_root', 'email', 'role', 'mfa']: + hist = attr.load_history() + if hist.has_changes(): + changes[attr.key] = ( + hist.deleted[0] if len(hist.deleted) >= 1 else None, + hist.added[0] if len(hist.added) >= 1 else None, + ) + if changes: + cherrypy.engine.publish('user_attr_changed', target, changes) diff --git a/rdiffweb/core/model/tests/test_user.py b/rdiffweb/core/model/tests/test_user.py index b5258958..6d95889b 100644 --- a/rdiffweb/core/model/tests/test_user.py +++ b/rdiffweb/core/model/tests/test_user.py @@ -36,11 +36,6 @@ class UserObjectTest(rdiffweb.test.WebCase): - - default_config = { - 'email-send-changed-notification': True, - } - def _read_ssh_key(self): """Readthe pub key from test packages""" filename = pkg_resources.resource_filename('rdiffweb.core.tests', 'test_publickey_ssh_rsa.pub') @@ -174,12 +169,16 @@ def test_get_set(self): user.refresh_repos() self.listener.user_attr_changed.assert_called_with(user, {'user_root': ('', self.testcases)}) self.listener.user_attr_changed.reset_mock() + user = UserObject.get_user('larry') user.role = UserObject.ADMIN_ROLE + user.add() self.listener.user_attr_changed.assert_called_with( user, {'role': (UserObject.USER_ROLE, UserObject.ADMIN_ROLE)} ) self.listener.user_attr_changed.reset_mock() + user = UserObject.get_user('larry') user.email = 'larry@gmail.com' + user.add() self.listener.user_attr_changed.assert_called_with(user, {'email': ('', 'larry@gmail.com')}) self.listener.user_attr_changed.reset_mock() diff --git a/rdiffweb/core/notification.py b/rdiffweb/core/notification.py index c22b20e4..f64103a9 100644 --- a/rdiffweb/core/notification.py +++ b/rdiffweb/core/notification.py @@ -78,19 +78,31 @@ def user_attr_changed(self, userobj, attrs={}): return # Leave if the mail was not changed. - if 'email' not in attrs: - return - - old_email = attrs['email'][0] - if not old_email: - logger.info("can't sent mail to user [%s] without an email", userobj.username) - return - - # If the email attributes was changed, send a mail notification. - body = self.app.templates.compile_template( - "email_changed.html", **{"header_name": self.app.cfg.header_name, 'user': userobj} - ) - self.bus.publish('queue_mail', to=old_email, subject=_("Email address changed"), message=body) + if 'email' in attrs: + old_email = attrs['email'][0] + if not old_email: + logger.info("can't sent mail to user [%s] without an email", userobj.username) + return + # If the email attributes was changed, send a mail notification. + subject = _("Email address changed") + body = self.app.templates.compile_template( + "email_changed.html", **{"header_name": self.app.cfg.header_name, 'user': userobj} + ) + self.bus.publish('queue_mail', to=old_email, subject=str(subject), message=body) + + if 'mfa' in attrs: + if not userobj.email: + logger.info("can't sent mail to user [%s] without an email", userobj.username) + return + subject = ( + _("Two-Factor Authentication turned off") + if userobj.mfa == UserObject.DISABLED_MFA + else _("Two-Factor Authentication turned on") + ) + body = self.app.templates.compile_template( + "email_mfa.html", **{"header_name": self.app.cfg.header_name, 'user': userobj} + ) + self.bus.publish('queue_mail', to=userobj.email, subject=str(subject), message=body) def user_password_changed(self, userobj): if not self.send_changed: diff --git a/rdiffweb/core/tests/test_notification.py b/rdiffweb/core/tests/test_notification.py index a4ecfd0c..27b54b1c 100644 --- a/rdiffweb/core/tests/test_notification.py +++ b/rdiffweb/core/tests/test_notification.py @@ -118,10 +118,13 @@ def test_email_changed(self): # Given a user with an email address user = UserObject.get_user(self.USERNAME) user.email = 'original_email@test.com' + user.add() self.listener.queue_email.reset_mock() # When updating the user's email + user = UserObject.get_user(self.USERNAME) user.email = 'email_changed@test.com' + user.add() # Then a email is queue to notify the user. self.listener.queue_email.assert_called_once_with( diff --git a/rdiffweb/templates/email_mfa.html b/rdiffweb/templates/email_mfa.html index da796644..cbdff642 100644 --- a/rdiffweb/templates/email_mfa.html +++ b/rdiffweb/templates/email_mfa.html @@ -3,16 +3,14 @@ {% trans username=(user.fullname or user.username) %}Hey {{ username }},{% endtrans %}

- {% trans %}To help us make sure it's really you, here's the verification code you'll need to log in:{% endtrans %} + {% if user.mfa %} + {% trans %}Your {{ header_name }} Account is now protected with Two-Factor Authentication. When you sign in on a new or untrusted device, you'll need your second factor to verify your identity.{% endtrans %} + {% else %} + {% trans %}Your {{ header_name }} account is no longer protected with Two-Factor Authentication. You don't need your second factor to sign in.{% endtrans %} + {% endif %}

- {{ code }} -

-

- {% trans %}If this wasn't you logging in, and you use a password to log in, please reset your password.{% endtrans %} -

-

- {% trans %}This code will expire in 1 hour. Once the code expires, you will need to request a new verification code by going through the login procedure again.{% endtrans %} + {% trans %}You received this email to let you know about important changes to your Google Account and services.{% endtrans %}

diff --git a/rdiffweb/templates/email_verification_code.html b/rdiffweb/templates/email_verification_code.html new file mode 100644 index 00000000..da796644 --- /dev/null +++ b/rdiffweb/templates/email_verification_code.html @@ -0,0 +1,18 @@ + + + + {% trans username=(user.fullname or user.username) %}Hey {{ username }},{% endtrans %} +

+ {% trans %}To help us make sure it's really you, here's the verification code you'll need to log in:{% endtrans %} +

+

+ {{ code }} +

+

+ {% trans %}If this wasn't you logging in, and you use a password to log in, please reset your password.{% endtrans %} +

+

+ {% trans %}This code will expire in 1 hour. Once the code expires, you will need to request a new verification code by going through the login procedure again.{% endtrans %} +

+ + diff --git a/tox.ini b/tox.ini index 7a2ae196..c64608a3 100644 --- a/tox.ini +++ b/tox.ini @@ -77,7 +77,7 @@ commands = black --check --diff setup.py rdiffweb skip_install = true [testenv:djlint] -deps = djlint==1.12.1 +deps = djlint==1.19.2 allowlist_externals = sh commands = sh -c 'djlint --check rdiffweb/templates/*.html rdiffweb/templates/**/*.html' skip_install = true