Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Send Identity Verification Email [#1010] #1115

Merged
merged 11 commits into from
Aug 23, 2022

Conversation

pattisdr
Copy link
Contributor

@pattisdr pattisdr commented Aug 19, 2022

Purpose

If subject identity verification is turned on, have the user verify their identity before queuing the request for execution.

Changes

  • Update existing create_privacy_request endpoint to check if identity verification is on
    • If yes, set privacy request status to identity_unverified
    • Caches a 6 digit code in Redis that lasts for ten minutes
    • Emails that code to the user
    • Suspends further processing of that request.
  • To resume where we left off, send a POST to /privacy-request/{{privacy_request_id}}/verify added here Send Identity Verification Email [#1010] #1115

Checklist

  • Update CHANGELOG.md file
    • Merge in main so the most recent CHANGELOG.md file is being appended to
    • Add description within the Unreleased 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.
    • Add a link to this PR at the end of the description with the PR number as the text. example: #1
  • Applicable documentation updated (guides, quickstart, postman collections, tutorial, fidesdemo, database diagram.
  • If docs updated (select one):
    • documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
    • documentation issue created (tag docs-team to complete issue separately)
  • Good unit test/integration test coverage
  • This PR contains a DB migration. If checked, the reviewer should confirm with the author that the down_revision correctly references the previous migration before merging
  • The Run Unsafe PR Checks label has been applied, and checks have passed, if this PR touches any external services

Ticket

Fixes #1010

@pattisdr pattisdr changed the title Send Identity Verification Email [#1010] [Draft] Send Identity Verification Email [#1010] Aug 19, 2022
… 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.
@pattisdr pattisdr changed the title [Draft] Send Identity Verification Email [#1010] Send Identity Verification Email [#1010] Aug 22, 2022
@@ -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
Copy link
Contributor Author

@pattisdr pattisdr Aug 22, 2022

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.

Comment on lines 260 to 274
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
),
)

Copy link
Contributor Author

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.

@pattisdr pattisdr marked this pull request as ready for review August 22, 2022 16:45
@@ -10,4 +10,5 @@ export const SubjectRequestStatusMap = new Map<string, string>([
["In Progress", "in_processing"],
["New", "pending"],
["Paused", "paused"],
["Unverified", "identity_unverified"],
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't have a choice! If a privacy request exists in the database, and it had this new request status, you'd get a UI error on the privacy request page that made it unusable.
Screen Shot 2022-08-22 at 12 35 35 PM

@@ -212,7 +226,14 @@ def create_privacy_request(
},
)
queue_privacy_request(privacy_request.id)

except EmailDispatchException as exc:
Copy link
Contributor

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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>",
Copy link
Contributor

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

Copy link
Contributor Author

@pattisdr pattisdr Aug 23, 2022

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

Copy link
Contributor

@seanpreston seanpreston left a 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

@pattisdr
Copy link
Contributor Author

Thank you for the review @seanpreston!

@seanpreston seanpreston self-assigned this Aug 23, 2022
@pattisdr
Copy link
Contributor Author

pattisdr commented Aug 23, 2022

Verified I received new email with Andrew's templating merge with:

Your privacy request verification code is 442512. Please return to the Privacy Center and enter the code to continue. This code will expire in 10 minutes

^ Note this code is just for my local environment, nothing secure is pasted here.

@seanpreston seanpreston merged commit 7193f3c into main Aug 23, 2022
@seanpreston seanpreston deleted the fidesops_1010_validation_email branch August 23, 2022 20:34
sanders41 pushed a commit that referenced this pull request Sep 22, 2022
* 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>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Backend - generate ID verification code
2 participants