Closed Bug 1504508 Opened 3 years ago Closed 3 years ago

Add and Ship WeTransfer FileLink Provider

Categories

(Thunderbird :: FileLink, enhancement)

enhancement
Not set
normal

Tracking

(thunderbird_esr6065+ fixed, thunderbird65 fixed)

RESOLVED FIXED
Thunderbird 65.0
Tracking Status
thunderbird_esr60 65+ fixed
thunderbird65 --- fixed

People

(Reporter: Fallen, Assigned: darktrojan)

References

Details

(Keywords: sec-other)

Attachments

(5 files, 6 obsolete files)

12.13 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
5.71 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
16.09 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
28.67 KB, patch
Details | Diff | Splinter Review
2.88 KB, patch
Fallen
: review+
Details | Diff | Splinter Review
We're working with WeTransfer to ship their FileLink provider. This bug is to prepare the engineering work. I'm keeping this private until we've finalized the contractual details within the Thunderbird Council.

I have the extension ready and working as a WebExtension with an experimental API based on the initial version in bug 1481052. The same code can easily be added as a bootstrapped extension, of which I have a slightly outdated version.

Here are things left to do:
* Figure out how to ship the code:
This could either be as normal code in mail/components/cloudfile/ or we could try shipping it as a system add-on. The system add-on could either be:
- The WebExtension including the experiment
- We finish bug 1481052 and adapt the add-on code to use the new API (may not work due to strings and the required uplift)
- A bootstrapped version of the extension

* Figure out where to store the secrets:
There is an API key involved. I believe our gmail oauth has some special sauce that allows us to only add the keys during the build process. The calendar gdata provider uses an obfuscated code block and a guilty conscience comment. I think the special sauce in combination with a guilty conscience comment would be best.

* Engineering in certain cases:
Depending on how we ship, the code may need to be adapted. This is sufficiently above in the shipping task and the paragraph above.

* QA testing to make sure it still works:
It has been a while since I touched this, so it is possible something doesn't quite work. Testing is pretty straightforward.

Magnus, can you make sure the remaining tasks have an owner? I'm happy to help with anything.
Flags: needinfo?(mkmelin+mozilla)
Why does this need to be security-sensitive?
Flags: needinfo?(philipp)
(In reply to :Gijs (he/him) from comment #1)
> Why does this need to be security-sensitive?

Just to keep private until we've finalized the contractual details within the Thunderbird Council.
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(philipp)
I think Geoff is most knowledgable here, and he's involved in the related bugs too.

For the API key I'm not aware of a special sauce added by the build process (but I could be wrong). 
Using obfuscated code block and a guilty conscience comment should be ok. These kind of secrets aren't super-secret anyway. Philipp, do we already have an API key? Official or for testing.

It would be nice if we could ship it as a WebExtension in one form or the other, so we don't have to go through a change cycle for 68. But it depends on the timing, as we'd like to ship this very soon. Geoff, any thoughts?
Assignee: nobody → geoff
Attached file Provider as bootstrapped add-on - v1 (obsolete) (deleted) β€”
This is the bootstrapped version of the add-on. The code is meant to be as similar to the WebExtension as possible.
I have an API key which I believe will also be the official API key. Geoff, ping me on IRC when you need it. I double checked and there is indeed no special sauce involved, so we can use an obfuscated code block and a guilty conscience comment as you mention.
Flags: needinfo?(geoff)
The content of attachment 9022577 [details] has been deleted for the following reason:

Deleted by the request of the user
Attached file Provider as bootstrapped add-on - v1 (obsolete) β€”
and now again without the API key included. It will be in the final code anyway, but it doesn't need to be exposed more than needed.
I got my own API key. Seems okay so far, except:

* The settings/management page really needs some explanation text. I was expecting to have to do something, but what I got was a picture (which loaded really slowly, and not at all once).
* Uploading didn't work, when clicking the link I see the file name and "This board is empty".
Flags: needinfo?(geoff)
We could possibly remove the picture, I added it because I read about the WeTransfer Moment add-on and it seemed like a good space filler. Otherwise, I'm sure we can use some copy instead.

As for the upload, I haven't tried recently. Maybe they changed something in their API again? Can you find out what the issue is or should I take a look?
Attached patch 1504508-wetransfer-extension-1.diff (obsolete) β€” β€” Splinter Review
This'll have the extension built from the tree and put in extensions, or distribution/extensions, like Lightning is. I'll post the API itself in bug 1481052. Note I have actually made some changes, including an upload aborted event so we can handle that properly.

I think it's worth doing it this way right up to ESR60. It'd be worth having the API available there so extensions for other services can use it.
Attachment #9023510 - Flags: feedback?(philipp)
Actually I just remembered, it's not just minor changes. I converted the whole thing to use the new v2 API.
Comment on attachment 9023510 [details] [diff] [review]
1504508-wetransfer-extension-1.diff

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

I'm all for making the API available in 60, this will also make it easier to transition. In 68, a pure WebExtension should be the way to do cloudfile extensions instead of the xpcom way we now have.

A few comments:

::: mail/components/cloudfile/wetransfer/background/background.js
@@ +1,5 @@
> +/* 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/. */
> +
> +/* globals browser */

Probably makes sense to put this into the .eslintrc.js for mail/components/cloudfile/wetransfer instead.

@@ +116,5 @@
> +
> +browser.cloudfile.onUploadFile.addListener(async ({ id, name, data }) => {
> +  let session = getSession();
> +  let controller = new AbortController();
> +  abortControllers.set(id, controller);

As mentioned in the other bug, I think we should have the API handle the AbortController. Would be interested to hear why you removed the signal from the onUploadFile call.

The signal can directly be passed to fetch, and if an add-on needs additional cleanup in something like onUploadAborted, then can instead do signal.addEventListener("abort", () => cleanup_here());

::: mail/components/cloudfile/wetransfer/moz.build
@@ +4,5 @@
> +
> +if CONFIG['NIGHTLY_BUILD']:
> +    DIST_SUBDIR = 'extensions/wetransfer-cloudfile@mozilla.kewis.ch'
> +else:
> +    DIST_SUBDIR = 'distribution/extensions/wetransfer-cloudfile@mozilla.kewis.ch'

We should consider making this a system add-on instead, i.e. put it in the features/ subdir. You can check screenshots and formautofill on how it is done there, and it may need to go into that json file that holds the ids of system addons.
Attachment #9023510 - Flags: feedback?(philipp) → feedback+
Looks like WeTransfer is going to sign the contract before the end of the week. Is this in a state that is shippable today?
Flags: needinfo?(philipp)
Flags: needinfo?(geoff)
It isn't. There are unresolved issues in bug 1481052, and a few here (mostly copy, L10n).
Flags: needinfo?(geoff)
Attached patch 1504508-wetransfer-extension-2.diff (obsolete) β€” β€” Splinter Review
Current version.
Attachment #9023510 - Attachment is obsolete: true
(In reply to Philipp Kewisch [:Fallen] [:πŸ“†] from comment #12)
> > +/* globals browser */
> 
> Probably makes sense to put this into the .eslintrc.js for
> mail/components/cloudfile/wetransfer instead.

browser is only available in background.js at the moment. The preferences iframe doesn't get the WebExtensions environment injected yet. (I assume that's what you're getting at.)
I'm building right now so I can't double-check this, but I'm pretty sure I left it in a working state. I think once the obfuscating magic happens it's ready to ship.
Attachment #9022431 - Attachment is obsolete: true
Attachment #9022592 - Attachment is obsolete: true
Attachment #9024879 - Attachment is obsolete: true
Flags: needinfo?(philipp)
Attachment #9028877 - Flags: review?(philipp)
And by ship, I mean in Daily, for at least a little while.
Oh, except the settings page still seems a bit weird and there's no L10n.
Comment on attachment 9028877 [details] [diff] [review]
1504508-wetransfer-extension-3.diff

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

Glad the system add-on thing was so easy! 

I've sent you some details on the obfuscated block via email

::: mail/components/cloudfile/wetransfer/background/background.js
@@ +136,5 @@
> +});
> +
> +browser.cloudFile.getAllAccounts().then(async (accounts) => {
> +  for (let account of accounts) {
> +    await browser.cloudFile.updateAccount(account.id, { uploadSizeLimit: TWO_GB });

Hmm I see. As a followup we should persist the quota on the wx api side. Providers shouldn't have to update the quota on each extension start.

::: mail/components/cloudfile/wetransfer/manifest.json
@@ +4,5 @@
> +  "description": "Share your attachments via WeTransfer",
> +  "version": "2.0.0",
> +  "applications": {
> +    "gecko": {
> +      "id": "wetransfer@thunderbird.net",

Let's use cloudfile-wetransfer@extensions.thunderbird.net. We never know if there may be another wetransfer add-on, and I'd like to avoid it being directly on @thunderbird.net because our email lands there.

::: mail/components/cloudfile/wetransfer/moz.build
@@ +1,5 @@
> +# 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/.
> +
> +DIST_SUBDIR = 'features/wetransfer@thunderbird.net'

Same
Attachment #9028877 - Flags: review?(philipp) → review+
https://hg.mozilla.org/comm-central/rev/ffb6350a5d0cf0a5f2c813368c06473a54cd44e2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 65.0
Let me know if you want this uplifted somewhere.
Not yet, it needs some time on beta, but I'll set the tracking flag anyway so we don't forget about it.
As we talked about on IRC, I've put the extension code into a git repository at https://github.com/thundernest/wetransfer-extension. I've converted the strings to .properties format for compatibility with Pontoon, and we can use the node module pontoon-to-webext to convert them back before uplifting back into comm-central.

Philipp, can you get this into Pontoon? I figure you're more likely to succeed than I am.
Flags: needinfo?(philipp)
Filed bug 1515502
Flags: needinfo?(philipp)
Group: mail-core-security → core-security-release
Comment on attachment 9028877 [details] [diff] [review]
1504508-wetransfer-extension-3.diff

The patch doesn't apply to ESR 60. Can you please rebase. Our advertising says that this will ship in TB 60.5 at the end of January, so it's time to get it ready. I usually land patches early so I can be the Guinea pig of the build.

BTW, the tracking query doesn't yield this private bug :-(
Flags: needinfo?(geoff)
That could've gone better. That fake install.rdf we add to make sure the thing is packaged, causes it to refuse to install.
To get around the packaging problems, I've converted the extension to a bootstrapped one with embedded WebExtension. I really hoped to avoid something like this, but it seems nothing is so easy.
Attachment #9034101 - Flags: review?(philipp)
Attachment #9034101 - Flags: approval-comm-esr60?
Attachment #9034045 - Attachment description: 1504508-wetransfer-extension-esr-1.diff → 1504508-wetransfer-extension-esr-part1.diff
Attachment #9034045 - Attachment filename: 1504508-wetransfer-extension-esr-1.diff → 1504508-wetransfer-extension-esr-part1.diff
Attachment #9034045 - Flags: approval-comm-esr60?
Attachment #9034101 - Flags: review?(philipp) → review+
Comment on attachment 9034045 [details] [diff] [review]
1504508-wetransfer-extension-esr-part1.diff

Good for TB 60.5.
Attachment #9034045 - Flags: approval-comm-esr60? → approval-comm-esr60+
Attachment #9034101 - Flags: approval-comm-esr60? → approval-comm-esr60+
Depends on: 1518076
Depends on: 1515502, 1481052

This time with the content actually localised, see bug 1518076 comment 6.

Attachment #9034045 - Attachment is obsolete: true
Attachment #9035439 - Flags: review+
Attachment #9035439 - Flags: approval-comm-release?
Attachment #9035439 - Flags: approval-comm-release? → approval-comm-esr60+

I'm planning to uplift this soon so we can all try it out before releasing it in about two weeks.

Please provide the patch promised in bug 1518076 comment #15.

Flags: needinfo?(geoff)
Flags: needinfo?(geoff)

I "accidentally" tried this in a locally compiled trunk build and WeTransfer is gone :-( - Working fine on TB 60.5 (preview) and TB 65 beta 2. What's happening?

Flags: needinfo?(geoff)

Sounds similar to what I saw. Restarting a few times fixed it.

Flags: needinfo?(geoff)

Does this bug still need to be hidden?

Flags: needinfo?(philipp)

I'd like to keep it hidden until we've made the announcements.

Flags: needinfo?(philipp)

This patch is only for ESR60. It changes the button label in the dialog from "Set Up Account" to "OK" if no settings URL is provided.

Attachment #9043179 - Flags: review?(philipp)
Attachment #9043179 - Flags: approval-comm-esr60?

Actually, it changes the label in all cases. I forgot things didn't happen how I originally intended.

Comment on attachment 9043179 [details] [diff] [review]
1504508-add-account-button-1.diff

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

You could consider making the OK button only show in specific cases, e.g. there are no settings. I think it would be acceptable in both cases though.
Attachment #9043179 - Flags: review?(philipp) → review+

(In reply to Geoff Lankow (:darktrojan) from comment #43)

Actually, it changes the label in all cases. I forgot things didn't happen how I originally intended.

Thats what I get for reading bugmail in order :) I see you got this.

Comment on attachment 9043179 [details] [diff] [review]
1504508-add-account-button-1.diff

https://hg.mozilla.org/releases/comm-esr60/rev/162c34e56ff8df3104b3a3895d7c04fb9836e5c0
Attachment #9043179 - Flags: approval-comm-esr60? → approval-comm-esr60+

Geoff, WeTransfer is completely broken now:

browser.messageManager is null ExtensionParent.jsm:365
_onExtensionBrowser resource://gre/modules/ExtensionParent.jsm:365:5
emit resource://gre/modules/ExtensionUtils.jsm:227:40
setIFrameSource chrome://messenger/content/cloudfile/addAccountDialog.js:188:5
AAD_accountTypeSelected chrome://messenger/content/cloudfile/addAccountDialog.js:294:5
AAD_onInit chrome://messenger/content/cloudfile/addAccountDialog.js:87:5
onload chrome://messenger/content/cloudfile/addAccountDialog.xul:1:8
addAccountDialog resource:///modules/cloudFileAccounts.js:246:5
CFT_addCloudFileAccount chrome://messenger/content/preferences/applications.js:808:22
doCommand chrome://messenger/content/preferences/applications.js:444:9
doCommand chrome://messenger/content/preferences/applications.js:488:5
goDoCommand chrome://global/content/globalOverlay.js:84:7
oncommand chrome://messenger/content/preferences/preferences.xul:1:1

And:

TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1550016139/build/tests/mozmill/cloudfile/test-cloudfile-add-account-dialog.js | test-cloudfile-add-account-dialog.js::setupModule
TEST-UNEXPECTED-FAIL | /Users/cltbld/tasks/task_1550016139/build/tests/mozmill/cloudfile/test-cloudfile-add-account-dialog.js | test-cloudfile-add-account-dialog.js::teardownModule

Did you do a try run of this before submitting an ESR(!!) patch to hit 25M users?

I have now. You need to land the patch from bug 1511945 that I requested approval for a week ago.

Sorry, I'm not clairvoyant. There was no indication made in that bug and usually we let patches ride the trains a little. Besides, my query doesn't see that bug since it has no target.

Group: core-security-release
You need to log in before you can comment on or make changes to this bug.