Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Hello!
The aes module is currently used by salesforce_api.module.
I recently did a review of both Encrypt and AES and decided to use Encrypt instead.
I think it's worth considering if Salesforce could instead use Encrypt.
* They both provied AES (though in Encrypt it's called Mcrypt AES 256
* Encrypt has simpletests
* AES has some code that seem questionable to me - #1800166: Make it more clear the "view passwords" permission would allow someone to take over the site, #1799546: Don't use $_POST to check for changing variables, #1762716: separate password viewing feature from rest of module
I did not do an exhaustive review of where Salesforce is using AES. If there are other features of AES that Salesforce needs please comment about them here.
Comments
Comment #1
kostajh CreditAttribution: kostajh commentedI don't have any objections to this. I would need to check but I think it would be fairly straightforward to support either.
Comment #2
kostajh CreditAttribution: kostajh commentedThis won't be done in 7.x-2.x. I don't think 7.x-3.x will utilize either AES or Encrypt.
Comment #3
seanBI've created a patch to use the encrypt module when storing the consumer_key / consumer_secret / refresh_token (only if it is installed, it is not a dependency). When adding encrypt module later, it's is necessary to enter the consumer_key and consumer_secret of the application again (since they need to be encrypted).
Would you consider adding this?
Comment #4
mariacha1 CreditAttribution: mariacha1 at ThinkShout commentedThis looks good, and could be helpful for saving token data in features, so that's great.
Side note that this will require us to be disciplined with our use of
new Salesforce()in the future. We should always be usingsalesforce_get_api()anyway, as that will be easier for writing tests.We'll try to get this into the next release, which should be out soon. Marking as RTBC for now.
Comment #5
mariacha1 CreditAttribution: mariacha1 at ThinkShout commentedComment #7
mariacha1 CreditAttribution: mariacha1 at ThinkShout commentedComment #8
gregglesThanks, @mariacha1!
Comment #9
mariacha1 CreditAttribution: mariacha1 at ThinkShout commentedEncrypt has a D8 release. Determine if we want to patch this to the D8 version.
Comment #10
seanBSweet! Thnx!
Comment #12
gregglesBack to needs work on 7.x-2.x I guess?
Comment #13
seanBTo explain the issue:
The salesforce module uses salesforce_get_api() and getRefreshToken() in some access callbacks. If the data is not encrypted in the DB yet, this could cause every page to return an error. Which makes it hard to fix it, since you can't access the form to store your key and secret.
The encrypt module throws an error if the decrypt fails (not ecrypted yet, wrong key or something like that). There are different reasons decryption could fail. The try/catch from the patch in issue https://www.drupal.org/node/2773237 could solve this. I guess the alternative would be to also save a 'encrypt' flag for the module. We could use that to check if we need to decrypt. This will only solve the error 'not ecrypted yet', but I guess other errors are not a problem that should be handled by the salesforce module (probably Encrypt config errors).
A patch is attached for the new approach using a flag. Please review!
Something to consider:
The module could still cause a problem on every pageload because of the access check, making the site unusable. So that could be a reason to still go for the try/catch approach. Input would be welcome.
Comment #14
seanBOne little extra change, we probably need to use 2 flags (1 for the key/secret and 1 for the refreshtoken), since they are both set at different times. I think the flag also removes the need to save the key/secret again after installing the encrypt module.
Comment #15
seanBHere is a new patch fixing a authorization issue in last patch (key/token got encrypted before sending when saving the form). I think this should be it!
Comment #16
mariacha1 CreditAttribution: mariacha1 at ThinkShout commentedHi everyone,
Sorry for the delay in reviewing this. I think the patch here is likely pretty sound, but, even though it's the idea I suggested, the variable to see if encrypt is implemented, plus the "if"s to check for the encrypt module isn't a terribly clean solution.
There's now a new issue: #2808033: Allow custom modules to manage salesforce keys that I believe could be used to accomplish what's desired here in a much cleaner way. Anyone want to take a look at that issue and see if you agree? If so, let's work on getting that one reviewed and committed.