Conversation
|
| FACIAL_MATCH_OPT_IN = %w[unsupervised_with_selfie].to_set.freeze | ||
|
|
||
| # IPP proofing levels | ||
| IPP_PROOFING_IDV_LEVELS = %w[in_person proofing_agent legacy_in_person].to_set.freeze |
There was a problem hiding this comment.
This is like AC current and ATM machine 😄
I suggest IN_PERSON_PROOFING_IDV_LEVELS.
There was a problem hiding this comment.
Also wondering if a better concept/construct is that what the service provider wants is pure facial_match as opposed to rejecting IPP. In other words, use FACIAL_MATCH_OPT_IN although that seems to be an awful name.
There was a problem hiding this comment.
I think the issue is that it wants facial match but we want to ONLY require users that were IPP proofed to be reproofed. So yes we can say if its facial match we should reproof but it would catch all users that are proofed already right?
| issuer == IdentityConfig.store.reproof_forcing_service_provider && | ||
| initiating_service_provider&.issuer != IdentityConfig.store.reproof_forcing_service_provider | ||
| def needs_to_reproof?(user_profile) | ||
| Idv::ReproofRequiredPolicy.new( |
There was a problem hiding this comment.
I don't see the value in creating a separate Policy class; I feel that the responsibility could still reside with ServiceProvider. We should get other opinions.
There was a problem hiding this comment.
I disagree. I think it makes it a little too noisy when its in the service provider and it can get confusing of whether ur talking about the current service provider or the service provider associated with the profile.
There was a problem hiding this comment.
I see @mdiarra3's point--I think its good to have a separate policy because for maintainability. It is easier to update logic about reproofing separately from SP characteristics.
There was a problem hiding this comment.
I think I'm good with having separate policy classes, too.
There was a problem hiding this comment.
If we are going with a separate Policy class, my concern is that Idv::ReproofRequiredPolicy doesn't take on the full responsibility. I would rename it to Idv::ServiceProviderBasedReproofingPolicy or similar. There is a similar convention with ServiceProviderMfaPolicy.
|
The PR instruction's testing plan needs to be updated to remove |
| issuer == IdentityConfig.store.reproof_forcing_service_provider && | ||
| initiating_service_provider&.issuer != IdentityConfig.store.reproof_forcing_service_provider | ||
| def needs_to_reproof?(user_profile) | ||
| Idv::ReproofRequiredPolicy.new( |
There was a problem hiding this comment.
If we are going with a separate Policy class, my concern is that Idv::ReproofRequiredPolicy doesn't take on the full responsibility. I would rename it to Idv::ServiceProviderBasedReproofingPolicy or similar. There is a similar convention with ServiceProviderMfaPolicy.
vrajmohan
left a comment
vrajmohan
left a comment
There was a problem hiding this comment.
Looks great. Thank you for your patience.
joshuaalley25
left a comment
joshuaalley25
left a comment
There was a problem hiding this comment.
Thank you for adding the logging
4 participants
🎫 Ticket
Link to the relevant ticket:
LG-17325
🛠 Summary of changes
Introduces Idv::ReproofRequiredPolicy to consolidate and extend reproofing logic. Previously, ServiceProvider#needs_to_reproof? only handled the reproof-forcing SP case inline. This PR extracts that logic into a dedicated policy class and adds support for IPP (In-Person Proofing) reproofing, gated behind a feature flag.
📜 Testing Plan
New config keys:
reproof_non_facial_match_service_providers(JSON array) added to IdentityConfig.Test Locally: