Closed (fixed)
Project:
Backup and Migrate Dropbox
Version:
7.x-1.x-dev
Component:
Code
Priority:
Major
Category:
Bug report
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
8 May 2013 at 03:04 UTC
Updated:
18 May 2016 at 18:20 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
arrubiu commentedI notice that the problem is that the request sent to dropbox is not authorized.
If you make a backup Drupal dropbox ask confirmation, I think that cron does not confirm authorization.
Comment #2
Krizalys commentedThis is because this module was updated a few weeks ago and the update introduced a new API to communicate with your Dropbox account.
This new API uses the OAuth protocol, which basically is better for the security of your Dropbox account but also makes it more difficult to automate things.
The previous version, on the other hand, did not have this problem because its API was connecting "directly" to your account. Of course, security was also questionable since your Dropbox credentials were stored in clear in your database and since the module had access over your whole Dropbox account.
With that being said, I personally think that this module is almost useless if backups cannot be scheduled, so I strongly encourage the developers to either reintegrate the previous API or to improve the current version to allow one to schedule their backup as it was possible before.
Comment #3
brycefisherfleig commentedI agree with Krizalys. My use case for this module is to perform regular backups that can be cheaply stored in the cloud. Without the ability to backup on cron run or a regularly scheduled timeframe, this module is not very useful.
Is it possible to get a long lived OAuth token using the new API so that backups can be scheduled? I'd be willing to contribute a patch against the latest version of the module to get this functionality working.
@wundo, what are your thoughts on the best way forward? What's the ideal use case this module tries to fill?
Comment #4
Krizalys commentedAttached is a patch that modifies the module to allow the use of both the current (PHP SDK for) Dropbox REST API, which is not designed for scheduling, and the previous Dropbox Uploader, which is. Functionality has been mostly recovered from a previous version. Following this logic, the patch disables scheduling for the Dropbox REST API "destination"; only the Dropbox Uploader "destination" may be scheduled.
The patch also fixes an inconsistency between the path to the Dropbox REST API checked at install time and the path actually loaded at run time. When using this patch, the Dropbox REST API is expected to be installed at sites/all/libraries/Dropbox (note the capital D). Please make sure your install is conforming.
Also, for those who will apply the patch on an installed instance of the module, remember to install the Dropbox Uploader at sites/all/libraries/DropboxUploader since this check is done only at install time.
Comment #5
wundo commentedComment #6
okeedoak commentedHaven't tested the patch yet but one thing I notice right away is that the path to the Dropbox REST API folder is not consistent with the newly committed patch at: Path mismatch between destination and install file. It looks like the path should be sites/all/libraries/dropbox with a lowercase d.
Comment #7
okeedoak commentedCouldn't get the patch to apply. Maybe I'm doing something wrong?
Comment #8
Krizalys commentedThanks for your notice okeedoak.
It used to be with a lowercase d, however this was not consistent with the folder mentioned at different locations within the module files, which were sometimes using a capital D. For one reason (because the name of the folder is
Dropbox-masterwhen extracting from the GitHub archive I guess), I normalized this path using a capital D everywhere, so you would have to rename your library folder accordingly if it is already installed on your website, like explained in my previous post.Edit: there were new commits to the repo after I proposed my patch. Those commits introduced more consistency but adopted the lowercase d, so I will update the patch and take this into consideration.
Comment #9
Krizalys commentedThanks for the report okeedoak, this was due to the fact that new commits have been pushed after my patch, I guess.
Therefore, attached is an updated patch. It should apply correctly against the latest commits, and it also adopts the lowercase d for the Dropbox REST API folder, so you should have no need to rename it.
Let me know if you have issues while applying it.
Comment #10
Krizalys commentedAny update on this issue?
Comment #11
brycefisherfleig commentedThe patch applies cleanly, install works flawlessly, Krizalys. Thanks so much!
I can manually trigger the DropboxUploader destination, and clicking 'Run Cron" from the admin menu works as expected too.
However, triggering cron via drush fails due to a certificate error. Since this works from the admin interface, I'm happy to mark this reviewed and open a separate issue to deal with drush cron (which probably not a big deal for many use cases).
Comment #12
arrubiu commentedSorry, but I think that is not a question of execute cron via drush.
The problem is that it seems works *only* if the cron is executed manually and every site as a cron job to execute drupal cron.
Or not?
Comment #13
brycefisherfleig commentedI've rolled a new patch here that addresses the drush cron problem from #11, and fixes other some certificate issues I ran into after more testing. Here's the error message I was getting:
Although the path to the certificate was correct all the way through the script, when PHP handed cURL the certificate location, curl was executing from a different place (at least on my Windows machine). Simply letting PHP determine the absolute file path to the certificate before sending the path to DropboxUploader.php fixed the problem for me using
realpath(). It should also work on Linux and Mac (and anywhererealpath()works).I've rolled the patch from #9 here. The only difference is in line 64 of destinations.dropbox_uploader.inc.
EDIT The patch in #9 was passing a relative path.
Comment #14
brycefisherfleig commentedDoes anyone want to review these changes?
Comment #15
Krizalys commentedHello brycefisherfleig and thanks for your help.
I reviewed your patch, it looks like the changes introduced in #9 are gone. Is it what you meant?
Comment #16
brycefisherfleig commentedThanks, Krizalys! That was not what I meant at all. Looks like I still have learning to do about how to roll a patch!
Here is the patch I meant to roll. The only difference from the patch in #9 should is that I've made a call to realpath() in order to make sure that curl can find the SSL cert necessary for the old Dropbox uploader destination to work. I'd appreciate another round of review. Thanks!!
Comment #17
Krizalys commentedHello brycefisherfleig, thanks for the updated patch.
I have to mention that even before applying it, I never got the error your reported. I nonetheless tested it successfully on my Drupal 7.23/PHP 5.5 system. By successfully, I mean there that running or scheduling the cron (still) did not trigger the error you reported, and the files were uploading correctly on Dropbox. There was an error message related to the use of a deprecated feature in PHP 5.5 but I will file a separate issue for this.
That being said, I believe I did not get the error in the first place because I was using Linux and I recall from another project that cURL on Windows behaves differently when paths are involved. I had the issue with the (relative) path of the cookies if I remember correctly, which was resolving correctly on Linux but not on Windows. In the end, I had to use only absolute paths to have the code working on both systems, and this looks quite similar to your fix of our problem here.
If you were using Windows, it's very likely that your issue is also related to this specific path resolution behavior on this OS, hence, I think we should wait another review of your patch by a Windows user.
Comment #18
caspervoogt commentedGood to hear. Is this getting committed?
Comment #19
brycefisherfleig commented@plethoradesign, not yet. We're waiting for another Drupal user with a windows box to apply and test my patch from #16.
If you have a windows machine and xampp installed, I can help you figure out how to test and apply the patch via IRC or google hangouts. Let me know if you're game!
Comment #20
caspervoogt commentedHmm. I use a Macbook with MAMP, and some cloud hosting as well .. no Windows for dev. So hopefully someone with a Windows dev box can try it out.
Comment #21
brycefisherfleig commented@wundo, any chance we could commit this to dev branch? The current patch appears to be working.
Comment #22
elvizzi commentedI'm still having issues with the DropboxUploader; receiving the error, "Could not run backup because the file could not be saved to the destination." Dropbox deprecated that method, and I'm wondering if they've discontinued access. I'll continue to look; but is anyone still having luck with scheduling backups with the latest patch?
Comment #23
elvizzi commentedHelps if I turn on watchdog. The cert wasn't working; followed the instructions at https://github.com/jakajancar/DropboxUploader for replacing it and things work as expected now.
Comment #24
danielphenry commentedI just connected this patch and theirs seems to be an update issue. My previous dropbox configuration is now empty and can not be deleted:
http://skitch.danielphenry.net/Backup_and_Migrate_%7C_VBS_Resource-20140...
This patch should have a clean upgrade path from alpha 1 before being added in.
Comment #25
deanflory commentedIs this still an issue? Is this module working?
Comment #26
botrisI'm just recently added as a maintainer.
Not sure what the status is on this, just did a quick test and it seems that running cron does create a backup, but only after the app has been approved.
The first time you try to backup to Dropbox you need to approve the app and it doesn't create a backup until the second time you backup.
Please have a try and I'll update the module / documentation if need be.
Comment #27
danielphenry commentedThis is still an issue.
Just set up all my sites to run a dropbox backup and it doesn't work. If I manually run a backup, it will ask for approval the first time. It then works for the next few times. But I Have had it running on cron for the last few days and it has not succeeded in actually storing the data into dropbox, The profile claims it succeded but dropbox does not have the file.
Comment #28
botrisIt sounds like it needs to re-authenticate at Dropbox for some reason, I'll dig into it.
Those "next few times" are those manual backups or backups run by cron?
Comment #29
danielphenry commentedAfter a sucrssful manual backup (approving via dropbox), the upload seems to work for the remaining manual uploads. I didnt test cron directly.
The dropbox api states a request is supposed to be made for a token after authentication. I dont see that in the code. That token should be stored in the destination variables, which also never happenes. It may be stored in some seasion variable by the external librar, allowing it to work a few times after initial approval. But certainly not the database.
Comment #30
danielphenry commentedAlso it should be noted the approval takes place as part of the first backup. It should instead take plce when the destination is added. It would really clean up the workflow.
Comment #31
danielphenry commentedSorry to spam the issue queue but I just confirmed my theory.
Working process:
Not working:
The approval only works for the active session instead of for the destination.
This has just elevated from not working to a security issue as it is preventing cron from completing. Updating to "Major" and "Needs work". I'll try to take a look at this later today but I'm out of time for the morning.
Comment #32
danielphenry commentedSo I basically refactored the latest dev version to not use the 3rd party api and just curl up the files right from the module. I included both single request small file upload and multi-request large file upload. It's rather easy to do and eliminates and outdated and bloated 3rd party tool.
Oauth workflow redirecting to dropbox and coming back is not needed any more. Since an app has to be created for the uploads to work it's easier just to generate a key from the app and use that to connect to dropbox. I updated the directions to reflect that in my patch.
There are two clear issues with this patch
1. No error handling. I will add this at some point but I just thought I should post my working copy. If someone who know a bit more about curl and error handling wants to jump in it would be appreciated. Not really my area.
2. The max upload size should be using php_memory_limit instead of my arbitrarily chosen number.
Someone give me some feedback on what they think about moving in this direction. I really like slimming down the module and dumping the 3rd party too that was using deprecated api calls.
Comment #33
danielphenry commentedAs an additional benefit the 3rd party api was using a deprecated curl function that was throwing notices on smaller database files for php 5.5. This system does not do that.
Comment #34
botrisI think we should focus on the issue at hand; making sure we can create backups on cron runs.
And the solution seems to be storing the authentication key in the db and not the session.
I totally agree on moving away from the current 3rd party API, but I don't think using curl is the way forward.
Also I don't want to break current behavior / current sites. So I've created an parent issue to start a new 7.2 branch that will use the official Dropbox PHP SDK: #2728271: Create stable release.
Let's continue the API / SDK conversation there.
Comment #35
danielphenry commentedI can appreciate thinking of people who already use the module but:
1. This module is alpha. They should be expecting such changes.
2. As the functionality is already broken It will have little effect on people actively using the module. They will no longer be able to do manual backups but it's very easy to fix. And as this is alpha...
3. The current module will actually breaks cron if someone has this destination scheduled. Shouldn't we make this fix a priority. This will prevent backups on upgrade but will fix cron.
I'll focus on the 7.2 branch if you prefer but for the reasons above I believe that is a unnecessary and possibly bad call.
Comment #36
botrisPossible making another branch is not necessary, maybe make it a release plan towards beta. But we can continue that conversation in #2728271: Create stable release.
I agree on making this fix a priority, but I'm not convinced the 3rd party API is the cause of cron breaking. So the solution / patch in this issue should focus on storing the key in db.
The move away from the 3rd party API could / should be done in a separate issue.
Comment #37
botrisTested and backups work fine with cron.
Patch has been committed with minor adjustment.