Skip to content

Commit b35b3db

Browse files
anjalitrace2-nhsanjalitrace2-nhs
committed
NRL-2015 Make v2 vs v1 decision (hopefully) more readable & reduce duplication of work
1 parent b2e301a commit b35b3db

File tree

8 files changed

+72
-79
lines changed

8 files changed

+72
-79
lines changed

‎layer/nrlf/core/authoriser.py‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,17 +25,24 @@ def get_pointer_permissions_v2(
2525
# check for app-wide permissions
2626
app_wide_key = f"{producer_or_consumer}/{app_id}.json"
2727
if path.isfile(f"/opt/python/nrlf_permissions/{app_wide_key}"):
28-
logger.log(LogReference.V2PERMISSIONS011, key=app_wide_key)
2928
key = app_wide_key
3029
else: # use org level
3130
key = f"{producer_or_consumer}/{app_id}/{ods_code}.json"
32-
logger.log(LogReference.V2PERMISSIONS011, key=key)
31+
32+
logger.log(LogReference.V2PERMISSIONS011, key=key)
3333
file_path = f"/opt/python/nrlf_permissions/{key}"
3434

3535
pointer_permissions = {}
3636
try:
3737
with open(file_path) as file:
3838
pointer_permissions = json.load(file)
39+
except FileNotFoundError as exc:
40+
logger.log(
41+
LogReference.V2PERMISSIONS013,
42+
exc_info=sys.exc_info(),
43+
error=str(exc),
44+
)
45+
raise exc
3946
except Exception as exc:
4047
logger.log(
4148
LogReference.S3PERMISSIONS005,

‎layer/nrlf/core/decorators.py‎

Lines changed: 24 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,11 @@
2525
X_REQUEST_ID_HEADER,
2626
AccessControls,
2727
PointerTypes,
28-
V2Headers,
2928
)
3029
from nrlf.core.dynamodb.repository import DocumentPointerRepository
3130
from nrlf.core.errors import OperationOutcomeError, ParseError
3231
from nrlf.core.logger import LogReference, logger
33-
from nrlf.core.model import PermissionsPolicy
32+
from nrlf.core.model import ConnectionMetadata, PermissionsPolicy
3433
from nrlf.core.request import parse_body, parse_headers, parse_params, parse_path
3534
from nrlf.core.response import Response
3635

@@ -73,7 +72,7 @@ def wrapper(*args, **kwargs) -> Dict[str, Any]:
7372

7473

7574
def header_handler(
76-
wrapped_func: Callable[..., Dict[str, Any]]
75+
wrapped_func: Callable[..., Dict[str, Any]],
7776
) -> Callable[..., Dict[str, Any]]:
7877
"""
7978
Wraps the function to set the specific headers in the request and response
@@ -117,7 +116,7 @@ def wrapper(*args, **kwargs) -> Dict[str, Any]:
117116

118117

119118
def logger_initialiser(
120-
wrapper_func: Callable[..., Dict[str, Any]]
119+
wrapper_func: Callable[..., Dict[str, Any]],
121120
) -> Callable[..., Dict[str, Any]]:
122121
"""
123122
Wraps the function and initialises the request logger
@@ -144,29 +143,25 @@ def wrapper(*args, **kwargs) -> Dict[str, Any]:
144143
RepositoryType = Union[Type[DocumentPointerRepository], None]
145144

146145

147-
def _use_v2_permissions_model(headers: Dict[str, str], path: str) -> bool:
148-
case_insensitive_headers = {key.lower(): value for key, value in headers.items()}
149-
150-
v2_headers_provided = (
151-
V2Headers.NHSD_END_USER_ORGANISATION_ODS in case_insensitive_headers.keys()
152-
and V2Headers.NHSD_NRL_APP_ID in case_insensitive_headers.keys()
153-
)
154-
if not v2_headers_provided:
155-
return False
156-
157-
metadata = parse_headers(headers, use_v2_permissions=True)
158-
v2_permissions_configured = get_pointer_permissions_v2(metadata, path) != {}
146+
def v1_perms_stuff(metadata: ConnectionMetadata, config: Config):
147+
if PERMISSION_ALLOW_ALL_POINTER_TYPES in metadata.nrl_permissions:
148+
logger.log(LogReference.HANDLER004a)
149+
metadata.pointer_types = PointerTypes.list()
150+
return metadata
159151

160-
return v2_permissions_configured
152+
logger.log(LogReference.HANDLER004b)
153+
pointer_types = parse_permissions_file(metadata)
154+
if not pointer_types and not metadata.is_test_event:
155+
logger.log(LogReference.HANDLER004)
156+
pointer_types = get_pointer_types(metadata, config)
161157

158+
metadata.pointer_types = pointer_types
159+
logger.log(LogReference.HANDLER004c, pointer_types=pointer_types)
162160

163-
def _load_v2_connection_metadata(headers: Dict[str, str], path: str):
164-
logger.log(LogReference.HANDLER004d)
161+
return metadata
165162

166-
metadata = parse_headers(headers, use_v2_permissions=True)
167-
logger.log(LogReference.HANDLER003, metadata=metadata.model_dump())
168163

169-
logger.log(LogReference.HANDLER004b)
164+
def v2_perms_stuff(metadata: ConnectionMetadata, path=""):
170165
pointer_permissions = get_pointer_permissions_v2(metadata, path)
171166

172167
metadata.nrl_permissions_policy = PermissionsPolicy.model_validate(
@@ -195,27 +190,16 @@ def _load_v2_connection_metadata(headers: Dict[str, str], path: str):
195190
def load_connection_metadata(headers: Dict[str, str], config: Config, path=""):
196191
logger.log(LogReference.HANDLER002, headers=headers)
197192

198-
if _use_v2_permissions_model(headers, path):
199-
return _load_v2_connection_metadata(headers, path)
200-
201-
metadata = parse_headers(headers, use_v2_permissions=False)
193+
metadata = parse_headers(headers)
202194
logger.log(LogReference.HANDLER003, metadata=metadata.model_dump())
203195

204-
if PERMISSION_ALLOW_ALL_POINTER_TYPES in metadata.nrl_permissions:
205-
logger.log(LogReference.HANDLER004a)
206-
metadata.pointer_types = PointerTypes.list()
207-
return metadata
208-
209-
logger.log(LogReference.HANDLER004b)
210-
pointer_types = parse_permissions_file(metadata)
211-
if not pointer_types and not metadata.is_test_event:
212-
logger.log(LogReference.HANDLER004)
213-
pointer_types = get_pointer_types(metadata, config)
214-
215-
metadata.pointer_types = pointer_types
216-
logger.log(LogReference.HANDLER004c, pointer_types=pointer_types)
196+
try:
197+
return v2_perms_stuff(metadata, path)
198+
except FileNotFoundError:
199+
# No v2 perms file found, so try v1 instead
200+
pass
217201

218-
return metadata
202+
return v1_perms_stuff(metadata, config)
219203

220204

221205
def filter_kwargs(handler_func: RequestHandler, kwargs: Dict[str, Any]):

‎layer/nrlf/core/log_references.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ class LogReference(Enum):
8888
"INFO", "Retrieved v2 pointer permissions from lambda layer"
8989
)
9090
V2PERMISSIONS013 = _Reference(
91-
"WARN", "No v2 permissions file found in lambda layer"
91+
"INFO", "No v2 permissions file found in lambda layer"
9292
)
9393

9494
# Parse Logs

‎layer/nrlf/core/request.py‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -11,12 +11,10 @@
1111
from nrlf.core.model import ClientRpDetails, ConnectionMetadata
1212

1313

14-
def _fetch_ods_app_id_headers(headers: dict[str, str]):
15-
14+
def _fetch_v2_ods_app_id_headers(headers: dict[str, str]):
1615
case_insensitive_headers = {key.lower(): value for key, value in headers.items()}
1716

1817
ods_code = case_insensitive_headers.get(V2Headers.NHSD_END_USER_ORGANISATION_ODS)
19-
2018
if not ods_code or len(ods_code.strip()) == 0:
2119
logger.log(
2220
LogReference.HANDLER003a,
@@ -33,9 +31,7 @@ def _fetch_ods_app_id_headers(headers: dict[str, str]):
3331
return ods_code, nrl_app_id
3432

3533

36-
def parse_headers(
37-
headers: Dict[str, str], use_v2_permissions=False
38-
) -> ConnectionMetadata:
34+
def parse_headers(headers: Dict[str, str]) -> ConnectionMetadata:
3935
"""
4036
Parses the connection metadata and client rp details from the headers passed from Apigee
4137
"""
@@ -49,8 +45,8 @@ def parse_headers(
4945
case_insensitive_headers.get(CONNECTION_METADATA, "{}")
5046
)
5147

52-
if use_v2_permissions:
53-
ods_code, nrl_app_id = _fetch_ods_app_id_headers(case_insensitive_headers)
48+
ods_code, nrl_app_id = _fetch_v2_ods_app_id_headers(case_insensitive_headers)
49+
if ods_code and nrl_app_id:
5450
raw_connection_metadata["nrl.ods-code"] = ods_code
5551
raw_connection_metadata["nrl.app-id"] = nrl_app_id
5652
raw_client_rp_details["developer.app.id"] = nrl_app_id

‎layer/nrlf/core/tests/test_authoriser.py‎

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
from unittest.mock import mock_open, patch
22

3+
import pytest
4+
35
from nrlf.core.authoriser import get_pointer_permissions_v2, parse_permissions_file
46
from nrlf.core.logger import LogReference, logger
57
from nrlf.core.request import parse_headers
@@ -76,17 +78,16 @@ def test_authoriser_get_v2_permissions_with_app_pointer_types(
7678
spy.assert_called_with(LogReference.V2PERMISSIONS011, key=expected_lookup_key)
7779

7880

79-
def test_authoriser_parse_v2_permission_file_with_no_permission_file(mocker):
80-
spy = mocker.spy(logger, "log")
81-
expected_lookup_key = "consumer/NotAnApp/NotFound.json"
81+
def test_authoriser_parse_v2_permission_file_with_no_permission_file():
82+
with pytest.raises(FileNotFoundError) as error:
83+
get_pointer_permissions_v2(
84+
connection_metadata=parse_headers(
85+
create_headers(ods_code="NotFound", nrl_app_id="NotAnApp")
86+
),
87+
request_path="/consumer/_status",
88+
)
8289

83-
metadata_result = get_pointer_permissions_v2(
84-
connection_metadata=parse_headers(
85-
create_headers(ods_code="NotFound", nrl_app_id="NotAnApp")
86-
),
87-
request_path="/consumer/_status",
90+
assert (
91+
f"No such file or directory: '/opt/python/nrlf_permissions/consumer/NotAnApp/NotFound.json'"
92+
in str(error.value)
8893
)
89-
90-
assert metadata_result == {}
91-
92-
spy.assert_any_call(LogReference.V2PERMISSIONS011, key=expected_lookup_key)

‎layer/nrlf/core/tests/test_decorators.py‎

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -794,14 +794,17 @@ def test_request_load_connection_metadata_with_permission_headers():
794794
expected_metadata = load_connection_metadata(
795795
headers=create_headers(nrl_permissions=[PERMISSION_ALLOW_ALL_POINTER_TYPES]),
796796
config=Config(),
797+
path="/consumer/something",
797798
)
798799

799800
assert expected_metadata.pointer_types == PointerTypes.list()
800801

801802

802803
def test_request_load_connection_metadata_with_no_permission_lookup_or_file():
803804
expected_metadata = load_connection_metadata(
804-
headers=create_headers(nrl_app_id="someId"), config=Config()
805+
headers=create_headers(nrl_app_id="someId"),
806+
config=Config(),
807+
path="/producer/something",
805808
)
806809

807810
assert expected_metadata.pointer_types == []
@@ -892,7 +895,7 @@ def test_load_connection_metadata_gets_v1_permissions_when_v2_permission_file_mi
892895
)
893896
mocker.patch(
894897
"nrlf.core.decorators.get_pointer_permissions_v2",
895-
return_value={},
898+
side_effect=FileNotFoundError("nope no v2 file here"),
896899
)
897900

898901
v1_plus_v2_headers = create_headers(
@@ -906,11 +909,11 @@ def test_load_connection_metadata_gets_v1_permissions_when_v2_permission_file_mi
906909
headers=v1_plus_v2_headers, config=Config(), path="/producer/DocumentReference"
907910
)
908911

909-
assert metadata.pointer_types == v1_permissions
910912
assert metadata.nrl_permissions_policy == None # no v2 permissions
913+
assert metadata.pointer_types == v1_permissions
911914

912915

913-
def test_load_connection_metadata_gets_v1_permissions_when_v2_headers_missing(
916+
def test_load_connection_metadata_throws_error_when_v2_permissions_lookup_encounters_genuine_error(
914917
mocker,
915918
):
916919
v1_permissions = [
@@ -921,24 +924,26 @@ def test_load_connection_metadata_gets_v1_permissions_when_v2_headers_missing(
921924
"nrlf.core.decorators.parse_permissions_file",
922925
return_value=v1_permissions,
923926
)
924-
v2_permissions = {"access_controls": [AccessControls.ALLOW_ALL_TYPES.value]}
925927
mocker.patch(
926928
"nrlf.core.decorators.get_pointer_permissions_v2",
927-
return_value=v2_permissions,
929+
side_effect=Exception("AAAH THIS IS A BIG PROBLEM"),
928930
)
929931

930-
v1_headers_only = create_headers(
932+
v1_plus_v2_headers = create_headers(
931933
additional_headers={
932934
V2Headers.NHSD_END_USER_ORGANISATION_ODS: "Y05868",
935+
V2Headers.NHSD_NRL_APP_ID: "Y05868-TestApp-12345678",
933936
}
934937
)
935938

936-
metadata = load_connection_metadata(
937-
headers=v1_headers_only, config=Config(), path="/producer/DocumentReference"
938-
)
939+
with pytest.raises(Exception) as err:
940+
load_connection_metadata(
941+
headers=v1_plus_v2_headers,
942+
config=Config(),
943+
path="/producer/DocumentReference",
944+
)
939945

940-
assert metadata.pointer_types == v1_permissions
941-
assert metadata.nrl_permissions_policy == None # no v2 permissions
946+
assert "AAAH THIS IS A BIG PROBLEM" in str(err.value)
942947

943948

944949
def test_load_v2_connection_metadata_allow_all_types(mocker: MockerFixture):

‎layer/nrlf/core/tests/test_request.py‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
from nrlf.core.errors import OperationOutcomeError, ParseError
66
from nrlf.core.logger import LogReference, logger
7-
from nrlf.core.request import _fetch_ods_app_id_headers, parse_body, parse_headers
7+
from nrlf.core.request import _fetch_v2_ods_app_id_headers, parse_body, parse_headers
88
from nrlf.producer.fhir.r4.model import DocumentReference
99
from nrlf.tests.data import load_document_reference_data
1010

@@ -59,7 +59,7 @@ def test_fetch_ods_app_id_headers(
5959
headers, expected_ods, expected_app_id, expected_log, mocker
6060
):
6161
spy = mocker.spy(logger, "log")
62-
ods_code, nrl_app_id = _fetch_ods_app_id_headers(headers)
62+
ods_code, nrl_app_id = _fetch_v2_ods_app_id_headers(headers)
6363

6464
assert ods_code == expected_ods
6565
assert nrl_app_id == expected_app_id
@@ -207,7 +207,7 @@ def test_parse_headers_valid_headers_v2_permissions():
207207
"nhsd-nrl-app-id": "X26-TestApp-12345",
208208
}
209209

210-
metadata = parse_headers(headers, use_v2_permissions=True)
210+
metadata = parse_headers(headers)
211211

212212
assert metadata.pointer_types == ["pointer_type"]
213213
assert metadata.ods_code == "X26"

‎layer/nrlf/tests/events.py‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ def create_test_api_gateway_event(
4949
) -> Dict[str, Any]:
5050
return {
5151
"resource": "/",
52-
"path": "/",
52+
"path": "/consumer/producer/",
5353
"httpMethod": "GET",
5454
"requestContext": {
5555
"resourcePath": "/",

0 commit comments

Comments
 (0)