-
Notifications
You must be signed in to change notification settings - Fork 16
Send Identity Verification Email [#1010] #1115
Conversation
… autouse override for just the tests where we want to turn on identity verification.
Start with identity_verification_required set to False for now until all the related pieces are in.
src/fidesops/ops/core/config.py
Outdated
@@ -38,6 +38,7 @@ class ExecutionSettings(FidesSettings): | |||
task_retry_count: int | |||
task_retry_delay: int # In seconds | |||
task_retry_backoff: int | |||
identity_verification_required: bool = False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am just setting a config variable for now, but this is subject to change with #1009. I think we should leave this as False until all the relevant pieces of this epic are in.
def _send_verification_code_to_user( | ||
db: Session, privacy_request: PrivacyRequest, email: Optional[str] | ||
) -> None: | ||
"""Generate and cache a verification code, and then email to the user""" | ||
verification_code: str = generate_id_verification_code() | ||
privacy_request.cache_identity_verification_code(verification_code) | ||
dispatch_email( | ||
db, | ||
action_type=EmailActionType.SUBJECT_IDENTITY_VERIFICATION, | ||
to_email=email, | ||
email_body_params=SubjectIdentityVerificationBodyParams( | ||
access_code=verification_code | ||
), | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified in local integration testing, after setting identity_verification_required to True and setting up a separate mailgun EmailConfig
, submitting a privacy request for an email address I own, sent the following to my email:
Your one-time code is XXXXXX. Hurry! It expires in 10 minutes.
@@ -10,4 +10,5 @@ export const SubjectRequestStatusMap = new Map<string, string>([ | |||
["In Progress", "in_processing"], | |||
["New", "pending"], | |||
["Paused", "paused"], | |||
["Unverified", "identity_unverified"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding these frontend elements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -212,7 +226,14 @@ def create_privacy_request( | |||
}, | |||
) | |||
queue_privacy_request(privacy_request.id) | |||
|
|||
except EmailDispatchException as exc: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's possible for this exception to be thrown when FIDESOPS__EXECUTION__IDENTITY_VERIFICATION_REQUIRED
is toggled off? if so we should check whether to enforce it in this or otherwise move this try / except
block closer to _send_verification_code_to_user
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we're triggering an email send anywhere else in the flow right now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll do it via tasks too as part of #1088, so maybe it's best to leave this for then
email_config=email_config, | ||
email=EmailForActionType( | ||
subject="Your one-time code", | ||
body=f"<html>Your one-time code is {pr.get_cached_verification_code()}. Hurry! It expires in 10 minutes.</html>", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to refer directly to config.redis.identity_verification_code_ttl_seconds
in the copy for this email — that way if that value is updated the emails will still show correct information
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like @TheAndrewJackson 's templating work covers this, I think there's just a little bit of merging effort, depending on whose PR goes in first. https://github.com/ethyca/fidesops/pull/1123/files#diff-aa27823caeff69b0d8b6124395820278bd5708e9a6ce0aa8521c5f5d5ad00f5eR36
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two minor changes:
- Update the email copy to directly refer to
config.redis.identity_verification_code_ttl_seconds
- Update the toggle var name to include
__SUBJECT_
to be clearer
Thank you for the review @seanpreston! |
…n_required for clarity.
Verified I received new email with Andrew's templating merge with:
^ Note this code is just for my local environment, nothing secure is pasted here. |
* If identity verification required, send email to the user with the verification code. * Adjust the identity_verification_required autouse fixture, and add an autouse override for just the tests where we want to turn on identity verification. * Add starting docs and updating the changelog. Start with identity_verification_required set to False for now until all the related pieces are in. * Update some of the docstrings. * Add unverified status color in the FE. * Add new privacy request status to types and constants. * Restore trailing comma. * Update identity_verification_required to subject_identity_verification_required for clarity. * Adjust email_body_params to accommodate new template. Co-authored-by: Sean Preston <sean@ethyca.com>
Purpose
If subject identity verification is turned on, have the user verify their identity before queuing the request for execution.
Changes
create_privacy_request
endpoint to check if identity verification is onidentity_unverified
Checklist
CHANGELOG.md
fileCHANGELOG.md
file is being appended toUnreleased
section in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.Run Unsafe PR Checks
label has been applied, and checks have passed, if this PR touches any external servicesTicket
Fixes #1010