Closed Bug 1021684 Opened 10 years ago Closed 10 years ago

Update box.com Filelink implementation to new APIs

Categories

(Thunderbird :: FileLink, defect)

defect
Not set
normal

Tracking

(thunderbird31+ fixed)

RESOLVED FIXED
Thunderbird 33.0
Tracking Status
thunderbird31 + fixed

People

(Reporter: standard8, Assigned: clokep)

References

Details

Attachments

(2 files, 4 obsolete files)

In a message received from box.com:

The APIs used by your application (Thunderbird) is using old APIs that
are about to be turned off.

A new version of APIs for Box was released in December of 2012, and the
old version was deprecated in December of 2013.  We are about to stop
supporting API calls to the old version, and your application will be
affected.

In order to support our mutual customers, please upgrade your
application to the new API.

Documentation on the API : http://developers.box.com/docs
SDKs to help you build faster: http://developers.box.com/sdks

We currently plan to stop allowing API calls to the old endpoints on July 1st.
Any takers?
I've started taking a bit of look at this, but I'd like some opinions about how to move forward on it. Obviously we want this done the right way, but also quickly. Additionally, we'll probably want to land this on beta to be in TB 31.

Anyway, we need to switch to using OAuth2, Fallen made a nice OAuth2 module that I believe we can reuse [1], but it lives in calendar. I'd have to move this to mailnews (which doesn't even seem to have a modules directory under base?) so that SeaMonkey would have access to it (for calendar only). However, bug 1014644 is moving FileLink under chat so that it can also be used in Instantbird. IB does not have access to mailnews/ so we wouldn't get OAuth2.jsm in this case. Of course we can fork it, but that doesn't really help the situation.

Note that OAuth2.jsm would need to be tweaked a bit too (once of the resources request is oauth2/auth, box.com uses oauth2/authorize). I haven't really looked over the whole code, there might be other things too.

The other option is to just do the OAuth2 dance inside of nsBox.js, but this still requires a browser window, we currently use auth.(xul|js) for this, but they seem to be similar to the more generic browserRequest.(xul|js) (which is what OAuth2.jsm uses btw, these files already are forked for TB/SM/IB) [2].

A couple ways forward:
1. Move OAuth2.jsm/browserRequest.(xul|js) to toolkit and use them.
2. Move OAuth2.jsm to mailnews (and make a copy of it for Instantbird, when we need it or go back to #1 at this point).
3. Ignore OAuth2.jsm and do all custom code. (This will still have some funkiness with browserRequest.(xul|js) once this all lands in Instantbird.)

Any thoughts / advice?

[1] http://dxr.mozilla.org/comm-central/source/calendar/base/modules/OAuth2.jsm
[2] http://dxr.mozilla.org/comm-central/search?q=path%3AbrowserRequest.xul&case=true&redirect=true
I'd opt for moving this stuff into toolkit if we can, personally.
I talked to Mike over IRC briefly, we think that a combination of #2 in the short term and #1 eventually will be our goal:
1. Move OAuth2.jsm to mailnews and get stuff working (quickly!)
2. Once FileLink lands in Instantbird, deal with any fallout, this most likely means getting this code landed in toolkit (or copying it to Instantbird).
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Attached patch WIP v1 (obsolete) β€” β€” Splinter Review
I'm attaching a WIP that is the start of what should work. I seem to get an unauthorized error when fetching the user info, however. I haven't had a chance to debug this, but wanted to get it up in case someone has some free cycles.
Currently to test this (I have a more working version that does everything except upload files...yeah, I know that's not very useful) I generate a pair of API keys (they're in the above patch), but to do this I had create my own application.

I talked to Mike Conley and he thinks Brian King generated the original API key and is the one who has access to the Thunderbird application on box.com. Can anyone verify this / generate me new API keys?
Flags: needinfo?(bking)
Attached patch Patch v1 (obsolete) β€” β€” Splinter Review
This patch includes working functionality for:
- Logging in
- Displaying the management page (how much space is used, etc.)
- Uploading a file
- Deleting a file
- Getting a share link for a file

A few things to note:
- An error ("No such element") is thrown into the console when a file is deleted, but the file *IS* successfully deleted. I've been unable to track where this error is from.
- The API keys need to be updated, as mentioned previously. (which is why this is f? and not r?)
- It seems like new access tokens are fetched every time the preferences window is opened, is this expected? (It would be if the nsIMsgCloudFileProvider is instantiated each time instead of cached.)
- We need to ensure this doesn't break Lightning.
Attachment #8440655 - Attachment is obsolete: true
Attachment #8441816 - Flags: feedback?(philipp)
Attachment #8441816 - Flags: feedback?(mconley)
Comment on attachment 8441816 [details] [diff] [review]
Patch v1

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

I'd suggest obfuscating the client secret, for reasoning see the comment in calDavCalendar.js.

Also, a few more changes for OAuth.jsm we should do when this is moving to mailnews:

* Bonus points for adding a method doXHR(aUri, aPostData, aSuccess, aFailure, thisObj) that just calls httpRequest, so I can easily override it in the gdata provider to support support both tb24 (with app/modules/http.jsm) and tb25+ (with gre/modules/Http.jsm)
* Make the URI for the browserRequest.xul a property on the oauth object, so it can be easily modified
* Add aError/aData parameters to onAuthorizationFailed so the data from the httpRequest can be accessed
* Add (at least) aData parameter to connectFailureCallback and pipe it through in onAccessTokenFailed and onAuthorizationFailed
* For manual callers of onAuthorizationFailed and friends, pass a nserror code and a JSON string that looks like a correct error. I used these:
@@ -65,7 +94,7 @@
             this.requestAccessToken(this.refreshToken, OAuth2.CODE_REFRESH);
         } else {
             if (!aWithUI) {
-                aFailure();
+                aFailure('{ "error": "auth_noui" }');
                 return;
             }
             this.connecting = true;
@@ -93,7 +121,7 @@
                 }
 
                 this.account.finishAuthorizationRequest();
-                this.account.onAuthorizationFailed();
+                this.account.onAuthorizationFailed(Components.results.NS_ERROR_ABORT, '{ "error": "cancelled"}');
             },
 
             loaded: function (aWindow, aWebProgress) {

::: mailnews/base/util/OAuth2.jsm
@@ +29,1 @@
>      this.tokenURI = aBaseURI + "oauth2/token";

If you modify the authURI like this, maybe it also makes sense to provide a similar parameter for the token uri?

@@ +80,5 @@
>              ["redirect_uri", this.completionURI],
> +        ];
> +        // The scope can be optional.
> +        if (this.scope)
> +          params.push(["scope", this.scope]);

Does it confuse servers if the scope parameter is there but empty? It would help keep the code cleaner :)

In any case, I suggest {} even for one line ifs.
Attachment #8441816 - Flags: feedback?(philipp) → feedback+
> * Bonus points for adding a method doXHR(aUri, aPostData, aSuccess, aFailure, thisObj) that just calls 
> httpRequest, so I can easily override it in the gdata provider to support support both tb24 (with 
> app/modules/http.jsm) and tb25+ (with gre/modules/Http.jsm)

Hmm a hack like this would require lazy-loading Http.jsm, or ignoring if the import failure. Are you targeting tb31 with this? If so I would instead appreciate a temporary solution for tb31, supporting both http.jsm's directly in OAuth2.jsm. I have code to do this if you need it:

+let compatXHR = (function() {
+  let scope = {};
+  try {
+    // Thunderbird 25+
+    Cu.import("resource://gre/modules/Http.jsm", scope);
+  } catch (e) {
+    // Thunderbird 24
+    Cu.import("resource:///modules/http.jsm", scope);
+  }
+
+  if ("httpRequest" in scope) {
+    // Thunderbird 25+
+    return function(uri, postData, success, failure, thisObj) {
+      scope.httpRequest(uri, {
+        postData: postData,
+        onLoad: success.bind(thisObj),
+        onError: failure.bind(thisObj)
+      });
+    };
+  } else {
+    // Thunderbird 24
+    return function(uri, postData, success, failure, thisObj) {
+      scope.doXHRequest(uri, null, postData, success, failure, thisObj);
+    };
+  }
+})();
If Brian King doesn't know about the Box API key, Standard8 might.
Flags: needinfo?(standard8)
Comment on attachment 8441816 [details] [diff] [review]
Patch v1

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

This looks good, and looks like it cleans up the code a bunch, which is good!

It might be a good idea to write some tests for this component at some point, but I really don't think we should block landing this stuff on that.

Great job on this, clokep.

::: mail/components/cloudfile/nsBox.js
@@ +645,5 @@
> +                this.callback(this.requestObserver,
> +                  Ci.nsIMsgCloudFileProvider.uploadErr);
> +              }
> +
> +              // Can't use download_url because free accounts do not have access

