From d47f9933d2fb7317ce8155bf4211dc10327f4de7 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Mon, 13 May 2024 13:39:51 -0700 Subject: [PATCH 1/4] add check for government email addresses --- app/main/views/register.py | 20 +++++++++++++++++ app/main/views/sign_in.py | 31 ++++++++++++++++++++++++++- tests/app/main/views/test_register.py | 20 +++++++++++++++++ 3 files changed, 70 insertions(+), 1 deletion(-) diff --git a/app/main/views/register.py b/app/main/views/register.py index 4638eaeab..1910c5c5b 100644 --- a/app/main/views/register.py +++ b/app/main/views/register.py @@ -146,6 +146,26 @@ def check_invited_user_email_address_matches_expected( debug_msg("invited user email did not match expected email, abort(403)") flash("You cannot accept an invite for another person.") abort(403) + check_for_gov_email_address(user_email) + + +def check_for_gov_email_address(user_email): + # We could try to check that it is a government email at the time the invite is + # sent, but due to the way login.gov allows multiple emails, it would not be effective. + # We track the login.gov user by their uuid, so if they have a login.gov account + # with a .gov email address and a .com email address, the .com address will work without + # having a check here. + if ( + user_email.lower().endswith(".gov") + or user_email.lower().endswith(".mil") + or user_email.lower().endswith(".si.edu") + ): + # everything is good, proceed + pass + else: + debug_msg("invited user has a non-government email address.") + flash("You must use a government email address.") + abort(403) @main.route("/set-up-your-profile", methods=["GET", "POST"]) diff --git a/app/main/views/sign_in.py b/app/main/views/sign_in.py index 618a35654..23a9cefd4 100644 --- a/app/main/views/sign_in.py +++ b/app/main/views/sign_in.py @@ -4,7 +4,16 @@ import uuid import jwt import requests -from flask import Response, current_app, redirect, render_template, request, url_for +from flask import ( + Response, + abort, + current_app, + flash, + redirect, + render_template, + request, + url_for, +) from flask_login import current_user from notifications_utils.url_safe_token import generate_token @@ -88,6 +97,7 @@ def _do_login_dot_gov(): try: access_token = _get_access_token(code, state) user_email, user_uuid = _get_user_email_and_uuid(access_token) + check_for_gov_email_address(user_email) redirect_url = request.args.get("next") user = user_api_client.get_user_by_uuid_or_email(user_uuid, user_email) @@ -171,3 +181,22 @@ def sign_in(): @login_manager.unauthorized_handler def sign_in_again(): return redirect(url_for("main.sign_in", next=request.path)) + + +def check_for_gov_email_address(user_email): + # We could try to check that it is a government email at the time the invite is + # sent, but due to the way login.gov allows multiple emails, it would not be effective. + # We track the login.gov user by their uuid, so if they have a login.gov account + # with a .gov email address and a .com email address, the .com address will work without + # having a check here. + if ( + user_email.lower().endswith(".gov") + or user_email.lower().endswith(".mil") + or user_email.lower().endswith(".si.edu") + ): + # everything is good, proceed + pass + else: + current_app.logger.warning("invited user has a non-government email address.") + flash("You must use a government email address.") + abort(403) diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index 688ab1623..194ef000d 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -401,6 +401,26 @@ def test_check_invited_user_email_address_doesnt_match_expected(mocker): mock_abort.assert_called_once_with(403) +def test_check_user_email_address_fails_if_not_government_address(mocker): + mock_flash = mocker.patch("app.main.views.register.flash") + mock_abort = mocker.patch("app.main.views.register.abort") + + check_invited_user_email_address_matches_expected( + "fake@fake.bogus", "Fake@Fake.BOGUS" + ) + mock_flash.assert_called_once_with("You must use a government email address.") + mock_abort.assert_called_once_with(403) + + +def test_check_user_email_address_succeeds_if_government_address(mocker): + mock_flash = mocker.patch("app.main.views.register.flash") + mock_abort = mocker.patch("app.main.views.register.abort") + + check_invited_user_email_address_matches_expected("fake@fake.mil", "Fake@Fake.MIL") + mock_flash.assert_not_called() + mock_abort.assert_not_called() + + def decode_invite_data(state): state = state.encode("utf8") state = base64.b64decode(state) From c79092114166f0e903ecfa83ad63c3db257bcab8 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl Date: Mon, 20 May 2024 11:24:39 -0700 Subject: [PATCH 2/4] Update app/main/views/sign_in.py Co-authored-by: Carlo Costino --- app/main/views/sign_in.py | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/app/main/views/sign_in.py b/app/main/views/sign_in.py index 23a9cefd4..35f0e5251 100644 --- a/app/main/views/sign_in.py +++ b/app/main/views/sign_in.py @@ -181,22 +181,3 @@ def sign_in(): @login_manager.unauthorized_handler def sign_in_again(): return redirect(url_for("main.sign_in", next=request.path)) - - -def check_for_gov_email_address(user_email): - # We could try to check that it is a government email at the time the invite is - # sent, but due to the way login.gov allows multiple emails, it would not be effective. - # We track the login.gov user by their uuid, so if they have a login.gov account - # with a .gov email address and a .com email address, the .com address will work without - # having a check here. - if ( - user_email.lower().endswith(".gov") - or user_email.lower().endswith(".mil") - or user_email.lower().endswith(".si.edu") - ): - # everything is good, proceed - pass - else: - current_app.logger.warning("invited user has a non-government email address.") - flash("You must use a government email address.") - abort(403) From 9ba5e3b917585d67ab8b6c14ae6ab33e1deee154 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Mon, 20 May 2024 12:09:49 -0700 Subject: [PATCH 3/4] code review feedback --- app/__init__.py | 2 +- app/config.py | 4 ++-- app/main/views/register.py | 18 ++---------------- app/main/views/send.py | 2 +- app/main/views/sign_in.py | 27 +++++++-------------------- app/main/views/templates.py | 2 +- app/notify_client/__init__.py | 2 +- app/utils/user.py | 3 +-- tests/app/main/views/test_register.py | 2 +- 9 files changed, 17 insertions(+), 45 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index d2c61d0a0..f76bd4f87 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -23,7 +23,6 @@ from flask_wtf import CSRFProtect from flask_wtf.csrf import CSRFError from itsdangerous import BadSignature from notifications_python_client.errors import HTTPError -from notifications_utils import logging, request_helper from notifications_utils.formatters import ( formatted_list, get_lines_with_normalised_whitespace, @@ -114,6 +113,7 @@ from app.notify_client.upload_api_client import upload_api_client from app.notify_client.user_api_client import user_api_client from app.url_converters import SimpleDateTypeConverter, TemplateTypeConverter from app.utils.govuk_frontend_jinja.flask_ext import init_govuk_frontend +from notifications_utils import logging, request_helper login_manager = LoginManager() csrf = CSRFProtect() diff --git a/app/config.py b/app/config.py index 0a05908f5..f0be4d0fe 100644 --- a/app/config.py +++ b/app/config.py @@ -2,9 +2,9 @@ import json from os import getenv import newrelic.agent -from notifications_utils import DAILY_MESSAGE_LIMIT from app.cloudfoundry_config import cloud_config +from notifications_utils import DAILY_MESSAGE_LIMIT class Config(object): @@ -38,7 +38,7 @@ class Config(object): NR_MONITOR_ON = settings and settings.monitor_mode COMMIT_HASH = getenv("COMMIT_HASH", "--------")[0:7] - GOVERNMENT_EMAIL_DOMAIN_NAMES = ["gov"] + GOVERNMENT_EMAIL_DOMAIN_NAMES = ["gov", "mil", "is.edu"] # Logging NOTIFY_LOG_LEVEL = getenv("NOTIFY_LOG_LEVEL", "INFO") diff --git a/app/main/views/register.py b/app/main/views/register.py index 1910c5c5b..56472ed07 100644 --- a/app/main/views/register.py +++ b/app/main/views/register.py @@ -26,6 +26,7 @@ from app.main.views import sign_in from app.main.views.verify import activate_user from app.models.user import InvitedOrgUser, InvitedUser, User from app.utils import hide_from_search_engines, hilite +from app.utils.user import is_gov_user @main.route("/register", methods=["GET", "POST"]) @@ -146,23 +147,8 @@ def check_invited_user_email_address_matches_expected( debug_msg("invited user email did not match expected email, abort(403)") flash("You cannot accept an invite for another person.") abort(403) - check_for_gov_email_address(user_email) - -def check_for_gov_email_address(user_email): - # We could try to check that it is a government email at the time the invite is - # sent, but due to the way login.gov allows multiple emails, it would not be effective. - # We track the login.gov user by their uuid, so if they have a login.gov account - # with a .gov email address and a .com email address, the .com address will work without - # having a check here. - if ( - user_email.lower().endswith(".gov") - or user_email.lower().endswith(".mil") - or user_email.lower().endswith(".si.edu") - ): - # everything is good, proceed - pass - else: + if not is_gov_user(user_email): debug_msg("invited user has a non-government email address.") flash("You must use a government email address.") abort(403) diff --git a/app/main/views/send.py b/app/main/views/send.py index a0ef0b905..daa7e8115 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -7,7 +7,6 @@ from flask import abort, flash, redirect, render_template, request, session, url from flask_login import current_user from markupsafe import Markup from notifications_python_client.errors import HTTPError -from notifications_utils import SMS_CHAR_COUNT_LIMIT from notifications_utils.insensitive_dict import InsensitiveDict from notifications_utils.recipients import RecipientCSV, first_column_headings from notifications_utils.sanitise_text import SanitiseASCII @@ -39,6 +38,7 @@ from app.utils import PermanentRedirect, should_skip_template_page, unicode_trun from app.utils.csv import Spreadsheet, get_errors_for_csv from app.utils.templates import get_template from app.utils.user import user_has_permissions +from notifications_utils import SMS_CHAR_COUNT_LIMIT def get_example_csv_fields(column_headers, use_example_as_example, submitted_fields): diff --git a/app/main/views/sign_in.py b/app/main/views/sign_in.py index 23a9cefd4..bcf434d91 100644 --- a/app/main/views/sign_in.py +++ b/app/main/views/sign_in.py @@ -25,6 +25,7 @@ from app.models.user import User from app.utils import hide_from_search_engines from app.utils.login import is_safe_redirect_url from app.utils.time import is_less_than_days_ago +from app.utils.user import is_gov_user def _reformat_keystring(orig): @@ -97,7 +98,12 @@ def _do_login_dot_gov(): try: access_token = _get_access_token(code, state) user_email, user_uuid = _get_user_email_and_uuid(access_token) - check_for_gov_email_address(user_email) + if not is_gov_user(user_email): + current_app.logger.error( + "invited user has a non-government email address." + ) + flash("You must use a government email address.") + abort(403) redirect_url = request.args.get("next") user = user_api_client.get_user_by_uuid_or_email(user_uuid, user_email) @@ -181,22 +187,3 @@ def sign_in(): @login_manager.unauthorized_handler def sign_in_again(): return redirect(url_for("main.sign_in", next=request.path)) - - -def check_for_gov_email_address(user_email): - # We could try to check that it is a government email at the time the invite is - # sent, but due to the way login.gov allows multiple emails, it would not be effective. - # We track the login.gov user by their uuid, so if they have a login.gov account - # with a .gov email address and a .com email address, the .com address will work without - # having a check here. - if ( - user_email.lower().endswith(".gov") - or user_email.lower().endswith(".mil") - or user_email.lower().endswith(".si.edu") - ): - # everything is good, proceed - pass - else: - current_app.logger.warning("invited user has a non-government email address.") - flash("You must use a government email address.") - abort(403) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index e9e5f5b61..5c59e1e7c 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -4,7 +4,6 @@ from flask import abort, flash, jsonify, redirect, render_template, request, url from flask_login import current_user from markupsafe import Markup from notifications_python_client.errors import HTTPError -from notifications_utils import SMS_CHAR_COUNT_LIMIT from app import ( current_service, @@ -30,6 +29,7 @@ from app.models.template_list import TemplateList, TemplateLists from app.utils import NOTIFICATION_TYPES, should_skip_template_page from app.utils.templates import get_template from app.utils.user import user_has_permissions +from notifications_utils import SMS_CHAR_COUNT_LIMIT form_objects = { "email": EmailTemplateForm, diff --git a/app/notify_client/__init__.py b/app/notify_client/__init__.py index 1fc14f811..2cad0b68a 100644 --- a/app/notify_client/__init__.py +++ b/app/notify_client/__init__.py @@ -2,9 +2,9 @@ from flask import abort, has_request_context, request from flask_login import current_user from notifications_python_client import __version__ from notifications_python_client.base import BaseAPIClient -from notifications_utils.clients.redis import RequestCache from app.extensions import redis_client +from notifications_utils.clients.redis import RequestCache cache = RequestCache(redis_client) diff --git a/app/utils/user.py b/app/utils/user.py index 668fcb646..a40de8558 100644 --- a/app/utils/user.py +++ b/app/utils/user.py @@ -4,7 +4,6 @@ from flask import abort, current_app from flask_login import current_user, login_required from app import config -from app.notify_client.organizations_api_client import organizations_client user_is_logged_in = login_required @@ -51,7 +50,7 @@ def user_is_platform_admin(f): def is_gov_user(email_address): return _email_address_ends_with( email_address, config.Config.GOVERNMENT_EMAIL_DOMAIN_NAMES - ) or _email_address_ends_with(email_address, organizations_client.get_domains()) + ) # or _email_address_ends_with(email_address, organizations_client.get_domains()) def _email_address_ends_with(email_address, known_domains): diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index 194ef000d..076d1e876 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -148,7 +148,7 @@ def test_should_return_200_when_email_is_not_gov_uk( "email_address", [ "notfound@example.gsa.gov", - "example@lsquo.net", + "example@lsquo.is.edu", ], ) def test_should_add_user_details_to_session( From 16dc4512dd71333a9706f6c512520b03660cd3e5 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 21 May 2024 07:38:23 -0700 Subject: [PATCH 4/4] fix si.edu --- app/config.py | 2 +- tests/app/main/views/test_register.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/config.py b/app/config.py index f0be4d0fe..77138ca16 100644 --- a/app/config.py +++ b/app/config.py @@ -38,7 +38,7 @@ class Config(object): NR_MONITOR_ON = settings and settings.monitor_mode COMMIT_HASH = getenv("COMMIT_HASH", "--------")[0:7] - GOVERNMENT_EMAIL_DOMAIN_NAMES = ["gov", "mil", "is.edu"] + GOVERNMENT_EMAIL_DOMAIN_NAMES = ["gov", "mil", "si.edu"] # Logging NOTIFY_LOG_LEVEL = getenv("NOTIFY_LOG_LEVEL", "INFO") diff --git a/tests/app/main/views/test_register.py b/tests/app/main/views/test_register.py index 076d1e876..19d8c5a4b 100644 --- a/tests/app/main/views/test_register.py +++ b/tests/app/main/views/test_register.py @@ -148,7 +148,7 @@ def test_should_return_200_when_email_is_not_gov_uk( "email_address", [ "notfound@example.gsa.gov", - "example@lsquo.is.edu", + "example@lsquo.si.edu", ], ) def test_should_add_user_details_to_session(