Closed iamdharmesh closed 3 months ago
@iamdharmesh We'll also want to ensure the readmes get updated with information on how encryption works and the need to set the custom defines, with a warning that if this isn't done, decryption may fail (if the LOGGED_IN_SALT
changes) and the authentication flow will need to be gone through again
Thanks for the review @dkotter. I have addressed all feedback and updated the readmes with the encryption information. Could you please help to re-review once and let me know if any additional changes are needed here?
Thank you.
Noting here that once we get the wordpress.mailchimpapp.com service we'll need to make the following changes:
woocommerce.mailchimpapp.com
with wordpress.mailchimpapp.com
Log in and authorize
pop-up windowMailchimp for WooCommerce
text and link are updated to Mailchimp List Subscribe Form
and https://wordpress.org/plugins/mailchimp/ in the Log in and authorize
pop-up windowFor the logout process, no API is called to revoke the access token or disconnect the integration on the Mailchimp side. I checked the Mailchimp for WooCommerce plugin and didn’t find any API call for this. I also reviewed the API documentation but didn’t find any API for revoking tokens. Since Mailchimp uses non-expiring tokens, it is crucial to revoke the token upon logout.
@dkotter can you coordinate with @justin0440 / Vextras (in case we need to proxy through their wordpress.mailchimpapp.com service) to confirm what API should be used on logout to disconnect the respective WordPress site from the connected Mailchimp account?
Hello @iamdharmesh Thanks for the PR.
I have verified this PR in the enhancement/9
branch and is functioning as intended.
I tested the following on this branch:
https://github.com/user-attachments/assets/bd2cbdab-c33e-4c0e-9185-1ef5ab501340
Testing Environment
Steps to Test- As mentioned in the PR description. Test Results - It is working as expected. Functional Demo / Screencast - Special Notes - Ready for code review (Woo) Testing Document status: Cases related to this Issue/PR are added to the Critical Flow Wiki pages:
Note: Not related to this PR, I've found PHP warning, Can you please check from your end?
[07-Aug-2024 14:18:57 UTC] PHP Warning: Undefined array key "field" in /Users/sumitbagthariya/Local Sites/mailchimp/app/public/wp-content/plugins/wordpress/lib/mailchimp/mailchimp.php on line 190
@iamdharmesh Code looks good here and tests well, nice work!
I do have feedback on a few things that I think is worth addressing:
In simulating an access token failure (I just directly modified the token in the database so it was invalid) the forms on the front-end still show, though the forms won't work. My opinion here is we should hide those forms if we don't have valid credentials. Note this will need to work for a shortcode form, widget form and block form.
In testing what happens if decryption fails (I modified the salt we use to encrypt things) I noticed the block form and widget form don't show (there is a PHP warning attempt to read property datacenter on bool
that I'm wondering is what's causing those not to render) but the shortcode form does show. Ideally as above, we don't show any of those forms if we don't have valid credentials
This is more an edge case due to how I was testing but if I modified the access token, I get an admin message letting me know I need to re-authenticate. If I modify the access token to be valid again, that message still shows. Again, this is an edge case but seems like once we have a valid API call made, we should clear out that error message
The admin notice we show overlaps weird in our settings page. I'm assuming some minor markup or CSS changes would fix this and ensure the notice shows above our header:
Also wondering if we should change that notice from a warning to an error, since things won't work until the connection is reestablished.
^ The above are things I think we for sure should look at addressing. Here's a few things I'm raising for discussion purposes (cc / @jeffpaul @iamdharmesh):
For our encryption, we look for two custom constants first (MAILCHIMP_SF_ENCRYPTION_KEY
and MAILCHIMP_SF_ENCRYPTION_SALT
). This is documented in the readme but the reality here is most sites won't set these. We then try using some constants that are typically set across all WordPress sites (LOGGED_IN_KEY
and LOGGED_IN_SALT
). If these aren't set, we then fallback to a hardcoded string.
Our experience on a couple other projects has shown us that some security plugins will periodically rotate those core WordPress salts. This causes the decryption to no longer work for sites that aren't using the custom constants and will require the site to go through the authentication process again.
There's a couple options to consider here. We can leave this as-is, noting it is the most secure implementation and we have documentation on how to avoid this issue. Or we could remove the use of those core WordPress salts, meaning we would only rely on the custom constants or fallback to the hardcoded strings. This would prevent situations where decryption no longer works (so a better user experience) but is less secure for those that don't set those constants (which will probably be most sites)
For existing users, we allow them to continue to use their API key with no problems, which is the correct approach. There is no prompt though to update to the new connection method. Wondering if we want to show an admin notice letting them know there's a new authentication method they should update to? Not sure how hard we want to push that, especially initially. I think the ideal would be to get everyone using OAuth but maybe this is something we add in a follow up release?
I agree that some sort of admin notice, sigh adding another one to folks already likely clogged up admin notices, to direct folks who are using the API Key to disconnect and reconnect via OAuth would be ideal and within this release.
There's a couple options to consider here. We can leave this as-is, noting it is the most secure implementation and we have documentation on how to avoid this issue. Or we could remove the use of those core WordPress salts, meaning we would only rely on the custom constants or fallback to the hardcoded strings. This would prevent situations where decryption no longer works (so a better user experience) but is less secure for those that don't set those constants (which will probably be most sites)
I think its fine to leave as-is. Ensuring we've got a good FAQ item in the readme as well as details for the Woo.com product documentation on the topic will be helpful to direct folks to that run into the core salts being rotated and then, finally, properly adding the MC constants instead.
Thanks for the detailed feedback @dkotter. I have address those in https://github.com/mailchimp/wordpress/pull/47/commits/57c0058b8ea70d13a3970992a25fc9bfc4e6b2b4.
I agree that some sort of admin notice, sigh adding another one to folks already likely clogged up admin notices, to direct folks who are using the API Key to disconnect and reconnect via OAuth would be ideal and within this release.
Thanks for confirming @jeffpaul. Could you please help with adjust the wording of the notice on how hard we want to push the users to switch to OAuth. eg: is deprecated vs is going to deprecated in future.
(Heads up! It looks like you're using an API key to connect with Mailchimp, which is now deprecated. Please log out and reconnect your Mailchimp account using the new OAuth authentication by clicking the "Connect Account" button.)
For encryption (#2
), @dkotter I am more inclined towards keeping it as it is.
Thank you.
@iamdharmesh I'm fine with that existing notice noting deprecated.
@iamdharmesh I'm fine with that existing notice noting deprecated.
@jeffpaul This is added in https://github.com/mailchimp/wordpress/pull/47/commits/41757747b0c5453996822eb5aaac3b95e2860ab6
For the logout process, no API is called to revoke the access token or disconnect the integration on the Mailchimp side. I checked the Mailchimp for WooCommerce plugin and didn’t find any API call for this. I also reviewed the API documentation but didn’t find any API for revoking tokens. Since Mailchimp uses non-expiring tokens, it is crucial to revoke the token upon logout.
@dkotter can you coordinate with @justin0440 / Vextras (in case we need to proxy through their wordpress.mailchimpapp.com service) to confirm what API should be used on logout to disconnect the respective WordPress site from the connected Mailchimp account?
Sounds like there isn't a public facing API to do this, so the approach we're taking here is the best we can do for now
Description of the Change
The PR replaces the existing API key-based authentication flow with OAuth authentication to improve the overall connection flow experience. Here are the details of the tasks covered in this PR.
mailchimpapp.com
service to generate an access tokenCloses #9
How to test the Change
New setup:
Existing Users
Changelog Entry
Credits
Props @jeffpaul @dkotter @iamdharmesh
Checklist: