Skip to content

Log email sending and check for PII when sending asynchronous email#7155

Merged
mitchellhenke merged 6 commits intomainfrom
mitchellhenke/email-logging-and-pii-checking
Oct 19, 2022
Merged

Log email sending and check for PII when sending asynchronous email#7155
mitchellhenke merged 6 commits intomainfrom
mitchellhenke/email-logging-and-pii-checking

Conversation

@mitchellhenke
Copy link
Contributor

@mitchellhenke mitchellhenke commented Oct 17, 2022

🛠 Summary of changes

Builds on #7106 to check for PII when sending async emails since they get saved in the database!

I don't love the approach of instance_variable_get and instance_variable_set, and would very much have preferred to use the instrumented deliver method from ActionMailer, but it doesn't include the arguments given to the Mailer (like user, which we want to log, or the action the mail was derived from).

The only other hook that exists after the mail is delivered is the email observer delivered_email. A list of all the hooks in the order that they happen:

  # UserMailer#before_action
  # UserMailer#after_action
  # IdentityMailLogSubscriber#process
  # EmailDeliveryObserver.delivering_email
  # Email actually sent
  # IdentityMailLogSubscriber#deliver
  # EmailDeliveryObserver.delivered_email

@mitchellhenke mitchellhenke marked this pull request as draft October 17, 2022 16:01
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/email-logging-and-pii-checking branch 2 times, most recently from 8280a26 to 1f57afd Compare October 18, 2022 21:17
@mitchellhenke mitchellhenke marked this pull request as ready for review October 18, 2022 22:02
@mitchellhenke mitchellhenke force-pushed the mitchellhenke/email-logging-and-pii-checking branch from 1f57afd to a5aec9a Compare October 18, 2022 22:31
Comment on lines 8 to 10
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 what if we only applied the lint to UserMailer and its subclasses instead of ApplicationMailer?

Copy link
Contributor Author

@mitchellhenke mitchellhenke Oct 19, 2022

Choose a reason for hiding this comment

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

I think that'd make sense, I don't know how to do that off-hand from a brief look at the current linter.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok! I'll poke around at it for a follow-up

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

Choose a reason for hiding this comment

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

I just remembered we can use prepend to avoid alias_method dances in #7165

So here we could define somewhere the file:

module DeliverLaterArgumentChecker
  def deliver_later(...)
    MailerSensitiveInformationChecker.check_for_sensitive_pii!(@params, @args, @action)
    super(...)
  end
end

and turn this into:

Suggested change
alias_method :original_deliver_later, :deliver_later
prepend DeliverLaterArgumentChecker

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for reminding me, added in b8f658b

@mitchellhenke mitchellhenke force-pushed the mitchellhenke/email-logging-and-pii-checking branch from a5aec9a to fa51717 Compare October 19, 2022 15:08
@mitchellhenke mitchellhenke merged commit 8bb355a into main Oct 19, 2022
@mitchellhenke mitchellhenke deleted the mitchellhenke/email-logging-and-pii-checking branch October 19, 2022 16:05
This was referenced Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants