Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mailchimp Transactional Consent Connector [#2150] #2194

Merged
merged 11 commits into from
Jan 13, 2023

Conversation

pattisdr
Copy link
Contributor

@pattisdr pattisdr commented Jan 10, 2023

Closes #2150
👉 Note this is against the consent-propagation branch not main!

Code Changes

  • Adds a new consent_requests top-level header to saas configs - currently a single opt_in request and/or opt_out request can be defined. Subject to change as we generalize to other connectors.
  • Add a new Mailchimp Transactional saas connector that has the ability to make consent requests by adding or removing users from the allowlist
  • Add the ability to the existing ApiKeyAuthenticationStrategy to place an api_key in the request body in addition to the already supported query params or headers
  • Expand the saas_connector.run_consent_request to be able to pull either the opt-in or opt-out request off of the saas config and fire. We are separately adding restrictions that only one consent preference will be passed to the Privacy Request as "executable" to start.

Steps to Confirm

  • In your privacy center config.json, set both advertising and improve executable fields to false, leaving only advertising.first_party as something that is executable. Allison is further refining executable logic here but this will work for now
  • Run nox -s test_env
  • Go to your admin ui and add a new Mailchimp Transactional connector. You'll need an API key.
  • Go to the Privacy Center and update your consent preferences (in particular, pay attention to whether you're opting in or opting out of "Email Marketing".
  • Return to the admin UI and approve the latest privacy request that was created to propagate consent preferences to third party systems
  • Privacy request should complete quickly with a completed status
  • Check the docker logs in the fides container. As we're in dev mode, you should see evidence of the request made to the Mailchimp Transactional API (mandrill). For example, if I'm opting out:
-----------SAAS REQUEST-----------
POST https://mandrillapp.com/api/1.0/allowlists/delete
headers: {'Content-Type': 'application/json', 'Content-Length': '71'}
body: {"email": "REDACTED", "key":  REDACTED"}
response: b'{"email":"REDACTED","deleted":true}'

Pre-Merge Checklist

Description Of Changes

Add the ability for the SaaS Connector Framework to make consent requests, and demonstrate this end to end in the form of a Mailchimp Transactional connector.

The saas config format is very early and subject to change as we need to generalize to connect to more services.

…r transactional api requests.

Update saas_connector.run_consent_request to select the appropriate opt_in or opt out request depending on whether the consent preference is opt in or opt out.   There is ongoing work to update that consent preferences will have an exectuable flag on them, and only one will be executable.

For now to test, set executable to false for two preferences.
Copy link
Contributor Author

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

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

No tests yet, still mulling over some details, but this demonstrates making a consent request to the mailchimp transactional API

… connector. It's different enough to warrant it's own connector. There are different ways on authenticating the request, different domains, different credentials for this connector.
…equests.

Remove stubbed test_consent_request now that the saas connector is wired to make real requests.
…single opt in or single opt out request instead of a list of opt-in or opt-out. A lot of this is subject to change, so let's just the simplest case working for Mailchimp Transactional and then survey the landscape more broadly.
@pattisdr pattisdr changed the title [DRAFT] Mailchimp Consent Connector [#2150] Mailchimp Transactional Consent Connector [#2150] Jan 11, 2023
@pattisdr pattisdr added the run unsafe ci checks Runs fides-related CI checks that require sensitive credentials label Jan 11, 2023
Comment on lines +29 to +44
consent_requests:
opt_in:
method: POST
path: /allowlists/add
body: |
{
"email": "<email>"
}

opt_out:
method: POST
path: /allowlists/delete
body: |
{
"email": "<email>"
}
Copy link
Contributor Author

@pattisdr pattisdr Jan 11, 2023

Choose a reason for hiding this comment

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

Format taken from here: #2144 (comment), except I'm making an opt_in/opt_out distinction. This is just a tentative format to have something to demonstrate, I need to do a more broad survey in #2144.

Right now (design decision by Neville) the Privacy Center will restrict so only one consent preference can be considered executable at any given time, so by the time this gets to consent request execution, there will be at most one consent preference, with opt-in or opt-out that we will fire accordingly.

There is currently no logic linking which requests are made for which types of data uses / purposes of processing, but this is part of a broader discussion about where this should happen between multiple teams.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the opt_in/opt_out distinction, it makes the purpose of these requests very clear

Comment on lines +1 to +5
dataset:
- fides_key: <instance_fides_key>
name: Mailchimp Transactional Dataset
description: A sample dataset representing the Mailchimp Transactional connector for Fides
collections: []
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Placeholder dataset for now, as we don't yet have a need for collections to be defined, although this is subject to change.

Comment on lines +8 to +22
connector_params:
- name: domain
default_value: mandrillapp.com/api/1.0
- name: api_key

client_config:
protocol: https
host: <domain>
authentication:
strategy: api_key
configuration:
body: |
{
"key": "<api_key>"
}
Copy link
Contributor Author

@pattisdr pattisdr Jan 11, 2023

Choose a reason for hiding this comment

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

Making Mailchimp Transactional a separate connector as it is a separate Mailchimp addon. Mailchimp purchased Mandrill, and the connector params and client config and test calls are different than the base mailchimp connector.

Comment on lines +64 to +75
if self.body:
body_with_api_key: Optional[str] = assign_placeholders(
self.body, secrets or {}
)
api_key_request_body: Dict = json.loads(body_with_api_key or "{}")

original_request_body: Dict = json.loads(request.body or "{}")
original_request_body.update(api_key_request_body)

request.prepare_body(
data=json.dumps(original_request_body), files=None, json=True
)
Copy link
Contributor Author

@pattisdr pattisdr Jan 11, 2023

Choose a reason for hiding this comment

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

I added this logic here to take the original request body and add an API key to it if specified in the saas config.

I suspect I'd run into issues if the original request body wasn't json, but I think we're building json bodies generally in saas connectors?

Mailchimp Transactional connector adds API keys to the request body. Alternatively, I could put the <api_key> in the body directly in the opt in and opt out requests (this was in the original draft), so this isn't needed, but it seemed like I should separate out what was the authentication piece.

Copy link
Contributor

Choose a reason for hiding this comment

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

This makes sense, we originally didn't implement the body because we had concerns about how an authentication body would merge with a request body but it's good to see this working as expected here.

Copy link
Contributor

Choose a reason for hiding this comment

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

agreed, i like this implementation!

@pattisdr
Copy link
Contributor Author

pattisdr commented Jan 11, 2023

argh nox -s static_checks passed, but I still have an unused import. I'll push that change after I get comments. Also, is there any way to unskip these tests here from github actions UI?

@pattisdr pattisdr marked this pull request as ready for review January 11, 2023 17:56
@pattisdr pattisdr requested review from adamsachs, galvana, NevilleS and allisonking and removed request for galvana January 11, 2023 17:56
Copy link
Contributor

@adamsachs adamsachs left a comment

Choose a reason for hiding this comment

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

@pattisdr really nice work! it all looks solid to me, don't think i have any comments on the application code itself. i'm sure we'll need to adjust as we go a bit, but this is a great start!

just a few relatively minor nits within the tests.

@pattisdr
Copy link
Contributor Author

Thanks so much @adamsachs and @galvana for your comments here

…ion test that opt-out consent request has removed the user from the allowlist
@pattisdr pattisdr added run unsafe ci checks Runs fides-related CI checks that require sensitive credentials and removed run unsafe ci checks Runs fides-related CI checks that require sensitive credentials labels Jan 12, 2023
…connector

# Conflicts:
#	tests/ops/conftest.py
Add logging for executable consent options.
@pattisdr
Copy link
Contributor Author

Failing ctl-external tests unrelated. Turned on the saas tests, verified mailchimp transactional tests working here.

@pattisdr pattisdr merged commit bad52aa into consent-propagation Jan 13, 2023
@pattisdr pattisdr deleted the fides_2150_mailchimp_consent_connector branch January 13, 2023 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
run unsafe ci checks Runs fides-related CI checks that require sensitive credentials
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants