From afda2dbac78c7055c23d2c91ea83d6c05639951b Mon Sep 17 00:00:00 2001 From: Cyrus <22678716+cyrusdobbs@users.noreply.github.com> Date: Thu, 30 Nov 2023 11:11:28 +0000 Subject: [PATCH 1/3] [BAU] improve authorisation implementation by encapsulating details in classes and abstracting out towns fund specifics --- app/__init__.py | 11 +- app/const.py | 3 +- app/main/authorisation.py | 139 ++++++++++++++++--------- app/main/routes.py | 32 +++++- tests/test_authorisation.py | 195 ++++++++++++++++++++++-------------- 5 files changed, 246 insertions(+), 134 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 3cde89d..61d1cc0 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -9,7 +9,8 @@ from jinja2 import ChoiceLoader, PackageLoader, PrefixLoader import static_assets -from app.const import EMAIL_DOMAIN_TO_LA_AND_PLACE_NAMES_AND_FUND_TYPES +from app.const import TOWNS_FUND_AUTH +from app.main.authorisation import AuthMapping, build_auth_mapping from config import Config assets = Environment() @@ -43,9 +44,11 @@ def create_app(config_class=Config): health = Healthcheck(app) health.add_check(FlaskRunningChecker()) - # instantiate email to LA and place and fund types mapping used for authorizing submissions - app.config["EMAIL_TO_LA_AND_PLACE_NAMES_AND_FUND_TYPES"] = copy(EMAIL_DOMAIN_TO_LA_AND_PLACE_NAMES_AND_FUND_TYPES) - app.config["EMAIL_TO_LA_AND_PLACE_NAMES_AND_FUND_TYPES"].update(app.config.get("ADDITIONAL_EMAIL_LOOKUPS", {})) + # TODO: TOWNS_FUND_AUTH is currently stored in const.py but this isn't isn't a good solution. + # We need to decide where we should store and inject specific auth mappings from. + email_mapping = copy(TOWNS_FUND_AUTH) + email_mapping.update(Config.ADDITIONAL_EMAIL_LOOKUPS) + app.config["AUTH_MAPPING"]: AuthMapping = build_auth_mapping(Config.FUND_NAME, email_mapping) logging.init_app(app) return app diff --git a/app/const.py b/app/const.py index 2850ed7..7c5db38 100644 --- a/app/const.py +++ b/app/const.py @@ -16,7 +16,8 @@ class MIMETYPE(StrEnum): "HS": "Future High Streets Fund", } -EMAIL_DOMAIN_TO_LA_AND_PLACE_NAMES_AND_FUND_TYPES = { +# domain/email: (LAs, Places, Funds) +TOWNS_FUND_AUTH = { "ambervalley.gov.uk": (("Amber Valley Borough Council",), ("Heanor",), ("Town_Deal", "Future_High_Street_Fund")), "ashfield.gov.uk": ( ("Ashfield District Council",), diff --git a/app/main/authorisation.py b/app/main/authorisation.py index 161478b..2ba46fa 100644 --- a/app/main/authorisation.py +++ b/app/main/authorisation.py @@ -1,57 +1,98 @@ -from flask import abort, current_app, g +from abc import ABC, abstractmethod -def check_authorised() -> tuple[tuple[str], dict[str]]: - """Checks that the user is authorized to submit. +class AuthBase(ABC): + """Auth class ABC. Classes that inherit must implement a constructor, organisations and auth_dict methods.""" - Returns any LAs, places, and fund types that the user is authorized to submit for, otherwise aborts and redirects - to 401 (unauthorised) page. + @abstractmethod + def __init__(self, *args): + pass - :return: the LAs as a tuple, and a dictionary with both the place_names and fund_types - """ - local_authorities, place_names, fund_types = get_local_authority_and_place_names_and_fund_types(g.user.email) - if local_authorities is None or place_names is None or fund_types is None: - current_app.logger.error( - f"User: {g.user.email} has not been assigned any local authorities and/or places and/or fund types" - ) - abort(401) # unauthorized - current_app.logger.info( - f"User: {g.user.email} from {', '.join(local_authorities)} is authorised for places: {', '.join(place_names)}" - f"and fund types: {', '.join(fund_types)}" - ) - return local_authorities, {"Place Names": place_names, "Fund Types": fund_types} - - -def get_local_authority_and_place_names_and_fund_types( - user_email: str, -) -> tuple[tuple[str] | None, tuple[str] | None, tuple[str] | None]: + @abstractmethod + def get_organisations(self) -> tuple[str, ...]: + """Return organisations associated with this level of authorisation.""" + pass + + @abstractmethod + def get_auth_dict(self) -> dict: + """Return other details associated with this authorisation.""" + pass + + +class TFAuth(AuthBase): + """A Towns Fund Auth Class""" + + local_authorities: tuple[str, ...] + place_names: tuple[str, ...] + fund_types: tuple[str, ...] + + def __init__(self, local_authorities: tuple[str, ...], place_names: tuple[str, ...], fund_types: tuple[str, ...]): + self.local_authorities = local_authorities + self.place_names = place_names + self.fund_types = fund_types + + def get_organisations(self) -> tuple[str, ...]: + return self.local_authorities + + def get_auth_dict(self) -> dict: + return {"Place Names": self.place_names, "Fund Types": self.fund_types} + + +class AuthMapping: + """Encapsulates an email mapping dictionary. Allows lookup of an email address.""" + + _auth_class: type[AuthBase] + _mapping: dict[str, AuthBase] + + def __init__(self, auth_class: type[AuthBase], mapping: dict[str, tuple[tuple[str, ...], ...]]): + """Instantiates an AuthMapping from an Auth class and a set of dictionary mappings. + + :param auth_class: the Auth class implementation that this AuthMapping will store + :param mapping: a dictionary mapping emails to a set of auth details that are held within Auth objects + """ + self._auth_class = auth_class + # for each item in the dictionary, encapsulate the auth details values in an instance of the auth_class + self._mapping = {email: auth_class(*auth_details) for email, auth_details in mapping.items()} + + def get_auth(self, email: str) -> AuthBase | None: + """Get the authorisation information associated with the given email address. + + This lookup is case-insensitive. + + Lookup hierarchy: + 1. Full Email + 2. Email Domain + + :param email: email address + :return: the associated Auth + """ + domain = email.split("@")[1] + # first match on full email, then try domain + auth = self._mapping.get(email.lower()) or self._mapping.get(domain.lower()) + return auth + + +def _auth_class_factory(fund: str) -> type[AuthBase]: + """Given a fund, returns the associated auth class. + + :param fund: Fund Name + :return: associated Auth class + :raises ValueError: """ - Get the local authority, place names, and fund types corresponding to a user's email. + match fund: + case "Towns Fund": + return TFAuth + case _: + raise ValueError("Unknown Fund") + - This function takes a user's email address and uses the domain part (after '@') - to look up the corresponding place names and fund types the user can submit returns for. - If the domain is not present in the look-up, the user may be a private contractor - who cannot be verified by the domain alone, and so a look-up of the entire - e-mail address is performed. Where this is not found, a tuple containing None - will be returned. +def build_auth_mapping(fund_name: str, mapping: dict[str, tuple[tuple[str, ...], ...]]) -> AuthMapping: + """Given a fund and a set of email mappings, return an auth mapping object. - :param user_email: A string representing the user's email address. - :return: A tuple of local authorities, place names, and fund types under their remit. + :param fund_name: the fund associated with this mapping + :param mapping: a mapping of email/domains -> (organisation, *other_auth_details) + :return: an AuthMapping """ - email_mapping = current_app.config["EMAIL_TO_LA_AND_PLACE_NAMES_AND_FUND_TYPES"] - email_domain = user_email.split("@")[1] - # if the domain is not present in the lookup, we will check with the whole e-mail - la_and_place_names_and_fund_types = email_mapping.get(email_domain.lower()) or email_mapping.get( - user_email.lower(), (None, None, None) - ) - - # TODO: remove this once successfully deployed with updated secret - if len(la_and_place_names_and_fund_types) == 3: - return la_and_place_names_and_fund_types - else: - current_app.logger.warning("Secret auth mapping is invalid - adding TD and FHSF and continuing") - return ( - la_and_place_names_and_fund_types[0], - la_and_place_names_and_fund_types[1], - ("Town_Deal", "Future_High_Street_Fund"), - ) + auth_class: type[AuthBase] = _auth_class_factory(fund_name) + auth_mapping = AuthMapping(auth_class, mapping) + return auth_mapping diff --git a/app/main/routes.py b/app/main/routes.py index 6be9b3b..97fc806 100644 --- a/app/main/routes.py +++ b/app/main/routes.py @@ -1,6 +1,6 @@ import json -from flask import current_app, g, redirect, render_template, request, url_for +from flask import abort, current_app, g, redirect, render_template, request, url_for from fsd_utils.authentication.config import SupportedApp from fsd_utils.authentication.decorators import login_requested, login_required from werkzeug.datastructures import FileStorage @@ -14,7 +14,7 @@ VALIDATION_LOG, ) from app.main import bp -from app.main.authorisation import check_authorised +from app.main.authorisation import AuthBase, AuthMapping from app.main.data_requests import post_ingest from app.main.notify import send_confirmation_emails from app.utils import calculate_days_to_deadline, is_load_enabled @@ -38,7 +38,7 @@ def login(): @bp.route("/upload", methods=["GET", "POST"]) @login_required(return_app=SupportedApp.POST_AWARD_SUBMIT, roles_required=[Config.TF_SUBMITTER_ROLE]) def upload(): - local_authorities, auth = check_authorised() + local_authorities, auth_dict = check_authorised() if request.method == "GET": return render_template( @@ -67,7 +67,7 @@ def upload(): success, pre_errors, validation_errors, metadata = post_ingest( excel_file, - {"reporting_round": 4, "auth": json.dumps(auth), "do_load": is_load_enabled()}, + {"reporting_round": 4, "auth": json.dumps(auth_dict), "do_load": is_load_enabled()}, ) if success: @@ -104,6 +104,30 @@ def upload(): ) +def check_authorised() -> tuple[tuple[str, ...], dict[str, tuple[str, ...]]]: + """Checks that the user is authorized to submit. + + Returns any Organisations that the user is authorized to submit for, along with any authorisation details to check + against the submission. + + Otherwise, if they are not authorised for any submissions, aborts and redirects to 401 (unauthorised) page. + + :return: the organisation as a tuple, and a dictionary of authorisation details to check against the submission + """ + auth_mapping: AuthMapping = current_app.config["AUTH_MAPPING"] + auth: AuthBase = auth_mapping.get_auth(g.user.email) + + if auth is None: + current_app.logger.error(f"User: {g.user.email} has not been assigned any authorisation") + abort(401) # unauthorized + + current_app.logger.info( + f"User: {g.user.email} from {', '.join(auth.get_organisations())} is authorised for: {auth.get_auth_dict()}" + ) + + return auth.get_organisations(), auth.get_auth_dict() + + def check_file(excel_file: FileStorage) -> str | None: """Basic checks on an uploaded file. diff --git a/tests/test_authorisation.py b/tests/test_authorisation.py index fc7166b..34dfd42 100644 --- a/tests/test_authorisation.py +++ b/tests/test_authorisation.py @@ -1,82 +1,125 @@ import pytest -from flask import g -from fsd_utils.authentication.models import User -from werkzeug.exceptions import Unauthorized from app.main.authorisation import ( - check_authorised, - get_local_authority_and_place_names_and_fund_types, + AuthBase, + AuthMapping, + TFAuth, + _auth_class_factory, + build_auth_mapping, ) -def test_custom_las_and_place_names(flask_test_client): - """Check that custom domains/emails are being added to the mapping used by authorization.""" - assert ( - "contractor@contractor.com" - in flask_test_client.application.config[("EMAIL_TO_LA_AND_PLACE_NAMES_AND_FUND_TYPES")] - ) - - -def test_check_authorized_success(flask_test_client): - valid_user = User(full_name="Test", roles=[], highest_role_map={}, email="user@wigan.gov.uk") - - with flask_test_client.application.app_context(): - g.user = valid_user - local_authorities, auth = check_authorised() - assert local_authorities - assert auth["Place Names"] == ("Wigan",) - assert auth["Fund Types"] == ("Town_Deal", "Future_High_Street_Fund") - - -def test_check_authorized_failure(flask_test_client): - invalid_user = User(full_name="Test", roles=[], highest_role_map={}, email="unknown_user@unknown.gov.uk") - - with pytest.raises(Unauthorized): - with flask_test_client.application.app_context(): - g.user = invalid_user - check_authorised() - - -def test_get_local_authority_place_names_and_fund_types(flask_test_client): - with flask_test_client.application.app_context(): - # tests mapping the email domain - domain_mapping_1 = get_local_authority_and_place_names_and_fund_types("user@bolton.gov.uk") - domain_mapping_2 = get_local_authority_and_place_names_and_fund_types("user@newcastle-staffs.gov.uk") - domain_mapping_3 = get_local_authority_and_place_names_and_fund_types("user@wigan.gov.uk") - domain_mapping_4 = get_local_authority_and_place_names_and_fund_types("user@cumberland.gov.uk") - # tests mapping the whole email address - email_mapping = get_local_authority_and_place_names_and_fund_types("contractor@contractor.com") - # test mapping of case-insensitive e-mail - case_insensitive_mapping = get_local_authority_and_place_names_and_fund_types("Contractor@contractor.com") - # no mapping exists - no_mapping = get_local_authority_and_place_names_and_fund_types("user@wmadeup.gov.uk") - # only authorised for TD and not HS - only_td_mapping = get_local_authority_and_place_names_and_fund_types("td_only@contractor.com") - - assert domain_mapping_1 == ( - ("Bolton Metropolitan Borough Council",), - ( - "Farnworth", - "Bolton", - ), - ("Town_Deal", "Future_High_Street_Fund"), - ) - assert domain_mapping_2 == ( - ("Newcastle-under-Lyme Borough Council",), - ("Newcastle-Under-Lyme Town Centre", "Kidsgrove", "Newcastle-under-Lyme", "Newcastle-under-Lyme Town Centre"), - ("Town_Deal", "Future_High_Street_Fund"), - ) - assert domain_mapping_3 == (("Wigan Council",), ("Wigan",), ("Town_Deal", "Future_High_Street_Fund")) - assert domain_mapping_4 == ( - ("Cumberland Council",), - ("Workington", "Cleator Moor", "Millom", "Carlisle", "Carlisle City Centre", "Maryport Town Centre"), - ("Town_Deal", "Future_High_Street_Fund"), - ) - assert email_mapping == (("Amber Valley Borough Council",), ("Heanor",), ("Town_Deal", "Future_High_Street_Fund")) - assert case_insensitive_mapping == ( - ("Amber Valley Borough Council",), - ("Heanor",), - ("Town_Deal", "Future_High_Street_Fund"), - ) - assert no_mapping == (None, None, None) - assert only_td_mapping == (("Rotherham Metropolitan Borough Council",), ("Rotherham",), ("Town_Deal",)) +@pytest.fixture +def mock_mapping(): + mapping = { + "wizard1@hogwarts.magic.uk": (("Hogwarts",), ("Professor Albus Dumbledore",)), + "hogwarts.magic.uk": (("Hogwarts",), ("Harry Potter", "Ron Weasley")), + } + return mapping + + +@pytest.fixture +def mock_auth_class(): + class WizardAuth(AuthBase): + def __init__(self, schools, wizards): + self.schools = schools + self.wizards = wizards + + def get_organisations(self) -> tuple[str, ...]: + return self.schools + + def get_auth_dict(self) -> dict: + return {"Wizards": self.wizards} + + return WizardAuth + + +@pytest.fixture +def mock_auth_mapping(mock_auth_class, mock_mapping): + return AuthMapping(mock_auth_class, mock_mapping) + + +def test_auth_mapping_email_match(mock_auth_mapping, mock_auth_class): + """ + GIVEN a valid AuthMapping object + WHEN an email address is passed to it that DOES match an exact email AND a domain + THEN it should return the correct mapping for that exact email + """ + auth = mock_auth_mapping.get_auth("wizard1@hogwarts.magic.uk") + + assert isinstance(auth, mock_auth_class) + assert auth.get_organisations() == ("Hogwarts",) + assert auth.get_auth_dict() == {"Wizards": ("Professor Albus Dumbledore",)} + + +def test_auth_mapping_email_match_case_insensitive(mock_auth_mapping, mock_auth_class): + """ + GIVEN a valid AuthMapping object + WHEN an email address is passed to it that DOES match an email by content but NOT case + THEN it should return the mapping for that email regardless of case + """ + auth = mock_auth_mapping.get_auth("WIZARD1@hogwarts.magic.uk") + + assert isinstance(auth, mock_auth_class) + assert auth.get_organisations() == ("Hogwarts",) + assert auth.get_auth_dict() == {"Wizards": ("Professor Albus Dumbledore",)} + + +def test_auth_mapping_domain_match(mock_auth_mapping, mock_auth_class): + """ + GIVEN a valid AuthMapping object + WHEN an email address is passed to it that DOES NOT match an exact email BUT DOES match a domain + THEN it should return the correct mapping for that domain + """ + auth = mock_auth_mapping.get_auth("anotherwizard@hogwarts.magic.uk") + + assert isinstance(auth, mock_auth_class) + assert auth.get_organisations() == ("Hogwarts",) + assert auth.get_auth_dict() == {"Wizards": ("Harry Potter", "Ron Weasley")} + + +def test_auth_mapping_no_match(mock_auth_mapping, mock_auth_class): + """ + GIVEN a valid AuthMapping object + WHEN an email address is passed to it that DOES NOT match an exact email OR a domain + THEN it should None + """ + auth = mock_auth_mapping.get_auth("wizard@azkaban.magic.uk") + + assert auth is None + + +def test_auth_class_factory_valid_fund(): + """ + GIVEN a valid Fund + WHEN it is passed to _auth_class_factory + THEN it should return the correct Auth class + """ + auth_class = _auth_class_factory("Towns Fund") + assert issubclass(auth_class, AuthBase) + assert auth_class == TFAuth + + +def test_auth_class_factory_unknown_fund(): + """ + GIVEN an unknown Fund + WHEN it is passed to _auth_class_factory + THEN it should raise a ValueError + """ + with pytest.raises(ValueError) as error: + _auth_class_factory("New Fund") + assert error.value.args[0] == "Unknown Fund" + + +def test_build_auth_mapping(mocker, mock_auth_class, mock_mapping): + """ + GIVEN a mapping and a mocked out _auth_class_factory + WHEN I pass the mapping to build_auth_mapping + THEN it should return a valid AuthMapping of that data + """ + mocker.patch("app.main.authorisation._auth_class_factory", return_value=mock_auth_class) + + auth_mapping = build_auth_mapping("Fund", mock_mapping) + + assert isinstance(auth_mapping, AuthMapping) + assert auth_mapping.get_auth(list(mock_mapping.keys())[0]), "Mapping does not map to source data" From 487091bea5c252c91974db1ff8d3d16da65a56f3 Mon Sep 17 00:00:00 2001 From: Cyrus <22678716+cyrusdobbs@users.noreply.github.com> Date: Thu, 30 Nov 2023 15:52:25 +0000 Subject: [PATCH 2/3] [BAU] add missing fund types to mappings --- app/const.py | 84 +++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 60 insertions(+), 24 deletions(-) diff --git a/app/const.py b/app/const.py index 7c5db38..00d7b95 100644 --- a/app/const.py +++ b/app/const.py @@ -119,30 +119,66 @@ class MIMETYPE(StrEnum): ), ("Town_Deal", "Future_High_Street_Fund"), ), - "eastriding.gov.uk": (("East Riding of Yorkshire Council",), ("Goole",)), - "eaststaffsbc.gov.uk": (("East Staffordshire Borough Council",), ("Burton",)), - "erewash.gov.uk": (("Erewash Borough Council",), ("Long Eaton",)), - "fenland.gov.uk": (("Fenland District Council",), ("March High Street",)), - "fylde.gov.uk": (("Fylde Council",), ("Kirkham Town Centre",)), - "great-yarmouth.gov.uk": (("Great Yarmouth Borough Council",), ("Great Yarmouth",)), - "royalgreenwich.gov.uk": (("Royal Borough of Greenwich",), ("Woolwich Town Centre",)), - "halton.gov.uk": (("Halton Borough Council",), ("Runcorn",)), - "haringey.gov.uk": (("London Borough of Haringey",), ("Tottenham High Road",)), - "harlow.gov.uk": (("Harlow District Council",), ("Harlow",)), - "harrow.gov.uk": (("London Borough of Harrow",), ("Wealdstone",)), - "hartlepool.gov.uk": (("Hartlepool Borough Council",), ("Hartlepool",)), - "hastings.gov.uk": (("Hastings Borough Council",), ("Hastings",)), - "highpeak.gov.uk": (("High Peak Borough Council",), ("Buxton",)), - "huntingdonshire.gov.uk": (("Huntingdonshire District Council",), ("St Neots",)), - "ipswich.gov.uk": (("Ipswich Borough Council",), ("Ipswich",)), - "west-norfolk.gov.uk": (("King's Lynn and West Norfolk",), ("King's Lynn",)), - "kirklees.gov.uk": (("Kirklees Council",), ("Dewsbury")), - "leeds.gov.uk": (("Leeds Council",), ("Morley")), - "lewes-eastbourne.gov.uk": (("Lewes District Council",), ("Eastbourne Borough Council", "Newhaven")), - "ad.lewes-eastbourne.gov.uk": (("Lewes District Council",), ("Eastbourne Borough Council", "Newhaven")), - "lincoln.gov.uk": (("City of Lincoln Council",), ("Lincoln",)), - "mansfield.gov.uk": (("Mansfield District Council",), ("Mansfield",)), - "medway.gov.uk": (("Medway Council",), ("Chatham Town Centre",)), + "eastriding.gov.uk": ( + ("East Riding of Yorkshire Council",), + ("Goole",), + ("Town_Deal", "Future_High_Street_Fund"), + ), + "eaststaffsbc.gov.uk": ( + ("East Staffordshire Borough Council",), + ("Burton",), + ("Town_Deal", "Future_High_Street_Fund"), + ), + "erewash.gov.uk": (("Erewash Borough Council",), ("Long Eaton",), ("Town_Deal", "Future_High_Street_Fund")), + "fenland.gov.uk": (("Fenland District Council",), ("March High Street",), ("Town_Deal", "Future_High_Street_Fund")), + "fylde.gov.uk": (("Fylde Council",), ("Kirkham Town Centre",), ("Town_Deal", "Future_High_Street_Fund")), + "great-yarmouth.gov.uk": ( + ("Great Yarmouth Borough Council",), + ("Great Yarmouth",), + ("Town_Deal", "Future_High_Street_Fund"), + ), + "royalgreenwich.gov.uk": ( + ("Royal Borough of Greenwich",), + ("Woolwich Town Centre",), + ("Town_Deal", "Future_High_Street_Fund"), + ), + "halton.gov.uk": (("Halton Borough Council",), ("Runcorn",), ("Town_Deal", "Future_High_Street_Fund")), + "haringey.gov.uk": ( + ("London Borough of Haringey",), + ("Tottenham High Road",), + ("Town_Deal", "Future_High_Street_Fund"), + ), + "harlow.gov.uk": (("Harlow District Council",), ("Harlow",), ("Town_Deal", "Future_High_Street_Fund")), + "harrow.gov.uk": (("London Borough of Harrow",), ("Wealdstone",), ("Town_Deal", "Future_High_Street_Fund")), + "hartlepool.gov.uk": (("Hartlepool Borough Council",), ("Hartlepool",), ("Town_Deal", "Future_High_Street_Fund")), + "hastings.gov.uk": (("Hastings Borough Council",), ("Hastings",), ("Town_Deal", "Future_High_Street_Fund")), + "highpeak.gov.uk": (("High Peak Borough Council",), ("Buxton",), ("Town_Deal", "Future_High_Street_Fund")), + "huntingdonshire.gov.uk": ( + ("Huntingdonshire District Council",), + ("St Neots",), + ("Town_Deal", "Future_High_Street_Fund"), + ), + "ipswich.gov.uk": (("Ipswich Borough Council",), ("Ipswich",), ("Town_Deal", "Future_High_Street_Fund")), + "west-norfolk.gov.uk": ( + ("King's Lynn and West Norfolk",), + ("King's Lynn",), + ("Town_Deal", "Future_High_Street_Fund"), + ), + "kirklees.gov.uk": (("Kirklees Council",), ("Dewsbury",), ("Town_Deal", "Future_High_Street_Fund")), + "leeds.gov.uk": (("Leeds Council",), ("Morley",), ("Town_Deal", "Future_High_Street_Fund")), + "lewes-eastbourne.gov.uk": ( + ("Lewes District Council",), + ("Eastbourne Borough Council", "Newhaven"), + ("Town_Deal", "Future_High_Street_Fund"), + ), + "ad.lewes-eastbourne.gov.uk": ( + ("Lewes District Council",), + ("Eastbourne Borough Council", "Newhaven"), + ("Town_Deal", "Future_High_Street_Fund"), + ), + "lincoln.gov.uk": (("City of Lincoln Council",), ("Lincoln",), ("Town_Deal", "Future_High_Street_Fund")), + "mansfield.gov.uk": (("Mansfield District Council",), ("Mansfield",), ("Town_Deal", "Future_High_Street_Fund")), + "medway.gov.uk": (("Medway Council",), ("Chatham Town Centre",), ("Town_Deal", "Future_High_Street_Fund")), "mendip.gov.uk": ( ("Somerset Council",), ( From 8ff779cf49ff9519852eed3fc327083d0d29663a Mon Sep 17 00:00:00 2001 From: Cyrus <22678716+cyrusdobbs@users.noreply.github.com> Date: Thu, 30 Nov 2023 16:01:00 +0000 Subject: [PATCH 3/3] [BAU] add auth mapping validation --- app/main/authorisation.py | 21 +++++++++++++++++++++ tests/test_authorisation.py | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+) diff --git a/app/main/authorisation.py b/app/main/authorisation.py index 2ba46fa..051a6eb 100644 --- a/app/main/authorisation.py +++ b/app/main/authorisation.py @@ -1,6 +1,26 @@ from abc import ABC, abstractmethod +def validate_auth_args(func): + """Validates that all args passed to the decorated function are tuples of strings. + + :param func: the decorated function + :raises ValueError: if the args are invalid + """ + + def wrapper(*args): + for arg in args: + if isinstance(arg, AuthBase): + continue # don't validate self + if not isinstance(arg, tuple): + raise ValueError(f"Expected a tuple, but got {type(arg).__name__} in args: {args}") + if not all(isinstance(item, str) for item in arg): + raise ValueError(f"All elements in the tuple must be strings in args: {args}") + return func(*args) + + return wrapper + + class AuthBase(ABC): """Auth class ABC. Classes that inherit must implement a constructor, organisations and auth_dict methods.""" @@ -26,6 +46,7 @@ class TFAuth(AuthBase): place_names: tuple[str, ...] fund_types: tuple[str, ...] + @validate_auth_args def __init__(self, local_authorities: tuple[str, ...], place_names: tuple[str, ...], fund_types: tuple[str, ...]): self.local_authorities = local_authorities self.place_names = place_names diff --git a/tests/test_authorisation.py b/tests/test_authorisation.py index 34dfd42..ab2a14f 100644 --- a/tests/test_authorisation.py +++ b/tests/test_authorisation.py @@ -6,6 +6,7 @@ TFAuth, _auth_class_factory, build_auth_mapping, + validate_auth_args, ) @@ -123,3 +124,36 @@ def test_build_auth_mapping(mocker, mock_auth_class, mock_mapping): assert isinstance(auth_mapping, AuthMapping) assert auth_mapping.get_auth(list(mock_mapping.keys())[0]), "Mapping does not map to source data" + + +def test_validate_auth_args_valid(mock_auth_class): + @validate_auth_args + def dummy_func(*args): + pass + + mock_auth_class_instance = mock_auth_class(("Hogwarts",), ("Professor Albus Dumbledore",)) + + # test with valid arguments + dummy_func(mock_auth_class_instance, ("arg1", "arg2"), ("arg3", "arg4")) + + +def test_validate_auth_args_invalid_type(): + @validate_auth_args + def dummy_func(*args): + pass + + # test with invalid argument type + with pytest.raises(ValueError) as excinfo: + dummy_func(("arg1", "arg2"), ["arg3", "arg4"]) + assert str(excinfo.value) == "Expected a tuple, but got list in args: (('arg1', 'arg2'), ['arg3', 'arg4'])" + + +def test_validate_auth_args_invalid_element(): + @validate_auth_args + def dummy_func(*args): + pass + + # test with invalid tuple element + with pytest.raises(ValueError) as excinfo: + dummy_func(("arg1", "arg2"), ("arg3", 123)) + assert str(excinfo.value) == "All elements in the tuple must be strings in args: (('arg1', 'arg2'), ('arg3', 123))"