Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions app/controllers/concerns/idv/verify_info_concern.rb
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@ def shared_update
Funnel::DocAuth::RegisterStep.new(current_user.id, sp_session[:issuer]).
call('verify', :update, true)

set_state_id_type

ssn_rate_limiter.increment!

document_capture_session = DocumentCaptureSession.create(
Expand Down
8 changes: 0 additions & 8 deletions app/controllers/idv/in_person/verify_info_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,6 @@ def flow_param
'in_person'
end

# state_id_type is hard-coded here because it's required for proofing against
# AAMVA. We're sticking with driver's license because most states don't discern
# between various ID types and driver's license is the most common one that will
# 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
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Continue just claiming the document is a driver's license when we don't know, because it's correct the majority of a time.
  2. Ask the user, in the UI, if they are using a Driver's License or (non-driver) ID Card, and send that value.
  3. 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


def invalid_state?
pii_from_user.blank?
end
Expand Down
3 changes: 0 additions & 3 deletions app/controllers/idv/verify_info_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,6 @@ def self.step_info

def flow_param; end

# state ID type isn't manually set for Idv::VerifyInfoController
def set_state_id_type; end

def prev_url
idv_ssn_url
end
Expand Down
27 changes: 27 additions & 0 deletions app/services/proofing/aamva/request/verification_request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,36 @@ def add_user_provided_data_to_body
inside: '//dldv:verifyDriverLicenseDataRequest',
)

if IdentityConfig.store.aamva_send_id_type
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

add_state_id_type(
applicant.state_id_data.state_id_type,
document,
)
end

@body = document.to_s
end

def add_state_id_type(id_type, document)
category_code = case id_type
when 'drivers_license'
1
when 'drivers_permit'
2
Comment on lines +101 to +102
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

when 'state_id_card'
3
end

Comment on lines +98 to +106
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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:)

if category_code
add_optional_element(
'aa:DocumentCategoryCode',
value: category_code,
document:,
inside: '//dldv:verifyDriverLicenseDataRequest',
)
end
end

def add_optional_element(name, value:, document:, inside: nil, after: nil)
return if value.blank?

Expand Down
4 changes: 2 additions & 2 deletions app/services/proofing/mock/state_id_mock_client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ def invalid_state_id_number?(state_id_number)
end

def invalid_state_id_type?(state_id_type)
!SUPPORTED_STATE_ID_TYPES.include?(state_id_type) ||
state_id_type.nil?
!SUPPORTED_STATE_ID_TYPES.include?(state_id_type) &&
!state_id_type.nil?
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end
end
end
Expand Down
2 changes: 2 additions & 0 deletions config/application.yml.default
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ aamva_auth_url: 'https://example.org:12345/auth/url'
aamva_cert_enabled: true
aamva_private_key: ''
aamva_public_key: ''
aamva_send_id_type: true
aamva_supported_jurisdictions: '["AL","AR","AZ","CO","CT","DC","DE","FL","GA","HI","IA","ID","IL","IN","KS","KY","MA","MD","ME","MI","MO","MS","MT","NC","ND","NE","NJ","NM","NV","OH","OR","PA","RI","SC","SD","TN","TX","VA","VT","WA","WI","WV","WY"]'
aamva_verification_request_timeout: 5.0
aamva_verification_url: https://example.org:12345/verification/url
Expand Down Expand Up @@ -501,6 +502,7 @@ development:
#
production:
aamva_auth_url: 'https://authentication-cert.aamva.org/Authentication/Authenticate.svc'
aamva_send_id_type: false
aamva_verification_url: 'https://verificationservices-cert.aamva.org:18449/dldv/2.1/online'
available_locales: 'en,es,fr'
biometric_ial_enabled: false
Expand Down
1 change: 1 addition & 0 deletions lib/identity_config.rb
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ def self.store
config.add(:aamva_cert_enabled, type: :boolean)
config.add(:aamva_private_key, type: :string)
config.add(:aamva_public_key, type: :string)
config.add(:aamva_send_id_type, type: :boolean)
config.add(:aamva_supported_jurisdictions, type: :json)
config.add(:aamva_verification_request_timeout, type: :float)
config.add(:aamva_verification_url)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -234,22 +234,15 @@
allow(user).to receive(:establishing_in_person_enrollment).and_return(enrollment)
end

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(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

state_id_type: 'drivers_license',
ssn: Idp::Constants::MOCK_IDV_APPLICANT_SAME_ADDRESS_AS_ID[:ssn],
consent_given_at: subject.idv_session.idv_consent_given_at,
),
).and_call_original

