Closed Bug 744035 Opened 12 years ago Closed 3 years ago

Add Spideroak support for Filelink

Categories

(Thunderbird :: FileLink, enhancement)

x86
All
enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED INVALID

People

(Reporter: mconley, Assigned: tom)

References

()

Details

Attachments

(2 files, 9 obsolete files)

We're aiming to support Spideroak with the Filelink feature.

This work was originally being tracked in bug 698925.  We're moving that work over here.

I'll bring over Tom's patches momentarily.
Attached patch OAuth patch (obsolete) — Splinter Review
Attachment #613636 - Flags: review?(dbienvenu)
Attached patch Spideroak Filelink component (obsolete) — Splinter Review
Attached image Spideroak Icon (obsolete) —
Attached patch cleanup code a bit (obsolete) — Splinter Review
this is what I think I'll land. This cleans up the handling of the default parameters to match what we do in other code. I haven't checked that spider oak still works, but dropbox does. I'll try spider oak patch separately.
Attachment #613636 - Attachment is obsolete: true
Attachment #613636 - Flags: review?(dbienvenu)
Attachment #613822 - Flags: review+
Comment on attachment 613637 [details] [diff] [review]
Spideroak Filelink component

Review of attachment 613637 [details] [diff] [review]:
-----------------------------------------------------------------

Driveby

