LG-14830 | Send ID type to AAMVA by n1zyy · Pull Request #11428 · 18F/identity-idp

n1zyy · 2024-10-30T19:38:56Z

changelog: Internal, Identity Proofing, Send state ID type to AAMVA

🎫 Ticket

Link to the relevant ticket:
LG-14830

🛠 Summary of changes

As part of identity proofing, we classify the uploaded identity document: is it a driver's license or an non-driver ID card?

DLDV has a field for this, DocumentCategoryCode. We have some code in place to send this information, but at the API layer we don't actually do so. Sending this information should, at a minimum, help ID card holders in a few states proof successfully: MS and PA, which are among the country's hardest-to-spell states, only return ID Card results if DocumentCategoryCode is set appropriately.

This work is behind a feature flag for the unlikely case that this change in how we use their API has unexpected side effects.

🧑‍💼 Still to do

Update test to cover the case where ID type is nil
Stop hardcoding the value
~~Test return types~~ Existing tests already do this

changelog: Internal, Identity Proofing, Send state ID type to AAMVA

This likely breaks tests. Let's see which ones.

n1zyy · 2024-11-01T21:22:53Z

app/controllers/idv/in_person/verify_info_controller.rb

-      # be supported. See also LG-3852 and related findings document.
-      def set_state_id_type
-        pii_from_user[:state_id_type] = 'drivers_license' unless invalid_state?
-      end


I want to run this by Team Joy. I think I hadn't initially grokked exactly what this was doing.

In doc auth, we have state_id_type from another vendor's response and can pass that to AAMVA. This, however, is in the IPP flow, where data parsed from the image of the driver's license is unavailable, because the user is not taking this path. This code was setting it to drivers_license, which is the most common value.

It turns out that, in VerificationRequest, we were never (whether IPP or not) actually sending state_id_type to AAMVA, ever. We passed it down the chain thinking we were, but it was (seemingly inadvertently) left out of the XML template.

I think there are 3 options we could take in this controller here:

Continue just claiming the document is a driver's license when we don't know, because it's correct the majority of a time.

Ask the user, in the UI, if they are using a Driver's License or (non-driver) ID Card, and send that value.

Remove this block and do not send state_id_type to AAMVA when we don't have that information.

I went for option 3 here, which feels most correct. We know it's OK to not send because that's currently what we do for everyone. Option 2 could be added later if Team Joy wants.

FTR I agree we should not send this information to AAMVA unless the user has actually given it to us (or maybe in some states we can suss out the document type based on the format of the ID number? I don't actually know)

Sorry so late getting to this. I agree, option 3 seems like the best path. I will talk with product about option 2. No need to implement new behavior in your pr

n1zyy · 2024-11-01T21:23:11Z

app/services/proofing/aamva/request/verification_request.rb

            inside: '//dldv:verifyDriverLicenseDataRequest',
          )

+          if IdentityConfig.store.aamva_send_id_type


This is feature-flagged so we can disable this if for some reason it goes awry.

👍 We should add a ticket to remove the feature flag if we don't encounter any issues having it enabled.

n1zyy · 2024-11-01T21:24:44Z

app/services/proofing/aamva/request/verification_request.rb

+                          when 'drivers_permit'
+                            2


I haven't seen this in the wild in Cloudwatch, but it's listed in other classes as an allowed type, and is something AAMVA supports.

I only see this mentioned in the mock client? Can we just remove it there and then remove this (maybe leave a comment that AAMVA supports "2" to mean drivers permit?)

I was conflicted about this.

I didn't see it in Cloudwatch but wasn't positive if that's because it was very uncommon (most people with permits are probably minors, and people with a permit probably tend to upgrade it to a driver's license within a year or so), or because it's not supported.

My instinct is to keep it in the API client, partly for documentation and partly in case we start sending it some day.

However, I don't feel strongly at all if you think it's best to remove.

n1zyy · 2024-11-01T21:27:34Z

app/services/proofing/mock/state_id_mock_client.rb

-        !SUPPORTED_STATE_ID_TYPES.include?(state_id_type) ||
-          state_id_type.nil?
+        !SUPPORTED_STATE_ID_TYPES.include?(state_id_type) &&
+          !state_id_type.nil?


This caused a lot of grief in tests. AFAICT, the non-mock client does not require the presence of state_id_type. (See, e.g., null/empty-string responses in Cloudwatch.) The mock client shouldn't implement validation that real clients don't have and that does not serve a clear purpose.

n1zyy · 2024-11-01T21:28:05Z

spec/controllers/idv/in_person/verify_info_controller_spec.rb

      put :update
-
-      expect(subject.user_session['idv/in_person'][:pii_from_user][:state_id_type]).
-        to eq 'drivers_license'


This was explicitly testing that we shoehorned 'drivers_license' in from the VerifyInfoController, which I ripped out.

n1zyy · 2024-11-01T21:29:10Z

spec/features/idv/analytics_spec.rb

                                 vendor_workflow: nil,
                                 verified_attributes: nil },
-          state_id: state_id_resolution_with_id_type,
+          state_id: state_id_resolution,


This is the let(:in_person_path_proofing_results) block. As discussed above, we don't set this for IPP now.

matthinz

lmgeorge · 2024-11-05T17:15:26Z

