code review feedback

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()

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")

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)

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):

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)

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,

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)
 

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):

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(