::: mail/components/cloudfile/content/Spideroak/management.js
@@ +1,1 @@
> +function onLoadProvider(provider) {

No MPL2 header?

::: mail/components/cloudfile/content/Spideroak/management.xhtml
@@ +11,5 @@
> +    <script type="text/javascript;version=1.8"
> +            src="chrome://messenger/content/cloudfile/Spideroak/management.js"/>
> +    <link rel="stylesheet"
> +        type="text/css"
> +        href="chrome://messenger/skin/preferences/preferences.css" />

Align attributes

::: mail/components/cloudfile/content/Spideroak/settings.xhtml
@@ +1,5 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!DOCTYPE html
> +PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN"
> +"DTD/xhtml1-strict.dtd">
> +<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">

Are these going to get localized? If so we shouldn't hardcode language

::: mail/components/cloudfile/nsSpideroak.js
@@ +4,5 @@
> +
> +/* This file implements the nsIMsgCloudFileProvider interface.
> + *
> + * This component handles the Spideroak implementation of the
> + * nsIMsgCloudFileProvider interface.

Please use doxygen /** foo */ comments for file headers. Then the description shows up properly in mxr file listings etc.
The "This file implements the nsIMsgCloudFileProvider interface." probably shouldn't be first.

@@ +172,5 @@
> +    if (!this._uploader) {
> +      this._uploader = new nsSpideroakFileUploader(this, aFile,
> +                                                 this._uploaderCallback
> +                                                     .bind(this),
> +                                                 aCallback);

align params

@@ +194,5 @@
> +      for (let i = 0; i < this._uploads.length; i++)
> +        if (this._uploads[i].file == aFile) {
> +          this._uploads.splice(i, 1);
> +          return;
> +        }

braces "missing" for the large for block

@@ +207,5 @@
> +   *                        is successful
> +   * @param failureCallback the function called if information retrieval fails
> +   */
> +  _getUserInfo: function nsSpideroak__getUserInfo(successCallback,
> +                                                failureCallback) {

align arguments

@@ +403,5 @@
> +  },
> +
> +  /**
> +   * logon to the spideroak account.
> +   *

Capitalize

@@ +522,5 @@
> +    this.request = this.spideroak._connection.signAndSend(url, "", "POST", bufStream,
> +      function(aResponseText, aRequest) {
> +      }.bind(this),
> +      function(aException, aResponseText, aRequest) {
> +        if(aException === "201 - Created"){

Spaces around (, and before {. 
But why is 201 an exception?
Comment on attachment 613822 [details] [diff] [review]
cleanup code a bit

Review of attachment 613822 [details] [diff] [review]:
-----------------------------------------------------------------

Drive by...

::: mail/base/modules/oauth.jsm
@@ +35,5 @@
> +
> +  this.signatureMethod = aSignatureMethod || "HMAC-SHA1";
> +  this.tokenMethodA = aTokenMethodA || "oauth/request_token";
> +  this.tokenMethodB = aTokenMethodB || "oauth/authorize";
> +  this.tokenMethodC = aTokenMethodC || "oauth/access_token";

Do the tokenMethodA, tokenMethodB and tokenMethodC names make any sense? (are they part of a spec?). If not, I think I would have preferred more descriptive names like requestTokenURL, authorizeURL, accessTokenURL. (URL may not be the best word, as it's only used for a part of the URL, but I think you get the idea.)

@@ +123,4 @@
>      }
>      this.log.info("in sign and send url = " + url + "\nurlSpec = " + urlSpec);
>      this.log.info("dataParams = " + dataParams);
> +    if (this.signatureMethod === "HMAC-SHA1"){

Nit: space between ) and {

@@ +145,5 @@
> +      this.signature = encodeURIComponent(hmac.finish(true));
> +    }
> +    else
> +      this.signature = encodeURIComponent(this.consumerSecret) + "%26" +
> +                       encodeURIComponent(this.tokenSecret);

Do we know the name of this signature method? Shouldn't we check for it, and throw a NOT IMPLEMENTED exception if the name of the requested signature method isn't supported?

Coding style nit: { } around code blocks of more than one line, even if it's only a single instruction that has been wrapped. (I think that's what the rest of this file does.)
I've addressed Florian's comments as best I can. The OAuth spec refers to temporary credentials, authorizing, and getting credentials, which is what spider oak clearly does, dropbox a little less clearly, but I've went with what the RFC describes.
Attachment #613822 - Attachment is obsolete: true
Comment on attachment 613637 [details] [diff] [review]
Spideroak Filelink component

the missing comma on the maxFileSize line completely breaks your component.

+  _lastErrorText : "",
+  _maxFileSize : -1
+  _availableStorage : -1,

So I fixed that locally, but the setup account process just gave me a login screen, without any way of creating a new account, so I couldn't test anything beyond that. I think that logon screen issue is on your web-site side, so that'll need to be fixed before I can make progress testing the provider. Let me know when that works and I'll try again.

I'll attach a cumulative patch that includes the .png file in a moment. Speaking of which, you need to change the gnomestripe and pinstripe themes to include spiderOak.png so that you have an icon on mac and linux. I've also fixed some other minor things, like using throw to return errors, and fixed cancel.
Attachment #613637 - Attachment is obsolete: true
Attachment #613638 - Attachment is obsolete: true
I'm having issues building thunderbird at the moment- can someone point me to a relatively recent pair of revisions that may likely work? (on http://hg.mozilla.org/users/bienvenu_nventure.com/big-files and comm-central)
(In reply to Tom Thompson from comment #10)
> I'm having issues building thunderbird at the moment- can someone point me
> to a relatively recent pair of revisions that may likely work? (on
> http://hg.mozilla.org/users/bienvenu_nventure.com/big-files and comm-central)

ignore the bienvenu_nventure.com repository; it's been abandoned. Just use comm-central.
Is this work ready for review?
(In reply to Mike Conley (:mconley) from comment #12)
> Is this work ready for review?

Account setup didn't work for me, so I couldn't test the code.
Hm.  So, now with a fresh compile, this patch is causing me to *crash* when I attempt to set up an account.

Backtrace: http://pastebin.mozilla.org/1614586
(In reply to Mike Conley (:mconley) from comment #14)
> Hm.  So, now with a fresh compile, this patch is causing me to *crash* when
> I attempt to set up an account.
> 
> Backtrace: http://pastebin.mozilla.org/1614586

Yikes. Tom, are you able to do development on your patch, or are you having the same problem as Mike?
Last I checked it was ok; I'll see if I can reproduce and figure out what's causing it.

-Tom
I've been having issues building thunderbird from the instructions on https://developer.mozilla.org/En/Simple_Thunderbird_build

Can anyone point me to a pair of revisions that they've confirmed work on windows and/or OSX? I'd at least like to be able to confirm if something in my environment is causing things to fail in subtle and unexpected ways.

-Tom
(In reply to Tom Thompson from comment #17)
> I've been having issues building thunderbird from the instructions on
> https://developer.mozilla.org/En/Simple_Thunderbird_build
> 
> Can anyone point me to a pair of revisions that they've confirmed work on
> windows and/or OSX? I'd at least like to be able to confirm if something in
> my environment is causing things to fail in subtle and unexpected ways.

The tip of the trunk should build fine - what errors are you seeing? You can also ask on #maildev on irc.mozilla.org.

> -Tom
I got spideroak integration working again; was having issues writing the tests last night, will get them in shortly.

Patch includes OAuth fixes, as it seems they haven't merged in yet.
Component: Message Compose Window → FileLink
QA Contact: message-compose → filelink
Tom:

Any progress on this? Are you ready for review yet?

-Mike
Mike-

Sorry, I got caught on deadlines for other projects. I'll have the unit tests ready tomorrow.

-Tom
Comment on attachment 614241 [details] [diff] [review]
patch addressing florian's comments

Review of attachment 614241 [details] [diff] [review]:
-----------------------------------------------------------------

I was looking at using this patch for the Ubuntu One back end, so thought I'd give a few review comments of my own.

::: mail/base/modules/oauth.jsm
@@ +30,5 @@
>    this.consumerKey = aAppKey;
>    this.consumerSecret = aAppSecret;
>    this.log = Log4Moz.getConfiguredLogger("TBOAuth");
> +  this.verifier = "";
> +  this.signature = "";

I'm not sure why these need to be attributes on the OAuth object. They don't seem to represent state that needs to be maintained long term.

@@ +123,4 @@
>      }
>      this.log.info("in sign and send url = " + url + "\nurlSpec = " + urlSpec);
>      this.log.info("dataParams = " + dataParams);
> +    if (this.signatureMethod === "HMAC-SHA1") {

If you put a "let signature;" statement just outside this block, then the signature value you calculate inside this block will still be in scope when it is needed.  That seems a bit cleaner than saving it as an attribute on "this".

@@ +145,5 @@
> +      this.signature = encodeURIComponent(hmac.finish(true));
> +    }
> +    else if (this.signatureMethod == "PLAINTEXT") {
> +      this.signature = encodeURIComponent(this.consumerSecret) + "%26" +
> +                       encodeURIComponent(this.tokenSecret);

This doesn't quite match what is in the spec (http://oauth.net/core/1.0a/#anchor21).  First off, it should probably use percentEncode() rather than uncodeURIComponent, and secondly the spec says the values need to be encoded twice.  Something like this:

percentEncode(percentEncode(consumerSecret) + "&" + percentEncode(tokenSecret))

It won't make any difference if the secrets don't contain any characters that need escaping, but it would be better not to leave this trap around for future users.

@@ +275,5 @@
>    },
>    onAuthorizationReceived: function(aData) {
>      this.log.info("authorization received");
> +    let data = this._parseURLData(aData);
> +    this.verifier = data.oauth_verifier;

From the look of it, this will cause the oauth_verifier value to be included in all subsequent signed requests.  From the spec it only needs to be passed to the "Access Token URL".

Might it be a bit cleaner to instead pass the value to requestAccessToken(), and then have that method pass [["oauth_verifier", verifier]] as the aOAuthParams argument to signAndSend()?
James-

Moving the signature out of 'this' does indeed seem cleaner. Trying to move the verifier as you suggested caused several unit tests to fail; I'll see what I can do with it after I finish cleaning up the unit tests for spideroak. I'll attach an updated version of oath.jsm at that time.

-Tom
verifier and signature moved out of 'this'
Mike-

Let me know if there is anything else I need to do with this. I'll try to address any feedback promptly, etc.

Thanks,
-Tom
Attachment #630575 - Attachment is obsolete: true
Comment on attachment 636119 [details] [diff] [review]
oauth patch; addresses feedback from james

Review of attachment 636119 [details] [diff] [review]:
-----------------------------------------------------------------

Drive by...

I looked only very quickly (not a full review), but it seems quite good, except for two nits:

::: mail/base/modules/oauth.jsm
@@ +145,5 @@
> +      signature = encodeURIComponent(hmac.finish(true));
> +    }
> +    else if (this.signatureMethod == "PLAINTEXT") {
> +      signature = percentEncode(percentEncode(this.consumerSecret) + "&" +
> +                       percentEncode(this.tokenSecret));

Please fix the indent of this line (align it after the '(' of the line above it).

@@ +156,5 @@
>  
>      let authorization =
>        "OAuth " + params.map(function (p) p[0] + "=\"" + p[1] + "\"").join(", ");
>      let headers = (aHeaders || []).concat([["Authorization", authorization]]);
> +	this.log.info(headers);

Did you mean to remove this line after finishing your testing?
Attachment #636119 - Attachment is obsolete: true
I apparently forgot to replace the AppKey/Identifier's I was using for testing with real ones.
Attachment #636120 - Attachment is obsolete: true
Blocks: 764312
Attachment #636163 - Flags: review?(dbienvenu)
Comment on attachment 636163 [details] [diff] [review]
oauth patch; address florian's comments

I haven't verified that the dropbox extension still works, so we should be on the lookout for any new problems reported with it.
Attachment #636163 - Flags: review?(dbienvenu) → review+
Do we need to check some things in the tree ?
Assignee: nobody → tom
Comment on attachment 636165 [details] [diff] [review]
cumulative patch for spideroak support for filelink; tests included

Putting into my review queue.
Attachment #636165 - Flags: review?(mconley)
Just a note that if we want this in 15 (and I think we do), we'll need to do what Ubuntu One did and steal a string to replace "spideroakMgmt.viewSettings" so as to not rock the l10n boat.
Attachment #614410 - Attachment is obsolete: true
Attachment #614241 - Attachment is obsolete: true
Comment on attachment 636165 [details] [diff] [review]
cumulative patch for spideroak support for filelink; tests included

Review of attachment 636165 [details] [diff] [review]:
-----------------------------------------------------------------

Hey Tom,

I think this patch has bitrotted a bit - likely because we removed the Dropbox component. Can you please regenerate?

I've also added a few review comments.

-Mike

::: mail/components/cloudfile/content/Spideroak/management.xhtml
@@ +26,5 @@
> +    </div>
> +    <div id="provider-spacebox">
> +      <div id="provider-space-visuals"></div>
> +      <div id="provider-space">
> +        <div>&cloudfileMgmt.usedSpace;<span id="file-space-used"/><span id="file-space-used-swatch" class="space-swatch"/></div>

the usedSpace and unusedSpace strings should be within a label element.  See http://mxr.mozilla.org/comm-central/source/mail/components/cloudfile/content/YouSendIt/management.xhtml#35

@@ +28,5 @@
> +      <div id="provider-space-visuals"></div>
> +      <div id="provider-space">
> +        <div>&cloudfileMgmt.usedSpace;<span id="file-space-used"/><span id="file-space-used-swatch" class="space-swatch"/></div>
> +        <div>&cloudfileMgmt.unusedSpace;<span id="remaining-file-space"/><span id="remaining-file-space-swatch" class="space-swatch"/></div>
> +				<!--TODO-->

What's this TODO for?

::: mail/components/cloudfile/content/Spideroak/settings.xhtml
@@ +4,5 @@
> +"DTD/xhtml1-strict.dtd">
> +<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
> +  <head>
> +    <link rel="stylesheet"
> +        type="text/css"

Nit - please line up the type and href attributes with rel.

@@ +7,5 @@
> +    <link rel="stylesheet"
> +        type="text/css"
> +        href="chrome://messenger/skin/preferences/preferences.css" />
> +  </head>
> +  <body id="provider-settings">

So I guess you're OK with not showing "Learn more..." or "Need an account?" links?

::: mail/components/cloudfile/nsSpideroak.js
@@ +103,5 @@
> +   *                         nsSpideroakFileUploader
> +   * @param aStatus the result of the upload
> +   */
> +  _uploaderCallback : function nsSpideroak__uploaderCallback(aRequestObserver,
> +                                                           aStatus) {

Nit - aStatus should be lined up with aRequestObserver.

@@ +138,5 @@
> +
> +    // if we're uploading a file, queue this request.
> +    if (this._uploadingFile && this._uploadingFile != aFile) {
> +      let uploader = new nsSpideroakFileUploader(this, aFile,
> +                                               this._uploaderCallback

Nit: these arguments need to be lined up properly.

@@ +148,5 @@
> +    this._file = aFile;
> +    this._uploadingFile = aFile;
> +
> +    let successCallback = this._finishUpload.bind(this, aFile, aCallback);
> +    if (!this._loggedIn)

For one-liners like this, we generally don't use the curly braces.

So this is fine:

if (!this._loggedIn)
  return this._logonAndGetUserInfo(successCallback, null, true);

Also, please indent with 2 spaces, and not 4.

@@ +177,5 @@
> +    delete this._userInfo; // force us to update userInfo on every upload.
> +
> +    if (!this._uploader) {
> +      this._uploader = new nsSpideroakFileUploader(this, aFile,
> +                                                 this._uploaderCallback

These need to be lined up properly.

@@ +216,5 @@
> +   *                        is successful
> +   * @param failureCallback the function called if information retrieval fails
> +   */
> +  _getUserInfo: function nsSpideroak__getUserInfo(successCallback,
> +                                                failureCallback) {

Nit - this needs to be lined up.

@@ +267,5 @@
> +   * @param aWithUI a boolean for whether or not we should display authorization
> +   *                UI if we don't have a valid token anymore, or just fail out.
> +   */
> +  _logonAndGetUserInfo: function nsSpideroak_logonAndGetUserInfo(aSuccessCallback,
> +                                                               aFailureCallback,

Nit - lines 271-272 need to be lined up.

@@ +299,5 @@
> +   * @param aCallback an nsIRequestObserver for observing the starting and
> +   *                  ending states of the request.
> +   */
> +  refreshUserInfo: function nsSpideroak_refreshUserInfo(aWithUI, aCallback) {
> +      this.log.info("refreshUserInfo");

This needs to be indented 2 spaces, not 4.

@@ +304,5 @@
> +    if (Services.io.offline)
> +      throw Ci.nsIMsgCloudFileProvider.offlineErr;
> +    this.requestObserver = aCallback;
> +    aCallback.onStartRequest(null, null);
> +    if (!this._loggedIn)

For consistencies sake, please put the opening curly brace on the same line as the if.

@@ +306,5 @@
> +    this.requestObserver = aCallback;
> +    aCallback.onStartRequest(null, null);
> +    if (!this._loggedIn)
> +    {
> +        this.log.info("not logged in, logonAndGetUserInfo");

These lines, and lines 315-316, should be indented 2 spaces, not 4.

@@ +337,5 @@
> +   * This function does not appear to be called from the BigFiles UI, and
> +   * might be excisable.
> +   */
> +  createExistingAccount: function nsSpideroak_createExistingAccount(aRequestObserver) {
> +     // XXX: replace this with a better function

Nit - this line should be indented 2 spaces, not 4.

@@ +431,5 @@
> +  logon: function nsSpideroak_logon(successCallback, failureCallback, aWithUI) {
> +    let authToken = this._cachedAuthToken;
> +    let authSecret = this._cachedAuthSecret;
> +    if (!aWithUI && (!authToken.length || !authSecret.length)) {
> +        this.log.info("aWithUI: " + aWithUI);

Lines 435 - 437 aren't lined up correctly.

@@ +438,5 @@
> +      failureCallback();
> +      return;
> +    }
> +
> +this._connection = new OAuth(this.displayName, gServerUrl, gAuthUrl,

Lines 442 - 445 are not indented correctly.

@@ +533,5 @@
> +    let fstream = Cc["@mozilla.org/network/file-input-stream;1"]
> +                     .createInstance(Ci.nsIFileInputStream);
> +    fstream.init(this.file, -1, 0, 0);
> +    let bufStream = Cc["@mozilla.org/network/buffered-input-stream;1"].
> +      createInstance(Ci.nsIBufferedInputStream);

Nit - we generally break long Cc instantiation lines just before the periods.  Please make this:

let bufStream = Cc["blah"]
                  .createInstance(Ci.blah);

@@ +537,5 @@
> +      createInstance(Ci.nsIBufferedInputStream);
> +    bufStream.init(fstream, this.file.fileSize);
> +    bufStream = bufStream.QueryInterface(Ci.nsIInputStream);
> +    let contentLength = fstream.available();
> +    let oauthParams =

Why is this broken up over two lines?

::: mail/locales/en-US/chrome/messenger/cloudfile/Spideroak/management.dtd
@@ +1,4 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this file,
> +   - You can obtain one at http://mozilla.org/MPL/2.0/.  -->
> +<!ENTITY spideroakMgmt.viewSettings "View my account settings on spideroak.com">

As I mentioned earlier in the bug, if we're serious about getting this in TB 15, we can't introduce these new strings. We'll need a separate patch and we'll need to steal some strings.

::: mail/test/mozmill/cloudfile/test-cloudfile-backend-spideroak.js
@@ +83,5 @@
> +  provider.uploadFile(file, requestObserver);
> +
> +  mc.waitFor(function () requestObserver.success);
> +
> +      let urlForFile = provider.urlForFile(file);

Lines 87-88 are not indented properly.
Attachment #636165 - Flags: review?(mconley) → review-
Even if the Spideroak backend has not reached a stage where it can be landed, would it be possible to get the OAuth PLAINTEXT patch landed? (attachment 636163 [details] [diff] [review]).

As described in bug 764312, this change is also required by the Ubuntu One backend (the patch from that bug was applied, but has no effect without the changes to oauth.jsm).
(In reply to James Henstridge from comment #34)
> Even if the Spideroak backend has not reached a stage where it can be
> landed, would it be possible to get the OAuth PLAINTEXT patch landed?
> (attachment 636163 [details] [diff] [review]).

It can land for comm-central, but I assume your goal is to fix the Ubuntu One bug soon, so you will want to request approval-comm-aurora (for Thunderbird 17) and maybe approval-comm-beta too (for Thunderbird 16; it may be a bit late for that, but you can argue for it :-), what do you think Mark?).

After landing this, we will want to have someone check that it actually fixes the Ubuntu One bug, and check that it doesn't break the Dropbox add-on. Especially if it lands for Thunderbird 16, as there would be little time to wait for nightly testers to report regressions.
I would suggest that we spilt the oauth change out into s separate bug and get Mike to review it there - he can verify the Dropbox add-on still works successfully. When landing we can still credit Tom as providing the original patch.

James, if you could split out the necessary bits into a separate bug, that would be great.
(In reply to Mark Banner (:standard8) from comment #36)
> I would suggest that we spilt the oauth change out into s separate bug

I will take care of this, and reattach the oauth patch there, so that we can clearly track what lands for which release.
Depends on: 793749
No longer blocks: 764312
So what's the status with this one? Is the spideroak backend ready?
(In reply to Magnus Melin from comment #38)
> So what's the status with this one? Is the spideroak backend ready?

Perhaps Matt or dhain can answer.
(Tom's email bounces.)
Severity: normal → enhancement
Flags: needinfo?(matt+mozbugz)
Flags: needinfo?(dhain)
Flags: needinfo?(matt+mozbugz)
Flags: needinfo?(dhain)

SpiderOak ... no longer pursuing this functionality

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → INVALID
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: