LG-9065 Write an exception result when TMx has an invalid review status#8149
Merged
LG-9065 Write an exception result when TMx has an invalid review status#8149
Conversation
jmhooper
commented
Apr 6, 2023
zachmargolis
reviewed
Apr 6, 2023
Comment on lines
339
to
340
Contributor
There was a problem hiding this comment.
good candidate for .dig?
Suggested change
| result_context = result[:context] | |
| result_context_stages = result_context[:stages] | |
| result_context_stages_threatmetrix = result_context_stages[:threatmetrix] | |
| result_context_stages_threatmetrix = result.dig(:context, :stages, :threatmetrix) |
Contributor
Author
There was a problem hiding this comment.
I went with this approach because it matches a pattern used in other test examples in this file.
Examples:
My read is that the pattern's purpose is to document which part of the large results context hash these values are coming from. We can change this spec but that will leave it an oddball.
kbighorse
reviewed
Apr 6, 2023
Contributor
There was a problem hiding this comment.
[question] is this "unexpected" in the sense that truly nothing should trip this or "unexpected" in the sense that something expected could trip this but only inappropriately? (thinking explicitly of the no_result that we previously thought could happen)
Contributor
Author
There was a problem hiding this comment.
We can consider changing the text here. Do you have a suggestion for an improved message?
soniaconnolly
approved these changes
Apr 6, 2023
Contributor
soniaconnolly
left a comment
soniaconnolly
left a comment
There was a problem hiding this comment.
LGTM, with a test suggestion and I second Zach's dig suggestion.
Contributor
There was a problem hiding this comment.
Would it work to create a constant BAD_REVIEW_STATUS = 'unexpected_review_status_that_causes_problems' and then use it wherever it comes up in the tests?
Contributor
Author
There was a problem hiding this comment.
Can you help me understand the value a constant would provide? The value is always used shortly after it is set. Subbing in a constant doesn't seem like it helps readability. As a constant the name may need to be even longer than this in order to disambiguate. Especially if it is going to be defined outside of the tests where it is used.
Contributor
Author
There was a problem hiding this comment.
Check out ae9369e and see if that helps to alleviate some of the concern.
Contributor
There was a problem hiding this comment.
Yes, that works. My thought was to make it easier to see that the value being set and the value being compared against were the same.
We use ThreatMetrix as a tool for device profiling. It gives us a `review_status` result which we store in the database as a proofing component. That proofing component is used later in the process to determine if a profile needs additional review. We communicate with ThreatMetrix when we are running the `ResolutionProofingJob` if device profiling is enabled or in collect only mode. We do not communicate with threatMetrix when device profiling is disabled. Previously, we would write `nil` to the proofing component if we received that or an unexpected review status from ThreatMetrix. This leads to ambiguity later in the process when determining if a user passed the ThreatMetrix check. Specifically, `nil` could mean the ThreatMetrix did not run _or_ it could mean that an error occured and no `review_status` was provided. This commit changes the behavior of ThreatMetrix such that if ThreatMetrix is enabled it will write a review status or the job will respond with an exception result. This exception result will result in the user seeing an error and needing to re-submit. This resolves the ambiguity by making `nil` mean that ThreatMetrix was not enabled. [skip changelog]
jmhooper
force-pushed
the
jmhooper-tmx-exception
branch
from
ae9369e to
0c49de2
Compare
mitchellhenke
pushed a commit
that referenced
this pull request
Apr 7, 2023
…us (#8149) We use ThreatMetrix as a tool for device profiling. It gives us a review_status result which we store in the database as a proofing component. That proofing component is used later in the process to determine if a profile needs additional review. We communicate with ThreatMetrix if device profiling is enabled or in collect only mode. We do not communicate with threatMetrix when device profiling is disabled. Previously, we would write nil to the proofing component if we received that or an unexpected review status from ThreatMetrix. This leads to ambiguity later in the process when determining if a user passed the ThreatMetrix check. Specifically, nil could mean the ThreatMetrix did not run because device profiling is disabled or it could mean that an error occurred and no review_status was provided. This commit changes the behavior of ThreatMetrix such that if ThreatMetrix is enabled it will write a review status or the job will respond with an exception result. This exception result will result in the user seeing an error and needing to re-submit and re-run the job. This resolves the ambiguity by making nil mean that ThreatMetrix was not enabled. [skip changelog]
jmhooper
added a commit
that referenced
this pull request
Apr 10, 2023
…g the result hash (#8160) Prior to this commit the majority of our proofers returned an object with a #to_h method that returned a hash suitable for logging with the results of the proofing transaction. The exception was the DDP proofer. The DDP proofer's logged result needed to be constructed in the proofing job where it was invoked. This commit makes the DDP result mirror the other result classes. This will make it easier to reconstruct the DDP class to add logic around things like whether exceptions are present (see #8149). [skip changelog]
solipet
added a commit
that referenced
this pull request
Apr 11, 2023
* LG-9065 Write an exception result when TMx has an invalid review status (#8149) We use ThreatMetrix as a tool for device profiling. It gives us a review_status result which we store in the database as a proofing component. That proofing component is used later in the process to determine if a profile needs additional review. We communicate with ThreatMetrix if device profiling is enabled or in collect only mode. We do not communicate with threatMetrix when device profiling is disabled. Previously, we would write nil to the proofing component if we received that or an unexpected review status from ThreatMetrix. This leads to ambiguity later in the process when determining if a user passed the ThreatMetrix check. Specifically, nil could mean the ThreatMetrix did not run because device profiling is disabled or it could mean that an error occurred and no review_status was provided. This commit changes the behavior of ThreatMetrix such that if ThreatMetrix is enabled it will write a review status or the job will respond with an exception result. This exception result will result in the user seeing an error and needing to re-submit and re-run the job. This resolves the ambiguity by making nil mean that ThreatMetrix was not enabled. [skip changelog] * Return an empty hash from `#flow_session` if it has not been created (#8153) * Return an empty hash from `#flow_session` if it has not been created We have observed a number of 500 errors that are the result of the flow session being nil. There is a new one present in the document capture controller. This commit fixes all of these issues and hopefully prevents new ones by modifying the non-FSM implementaitons of `#flow_session` to return an empty hash when the flow session has not been constructed yet. [skip changelog] * fix a test * Maintain request ID in password reset (#8145) * Maintain request ID in password reset changelog: Bug Fixes, Password Reset, Maintain partner request when user resets password in new session * Use branded experience assertion consistently These assertions are prone to false positives because the footer GSA logo includes the sp-logos path * Use perform_in_browser helper to initiate password reset * Clarify branded experience assertion * Move request_id handling from password controller So that it isn't persisted only in the case of a successful password submission, because otherwise the associated SP is dropped * Simplify parameter access * Add specs for SP metadata store * Store passed AAL as text to preserve all AAL details (#8012) * changelog: Bug Fix, OpenID Connect, Update AAL in userinfo response to keep specific values Store passed AAL as text to store extra information * Update AAL1 tests * change AAL1 tests * update migration * update specs --------- Co-authored-by: Mitchell Henke <mitchell.henke@gsa.gov> * Link to idv_address_url from verify_info show template (#8157) We no longer need to use the RedoAddressAction in the flow state machine to link from the VerifyInfo screen to the Address screen in the doc_auth flow. This is in preparation for deleting the RedoAddressAction. changelog: Internal, Verify Info step, Stop using RedoAddressAction and link directly to Address page to update address * LG-9065 Make the DDP result class and proofer responsible for building the result hash (#8160) Prior to this commit the majority of our proofers returned an object with a #to_h method that returned a hash suitable for logging with the results of the proofing transaction. The exception was the DDP proofer. The DDP proofer's logged result needed to be constructed in the proofing job where it was invoked. This commit makes the DDP result mirror the other result classes. This will make it easier to reconstruct the DDP class to add logic around things like whether exceptions are present (see #8149). [skip changelog] * LG-9426: Update mock DDP proofer results to match the real one (#8155) * Allow specifying fixture file for DDP Mock Proofer * Add review_status error to mock DDP proofer If review status was not pass, add an error * Record request_result error on mock ddp proofer * Cover the 'review' status in DDP mock proofer Not used in prod currently, but _could be_ * Add extra check to pass case * changelog: Internal, Fraud monitoring, Update mock DDP proofer to include errors like the real proofer * LG-9141 Skip address page if same address as ID is true (#8151) * round of changes complete * Adding and modifying tests * changelog: User-Facing Improvements, In-Person Proofing, Skip address page if same address as ID * Linting * update tests and state id spec to change jurisdiction * Missed jurisdiction in test * last jurisdiction * undoing call to mark address incomplete * Remove unused EmailSent step (#8158) * Remove EmailSentStep and associated references EmailSentStep is no longer referenced. It allowed users on mobile to receive an email and continue uploading photos of their ID on desktop. [skip changelog] * Small refactor in UploadStep * DocAuthLog and RegisterStep cleanup Mark email_sent columns as ignored Remove unused send_link token and mark those columns as ignored * Remove doc_auth_desktop_link_to_sp mailer * Remove unused doc_auth_link translations and mailer template * remove unused image state-id-confirm@3x.png * Update documentation to remove references to design.login.gov (#8159) * Update documentation to remove references to design.login.gov * Fix Markdown syntax * Add changelog changelog: Internal, Documentation, Update documentation to remove references to Design System documentation * Fix flaky IdV outage spec (#8168) * Fix Idv outage spec cleanup process Ensure config-related stubs are torn down before calling `Rails.application.reload_routes!` [skip changelog] * Update spec/features/idv/outage_spec.rb Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov> * Appease linter --------- Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov> * LG-9103: capture error when ArcGIS service is not returning a token (#8087) * LG-9301 capture error and alert when ArcGIS service is not returning a token If ArcGIS is unable to return a token it was causing an exception in retrieve_token and throwing HTTP 500 responses. The UI had no indication of error and the responses were causing the on-call engineer to be notified. This change catches the missing token, logs the issue, and returns a 400 error which allows a retry. changelog: Internal, PO-Search, Return 400 instead of 500 and logs ArcGIS token issues * LG-9103 (erroneously noted 9301 previously) Remove existing token from cache if service is unavailable or creditials are invalid. changelog: Internal, In-person-proofing address lookup, Prevent http 500 errors from occurring with invalid token * LG-9103 Capture error and alert when ArcGIS is not returning a token Add two tests, one for no service and one for invalid credentials (no token returned) * Add safer parsing of dynamic SAML urls (LG-8837) (#8166) * Add safer parsing of dynamic SAML urls (LG-8837) * Add additional spec coverage - Pulls ideas from #8105 changelog: Internal, Source code, Add stricter checking of URLs * LG-9216: A/B test for tabbed Sign In view (#8112) * LG-9216: A/B test for tabbed Sign In view changelog: Upcoming Features, Sign In Experience, Implement A/B test for tabbed Sign In view * Add specs for TabNavigationComponent * AddTabNavigationComponent preview * Add translations for headings * Rename ivar for sign in bucket See: https://github.com/18F/identity-idp/pull/8112/files#r1154971724 Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> * Switch to session ID for bucket discriminator May be more reliable than IP if we're expecting to log values in later events * Add aria-current to current link * Rename ivar for consistency See: https://github.com/18F/identity-idp/pull/8112/files#r1159978157 * Enhance TabNavigationComponent to handle query parameters * Add request_id to tab navigation links * Include bucket in visit analytics * Include all options in config default Clearer usage * Track registration visit from homepage * Add analytics property for completions event * Fix new session view link parameter assertion * Add specs for devise/sessions/new.html.erb * Add specs for sign_up/registrations/new.html.erb * Normalize YAML * Fix specs to check for new parameter * Add approved label for tab navigation * Fix syntax error --------- Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> * Remove request_id parameters from Sign In view links (#8137) * Remove request_id parameters from Sign In view links They're unnecessary, since the request_id will have already been saved to the session * changelog: Internal, Code Quality, Simplify markup for Sign In page * Refactor RegistrationsController to use session request ID * Use only session value for request ID Not intending to support to provide the request_id at the "Create an account" URL, this existed previously only as a carry over from the original implementation which did not persist the request ID from the initial visit to the Sign In page. * Revert RegisterUserEmailForm to supply request_id by submission Since it's not needed as a form parameter anymore * WIP remove more request_id * Avoid storing request_id in SessionsController This should already be stored at this point via SamlIdpAuthConcern#store_saml_request / OpenidConnect::AuthorizationController#store_request * Restore request_id parameter handling in SessionController 1. we rely on it for certain behaviors like timeout 2. there's at least safeguards to prevent double-storing metadata if it already exists in the session * Remove request_id from "Use a different email" link * Use request_id direct from SP session Save a Redis read * Remove request_id parameter from A/B tabbed llinks * Refactor implicit calculation of two-factor authentication methods (#8172) * Refactor implicit calculation of two-factor authentication methods changelog: Internal, Two-Factor Authentication, Refactor implicit calculation of two-factor authentication methods * use keyword argument for next_url argument * Remove phone_confirmed column from profiles table (#8175) * Remove phone_confirmed column from profiles table * Remove `ignored_columns` for column to be deleted changelog: Internal, Database Maintenance, remove unused column --------- Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov> Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> Co-authored-by: Jeremy Curcio <jeremy.curcio@gsa.gov> Co-authored-by: Mitchell Henke <mitchell.henke@gsa.gov> Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov> Co-authored-by: Matt Hinz <matt.hinz@gsa.gov> Co-authored-by: Jack Ryan <jackryan@navapbc.com> Co-authored-by: Micah <micah_neveu@hotmail.com> Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> Co-authored-by: Kimball Bighorse <kimball.bighorse@gsa.gov>
racingspider
added a commit
that referenced
this pull request
Apr 12, 2023
* Stop writing data to reports.log (LG-9415) (#8143) * Stop writing data to reports.log (LG-9415) changelog: Internal, Reporting, Remove unused data logging * Remove unused StoreSpMetadataInSession#event_attributes (#8140) changelog: Internal, Code Quality, Remove unused code * Update knapsack report (#8144) - Uses artifact from 69071c2 changelog: Internal, CI, Update knapsack report for spec timing * LG-9398 Add Fraud Review / Rejection Timestamp Columns (#8142) * Adding initial db migrations for fraud timestamp fields * Updating Profile model methods to use fraud timestamp columns * Add Profile#fraud_rejection? and #fraud_review_at? * Add references to fraud_rejection_at in specs * Use new timestamp columns in profile_spec Co-authored-by: Alex Bradley <alexander.bradley@gsa.gov> Co-authored-by: John Maxwell <john.maxwell@gsa.gov> Co-authored-by: John Skinner <john.skinner@gsa.gov> Co-authored-by: Amir Reavis-Bey <amir.reavis-bey@gsa.gov> Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov> * Friday test hacking/fix phone rate limiting test (#8116) * Fixed phone number throttling test This test failed because it sometimes said '9 minutes' instead of '10 minutes' We removed `freeze_time`, and changed the test to just check for one of the two possibilities. We don't understand why `freeze_time` wasn't working, but it was simple enough to fix the problem without it. * LG-9237: Collect issuing state on state id page (#8121) * add issuing state dropdown and feature test * display issuing state on verify info page * changelog: Upcoming Features, In-person proofing, Collect issuing state on state id page * correct spec expectation * fix issuing state dropdown to link to state_id_jurisdiction * display correct issuing state on verify page * fix pii attributes comments * add state_id_state to encryptor * respond to feedback * LG-9237: Send USPS the state instead of jurisdiction from state ID (#8131) [skip changelog] * Add a total users across all SPs to user count reports (LG-9408) (#8135) * Update sp-user-counts-report to count users as either IAL1 or IAL2 * Update sp-active-user-counts to count users as either IAL1 and IAL2, not both * Switch to nil issuer instead of LOGIN_ALL - Minimize chances of a colliding with actual issuer in the future changelog: Internal, Reporting, Add total count acros all SP to user reports * LG-9238: Test that state id jurisdiction is sent to aamva (#8138) * test that state id jurisdiction is sent to aamva * changelog: Internal, In-person proofing, test state id jurisdiction is sent to aamva * refactor test to use nokogiri * Fix error when verifying nil backup code (#8148) * Fix error when verifying nil backup code changelog: Bug Fixes, Backup Codes, Fix error when verifying nil backup code * Update app/services/backup_code_generator.rb Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> --------- Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> * Avoid writing .irb_history on deployed boxes (#8147) **Why**: avoids permission error changelog: Internal, Configuration, Stop recording IRB console history * LG-9237: Clean up state id translations and legend text (#8150) * fix translations for issuing state dropdown * attempt to make legend larger * make legend larger * changelog: Bug Fixes, In-person proofing, fix translations and heading for state id page * tweak heading * Fix 500 error on phone verification warning screen (#8152) * Guard against previous_phone_step_params being nil Prevent a 500 when the user visits the page directly. changelog: Bug Fixes, Identity verification, Prevent a 500 error on the phone warning screen. * Don't render "You entered:" line when phone not present * LG-9065 Write an exception result when TMx has an invalid review status (#8149) We use ThreatMetrix as a tool for device profiling. It gives us a review_status result which we store in the database as a proofing component. That proofing component is used later in the process to determine if a profile needs additional review. We communicate with ThreatMetrix if device profiling is enabled or in collect only mode. We do not communicate with threatMetrix when device profiling is disabled. Previously, we would write nil to the proofing component if we received that or an unexpected review status from ThreatMetrix. This leads to ambiguity later in the process when determining if a user passed the ThreatMetrix check. Specifically, nil could mean the ThreatMetrix did not run because device profiling is disabled or it could mean that an error occurred and no review_status was provided. This commit changes the behavior of ThreatMetrix such that if ThreatMetrix is enabled it will write a review status or the job will respond with an exception result. This exception result will result in the user seeing an error and needing to re-submit and re-run the job. This resolves the ambiguity by making nil mean that ThreatMetrix was not enabled. [skip changelog] * Return an empty hash from `#flow_session` if it has not been created (#8153) * Return an empty hash from `#flow_session` if it has not been created We have observed a number of 500 errors that are the result of the flow session being nil. There is a new one present in the document capture controller. This commit fixes all of these issues and hopefully prevents new ones by modifying the non-FSM implementaitons of `#flow_session` to return an empty hash when the flow session has not been constructed yet. [skip changelog] * fix a test * Maintain request ID in password reset (#8145) * Maintain request ID in password reset changelog: Bug Fixes, Password Reset, Maintain partner request when user resets password in new session * Use branded experience assertion consistently These assertions are prone to false positives because the footer GSA logo includes the sp-logos path * Use perform_in_browser helper to initiate password reset * Clarify branded experience assertion * Move request_id handling from password controller So that it isn't persisted only in the case of a successful password submission, because otherwise the associated SP is dropped * Simplify parameter access * Add specs for SP metadata store * Store passed AAL as text to preserve all AAL details (#8012) * changelog: Bug Fix, OpenID Connect, Update AAL in userinfo response to keep specific values Store passed AAL as text to store extra information * Update AAL1 tests * change AAL1 tests * update migration * update specs --------- Co-authored-by: Mitchell Henke <mitchell.henke@gsa.gov> * Link to idv_address_url from verify_info show template (#8157) We no longer need to use the RedoAddressAction in the flow state machine to link from the VerifyInfo screen to the Address screen in the doc_auth flow. This is in preparation for deleting the RedoAddressAction. changelog: Internal, Verify Info step, Stop using RedoAddressAction and link directly to Address page to update address * LG-9065 Make the DDP result class and proofer responsible for building the result hash (#8160) Prior to this commit the majority of our proofers returned an object with a #to_h method that returned a hash suitable for logging with the results of the proofing transaction. The exception was the DDP proofer. The DDP proofer's logged result needed to be constructed in the proofing job where it was invoked. This commit makes the DDP result mirror the other result classes. This will make it easier to reconstruct the DDP class to add logic around things like whether exceptions are present (see #8149). [skip changelog] * LG-9426: Update mock DDP proofer results to match the real one (#8155) * Allow specifying fixture file for DDP Mock Proofer * Add review_status error to mock DDP proofer If review status was not pass, add an error * Record request_result error on mock ddp proofer * Cover the 'review' status in DDP mock proofer Not used in prod currently, but _could be_ * Add extra check to pass case * changelog: Internal, Fraud monitoring, Update mock DDP proofer to include errors like the real proofer * LG-9141 Skip address page if same address as ID is true (#8151) * round of changes complete * Adding and modifying tests * changelog: User-Facing Improvements, In-Person Proofing, Skip address page if same address as ID * Linting * update tests and state id spec to change jurisdiction * Missed jurisdiction in test * last jurisdiction * undoing call to mark address incomplete * Remove unused EmailSent step (#8158) * Remove EmailSentStep and associated references EmailSentStep is no longer referenced. It allowed users on mobile to receive an email and continue uploading photos of their ID on desktop. [skip changelog] * Small refactor in UploadStep * DocAuthLog and RegisterStep cleanup Mark email_sent columns as ignored Remove unused send_link token and mark those columns as ignored * Remove doc_auth_desktop_link_to_sp mailer * Remove unused doc_auth_link translations and mailer template * remove unused image state-id-confirm@3x.png * Update documentation to remove references to design.login.gov (#8159) * Update documentation to remove references to design.login.gov * Fix Markdown syntax * Add changelog changelog: Internal, Documentation, Update documentation to remove references to Design System documentation * Fix flaky IdV outage spec (#8168) * Fix Idv outage spec cleanup process Ensure config-related stubs are torn down before calling `Rails.application.reload_routes!` [skip changelog] * Update spec/features/idv/outage_spec.rb Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov> * Appease linter --------- Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov> * LG-9103: capture error when ArcGIS service is not returning a token (#8087) * LG-9301 capture error and alert when ArcGIS service is not returning a token If ArcGIS is unable to return a token it was causing an exception in retrieve_token and throwing HTTP 500 responses. The UI had no indication of error and the responses were causing the on-call engineer to be notified. This change catches the missing token, logs the issue, and returns a 400 error which allows a retry. changelog: Internal, PO-Search, Return 400 instead of 500 and logs ArcGIS token issues * LG-9103 (erroneously noted 9301 previously) Remove existing token from cache if service is unavailable or creditials are invalid. changelog: Internal, In-person-proofing address lookup, Prevent http 500 errors from occurring with invalid token * LG-9103 Capture error and alert when ArcGIS is not returning a token Add two tests, one for no service and one for invalid credentials (no token returned) * Add safer parsing of dynamic SAML urls (LG-8837) (#8166) * Add safer parsing of dynamic SAML urls (LG-8837) * Add additional spec coverage - Pulls ideas from #8105 changelog: Internal, Source code, Add stricter checking of URLs * LG-9216: A/B test for tabbed Sign In view (#8112) * LG-9216: A/B test for tabbed Sign In view changelog: Upcoming Features, Sign In Experience, Implement A/B test for tabbed Sign In view * Add specs for TabNavigationComponent * AddTabNavigationComponent preview * Add translations for headings * Rename ivar for sign in bucket See: https://github.com/18F/identity-idp/pull/8112/files#r1154971724 Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> * Switch to session ID for bucket discriminator May be more reliable than IP if we're expecting to log values in later events * Add aria-current to current link * Rename ivar for consistency See: https://github.com/18F/identity-idp/pull/8112/files#r1159978157 * Enhance TabNavigationComponent to handle query parameters * Add request_id to tab navigation links * Include bucket in visit analytics * Include all options in config default Clearer usage * Track registration visit from homepage * Add analytics property for completions event * Fix new session view link parameter assertion * Add specs for devise/sessions/new.html.erb * Add specs for sign_up/registrations/new.html.erb * Normalize YAML * Fix specs to check for new parameter * Add approved label for tab navigation * Fix syntax error --------- Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> * Remove request_id parameters from Sign In view links (#8137) * Remove request_id parameters from Sign In view links They're unnecessary, since the request_id will have already been saved to the session * changelog: Internal, Code Quality, Simplify markup for Sign In page * Refactor RegistrationsController to use session request ID * Use only session value for request ID Not intending to support to provide the request_id at the "Create an account" URL, this existed previously only as a carry over from the original implementation which did not persist the request ID from the initial visit to the Sign In page. * Revert RegisterUserEmailForm to supply request_id by submission Since it's not needed as a form parameter anymore * WIP remove more request_id * Avoid storing request_id in SessionsController This should already be stored at this point via SamlIdpAuthConcern#store_saml_request / OpenidConnect::AuthorizationController#store_request * Restore request_id parameter handling in SessionController 1. we rely on it for certain behaviors like timeout 2. there's at least safeguards to prevent double-storing metadata if it already exists in the session * Remove request_id from "Use a different email" link * Use request_id direct from SP session Save a Redis read * Remove request_id parameter from A/B tabbed llinks * Refactor implicit calculation of two-factor authentication methods (#8172) * Refactor implicit calculation of two-factor authentication methods changelog: Internal, Two-Factor Authentication, Refactor implicit calculation of two-factor authentication methods * use keyword argument for next_url argument * Remove phone_confirmed column from profiles table (#8175) * Remove phone_confirmed column from profiles table * Remove `ignored_columns` for column to be deleted changelog: Internal, Database Maintenance, remove unused column * Revert "Add safer parsing of dynamic SAML urls (LG-8837) (#8166)" (#8181) This reverts commit 111dfd9. * Remove implicit two_factor_authentication_method in favor of explicitly passing the authentication method name (#8178) * Remove implicit two_factor_authentication_method in favor of explicitly passing the authentication method name changelog: Internal, Authentication, Remove implicit two_factor_authentication_method in favor of explicitly passing the authentication method name * use keyword argument in handle_valid_otp_for_context * Remove unused RedoAddressAction (#8165) This action was used by the VerifyInfo step in the Flow State Machine to update an address. The VerifyInfoController can redirect to the update address page directly, so this action is no longer needed. changelog: Internal, Flow State Machine extraction, remove unused RedoAddressAction * Update nokogiri to patch reported vulnerability (#8184) changelog: Internal, Security, Update nokogiri to patch reported vulnerability * Temporarily skip IdV outage spec (#8185) This spec is currently very flaky in CI, skipping it temporarily to unblock other teams. [skip changelog] * Lg 9399 light refactor of profile and spec (#8170) * Add line breaks * Add line breaks * Remove debug statement * Create/build objects via association changelog: Internal, Source code, Track fraud status in profiles * Create test objects through association * Refactor time calculation * LG-9362 Please call page after personal key page in GPO flow (#8156) * move please call page to after personal key in gpo flow - Remove redirect to please call page from GPOVerifyController - PersonalKeyController already had a redirect to please call page for other flows, so just remove check for GPO that went to come back later page instead. * remove gpo_pending deactivation reason when verified remove gpo_pending deactivation reason when a user verifies their GPO code, but is placed under fraud review * Remove two tests that expect Personal Key to redirect to come_back_later path These tests were leftover from when the Personal Key was shown before sending the GPO code. Since the user has now entered their code before seeing that screen, it should never redirect to the Come Back Later screen * Check threatmetrix result directly rather than looking at user profile Specs are looking at ProofingComponent, and that's what the code previously used to decide whether to show the verified alert (as well as sad face screen). * Update GPO specs to expect personal key, not please call screen changelog: User-facing Improvements, Verify by Mail, "Please Call" screen after Personal Key screen. --------- Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov> * Add files for Acuant SDK upgrade to v 11.8.1 (#8169) * Add files for Acuant SDK upgrade to v 11.8.1 * [skip changelog] * Remove saml_internal_post feature flag (#8180) This has been live in production for some time and is not expected to need to be disabled. changelog: Internal, Code Quality, Remove unnecessary feature flag * LG-9232: Improve the usability of the password page (#8085) * edit and add content to new password page changelog: User-facing improvements, passwords, improve usability of password page * lint yml files * replace current state text with new copy * change event listener to focusout * fix failing lint tests * analyze password on keydown and touchout * fix for proper syntax * remove focusout and touchout events * Fix test * add once option to event listener * trying something: what if there is another class on page load? * Empty commit * roll back test class * change how password strength bar appears * add test for modified pw strenth bar * update user spec password * Update spec/features/users/user_profile_spec.rb Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> * Update spec/features/visitors/set_password_spec.rb Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> * Update spec/features/visitors/set_password_spec.rb Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> * fix lint error * bold "12 characters" * WIP: Figure out insertion of character count * add password length data * pass minimum pw length to js * fix test for character limit test * lint fixes * edit assignment, change class from so->average * iterate on password strength * turn string value into a number * add code review comments; WIP: first draft of js test * give test a hard coded password to calculate from * remove score check, use zxcvbn instead of manually created result * cleanup * update copy for password length * update tests (round 1) * Edit test round 2 * improve readability * change one password hint for consistency --------- Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> --------- Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov> Co-authored-by: Eric Gade <105373963+eric-gade@users.noreply.github.com> Co-authored-by: John Maxwell <john.maxwell@gsa.gov> Co-authored-by: eileen-nava <80347702+eileen-nava@users.noreply.github.com> Co-authored-by: Tim Bradley <90272033+NavaTim@users.noreply.github.com> Co-authored-by: Mitchell Henke <mitchell.henke@gsa.gov> Co-authored-by: Matt Hinz <matt.hinz@gsa.gov> Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov> Co-authored-by: Jeremy Curcio <jeremy.curcio@gsa.gov> Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov> Co-authored-by: Jack Ryan <jackryan@navapbc.com> Co-authored-by: Kimball Bighorse <kimball.bighorse@gsa.gov> Co-authored-by: Alex Bradley <alexander.bradley@gsa.gov> Co-authored-by: John Skiles Skinner <john.skinner@gsa.gov> Co-authored-by: Jessica Dembe <jessica.dembe@gsa.gov>
jmhooper
added a commit
that referenced
this pull request
Apr 14, 2023
…result` In a previous commit we changed the DDP proofer to respond with an exception result when the result from DDP included an unexpected status (ref: #8149). This includes when the result is nil. This commit changes the DDP mock's behavior to align with the DDP proofer's behavior. [skip changelog]
jmhooper
added a commit
that referenced
this pull request
Apr 17, 2023
…result` (#8214) In a previous commit we changed the DDP proofer to respond with an exception result when the result from DDP included an unexpected status (ref: #8149). This includes when the result is nil. This commit changes the DDP mock's behavior to align with the DDP proofer's behavior. [skip changelog]
jmdembe
added a commit
that referenced
this pull request
Apr 18, 2023
* Remove unused configuration redis_throttle_alternate_url and redis_throttle_alternate_pool_write_enabled configurations (#8211) changelog: Internal, Configuration, Remove unused Redis migration configuration * Override tag style to leave case unchanged and make text bold (#8212) * Override tag style to leave case unchanged and make text bold Per Andrew Duthie the Login style for tags is Title case and bold, but the design system is still all caps, so override it for idp for now. changelog: User-facing Improvements, Design, change 'tag' style to be bold and preserve case * LG-9333 Preserve the SSN when returning to the SSN controller (#8197) The SSN controller allows a user to enter or update their SSN. Prior to this commit the previous value of the SSN was not maintained when the user was updating their SSN. This commit attempts to fix that by making use of the SSN form that is used in the controller. The template is changed to eventually read the SSN from the form. Additionally, the form is used in a pattern that better matches the pattern you expect a form object to be used in. This was not the case before because of constraints that the flow state machine placed on HTML forms. changelog: Improvements, Proofing workflow, The user's SSN is preserved for users who enter and SSN then continue to the verify step but return to the SSN in the unsupervised remote proofing flow. * LG-8714 Remove UserDecorator (#8204) * Moving UserDecorator methods to User * Updating all references to user.decorate Also: - Move UserDecorator specs to User spec (with adjustments) - Delete old UserDecorator class and spe Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> * Jskinne3 lg 9361 mobile local md (#8217) * Transfer mobile guide from old branch * Link to mobile docs from other docs * Check spelling * [skip changelog] * Auto-generate README * Soften USB hub wording becase it works for some people * Seperate mobile device and VM instructions * Configure the DDP mock client to respond with a failed result on `no_result` (#8214) In a previous commit we changed the DDP proofer to respond with an exception result when the result from DDP included an unexpected status (ref: #8149). This includes when the result is nil. This commit changes the DDP mock's behavior to align with the DDP proofer's behavior. [skip changelog] * LG-9034 Log TrueID decision product status (#8195) * LG-9034 Log TrueID decision product status Capture decision product status for TrueID responses Decision product status contains the result of TrueID after decisioning. Prior to this change, product status was only being captured for @productType='TrueID' and ignoring product status for @productType='TrueID_Decision' * [skip changelog] * successful if TrueID_Decision product does not exist * refactor obtaining decision_product for reuse * happy linting * Update spec/services/doc_auth/lexis_nexis/responses/true_id_response_spec.rb Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov> * remove unnecessary expect statements since the check is done a few lines below * remove line no longer used in test --------- Co-authored-by: AmirReavis-Bey <amirreavis-bey@fcoh2j-wyp9w9mv.localdomain> Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov> * LG-9488: Entry controller for hybrid mobile document capture flow (#8209) * Add placeholder CaptureDocController * Add controller to handle entry into hybrid flow Controller looks at `document-capture-session` and does a "light login" to enable the user to start document capture. changelog: Internal, Flow state machine removal, Add controller for entry into hybrid mobile document capture * Refactor feature flag check out to HybridMobileConcern * Add tests around the 'Doc Auth' event * Send links to new hybrid mobile experience Going with /verify/documents?document-capture-session=xxxxx as a temporary url for now * CaptureDocController -> DocumentCaptureController * Tweak doc capture entry url naming & add comment * idv_hybrid_mobile_document_capture_entry_url -> idv_hybrid_mobile_entry_url * Refactor EntryController not to use before_action * Move HybridMobileConcern into Idv::HybridMobile namespace * WHOOPS need to see why this wasn't breaking any tests * Remove extra redirect Rails doesn't preserve the querystring when redirecting routes, and this is just an unnecessary extra step anyway * Add feature spec for mobile hybrid flow entry * Tweaks to capture complete step spec Ultimately we'll merge this with a doc capture step, presumably * Fix email confirmation 500 (#8224) * add failing spec * Use strong parameters in email confirmation controller to fix 500 error changelog: Bug Fixes, Email Confirmation, Use strong parameters in email confirmation controller to fix 500 error * LG-9321: New field on Enrollment Outcomes for GetUspsProofingResultsJob Summary (#8216) * LG-9321 Add data to usps proof results job * LG-9321 Added one more test * LG-9321 Fix lint issues * changelog: Internal, In-person-proofing, new metric on enrollment outcomes summary for GetUspsProofingResultsJob * LG-9321 Add arg to round() for precision * LG-9321 Update round to have more precision Co-authored-by: Tim Bradley <90272033+NavaTim@users.noreply.github.com> * LG-9321 Remove initial unnecessary assignment Co-authored-by: Tim Bradley <90272033+NavaTim@users.noreply.github.com> --------- Co-authored-by: Tim Bradley <90272033+NavaTim@users.noreply.github.com> --------- Co-authored-by: Mitchell Henke <mitchell.henke@gsa.gov> Co-authored-by: Sonia Connolly <sonia.connolly@gsa.gov> Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov> Co-authored-by: Eric Gade <105373963+eric-gade@users.noreply.github.com> Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com> Co-authored-by: John Skiles Skinner <john.skinner@gsa.gov> Co-authored-by: Amir Reavis-Bey <1261794+amirbey@users.noreply.github.com> Co-authored-by: AmirReavis-Bey <amirreavis-bey@fcoh2j-wyp9w9mv.localdomain> Co-authored-by: Matt Hinz <matt.hinz@gsa.gov> Co-authored-by: gina-yamada <125507397+gina-yamada@users.noreply.github.com> Co-authored-by: Tim Bradley <90272033+NavaTim@users.noreply.github.com>
jc-gsa
pushed a commit
that referenced
this pull request
Apr 19, 2023
…us (#8149) We use ThreatMetrix as a tool for device profiling. It gives us a review_status result which we store in the database as a proofing component. That proofing component is used later in the process to determine if a profile needs additional review. We communicate with ThreatMetrix if device profiling is enabled or in collect only mode. We do not communicate with threatMetrix when device profiling is disabled. Previously, we would write nil to the proofing component if we received that or an unexpected review status from ThreatMetrix. This leads to ambiguity later in the process when determining if a user passed the ThreatMetrix check. Specifically, nil could mean the ThreatMetrix did not run because device profiling is disabled or it could mean that an error occurred and no review_status was provided. This commit changes the behavior of ThreatMetrix such that if ThreatMetrix is enabled it will write a review status or the job will respond with an exception result. This exception result will result in the user seeing an error and needing to re-submit and re-run the job. This resolves the ambiguity by making nil mean that ThreatMetrix was not enabled. [skip changelog]
jc-gsa
pushed a commit
that referenced
this pull request
Apr 19, 2023
…g the result hash (#8160) Prior to this commit the majority of our proofers returned an object with a #to_h method that returned a hash suitable for logging with the results of the proofing transaction. The exception was the DDP proofer. The DDP proofer's logged result needed to be constructed in the proofing job where it was invoked. This commit makes the DDP result mirror the other result classes. This will make it easier to reconstruct the DDP class to add logic around things like whether exceptions are present (see #8149). [skip changelog]
jc-gsa
pushed a commit
that referenced
this pull request
Apr 19, 2023
…result` (#8214) In a previous commit we changed the DDP proofer to respond with an exception result when the result from DDP included an unexpected status (ref: #8149). This includes when the result is nil. This commit changes the DDP mock's behavior to align with the DDP proofer's behavior. [skip changelog]
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We use ThreatMetrix as a tool for device profiling. It gives us a
review_statusresult which we store in the database as a proofing component. That proofing component is used later in the process to determine if a profile needs additional review.We communicate with ThreatMetrix if device profiling is enabled or in collect only mode. We do not communicate with threatMetrix when device profiling is disabled.
Previously, we would write
nilto the proofing component if we received that or an unexpected review status from ThreatMetrix. This leads to ambiguity later in the process when determining if a user passed the ThreatMetrix check. Specifically,nilcould mean the ThreatMetrix did not run because device profiling is disabled or it could mean that an error occurred and noreview_statuswas provided.This commit changes the behavior of ThreatMetrix such that if ThreatMetrix is enabled it will write a review status or the job will respond with an exception result. This exception result will result in the user seeing an error and needing to re-submit and re-run the job. This resolves the ambiguity by making
nilmean that ThreatMetrix was not enabled.