LG-12858 - Add new vtr option (enhanced ipp)

[gina-yamada]

🎫 Ticket

LG-12858 Implementation: Add new Vector of Trust Option on Sinatra for F&F testing

🛠 Summary of changes

• Set eipp_allowed in staging in oidc-sinatra*
• Set eipp_allowed to false in all other environments in oidc-sinatra*
  * I think this is achieved. Please see changes in .env.example and config.rb
• Added Enhanced In-Person Proofing option to the ial (level of service) drop down conditionally (to display in staging only)
• Mapped new ial (enhanced-ipp-required) to vtr_values P1.Pe (P1 : Identity proofing is performed Pe :Enhanced proofing)
• Mapped new ial to arc_value

📜 Testing Plan

1. Pull down branch keithw/LG-13235-consume-eipp-data-from-sinatra from idp. Open file app/services/service_provider_request_handler.rb and add puts sp_session on the first line in decorated_sp_session.
2. Start server on branch keithw/LG-13235-consume-eipp-data-from-sinatra
3. Pull down this branch locally.
4. Open .env and ensure eipp_allowed is set to false
5. Start the server.
6. Visit http://localhost:9292/
7. Click on the Level of Service drop down. You should not see Enhanced In-Person Proofing as an option.
8. Kill the server. Change the value of eipp_allowed to true. Start the server.
9. Click on the Level of Service drop down. You should see Enhanced In-Person Proofing as an option now.
10. Change the Level of Service drop down to Enhanced In-Person Proofing and click sign in.
11. You should be taken to Login locally
12. Inspect the identity-oidc-sinatra terminal. You should see the vtr (vector of trust contains value C1.P1.Pe (Pe= enhanced proofing) and that the ial is set to enhanced-ipp-required
13. Inspect the identity-idp terminal for the log you set up in testing plan 1. You should see that vrt is added to the session (that is Keith's PR) and that ["C1.P1.Pe"] is set (look at last line of log).
14. Visit http://localhost:9292/ again.
15. Confirm we are not changing existing behavior. Pick other options in the Level of Service drop down. Ensure that you are not seeing C1.P1.Pe value be passed in when Enhanced In-Person Proofing is not selected (ie you should see Pb for biometrics rather than Pe) and that the ial set is not enhanced-ipp-required. (Step 12 and 13)

📷 Screenshots of env variable set in Cloud.gov [env]-identity-oidc-sinatra

[zachmargolis]

1. I'd drop the is_ and name this with a ? to match the previous comment
2. No need for the final assignment, the result is just the value

Suggested change

	def is_eipp_allowed()
	env = ENV['idp_environment']
	is_eipp_allowed = (env == 'dev' || env == 'staging')
	end
	def eipp_allowed?
	env = ENV['idp_environment']
	env == 'dev' || env == 'staging'
	end

[zachmargolis]

Another thought: hardcoding specific environment names in source code is a 12-factor app antipattern, could we add a new config for ENV['eipp_allowed'] and set that in the appropriate environments?

[gina-yamada]

@zachmargolis I think that is a better solution. Resolved with commit 97fd794

This repository is set up different than idp and is new territory for me. I paired with others and this is what we came up with. Please double check that I set up the variable accurately to only display the eipp option in staging once deployed. (It works locally when I edit the value of idp_enviroment in .env). I believe I do not need to do anything else, is that correct?

Thank you!

[eileen-nava]

I tested this locally with Keith's branch from identity-idp and it worked as expected. LGTM!

(Caveat: I'm still getting back into the swing of things on Joy, which means it might be helpful to have another Joy developer review this, too.)

[gina-yamada]

@KeithNava @eileen-nava I am re-requesting a review because I made what I think are fairly large changes to the conditional logic of the drop down. Pay extra attention to the last commit (see default_config func in config.rb). I think this is how we should go about this but not 100% confident. Please review and fire up again. The testing plan has been updated. Thanks!

[zachmargolis]

LGTM with some suggestions

[eileen-nava]

I tested this locally and it worked well. Approved.

[eileen-nava]

I’d expect this method to return nil when it returns from line 276. I know there's a Ruby style convention that if a method returns a boolean, then the method should end with ?. Should a method also end with ? if it returns a boolean or nil?

[gina-yamada]

I am now returning false, see commit d9607e4

[zachmargolis]

maybe we add an explicit false here to address Eileen's comment?

Suggested change

	return unless config.vtr_disabled?
	return false unless config.vtr_disabled?

[gina-yamada]

Done. Resolved with commit d9607e4

[gina-yamada]

@eileen-nava @KeithNava This is ready for a final review! Sorry for the delay.

I updated the solution to use env variable set in Cloud.gov as suggested by @zachmargolis. 🙏 💯 I added screenshots above for each env so you can see how "eipp_allowed" is set in each environment. I also updated the testing instructions to fit this new approach.

I request review again primary to ensure how I am fetching that env variable is accurate. It should default to false (even though I set in all envs) and I am expecting it to come in as a string. I was using vtr_disabled as an example.

Thanks again.