-
Notifications
You must be signed in to change notification settings - Fork 166
Fix Async Hybrid Flow (LG-4172) #4665
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
Changes from all commits
ea03664
82f1f34
470973a
17608a4
7c408b8
c502b97
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 |
|---|---|---|
|
|
@@ -22,6 +22,8 @@ def done? | |
| status == DocumentCaptureSessionAsyncResult::DONE | ||
| end | ||
|
|
||
| alias_method :success?, :done? | ||
|
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. reason this is needed: the capture doc flow expects to load a |
||
|
|
||
| def in_progress? | ||
| status == DocumentCaptureSessionAsyncResult::IN_PROGRESS | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,9 +30,13 @@ def form | |
| end | ||
|
|
||
| def enqueue_job | ||
| verify_document_capture_session = create_document_capture_session( | ||
| verify_document_capture_session_uuid_key, | ||
| ) | ||
| verify_document_capture_session = if hybrid_flow_mobile? | ||
| document_capture_session | ||
| else | ||
| create_document_capture_session( | ||
| verify_document_capture_session_uuid_key, | ||
| ) | ||
| end | ||
|
Comment on lines
+33
to
+39
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. reason this is needed: in the hybrid mobile flow, the
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. the key is only used once either way, so we don't need to worry about overwriting?
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. I believe, yes |
||
| verify_document_capture_session.requested_at = Time.zone.now | ||
| verify_document_capture_session.create_doc_auth_session | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,8 @@ class CaptureDocFlow < Flow::BaseFlow | |
|
|
||
| ACTIONS = { | ||
| reset: Idv::Actions::ResetAction, | ||
| verify_document: Idv::Actions::VerifyDocumentAction, | ||
| verify_document_status: Idv::Actions::VerifyDocumentStatusAction, | ||
|
Comment on lines
+11
to
+12
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. reason this is needed: these polling actions for async work were missing from the hybrid flow |
||
| }.freeze | ||
|
|
||
| def initialize(controller, session, _name) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,7 +31,10 @@ def take_photo_with_phone_successful? | |
| end | ||
|
|
||
| def document_capture_session_result | ||
| @document_capture_session_result ||= document_capture_session&.load_result | ||
| @document_capture_session_result ||= ( | ||
| document_capture_session&.load_result || | ||
| document_capture_session&.load_doc_auth_async_result | ||
| ) | ||
|
Comment on lines
+34
to
+37
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. allow loading data from the async flow |
||
| end | ||
|
|
||
| def mark_steps_complete | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,10 +18,10 @@ | |
| mock_client: (DocAuthRouter.doc_auth_vendor == 'mock').presence, | ||
| document_capture_session_uuid: flow_session[:document_capture_session_uuid], | ||
| endpoint: FeatureManagement.document_capture_async_uploads_enabled? ? | ||
| idv_doc_auth_step_path(step: :verify_document) : | ||
| send(@step_url, step: :verify_document) : | ||
|
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. reason this is needed: In the non-hybrid flow, we link to |
||
| api_verify_images_url, | ||
| status_endpoint: FeatureManagement.document_capture_async_uploads_enabled? ? | ||
| idv_doc_auth_step_path(step: :verify_document_status) : | ||
| send(@step_url, step: :verify_document_status) : | ||
| nil, | ||
| status_poll_interval_ms: AppConfig.env.poll_rate_for_verify_in_seconds.to_i * 1000, | ||
| sp_name: sp_name, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| require 'rails_helper' | ||
|
|
||
| describe 'Hybrid Flow' do | ||
| include IdvHelper | ||
| include DocAuthHelper | ||
|
|
||
| before do | ||
| allow(FeatureManagement).to receive(:doc_capture_polling_enabled?).and_return(true) | ||
| allow(AppConfig.env).to receive(:doc_auth_enable_presigned_s3_urls).and_return('true') | ||
| allow(AppConfig.env).to receive(:document_capture_async_uploads_enabled).and_return('true') | ||
| allow(LoginGov::Hostdata::EC2).to receive(:load). | ||
| and_return(OpenStruct.new(region: 'us-west-2', account_id: '123456789')) | ||
| end | ||
|
|
||
| it 'proofs and hands off to mobile', js: true do | ||
| user = nil | ||
| sms_link = nil | ||
|
|
||
| expect(Telephony).to receive(:send_doc_auth_link).and_wrap_original do |impl, config| | ||
| sms_link = config[:link] | ||
| impl.call(config) | ||
| end | ||
|
|
||
| perform_in_browser(:desktop) do | ||
| user = sign_in_and_2fa_user | ||
| complete_doc_auth_steps_before_send_link_step | ||
| fill_in :doc_auth_phone, with: '415-555-0199' | ||
| click_idv_continue | ||
|
|
||
| expect(page).to have_content(t('doc_auth.headings.text_message')) | ||
| end | ||
|
|
||
| expect(sms_link).to be_present | ||
|
|
||
| perform_in_browser(:mobile) do | ||
| visit sms_link | ||
| attach_and_submit_images | ||
| expect(page).to have_text(t('doc_auth.instructions.switch_back')) | ||
| end | ||
|
|
||
| perform_in_browser(:desktop) do | ||
| expect(page).to_not have_content(t('doc_auth.headings.text_message'), wait: 10) | ||
|
|
||
| fill_out_ssn_form_ok | ||
| click_idv_continue | ||
|
|
||
| expect(page).to have_content(t('doc_auth.headings.verify')) | ||
| click_idv_continue | ||
|
|
||
| fill_out_phone_form_mfa_phone(user) | ||
| click_idv_continue | ||
|
|
||
| fill_in :user_password, with: Features::SessionHelper::VALID_PASSWORD | ||
| click_idv_continue | ||
|
|
||
| acknowledge_and_confirm_personal_key | ||
|
|
||
| expect(page).to have_current_path(account_path) | ||
| expect(page).to have_content(t('headings.account.verified_account')) | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,29 +11,42 @@ | |
| let(:selfie_image_upload_url) { nil } | ||
|
|
||
| before do | ||
| allow(view).to receive(:flow_session).and_return(flow_session) | ||
| allow(view).to receive(:sp_name).and_return(sp_name) | ||
| allow(view).to receive(:failure_to_proof_url).and_return(failure_to_proof_url) | ||
| allow(view).to receive(:front_image_upload_url).and_return(front_image_upload_url) | ||
| allow(view).to receive(:back_image_upload_url).and_return(back_image_upload_url) | ||
| allow(view).to receive(:selfie_image_upload_url).and_return(selfie_image_upload_url) | ||
| allow(view).to receive(:url_for).and_return('https://example.com/') | ||
|
|
||
| allow(FeatureManagement).to receive(:document_capture_async_uploads_enabled?). | ||
| and_return(async_uploads_enabled) | ||
|
|
||
| assign(:step_url, :idv_doc_auth_step_url) | ||
|
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 is the key change in this file, now that the partial uses |
||
| end | ||
|
|
||
| subject(:render_partial) do | ||
| render partial: 'idv/shared/document_capture', locals: { | ||
| flow_session: flow_session, | ||
| sp_name: sp_name, | ||
| failure_to_proof_url: failure_to_proof_url, | ||
| front_image_upload_url: front_image_upload_url, | ||
| back_image_upload_url: back_image_upload_url, | ||
| selfie_image_upload_url: selfie_image_upload_url, | ||
| } | ||
| end | ||
|
|
||
| describe 'async upload urls' do | ||
| context 'when async upload is disabled' do | ||
| let(:async_uploads_enabled) { false } | ||
|
|
||
| it 'does not modify CSP connect_src headers' do | ||
| allow(SecureHeaders).to receive(:append_content_security_policy_directives).with(any_args) | ||
| expect(SecureHeaders).to receive(:append_content_security_policy_directives).with( | ||
| controller.request, | ||
| connect_src: [], | ||
| ) | ||
|
|
||
| render | ||
| render_partial | ||
| end | ||
| end | ||
|
|
||
| context 'when async upload is enabled' do | ||
| context 'when async upload are enabled' do | ||
| let(:async_uploads_enabled) { true } | ||
| let(:front_image_upload_url) { 'https://s3.example.com/bucket/a?X-Amz-Security-Token=UAOL2' } | ||
| let(:back_image_upload_url) { 'https://s3.example.com/bucket/b?X-Amz-Security-Token=UAOL2' } | ||
| let(:selfie_image_upload_url) { 'https://s3.example.com/bucket/c?X-Amz-Security-Token=UAOL2' } | ||
|
|
@@ -49,7 +62,7 @@ | |
| ], | ||
| ) | ||
|
|
||
| render | ||
| render_partial | ||
| end | ||
| end | ||
| end | ||
|
|
||
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.
reason this is needed: the VerifyDocumentAction stores data via
store_doc_auth_resultwhich still contains PII but has slightly different redis keys than thestore_result_from_responsethat the old flow uses