348389 - properly support licenseURL attribute in updates.xml on the client side

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Description

[(not reading, please use [e-mail] instead)]

properly support licenseURL attribute in updates.xml on the client side

from http://wiki.mozilla.org/Software_Update:updates.xml_Format

licenseURL

A URL string to a web page with additional license terms that need to be accepted before the update can be installed (optional, do NOT provide it if this is going to be empty)

there is code and UI to handle licenseURL, but as far as I can tell, to get the license page as part of the update flow, changes will need to be made to update.xul/update.js/update.xml

additionally, chris beard adds:

"we're looking at a requirement to have user's agreement to a revised EULA and privacy policy as part of the update process.

i'd really like to have it as an option for both major and minor and have its display be selectable for each update.  however, having it so that only major updates display is "good enough" and we can define that changes to the license agreement and privacy policy in and of itself constitutes a major update (although, not really)

i.e. perhaps there is a bit we set if the EULA or privacy policy changed between the version the user is on and the version being offered, and if so, we display and require acceptance before proceeding

we will need to ensure we point to the appropriate version of the EULA
which is distinct by product, version and locale

we probably want to set the URL following the nascent standard we're pushing forward on

we may need to make some UI changes to have it be compliant with legal requirements, i.e. equivalent to the installer and how we use radio buttons and specific language to make the agreement binding

Comment 1

[(not reading, please use [e-mail] instead)]

this may block ff2, not sure.

Comment 2

[(not reading, please use [e-mail] instead)]

Attachment: screen shot of license downloading

Comment 3

[(not reading, please use [e-mail] instead)]

Attachment: license not found

Comment 4

[(not reading, please use [e-mail] instead)]

Attachment: license ok

Comment 5

[(not reading, please use [e-mail] instead)]

these screen shot indicate what is in the tree right now, but it isn't completely functional.

the big question for ff 2b2 is do we need any new strings?

Comment 6

[(not reading, please use [e-mail] instead)]

pro-actively adding late-l10n

Comment 7

[Chris Beard]

We should look into use the existing strings from the installer.  If we do need to make a string change, it will need to be for both use cases, i.e. first time installation and upgrade to a version with revisions to the license.  We will have a definitive answer to this on by Monday.

This functionality will need to be in 1.5.0.x as a priority, and Fx 2 in preparation for 3 and/or any significant change in the license agreement in a subsequent minor/major update (or major update only if we end up taking that path for sake of simplicity).

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

--> blocking Fx2b2

We need to at least make a decision before shipping what is to be the string frozen beta. Note that we do have some strings available to us (but are they translated?) from the macbuild directory:

http://lxr.mozilla.org/mozilla1.8.0/source/browser/app/macbuild/license.r#32

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #8)
> Note that we do have some strings available to us (but are they
> translated?) from the macbuild directory:
> 
> http://lxr.mozilla.org/mozilla1.8.0/source/browser/app/macbuild/license.r#32

No, those are not localized. Even if they were, they're not in a format that's usable from XUL.

Comment 10

[Robert Strong (they/them - no direct email)]

(In reply to comment #7)
> We should look into use the existing strings from the installer.
I agree for 3.0. There are lots of strings that are repeated and for this to be shared it should be a common location that is shared by the build process vs. the win32 installer file being used for it.

Comment 11

[Daniel Veditz [:dveditz]]

This doesn't seem realistic to get into 1.8.0.x given localization changes... leaving nominated for now, but we'll need to start seeing code *soon* (patch approval freeze August 22, 2006).

Comment 12

[(not reading, please use [e-mail] instead)]

note, this bug (and bug #329729) would involve using (for the first time, I believe) the <license> binding in http://lxr.mozilla.org/mozilla1.8/source/toolkit/mozapps/update/content/updates.xml#180

notice, it uses a xul:browser.  

for security reasons, should I be disabling js, auth, plugins, meta redirects, subframes on the docshell?

cc dveditz for his opinion.

thanks to robert strong for pointing this out.

Comment 13

[(not reading, please use [e-mail] instead)]

There are 9 strings:

3 strings from http://lxr.mozilla.org/mozilla1.8/source/toolkit/locales/en-US/chrome/mozapps/update/updates.dtd

<!ENTITY  license.title                   "License Agreement">
<!ENTITY  license.intro                   "To install this update you must agree to this license agreement.
                                          Please read it carefully:">
<!ENTITY  license.instructions            "If you agree to the terms of this agreement, click I Agree
                                          below to continue installing this update.">

6 strings from http://lxr.mozilla.org/mozilla1.8/source/toolkit/locales/en-US/chrome/mozapps/update/updates.properties

IAgree=I Agree
IAgree.accesskey=A
IDoNotAgree=I Disagree
IDoNotAgree.accesskey=D
licenseContentNotFound=The license file for this version could not be found. Please visit the %S homepage for more information.
licenseContentDownloading=Getting license for %S %S...

There are two type of initial screen shots that we care about:  minor available or major available.

1) A minor update:  https://bugzilla.mozilla.org/attachment.cgi?id=233009
2) A major update:  https://bugzilla.mozilla.org/attachment.cgi?id=233040

Those both (could) lead to the license screen page, if there licenseURL attribute was set on the updates.xml file (See http://wiki.mozilla.org/Software_Update:updates.xml_Format)

The license panel of the update wizard has has three states:  loading, error, or success:

3) loading:  https://bugzilla.mozilla.org/attachment.cgi?id=233315
4) error:    https://bugzilla.mozilla.org/attachment.cgi?id=233316
5) success:  https://bugzilla.mozilla.org/attachment.cgi?id=233317

(My apologies for "success" state screen shot, I used http://www.sspitzer.org/index.html as my EULA)

Let me know if you need any additional information. 

Comment 14

[Chris Beard]

Is there no way that we can use the existing strings from the Win32 installer? 

That is, the product/legal requirement for the presentation and acceptance of
the license agreement are the same as the installer, i.e. you need to explicitly
check or select the "I Accept" and not have the default action allow you to
click through.

It doesn't make a whole lot of sense (to me) to have different strings here. 

Comment 15

[Robert Strong (they/them - no direct email)]

Benjamin, cbeard would like to use the strings from the win32 installer for this bug.

Comment 16

[(not reading, please use [e-mail] instead)]

Attachment: screen shot with wording from license.r

Comment 17

[(not reading, please use [e-mail] instead)]

Attachment: patch for strings

Comment 18

[(not reading, please use [e-mail] instead)]

cbeard / sherman:  please see the latest screen shot

https://bugzilla.mozilla.org/attachment.cgi?id=233793 

I've just used the wording from http://lxr.mozilla.org/mozilla1.8.0/source/browser/app/macbuild/license.r#32 and copies those strings into updates.dtd and updates.properties

Note, I've also added (but not used yet) "Save As" and "Print...", which were also in license.r.

Perhaps there is some sort of legal requirement to have that functionality when displaying a eula?

note, in the screen shot above, I am doing licenseURL="http://www.mozilla.org/foundation/EULA/firefox-en.txt" in my updates.xml sample.

Comment 19

[Chris Beard]

To be clear, we should be using a construct like this, as we do in the installer for Win32. I'm sure beltzner can provide a better mock up, if needed.

---
<b>Software License Agreement</b>
    Terms and conditions for using this software

Please read the following license agreement. Use the scroll bar to view the rest of this agreement.

{Firefox EULA displayed in text box with scroll bars}

( ) I Accept the terms of the License Agreement
(X) I do NOT Accept the terms of the License Agreement

[Cancel] [Next >]
---

Comment 20

[Sherman Dickman]

If we use the same construct as advocated by cbeard above, could we not use the existing strings (and localized versions) from the existing win32 installer?

Comment 21

[Benjamin Smedberg]

Why is "not accept" the default action? I always found that annoying about our installer, compared to most others, and I'd hate it to seriously interrupt workflow in the update case :-(

Comment 22

[Chris Beard]

(In reply to comment #21)
> Why is "not accept" the default action? I always found that annoying about our
> installer, compared to most others, and I'd hate it to seriously interrupt
> workflow in the update case :-(
> 

In legal terms, there must be an unambiguous action of assent, and it must take place before the software is given to the end user. If the default action allows for "accidental" acceptance of the license agreement, we may be in a situation where it could be argued that there is no binding agreement. If there is no agreement, the user will have default rights under law that are similar to those granted in the EULA.  But the limitations of liability, warranty disclaimers, and similar provisions would not apply.

Comment 23

[(not reading, please use [e-mail] instead)]

Attachment: screen shot, per beard's ascii art

Comment 24

[(not reading, please use [e-mail] instead)]

Attachment: string changes only

Comment 25

[(not reading, please use [e-mail] instead)]

the strings I have added come from:

http://lxr.mozilla.org/mozilla1.8/source/browser/locales/en-US/installer/installer.inc#15

notes:

1)  do we want the back button, which takes you back to the major / minor upgrade available page?

2)  nit:  beard had "Terms and conditions for using this software" (no period), I'm using "Terms and conditions for using this software." because installer.inc has "#define LICENSE_SUBTITLE Terms and conditions for using this software."

Comment 26

[(not reading, please use [e-mail] instead)]

robert points out that 
http://lxr.mozilla.org/mozilla1.8/source/browser/locales/en-US/installer/installer.inc#15  (that I am using as a reference) are from the xpinstall installer.

for the NSIS installer, we have these strings:

http://lxr.mozilla.org/mozilla1.8/source/browser/locales/en-US/installer/override.properties

 62 AcceptBtn=I &accept the terms in the License Agreement
 63 DontAcceptBtn=I &do not accept the terms in the License Agreement   
 74 LicenseTextRB=Please review the license agreement before installing $(^NameDA). If you accept all terms of the agreement, select the first option below. $_CLICK

http://lxr.mozilla.org/mozilla1.8/source/browser/locales/en-US/installer/mui.properties

 57 MUI_TEXT_LICENSE_TITLE=License Agreement
 58 MUI_TEXT_LICENSE_SUBTITLE=Please review the license terms before installing $(^NameDA).
 59 MUI_INNERTEXT_LICENSE_TOP=Press Page Down to see the rest of the agreement.
 60 MUI_INNERTEXT_LICENSE_BOTTOM_RADIOBUTTONS=If you accept the terms of the agreement, select the first option below. You must accept the agreement to install $(^NameDA). $_CLICK

Comment 27

[Daniel Veditz [:dveditz]]

Strings won't be translated in time for 1.5.0.7, will get this in 1.5.0.8

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 233815 [details]
screen shot, per beard's ascii art

One nit: I think that in cbeard's mockup the "Terms and conditions for using this software" was actually the wizard panel title, and the "Software License Agreement" was the window title. That string looks awkward here, and I think we should just nix it (will get to that in next review).

Other than that, this looks OK to me. To be clear, it will be shown after the user has decided to download the update, but before the download begins, yes? (or the download could begin silently in the background, I guess, if we wanted to make better use of bandwidth)

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 233817 [details] [diff] [review]
string changes only

+<!ENTITY  license.intro                   "Terms and conditions for using this 

Might as well leave this string in here, just in case we need it for a subhead or whatever.

Comment 30

[(not reading, please use [e-mail] instead)]

Attachment: a screen shot from robert of the NSIS installer on the trunk

Comment 31

[Robert Strong (they/them - no direct email)]

(In reply to comment #30)
> Created an attachment (id=233969) [edit]
> a screen shot from robert of the NSIS installer on the trunk
This just demonstrates that we can use rtf for the license file and is not actually what is on the trunk.

Comment 32

[(not reading, please use [e-mail] instead)]

> This just demonstrates that we can use rtf for the license file and is not
> actually what is on the trunk.

sorry robert, I should have clarified why I attached that screen shot.

it wasn't for the RTF license, it was for the rest of the UI and strings.

Comment 33

[(not reading, please use [e-mail] instead)]

Attachment: screen shot, can't choose next on failure to load EULA

Comment 34

[(not reading, please use [e-mail] instead)]

Attachment: screen shot, cant' choose next until EULA has finished loading

Comment 35

[(not reading, please use [e-mail] instead)]

Attachment: screen shot, can't choose next until accepted

Comment 36

[(not reading, please use [e-mail] instead)]

Attachment: scree shot, EULA has loaded and user has accepted, only now can the user can click next

Comment 37

[(not reading, please use [e-mail] instead)]

> One nit: I think that in cbeard's mockup the "Terms and conditions for using
> this software" was actually the wizard panel title, and the "Software License
> Agreement" was the window title. That string looks awkward here, and I think we
> should just nix it (will get to that in next review).

Thanks for reviewing the strings and the UI.  I'll go remove the "Terms and conditions for using this software." string from the UI, but keep it in the .dtd (just in case, as you point out.0

> Other than that, this looks OK to me. To be clear, it will be shown after the
> user has decided to download the update, but before the download begins, yes?
> (or the download could begin silently in the background, I guess, if we wanted
> to make better use of bandwidth)

yes, that is the flow for a major update, since major updates will not happen silently.

but you have pointed out there is a scenario which I had not considered and that I need to look into, which is:  

if we have a minor update with a licenseURL value, and the user's update prefs allow for a silent download and install, when will they see the EULA?  What happens if they decline it?

Comment 38

[(not reading, please use [e-mail] instead)]

Attachment: screen shot without that string, per beltzner

Comment 39

[(not reading, please use [e-mail] instead)]

Attachment: change Accept to accept, per beltzner

Comment 40

[(not reading, please use [e-mail] instead)]

A couple open questions:

question 1)  if the user chooses to do a silent install of a minor update, should they have to accept the EULA?

question 2)  should we ask the user to accept the EULA every time, for each minor point release, even if they have agreed to the EULA already?

Note, the eula could change between point releases.  

with my current patch, if licenseURL is set, the user will need to accept it.  if we start setting licenseURL attributes on the update, this will default the purpose of silent, automatic installs.

Idea:  We could store the last licenseURL accepted as a pref, and if the licenseURL matches the stored URL, I would act as if there was no licenseURL, so the user would not be prompted (for a minor release, if auto install was enabled) and they would not see the EULA for a major release, if the EULA had not changed.

this lastLicenseURL pref could be just for minor updates, and major updates would always have to accept the EULA.

beltzner/shrep/cbeard/sherman, comments?

Comment 41

[Chris Beard]

(In reply to comment #40)
> question 1)  if the user chooses to do a silent install of a minor update,
> should they have to accept the EULA?

Yes. But we should only display and require that the EULA be agreed to if it changes. (We could decide that if the EULA changes that consitutes a *major* updat, and then only support this functionality for those updates. Having flexibility to do both would be a nice to have.)

> question 2)  should we ask the user to accept the EULA every time, for each
> minor point release, even if they have agreed to the EULA already?

No.  We should only present and require that a user accept the EULA if it has been updated.

Comment 42

[(not reading, please use [e-mail] instead)]

based on beards comments, I'll go implement my idea of using a pref to store the last accepted licenseURL.

we'll need agreement from the server side folks that for each new version of the EULA, we'll have a new url to use.

we may decided to not use the licenseURL as the key, but changing to use something else shouldn't be hard.

working on this now...

Comment 43

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #42)
> based on beards comments, I'll go implement my idea of using a pref to store
> the last accepted licenseURL.

I hope we'd do this as sparingly as possible, since minor updates aren't really supposed to hold new functionality and shouldn't really have new EULAs attached. 

When we *do* it, though, we should basically treat it like we'd treat an update that broke extension compatibility. That is to say, not perform a silent download and verification, but instead pop the "An Update is Available" screen and have the Next > button lead to the EULA page.

A bigger question in my mind is what happens if the user cancels. Under the current architecture, we'd end up nagging them the next day. And the next day. And then users would go turn off auto-update checks.

> we'll need agreement from the server side folks that for each new version of
> the EULA, we'll have a new url to use.

I'd imagine they'll be constructed something like:

http://%LOCALE%.www.mozilla.com/%LOCALE%/%PRODUCT%/%VERSION%/eula