# our test data already has the expected value by default
subject.user_session['idv/in_person'][:pii_from_user].delete(:state_id_type)

put :update

expect(subject.user_session['idv/in_person'][:pii_from_user][:state_id_type]).
to eq 'drivers_license'
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end

context 'a user does not have an establishing in person enrollment associated with them' do
Expand Down
2 changes: 1 addition & 1 deletion spec/features/idv/analytics_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@
vendor_name: 'ResolutionMock',
vendor_workflow: nil,
verified_attributes: nil },
state_id: state_id_resolution_with_id_type,
state_id: state_id_resolution,
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

threatmetrix: threatmetrix_response,
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
<nc:LocationStateUsPostalServiceCode>VA</nc:LocationStateUsPostalServiceCode>
<nc:LocationPostalCode>20176</nc:LocationPostalCode>
</aa:Address>
</dldv:verifyDriverLicenseDataRequest>
<aa:DocumentCategoryCode>1</aa:DocumentCategoryCode></dldv:verifyDriverLicenseDataRequest>
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</dldv:VerifyDriverLicenseData>
</soap:Body>
</soap:Envelope>
</soap:Envelope>
Copy link
Copy Markdown
Contributor Author

@n1zyy n1zyy Nov 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

2 changes: 1 addition & 1 deletion spec/services/proofing/aamva/applicant_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
end

describe '.from_proofer_applicant(applicant)' do
it 'should create an AAMVA applicant with necessary proofer applcant data' do
it 'should create an AAMVA applicant with necessary proofer applicant data' do
aamva_applicant = described_class.from_proofer_applicant(proofer_applicant)

expect(aamva_applicant.uuid).to eq(proofer_applicant[:uuid])
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@

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)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

  1. As @lmgeorge pointed out, expecting absolute string equality when comparing XML documents isn't quite right.
  2. add_optional_element is somehow leading to ugly mis-indentation.
  3. 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.

end

it 'should escape XML in applicant data' do
Expand Down Expand Up @@ -88,6 +88,64 @@
'<aa:DriverLicenseExpirationDate>2030-01-02</aa:DriverLicenseExpirationDate>',
)
end

context '#state_id_type' do
context 'when the feature flag is off' do
before do
expect(IdentityConfig.store).to receive(:aamva_send_id_type).and_return(false)
end

it 'does not add a DocumentCategoryCode' do
applicant.state_id_data.state_id_type = 'drivers_license'
expect(subject.body).to_not include('<aa:DocumentCategoryCode>')
end
end

context 'when the feature flag is on' do
before do
expect(IdentityConfig.store).to receive(:aamva_send_id_type).and_return(true)
end

context 'when the type is a Drivers License' do
it 'includes DocumentCategoryCode=1' do
applicant.state_id_data.state_id_type = 'drivers_license'
expect(subject.body).to include(
'<aa:DocumentCategoryCode>1</aa:DocumentCategoryCode>',
)
end
end

context 'when the type is a learners permit' do
it 'includes DocumentCategoryCode=2' do
applicant.state_id_data.state_id_type = 'drivers_permit'
expect(subject.body).to include(
'<aa:DocumentCategoryCode>2</aa:DocumentCategoryCode>',
)
end
end

context 'when the type is an ID Card' do
it 'includes DocumentCategoryCode=3' do
applicant.state_id_data.state_id_type = 'state_id_card'
expect(subject.body).to include(
'<aa:DocumentCategoryCode>3</aa:DocumentCategoryCode>',
)
end
end

context 'when the type is something invalid' do
it 'does not add a DocumentCategoryCode for nil ID type' do
applicant.state_id_data.state_id_type = nil
expect(subject.body).to_not include('<aa:DocumentCategoryCode>')
end

it 'does not add a DocumentCategoryCode for invalid ID types' do
applicant.state_id_data.state_id_type = 'License to Keep an Alpaca'
expect(subject.body).to_not include('<aa:DocumentCategoryCode>')
end
end
end
end
end

describe '#headers' do
Expand Down