-
Notifications
You must be signed in to change notification settings - Fork 166
LG-12631: handoff ipp #10267
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
LG-12631: handoff ipp #10267
Changes from all commits
841a0f2
b26d41b
188a45d
be3faa3
2cd1977
95d23c1
b31b55b
4e23871
0b308b1
527e65a
22580ba
36d42d4
3441b4b
62063fd
0d1343d
308f3f0
d3b4557
2a11005
850a873
b83783e
e44e1ff
4dda380
2f130b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -7,7 +7,7 @@ class DocumentCaptureController < ApplicationController | |||
| include StepIndicatorConcern | ||||
|
|
||||
| before_action :confirm_not_rate_limited, except: [:update] | ||||
| before_action :confirm_step_allowed | ||||
| before_action :confirm_step_allowed, unless: -> { allow_direct_ipp? } | ||||
| before_action :override_csp_to_allow_acuant | ||||
|
|
||||
| def show | ||||
|
|
@@ -47,6 +47,7 @@ def extra_view_variables | |||
| sp_name: decorated_sp_session.sp_name, | ||||
| failure_to_proof_url: return_to_sp_failure_to_proof_url(step: 'document_capture'), | ||||
| skip_doc_auth: idv_session.skip_doc_auth, | ||||
| skip_doc_auth_from_handoff: idv_session.skip_doc_auth_from_handoff, | ||||
| opted_in_to_in_person_proofing: idv_session.opted_in_to_in_person_proofing, | ||||
| doc_auth_selfie_capture: decorated_sp_session.selfie_required?, | ||||
| }.merge( | ||||
|
|
@@ -62,6 +63,7 @@ def self.step_info | |||
| preconditions: ->(idv_session:, user:) { | ||||
| idv_session.flow_path == 'standard' && ( | ||||
| # mobile | ||||
| idv_session.skip_doc_auth_from_handoff || | ||||
| idv_session.skip_hybrid_handoff || | ||||
| idv_session.skip_doc_auth || | ||||
| !idv_session.selfie_check_required || # desktop but selfie not required | ||||
|
|
@@ -109,5 +111,22 @@ def handle_stored_result | |||
| failure(I18n.t('doc_auth.errors.general.network_error'), extra) | ||||
| end | ||||
| end | ||||
|
|
||||
| def allow_direct_ipp? | ||||
| return false unless idv_session.welcome_visited && | ||||
| idv_session.idv_consent_given | ||||
| # not allowed when no step param and action:show(get request) | ||||
| return false if params[:step].blank? || params[:action].to_s != 'show' || | ||||
| idv_session.flow_path == 'hybrid' | ||||
| # Only allow direct access to document capture if IPP available | ||||
| return false unless IdentityConfig.store.in_person_doc_auth_button_enabled && | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍🏻 |
||||
| Idv::InPersonConfig.enabled_for_issuer?(decorated_sp_session.sp_issuer) | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For posterity (you're the one that shared this, just want to note it here as well): that first line is due to https://gsa-tts.slack.com/archives/C5QUGUANN/p1689380596444809?thread_ts=1689376275.536679&cid=C5QUGUANN — it's a rarely-used shutoff valve. We have not been using
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I chatted with @JackRyan1989 a bit about this, and filed LG-12768 for us to update our code to use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @n1zyy , i used this condition based on like in the frontend
Here I did not add
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @n1zyy is there existing documentation about
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eileen-nava I don't think there is (yet). :( For want of a better solution, I left comments here as little breadcrumbs. I'm still not quite sure what the best way to capture this information is. Do you have a better sense than I do of where we could capture this knowledge?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @n1zyy Since the knowledge that needs to be documented is Team Joy domain, I will leave the decision up to you of where to document it. |
||||
| @previous_step_url = params[:step] == 'hybrid_handoff' ? idv_hybrid_handoff_path : nil | ||||
| # allow | ||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Helpful comment, I didn't wrap my head around this at first. |
||||
| idv_session.flow_path = 'standard' | ||||
| idv_session.skip_doc_auth_from_handoff = true | ||||
| idv_session.skip_hybrid_handoff = nil | ||||
| true | ||||
| end | ||||
| end | ||||
| end | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,11 @@ def show | |
| @upload_disabled = idv_session.selfie_check_required && | ||
| !idv_session.desktop_selfie_test_mode_enabled? | ||
|
|
||
| @direct_ipp_with_selfie_enabled = IdentityConfig.store.in_person_doc_auth_button_enabled && | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍🏻 |
||
| Idv::InPersonConfig.enabled_for_issuer?( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I may very much be wrong here so please poke back- vacation brain. I am wondering if
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gina-yamada , that
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This raises the question should we push
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dawei-nava looks like we maybe don't want to since that is a global configuration value independent of the service provider? See link |
||
| decorated_sp_session.sp_issuer, | ||
| ) | ||
|
|
||
| @selfie_required = idv_session.selfie_check_required | ||
|
|
||
| analytics.idv_doc_auth_hybrid_handoff_visited(**analytics_arguments) | ||
|
|
@@ -22,6 +27,8 @@ def show | |
| true | ||
| ) | ||
|
|
||
| # reset if we visit or come back | ||
| idv_session.skip_doc_auth_from_handoff = nil | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm checking my own understanding: why do we need to reset
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @eileen-nava , yes you are correct, it's kind of non-reentrant ticket, once you exit the venue, you have to get a ticket again. |
||
| render :show, locals: extra_view_variables | ||
| end | ||
|
|
||
|
|
@@ -55,7 +62,9 @@ def self.step_info | |
| next_steps: [:link_sent, :document_capture], | ||
| preconditions: ->(idv_session:, user:) { | ||
| idv_session.idv_consent_given && | ||
| self.selected_remote(idv_session: idv_session) | ||
| (self.selected_remote(idv_session: idv_session) || # from opt-in screen | ||
| # back from ipp doc capture screen | ||
| idv_session.skip_doc_auth_from_handoff) | ||
| }, | ||
| undo_step: ->(idv_session:, user:) do | ||
| idv_session.flow_path = nil | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,5 +9,6 @@ | |
| acuant_version: acuant_version, | ||
| opted_in_to_in_person_proofing: opted_in_to_in_person_proofing, | ||
| skip_doc_auth: skip_doc_auth, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @n1zyy I vote for opening a ticket to rename
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @n1zyy and @JackRyan1989 ... I also strongly vote to rename When reviewing this PR, I was confused and really had to dig around in the code to understand the goal of is there a ticket open that I can follow? Thanks 🙏🏿
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @amirbey hot off the presses: https://cm-jira.usa.gov/browse/LG-13006 |
||
| skip_doc_auth_from_handoff: skip_doc_auth_from_handoff, | ||
| doc_auth_selfie_capture: doc_auth_selfie_capture, | ||
| ) %> | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this does not enforce that welcome and agreement pages were completed if IPP is enabled? 🤔
perhaps we should make this less lax?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amirbey, good suggestions, i was thinking add checking the referer, but this sounds better. Added check and tests.