1504508 - Add and Ship WeTransfer FileLink Provider

Status: RESOLVED FIXED

(Thunderbird :: FileLink, enhancement)

Description

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Provider as WebExtension with Experiment - v1

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.

Comment 1

[:Gijs (he/him)]

Why does this need to be security-sensitive?

Comment 2

[Magnus Melin [:mkmelin]]

(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.

Comment 3

[Magnus Melin [:mkmelin]]

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?

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Provider as bootstrapped add-on - v1

This is the bootstrapped version of the add-on. The code is meant to be as similar to the WebExtension as possible.

Comment 5

[Philipp Kewisch [:Fallen][📱📆]]

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.

Comment 6

[Dylan Hardison [:dylan] (he/him)]

The content of attachment 9022577 [details] has been deleted for the following reason:

Deleted by the request of the user

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Provider as bootstrapped add-on - v1

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.

Comment 8

[Geoff Lankow (:darktrojan)]

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".

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

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?

Comment 10

[Geoff Lankow (:darktrojan)]

Attachment: 1504508-wetransfer-extension-1.diff

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.

Comment 11

[Geoff Lankow (:darktrojan)]

Actually I just remembered, it's not just minor changes. I converted the whole thing to use the new v2 API.

Comment 12

[Philipp Kewisch [:Fallen][📱📆]]

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/[e-mail]'
> +else:
> +    DIST_SUBDIR = 'distribution/extensions/[e-mail]'

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.

Comment 13

[Ryan Lee Sipes]

Looks like WeTransfer is going to sign the contract before the end of the week. Is this in a state that is shippable today?

Comment 14

[Geoff Lankow (:darktrojan)]

It isn't. There are unresolved issues in bug 1481052, and a few here (mostly copy, L10n).

Comment 15

[Geoff Lankow (:darktrojan)]

Attachment: 1504508-wetransfer-extension-2.diff

Current version.

Comment 16

[Geoff Lankow (:darktrojan)]

(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.)

Comment 17

[Geoff Lankow (:darktrojan)]

Attachment: 1504508-wetransfer-extension-3.diff

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.

Comment 18

[Geoff Lankow (:darktrojan)]

And by ship, I mean in Daily, for at least a little while.

Comment 19

[Geoff Lankow (:darktrojan)]

Oh, except the settings page still seems a bit weird and there's no L10n.

Comment 20

[Philipp Kewisch [:Fallen][📱📆]]

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": "[e-mail]",

Let's use [e-mail]. 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/[e-mail]'

Same

Comment 21

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/comm-central/rev/ffb6350a5d0cf0a5f2c813368c06473a54cd44e2

Comment 22

[Jorg K (CEST = GMT+2)]

Let me know if you want this uplifted somewhere.

Comment 23

[Geoff Lankow (:darktrojan)]

Not yet, it needs some time on beta, but I'll set the tracking flag anyway so we don't forget about it.

Comment 24

[Geoff Lankow (:darktrojan)]

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.

Comment 25

[Philipp Kewisch [:Fallen][📱📆]]

Filed bug 1515502

Comment 26

[Jorg K (CEST = GMT+2)]

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 :-(

Comment 27

[Geoff Lankow (:darktrojan)]

Attachment: 1504508-wetransfer-extension-esr-part1.diff

Try build in progress:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f137d9d616a7f8a1b27da59916ba00ee242e45da

Comment 28

[Geoff Lankow (:darktrojan)]

That could've gone better. That fake install.rdf we add to make sure the thing is packaged, causes it to refuse to install.

Comment 29

[Geoff Lankow (:darktrojan)]

Attachment: 1504508-wetransfer-extension-esr-part2.diff

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.

Comment 30

[Geoff Lankow (:darktrojan)]

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=589dd4cf2442ec7430f440e59c433bf37a0b7d2f

Comment 31

[Jorg K (CEST = GMT+2)]

Comment on attachment 9034045 [details] [diff] [review]
1504508-wetransfer-extension-esr-part1.diff

Good for TB 60.5.

Comment 32

[Geoff Lankow (:darktrojan)]

Attachment: 1504508-wetransfer-extension-esr-part1.diff

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

Comment 33

[Jorg K (CEST = GMT+2)]

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.

Comment 34

[Geoff Lankow (:darktrojan)]

Attachment: 1518076-wetransfer-github-esr-1.diff

Comment 35

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-esr60/rev/19f893e2323e4dd72f9205fc3b329e2c3e6479dd
https://hg.mozilla.org/releases/comm-esr60/rev/463c57584f10cf58ca372338d0cb2f92c99d5a4a
https://hg.mozilla.org/releases/comm-esr60/rev/e07530156f47eceedb59cb8dcc34c1fc32a7d43c - Bug 1518076, locales.

Comment 36

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-esr60/rev/6f39ac4f8355f15bdede6a539ccbf4072420bc9b - fix duplicate bustage

Comment 37

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-esr60/rev/44f7741e0287b454d1584ea0880ba1989271c035

Comment 38

[Jorg K (CEST = GMT+2)]

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?

Comment 39

[Geoff Lankow (:darktrojan)]

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

Comment 40

[Geoff Lankow (:darktrojan)]

Does this bug still need to be hidden?

Comment 41

[Philipp Kewisch [:Fallen][📱📆]]

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

Comment 42

[Geoff Lankow (:darktrojan)]

Attachment: 1504508-add-account-button-1.diff

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.

Comment 43

[Geoff Lankow (:darktrojan)]

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

Comment 44

[Philipp Kewisch [:Fallen][📱📆]]

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.

Comment 45

[Philipp Kewisch [:Fallen][📱📆]]

(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 46

[Jorg K (CEST = GMT+2)]

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

https://hg.mozilla.org/releases/comm-esr60/rev/162c34e56ff8df3104b3a3895d7c04fb9836e5c0

Comment 47

[Jorg K (CEST = GMT+2)]

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?

Comment 48

[Geoff Lankow (:darktrojan)]

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

Comment 49

[Jorg K (CEST = GMT+2)]

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.