Can the user profile tell us whether or not they're on a paid account? If so, we might want to use download_url in that case.
Attachment #8441816 - Flags: feedback?(mconley) → feedback+
(In reply to Philipp Kewisch [:Fallen] from comment #8)
> I'd suggest obfuscating the client secret, for reasoning see the comment in
> calDavCalendar.js.
I wasn't going to worry about this until I had real keys -- those keys only work w/ my account.

> Also, a few more changes for OAuth.jsm we should do when this is moving to
> mailnews:
I wanted to keep these changes as small as possible to uplift them to c-beta (to get into TB 31). Would it be alright with you to file a follow up with these changes afterward? (Targeting c-c, not c-b.)

> * Bonus points for adding a method doXHR(aUri, aPostData, aSuccess,
> aFailure, thisObj) that just calls httpRequest, so I can easily override it
> in the gdata provider to support support both tb24 (with
> app/modules/http.jsm) and tb25+ (with gre/modules/Http.jsm)
Why do you need compatibility with both?

> ::: mailnews/base/util/OAuth2.jsm
> @@ +29,1 @@
> >      this.tokenURI = aBaseURI + "oauth2/token";
> 
> If you modify the authURI like this, maybe it also makes sense to provide a
> similar parameter for the token uri?
My plan is to file a follow up to get this into m-c, I figured I'd redo some of the API as part of that (and probably pass in an options object, similar to Http.jsm).

> @@ +80,5 @@
> >              ["redirect_uri", this.completionURI],
> > +        ];
> > +        // The scope can be optional.
> > +        if (this.scope)
> > +          params.push(["scope", this.scope]);
> 
> Does it confuse servers if the scope parameter is there but empty? It would
> help keep the code cleaner :)

I'll try this and revert this change if so.

(In reply to Mike Conley (:mconley) from comment #11)
> It might be a good idea to write some tests for this component at some
> point, but I really don't think we should block landing this stuff on that.

I definitely won't have time to do that soon, unfortunately.

> ::: mail/components/cloudfile/nsBox.js
> @@ +645,5 @@
> > +                this.callback(this.requestObserver,
> > +                  Ci.nsIMsgCloudFileProvider.uploadErr);
> > +              }
> > +
> > +              // Can't use download_url because free accounts do not have access
> 
> Can the user profile tell us whether or not they're on a paid account? If
> so, we might want to use download_url in that case.

It looks like there's a "can_download" parameter we get back. I won't be able to test this, however.

I'll try to get a new patch up for you guys soon! Thanks for the feedback. :)
(In reply to Patrick Cloke [:clokep] from comment #12)
> (In reply to Philipp Kewisch [:Fallen] from comment #8)
> > I'd suggest obfuscating the client secret, for reasoning see the comment in
> > calDavCalendar.js.
> I wasn't going to worry about this until I had real keys -- those keys only
> work w/ my account.
Ah ok, no problem. I thought these were the final keys when I wrote that comment.

> > Also, a few more changes for OAuth.jsm we should do when this is moving to
> > mailnews:
> I wanted to keep these changes as small as possible to uplift them to c-beta
> (to get into TB 31). Would it be alright with you to file a follow up with
> these changes afterward? (Targeting c-c, not c-b.)
The reason I mentioned all these changes was that I would like to use OAuth2.jsm for the gdata provider, for which I need to release a new version before December when they shut down the v1 API. Since I don't want to leave tb24 users in the dark, I would like to make it compatible with both tb24 and tb31. Therefore I'd love to have the possibility to support both versions easily without duplicating the file. There are a few extensions I'd like to do, i.e specifying which calendar is being authenticated for when showing the auth dialog.

I can of course include my own copy of OAuth2.jsm in the gdata-provider, but this means it will stick around until TB38 or TB45. I'm fine with doing that if you say you'd like to keep changes low, I just thought I'd mention it just in case.
(In reply to Philipp Kewisch [:Fallen] from comment #13)
> I can of course include my own copy of OAuth2.jsm in the gdata-provider, but
> this means it will stick around until TB38 or TB45. I'm fine with doing that
> if you say you'd like to keep changes low, I just thought I'd mention it
> just in case.

I totally understand, but frankly I'm a bit scared of breaking Lightning as is, so I don't want to add more changes to this. I'll happily work with you and do reviews for you for these changes though! (Although I can't review things in mail/ so I'd need a sr from Mike or someone else.)

(In reply to Patrick Cloke [:clokep] from comment #12)
> > ::: mail/components/cloudfile/nsBox.js
> > @@ +645,5 @@
> > > +                this.callback(this.requestObserver,
> > > +                  Ci.nsIMsgCloudFileProvider.uploadErr);
> > > +              }
> > > +
> > > +              // Can't use download_url because free accounts do not have access
> > 
> > Can the user profile tell us whether or not they're on a paid account? If
> > so, we might want to use download_url in that case.
> 
> It looks like there's a "can_download" parameter we get back. I won't be
> able to test this, however.

So this parameter is unrelated: it's whether the /current user/ can download the file or not. That's pretty useless.

I'll take care of using and obfuscating the new keys and get a new patch up tonight.
Flags: needinfo?(standard8)
Flags: needinfo?(bking)
Attached patch Patch v2 (obsolete) β€” β€” Splinter Review
Sorry if I missed some review comments, but this is the minimum set of changes I could get everything to work with.

I tested:
- Creating a box account and uploading a file to it (and getting the info, deleting a file, etc.)
- Creating a Google CalDAV Calendar

I did not try migrating an account from the old API to the new one, but I think it will work OK. I plan to test this tomorrow.
Attachment #8441816 - Attachment is obsolete: true
Attachment #8446281 - Flags: review?(philipp)
Attachment #8446281 - Flags: review?(mconley)
(In reply to Patrick Cloke [:clokep] from comment #14)
> (In reply to Philipp Kewisch [:Fallen] from comment #13)
> > I can of course include my own copy of OAuth2.jsm in the gdata-provider, but
> > this means it will stick around until TB38 or TB45. I'm fine with doing that
> > if you say you'd like to keep changes low, I just thought I'd mention it
> > just in case.
> 
> I totally understand, but frankly I'm a bit scared of breaking Lightning as
> is, so I don't want to add more changes to this. I'll happily work with you
> and do reviews for you for these changes though! (Although I can't review
> things in mail/ so I'd need a sr from Mike or someone else.)
Sounds good to me :-) The changes are quite minor, I'll attach a patch as soon as I get to it.
I sent an email to :mconley with the login details of the Box account that holds our API Key. I'm not sure who should be the guardian of this moving forward, but probably not me.
Mike forwarded it to me, thanks Brian! I was able to grab new keys from there.
Comment on attachment 8446281 [details] [diff] [review]
Patch v2

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

::: mail/components/cloudfile/nsBox.js
@@ +28,5 @@
>                                     "nsIExternalProtocolService");
>  
>  function nsBox() {
>    this.log = Log4Moz.getConfiguredLogger("BoxService");
> +  this._oauth = new OAuth2(kAuthBaseUrl, null, kClientId, kClientSecret, kAuthUrl);

What are kClientId and kClientSecret? These don't appear to be defined anywhere.
Comment on attachment 8446281 [details] [diff] [review]
Patch v2

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

This looks fine - but as I said in IRC, I had a failure with an old Box account attempting to upgrade because during the authentication step, the Box server complained about an invalid scope. Apparently, sending an empty scope is not what we want to do - we should send no scope at all. When I manually removed the scope from OAuth2.jsm, all was well.
Attachment #8446281 - Flags: review?(mconley) → review-
Attached patch Patch v3 (obsolete) β€” β€” Splinter Review
(In reply to Mike Conley (:mconley) from comment #19)
> Comment on attachment 8446281 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 8446281 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/components/cloudfile/nsBox.js
> @@ +28,5 @@
> >                                     "nsIExternalProtocolService");
> >  
> >  function nsBox() {
> >    this.log = Log4Moz.getConfiguredLogger("BoxService");
> > +  this._oauth = new OAuth2(kAuthBaseUrl, null, kClientId, kClientSecret, kAuthUrl);
> 
> What are kClientId and kClientSecret? These don't appear to be defined
> anywhere.

I explained this over IRC, but I'll include it here too for someone reading the bug: these are defined at the bottom of the function in an obfuscated block of code. I can show someone the code I used to generate it, but I won't be attaching it here since that kind of defeats the purpose.

I've uploaded a new patch that fixes the scope changes (similar to how I had it in Patch v1, but taking into account Philipp's requested changes).
Attachment #8446281 - Attachment is obsolete: true
Attachment #8446281 - Flags: review?(philipp)
Attachment #8447752 - Flags: review?(philipp)
Attachment #8447752 - Flags: review?(mconley)
For future reference, maybe include a code comment about that.
(In reply to Magnus Melin from comment #22)
> For future reference, maybe include a code comment about that.

About what? Please be more specific. There's a huge block comment above the obfuscated code already, please take a look at the patch.
I meant the part that "kClientId and kClientSecret are defined in obfuscated code"
or is that simply assigned only? in that case add the declarations too
Comment on attachment 8447752 [details] [diff] [review]
Patch v3

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

This looks good, but I really want to see us make an effort to use download_url if we detect that the user account can handle it.

::: mail/components/cloudfile/nsBox.js
@@ +28,5 @@
>                                     "nsIExternalProtocolService");
>  
>  function nsBox() {
>    this.log = Log4Moz.getConfiguredLogger("BoxService");
> +  this._oauth = new OAuth2(kAuthBaseUrl, null, kClientId, kClientSecret, kAuthUrl);

I agree that while we don't want to disclose the values of kClientId and kClientSecret, to ease future debugging / development, we should add a comment here explaining where these values are defined and why they're defined that way.

@@ +645,5 @@
> +                this.callback(this.requestObserver,
> +                  Ci.nsIMsgCloudFileProvider.uploadErr);
> +              }
> +
> +              // Can't use download_url because free accounts do not have access

Why about my suggestion to use download_url if we _are_ using a paid account? This isn't an end-of-the-world thing if we can't do this, but it'd be nice to give the people who pay for the service the experience they've paid for.
Attachment #8447752 - Flags: review?(mconley) → review+
(In reply to Patrick Cloke [:clokep] from comment #23)
> (In reply to Magnus Melin from comment #22)
> > For future reference, maybe include a code comment about that.
> 
> About what? Please be more specific. There's a huge block comment above the
> obfuscated code already, please take a look at the patch.

I would advise against adding an extra comment for kClientId etc, the point is to obfuscate these and not point out what needs to be evaluated to get around the obfuscation.
(In reply to Mike Conley (:mconley) from comment #26)
> Comment on attachment 8447752 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 8447752 [details] [diff] [review]:
First, thank you for the review.

> This looks good, but I really want to see us make an effort to use
> download_url if we detect that the user account can handle it.
>
> @@ +645,5 @@
> > +                this.callback(this.requestObserver,
> > +                  Ci.nsIMsgCloudFileProvider.uploadErr);
> > +              }
> > +
> > +              // Can't use download_url because free accounts do not have access
> 
> Why about my suggestion to use download_url if we _are_ using a paid
> account? This isn't an end-of-the-world thing if we can't do this, but it'd
> be nice to give the people who pay for the service the experience they've
> paid for.

I did make this effort and was unable to do it, I had previously responded to this request:

(In reply to Patrick Cloke [:clokep] from comment #14)
> (In reply to Patrick Cloke [:clokep] from comment #12)
> > > ::: mail/components/cloudfile/nsBox.js
> > > @@ +645,5 @@
> > > > +                this.callback(this.requestObserver,
> > > > +                  Ci.nsIMsgCloudFileProvider.uploadErr);
> > > > +              }
> > > > +
> > > > +              // Can't use download_url because free accounts do not have access
> > > 
> > > Can the user profile tell us whether or not they're on a paid account? If
> > > so, we might want to use download_url in that case.
> > 
> > It looks like there's a "can_download" parameter we get back. I won't be
> > able to test this, however.
> 
> So this parameter is unrelated: it's whether the /current user/ can download
> the file or not. That's pretty useless.
> 
> I'll take care of using and obfuscating the new keys and get a new patch up
> tonight.

I could not figure out any other way to check if you have an account that can directly send download links or not. If you'd like me to try again, I can. I looked at the return from the file share request as well as the user info. It's possible I missed it -- not all the fields are well documented. Alternately, if we have a contact at Box, we could ask them directly.

> ::: mail/components/cloudfile/nsBox.js
> @@ +28,5 @@
> >                                     "nsIExternalProtocolService");
> >  
> >  function nsBox() {
> >    this.log = Log4Moz.getConfiguredLogger("BoxService");
> > +  this._oauth = new OAuth2(kAuthBaseUrl, null, kClientId, kClientSecret, kAuthUrl);
> 
> I agree that while we don't want to disclose the values of kClientId and
> kClientSecret, to ease future debugging / development, we should add a
> comment here explaining where these values are defined and why they're
> defined that way.

I do not believe we should, as this makes it really obvious that evaluating the obfuscated values will give you the ClientId and ClientSecret.
Ok, that's fine.

Let's try this:

1) Let's get Fallen to sign off on this (or fix up any problems he finds)
2) Let's land this as-is on central ASAP and get people to run it through some paces
3) Let's request uplift approval for aurora and beta, and prepare a Box-provider disabling patch if we can't get the uplift
4) Let's file follow-up bugs for the paid-Box service thing, since I think it's worth pursuing, but definitely not worth blocking this on.

How does that sound?
Mike, that seems like a good way forward. Thanks!
Comment on attachment 8447752 [details] [diff] [review]
Patch v3

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

r=philipp.

::: mailnews/base/util/OAuth2.jsm
@@ +80,5 @@
>              ["redirect_uri", this.completionURI],
> +        ];
> +        // The scope can be optional.
> +        if (this.scope) {
> +          params.push(["scope", this.scope]);

Please adapt to 4-space indent to match the rest of the file.
Attachment #8447752 - Flags: review?(philipp) → review+
Attached patch Patch v3 extras β€” β€” Splinter Review
Here are the extras I am suggesting. One very important change that I think we should take in any case is opening browserRequest.xul in private mode, which avoids autocomplete and some other things that you might want to keep private.
Attachment #8452341 - Flags: review?(clokep)
Attachment #8452341 - Flags: review?(clokep) → review+
(In reply to Philipp Kewisch [:Fallen] from comment #32)
> Created attachment 8452341 [details] [diff] [review]
> Patch v3 extras
> 
> Here are the extras I am suggesting. One very important change that I think
> we should take in any case is opening browserRequest.xul in private mode,
> which avoids autocomplete and some other things that you might want to keep
> private.
That change was nicely hidden in there! Thanks for the improvements!

Once the tree is open I'll check these in.
Attached patch Patch v3.1 β€” β€” Splinter Review
Carrying forward my r+ from attachment 8447752 [details] [diff] [review]. This fixes the space of scope as Fallen asked for (and switches the commit message to r=mconley,philipp instead of Fallen).
Attachment #8447752 - Attachment is obsolete: true
Attachment #8452625 - Flags: review+
Keywords: checkin-needed
I didn't include the tb24 compat bits in the extras patch because I had the feeling you'd prefer not to include them. If thats not the case I can provide another patch.
(In reply to Philipp Kewisch [:Fallen] from comment #35)
> I didn't include the tb24 compat bits in the extras patch because I had the
> feeling you'd prefer not to include them. If thats not the case I can
> provide another patch.

Can we do that in a separate bug? This one is already getting quite long! Thanks.
As I have observed my first occurrence in Support related to this. (at least I think it is the changed API.) The situation of creating a link and sending an attachment is not desirable.  https://support.mozilla.org/en-US/questions/1010025

Is there a version of Thunderbird that currently supports box? I don't see a commit, so I assume not.

Is there a bug for the TB24 bits?
https://hg.mozilla.org/comm-central/rev/5a11b67a0879
https://hg.mozilla.org/comm-central/rev/2e7786b176ff
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 33.0
Comment on attachment 8452625 [details] [diff] [review]
Patch v3.1

Approval Request Comment
[Feature/regressing bug #]: N/A, Box.com made API changes
[User impact if declined]: Users will be unable to use Box.com as a FileLink provider for TB 31.
[Describe test coverage new/current, TBPL]: Testing was done manually by both mconley and I.
[Risks and why]: This has not been insanely tested, but both Mike and I tested it with our accounts. This has not been baking on Daily for a long time due to all the closures.
[String/UUID change made/needed]: N/A

I assume we'd want to land both this and the OAuth extras patch. Fallen, please correct me if this is wrong.
Attachment #8452625 - Flags: approval-mozilla-beta?
Attachment #8452625 - Flags: approval-mozilla-aurora?
Comment on attachment 8452625 [details] [diff] [review]
Patch v3.1

I guess you meant comm instead of Mozilla.
Attachment #8452625 - Flags: approval-mozilla-beta?
Attachment #8452625 - Flags: approval-mozilla-beta-
Attachment #8452625 - Flags: approval-mozilla-aurora?
Attachment #8452625 - Flags: approval-mozilla-aurora-
Attachment #8452625 - Flags: approval-comm-beta?
Attachment #8452625 - Flags: approval-comm-aurora?
Oops! I did mean comm-, thanks sylvestre.
Comment on attachment 8452625 [details] [diff] [review]
Patch v3.1

a=Standard8 for both patches
Attachment #8452625 - Flags: approval-comm-beta?
Attachment #8452625 - Flags: approval-comm-beta+
Attachment #8452625 - Flags: approval-comm-aurora?
Attachment #8452625 - Flags: approval-comm-aurora+
Depends on: 1045845
You need to log in before you can comment on or make changes to this bug.