LG-13382: Don't add SP costs again if worker already did it #10743
Conversation
This file contains 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
Update VerifyInfoConcern to not add SP costs. changelog: Internal, Identity verification, Guard against double-counting SP costs
jmhooper
approved these changes
Jun 3, 2024
zachmargolis
reviewed
Jun 3, 2024
colter-nattrass
pushed a commit
that referenced
this pull request
Jun 3, 2024
Update VerifyInfoConcern to not add SP costs. changelog: Internal, Identity verification, Guard against double-counting SP costs
jmhooper
added a commit
that referenced
this pull request
Jun 5, 2024
We are in the process of moving the logic to track SP costs from the IdP into workers. This will let us more accurately track costs by marking costly events where they actually happen (in the worker). It will also give us some flexibility with the result object that is used during resolution since it will not have SP cost tracking as a dependency. In #10743 we stopped writing the costs iin the controller if the result hash indicates that costs were alreay written. This commit does the work of writing costs in the `ProgressiveProofer` and returns that they were written in the result. The next step will be removing the logic to write costs in the controller This commit can only be safely deployed when the changes in #10743 are fully deployed. [skip changelog]
jmhooper
added a commit
that referenced
this pull request
Jun 5, 2024
We are in the process of moving the logic to track SP costs from the IdP into workers. This will let us more accurately track costs by marking costly events where they actually happen (in the worker). It will also give us some flexibility with the result object that is used during resolution since it will not have SP cost tracking as a dependency. In #10743 we stopped writing the costs iin the controller if the result hash indicates that costs were alreay written. This commit does the work of writing costs in the `ProgressiveProofer` and returns that they were written in the result. The next step will be removing the logic to write costs in the controller This commit can only be safely deployed when the changes in #10743 are fully deployed. [skip changelog]
Merged
aduth
added a commit
that referenced
this pull request
Jun 6, 2024
* Bump phonelib from 0.8.8 to 0.8.9 (#10746) Bumps [phonelib](https://github.com/daddyz/phonelib) from 0.8.8 to 0.8.9. - [Release notes](https://github.com/daddyz/phonelib/releases) - [Changelog](https://github.com/daddyz/phonelib/blob/master/CHANGELOG.md) - [Commits](daddyz/phonelib@v0.8.8...v0.8.9) --- updated-dependencies: - dependency-name: phonelib dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Don't add SP costs twice if worker already did it (#10743) Update VerifyInfoConcern to not add SP costs. changelog: Internal, Identity verification, Guard against double-counting SP costs * Revert "Update to newest saml_idp (#10734)" (#10748) This reverts commit 21bb1cf. * Update Spanish, and Chinese content that was missed in big translation (#10719) * Update Spanish content that was missed in big translation changelog: Internal, Internationalization, Update Spanish content * add additional Chinese translations * Normalize FormResponse documentation in AnalyticsEvents (#10745) * Document standard FormResponse analytics consistently changelog: Internal, Analytics, Document standard FormResponse analytics consistently * Restore optional errors passed in webauthn_setup_submitted * Restore multi_factor_auth_setup errors as nilable * Try disregarding nil values from actual logged These aren't really logged as meaningfully query values anyways, and we should only care if we explicitly expect a nil value * Update expect receive to have_logged_event * Remove unused error_details from piv_cac_login * Add missing error_details * Remove unnecessary allowed_extra_analytics * Revert unused error_details from webauthn_setup_submitted * Add missing error_details in spec assertions * Remove unused error_details from idv_in_person_locations_searched * Remove invalid error_details Remove invalid errors property from idv_in_person_locations_searched * Restore idv_in_person_locations_searched errors as nil-able Sometimes it's sent * Remove error_details from vendor submitted event Not passed (via DocAuthClient response) Remove unused analytics YARDoc error_details * Update IdV analytics spec expectations * Revert unused error_details in fraud review events * Simplify diff * Expect success, errors always in logout events * Make success, errors nilable for logout_initiated Not always provided: https://github.com/18F/identity-idp/blob/a10bd0b98d30d64d08eac23df91db3aa0830e363/app/controllers/sign_out_controller.rb#L7 * Remove unnecessary allowed_extra_analytics in accessibility specs (#10749) * Remove unnecessary allowed_extra_analytics in accessibility specs changeog: Internal, Automated Testing, Remove unnecessary allowed_extra_analytics in accessibility specs * Add changelog changelog: Internal, Automated Testing, Remove unnecessary allowed_extra_analytics in accessibility specs * LG-13221 New device sign-in delayed authentication doesn't list failed MFA attempts (#10659) * changelog: Bug Fixes, Fraud prevention, new device sign in list failed mfa attempts * adjust how created_at is calculated * change update sign_in_new_device_at from last disavowal to last notification expir time * fix specs that were missing the expected timeframe_expired event * add guardrails against missing timeframe_expired and limit query for performance * reduce delay buffer because of stale events, reset test from before this branch * refactor extraneous fetching in query add analytics for missing timeframe_expired event * replace mis-deleted block * revise timeframe expir query, update spec to include timeframe_expired paths, restore previous test setups * account for actual ordering of events * move timeframe_expired_event to private method, add regression test to new device spec * rename method for clarity of purpose * LG-13352 fixed bug where new_unique_users_year_unknown columns were not populating (#10752) * Fixed the bug where the new_unique_users_unknown column was not populating properly changelog: Internal, Reporting, Fixed the bug where the new_unique_users_unknown column was not populating properly * Refactor NameID format related tests (#10727) **Why**: 1. The tests are hard to understand and do not clearly show the different contexts 2. The tests assert on a side effect (the analytics event posted) rather than the actual desired behavior (the return values) 3. The tests are actually misleading - they conflate "unspecified" and unsupported **How**: - Refactor tests with clear contexts and shared examples - Assert on actual return value and not on a side effect - Clean up test case descriptions - Assert that the correct attributes are logged in the analytics event changelog: Internal, Automated Testing, Refactor NameID format related tests * LG-13383 Use the SP issuer to compute the UUID and UUID prefix in the `ResolutionProofingJob` (#10728) In #10726 started passing the service provider issuer to the `ResolutionProofingJob`. This commit uses it to compute the UUID and UUID prefix for downstream proofers. This will eventually supersede the logic that does the same in the `VerifyInfoConcern`. The logic is left in the `VerifyInfoConcern` since we will still need to pass the UUID and UUID prefix to jobs until this is fully deployed. Once this is fully deployed that logic can be removed. [skip changelog] * Remove `instant_verify_ab_test_discriminator` background job argument (#10725) The code that depended on this argument was removed in #10724. The argument was left to avoid `ArgumentError`s during the 50/50 state. This commit removes the argument. It cannot be deployed until the changes in #10724 are fully deployed in production. [skip changelog] * Validate unnecessary exempted files in TypeScript enforcement (#10760) * Validate unnecessary exempted files in TypeScript enforcement changelog: Internal, Automated Testing, Validate unnecessary exempted files in TypeScript enforcement * Remove unnecessary array splat * Create production-ready docker image (#10738) Add prod idp image generation **Why**: - We need to have a production-like IDP docker image so we can start testing idp environments in kubernetes. **How**: - Created a dockerfile that is more production-ready than our review-apps one. - Added real .mmdb files and pwned_passwords.txt files - Added RDS cert - Changed file permissions so the app user cannot change the code - Moved the application.yml file from tmp to config - Created an application.yml file for the production dockerfile in case we want to put defaults in it. - Added a section to .gitlab-ci.yml that should build the idp image and put it in ECR. changelog: Internal, Platform Automation, Add production IDP image creation * Log the requested NameID format (#10761) changelog: User-Facing Improvements, Analytics, Log the requested NameID format * Bump libphonenumber-js from 1.11.2 to 1.11.3 (#10766) Bumps [libphonenumber-js](https://gitlab.com/catamphetamine/libphonenumber-js) from 1.11.2 to 1.11.3. - [Changelog](https://gitlab.com/catamphetamine/libphonenumber-js/blob/master/CHANGELOG.md) - [Commits](https://gitlab.com/catamphetamine/libphonenumber-js/compare/v1.11.2...v1.11.3) --- updated-dependencies: - dependency-name: libphonenumber-js dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Use ivars to clean up `ProgressiveProofer` methods (#10764) The `ProgressiveProofer` class has a `#proof` method which does the bulk of the footwork when it comes to orchestrating proofing API calls on the Verify-Your-Info step. This includes things like double address verification, get-to-yes, and skipping necessary API calls for users who cannot pass. Prior to this commit this class did not store any instance variables. This meant that the child methods in the job needed all of the context necessary to do their work passed in as arguments. This led to long lists of keyword arguments for these methods which distracted from the methods' purpose. This commit refactors the `ProgressiveProofer` to take the arguments that were passed to `#proof` in an initializer and store them as ivars. It also makes a change to store the vendor results as ivars. Finally since the PII passed into the job is an ivar it adds helper methods for transforming and operating on the PII with the aim of making that self-documenting. This adds some brevity to the implementation and hopefully removes some of the distraction that was associated with tracing arguments through the call stack. [skip changelog] * Update rails to address CVE-2024-28103 (#10770) changelog: Internal, Dependencies, Update dependencies to latest versions * LG-13503 Billing Model V2 Enhancments (#10769) changelog: Internal, Reporting, Modified logic in partner helper to reflect new billing requirements * LG-13503 Modified logic in partner helper to reflect new billing requirements * updated issuer columns to sync with partner better * updated compound key usage for uniqueness to a more unpackable object * adding spec to combined invoice report to account for single issuer partners matching idv age columns * Fixing date values to use utc --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Matt Hinz <matt.hinz@gsa.gov> Co-authored-by: Davida (she/they) <davida.marion@gsa.gov> Co-authored-by: Mitchell Henke <mitchell.henke@gsa.gov> Co-authored-by: Kevin Masters <135744319+kevinsmaster5@users.noreply.github.com> Co-authored-by: Colter <59977618+colter-nattrass@users.noreply.github.com> Co-authored-by: Vraj Mohan <vraj.mohan@gsa.gov> Co-authored-by: Jonathan Hooper <jonathan.hooper@gsa.gov> Co-authored-by: Tim Spencer <timothy.spencer@gsa.gov> Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
jmhooper
added a commit
that referenced
this pull request
Jun 6, 2024
We are in the process of moving the logic to track SP costs from the IdP into workers. This will let us more accurately track costs by marking costly events where they actually happen (in the worker). It will also give us some flexibility with the result object that is used during resolution since it will not have SP cost tracking as a dependency. In #10743 we stopped writing the costs iin the controller if the result hash indicates that costs were alreay written. This commit does the work of writing costs in the `ProgressiveProofer` and returns that they were written in the result. The next step will be removing the logic to write costs in the controller This commit can only be safely deployed when the changes in #10743 are fully deployed. [skip changelog]
jmhooper
added a commit
that referenced
this pull request
Jun 6, 2024
We are in the process of moving the logic to track SP costs from the IdP into workers. This will let us more accurately track costs by marking costly events where they actually happen (in the worker). It will also give us some flexibility with the result object that is used during resolution since it will not have SP cost tracking as a dependency. In #10743 we stopped writing the costs in the controller if the result hash indicates that costs were already written. This commit does the work of writing costs in the `ProgressiveProofer` and returns that they were written in the result. The next step will be removing the logic to write costs in the controller This commit can only be safely deployed when the changes in #10743 are fully deployed. [skip changelog]
jmhooper
added a commit
that referenced
this pull request
Jun 11, 2024
The `VerifyInfoConcern` used to add SP costs to the database by inspecting the result from the `ResolutionProofingJob`. We have made the following changes in an effort to move the logic for tracking SP costs into the resolution proofing job: - In #10743 we added a conditional to the `VerifyInfoConcern` to stop writing SP costs if a `sp_costs_added` flag is present on the result - In #10767 we started adding costs in the `ResolutionProofingJob` and had the job set the `sp_costs_added` flag - In #10786 we removed the code that added the SP costs in the `VerifyInfoConcern` This commit includes the changes for the final step: Removing the `sp_costs_added` flag now that nothing is reading it. This commit can be safely merged into main when all of the above changes are fully deployed. [skip changelog]
jmhooper
added a commit
that referenced
this pull request
Jun 11, 2024
The `VerifyInfoConcern` used to add SP costs to the database by inspecting the result from the `ResolutionProofingJob`. We have made the following changes in an effort to move the logic for tracking SP costs into the resolution proofing job: - In #10743 we added a conditional to the `VerifyInfoConcern` to stop writing SP costs if a `sp_costs_added` flag is present on the result - In #10767 we started adding costs in the `ResolutionProofingJob` and had the job set the `sp_costs_added` flag - In #10786 we removed the code that added the SP costs in the `VerifyInfoConcern` This commit includes the changes for the final step: Removing the `sp_costs_added` flag now that nothing is reading it. This commit can be safely merged into main when all of the above changes are fully deployed. [skip changelog]
jmhooper
added a commit
that referenced
this pull request
Jun 13, 2024
The `VerifyInfoConcern` used to add SP costs to the database by inspecting the result from the `ResolutionProofingJob`. We have made the following changes in an effort to move the logic for tracking SP costs into the resolution proofing job: - In #10743 we added a conditional to the `VerifyInfoConcern` to stop writing SP costs if a `sp_costs_added` flag is present on the result - In #10767 we started adding costs in the `ResolutionProofingJob` and had the job set the `sp_costs_added` flag - In #10786 we removed the code that added the SP costs in the `VerifyInfoConcern` This commit includes the changes for the final step: Removing the `sp_costs_added` flag now that nothing is reading it. This commit can be safely merged into main when all of the above changes are fully deployed. [skip changelog]
brandemix
pushed a commit
to brandemix/18F-identity-idp
that referenced
this pull request
Jun 17, 2024
We are in the process of moving the logic to track SP costs from the IdP into workers. This will let us more accurately track costs by marking costly events where they actually happen (in the worker). It will also give us some flexibility with the result object that is used during resolution since it will not have SP cost tracking as a dependency. In 18F#10743 we stopped writing the costs in the controller if the result hash indicates that costs were already written. This commit does the work of writing costs in the `ProgressiveProofer` and returns that they were written in the result. The next step will be removing the logic to write costs in the controller This commit can only be safely deployed when the changes in 18F#10743 are fully deployed. [skip changelog]
jmhooper
added a commit
that referenced
this pull request
Jun 20, 2024
The `VerifyInfoConcern` used to add SP costs to the database by inspecting the result from the `ResolutionProofingJob`. We have made the following changes in an effort to move the logic for tracking SP costs into the resolution proofing job: - In #10743 we added a conditional to the `VerifyInfoConcern` to stop writing SP costs if a `sp_costs_added` flag is present on the result - In #10767 we started adding costs in the `ResolutionProofingJob` and had the job set the `sp_costs_added` flag - In #10786 we removed the code that added the SP costs in the `VerifyInfoConcern` This commit includes the changes for the final step: Removing the `sp_costs_added` flag now that nothing is reading it. This commit can be safely merged into main when all of the above changes are fully deployed. [skip changelog]
jmhooper
added a commit
that referenced
this pull request
Jun 20, 2024
The `VerifyInfoConcern` used to add SP costs to the database by inspecting the result from the `ResolutionProofingJob`. We have made the following changes in an effort to move the logic for tracking SP costs into the resolution proofing job: - In #10743 we added a conditional to the `VerifyInfoConcern` to stop writing SP costs if a `sp_costs_added` flag is present on the result - In #10767 we started adding costs in the `ResolutionProofingJob` and had the job set the `sp_costs_added` flag - In #10786 we removed the code that added the SP costs in the `VerifyInfoConcern` This commit includes the changes for the final step: Removing the `sp_costs_added` flag now that nothing is reading it. This commit can be safely merged into main when all of the above changes are fully deployed. [skip changelog]
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
🎫 Ticket
Link to the relevant ticket:
LG-13382
🛠 Summary of changes
We'd like to move SP cost tracking out of VerifyInfoConcern and into the background job. To handle the 50/50 state, we need to make sure that VIC is not doing SP cost tracking if the worker already did.
A future PR will add SP cost tracking to the worker and will ensure that
sp_costs_addedis set on the resolution result.