From 5c0db639ebbe51b1185d4e8be5d66ed4d3e0b6fb Mon Sep 17 00:00:00 2001 From: Karsten Hopp Date: Aug 27 2018 14:25:57 +0000 Subject: [PATCH 1/3] add configure option to restrict email to certain domains fixes #3550 add a config option ALLOWED_EMAIL_DOMAINS which can be used to limit allowed email addresses to a list of domains. Signed-off-by: Karsten Hopp --- diff --git a/files/pagure.cfg.sample b/files/pagure.cfg.sample index 2898966..8bfee4d 100644 --- a/files/pagure.cfg.sample +++ b/files/pagure.cfg.sample @@ -49,6 +49,12 @@ FROM_EMAIL = 'pagure@localhost.localdomain' DOMAIN_EMAIL_NOTIFICATIONS = 'localhost.localdomain' SALT_EMAIL = '' +### Restrict outgoing emails to these domains: +## If set, adding emailaccounts that don't end with these domainnames +## will not be permitted. Mails to already existing emailaccounts +## that are not covered by this list will not get sent. +# ALLOWED_EMAIL_DOMAINS = [ 'localhost.localdomain', 'example.com' ] + ### The URL at which the project is available. APP_URL = 'http://localhost.localdomain/' ### The URL at which the documentation of projects will be available diff --git a/pagure/lib/__init__.py b/pagure/lib/__init__.py index c6c9b24..0debe4a 100644 --- a/pagure/lib/__init__.py +++ b/pagure/lib/__init__.py @@ -3541,7 +3541,10 @@ def set_up_user( emails = set() emails.add(default_email) for email in emails: - add_email_to_user(session, user, email) + try: + add_email_to_user(session, user, email) + except pagure.exceptions.PagureException as err: + _log.exception(err) if ssh_key and not user.public_ssh_key: update_user_ssh(session, user, ssh_key, keydir) @@ -3549,8 +3552,26 @@ def set_up_user( return user +def allowed_emailaddress(email): + ''' check if email domains are restricted and if a given email address + is allowed. ''' + allowed_email_domains = pagure_config.get('ALLOWED_EMAIL_DOMAINS', None) + if allowed_email_domains: + for domain in allowed_email_domains: + if email.endswith(domain): + return + raise pagure.exceptions.PagureException( + 'The email address ' + email + ' ' + + 'is not in the list of allowed email domains:\n' + + "\n".join(allowed_email_domains)) + + def add_email_to_user(session, user, user_email): """ Add the provided email to the specified user. """ + try: + allowed_emailaddress(user_email) + except pagure.exceptions.PagureException: + raise emails = [email.email for email in user.emails] if user_email not in emails: useremail = model.UserEmail(user_id=user.id, email=user_email) @@ -3741,8 +3762,12 @@ def update_blocked_issue(session, repo, issue, blocks, username, ticketfolder): def add_user_pending_email(session, userobj, email): - """ Add the provided user to the specified user. + """ Add the provided email to the specified user. """ + try: + allowed_emailaddress(email) + except pagure.exceptions.PagureException: + raise other_user = search_user(session, email=email) if other_user and other_user != userobj: raise pagure.exceptions.PagureException( diff --git a/pagure/lib/notify.py b/pagure/lib/notify.py index cec176f..7504203 100644 --- a/pagure/lib/notify.py +++ b/pagure/lib/notify.py @@ -323,6 +323,10 @@ def send_email( smtp = None for mailto in to_mail.split(","): + try: + pagure.lib.allowed_emailaddress(mailto) + except pagure.exceptions.PagureException: + continue msg = MIMEText(text.encode("utf-8"), "plain", "utf-8") msg["Subject"] = Header("[%s] %s" % (subject_tag, subject), "utf-8") msg["From"] = from_email diff --git a/pagure/ui/app.py b/pagure/ui/app.py index 8daf0b0..c4f3425 100644 --- a/pagure/ui/app.py +++ b/pagure/ui/app.py @@ -1331,6 +1331,9 @@ def confirm_email(token): flask.g.session.delete(email) flask.g.session.commit() flask.flash("Email validated") + except pagure.exceptions.PagureException as err: + flask.flash(str(err), "error") + _log.exception(err) except SQLAlchemyError as err: # pragma: no cover flask.g.session.rollback() flask.flash( diff --git a/pagure/ui/login.py b/pagure/ui/login.py index e015ccc..283de31 100644 --- a/pagure/ui/login.py +++ b/pagure/ui/login.py @@ -61,17 +61,19 @@ def new_user(): flask.g.session.add(user) flask.g.session.flush() - pagure.lib.add_email_to_user( - flask.g.session, user, form.email_address.data - ) - try: + pagure.lib.add_email_to_user( + flask.g.session, user, form.email_address.data + ) flask.g.session.commit() send_confirmation_email(user) flask.flash( "User created, please check your email to activate the " "account" ) + except pagure.exceptions.PagureException as err: + flask.flash(str(err), "error") + _log.exception(err) except SQLAlchemyError: # pragma: no cover flask.g.session.rollback() flask.flash("Could not create user.") diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py index 1b4af4b..450ba31 100644 --- a/tests/test_pagure_lib.py +++ b/tests/test_pagure_lib.py @@ -2120,6 +2120,8 @@ class PagureLibtests(tests.Modeltests): statuses = pagure.lib.get_issue_statuses(self.session) self.assertEqual(sorted(statuses), ['Closed', 'Open']) + @patch.dict(pagure.config.config, { + 'ALLOWED_EMAIL_DOMAINS': ["fp.o"]}, clear=True) def test_set_up_user(self): """ Test the set_up_user of pagure.lib. """ @@ -2185,6 +2187,22 @@ class PagureLibtests(tests.Modeltests): self.assertEqual( sorted(['skvidal@fp.o', 'svidal@fp.o']), sorted([email.email for email in items[2].emails])) + # add again with forbidden email domain + pagure.lib.set_up_user( + session=self.session, + username='skvidal', + fullname='Seth', + default_email='svidal@example.o', + keydir=pagure.config.config.get('GITOLITE_KEYDIR', None), + ) + self.session.commit() + # Email not added + items = pagure.lib.search_user(self.session) + self.assertEqual(3, len(items)) + self.assertEqual('skvidal', items[2].user) + self.assertEqual( + sorted(['skvidal@fp.o', 'svidal@fp.o']), + sorted([email.email for email in items[2].emails])) def test_update_user_ssh(self): """ Test the update_user_ssh of pagure.lib. """ @@ -5495,6 +5513,8 @@ foo bar ) self.assertEqual(msg, 'Successfully edited your settings') + @patch.dict(pagure.config.config, { + 'ALLOWED_EMAIL_DOMAINS': ["pingoured.fr"]}, clear=True) def test_add_email_to_user_with_logs(self): """ Test the add_email_to_user function of pagure.lib when there are log entries associated to the email added. @@ -5523,6 +5543,18 @@ foo bar # Check emails after self.assertEqual(len(user.emails), 3) + # add another email address that is not in an allowed domain + self.assertRaises( + pagure.exceptions.PagureException, + pagure.lib.add_email_to_user, + session=self.session, + user=user, + user_email='foo@bar.com' + ) + self.session.commit() + # Check emails after + self.assertEqual(len(user.emails), 3) + @patch('pagure.lib.is_valid_ssh_key', MagicMock(return_value='foo bar')) def test_update_user_ssh_valid_key(self): """ Test the update_user_ssh function of pagure.lib. """ @@ -5557,6 +5589,8 @@ foo bar ) @patch('pagure.lib.notify.notify_new_email', MagicMock(return_value=True)) + @patch.dict(pagure.config.config, { + 'ALLOWED_EMAIL_DOMAINS': ["pingoured.fr"]}, clear=True) def test_add_user_pending_email(self): """ Test the add_user_pending_email function of pagure.lib. """ user = pagure.lib.search_user(self.session, username='pingou') @@ -5573,6 +5607,18 @@ foo bar self.assertEqual(len(user.emails), 2) self.assertEqual(len(user.emails_pending), 1) + # add another email address that is not allowed + self.assertRaises( + pagure.exceptions.PagureException, + pagure.lib.add_user_pending_email, + session=self.session, + userobj=user, + email='foo@bar.com' + ) + self.session.commit() + + self.assertEqual(len(user.emails), 2) + self.assertEqual(len(user.emails_pending), 1) def test_resend_pending_email_someone_else_email(self): """ Test the resend_pending_email function of pagure.lib. """ From d944cf70cdda9906b8c8112cb345808ebac2825b Mon Sep 17 00:00:00 2001 From: Karsten Hopp Date: Aug 27 2018 14:25:57 +0000 Subject: [PATCH 2/3] test with 2 allowed email domain names Signed-off-by: Karsten Hopp --- diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py index 450ba31..624ed62 100644 --- a/tests/test_pagure_lib.py +++ b/tests/test_pagure_lib.py @@ -2196,7 +2196,7 @@ class PagureLibtests(tests.Modeltests): keydir=pagure.config.config.get('GITOLITE_KEYDIR', None), ) self.session.commit() - # Email not added + # Email should not be added items = pagure.lib.search_user(self.session) self.assertEqual(3, len(items)) self.assertEqual('skvidal', items[2].user) @@ -2204,6 +2204,26 @@ class PagureLibtests(tests.Modeltests): sorted(['skvidal@fp.o', 'svidal@fp.o']), sorted([email.email for email in items[2].emails])) + # redo tests with 2 allowed domains + pagure.config.config['ALLOWED_EMAIL_DOMAINS'] = ["fp.o", "example.com"] + # add again with forbidden email domain + pagure.lib.set_up_user( + session=self.session, + username='skvidal', + fullname='Seth', + default_email='svidal@example.o', + keydir=pagure.config.config.get('GITOLITE_KEYDIR', None), + ) + self.session.commit() + # Email should not be added + items = pagure.lib.search_user(self.session) + self.assertEqual(3, len(items)) + self.assertEqual('skvidal', items[2].user) + self.assertEqual( + sorted(['skvidal@fp.o', 'svidal@fp.o']), + sorted([email.email for email in items[2].emails])) + + def test_update_user_ssh(self): """ Test the update_user_ssh of pagure.lib. """ @@ -5555,6 +5575,28 @@ foo bar # Check emails after self.assertEqual(len(user.emails), 3) + # redo tests with 2 allowed domains + pagure.config.config['ALLOWED_EMAIL_DOMAINS'] = ["pingoured.fr", "example.com"] + # Add the new_email to the user + pagure.lib.add_email_to_user( + self.session, user, 'another_email@example.com' + ) + self.session.commit() + # Check emails after + self.assertEqual(len(user.emails), 4) + + # add another email address that is not in an allowed domain + self.assertRaises( + pagure.exceptions.PagureException, + pagure.lib.add_email_to_user, + session=self.session, + user=user, + user_email='another_foo@bar.com' + ) + self.session.commit() + # Check emails after + self.assertEqual(len(user.emails), 4) + @patch('pagure.lib.is_valid_ssh_key', MagicMock(return_value='foo bar')) def test_update_user_ssh_valid_key(self): """ Test the update_user_ssh function of pagure.lib. """ From 66e2584d9dc624b56c478ece4a57b4d316c81036 Mon Sep 17 00:00:00 2001 From: Karsten Hopp Date: Aug 27 2018 14:48:04 +0000 Subject: [PATCH 3/3] use @patch to correctly reset config options after the tests Signed-off-by: Karsten Hopp --- diff --git a/tests/test_pagure_lib.py b/tests/test_pagure_lib.py index 624ed62..312dd9f 100644 --- a/tests/test_pagure_lib.py +++ b/tests/test_pagure_lib.py @@ -2204,8 +2204,45 @@ class PagureLibtests(tests.Modeltests): sorted(['skvidal@fp.o', 'svidal@fp.o']), sorted([email.email for email in items[2].emails])) - # redo tests with 2 allowed domains - pagure.config.config['ALLOWED_EMAIL_DOMAINS'] = ["fp.o", "example.com"] + @patch.dict(pagure.config.config, { + 'ALLOWED_EMAIL_DOMAINS': ["fp.o", "example.c"]}, clear=True) + def test_set_up_user_multiple_emaildomains(self): + """ Test the set_up_user of pagure.lib when there are + multimple domains configured in ALLOWED_EMAIL_DOMAINS""" + + items = pagure.lib.search_user(self.session) + self.assertEqual(2, len(items)) + self.assertEqual(2, items[0].id) + self.assertEqual('foo', items[0].user) + self.assertEqual(1, items[1].id) + self.assertEqual('pingou', items[1].user) + + pagure.lib.set_up_user( + session=self.session, + username='skvidal', + fullname='Seth', + default_email='skvidal@fp.o', + keydir=pagure.config.config.get('GITOLITE_KEYDIR', None), + ssh_key='foo key', + ) + self.session.commit() + + # Add the user a second time with a different email + pagure.lib.set_up_user( + session=self.session, + username='skvidal', + fullname='Seth', + default_email='skvidal@example.c', + keydir=pagure.config.config.get('GITOLITE_KEYDIR', None), + ) + self.session.commit() + # Email added + items = pagure.lib.search_user(self.session) + self.assertEqual(3, len(items)) + self.assertEqual('skvidal', items[2].user) + self.assertEqual( + sorted(['skvidal@fp.o', 'skvidal@example.c']), + sorted([email.email for email in items[2].emails])) # add again with forbidden email domain pagure.lib.set_up_user( session=self.session, @@ -2220,10 +2257,9 @@ class PagureLibtests(tests.Modeltests): self.assertEqual(3, len(items)) self.assertEqual('skvidal', items[2].user) self.assertEqual( - sorted(['skvidal@fp.o', 'svidal@fp.o']), + sorted(['skvidal@fp.o', 'skvidal@example.c']), sorted([email.email for email in items[2].emails])) - def test_update_user_ssh(self): """ Test the update_user_ssh of pagure.lib. """