app/services/proofing/aamva/request/verification_request.rb

+          category_code = case id_type
+                          when 'drivers_license'
+                            1
+                          when 'drivers_permit'
+                            2
+                          when 'state_id_card'
+                            3
+                          end
+


(suggestion, non-blocking): It would be great if this was stored as a const at the top of the class.
(:wishes fruitlessly that Ruby supported enums:)

lmgeorge · 2024-11-05T17:17:02Z

config/application.yml.default

 aamva_cert_enabled: true
 aamva_private_key: ''
 aamva_public_key: ''
+aamva_send_id_type: false


(question): Do we want to set this to true and then disable in prod instead?

lmgeorge · 2024-11-05T17:19:42Z

spec/controllers/idv/in_person/verify_info_controller_spec.rb

-    it 'sets ssn and state_id_type on pii_from_user' do
+    it 'sets ssn on pii_from_user' do
      expect(Idv::Agent).to receive(:new).with(
        hash_including(


(suggestion, non-blocking): This is more of a regression testing thing, but it would be good to have an expectation that ensures we aren't sending state_id_type as hash_including cannot assert the absence of a key-value pair.

This is a weird case and I'm on the fence. My approach with this was that it is no longer reasonable to require state_id_type's presence, but I'm not sure I'd go as far as requiring its absence here. "The controller no longer overwrites the value with a lie" feels like a kind of weird regression test after I ripped that code out.

lmgeorge

Overall, looks good. Have a couple of small suggestions/questions, but don't want that to be a blocker to getting this shipped.

n1zyy · 2024-11-05T20:00:50Z

spec/fixtures/proofing/aamva/requests/verification_request.xml

          <nc:LocationPostalCode>20176</nc:LocationPostalCode>
        </aa:Address>
-      </dldv:verifyDriverLicenseDataRequest>
+      <aa:DocumentCategoryCode>1</aa:DocumentCategoryCode></dldv:verifyDriverLicenseDataRequest>


"This looks misformatted!," you might argue. I totally agree! But this is what the code generates.

n1zyy · 2024-11-05T20:02:31Z

spec/fixtures/proofing/aamva/requests/verification_request.xml

    </dldv:VerifyDriverLicenseData>
  </soap:Body>
-</soap:Envelope>
+</soap:Envelope>


I think this is some DOS vs. UNIX line endings cursedness. There is not an extra newline at the end; it's just a plain text file. This came in when I edited the file.

n1zyy · 2024-11-05T20:06:39Z

spec/services/proofing/aamva/request/verification_request_spec.rb

  describe '#body' do
    it 'should be a request body' do
-      expect(subject.body).to eq(AamvaFixtures.verification_request)
+      expect(subject.body + "\n").to eq(AamvaFixtures.verification_request)


"This is disgusting!," you might protest again. And again, I agree.

I am going to file a followup story or five, because:

As @lmgeorge pointed out, expecting absolute string equality when comparing XML documents isn't quite right.

add_optional_element is somehow leading to ugly mis-indentation.

I tried to clean it up with REXML::Formatters::Pretty but somehow made an even bigger mess that complained it wasn't valid XML. And if we're going to format our XML nicely, I want us to be sending it to AAMVA that way, not just cleaning it up for a test.

n1zyy added 6 commits October 30, 2024 15:28

LG-14830 | Send ID type to AAMVA

526ecaa

changelog: Internal, Identity Proofing, Send state ID type to AAMVA

Tests for no/invalid ID type

bf22447

Stop hardcoding state_id_type to 'drivers_license'

3bc17f2

This likely breaks tests. Let's see which ones.

Clean up test failures

66ca749

Merge branch 'main' into mattw/LG-14830_aamva_id_type

fddc67c

Now with lintfixes

6e88ae5

n1zyy commented Nov 1, 2024

View reviewed changes

Merge branch 'main' into mattw/LG-14830_aamva_id_type

1db0c82

n1zyy marked this pull request as ready for review November 4, 2024 19:58

n1zyy requested a review from a team November 5, 2024 16:48

n1zyy changed the title ~~[WIP] LG-14830 | Send ID type to AAMVA~~ LG-14830 | Send ID type to AAMVA Nov 5, 2024

matthinz approved these changes Nov 5, 2024

View reviewed changes

lmgeorge reviewed Nov 5, 2024

View reviewed changes

lmgeorge approved these changes Nov 5, 2024

View reviewed changes

n1zyy added 3 commits November 5, 2024 12:48

Merge branch 'main' into mattw/LG-14830_aamva_id_type

fe5e5e9

Fix feature flag to default on, but be off in prod

4c3797c

Fix (grossly) test fixture not matching

cac657e

n1zyy commented Nov 5, 2024

View reviewed changes

n1zyy merged commit b899a25 into main Nov 5, 2024

n1zyy deleted the mattw/LG-14830_aamva_id_type branch November 5, 2024 20:39

jmhooper mentioned this pull request Nov 7, 2024

Deploy RC 429 to Production #11472

Merged

Conversation

n1zyy commented Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

🧑‍💼 Still to do

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!

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!

matthinz 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!

lmgeorge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n1zyy Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

n1zyy commented Oct 30, 2024 •

edited

Loading

n1zyy Nov 5, 2024 •

edited

Loading