[ryan-morosa]

JIRA Ticket:
BB2-5225

What Does This PR Do?

This PR ties the rest of asymmetric auth worked on from BB2-5227 into the token endpoint. We can now get an access_token_response with all the fields in it going through an asymmetric auth flow.
Summary Of Changes:

• Tied in asymmetric auth validation into the post token endpoint
• Added a full integration test for asymmetric auth flow and other unit tests for updating a couple other validation functions.
• Ensured CAN did not regress

What Should Reviewers Watch For?

• Any other tests needed?
• Is having the same required params for CAN and Asymmetric Auth okay (i.e. grant_type, scope, client_assertion, client_assertion_type)? Asymmetric Auth technically has code, code_verifier, and redirect_uri too but that gets validated by create_token_response later on in authorization.py. I was also unsure about scope being required for asymmetric auth since it wasn't in the specs found in references of this confluence page: https://confluence.cms.gov/pages/viewpage.action?pageId=1708786644&spaceKey=BB2&title=Asymmetric%20Auth%20Implementation%20Plan
• Is it okay to include logging in a different story? I think it coould be good to add to the scope of that here: https://jira.cms.gov/browse/BB2-5295

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

Validation

• The asymmetric auth flow end-to-end integration test and CAN integration test passing should be suffice for showing the output response.
• You can do a make run-local CAN_INTEGRATION_TEST=true and add a print statement of the access_token_response at the end of the asymmetric auth flow test and then run pytest -s apps/fhir/bluebutton/tests/test_jwt_integration.py::test_asymmetric_auth_flow_integration_successful to see the actual token response with everything in it.
• I haven't been able to do a manual test yet so not sure if/how we need to do that. We can probably follow the instructions here for that: https://hl7.org/fhir/smart-app-launch/STU2.2/example-app-launch-asymmetric-auth.html#step-5-access-token

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

[JamesDemeryNava]

Is it okay to include logging in a different story? I think it coould be good to add to the scope of that here: https://jira.cms.gov/browse/BB2-5295

For this question, I guess it depends on when we think 5295 will be completed, and whether or not this functionality will be used before the logging is in place. If the functionality won't be used before the logging is added, I think it's fine to do it in 5295.

If the functionality will be used before 5295 is merged, i'd say we should include it here. I'm not sure exactly where asymmetric auth is at right now

[ryan-morosa]

Is it okay to include logging in a different story? I think it coould be good to add to the scope of that here: https://jira.cms.gov/browse/BB2-5295

For this question, I guess it depends on when we think 5295 will be completed, and whether or not this functionality will be used before the logging is in place. If the functionality won't be used before the logging is added, I think it's fine to do it in 5295.

If the functionality will be used before 5295 is merged, i'd say we should include it here. I'm not sure exactly where asymmetric auth is at right now

yeah I think we talked about it at eng sync and it looks like we just included it as a part of 5295.

[JamesDemeryNava]

Validation steps all worked for me! Will go through the rest of the code review first thing tomorrow.

[ryan-morosa]

Making this comment to ensure we create the asymmetric_auth_validation waffle switch in test, sbx, and prod after this gets deployed. Probably needs to be noted in the next upcoming deployment management ticket.

[jimmyfagan]

That switch was already introduced right? Even if not, we can pre-emptively add the switch, might be worth doing that as part of your ticket to avoid missing it in deployment.

[JamesDemeryNava]

Validation steps work and tests look good! Left a few comments, and should probably wait for Jimmy to approve as well.

[jimmyfagan]

Didn't have a chance to test this out yet, but left some comments based on looking at the changes, I have a feeling some refactor here might be useful, the token post method is kind of messy given all of the different paths that go through that method, and I like the instinct to extract some of the request validation logic, I just don't know if the validators is the best place for that.

Also to answer your question from the description, scope should be required here, in particular because we require it for authorization_code token requests in v3, and asymmetric auth should only be on v3.

[jimmyfagan]

I like the improvements, will keep an eye out for more re-reviews

[jimmyfagan]

That switch was already introduced right? Even if not, we can pre-emptively add the switch, might be worth doing that as part of your ticket to avoid missing it in deployment.

[jimmyfagan]

Having some readability issues figuring out what these cases are. Can the parameters be named in the cases listed above to make it more clear which boolean goes to what? If not, maybe in-line comment labels or something? Probably some opportunity for us to adopt a new style guideline here. Can we add a function comment that describes what each parameter represents? In particular, token_request_fields, remove_field, add_field, field, are not very intuitive to me. These seem to work, but making them more human readable I think helps maintain, troubleshoot, and add new tests when needed.

[ryan-morosa]

totally fair - I'll try to see if there's a better way/add some doc strings

[jimmyfagan]

Do you know if we ever hit this case? I think it remains to be seen how an asymmetric-only app would be set, but as it is right now, it would still have a client secret since the client secret is set when the app is created. I suspect we may eventually want to add something like this if that changes, but as it is now, I don't know that this is necessary.

[ryan-morosa]

Gotcha. Yeah I though it would be good to add this piece after working on https://jira.cms.gov/browse/BB2-5229 since it looked like we might lean toward no client secret for asymmetric auth being set but I can remove it and make it a separate story if we think that's better. Might've scope creeped this story a bit.