Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions apps/fhir/bluebutton/tests/fhir_resources/fhir_meta_v1.json
Original file line number Diff line number Diff line change
Expand Up @@ -128,11 +128,6 @@
"type": "string",
"documentation": "The offset used for result pagination"
},
{
"name": "_has:Coverage",
"type": "token",
"documentation": "Part D coverage type"
},
{
"name": "cursor",
"type": "string",
Expand Down
5 changes: 0 additions & 5 deletions apps/fhir/bluebutton/tests/fhir_resources/fhir_meta_v2.json
Original file line number Diff line number Diff line change
Expand Up @@ -136,11 +136,6 @@
"type": "string",
"documentation": "The offset used for result pagination"
},
{
"name": "_has:Coverage",
"type": "token",
"documentation": "Part D coverage type"
},
{
"name": "cursor",
"type": "string",
Expand Down
2 changes: 1 addition & 1 deletion apps/fhir/bluebutton/tests/test_read_and_search.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
def get_expected_read_request(version: int):
return {
'method': 'GET',
'url': f'{FHIR_SERVER["FHIR_URL"]}/v{version}/fhir/Patient/{FHIR_ID_V2}/?_format=json',
'url': f'{FHIR_SERVER["FHIR_URL"]}/v{version}/fhir/Patient/{FHIR_ID_V2}/?_format=json&_id={FHIR_ID_V2}',
'headers': {
# 'User-Agent': 'python-requests/2.20.0',
'Accept-Encoding': 'gzip, deflate',
Expand Down
46 changes: 39 additions & 7 deletions apps/fhir/bluebutton/tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
crosswalk_patient_id,
get_resourcerouter,
build_oauth_resource,
valid_caller_for_patient_read,
valid_patient_read_or_search_call,
)

ENCODED = settings.ENCODING
Expand Down Expand Up @@ -70,16 +70,48 @@ def test_notNone(self):
response = notNone(listing, "number")
self.assertEqual(response, listing)

def test_valid_caller_for_patient_read(self):
result = valid_caller_for_patient_read('PatientId:-20140000008326', '-20140000008326')
def test_valid_patient_read_or_search_call_valid_read_calls(self):
result = valid_patient_read_or_search_call('PatientId:-20140000008329', '-20140000008329', '')
assert result is True

result = valid_caller_for_patient_read('PatientId:-20140000008326', '-20140000008329')
result = valid_patient_read_or_search_call('PatientId:-99140000008329', '-99140000008329', '')
assert result is True

def test_valid_patient_read_or_search_call_invalid_read_calls(self):
result = valid_patient_read_or_search_call('PatientId:-20140000008329', '-99140000008329', '')
assert result is False

result = valid_patient_read_or_search_call('PatientId:-99140000008329', '-20140000008329', '')
assert result is False

# call with no colon in beneficiary_id to make sure
invalid_call = valid_caller_for_patient_read('PatientId-20140000008326', '-20140000008329')
assert invalid_call is False
def test_valid_patient_read_or_search_call_valid_search_calls(self):
result = valid_patient_read_or_search_call('PatientId:-20140000008329', None, '_id=-20140000008329')
assert result is True

result = valid_patient_read_or_search_call(
'PatientId:-99140000008329',
None,
'_lastUpdated=lt2024-06-15&startIndex=0&cursor=0&_id=-99140000008329'
)
Comment on lines +91 to +95
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we add a test case that would've caught the earlier issue, where _id is given first?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

definitely, will add it after the release!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was actually able to add it now.

assert result is True

result = valid_patient_read_or_search_call(
'PatientId:-99140000008329',
None,
'_id=-99140000008329&_lastUpdated=lt2024-06-15&startIndex=0&cursor=0'
)
assert result is True

def test_valid_patient_read_or_search_call_invalid_search_calls(self):
result = valid_patient_read_or_search_call('PatientId:-20140000008329', None, '_id=-99140000008329')
assert result is False

result = valid_patient_read_or_search_call(
'PatientId:-99140000008329',
None,
'_lastUpdated=lt2024-06-15&startIndex=0&cursor=0&_id=-20140000008329'
)
assert result is False


class BlueButtonUtilSupportedResourceTypeControlTestCase(TestCase):
Expand Down
35 changes: 25 additions & 10 deletions apps/fhir/bluebutton/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
from collections import OrderedDict
from datetime import datetime
from pytz import timezone
from typing import Optional
from urllib.parse import parse_qs

from django.conf import settings
from django.contrib import messages
Expand Down Expand Up @@ -745,18 +747,31 @@ def get_patient_by_mbi_hash(mbi_hash, request):
return response.json()


def valid_caller_for_patient_read(beneficiary_id: str, patient_id: str) -> bool:
"""When making a read patient call, we only want to ping BFD if the patient_id
being passed matches the fhir_id (currently v2) associated with the current session
def valid_patient_read_or_search_call(beneficiary_id: str, resource_id: Optional[str], query_param: str) -> bool:
"""Determine if a read or search Patient call is valid, based on what was passed for the resource_id (read call)
or the query_parameter (search call)

Args:
beneficiary_id (str): beneficiary_id that is associated with the current session/
Should have format 'patientId:00000000000000 coming in
patient_id (str): patient_id that is will be passed to BFD
beneficiary_id (str): This comes from the BlueButton-OriginalQuery attribute of the headers for the API call.
Has the format, patientId:{{patientId}}, where patientId comes from the bluebutton_crosswalk table
resource_id (Optional[str]): This will only be populated for read calls, and it is the id being passed to BFD
query_param (str): String for the query parameter being passed for search calls. For read calls this is a blank string

Returns:
bool: If the patient_id matches the beneficiary_id, then return True, else False
bool: Whether or not the call is valid
"""
parts = beneficiary_id.split(':', 1)
beneficiary_comparison_id = parts[1] if len(parts) == 2 else None
return beneficiary_comparison_id == patient_id
bene_split = beneficiary_id.split(':', 1)
beneficiary_id = bene_split[1] if len(bene_split) > 1 else None
# Handles the case where it is a read call, but what is passed does not match the beneficiary_id
# which is constructed using the patient id for the current session in generate_info_headers.
if resource_id and beneficiary_id and resource_id != beneficiary_id:
return False

# Handles the case where it is a search call, but what is passed does not match the beneficiary_id
# so a 404 Not found will be thrown before reaching out to BFD
query_dict = parse_qs(query_param)
passed_identifier = query_dict.get('_id', [None])
if passed_identifier[0] and passed_identifier[0] != beneficiary_id:
return False

return True
24 changes: 17 additions & 7 deletions apps/fhir/bluebutton/views/generic.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
from ..utils import (build_fhir_response,
FhirServerVerify,
get_resourcerouter,
valid_caller_for_patient_read)
valid_patient_read_or_search_call)

logger = logging.getLogger(bb2logging.HHS_SERVER_LOGNAME_FMT.format(__name__))

Expand Down Expand Up @@ -151,16 +151,26 @@ def fetch_data(self, request, resource_type, *args, **kwargs):

prepped = s.prepare_request(req)

resource_id = kwargs.get('resource_id')
beneficiary_id = prepped.headers.get('BlueButton-BeneficiaryId')
if resource_type == 'Patient':
query_param = prepped.headers.get('BlueButton-OriginalQuery')
resource_id = kwargs.get('resource_id')
beneficiary_id = prepped.headers.get('BlueButton-BeneficiaryId')

if resource_type == 'Patient' and resource_id and beneficiary_id:
# If it is a patient read request, confirm it is valid for the current user
# If not, throw a 404 before pinging BFD
if not valid_caller_for_patient_read(beneficiary_id, resource_id):
# For patient read and search calls, we need to ensure that what is being passed, either in
# query parameters for search calls, or in the resource_id for read calls, is valid for the
# current session (matching the beneficiary_id). If not, raise a 404 Not found before calling BFD.
if not valid_patient_read_or_search_call(beneficiary_id, resource_id, query_param):
error = NotFound('Not found.')
raise error

# Handle the case where it is a patient search call, but neither _id or identifier were passed
if '_id' not in get_parameters.keys() and 'identifier' not in get_parameters.keys():
# depending on if 4166 is merged first, this will need to be updated
get_parameters['_id'] = request.crosswalk.fhir_id(2)
# Reset the request parameters and the prepped request after adding the missing, but required, _id param
req.params = get_parameters
prepped = s.prepare_request(req)

match self.version:
case Versions.V1:
api_ver_str = 'v1'
Expand Down
7 changes: 5 additions & 2 deletions apps/fhir/bluebutton/views/search.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,11 @@ def build_url(self, resource_router, resource_type, *args, **kwargs):
class SearchViewPatient(SearchView):
# Class used for Patient resource search view
required_scopes = ['patient/Patient.read', 'patient/Patient.rs', 'patient/Patient.s']
QUERY_SCHEMA = {
**SearchView.QUERY_SCHEMA,
'_id': str,
'identifier': str
}
Comment on lines +86 to +90
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were these params previously not even being processed at all because this was missing? Also is it true that SearchView.QUERY_SCHEMA already has all the other params we might expect here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an additional general question, I'm seeing that when invalid query parameters are provided, those are just ignored. Are you seeing any mechanism in these QUERY_SCHEMAs or elsewhere that would lead to a 400 error in cases where invalid params are provided? It would be fair to not worry about that in this PR, but to maybe open a separate ticket to do more query param validation that would return 400s when appropriate (maybe even just on v3 endpoints).

Copy link
Contributor Author

@JamesDemeryNava JamesDemeryNava Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my testing in SBX and prod, for Patient search specifically, lastUpdated and startIndex were both being passed to BFD, though i'm not sure how that is happening as there is no QUERY_SCHEMA for patient search in those environments currently. _id and identifier are not currently being parsed/passed in deployed environments. Also appears that cursor is not being passed, though i'm not 100% sure.

Looking into a way to throw 400s for invalid params, if I can find something quickly, i'll include it here and update the ticket. Otherwise, will write up a separate ticket for that.

When startIndex is included for v3, a 400 is thrown, so there's appear to be some handling of that either by BFD or HAPI FHIR.

Copy link
Contributor

@jimmyfagan jimmyfagan Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it inherits last updated and start index from SearchView.QUERY_SCHEMA, so that seems to make sense. I think you're right that cursor is not used and is not passed through, it is only included in tests in such a way that it would make no difference whether cursor was provided or not. We should remove that from our swagger docs, but not in this ticket probably (edit: since we're removing has:coverage in this PR, maybe removing cursor makes sense too, but if we decide not to, maybe we just open a separate ticket for that).

I don't think start Index is currently supported in BFD, so that's probably all that is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm leaning towards writing a new ticket for throwing 400 for unsupported params before we even ping BFD, but not 100% sure yet. Need to understand a bit more the level of effort required around that. I imagine we could include the cursor removal from swagger docs in that ticket.


def __init__(self, version=1):
super().__init__(version)
Expand All @@ -91,8 +96,6 @@ def __init__(self, version=1):
def build_parameters(self, request, *args, **kwargs):
return {
'_format': 'application/json+fhir',
# BB2-4166-TODO : this needs to use self.version to determine fhir_id
'_id': request.crosswalk.fhir_id(2)
}


Expand Down
Loading
Loading