LG-12275 | Fixes opt-in IPP page displaying incorrectly#10016
Conversation
…red to participating SPs.
Each env has an OIDC and SAML test app. I assume these could be opted into IPP separately. https://dashboard.dev.identitysandbox.gov/env |
spec/support/features/idv_helper.rb
Outdated
| elsif sp == :oidc | ||
| @state = SecureRandom.hex | ||
| @client_id = sp_oidc_issuer | ||
| @client_id = client_id_override || sp_oidc_issuer |
There was a problem hiding this comment.
I think I am going to split this out into another PR later.
I think this was a good idea, but IdvHelper ends up just passing strings around, not instantiating a real ServiceProvider, so the hardcoded redirect and nonce don't work. I'd like to make it operate on an actual ServiceProvider object, to more easily allow passing in an override.
changelog: Bug Fixes, In Person Proofing, Opt-in is now only offered to participating SPs
n1zyy
changed the title
|
@n1zyy I pushed up more tests and inlined preconditions. I think we have every case covered in idv/doc_auth/how_to_verify_spec -- including the most important case: ipp enabled, opt-in ipp enabled, where sp did not opt in. |
|
|
||
| complete_how_to_verify_step(remote: true) | ||
| expect(page).to have_current_path(idv_hybrid_handoff_url) | ||
| complete_how_to_verify_step(remote: true) |
There was a problem hiding this comment.
we should add another step where remote is false. Perhaps instead of an additional test we can go back after line 141 and then redo w/ remote = false
There was a problem hiding this comment.
Good catch, and thanks for suggesting a much easier option than I would have thought of. 👼
There was a problem hiding this comment.
thanks for adding this additional test!
| end | ||
|
|
||
| describe 'navigating to How To Verify from Agreement page in 50/50 state' do | ||
| describe 'navigating to How To Verify from Agreement page in 50/50 state |
There was a problem hiding this comment.
do we need to mock opt in enabled for the service_provider for this spec?
There was a problem hiding this comment.
I didn't realize this when we were discussing this on a call, but we have this covered further below. We test it both ways: 171-172 tests the opt-in provider case, and then further down on 180 we test with the opt-in feature flag disabled.
| include InPersonHelper | ||
| org = 'test_org' | ||
|
|
||
| let(:ipp_service_provider) { create(:service_provider, :active, :in_person_proofing_enabled) } |
There was a problem hiding this comment.
perhaps we should add a case where the In_person_proofing_enabled is false
n1zyy
left a comment
n1zyy
left a comment
There was a problem hiding this comment.
Thanks Shannon for your review! I appreciate the thoughtful feedback on how to expand the tests. Just pushed up some expanded coverage.
| end | ||
|
|
||
| describe 'navigating to How To Verify from Agreement page in 50/50 state' do | ||
| describe 'navigating to How To Verify from Agreement page in 50/50 state |
There was a problem hiding this comment.
I didn't realize this when we were discussing this on a call, but we have this covered further below. We test it both ways: 171-172 tests the opt-in provider case, and then further down on 180 we test with the opt-in feature flag disabled.
|
|
||
| complete_how_to_verify_step(remote: true) | ||
| expect(page).to have_current_path(idv_hybrid_handoff_url) | ||
| complete_how_to_verify_step(remote: true) |
There was a problem hiding this comment.
Good catch, and thanks for suggesting a much easier option than I would have thought of. 👼
|
I re-triggered the failing spec from your last push. Pipeline just passed. Code changes look good. I will smash approve if we don't get a third party to review soon |
|
LGTM! Thanks for adding the additional specs! Also the doc Gina created to go through how to test was super helpful! |
5 participants
🎫 Ticket
Link to the relevant ticket:
LG-12275
🛠 Summary of changes
We identified a bug in how we were checking for opt-in. We checked the relevant feature flags, but were not properly checking whether the ServiceProvider had enabled IPP, causing the screen to be displayed inappropriately. Users were not able to proceed if the SP did not have IPP enabled, which prevented any serious adverse impact to partners, but also lead the user into a dead-end.
📜 Testing Plan
Directly, this is tested by unit tests that should assert and prove correct behavior.
NOTE: With this change, it will no longer be possible to just go to
/verifyand test the opt-in process, because there is no ServiceProvider set. You'll have to come in through Sinatra.For manual testing, unless we have multiple sinatra-oidc instances I don't know about, it might be easiest to just have an engineer hop into the Rails console and toggle the setting. You will likely want something like: