Closed
Bug 1514522
Opened 6 years ago
Closed 6 years ago
remove link to 3rd party site for account type add-ons
Categories
(Thunderbird :: Account Manager, task)
Thunderbird
Account Manager
Tracking
(thunderbird_esr6065+ fixed, thunderbird65 fixed, thunderbird66 fixed)
VERIFIED
FIXED
Thunderbird 66.0
People
(Reporter: mkmelin, Assigned: mkmelin)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
jorgk-bmo
:
review+
jorgk-bmo
:
approval-comm-esr60+
|
Details | Diff | Splinter Review |
Remove the beonex link even for nightlies.
The release link will change too once we figure that out.
Assignee | ||
Comment 1•6 years ago
|
||
Attachment #9031719 -
Flags: review?(jorgk)
Assignee | ||
Comment 2•6 years ago
|
||
For reference, could have privacy and legal issues.
Assignee | ||
Updated•6 years ago
|
Version: 60 → Trunk
Comment 3•6 years ago
|
||
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 4•6 years ago
|
||
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+
Comment 5•6 years ago
|
||
(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?
Comment 6•6 years ago
|
||
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: 6 years ago
Resolution: --- → FIXED
Comment 8•6 years ago
|
||
Landed with:
pref("mailnews.auto_config.addons_url", "https://live.thunderbird.net/autoconfig/addons.json");
Target Milestone: --- → Thunderbird 66.0
Assignee | ||
Comment 9•6 years ago
|
||
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)
Comment 10•6 years ago
|
||
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 11•6 years ago
|
||
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-
Assignee | ||
Comment 12•6 years ago
|
||
(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,
Comment 13•6 years ago
|
||
> 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.
Comment 14•6 years ago
|
||
(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.
Comment 15•6 years ago
|
||
(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?
Comment 16•6 years ago
|
||
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.
Updated•6 years ago
|
Status: RESOLVED → VERIFIED
Updated•6 years ago
|
Attachment #9031719 -
Flags: review-
Comment 17•6 years ago
|
||
I've created a PR for the icon URL to change to a data: URL. It should go live once sancus approves it.
Comment 18•6 years ago
|
||
(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!
Comment 19•6 years ago
|
||
Do we have a meta-bug for this feature? Magnus, can you check my followup suggestions and file accordingly?
Flags: needinfo?(mkmelin+mozilla)
Updated•6 years ago
|
Blocks: exchange-meta
Comment 20•6 years ago
|
||
> 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.
Comment 21•6 years ago
|
||
> 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.
Comment 22•6 years ago
|
||
(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)
Comment 23•6 years ago
|
||
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.
Comment 24•6 years ago
|
||
> 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.
Comment 25•6 years ago
|
||
Beta (TB 65):
https://hg.mozilla.org/releases/comm-beta/rev/44241fb0b16211fe4e3cbe66c7689e9c742e9730
status-thunderbird65:
--- → fixed
status-thunderbird66:
--- → fixed
Comment 26•6 years ago
|
||
Beta backout:
https://hg.mozilla.org/releases/comm-beta/rev/195a2aef72e1ab507c07ca93e703600b4b09c3b3
status-thunderbird65:
fixed → ---
Comment 27•6 years ago
|
||
Part of the Owl set, needs uplift.
Attachment #9034482 -
Flags: review+
Attachment #9034482 -
Flags: approval-comm-esr60?
Updated•6 years ago
|
Attachment #9031719 -
Attachment is obsolete: true
Comment 28•6 years ago
|
||
TB 65 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/fb8839b8e49ed532baa674b6472042922ceb653b
status-thunderbird65:
--- → fixed
Updated•6 years ago
|
Attachment #9034482 -
Flags: approval-comm-esr60? → approval-comm-esr60+
Comment 29•6 years ago
|
||
TB 60.5.0 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/803b4914794b249e566ff169f0922d11630b53db
status-thunderbird_esr60:
--- → fixed
tracking-thunderbird_esr60:
--- → 65+
Updated•5 years ago
|
Type: enhancement → task
You need to log in
before you can comment on or make changes to this bug.
Description
•