Skip to content

BB2-4961: Switch Account Link#1649

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

BB2-4961: Switch Account Link#1649
JamesDemeryNava wants to merge 14 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
'state': self.request.GET.get('state'),
'code_challenge': self.request.GET.get('code_challenge'),
'code_challenge_method': self.request.GET.get('code_challenge_method'),
}

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.

We could consider using a list here to cut down on the repeated request.get calls here. Maybe something like

params = ['client_id', 'redirect_uri', 'response_type', 'scope', 'state', 'code_challenge', 'code_challenge_method']
oauth_params = {}
for param in params:
    oauth_params[param] = self.request.GET.get(param)
self.request.session['oauth_params'] = oauth_params

Comment thread apps/testclient/views.py Outdated
def _get_oauth2_session_with_redirect(request: HttpRequest) -> OAuth2Session:
client_id = request.session['client_id']
redirect_uri = request.session['redirect_uri']
if request.session.get('client_id'):

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.

Looks like a similar retrieval pattern as from dot_ext/authorization with trying to grab a key/parameter from the session and then possibly falling back to a different key. I think a helper in utils.py that can be generalized for both spots could be helpful.

@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 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?

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