One of the advantages with Stripe is that theoretically, the credit card number doesn't have
to go through the merchant's less secure web server. The current implementation of uc_stripe
uses the default payment gateway functionality of ubercart and the credit card does have to
pass through the merchant's drupal installation and get stored in a session.

One way around this is to do what uc_authorize_simdpm does and create another payment method
that by-passes Ubercarts less secure payment gateway feature. The DPM method in that module
is a great example of what uc_stripe could do. It would be awesome if uc_stripe
could implement this method because then the merchant's would qualify for SAQ-A and not need
all the expensive security scanning and auditing. As it is, the system needs all the auditing that
any other payment gateway like Authorize.net AIM would require.

Comments

wizonesolutions’s picture

Yeah, or just implement Stripe.js, in which case using a credit card gateway module is hardly a necessity at all.

Not sure if the other approach would be needed as a fallback if JS is disabled, though...I don't really see how it could be done securely, so probably best just to require JS even though some people dislike that.

mail@victorquinn.com’s picture

For the curious, I went about it this way using the Stripe PHP libraries instead of using Stripe.js because it fit in more with the Ubercart way of doing things. All the rest of the major Ubercart payment processing modules (as far as I could tell anyway) are done in a similar manner, using PHP libraries and doing the processing server-side. Doing it in JS client-side would indeed be very cool, but then things would have to be written a bit differently to make everything flow. Since this was my first Ubercart payment module though, I didn't feel comfortable trying to do something entirely different from the other payment modules.

However, I've been talking with some of the Stripe devs and they are pushing to move toward Stripe.js so we're trying to figure out a way to get us there in the not too distant future.

wizonesolutions’s picture

Version: 6.x-1.0 » 6.x-1.x-dev

I've been thinking about this and had 2 ideas. One is to create an intermediary callback to which the form submits, kind of like uc_paypal, but on the user's own site instead. That callback would receive the token Stripe.js generated, handle the payment processing, and then redirect back to the site itself with success or failure. Of course, uc_credit already has that kind of thing built-in, and I notice that Commerce Stripe simply submits empty form values and uses JS to populate the Stripe token.

I think the latter approach would make more sense if it's possible since it basically is a credit card gateway; it just uses a token instead of a credit card number.

wizonesolutions’s picture

Hm. After backporting Stripe, I'm a little discouraged; it feels like my backport is going to wind up diverging from the Drupal 7 version a fair amount (because I fix things along the way), and I'm not sure what I think about that.

I'm going to Stripe Office Hours tonight and will make the call whether I just implement stuff straight in uc_stripe (because Stripe's API is already pretty abstracted and easy-to-use) or whether I use my backport of the Stripe module.

Main benefits of Stripe module is that it has a working implementation of Stripe.js and that it collects all of the API keys on its own configuration screen, not just the secret key. But it would not be hard to copy-paste the good parts of that code, hence my uncertainty. It doesn't even really have any abstraction of the actual Stripe operations (wrapper functions). Seems like it might be better to start by using the library directly in uc_stripe, but abstract things out so that the API parts could potentially turn into a D6 version of Stripe organically rather than trying to fit a round peg into a square hole, cuz that's what this is currently feeling like to me (even though I was so for it before).

Any comments either way to sway my decision are welcome. Either way, I'm shooting to solve this problem tonight! :)

mail@victorquinn.com’s picture

Hey wizeonesolutions, for sure, any info/guidance you can get from them on this would be great.

I wish I had the time to do it myself, but unfortunately I'm insanely busy as of late with no signs of it letting up.

Again, I think we're all in agreement that the Stripe JS library is the way to go, I just wasn't sure how to reconcile the round peg square hole issue you highlighted. Particularly when working within the bounds of Ubercart, it was trick to figure out how to mash that together with a JS payment library like this.

It's certainly possible and would ultimately be far slicker but it's a non-trivial task..

wizonesolutions’s picture

Appreciate you taking a bit of time to reply all the same! I understand busy. If I get something together though will you be able to commit it or hook me up with co-maintainership (even temporarily) so I can, if it meets your vision and whatnot.

wizonesolutions’s picture

Realized the office hours are tomorrow, so just dug into the code a bit tonight. Looks like I have to understand how the magic of uc_credit works. At a glance, though, all I have to do is add a bit to the checkout form alter so that Stripe.js sticks a token in a separate, named field instead of sensitive data. And then I will tell the charge function to use that instead of the CC number. Stripe's API works fine with a token.

I'm not going to try integrating this with my backport of the Stripe module at this point...I see no real gain. I'll just add a couple fields for the publishable keys, use the same JS in a uc_stripe.js file, and we'll be off to the races.

Fingers crossed for some good news tomorrow night.

wizonesolutions’s picture

OK, so I've finally started to get some traction on this. Not done yet, but getting there. So what will happen is that the form elements start disabled (via form alter) and then JS is used to enable them and remove the name attributes. This ensures that CC data doesn't hit the server even if JS is disabled and prevents PCI compliance requirements being introduced. Next is to bypass validation somehow and get it to stop complaining about the credit card number and such (since we validate with Stripe client-side). I probably have to change the form validation handler. I have to add a hidden field to store the token Stripe.js sends us back and make sure that goes into $order->payment_details.

The charge logic shouldn't need much tweaking, other than making it use the Stripe.js token instead of the CC number. And on that note, I have to add the publishable keys to the settings form.

Then edge cases like making sure the server ignores/unsets the CC data even if it somehow makes it through (which shouldn't be possible, but I'm not sure if Firebug can be used with JS off or not.

I want to get this done for use on my own site, so hopefully I'll have a patch to submit soon.

wizonesolutions’s picture

No joke on this being a non-trivial task. I worked on it more today though. Main issue now is how to get the JS in there at the right time...I basically need to respond to Ubercart being done with loading the payment form and inject uc_stripe.js and the publishable key setting. Tricky stuff since the payment form is retrieved with AJAX. I need to look into something like ajax_load or at least how it does it. I could probably get away with generating the JS for the publishable key in PHP if I absolutely must, at least to start (if ajax_load can't also do JS settings). We'll see next time I tinker at this.

It's definitely getting somewhere though, which is good.

wizonesolutions’s picture

OK, I managed to get the JavaScript in there. I'm not proud of how I did it, but improvements can always be made later. I basically shoehorned it in with the credit card theming function. I could have added markup to the form_alter, but I would have had to output it in the overridden theme function anyway, which isn't much better.

Anyway, I just have to override the validation so it doesn't check for credit card info (it isn't there) and check that the token is set instead. And then I can just tweak the charge function a bit, and I'll hopefully have it working!

I may or may not fix the administration form side of things (manually entering a payment). If it's not too hard, I just will. But I might just leave it out of eagerness to get something going.

Patch should be coming this week...

wizonesolutions’s picture

Assigned: Unassigned » wizonesolutions

OK, it's later than "next week" but at a Stripe hackathon now and hoping to bust out something today.

wizonesolutions’s picture

Assigned: wizonesolutions » Unassigned
StatusFileSize
new7.53 KB

By request, posting what I have so far. NOT done. But wrapping up for now. I figured out that I need to use hook_payment_method_alter() to change the validation callback for uc_credit_payment_method(). We need to write our own...that thing makes too many assumptions. Eventually, uc_stripe shouldn't even use uc_credit. But one thing at a time.

wizonesolutions’s picture

Pushed work so far to 6.x-1.x-1467886 on http://drupal.org/sandbox/wizonesolutions/1764778 (so I can get to it from elsewhere later and continue it - not ready to roll a patch yet).

bitcookie’s picture

Okay,

So I've got something working. Unfortunately I couldn't access wizeonesolution's code dump, so I had to start from the beginning. I pretty much re-wrote all of uc_stripe.

Basically the way this works is by hiding ubercart's normal card entry form, inserting bogus test values into it, and creating a faux collection form for stripe.js. A token gets inserted into a hidden field and picked up during the _charge callback to use instead of the user's credit card number.

Also, this module uses on-site subscription management for uc_recurring instead of setting up a stripe subscription.

I've only used this in a test environment and not a production environment. I don't know if ubercart will accept 4242424242424242 as a card number when the gateway is live.

Anyway, enough of this explanation nonsense. Here's a link to the bleeding edge code. I'll update this blog article as I continue to build out the gateway and eventually do my live testing.

http://bitcookie.com/blog/ubercart-and-stripe-js

restyler’s picture

Hi Bitcookie,

thank you for your work!
we are really interested in using javascript powered approach, too.
Did you have a chance to test your code in production?

restyler’s picture

(we don't really need subscription stuff, just regular payments)

wizonesolutions’s picture

rickmanelius’s picture

This is an extremely important issue. As stated in the summary, the primary benefit of using Stripe is to prevent the credit card from passing through the merchant server. Given that someone evaluating this module (particularly site builders that might not look under the hood) would expect this to be implemented in a shared-management approach, it should be noted on the project page until this patch is committed.

And FWIW, even when this is updated, a direct post method like Stripe will now be considered SAQ A-EP. See the next draft of the PCI compliance white paper for details.
https://github.com/rickmanelius/drupalpcicompliance/blob/ver_1_1/DrupalP...

mike.davis’s picture

Hi @Bitcookie, I am interested in using Stripe with Ubercart on a site, but when I have tried your code it didn't seem to take any payment. I can see it making a request for the card token but this doesn't seem to get set correctly and the payment is not made with Stripe. Is this a working version?

I have also tried the patch from @wizonesolutions but this just generates an error after when trying to review the order due to card number being entered.

Is this still very much a work in progress? I am only interested in taking payments at the moment, but would be interested in the recurring payments in the future.

rfay’s picture

Title: No credit card storage through direct post. » uc_stripe is not PCI-DSS compliant and it should and can be
Priority: Normal » Major

Changing title and priority to reflect the importance of this issue. Stripe goes so far toward relieving the PCI problem, and this module currently doesn't use that great framework.

rickmanelius’s picture

It's important to note that the gap between the number of controls one has to meet with this module in its current state versus what it could be is huge.

Right now, being that the credit card goes to the Drupal application, that now puts us into PCI SAQ D for version 3.0 of the standard. Based on stripe now using an iframe, that could put this to SAQ A (see https://support.stripe.com/questions/what-about-pci-dss-3-0). This means you go from 300+ security controls to ~15.

rfay’s picture

I turned this off on our site when I realized it wasn't meeting the promise, but really want to have a workable solution.

I am experimenting with @Bitcookie's published version, and find that it actually works, but needs some more work:

  • The CC form injected for Stripe is supposed to *not* have name attributes, to prevent a POST or accidental post from happening. CC form is also ugly, and shows even when credit card is not the selected payment type.
  • Recurring actually works, but only if uc_recurring has automatic triggering set. In other words, it does not use Stripe's subscriptions at all, which is not necessarily a bad thing.
  • Failures of local CC validation result in a broken form (reload is required - the submit button never gets re-enabled.
  • @Bitcookie changed the names of the variables that handle the private/public keys. This is easily adjusted with a hook_update_N() or other approach
  • Needs to be updated to stripe v2, but it seems this works with no trouble

I'm not completely convinced that I can solve these without getting sidetracked, but @Bitcookie's code definitely has potential. If you've updated since your post, @Bitcookie, could we get it onto Github or a sandbox?

It's also worth considering using the approach suggested in the OP, creating a different payment method and not using the CC payment method.

However, of the problems I list, only the first requires significant work. So maybe there's potential here. Should this go as a 6.x-2.x version? It doesn't look like the named maintainers are active here any more, or are they?

rfay’s picture

OK, I built a 6.x-2.x version which should be PCI-compliant, using stripe.js based on @BitCookie's work, with quite a lot of extra effort.

As far as I can tell, it works fine in a number of contexts. I'd love review.

I'm also willing to maintain this module, as it looks like I'm stuck with it for some time. So @victorquinn, @cweagans... let me have access if you're willing. Otherwise I'll have to maintain a fork, which is a lousy deal.

This version does *not* use stripe subscriptions. Instead it uses uc_recurring. The v1 implementation here does not seem to implement the webhooks that would be required to know if there was a charge made on a subscription, so that doesn't work very well. And keeping the recurring management within the Drupal site works fine.

The provided hook_update_N() updates from Stripe subscriptions to putting recurring fees local. However, it does *not* delete your Stripe subscriptions. I'll build a tool for that... since I have to have it.

I would dearly appreciate review both from the basic code side and the PCI compliance side. I've gone to a lot of effort to make sure that card details do not ever get to the Drupal server.

rfay’s picture

Status: Active » Needs review
rfay’s picture

@cweagans gave me commit access, so this is now in 6.x-2.x here. The 6.x-2.x-dev version is out there for your review.

rickmanelius’s picture

Hi @rfay. I'd be happy to review for PCI compliance considerations.

For anyone else following the thread, this can provide inspiration as to why this is a very important discussion http://drupalpcicompliance.org/. If we get an iframe solution in place for Ubercart, it would be a huge win for the community.

rfay’s picture

Title: uc_stripe is not PCI-DSS compliant and it should and can be » PCI-DSS compliance using stripe.js
Version: 6.x-1.x-dev » 6.x-2.x-dev

Updating title and version. Thanks for your review @rickmanelius

robertmaynord’s picture

I have been waiting and hoping for some form of dependable iframe solution for Drupal. I currently have an Authorize.net DPM account, and must upgrade shortly. It seems to me that many small businesses do not realize how much the transition from SAQ-A to SAQ-EP will cost. My merchant bank is working on a "cost saving" plan that will be priced around $5000 per year, including penetrations tests (not including server upgrades). For me, $5000 is still a prohibitive sum.

Anyway, hats off to all the Drupal folks working on a solution! (I run Ubercart version 7)

jamesoakley’s picture

With proper use of stripe.js, you shouldn't need the iframe route to get the same level of compliance, because all data is submitted directly to Stripe without ever touching the Drupal-site server. You get the same level of direct communication to Stripe as you'd get with an iframe, but without having to use frames.

Or have I missed something?

rfay’s picture

The 6.x-2.x implementation uses stripe.js, which does not involve an iframe.

jamesoakley’s picture

>> The 6.x-2.x implementation uses stripe.js, which does not involve an iframe.

Exactly.

Whereas robertmaynord, comment #28 in this thread, is specifically looking for an iframe solution, and I was wondering what made that necessary that couldn't be achieved in the way your work-in-progress is being built.

robertmaynord’s picture

Well, I am open to all solutions. However, the PCI DSS specifically lists iframes as qualifying for SAQ-A (https://www.pcisecuritystandards.org/documents/Understanding_SAQs_PCI_DS... PAGE 5). Direct Payment Methods such as Authorize.net's DPM no longer qualify. There are other SAQ-A possibilities such as Paypal and Authorize.net's SIM, but they take the customer away from the store site (to the credit card web site) for all the processing. This is thought to be less than optimal - it scares the customers away. An iframe allows the customer to stay on the store web site to complete the transaction.

According to Stripe: "We’ve been working with our Qualified Security Assessor and others to update Stripe.js to meet the new requirements and security constraints, while still keeping it simple to use. The new version of Stripe.js meets these criteria by performing all transmission of sensitive cardholder data within an iframe served off of a stripe.com domain controlled by Stripe. This in turn allows our customers to continue to be eligible for SAQ-A (the older questionnaire) in most cases."

With DPM, no data ever sees my website - I have no access to it. But according to my bank, DPM could be vulnerable to a "man in the middle attack", so it is no longer acceptable. My bank recommends that I convert to authorize.net's CIM program. But it MUST be the "Hosted" version (their web site), and the Drupal CIM is only for the non-hosted version of CIM.

Perhaps this new implementation you are working on is secure in the same way an iframe is. I am just wondering about the qualifications for SAQ-A --- what they will accept or not.

Again, I am open to all new possibilities....

jamesoakley’s picture

Thanks for explaining, and bringing me up to date with the latest requirements.

rickmanelius’s picture

DPM is vulnerable to keyloggers, for example, whereas an iframe adds an additional level of protection around the credit card form element within the iframe.

rickmanelius’s picture

I really apologize for not providing a review at this time (between having a sick baby and some work deadlines, I've been slammed). That said, if this goes through and does offer card on file support, it'll be added to the PCI white paper as a recommend SAQ A gateway. Tracking here => https://github.com/rickmanelius/drupalpcicompliance/issues/39

rickmanelius’s picture

@rfay. I apologize for not reviewing this sooner. At my company, we actually have a client that was using the D7 version of this and we got budget approval to review this. Unfortunately, it looks like your work is on the D6 version, which also appears to be considerably different than the D7 version. This is not to say that I won't still review this, but it's just going to be harder to see this through as I'd prefer to focus on the D7 branch given that it'll have a greater impact as D6 moves towards EOL.

rfay’s picture

We're porting warmshowers.org to D7 now, so you should see a D7 version in the next couple of months.

jamesoakley’s picture

Looking forward to that, @rfay.

I had a go at porting the D6 uc_stripe (6.x-2.x) to D7. I changed some of the hooks that had changed name, and a few other D6->D7 breaking changes. I got stuck at the jQuery function (or $()) seemingly not being available ot the uc_stripe_clean_cc_form() function in uc_stripe.js.

I think I'll have to wait until you are able to upload the port you're working on, then help out at the testing stage once most of the wrinkles have been ironed out.

One thing I was unclear on. Why is the module (as it stands) using client-side Javascript to remove the name attribute from the CC fields, rather than doing it directly in the hook_form_FORMID_alter implementation which is already being called? We'd still need to disable the submit button until we know Javascript is able to re-enable it, but I can't see why anything else needs altering client-side.

Anyway - thanks for your work on this. I'm following this issue, and will gladly help out reviewing draft code once you've got such.

(I don't need UC subscriptions to work, so if you get to the point where the rest of the module is written, I'll be able to review the rest of it while you work on recurring payments)

rfay’s picture

Removing the name attribute makes it impossible for the form field to be submitted. If the form were accidentally submitted, even if we were deliberately not handling the fields, the CC info would end up in session info, etc.

Letting the regular credit card handling module provide the form fields, on the other hand (and not removing them) allows for standard theming and such. The JS could provide the form fields, but they'd have to be separately themed.

It would be great if you'd upload your partial D7 patch or better, provide a link to a github or sandbox repo where your work is. It might help us.

jamesoakley’s picture

StatusFileSize
new8.06 KB

Removing the name attribute makes it impossible for the form field to be submitted. If the form were accidentally submitted, even if we were deliberately not handling the fields, the CC info would end up in session info, etc.

I'm clear on why the name attributes need removing.

I was more asking why they had to be removed using client-side javascript, rather than a server-side hook.

It would be great if you'd upload your partial D7 patch or better, provide a link to a github or sandbox repo where your work is. It might help us.

I'm afraid I'm not using a git repo of any kind - maybe I could set that up at some point. But, for now, I'll attach a patch. If you like, ignore the lines that are simply removing the uc_recurring functionality. I took those out to simplify my migration task for my use-case, and I wanted a module that worked. But the other modifications in the patch file will show you where I had got to.

rfay’s picture

Thanks @JamesOakley - I don't believe there's any way to convince the form API to get rid of the fundamental name attribute. If you know of one, let me know, because I think it would be reasonable.

Here's to hoping your patch will help push the port along. Thanks for putting it up!

jamesoakley’s picture

Here's to hoping, indeed.

I said one point I got stuck - I couldn't use the jQuery function ($) within uc_stripe_clean_cc_form()

The other was that I tried disabling all javascript first, to check that the "submit" button was being properly disabled, but that line didn't seem to be working either. I did some debugging, and tried a few changes to that line, but none of them work.

If you, in your development, overcome those hurdles, feel free to post your patch back up here, and I'll happily download a fresh copy to try again from there.

rfay’s picture

You'll want to read the whole D7 Javascript page, but especially the part about Using jQuery. It's easy, but not obvious at first.

jamesoakley’s picture

Thanks - I tried that namespace adjustment, and was still getting that both $ and $(document) were "undefined".

I'm sure I've missed something obvious, and it'll either come to me when I'm looking at something else, or someone else will solve it and I can resume working on the rest of the module.

rfay’s picture

Version: 6.x-2.x-dev » 7.x-2.x-dev

OK, the 7.x-2.x branch is ready for your review, and the 7.x-2.x-dev release is out there, https://www.drupal.org/node/2510952

Hoping all can look at it. I'll be able to respond to issues in the next week, but then will be gone for a couple of months. But this will get deployed on the site that forced it in the fall, so there's lots of energy to make it right.

rickmanelius’s picture

Excellent :) I have a client that's been really looking forward to this. Posting to them so we can test this and get to an RBTC. Thank you @rfay!

jamesoakley’s picture

Hmm... I'm having trouble getting this to work, but I'm leaving it at "Needs Review" in case it's me

An order goes through to the "Review Order" screen, but clicking the "Submit Order" button gives the error "We were unable to process your credit card payment. Please verify your details and try again. If the problem persists, contact us to complete your order."

I can see in my Stripe logs a successful POST request to /v1/tokens.

The uc_stripe_clean_cc_form javascript function doesn't appear to be firing. The cc-number field still has its name attribute. I've tried putting an alert() line at the top of that function, but I never get that pop up in the browser either.

Having said that, if it's not firing, I'm unclear how I'm able to pass through to the Review Order stage at all. The "Continue" button is supposed to be disabled until uc_stripe_clean_cc_form re-enables it.

Have I missed some key step in getting this module set up?

rfay’s picture

On the "review" page you'll use the PHP api to continue (it uses your token). If you get to the review page, it means (as you pointed out) that the token was captured successfully. The token is then used by the PHP api.

Please make sure you've looked in the watchdog logs for errors (admin/reports/dblog) and also looked at the requirements page to see if there are any complaints (admin/reports/status). I'll also be happy to spend some time with you on the phone or skype to see if we can sort this, because I'd love to see more users.

Note that this requires version 2.2.0 of the PHP API, which is quite a lot changed from earlier version. I'm sure you already read the README.txt and stepped through the requirements there, but just need to make sure.

jamesoakley’s picture

OK, I've made some progress. I think I was experiencing two distinct issues, and I've overcome one of them.

The first was the payment failing. I'd made two mistakes. First, my Stripe PHP library (which is 2.2.0), was in sites/all/libraries/stripe-php, rather than in sites/all/libraries/stripe. That's my fault - the README had the correct path. I was working too hastily, and blinded by (a) the fact that when you untar the library, its containing directory is named stripe-php-{version}, and (b) when configuring Stripe on a different kind of website altogether, stripe-php was the correct name.

My second mistake was not putting anything in the live token fields. It wouldn't charge in test mode unless there was something in those fields - even if it was random text.

I then managed to go the whole way through an order. One gotcha was that the default Stripe currency was USD, whereas my store is in GBP, so the order amount (£2.45) got charged as $2.45. That's editable in the payment gateway settings, but ideally the default Stripe currency would be whatever the store currency is.

So to my second issue: The CC fields still have names, the clean_cc function is not firing, and the continue button is not being disabled by PHP so that the JS can enable it later.

I've put loads of watchdog calls into uc_stripe_form_uc_cart_checkout_form_alter and they fire the whole way down that function. The JS and CSS includes are being processed correctly. But I don't get 1px.png or .uc_stripe_messages being added to the checkout form. (If I search the browser source HTML for the checkout page, they're not there at all). That explains why onload() never fires on 1px.png. It also explains why I don't see the "Test mode is ON" message - the div it goes in is missing.

I can't see why uc_cart_checkout_form_alter is running through, is adding JS and CSS, but is not successfully modifying the checkout form. Stuck again.

rfay’s picture

Do you know how to use the Chrome or Firefox js debugger, so you can step through the js code? If not, please give a quick google and give it a try. You'll also be much happier in life if you get a PHP debugger going - like the excellent PHPStorm. There are lots of tutorials around for it.

Please make sure you have css and js aggregation turned off at this point, so you can see clearly what's happening with those.

IRC or something else is probably better to support you, I'm rfay on #drupal-contribute, you can use d.o PM to send an email, etc. I'm randyfay on Skype.

The normal reason that a form-alter fails is that it attempts to alter the wrong element, so the correct element goes right on unchanged. That's hard to see without a debugger. A second reason that a form-alter can fail is another module changes the form *again* after the one you're studying has altered it.

rfay’s picture

A quick test says to me that the form-alter is failing, the image load is failing, etc. as you described. I'll work on that probably tomorrow.

jamesoakley’s picture

I've found one bug. $form['continue'] should be $form['actions']['continue']

That then disables the "continue" button as it should.

The dummy image is the next thing to work on. If I get anywhere I'll update this. (Unless you know of any PHP debuggers that work when you're running your dev site on a remote server, I'm limited to putting debug code - print_r, watchdog, etc - into the module itself).

rfay’s picture

I failed to properly port the use of #markup, which changed in D7. I just pushed a commit to fix that, which will be in the next dev package (it's done every 12 hours).

Although the form is now being altered correctly with respect to adding the image and the messages, I still don't see the clean_cc_form() being fired, so will work further on that.

jamesoakley’s picture

StatusFileSize
new1.59 KB

I cloned the repository, and can confirm that we now get the 1px.png image appearing OK.

The same alteration needed making for the message alerting when Stripe is in test mode; I've rolled that, and my comment from #52, into a patch - attached.

I can also confirm that the JS is not firing to remove the name attributes etc.

rfay’s picture

Committed your patch from #54, with slight alterations. Thanks!

  • rfay committed 29b8aa4 on 7.x-2.x authored by JamesOakley
    Issue #1467886-54 by JamesOakley - fix another #markup failure
    
rfay’s picture

The problem is seen in the js console in the browser:

Uncaught ReferenceError: uc_stripe_clean_cc_form is not defined

It's a result of moving the function inside the jquery closure. Should be easy to solve.

rfay’s picture

I can't say how much I appreciate your careful review and testing, @JamesOakley - you caught some super important stuff.

I've got several things both fixed and the js is improved and sorted out of the markup. I still have trouble triggering Ubercart's ajax behaviors on the cart/checkout page, which is different from how it worked in D6. Working on it.

  • rfay committed 544c362 on 7.x-2.x
    Issue #1467886: Make form submission play nicely with ajax and Drupal...
  • rfay committed 78cdf2d on 7.x-2.x
    Issue #1467886: Remove unnecessary image onload behavior
    
  • rfay committed ab95d66 on 7.x-2.x
    Issue #1467886: Improve hook_requirements()
    
    It was impossible to detect...
  • rfay committed ecead90 on 7.x-2.x
    Issue #1467886: Improve js and form alter to separate their functions
    
  • rfay committed ef6dc14 on 7.x-2.x
    Issue #1467886: Link to edit stripe settings was not quite right
    
rfay’s picture

Added several commits that respond to @JamesOakley's excellent review, testing, and debugging. I also discovered that with the way Libraries works the way I was checking for install didn't work if you hadn't already had a stripe library installed, so made that gentler.

Would appreciate review and testing.

If you are able to look at this today or tomorrow, I will probably be able to respond. I will be traveling from Friday afternoon for most of July and August, and unable to help with much.

Thanks!

jamesoakley’s picture

OK - my debugging time today is limited, but I've done some.

On the positive side, the name fields are now successfully removed from the card number fields, and the submit button is being re-enabled with Javascript successfully. Ha!

However three problems.

1. I've got two orders to go through successfully. On one, when I clicked "review order", a red message box appeared at the bottom of the screen that "a network error occurred - you have not been charged", before the final review page appeared as normal. Clicking submit order led to the order processing as normal. On the second time, I didn't get that error.

That could be a local problem on my computer (given the initial checkout screen only uses Javascript to call Stripe from the browser), however the red message appearing on the checkout page suggests something happened server-side to trigger that.

2. I'm having trouble with putting in valid card numbers only to get the messages at the top of the screen "You have entered an invalid credit card number. You have entered an invalid CVV number." If it starts deciding valid card numbers are invalid, nothing seems to get it unstuck. The only solution seems to be to clear caches and start checkout again.

In case it's reproducible, I placed an order with 5555555555554444 and a 3 digit CVV, and it went through. I added a product to cart, and checked out with 378282246310005 and a 4 digit CVV, and it rejected the card number. I then put 5555555555554444 back again, and it still rejected it. So it's not the test card number as such, but the site deciding it doesn't like any card number.

I then tried toggling the "Validate credit card numbers at checkout" setting. Having turned that from on to off, I was able to place an order. I then wondered if not validating card numbers was ironically making Ubercart report an invalid card number, and so this setting needs to be "on". But then my next attempted order failed with "invalid card number".

This is a nuisance, as intermittent problems are the hardest to reproduce and to debug.

3. With "validate" turned on, when I went to checkout the second time, Ubercart thought it knew my card number from last time. The "card number" field was pre-populated simply with the string "(last 4)" (with no actual digits supplied). I only mention that in case it's related.

4. Payment currency is always USD. But my store prices are in GBP. I've tried to find where in the UI this is changed, but can't. The PayPal Payments Pro tab has a currency drop-down, but the Stripe tab does not.

Sorry I've not got time to debug these further today, but I offer these experiences in the hope they help.

rfay’s picture

Thanks!

I think you'll want to have the "validate card" setting turned off, as Stripe does the validation and it doesn't make sense for uc_credit to do it as well. Also, uc_credit never ever sees the real credit card number, so validating it is crazy.

I have only used testing cards at this point (https://stripe.com/docs/testing)

It's nearly time for us to spin off additional issues (like currency selection) into new issues.

jamesoakley’s picture

I, too, am only using Stripe's test card numbers.

I've turned off validation. (TODO: Add to README)

I'm still only able to process one order after clearing Drupal caches. A second order, with the same card number, reports that it's an invalid card number. Clearing Drupal caches then allows that second order to go through on the next attempt (but the order after will report an invalid card number, until the caches are cleared, etc.)

  • rfay committed 410aca3 on
    Issue #1467886: Support the order's currency type
    
rfay’s picture

I pushed a commit supporting the order's currency type. It seems to work.

I can't recreate the problem you're having with repeated orders, and have no idea why that would have to do with the cache.

It seems the text about "network error" probably came from stripe, but most likely involved a network error.

Please make sure to check admin/reports/dblog every time an error occurs, and report what it says. The description of the problem should be there.

uc_stripe should definitely never know your card number - but it does get the last 4. When we re-fill the card number for Drupal's use, it's 424242424242 + the actual last 4. But uc_credit truncates *that* and it's the one that puts "Last 4" there, which isn't a function of uc_stripe.

Also, installing uc_stripe does in fact turn off the uc_credit_validate_numbers variable at install - see uc_stripe_install():

function uc_stripe_install() {
  // This turns ON the uc_recurring cron task to renew. We want this
  // ON because the renewal payments are handled by uc_recurring and NOT the stripe gateway
  variable_set('uc_recurring_trigger_renewals', TRUE);

  // Stripe does cc validation, so uc_credit must not... especially because
  // uc_credit gets a bogus cc number.
  variable_set('uc_credit_validate_numbers', FALSE);
}
rfay’s picture

@JamesOakley: If you'd disable, uninstall, and reinstall your uc_stripe, it would be much appreciated. We may have some issues with how many iterations of uc_stripe you've had that in there.

Also, please report the watchdog error any time you have an error.

Thanks!

owenbush’s picture

I've been testing with the following:

drupal 7.38
uc_stripe 7.x-2.x-dev
libraries 7.x-2.2
stripe PHP library 3.0.0

I'm encountering an issue with minimum spend. I have added a product which is $25 to my cart and when I proceed through checkout I get the following error:

"We were unable to process your credit card payment. Please verify your details and try again. If the problem persists, contact us to complete your order."

Looking in the watchdog reveals the following additional information:

"Payment failed for order 2727: Stripe Charge Failed for order 2727: Amount must be at least 50 cents"

One piece of information which may (or may not) be related is that the products being sold allow the customers to specify the amount they wish to pay (this is a donation site). So the actual cost/price of the products in Ubercart is zero. I suspected maybe that value was being sent to Stripe, but when the Stripe\Charge object is created the correct amount is being set. So I'm a little confused.

I've tried to debug this through the Stripe PHP Library but I feel like I'm going down a rabbit hole.

In Ubercart the order details page shows that the order is worth $25.

Any clues, or can you point me in the right direction for working this out?

Edit 1: I just did another order worth $125 and it was successfully processed by Stripe, which is good. However the value in the Stripe dashboard was listed as $1.25. So this seems to be some disconnect between passing the value in cents versus dollars I'd assume. I'll continue to play around and see what comes of it.

owenbush’s picture

StatusFileSize
new437 bytes

Patch attached to convert amount to cents before sending over to Stripe.

jamesoakley’s picture

I don't know, @owenbush. I've not had time to look at your patch. When I was testing this module I hit a few snags, but when payments did go through they were never out by a factor of 100. If there's an edge case when the price needs multiplying by 100, we need to determine what that edge case is, otherwise we get all other payments 100x too large.

owenbush’s picture

That is strange if it is an edge-case, I couldn't get a single payment to go through at the right amount, every single one was a factor of 100x to small. The other thing that raised an eyebrow for me was the comment above the line I changed which states:

// Format the amount in cents, which is what Stripe wants

But the uc_currency_format() function doesn't convert a dollar amount to cents, it just formats it with a currency symbol and thousand dividers etc (all of which in this case are FALSE). Which is why I thought maybe it was an oversight and the values were not converted to cents at all.

I may of course be wrong and I'm happy to help test any patches or re-work mine if it looks like it is just a weird edge case.

owenbush’s picture

I'd be interested to see what others have seen because there are several reasons now after further digging that makes me think that the $amount should be multiplied by 100.

uc_stripe.module:457 - function uc_stripe_charge()
$formatted_amount = $amount / 100;

I am not sure why the amount would be divided here to display in a formatted message for the customer if it was never converted to cents in the first place.

uc_stripe.module:532 - function uc_stripe_renew()
$amount = $amount * 100;

For recurring payments the amount is multiplied by 100 to get it into cents, but this is not done in uc_stripe_charge()

At the moment without my patch I cannot get a payment to go through at the right amount.

rickmanelius’s picture

I agree with @owenbush. There are two places where a card is charged with the following call:

    $charge = \Stripe\Charge::create(array(
        "amount" => $amount,
        "currency" => strtolower($order->currency),
        "customer" => $stripe_customer_id
      )
    );

There are two functions that call this:

  • uc_stripe_renew()
  • uc_stripe_charge()

In uc_stripe_charge(), we see the following:

  // Format the amount in cents, which is what Stripe wants
  $amount = uc_currency_format($amount, FALSE, FALSE, FALSE);

Unfortunately, uc_currency_format() doesn't do anything in the sense of formatting into cents. So if $amount is in dollars, then it's not being passed to stripe correctly.

rickmanelius’s picture

In terms of $amount, we can see that the test_gateway_charge() uses dollars.
http://www.drupalcontrib.org/api/drupal/contributions%21ubercart%21payme...

$amount == 12.34

So $amount is dollars, and the uc_stripe_charge() doesn't convert into cents. Hence the need for the patch.

rickmanelius’s picture

Just a quick followup. We've tested the patch in #68 and will be moving forward with applying it for a client site. That said, we'd love to see it a part of the official module.

rfay’s picture

I'll look at this in the near term. However, I'd like to have all issues of this type as new issues please, rather than continuing here.

Glad to hear it's working for you @rickmanelius !

rfay’s picture

Status: Needs review » Fixed

I just tested with actual products (in test mode) and both the initial charge and a recurring fee charge and renewal and both were for the correct, configured amount, so I'm skeptical about the patch in #68 - Please do these things:

1. Describe a path to demonstrate the problem.
2. Open a new issue with it.

I absolutely want to get problems resolved, but if I can't demonstrate them I can't debug them.

I'm going to take this issue to fixed, as it's run its course. Thanks to all of you for your participation!

Note that #2540152: _uc_stripe_get_customer_id does not load the user account correctly, which could have affected renewals, went in today.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

nhero’s picture

To demonstrate the problem go to admin/store/settings/store and under Currency Format select 0 or 1 for Number of Decimal Places. The amount returned by uc_currency_format decreases when the decimals are decreased. For example, a $30 transaction with 2 decimal places gets returned a value of 3000, with 1 decimal it gets set to 300 and with 0 it gets set to 30.

Patch attached with new function to convert amount to cents before sending over to Stripe.

rfay’s picture

@nhero your input and patch are very much appreciated, but could you please open a new issue for this? This one is marked closed(fixed)

Please describe the problem completely there, and repeat your instructions.

This will help us get this fixed *soon*.

Thanks,
-Randy

rfay’s picture

@nhero I moved your issue over to #2580025: Stripe must charge in "smallest currency unit" - currently biased toward dollars/cents - It's an important issue. I'd be interested in knowing exactly what your use case is and how you ran into this. Are you using a zero-decimal currency? Please reply and interact over in #2580025: Stripe must charge in "smallest currency unit" - currently biased toward dollars/cents .

rfay’s picture

For those of you who were interested in the different multiplier options in uc_stripe (to make cents out of dollars/cents) the followup issue with patch is #2580025-04: Stripe must charge in "smallest currency unit" - currently biased toward dollars/cents. It also removes the magical use of 100 as a multiplier, and the inappropriate use of uc_currency_format(). Your review is appreciated.