BB2-4294: Update fhir_ids on token refresh #1437

bwang-icf · 2025-12-05T19:11:32Z

JIRA Ticket:
BB2-4294

What Does This PR Do?

This PR ensures that both fhir_id_v2 and fhir_id_v3 are populated if null, or updated if the value in the DB is different than what is returned from BFD, on refresh token flow (this already happened on authorization flow).

What Should Reviewers Watch For?

Do the added/modified unit tests adequately cover this change?
Do we still update user_mbi and other crosswalk fields as we did before this change was made (specifically looking at mymedicare_cb/models.py)? I tested all of this, but want to make sure it looks good to reviewers.

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

Validation

Do unit and integration tests all pass?
Here's the process I did to validate the functionality in addition to adding/modifying tests:
- Authorize in Postman (did this for v2 and v3 auth/token URLs)
- Grab that crosswalk record from the database, note the fhir_id_v3 and fhir_id_v2 values
- Update fhir_id_v3 to null, then click refresh the token via Postman, query that crosswalk record again and make sure fhir_id_v3 is equal to what it was before updating to null
- Repeat the same process as above for fhir_id_v2, and also update fhir_id_v3/fhir_id_v2 to some other value, then refresh the token and make sure they were set back to the correct value before any updates were done

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

…omments for now to pass flake8

…ken-refresh-fhir-update' into brandon/BB2-4294-token-refresh-fhir-update

apps/mymedicare_cb/models.py

…still need to functional test this)

…ity of this ticket, added a TODO

JamesDemeryNava · 2025-12-10T19:02:19Z

apps/dot_ext/tests/test_authorization.py

        response = self.client.post(reverse('oauth2_provider:authorize'), data=payload)
        self.assertEqual(response.status_code, 400)

+    def search_fhir_id_by_identifier_side_effect(self, search_identifier, request, version) -> str:


Unsure how to DRY this out. Same function is in test_models.py of mymedicare_cb. Was unable to import this function in test_models.py.

The way to do that would be (looking at the code):

Pull the function out of the class in (say) models

Import it in the other file

Because this function does not actually use self in any meaningful way, there's no reason for it to be nestled inside of the test class/fixture. It can be bumped up outside of the class scope. I think.

apps/dot_ext/tests/test_authorization.py

apps/dot_ext/views/authorization.py

jadudm · 2025-12-11T14:25:33Z

apps/authorization/tests/test_data_access_grant_permissions.py

            "client_secret": application.client_secret_plain,
        }
-        response = self.client.post("/v1/o/token/", data=refresh_post_data)
+        response = self.client.post("/v2/o/token/", data=refresh_post_data)


Although trivial, lets use this opportunity to update these kinds of things to the Versions constants if we're going to touch them?

jadudm · 2025-12-11T14:28:17Z

apps/dot_ext/tests/test_authorization.py

        }
        c = Client()
-        response = c.post('/v1/o/token/', data=token_request_data)
+        response = c.post('/v2/o/token/', data=token_request_data)


Same thought as above. Trivial, but we fould `f'{Versions.V2}' this or similar.

jadudm · 2025-12-11T14:29:34Z

apps/dot_ext/tests/test_authorization.py

+        # BB2-4294: Null out fhir_id_v2, then run a refresh token call, make sure fhir_id_v2 is then populated
+        # Update fhir_id_v3 to a random value to make sure it is updated
+        crosswalk.fhir_id_v2 = None
+        crosswalk.fhir_id_v3 = 'randomvalue'


Is it a random value, or a "sentinel value" that a developer could look for? FWIW, it's not random. I would let this go, though, as it is buried down in a test, and not used?

jadudm · 2025-12-11T14:31:39Z

apps/mymedicare_cb/tests/test_models.py

        self.assertEqual(user.crosswalk.user_mbi, slsx_mbi)
        mock_archive.assert_called_once()
+
+    @patch('apps.fhir.server.authentication.search_fhir_id_by_identifier', side_effect=search_fhir_id_by_identifier_side_effect)


Per prior convo, this was a nice test sequence. Good use of the @patch construct.

jadudm · 2025-12-11T15:04:04Z

apps/mymedicare_cb/tests/test_models.py

+        # Confirm fhir_id_v3 is None before calling the function
+        assert crosswalk.fhir_id_v3 is None
+
+        user, crosswalk_type = get_and_update_from_refresh(user_mbi, username, user_hicn_hash, mock_request)


Do we have a test/want a test that asserts that the result of an update via each method produces the same result? E.g. something that calls one refresh, caches a result/value from the DB, then resets the DB (say, by nulling out the v3 id), calls the other refresh, and then compares the two? Or, because of how we mock things, is that redundant/not meaningful?

clewellyn-nava · 2025-12-11T16:41:45Z

apps/mymedicare_cb/models.py



-def get_and_update_user(slsx_client: OAuth2ConfigSLSx, request):
+def _get_and_update_user(mbi, user_id, hicn_hash, request, auth_type, slsx_client=None):


Code style, do we want to follow the underscore=private? I'm ambivalent, but if this is something we all want to do everywhere I'm fine with it.

I know it's mentioned in https://peps.python.org/pep-0008/, but it's weak internal, so if we wanted to do something else I'd be interested.

clewellyn-nava · 2025-12-11T16:47:53Z

apps/testclient/views.py

        case _:
            logger.error(f"Failed to get valid API version back from authorizing agent. Given: [{version}]")
-            return ResponseErrors.MissingCallbackVersionContext(version)
+            return ResponseErrors().MissingCallbackVersionContext(version)


Small style comment, but should these methods in ResponseErrors be @staticmethods? Wouldn't have to instantiate ResponseErrors, it just looks a little odd and feels unnecessary.

@bwang-icf can you speak to this one? Not sure why this change was made.

This was again probably something made in the middle of trying to get the other unit tests to work. Just tried running the testclient tests locally and they passed without them, so we can get rid of them.

bwang-icf and others added 9 commits November 28, 2025 10:04

intermediary commit

1287195

getting properly failing test on refreshs

de48bf1

moving functionality to a base get_and_update_user call. Putting in c…

29635b0

…omments for now to pass flake8

intermediary commit

3e02801

unit tests currently fail but functionality for refresh works

d4fb87f

Merge branch 'master' into brandon/BB2-4294-token-refresh-fhir-update

aa3583f

having crosswalk checking on POST only on refresh and logging updates

c9d8852

Merge remote-tracking branch 'refs/remotes/origin/brandon/BB2-4294-to…

842d025

…ken-refresh-fhir-update' into brandon/BB2-4294-token-refresh-fhir-update

Merge branch 'master' into brandon/BB2-4294-token-refresh-fhir-update

f6cd0c4

JamesDemeryNava reviewed Dec 9, 2025

View reviewed changes

apps/mymedicare_cb/models.py Show resolved Hide resolved

JamesDemeryNava added 5 commits December 9, 2025 16:20

Initial cleanup - fix a test by fixing import, modify a conditional (…

e313983

…still need to functional test this)

Merge branch 'master' into brandon/BB2-4294-token-refresh-fhir-update

0899596

Update comments, remove print

a7725fa

Modify test_refresh_token so it demonstrates the change in functional…

7cb6464

…ity of this ticket, added a TODO

Add tests for get_and_update_from_refresh

4ac598c

JamesDemeryNava reviewed Dec 10, 2025

View reviewed changes

apps/dot_ext/tests/test_authorization.py Show resolved Hide resolved

Modify comment, throw 404 if crosswalk DNE during token flow

ed62d1e

JamesDemeryNava reviewed Dec 10, 2025

View reviewed changes

apps/dot_ext/views/authorization.py Show resolved Hide resolved

JamesDemeryNava changed the title ~~Brandon/bb2 4294 token refresh fhir update~~ BB2-4294: Update fhir_ids on token refresh Dec 10, 2025

JamesDemeryNava marked this pull request as ready for review December 10, 2025 19:27

Update what function a splunk dashboard is looking for

306d91e

jadudm reviewed Dec 11, 2025

View reviewed changes

clewellyn-nava reviewed Dec 11, 2025

View reviewed changes



		def get_and_update_user(slsx_client: OAuth2ConfigSLSx, request):
		def _get_and_update_user(mbi, user_id, hicn_hash, request, auth_type, slsx_client=None):

BB2-4294: Update fhir_ids on token refresh #1437

Are you sure you want to change the base?

BB2-4294: Update fhir_ids on token refresh #1437

Conversation

bwang-icf commented Dec 5, 2025 • edited by JamesDemeryNava 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bwang-icf commented Dec 5, 2025 •

edited by JamesDemeryNava

Loading