LG-16327 Update ssn logic ial2 match sp by kevinsmaster5 · Pull Request #12326 · 18F/identity-idp

kevinsmaster5 · 2025-07-09T16:03:32Z

🎫 Ticket

Link to the relevant ticket:
LG-16327

🛠 Summary of changes

Ensure the restriction of duplicate SSN checks to only those IAL2 accounts that are associated with SP (Service Provider) connections that have opted into the upcoming OneAccount functionality.

Builds upon previous #12296

vrajmohan · 2025-07-10T18:57:24Z

app/services/idv/duplicate_ssn_finder.rb

The initiating_service_provider_issuer is merely the service provider who initiated the identity verification. The current service provider may be different.

I am now comparing the Profiles against the current session sp with a3e5ee0

vrajmohan · 2025-07-14T23:27:39Z

I'm not seeing how this fixes the issue either.

Say there are 2 service providers: SP1 and SP2. SP2 and SP3 are in eligible_one_account_providers.
Profile P1 with SSN SSN1 that was initiated by SP1 and later used by SP2.
Profile P2 with the same SSN (SSN1) that was initiated and later used by SP2.
This should be detected as a duplicate when an IAL2 with facial match is requested by SP2.

I don't see a way to detect this without using sp_return_logs.

vrajmohan · 2025-07-14T23:28:47Z

spec/services/idv/duplicate_ssn_finder_spec.rb

Can we use different service providers? The "inactive" in the name is misleading.

This is the service provider generated by the factory with create(:profile, :facial_match_proof
https://github.com/18F/identity-idp/blob/main/spec/factories/profiles.rb#L105-L108

Sorry, but that does not make it any better. It is wrong there as well.
That is an inactive service provider, which is used in tests like

identity-idp/spec/features/openid_connect/redirect_uri_validation_spec.rb

Line 28 in f3417c3

context 'when the service_provider is not active' do

. It does not cause harm here but is unnecessary misdirection.

FYI, I am cleaning up the existing problem here - #12377.

Thank you. I'm revising in my tests to avoid that as you requested.

vrajmohan · 2025-07-14T23:33:48Z

Also, the merged PR appears to have modified DuplicateSsnFinder#ssn_is_unique?. That method is not relevant for the One Account work and should be left alone.

See Cloudwatch query for loss of information.

kevinsmaster5 · 2025-07-15T19:09:17Z

app/services/idv/duplicate_ssn_finder.rb

Puts this method back to where it was before per
#12326 (comment)

app/services/idv/duplicate_ssn_finder.rb

kevinsmaster5 · 2025-07-16T14:20:24Z

app/services/idv/duplicate_ssn_finder.rb

Suggested change

Profile.joins(:sp_return_logs)

.active

.facial_match

.where(ssn_signature: ssn_signatures)

.where(sp_return_logs: { issuer: sp_eligible_for_one_account })

.where.not(user_id: user.id)

.distinct

Profile.joins("INNER JOIN identities ON identities.user_id = profiles.user_id")

.active

.facial_match

.where(ssn_signature: ssn_signatures)

.where(identities: { service_provider: sp_eligible_for_one_account })

.where(identities: { deleted_at: nil })

.where(identities: { ial: 2 })

.where.not(user_id: user.id)

.distinct

app/models/sp_return_log.rb

app/models/profile.rb

kevinsmaster5 · 2025-07-18T17:50:41Z

Slack question for Team https://gsa-tts.slack.com/archives/C05MGJ72GU9/p1752849537704709

app/models/profile.rb

vrajmohan

I think we would be better served by ensuring that the tests map to the spreadsheet with the combinations that we have created.

vrajmohan · 2025-07-28T22:08:33Z

spec/services/idv/duplicate_ssn_finder_spec.rb

Sorry, but that does not make it any better. It is wrong there as well.
That is an inactive service provider, which is used in tests like

identity-idp/spec/features/openid_connect/redirect_uri_validation_spec.rb

Line 28 in f3417c3

context 'when the service_provider is not active' do

. It does not cause harm here but is unnecessary misdirection.

vrajmohan · 2025-07-28T22:27:35Z

spec/services/duplicate_profile_checker_spec.rb

Why do we need the session_uuid set?

That's now removed from both _spec

vrajmohan · 2025-07-28T23:21:02Z

spec/services/idv/duplicate_ssn_finder_spec.rb

Why have the identities associated only in this context and not in the sibling contexts? In fact, having these be associated in the next context should demonstrate that the code is wrong because the test fails.

Added more tests with 857818b
These cover the spreadsheet mockup users 1 - 4. Users 5 & 6 would not ever reach the duplicate_ssn_finder class because they would be eliminated at

identity-idp/app/services/duplicate_profile_checker.rb

Line 14 in 0758644

return unless user_has_ial2_profile? && user_sp_eligible_for_one_account?

User 5 is covered with this test

identity-idp/spec/services/duplicate_profile_checker_spec.rb

Line 104 in 0758644

context 'when user does not have active IAL2 profile' do

User 6 is covered with

identity-idp/spec/services/duplicate_profile_checker_spec.rb

Line 34 in 0758644

context 'when user does not have other accounts with matching profile' do

vrajmohan · 2025-07-28T23:26:11Z

yarn.lock

What JavaScript changes are we making to cause this?

My local NPM must have done something strange. I'll revert that.

…rvice provider

vrajmohan

I am sorry but we don't appear to be converging: This still does not address the fundamental issue of duplicates within the requesting service provider. This will be clear if the requesting service provider from the Reduced Version spreadsheet is SP2 instead of SP1. In that case, User 3 should not come up as a duplicate when requesting for User 1 or User 2.

There are several other issues as well:

The test cases are not clear
Unnecessary fixture setup of session_uuid: SecureRandom.uuid
Repeated context in test of "describe '#associated_facial_match_profiles_with_ssn'".
Test assertion of specific profile ids with no sort specified.

Sorry to do this, but I'm creating a separate PR that leverages some of this work and that hopefully addresses all these issues.

kevinsmaster5 marked this pull request as ready for review July 9, 2025 19:19

kevinsmaster5 requested a review from vrajmohan July 9, 2025 19:27

vrajmohan reviewed Jul 10, 2025

View reviewed changes

kevinsmaster5 force-pushed the update-ssn-logic-ial2-match-sp branch from 7a680db to a3e5ee0 Compare July 14, 2025 15:41

kevinsmaster5 requested a review from vrajmohan July 14, 2025 16:07

vrajmohan reviewed Jul 14, 2025

View reviewed changes

kevinsmaster5 commented Jul 15, 2025

View reviewed changes

app/services/idv/duplicate_ssn_finder.rb Outdated Show resolved Hide resolved

kevinsmaster5 commented Jul 16, 2025

View reviewed changes

app/models/sp_return_log.rb Outdated Show resolved Hide resolved

kevinsmaster5 commented Jul 16, 2025

View reviewed changes

app/models/profile.rb Outdated Show resolved Hide resolved

kevinsmaster5 requested a review from a team July 18, 2025 17:50

kevinsmaster5 force-pushed the update-ssn-logic-ial2-match-sp branch from 0bb6bd8 to a53be99 Compare July 22, 2025 14:04

vrajmohan reviewed Jul 24, 2025

View reviewed changes

app/models/profile.rb Outdated Show resolved Hide resolved

vrajmohan requested changes Jul 28, 2025

View reviewed changes

kevinsmaster5 added 12 commits July 30, 2025 10:11

changelog: Internal, MVP One Account, Match duplicates within same se…

f52ddb2

…rvice provider

include eligible sp list in match query

a8002cb

correct word in supposition

823f6f7

remove profile service provider from initiator to fix unrelated test

4e95e03

derive sp issuer from current session sp

bcb25d9

update spec to include passed issuer parameter

138d7a3

join sp_return_logs to profile as an interation on building the query

8fbee7f

leverage identities

4a10c5f

add testing fix lint error

bf70292

removes belongs link to profile

a7fae57

remove unnecessary model affiliation

8ddb1e9

add test cases

857818b

kevinsmaster5 added 2 commits July 30, 2025 10:13

remove extra users

f384870

replace changes to yarn lock

58cd50e

kevinsmaster5 force-pushed the update-ssn-logic-ial2-match-sp branch from fa5db11 to 58cd50e Compare July 30, 2025 14:13

fix tests to receive sp placeholder

0758644

kevinsmaster5 requested a review from vrajmohan July 30, 2025 20:12

vrajmohan mentioned this pull request Aug 4, 2025

Undo unintended resolution proofing change #12390

Merged

vrajmohan reviewed Aug 4, 2025

View reviewed changes

kevinsmaster5 closed this Aug 15, 2025

-      Profile.joins(:sp_return_logs)
-        .active
-        .facial_match
-        .where(ssn_signature: ssn_signatures)
-        .where(sp_return_logs: { issuer: sp_eligible_for_one_account })
-        .where.not(user_id: user.id)
-        .distinct
+      Profile.joins("INNER JOIN identities ON identities.user_id = profiles.user_id")
+        .active
+        .facial_match
+        .where(ssn_signature: ssn_signatures)
+        .where(identities: { service_provider: sp_eligible_for_one_account })
+        .where(identities: { deleted_at: nil })
+        .where(identities: { ial: 2 })
+        .where.not(user_id: user.id)
+        .distinct

Conversation

kevinsmaster5 commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrajmohan commented Jul 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrajmohan commented Jul 14, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kevinsmaster5 commented Jul 18, 2025

Uh oh!

Uh oh!

vrajmohan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrajmohan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kevinsmaster5 commented Jul 9, 2025 •

edited

Loading