Skip to content

LG-6959 IDV verify redirects to inherited proofing#6896

Merged
jscodefix merged 2 commits intomainfrom
LG-6959-verify-redirect-to-inherited-proofing
Sep 6, 2022
Merged

LG-6959 IDV verify redirects to inherited proofing#6896
jscodefix merged 2 commits intomainfrom
LG-6959-verify-redirect-to-inherited-proofing

Conversation

@jscodefix
Copy link
Contributor

When a VA user completes authorization through openid_connect, they are redirected to inherited proofing verification.

changelog: Upcoming Features, Inherited proofing, verification step redirects to inherited proofing

Refer to the upper-right portion of this diagram: https://www.figma.com/file/Lj6tqLhohJMTR4IvvoU2BI/VA-Inherited-Proofing-Architecture-Diagram

When a VA user completes authorization through openid_connect, they are redirected to inherited proofing verification.

changelog: Upcoming Features, Inherited proofing, verification step redirects to inherited proofing
@jscodefix jscodefix added the inherited proofing Pull Requests for the Inherited Proofing feature label Sep 1, 2022
def verify_identity
analytics.idv_intro_visit
redirect_to idv_doc_auth_url
if va_inherited_proofing?
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong preference if it lives here vs. DocAuthController, but worth pointing out that there are similar redirects that happen there for GPO (verify by mail) verification and in-person proofing enrollment.

before_action :redirect_if_pending_profile
before_action :redirect_if_pending_in_person_enrollment

It does raise a question of whether we'd prefer that the user be forced into the inherited proofing flow if they attempted to navigate directly to the traditional unsupervised proofing flow (i.e. navigated to /verify/doc_auth).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, that there are many redirect conditions in the OpenidConnect::Auth, Idv, and DocAuth controllers. It seems odd that we would redirect to inherited proofing (IP) in DocAuth, esp. since we know IP will be required of the user when they first hit the Openid controller (unless, there is required logic in DocAuth?). Does it make sense that DocAuth is a default, or that the user is somehow directed to /verify/doc_auth (apart from Idv)?

Copy link
Contributor

Choose a reason for hiding this comment

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

@aduth If we do not force them into the IP flow, that may very well mess with the analytics/metrics we want to capture regarding whether or not the user is able to traverse the whole IP process successfully. However, what you bring up may also provide equally valuable analytics/metrics if the user does in fact decide (for whatever reason) to abandon the IP flow and (for example) go with the unsupervised flow - was it because they got lost, did they become frustrated, were they unable to complete due to errors, etc.? Good question to raise.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jscodefix Everything comes through this controller, but since this was previously redirecting everything to DocAuthController, it was a de facto default entrypoint for identity verification. I'm not familiar enough with inherited proofing to know if it's different enough to make it awkward to try to include there, vs. separately (as you've done here).

I think this could be fine, if:

  • We think inherited proofing is different enough from the traditional identity verification flow
  • We're okay with the idv_intro_visit analytics being logged
  • We're okay that someone could still find their way into the DocAuth flow
    • I wouldn't think this is common, but someone could manually edit the URL, or visit from a different partner requiring identity verification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aduth Responding to your 3 points above:

  1. To me, IP verification is significantly different than DocAuth; their PII is pulled from the SP via an API call.
  2. I don't know the intent of the (idv_intro_visit) analytics, but still seems applicable here. I'll add the question to Jira and discuss with the team.
  3. If an IP user doesn't arrive through the OIDC controller, then IP will not work.

@jscodefix jscodefix requested a review from jmhooper September 1, 2022 16:54
@jscodefix
Copy link
Contributor Author

Pasting here, what I had posted in my Slack message... some concerns:

  • that (by my implementation) we're using a request_url with auth_code (found in the decorated session) to control redirection (to inherited proofing) instead of using the (VA) ServiceProvider object.

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

With the caveat that I'm not as familiar with inherited proofing, this looks good to me.

To your question about using the auth code, I'll leave that question to you, though I think from the point-of-view of the controller, calling a method va_inherited_proofing? on the inherited proofing concern seems to be the right approach. I'm not as sure on the implementation details of how va_inherited_proofing? considers request or query parameters.

Comment on lines +58 to +71
it 'redirects to inherited proofing if an auth code parameter is included in request URL' do
session_service_provider = create(:service_provider)
users_auth_code = 'any_auth_code'
request_url = "https://login.gov/openid_connect/authorize?inherited_proofing_auth=#{users_auth_code}"

allow_any_instance_of(ApplicationController).to receive(:current_sp).
and_return(session_service_provider)
stub_sign_in
session[:sp] = { request_url: request_url }

get :index

expect(response).to redirect_to idv_inherited_proofing_path
end
Copy link
Contributor

Choose a reason for hiding this comment

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

This may come down to personal testing philosophies, but to me this seems pretty tightly coupled with the specific, current implementation of va_inherited_proofing?, where I might leave those considerations to the specs of the concern itself, and instead stub the expected return values here.

Suggested change
it 'redirects to inherited proofing if an auth code parameter is included in request URL' do
session_service_provider = create(:service_provider)
users_auth_code = 'any_auth_code'
request_url = "https://login.gov/openid_connect/authorize?inherited_proofing_auth=#{users_auth_code}"
allow_any_instance_of(ApplicationController).to receive(:current_sp).
and_return(session_service_provider)
stub_sign_in
session[:sp] = { request_url: request_url }
get :index
expect(response).to redirect_to idv_inherited_proofing_path
end
context 'with a va inherited proofing session' do
before do
stub_sign_in
allow(controller).to receive(:va_inherited_proofing?).and_return(true)
end
it 'redirects to inherited proofing' do
get :index
expect(response).to redirect_to idv_inherited_proofing_path
end
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Andrew, thanks for the sage advice. Yes, I agree (after you pointed it out), the test shouldn't be coupled to the concerns logic; but I did need to see (temporarily) that ApplicationController had access to the DecoratedSession, that the InheritedProofingConcern depends upon; I'll use your version of the test. Now, to backfill the missing concern tests. :-/

Copy link
Contributor Author

@jscodefix jscodefix Sep 2, 2022

Choose a reason for hiding this comment

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

Regarding use of the auth_code within the request_url, I may have an issue with how we've conflated inherited proofing with VA inherited proofing. What will happen when we next have a new service provider XYZ that sends an auth_code, will we direct them to the VA inherited proofing? Seems we should be checking the ServiceProvider object. We can cross that bridge when we get to the bridge. :-)

@jscodefix
Copy link
Contributor Author

Upon correctly configuring and running the identity-oidc-sinatra app, I was able to successfully show (in my dev environment) that the redirect is properly happening.

@jscodefix jscodefix merged commit ec76046 into main Sep 6, 2022
@jscodefix jscodefix deleted the LG-6959-verify-redirect-to-inherited-proofing branch September 6, 2022 16:29
@zachmargolis zachmargolis mentioned this pull request Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inherited proofing Pull Requests for the Inherited Proofing feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants