Adds the pathways for v3 handling around fhir_ids (BB-4166) #1419

bwang-icf · 2025-11-13T13:16:45Z

JIRA Ticket:
BB2-4166

What Does This PR Do?

Adds the pathways for v3 handling around fhir_ids. There are a lot of new spots where we are extending in the version that did not previously have it and should now have fhir_ids for either v1/v2 or v3 updated whenever there is an authorization or refresh.

What Should Reviewers Watch For?

Any places where the version retrieval is not correct. There are a lot of different ways that version is retrieved and there should be some work to change it so that there is one singular stable way of getting the version.
Any errant print statements that muddy up the code
There is a known area of TODOs around the apps/authorization/models that was determined to work primarily with Quicksight and was deemed not critical for the work here. This can be moved to a new ticket

If you're reviewing this PR, please check for these things in particular:

Validation

Validated with a local run and flow of the test client

During initial auth with v2 using a user that has v3 information (such as BBUser09003), view the local db columns for fhir_id, fhir_id_v2, and fhir_id_v3. For BBUser09003, they should all be populated with -10000010284539 for fhir_id/fhir_id_v2 and -206068516 for fhir_id_v3.
Manipulate the fhir_id values (erase them, add extra digits, remove digits, etc.) and perform a re-auth with the same user in v2. The values should return to their original values.
Perform the same steps as above using a v3 auth token using a user like BBUser09003 that has v3 information. We want to see the same fhir_id columns with the same values.
Perform steps 1 and 2 using a v2 auth token with a user that does NOT have v3 information (such as BBUser00001) and view the local db columns. The values should be updated to have a fhir_id_v2 but no fhir_id_v3.
Manipulate the fhir_id_v2 and re-auth to ensure that will also return to the original value.
Authenticate/re-authenticate with a user that does not have a fhir_id for the specific version you are requesting. If there is no fhir_id corresponding to the request's version, we should error out. We may need to reach out to BFD to ask for another user to use for this test case.

What Security Implications Does This PR Have?

Please indicate if this PR does any of the following:

Adds any new software dependencies
Modifies any security controls
Adds new transmission or storage of data
Any other changes that could possibly affect security?

[] Yes, one or more of the above security implications apply. This PR must not be merged without the ISSO or team
security engineer's approval.

Any Migrations?

Yes, there are migrations
- The migrations should be run PRIOR to the code being deployed
- The migrations should be run AFTER the code is deployed
- There is a more complicated migration plan (downtime,
  etc)
No migrations

…nized as v3

…s function still

…t appears to work. Will be open to refactoring

…estclient patient/EOB/coverage are now broken, some tests are broken as well. Will fix in morning

…in create_token_response

…function throwing 404

…nctionality is working)

…and 11 errors

…tend more off version

… failing and we have some TODOs, but v3 and v2 endpoints all work

…ly do not pass a value for a log statement

…rom BB2-4224

JamesDemeryNava · 2025-11-13T16:33:25Z

apps/mymedicare_cb/models.py

            hicn_updated = True

+        update_fhir_id = False
+        if user.crosswalk.fhir_id(Versions.V2) == '' or user.crosswalk.fhir_id(Versions.V3) == '':


@clewellyn-nava this changed as a result of 4224, as we now return a blank string from the fhir_id function. Wanted to call out to make sure this looks good to you. The behavior of a null fhir_id_v3 or v2 getting updated on a re-auth was broken as a result of this, but we've now confirmed it works again.

Apologies, it was silently returning None on implicit codepaths and I wanted to make it explicit, didn't think about downstream None checks in this PR.

This can be revisited when we remove the fhir_id fully, in my opinion.

apps/mymedicare_cb/models.py

…ids are always updated if DB values differ from what BFD returns

JamesDemeryNava · 2025-11-14T12:56:48Z

@bwang-icf can you add some details on validating specific scenarios like a fhir_id_v2 being none, a re-auth happens, fhir_id_v2 is populated, updating the columns with whatever comes from BFD, etc.?

bwang-icf · 2025-11-14T18:25:36Z

@bwang-icf can you add some details on validating specific scenarios like a fhir_id_v2 being none, a re-auth happens, fhir_id_v2 is populated, updating the columns with whatever comes from BFD, etc.?

That's a good idea. I'll add some more validation steps here and the expected outcomes. Some of these will need some input from the team/BFD to determine actual expected outcomes.

During initial auth with v2 using a user that has v3 information (such as BBUser09003), view the local db columns for fhir_id, fhir_id_v2, and fhir_id_v3. For BBUser09003, they should all be populated with -10000010284539 for fhir_id/fhir_id_v2 and -206068516 for fhir_id_v3.
Manipulate the fhir_id values (erase them, add extra digits, remove digits, etc.) and perform a re-auth with the same user in v2. The values should return to their original values.
Perform the same steps as above using a v3 auth token using a user like BBUser09003 that has v3 information. We want to see the same fhir_id columns with the same values.
Perform steps 1 and 2 using a v2 auth token with a user that does NOT have v3 information (such as BBUser00001) and view the local db columns. The values should be updated to have a fhir_id_v2 but no fhir_id_v3.
Manipulate the fhir_id_v2 and re-auth to ensure that will also return to the original value.
Authenticate/re-authenticate with a user that does not have a fhir_id for the specific version you are requesting. If there is no fhir_id corresponding to the request's version, we should error out. We may need to reach out to BFD to ask for another user to use for this test case.

JamesDemeryNava · 2025-11-14T20:06:22Z

@bwang-icf @jimmyfagan @jadudm @clewellyn-nava and anyone else who reviews this work:

For some v2 users, there will never be a fhir_id_v3 (as of now). One example is BBUser00001. It does not matter if it is the initial auth for that user, or a re-auth, fhir_id_v3 will not be retrievable from BFD, and the fhir_id_v3 in bluebutton_crosswalk will remain null
- For users like BBUser00001, if you try to go through the v3 testclient auth flow, you will see the "Your patient data cannot be found at this time screen".
For other v2 users, like BBUser00575, there is fhir_id_v3 data. BFD will return fhir_id_v3 data both on initial auth, and on re-auth. So on the initial auth for a user like BBUser00575, both fhir_id_v2 and fhir_id_v3 will be populated. BBUser00575 is able to go through the v3 testclient flow and then execute v3 API calls in the test client window.

I did not realize that not all v2 synthetic users have v3 data, that may already be common knowledge, but want to call it out again here.

stiwarisemanticbits · 2025-11-17T20:02:30Z

apps/authorization/views.py

+                case Versions.V1:
+                    user = Crosswalk.objects.get(fhir_id_v2=patient_id).user


Do we have fhir_id_v1 or both v1 and v2 share fhir_id_v2 , if so could you add comment in here to avoid confusion?

Sure thing. I can add a comment here, but for most areas we are definitely treating v1/v2 as the same thing since their pathways are fairly similar.

@stiwarisemanticbits , for reference (and so we have it here):

There used to be only fhir_id. We have now introduced _v2 and _v3, and the references to fhir_id became a function on the model that now takes a version parameter. So, everywhere we used to say:

crosswalk.fhir_id

(which was, at that time, implicitly already v1 and v2)

we now say:

crosswalk.fhir_id(Version.Vx)

So, commenting that V1 now shares V2 is something we would have to comment throughout the entire codebase. @bwang-icf , before we start commenting this in the code, we may want to decide if/where we need to document this kind of design work. I personally think it would be the TRD.

I think commenting on the model is sufficient, otherwise we run into issues you described.

I believe we can also use | in case statements in Python 3.11
case Version.V1 | Version.V2
This has the code read that they are treated the same, and if someone is curious as to why, they can reference the model comment. Just my two cents.

stiwarisemanticbits · 2025-11-17T20:03:43Z

apps/versions.py

+    def supported_versions():
+        return [Versions.V1, Versions.V2, Versions.V3]
+
+    def latest_versions():
+        return [Versions.V2, Versions.V3]
+


We renamed this file but https://github.com/CMSgov/bluebutton-web-server/blob/15c1c3a62c7489df3b3596ed05c3c8abcce14980/apps/integration_tests/selenium_tests.py
still seems to be calling constants.py

Good catch. Correcting this

stiwarisemanticbits · 2025-11-17T20:08:32Z

apps/fhir/bluebutton/utils.py

-        if crosswalk.fhir_id(2) is not None:
-            result["BlueButton-BeneficiaryId"] = "patientId:" + str(crosswalk.fhir_id(2))
+        # TODO: As we move to v2/v3, v3 does not use the hicnHash. We will want to refactor.
+        if crosswalk.fhir_id(version) is not None:


I checked crosswalk.fhir_id(version) is also returning empty string, which will not be none and will go to else block

I think that would be expected behavior if we couldn't find a fhir id. If it's v2, then we would try to use hicn and if it's v3, it should just error out to a patient not found. Are you thinking something unintended would happen?

@bwang-icf , you may want to look at this more closely. We did, for a bit, have behavior where crosswalk.fhir_id(version) could return None or a str. Now, we are always returning a str. (Unless that changed again?) So, this check will always fail.

I haven't expanded the code, but it would be good to reason through: if that code pathway always returns a string, and we're checking if it is not None (implying, under previous logic, that something was found)... is it the same now that we're always returning str, and we return '' when there is no value found?

(Edit: Actually, looking at this just a moment more, @stiwarisemanticbits has asked a really good question. I'd suggest walking through this code once more with that mindset: we no longer return None when something is not found, we return '', which means we may need to rethink the logic in this section in more way than one.)

Trying to get this with the debugger by running a v3 request with user 00001 and I'm getting the patient 404 Your patient data cannot be found at this time error before it can get hit. I wondered if it was something funky with the debugger and tried 09003 and that did go through and trigger this code, so it seems like if we don't have the fhir_id, we display that page before we get here. Will look some more into it tomorrow.

stiwarisemanticbits · 2025-11-17T20:19:22Z

apps/mymedicare_cb/models.py

+            fhir_id, hash_lookup_type = match_fhir_id(
+                mbi=slsx_client.mbi,
+                hicn_hash=hicn_hash,
+                request=request,
+                version=supported_version,


When looping over v3 for a user without v3 data, match_fhir_id raises exceptions.NotFound, which we are not handling here. Is that expected? Before we reach to v3 v2 lookup is already succeeded, but overall method will fail.

If I'm understanding correctly, you're seeing that if a request's version (v3 in this case) does NOT have a corresponding v3 fhir_id, we're erroring out even though we have a v2 fhir_id. That is expected behavior because we would not want to return the wrong version's fhir_id on a request as that could lead to complications down the line. Let me know if I'm missing something.

I don't understand... @JamesDemeryNava , didn't we add something to the else clause here, so that the dictionaries would be more consistent/have values for every version? Granted, I don't think that would address @stiwarisemanticbits 's point.

I've ticketed cleaning up match_fhir_id. It is in the critical path, and as @stiwarisemanticbits (implicitly) points out, it is too easy for us to miss things in this function that could impact behavior.

https://jira.cms.gov/projects/BB2/issues/BB2-4265

Perhaps we will pick up the ticket, or perhaps we'll come up with something else.

@jadudm We discussed it but I didn't push it. With doing a .get, I don't think it's necessary. Could be convinced otherwise, I think it depends where we land on the discussion above for v3 handling when it's not available.

clewellyn-nava

Good work to everyone involved on this one, definitely a doozy.

… some day

JamesDemeryNava

ensuring this isn't merged until the TODO in generic.py is addressed (working on it now)

todo has been addressed

bwang-icf and others added 30 commits November 6, 2025 10:40

initial commit

df80391

adding version specific permission checking so v3 resources are recog…

cfeb712

…nized as v3

TODO in authorization/views.py. Need to write unit tests for new util…

ef64ba6

…s function still

Address TODOs in bb2_tools/admin.py

cf33395

found request.path to contain version. Different from other places bu…

ab29772

…t appears to work. Will be open to refactoring

Ensure fhir_id v2 or v3 are updated if none when re-authorizing. v3 t…

433d5c5

…estclient patient/EOB/coverage are now broken, some tests are broken as well. Will fix in morning

adding extra validation for incorrect versions in utils and using it …

157d988

…in create_token_response

V2 calls all working, v3 patient returning correct data but built in …

2707686

…function throwing 404

adding more generalized version grabbing for crosswalk permissions

6cae063

Revert log to reduce test failures (will revisit this log once all fu…

a58570a

…nctionality is working)

Ensure v3 coverage and EOD endpoints work. Currently 7 test failures …

6bf69b0

…and 11 errors

changes to get_patient_id, test_read_and_search and other tests to ex…

cb698da

…tend more off version

Modify some tests, pass a version to generate_info_headers. Tests are…

4838058

… failing and we have some TODOs, but v3 and v2 endpoints all work

Merge branch 'master' into bw/BB2-4166

8614644

added notes and some changes to mymedicare views

c65b63c

Interim collab commit

90c7921

Remove completed TODOs

672c806

Remove more completed TODOs

87c39e7

Remove more TODOs that had been completed

717462b

Fix audit_logger tests, WIP on authentication

ebf5125

Fixed mocked tests

8525ea3

Removing print statements

8ea6bd7

Merge branch 'master' into bw/BB2-4166

6aec825

Add version param to create_fhir_mock to help tests succeed

e6bc40b

Revert unneeded test changes, ensure still passes

aa7dc60

Fix last failing test in test_authentication.py

f4c043d

Add unit test, remove completed TODOs

69e9d8d

WIP On fixing tests

dd315f8

Interim

98c523b

Fixed callback tests

b873afb

JamesDemeryNava added 3 commits November 13, 2025 10:47

revert to have v2 as default to fhir_id, as it broke tests, and we on…

3982daa

…ly do not pass a value for a log statement

Check on blank string in get_and_update_user to accommodate changes f…

0bdd43d

…rom BB2-4224

Single quotes and comment context

b95785b

JamesDemeryNava reviewed Nov 13, 2025

View reviewed changes

apps/mymedicare_cb/models.py Outdated Show resolved Hide resolved

JamesDemeryNava and others added 3 commits November 13, 2025 16:55

Refactor get_and_update_user to reduce BFD calls, and to ensure fhir_…

f6d4d7d

…ids are always updated if DB values differ from what BFD returns

Updates based on PR self-review with Brandon and Matt

1ea136e

adding latest versions and changes to the offset math test

85fd19c

bwang-icf marked this pull request as ready for review November 13, 2025 23:22

added defaults if there is no fhir_id found for a version

675f69d

bwang-icf added 2 commits November 14, 2025 14:18

Merge branch 'master' into bw/BB2-4166

72f0819

Merge branch 'master' into bw/BB2-4166

a31aeba

stiwarisemanticbits reviewed Nov 17, 2025

View reviewed changes

addressing comments

f19d8a4

clewellyn-nava previously approved these changes Nov 18, 2025

View reviewed changes

JamesDemeryNava and others added 3 commits November 18, 2025 13:55

Merge branch 'master' into bw/BB2-4166

4cf4355

Merge branch 'master' into bw/BB2-4166

4e41897

Resolve conflict, still work to do to resolve TODO

63e5245

JamesDemeryNava dismissed clewellyn-nava’s stale review via 63e5245 November 19, 2025 17:34

changing check to empty string over None but will change back to None…

80307e3

… some day

JamesDemeryNava previously requested changes Nov 19, 2025

View reviewed changes

Address TODO that arose from a change made in BB2-4233

6021985

use self.version instead

91c3a37

clewellyn-nava approved these changes Nov 20, 2025

View reviewed changes

bwang-icf merged commit 744a8f5 into master Nov 20, 2025
8 checks passed

bwang-icf deleted the bw/BB2-4166 branch November 20, 2025 17:02

		case Versions.V1:
		user = Crosswalk.objects.get(fhir_id_v2=patient_id).user

Adds the pathways for v3 handling around fhir_ids (BB-4166) #1419

Adds the pathways for v3 handling around fhir_ids (BB-4166) #1419

Uh oh!

Conversation

bwang-icf commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What Does This PR Do?

What Should Reviewers Watch For?

Validation

What Security Implications Does This PR Have?

Any Migrations?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

JamesDemeryNava commented Nov 14, 2025

Uh oh!

bwang-icf commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JamesDemeryNava commented Nov 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clewellyn-nava Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jadudm Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jadudm Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clewellyn-nava left a comment

Choose a reason for hiding this comment

Uh oh!

JamesDemeryNava left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bwang-icf commented Nov 13, 2025 •

edited

Loading

bwang-icf commented Nov 14, 2025 •

edited

Loading

clewellyn-nava Nov 18, 2025 •

edited

Loading

jadudm Nov 18, 2025 •

edited

Loading

jadudm Nov 18, 2025 •

edited

Loading