as per bug 347944.

Comment 44

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #41)
> (In reply to comment #40)
> > question 1)  if the user chooses to do a silent install of a minor update,
> > should they have to accept the EULA?
> 
> Yes. But we should only display and require that the EULA be agreed to if it
> changes. (We could decide that if the EULA changes that consitutes a *major*
> updat, and then only support this functionality for those updates. Having
> flexibility to do both would be a nice to have.)

Chris and I talked on the phone, and we both agreed that any EULA change should be considered a major update, and take the major update path in the software update UI (ie: allow the user to say "Never", only offer the next update automatically, offer the same update again if the user manually checks).

Seth: I don't think it's worth your time working on the code to enable that sort of behaviour for minor updates on the off chance that one might require a EULA change. I'd rather just say "EULA changes are always major updates".

Comment 45

[(not reading, please use [e-mail] instead)]

> Chris and I talked on the phone, and we both agreed that any EULA change should
>be considered a major update, and take the major update path in the software
>update UI (ie: allow the user to say "Never", only offer the next update
>automatically, offer the same update again if the user manually checks).

ok, that makes things simpler in my patch for bug #329729

the plan is:

1)  never show the eula for minor updates (even if licenseURL is set)
2)  always show the eula for major updates (if licenseURL is set), just as we alwyas prompt for a major update.
3)  do not keep track of lastLicense (because of rule #2)

if someone wanted to force a EULA change on a minor update, they would have to change it to be a major update.

I'll go fix my patch in bug #329729

Comment 46

[(not reading, please use [e-mail] instead)]

> I'll go fix my patch in bug #329729

patch has been fixed.

Comment 47

[(not reading, please use [e-mail] instead)]

I've landed the new strings to the branch, so they can be localized.

Comment 48

[(not reading, please use [e-mail] instead)]

> I've landed the new strings to the branch, so they can be localized.

to the trunk too.

Comment 49

[Mike Beltzner [:beltzner, not reading bugmail]]

Seth: if the code is in 329729 and the strings are landed, should this be marked fixed?

Comment 50

[(not reading, please use [e-mail] instead)]

1)

>+                    if (httpResponse >= 200 && httpResponse < 300) {
>
>requestSucceeded

fixed, thanks

2)

>+                    // the QI to nsIHttpChannel can fail if you try to hit
>+                    // www.mozillaxxxx.org or something that does not exist
>+                    // so we treat that as an error
>
>so, that shouldn't be the case. it is kind of strange that it is. what is
>aRequest.name in this case?

my mistake, the QI wasn't failing, it was the call to get the httpResponse
(and now requestSucceed)

aRequest.name is:  http://www.sspitzerx.org/

and the exception I get is:

[Exception... "Component returned failure code: 0x80040111
 (NS_ERROR_NOT_AVAILABLE) [nsIHttpChannel.requestSucceeded]"  nsresult: "0x80040
111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: chrome://mozapps/content/u
pdate/updates.xml :: anonymous :: line 302"  data: no]

I've fixed my comment.

Comment 51

[Christian :Biesinger (don't email me, ping me on IRC)]

(that comment was for bug 329729 afaict)

Comment 52

[(not reading, please use [e-mail] instead)]

> (that comment was for bug 329729 afaict)

oops, yes.  I've posted my resposne in the other bug, too.

Comment 53

[(not reading, please use [e-mail] instead)]

fixed on trunk (as part of bug #329729), awaiting for approval on the branch.

Comment 54

[(not reading, please use [e-mail] instead)]

fix landed on the branch along with the fix for bug #329729.

Comment 55

[Daniel Veditz [:dveditz]]

Restoring lost blocking flag

Comment 56

[Daniel Veditz [:dveditz]]

I believe this is wrapped up in the "offer major updates" bug and is needed in 1.8.0.8 despite the localization cost.

Comment 57

[(not reading, please use [e-mail] instead)]

backported to MOZILLA_1_8_0_BRANCH

Comment 58

[Robert Strong (they/them - no direct email)]

Removed in bug 1267891