LG-7014 Device Profiling on the SSN page by theabrad · Pull Request #6694 · 18F/identity-idp

theabrad · 2022-08-04T18:08:11Z

Why? Updating the SSN page to enable LexisNexis ThreatMetrix device profiling in the form of a javascript embed.

Basic tests around inclusion / non-inclusion of the TM <script> tag.

Get _most_ of them passing

changelog: Upcoming Features, Device Profiling, Add ThreatMetrix device profiling to ssn page

zachmargolis · 2022-08-04T18:24:50Z

app/services/idv/steps/ssn_step.rb

+      def threatmetrix_session_id
+        SecureRandom.uuid
+      end


Can we guard it with a feature flag here as well so that we don't put a session ID on the page if the flag is off?

zachmargolis · 2022-08-04T18:25:47Z

app/views/idv/shared/_ssn.html.erb

  <div class="tablet:grid-col-8">
+    <% if IdentityConfig.store.proofing_device_profiling_collecting_enabled %>
+      <% unless IdentityConfig.store.lexisnexis_threatmetrix_account_id.empty? || updating_ssn %>
+        <%= f.hidden_field :threatmetrix_session_id, value: threatmetrix_session_id %>


For security/assurance purposes, what if we persisted this ID on the backend, in the session somewhere, instead of printing it to the page?

zachmargolis · 2022-08-04T18:26:10Z

config/initializers/content_security_policy.rb

+    'h-api.online-metrix.net',
+    'h.online-metrix.net',


let's only add these domains if profiling is enabled?

matthinz · 2022-08-04T18:26:44Z

config/initializers/content_security_policy.rb

    '*.nr-data.net',
    'dap.digitalgov.gov',
    '*.google-analytics.com',
+    'h-api.online-metrix.net',


This domain will only be accessed by the backend, and doesn't need to be in CSP

Suggested change

'h-api.online-metrix.net',

matthinz · 2022-08-04T18:28:35Z

config/initializers/content_security_policy.rb

    'dap.digitalgov.gov',
    '*.google-analytics.com',
+    'h-api.online-metrix.net',
+    'h.online-metrix.net',


Something I came across in testing is that when RAILS_ENV is not production, the CSP doesn't get applied.
If proofing_device_profiling_collecting_enabled is true, we should include h.online-metrix.net in CSP for non-production environments.

allow the threatmetrix_session to persist in flow session

matthinz · 2022-08-04T22:40:24Z

app/services/idv/steps/ssn_step.rb

        flow_session[:pii_from_doc][:ssn] = flow_params[:ssn]
+
+        if IdentityConfig.store.proofing_device_profiling_collecting_enabled
+          flow_session[:threatmetrix_session_id] = threatmetrix_session_id


Will this overwrite the existing :threatmetrix_session_id (if the user is coming back to edit their SSN)? I think we want to hang on to the original session ID in that case.

It seems like it would, yeah, which we probably want to avoid?

matthinz · 2022-08-04T22:43:07Z

config/initializers/content_security_policy.rb

+  if IdentityConfig.store.proofing_device_profiling_collecting_enabled
+    script_src << 'h.online-metrix.net'
+  end
+


After talking with @zachmargolis, I realized that I gave some bad guidance here. Let's not modify CSP in all environments: We want to ensure that we are not making calls to 3rd parties outside of production / staging. Apologies!

Suggested change

if IdentityConfig.store.proofing_device_profiling_collecting_enabled

script_src << 'h.online-metrix.net'

end

We'd still need some CSP revisions for this to work at all, wouldn't we? I would expect some script_src, connect_src, and iframe_src.

That's my understanding as well

I think we'll need the following:

script_src

connect_src

child_src

image_src

(I'll leave a followup ticket for CSP stuff)

Out of curiosity, why would image_src be needed? I follow the other ones since we have tags for script/iframe and I presume the script will make calls (therefore connect_src).

ThreatMetrix uses <img> tags as part of its profiling, loading up images sourced from h.online-metrix.net

Co-authored-by: Matt Hinz <matthinz@gmail.com>

aduth · 2022-08-05T12:32:29Z

app/services/idv/steps/ssn_step.rb

        flow_session[:pii_from_doc][:ssn] = flow_params[:ssn]
+
+        if IdentityConfig.store.proofing_device_profiling_collecting_enabled
+          flow_session[:threatmetrix_session_id] = threatmetrix_session_id


Will we want this to be present in the in-person proofing flow version of the SSN step as well?

That is a very good question, I am not 100% certain, we need to confirm

update: Let's include this on all instances of the SSN page and we can add more specific feature flags if needed

app/views/idv/shared/_ssn.html.erb

spec/views/idv/shared/_ssn.html.erb_spec.rb

…e-profiling-ssn

…y-idp into lg-7014-device-profiling-ssn

mitchellhenke · 2022-08-09T14:11:44Z

app/services/idv/steps/ipp/ssn_step.rb

        end
+
+        def threatmetrix_session_id
+          return nil if !IdentityConfig.store.proofing_device_profiling_collecting_enabled


Do we need to check IdentityConfig.store.proofing_device_profiling_collecting_enabled twice (here and in the call function)?

mitchellhenke · 2022-08-09T14:11:56Z

app/services/idv/steps/ssn_step.rb

      end
+
+      def threatmetrix_session_id
+        return nil if !IdentityConfig.store.proofing_device_profiling_collecting_enabled


Same as above, do we need to check IdentityConfig.store.proofing_device_profiling_collecting_enabled twice (here and in the call function)?

zachmargolis

LGTM!

app/services/idv/steps/ipp/ssn_step.rb

zachmargolis · 2022-08-09T21:25:17Z

app/services/idv/steps/ssn_step.rb

        flow_session[:pii_from_doc].nil?
      end

+      def updating_ssn


let's file a FIXME ticket and see if we can find a way to share code with these two SSN steps? In case we ever add a 3rd?

zachmargolis · 2022-08-09T21:29:59Z

app/views/idv/shared/_ssn.html.erb


+<% if IdentityConfig.store.proofing_device_profiling_collecting_enabled %>
+  <% unless IdentityConfig.store.lexisnexis_threatmetrix_org_id.empty? || updating_ssn %>
+    <script type="text/javascript" src="https://h.online-metrix.net/fp/tags.js?org_id=<%= IdentityConfig.store.lexisnexis_threatmetrix_org_id %>&session_id=<%= flow_session[:threatmetrix_session_id] %>">


Questions, maybe we can address in a follow-up PR later, but:

Do we need to worry about adding a nonce to this <script> tag so our CSP will allow it in prod? ex

What if we used our URIService.add_params helper so we don't have to manually interpolate?

Suggested change

<script type="text/javascript" src="https://h.online-metrix.net/fp/tags.js?org_id=<%= IdentityConfig.store.lexisnexis_threatmetrix_org_id %>&session_id=<%= flow_session[:threatmetrix_session_id] %>">

<script type="text/javascript" src="<%= URIService.add_params("https://h.online-metrix.net/fp/tags.js", org_id: IdentityConfig.store.lexisnexis_threatmetrix_org_id, session_id: flow_session[:threatmetrix_session_id]) %>">

Another thought, we may want to turn the domain into a config value but that's not needed right now

I'll create a follow up ticket for these.

aduth

Can we address the comment at #6694 (comment) ?

Avoid blocking

zachmargolis · 2022-08-11T19:49:01Z

spec/services/idv/steps/ipp/ssn_step_spec.rb

+    Idv::Flows::InPersonFlow.new(controller, {}, 'idv/in_person').tap do |flow|
+      flow.flow_session = {
+        pii_from_user: {},
+        threatmetrix_session_id: threatmetrix_session_id,


if this sets the session id... then what does the assertion check below? should we leave it blank here to make sure it gets assigned/

theabrad and others added 7 commits August 1, 2022 16:31

added threatmetrix domain to CSP

719b9a1

added script and noscript for device profiling

84ddc25

Add speculative tests for ThreatMetrix integration

01ebbda

Basic tests around inclusion / non-inclusion of the TM <script> tag.

Added session_id to ssn page

2908bc7

Update ThreatMetrix JS include tests

ddd0ef0

Get _most_ of them passing

Fix linting errors. Make tests pass

227a018

moved around logic for checking if TM org_id is available

e20904d

theabrad requested a review from a team August 4, 2022 18:08

update changelog

1b6047b

changelog: Upcoming Features, Device Profiling, Add ThreatMetrix device profiling to ssn page

zachmargolis reviewed Aug 4, 2022

View reviewed changes

matthinz reviewed Aug 4, 2022

View reviewed changes

theabrad added 2 commits August 4, 2022 16:31

added feature flags on csp and threatmetrix session id

32a885b

move threatmetrix_session_id into flow session

10ecc4e

allow the threatmetrix_session to persist in flow session

matthinz reviewed Aug 4, 2022

View reviewed changes

theabrad and others added 2 commits August 4, 2022 20:55

fixup

82ee43c

Update config/initializers/content_security_policy.rb

aec8ab8

Co-authored-by: Matt Hinz <matthinz@gmail.com>

aduth reviewed Aug 5, 2022

View reviewed changes

zachmargolis reviewed Aug 8, 2022

View reviewed changes

app/views/idv/shared/_ssn.html.erb Outdated Show resolved Hide resolved

zachmargolis reviewed Aug 8, 2022

View reviewed changes

spec/views/idv/shared/_ssn.html.erb_spec.rb Outdated Show resolved Hide resolved

zachmargolis reviewed Aug 8, 2022

View reviewed changes

spec/views/idv/shared/_ssn.html.erb_spec.rb Outdated Show resolved Hide resolved

theabrad added 5 commits August 8, 2022 15:54

changed lexisnexis_account_id to org_id

a14ee08

moved test expectations into methods

7060439

add threatmetrix_session_id to ipp flow

75516f4

Merge branch 'main' of github.com:18F/identity-idp into lg-7014-devic…

b6f5357

…e-profiling-ssn

Merge branch 'lg-7014-device-profiling-ssn' of github.com:18F/identit…

0272f7c

…y-idp into lg-7014-device-profiling-ssn

mitchellhenke reviewed Aug 9, 2022

View reviewed changes

theabrad added 2 commits August 9, 2022 12:19

dont create a new session id if updating_ssn

b2bf717

fixup

c940d22

zachmargolis approved these changes Aug 9, 2022

View reviewed changes

theabrad added 2 commits August 10, 2022 11:49

changed function to generate_threatmetrix_session_id

0f6ebc5

add tests for ssn_step

d09f775

aduth previously requested changes Aug 11, 2022

View reviewed changes

tests for ipp ssn_step

13b089e

zachmargolis reviewed Aug 11, 2022

View reviewed changes

fixed logic and tests

5ced292

stevegsa mentioned this pull request Aug 12, 2022

LG-7012 LG-7010 Add/Integrate new DDP Proofer #6684

Merged

aduth mentioned this pull request Aug 12, 2022

[LG-7191] add configuration flipper to suppress content security policy (CSP) #6715

Closed

theabrad merged commit fdce5a6 into main Aug 12, 2022

theabrad deleted the lg-7014-device-profiling-ssn branch August 12, 2022 15:41

	if IdentityConfig.store.proofing_device_profiling_collecting_enabled
	script_src << 'h.online-metrix.net'
	end

	<script type="text/javascript" src="https://h.online-metrix.net/fp/tags.js?org_id=<%= IdentityConfig.store.lexisnexis_threatmetrix_org_id %>&session_id=<%= flow_session[:threatmetrix_session_id] %>">
	<script type="text/javascript" src="<%= URIService.add_params("https://h.online-metrix.net/fp/tags.js", org_id: IdentityConfig.store.lexisnexis_threatmetrix_org_id, session_id: flow_session[:threatmetrix_session_id]) %>">

Conversation

theabrad commented Aug 4, 2022

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!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

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!

aduth left a comment • 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!

5 participants

aduth left a comment •

edited

Loading