Skip to content

BB2-4961: Switch Account Link#1649

Open
JamesDemeryNava wants to merge 16 commits into
masterfrom
jamesdemery/bb2-4961-switch-account-link
Open

BB2-4961: Switch Account Link#1649
JamesDemeryNava wants to merge 16 commits into
masterfrom
jamesdemery/bb2-4961-switch-account-link

Conversation

@JamesDemeryNava

Copy link
Copy Markdown
Contributor

JIRA Ticket:
BB2-4961

What Does This PR Do?

Ensures that a beneficiary on the v3 permissions screen can click the Switch account link, and be redirected back to the CSPs landing page, where they can then re-authorize and get an access token. This works via Postman and the testclient, though some extra code was needed for it to work for the testclient.

Some minor styling changes to the v3 permissions screen as well.

What Should Reviewers Watch For?

  • Is the dot_ext/views/authorization.py file becoming too large? Maybe not a problem for this PR, but it's almost 1,500 lines now
  • Any concerns on the change in testclient/views.py: setup_testclient_http_response? Needed to make a change there so that, even after a user makes it through the testclient, that the resource links actually work

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

Validation

Styling changes:

  • Ensure that when you click into one of the accordions, that the sub-heading that appears (will end with 'if applicable:'), is now size 16 font
  • Ensure that if there is no support email on the application, that the Reviuew {{appName}}'s Privacy Policy and Terms and Conditions, appears in line with the question mark icon to the left

Switch account changes:

  • In Postman, go through a v3 auth flow. Log in as a user (BBUser00100), then click the switch account link at the top of the page

  • You will be redirected to the CSPs landing page, click Medicare.gov, and enter the credentials for BBUser00575

  • Click Share on the v3 permissions screen.

  • Run a v3 patient search call for your newly created token. If you used BBUser00575, the fhir_id_v3 should be a specific value that I can share with you (don't want to include in a PR)

  • In the testclient, go through a v3 auth flow. Log in as a user (BBUser00100), then click the switch account link at the top of the page

  • You will be redirected to the CSPs landing page, click Medicare.gov, and enter the credentials for BBUser00575

  • Click Share on the v3 permissions screen.

  • Click through every link on the testclient landing page to ensure they all work

  • Also go through testclient flows for v2/v3 without clicking switch account to ensure they still work. Do the same for Postman.

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?

None of the above apply necesarily, but @sb-benohe, curious if you have any concerns about logging a user out and then maintaining the oauth_params so another user can login.

  • 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

Comment thread apps/dot_ext/views/authorization.py Fixed
Comment thread apps/dot_ext/views/authorization.py Outdated
Comment thread apps/testclient/views.py Outdated
@bwang-icf

Copy link
Copy Markdown
Contributor

This can be followed up on in a future ticket, but I believe it would make sense to add this workflow to selenium tests.

@JamesDemeryNava

Copy link
Copy Markdown
Contributor Author

adding a get_oauth_param helper to simplify param retrieval

Definitely, I will write up a ticket for that

bwang-icf
bwang-icf previously approved these changes Jul 13, 2026

@bwang-icf bwang-icf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great job!

@annamontare-nava

Copy link
Copy Markdown
Contributor

Validation:

  • styling changes look good
  • switch account validation steps all worked

@annamontare-nava

Copy link
Copy Markdown
Contributor

Did you hear back from @sb-benohe on the security question?

@JamesDemeryNava

Copy link
Copy Markdown
Contributor Author

Did you hear back from @sb-benohe on the security question?

No

@annamontare-nava annamontare-nava left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All the validation steps worked! I have some security questions that may be non-issues due to my own misunderstanding. I also think potentially we could add more tests to this.

Comment thread apps/dot_ext/views/authorization.py
]
oauth_params = {}
for param in params:
oauth_params[param] = self.request.GET.get(param)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would this work if the request method was POST?

Comment thread apps/dot_ext/utils.py
Comment thread apps/dot_ext/views/authorization.py
Comment thread apps/dot_ext/views/authorization.py
Comment thread apps/dot_ext/utils.py


def get_oauth_param(request: HttpRequest, parameter: str, fallback_session_parameter: str = None) -> str | None:
"""Resolve an OAuth parameter from GET, then session, then session['oauth_params'].

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this handle POST too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I looked at Splunk and I swore we were only getting GET requests on authorize endpoints, which is why I wrote it as so. Looking now, I do see some POST requests, so i'll add handling for that as a backup.

Comment thread apps/testclient/views.py Outdated
Comment thread apps/testclient/views.py
Comment thread apps/testclient/views.py
Comment thread apps/testclient/views.py
Co-authored-by: annamontare-nava <267455234+annamontare-nava@users.noreply.github.com>
@JamesDemeryNava

Copy link
Copy Markdown
Contributor Author

All the validation steps worked! I have some security questions that may be non-issues due to my own misunderstanding. I also think potentially we could add more tests to this.

I'm going to write a ticket to add a Selenium test for this. What other tests do you think need to be added before this can be merged?

@annamontare-nava

Copy link
Copy Markdown
Contributor

All the validation steps worked! I have some security questions that may be non-issues due to my own misunderstanding. I also think potentially we could add more tests to this.

I'm going to write a ticket to add a Selenium test for this. What other tests do you think need to be added before this can be merged?

I think a selenium test would cover it!

'code_challenge_method': CODE_CHALLENGE_METHOD_S256,
'code_challenge': request.session.get('oauth_params', {}).get('code_challenge'),
'scope': request.session.get('oauth_params', {}).get('scope'),
'code_verifier': request.session.get('code_verifier'),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The code_verifier shouldn't be sent on the authorization request, no?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Great catch. It doesn't come through on Postman auth requests (similar to our actual apps), but it does in testclient. So that is why it's included here, so the testclient switch account flow works. I'll leave a comment to explain that.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it should be set in the session, but not placed in the authorize url parameters later on.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is there a change you are suggesting? I didn't add it to the authorize url parameters, just making it so it is retrievable after switch account is clicked for testclient.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It goes into the url parameters in line 1493 right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants