Closed Bug 1514522 Opened 5 years ago Closed 5 years ago

remove link to 3rd party site for account type add-ons

Categories

(Thunderbird :: Account Manager, task)

task
Not set
normal

Tracking

(thunderbird_esr6065+ fixed, thunderbird65 fixed, thunderbird66 fixed)

VERIFIED FIXED
Thunderbird 66.0
Tracking Status
thunderbird_esr60 65+ fixed
thunderbird65 --- fixed
thunderbird66 --- fixed

People

(Reporter: mkmelin, Assigned: mkmelin)

References

Details

Attachments

(1 file, 1 obsolete file)

Remove the beonex link even for nightlies. 
The release link will change too once we figure that out.
Attached patch bug1514522_beonex.patch (obsolete) — Splinter Review
Attachment #9031719 - Flags: review?(jorgk)
For reference, could have privacy and legal issues.
Version: 60 → Trunk
Comment on attachment 9031719 [details] [diff] [review]
bug1514522_beonex.patch

Sure, I believe it was my idea ;-)
22:08:23 - jorgk: You can just take the #if out and default it to the other item
Attachment #9031719 - Flags: review?(jorgk) → review+
Comment on attachment 9031719 [details] [diff] [review]
bug1514522_beonex.patch

Hold on, the https://live.thunderbird.net/autoconfig/addons.json was actually planned. So http://localhost/addons-test.json won't help anyone.

Why not just replace it in the #else branch in that case?
Attachment #9031719 - Flags: review+
(In reply to Magnus Melin [:mkmelin] from comment #0)
> The release link will change too once we figure that out.
I thought we had figured that out already, now?
Actually, that file is already there.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/63b0a933325a
remove link to 3rd party site for account type add-ons. r=jorgk
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Landed with:
pref("mailnews.auto_config.addons_url", "https://live.thunderbird.net/autoconfig/addons.json");
Target Milestone: --- → Thunderbird 66.0
Jörg, the localhost was meant so that it can be tested for anyone locally wanting to set up this kind of data. With the fix you landed there is still a privacy issue since the icon url goes to beonex.
Flags: needinfo?(jorgk)
Frankly, I don't understand any of this. You and Aceman reviewed this code:
https://hg.mozilla.org/comm-central/rev/1403c0fa3d1ac8b10ec74adb92b245575059298f#l22.12

+// The list of addons which can handle certain account types
+#ifdef RELEASE_OR_BETA
+pref("mailnews.auto_config.addons_url", "https://live.thunderbird.net/autoconfig/addons.json");
+#else
+pref("mailnews.auto_config.addons_url", "http://www.beonex.com/owl/addons-test.json");
+#endif

If it's not right, why did you approve it?

I didn't like it, so before landing, I talked to Ben:

15:20:48 - jorgk: Umm, the code still has http://www.beonex.com/owl/addons-test.json. Is that intended???
15:23:47 - BenB: das ist nur für dev, beta und release haben eine andere URL.
15:24:26 - BenB: ..., und ich brauche sancus, um die production-url zu pushen
15:24:50 - BenB: ich kann das in ein paar tagen ändern, nachdem sancus die neue datei hingelegt hat.

So apparently the www.beonex.com was going to be removed, which is what I did now since
  https://live.thunderbird.net/autoconfig/addons.json
is now online.

IMHO http://localhost/addons-test.json won't work since not every "localhost" is running a webserver, besides, there is no file path. Since this is a preference, people wishing to test it can change that preference to some file: URL. I don't think it makes sense to ship anything that does *not* work out of the box.

Sorry, I don't understand:
> ... there is still a privacy issue since the icon url goes to beonex

So why did you approve this code, see above, when there is an inherent problem?

All I can offer right now is to back out what I landed and you find a more qualified reviewer. To me your patch doesn't look right.
Flags: needinfo?(jorgk)
Comment on attachment 9031719 [details] [diff] [review]
bug1514522_beonex.patch

I don't think this is useful as per my previous comment.
Attachment #9031719 - Flags: feedback-
(In reply to Jorg K (GMT+1) from comment #10)
> IMHO http://localhost/addons-test.json won't work since not every
> "localhost" is running a webserver,

Of course, that's the idea. Only people actively working on it would set it up, until ready to ship.

 besides, there is no file path. Since
> this is a preference, people wishing to test it can change that preference
> to some file: URL. I don't think it makes sense to ship anything that does
> *not* work out of the box.

It is not intended to "ship" in current form. Just landed to iterate.

> Sorry, I don't understand:
> > ... there is still a privacy issue since the icon url goes to beonex
> 
> So why did you approve this code, see above, when there is an inherent
> problem?

I didn't notice. The patch is huge, probably 10 review iterations and phab doesn't work properly for that IMO.
 
> All I can offer right now is to back out what I landed and you find a more
> qualified reviewer. To me your patch doesn't look right.

I think it the patch I provides is correct. It let's people working on this set the pref/data to what they need. For everybody else there is nothing to see yet,
>   https://live.thunderbird.net/autoconfig/addons.json

I took a look at the data here, it seems icon32 is an URL. If this URL is hit every time the add-ons are displayed, we can have similar privacy issues. I proposed we change that to be a data url instead. Happy to put that into a separate bug if necessary.
(In reply to Magnus Melin [:mkmelin] from comment #12)
> (In reply to Jorg K (GMT+1) from comment #10)
> > IMHO http://localhost/addons-test.json won't work since not every
> > "localhost" is running a webserver,
> Of course, that's the idea. Only people actively working on it would set it
> up, until ready to ship.
How about making the preference empty and adding a comment:
  // Developers wishing to word on add-on integration, set this to a suitable value.
or similar.

You know what effort is involved to set up a web server on Windows? So it might as well be a file: URL.
(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #13)
> I proposed we change that to be a data url instead.
Good idea. So then pointing to the file as we do now wouldn't be an issue, would it?
Jörg, thanks for landing the correct patch. https://hg.mozilla.org/comm-central/rev/63b0a933325a is entirely correct. I had intended to do that once the json file at Thunderbird is fixed after Magnus (!) demanded file format changes, which I implemented, but I couldn't update the file on the server. That's fixed now, so the beonex URL is no longer needed, and it's fine to remove it.

It would be wrong to land localhost, because it would break the entire feature. If you want to test it, just change your local preference to another URL. I will do that from now on as well.

I'm a little insulted at the accusation that Beonex is a privacy threat. I'm a German entity, the server is hosted in Germany, and under the strict German privacy laws. I am not saving IP addresses. More so, I've been the biggest privacy advocate in the Thunderbird project.

That said, if you prefer this to be a data: URL, I don't mind. I can change that.

Next time you have an issue with my code, I formally ask you to notify me and include me. The patch Magnus had suggested would have destroyed the entire feature, and I'm not OK with that.
Status: RESOLVED → VERIFIED
Attachment #9031719 - Flags: review-
I've created a PR for the icon URL to change to a data: URL. It should go live once sancus approves it.
(In reply to Ben Bucksch (:BenB) from comment #16)
> It would be wrong to land localhost, because it would break the entire
> feature. If you want to test it, just change your local preference to
> another URL. I will do that from now on as well.
Maybe it would make sense to add code so that if someone sets the pref to empty the feature is disabled, instead of breaking it?


> I'm a little insulted at the accusation that Beonex is a privacy threat. I'm
> a German entity, the server is hosted in Germany, and under the strict
> German privacy laws. I am not saving IP addresses. More so, I've been the
> biggest privacy advocate in the Thunderbird project.
Ben, sorry for not being more verbose on this. This is nothing personal and I am certain that we can trust you because we know your stance about privacy and security. It is more about the fact that this is a server not controlled and maintained by Thunderbird. It is also about user expectations: The way the Thunderbird Privacy Policy is written it *could* be ok, but we'd prefer not to take that risk.

> That said, if you prefer this to be a data: URL, I don't mind. I can change
> that.
I think we should still do that. Yours may be fine, but if there are others it is unclear. We also need to add code in the product to ensure this is either a chrome://, resource:// moz-extension:// or data: uri (or a subst of those).

> Next time you have an issue with my code, I formally ask you to notify me
> and include me. The patch Magnus had suggested would have destroyed the
> entire feature, and I'm not OK with that.
Of course we will ask you for your input at times, but I'd suggest that instead you follow relevant components in Bugzilla. It is always possible that Thunderbird code may break in a nightly build or even a release. If there is sufficient test coverage, we'll notice this soon enough.

Thank you for your work on this Ben!
Do we have a meta-bug for this feature? Magnus, can you check my followup suggestions and file accordingly?
Flags: needinfo?(mkmelin+mozilla)
> if someone sets the pref to empty the feature is disabled, instead of breaking it?

That's inherently not possible. We have always been broken when we encounter an Exchange server. This feature repairs something that has always been broken. If you disable it, you leave users stranded.

With the feature enabled, users still have the same options that they had before.

> This is nothing personal and I am certain that we can trust you because we know your stance about
> privacy and security. It is more about the fact that this is a server not controlled and maintained
> by Thunderbird.

I understand how you meant it and that you did not mean to accuse me. I was referring to Magnus.

Note that the privacy protection on my servers is far higher than that of Mozilla or even the Thunderbird project. I am glad to see that Thunderbird is better than Mozilla Firefox, but it's still worse than Beonex. I just mention this to re-assure you that there wasn't any privacy problem at any time. But see next sentence.

> > That said, if you prefer this to be a data: URL, I don't mind. I can change that.
> I think we should still do that.

Yes, I understand, and I already did that. sancus already approved it. I guess it's going to be live in a few hours.

> Do we have a meta-bug for this feature? Magnus, can you check my followup suggestions and file accordingly?

I've filed bug 1514627 as meta bug and bug 1514628 for the data URL. That's the only outstanding issue.
> I just mention this to re-assure you that there wasn't any privacy problem at any time.

Just to make this even more clear: I checked my logs, and I do not have any private information about any Thunderbird end user due to this feature, not even IP addresses.
(In reply to Ben Bucksch (:BenB) from comment #20)
> > if someone sets the pref to empty the feature is disabled, instead of breaking it?
> 
> That's inherently not possible. We have always been broken when we encounter
> an Exchange server. This feature repairs something that has always been
> broken. If you disable it, you leave users stranded.

That doesn't mean it isn't possible. I guess it depends on the definition of broken, I mean the kind where the UI is borked, not that the feature is not working. I'm suggesting that when the pref is empty we just don't do any exchange detection. The default pref value would still be at its current value, this is really just for when the user or system admin deliberately sets the pref to empty.


> I understand how you meant it and that you did not mean to accuse me. I was
> referring to Magnus.
I think it is safe to say that Magnus was not accusing you either, this bug is the result of a separate discussion and Magnus happened to file it.


> > Do we have a meta-bug for this feature? Magnus, can you check my followup suggestions and file accordingly?
> 
> I've filed bug 1514627 as meta bug and bug 1514628 for the data URL. That's
> the only outstanding issue.
Thanks for filing the issues!
Flags: needinfo?(mkmelin+mozilla)
Let's move the pref discussion to bug 1514709. If there are any further questions about why this bug was filed in the first place let's move that outside of bugzilla.
> I guess it depends on the definition of broken, I mean the kind where the UI is borked

The UI won't be borked in any case, as far as I know.

> I'm suggesting that when the pref is empty we just don't do any exchange detection.
> for when the user or system admin deliberately sets the pref to empty.

The pref should never be empty. If you want to deactivate Exchange detection, you have another boolean preference "mailnews.auto_config.fetchFromExchange.enabled" for that. I added that only for completeness, in case Tor or a single user wants to disable it, a user or system admin, as you said. In other words, I had been anticipating your request and it's already implemented.
Part of the Owl set, needs uplift.
Attachment #9034482 - Flags: review+
Attachment #9034482 - Flags: approval-comm-esr60?
Attachment #9031719 - Attachment is obsolete: true
Attachment #9034482 - Flags: approval-comm-esr60? → approval-comm-esr60+
Type: enhancement → task
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: