852599 - Block downloads when they are disallowed globally by the parental control service

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Paolo Amadini]

The new API should not allow any download to be executed if downloads are
disallowed globally by nsIParentalControlsService.

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 2

[:Paolo Amadini]

Comment on attachment 762342 [details] [diff] [review]
v1

A blocked download should result in an error (thus, a promise rejection should
be notified to the caller). To define the cause of the error, we should define
two new boolean properties on the DownloadsError object:

* becauseBlocked: catch-all variable, set to true for all the cases where the
  download is blocked (the result code can just be NS_ERROR_FAILURE).
* becauseBlockedByParentalControls: set to true in this case.

In the future we may have cases like becauseBlockedByApplicationReputation
(we may also revise this and decide to go for a blockReason variable).

For now, both boolean variables should be set to true when blocked.

To support mocking, we should implement the check as a function that returns
a promise in the DownloadIntegration module. This should make it possible to
implement an automated test for success and one for failure.

Do you have a Windows system available for manual testing?

Comment 3

[Raymond Lee [:raymondlee]]

Attachment: v2

Comment 4

[Raymond Lee [:raymondlee]]

I have a Windows 7 machine, however, I have enabled for a user account with parental controls on but nsIParentalControlsService.parentalControlsEnabled still returns false.  Furthermore, there is no option to block downloads.

I have checked the code and googled about that, and it is only supported by Windows Vista
https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIParentalControlsService

Comment 5

[:Paolo Amadini]

(In reply to Raymond Lee [:raymondlee] from comment #4)
> I have a Windows 7 machine, however, I have enabled for a user account with
> parental controls on but nsIParentalControlsService.parentalControlsEnabled
> still returns false.  Furthermore, there is no option to block downloads.

It seems that the download blocking functionality in Windows 7 was moved to the
Windows Live Family Safety product, and then reintegrated in Windows 8.

http://msdn.microsoft.com/en-us/library/windows/desktop/dd630560%28v=vs.85%29.aspx
http://msdn.microsoft.com/en-us/library/windows/desktop/jj155495%28v=vs.85%29.aspx

I'm not sure whether the back-end API for the browser remained the same. Are you
able to test Windows Live Family Safety on your system?

http://windows.microsoft.com/en-us/windows-vista/protecting-your-kids-with-family-safety

Comment 6

[Raymond Lee [:raymondlee]]

(In reply to Paolo Amadini [:paolo] from comment #5)
> (In reply to Raymond Lee [:raymondlee] from comment #4)
> > I have a Windows 7 machine, however, I have enabled for a user account with
> > parental controls on but nsIParentalControlsService.parentalControlsEnabled
> > still returns false.  Furthermore, there is no option to block downloads.
> 
> It seems that the download blocking functionality in Windows 7 was moved to
> the
> Windows Live Family Safety product, and then reintegrated in Windows 8.
> 
> http://msdn.microsoft.com/en-us/library/windows/desktop/dd630560%28v=vs.
> 85%29.aspx
> http://msdn.microsoft.com/en-us/library/windows/desktop/jj155495%28v=vs.
> 85%29.aspx
> 
> I'm not sure whether the back-end API for the browser remained the same. Are
> you
> able to test Windows Live Family Safety on your system?
> 
> http://windows.microsoft.com/en-us/windows-vista/protecting-your-kids-with-
> family-safety

Yes, I have installed the Family Safety on my Win 7. The back-end API for browser remains the same and return the correct values for |parentalControlsService.parentalControlsEnabled| and |parentalControlsService.blockFileDownloadsEnabled|

Comment 7

[:Paolo Amadini]

Comment on attachment 766609 [details] [diff] [review]
v2

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

(In reply to Raymond Lee [:raymondlee] from comment #6)
> Yes, I have installed the Family Safety on my Win 7. The back-end API for
> browser remains the same and return the correct values for
> |parentalControlsService.parentalControlsEnabled| and
> |parentalControlsService.blockFileDownloadsEnabled|

Great! So, this feature has two independent tests to be done, one manual and one automatic.

The automatic test we're implementing checks that the download properties are updated correctly in response to a block. Executing the test should result in no effect in the system. We do this by enabling test mode for the DownloadIntegration module.

The manual test consists in executing two downloads, blocked and not blocked, and seeing the success and failure audits in the system, in addition to the actual result. For the moment, we can do this manually from a Scratchpad script, or by enabling the browser.download.useJSTransfer preference.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +230,5 @@
> +    if (DownloadIntegration.shouldBlockForParentalControls()) {
> +      return Promise.reject(new DownloadError(Cr.NS_ERROR_FAILURE,
> +                                              "Download blocked.", false,
> +                                              true));
> +    }

For correct state management, this check should be done just before the call to "this.saver.execute", throwing a DownloadError if blocked.

@@ +488,2 @@
>   */
> +function DownloadError(aResult, aMessage, aInferCause, aIsBlocked)

Let's not add a new constructor parameter for each cause, we can just create a new DownloadError(Cr.NS_ERROR_FAILURE, "Download blocked.") and then set the appropriate "because" properties before throwing.

@@ +530,5 @@
>     */
>    becauseTargetFailed: false,
> +
> +  /**
> +   * Indicates an error occurred when the download is blocked.

"Indicates the download failed because it was blocked.  If the reason for blocking is known, the corresponding property will be also set."

@@ +535,5 @@
> +   */
> +  becauseBlocked: false,
> +
> +  /**
> +   * Indicates an error occurred the download is blocked by parental controls.

"Indicates the download was blocked because downloads are globally disallowed by the Parental Controls or Family Safety features on Windows."

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +42,5 @@
> +    parentalControlsService =
> +      Cc["@mozilla.org/parental-controls-service;1"]
> +        .createInstance(Ci.nsIParentalControlsService);
> +  }
> +  return parentalControlsService;

Does getService work here? This code can also be simplified:

if (... in Cc) {
  return Cc[...]
           .getService(...);
}
return null;

@@ +194,5 @@
> +   * Checks to determine whether to block downloads for parental controls.
> +   *
> +   * @return The boolean indicates to block downloads or not.
> +   */
> +  shouldBlockForParentalControls: function DI_shouldBlockForParentalControls() {

We should use this function for auditing also:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#1626

Let's add an aDownload parameter that we can use to get the source URI. Also, let's return a promise here, in case the underlying API becomes asynchronous.

@@ +196,5 @@
> +   * @return The boolean indicates to block downloads or not.
> +   */
> +  shouldBlockForParentalControls: function DI_shouldBlockForParentalControls() {
> +    let shouldBlock = false;
> +#ifdef XP_WIN

No need to preprocess this, we already check if gParentalControlsService is null to see if the platform doesn't support it.

@@ +200,5 @@
> +#ifdef XP_WIN
> +    shouldBlock = (gParentalControlsService &&
> +                   gParentalControlsService.parentalControlsEnabled &&
> +                   gParentalControlsService.blockFileDownloadsEnabled) ||
> +                  this.testMode;

We need to enable test mode globally, during all the tests, to avoid interaction with the operating system.

This is the same approach as the "dontLoad" property in bug 835885 (by the way, I've just finished working on that bug, so this patch will need to be rebased on top of it).

Let's check this property at the very beginning of the function:

if (this.dontCheckParentalControls) {
  return Promise.resolve(this.shouldBlockInTest);
}

::: toolkit/components/jsdownloads/test/unit/test_DownloadCore.js
@@ +875,5 @@
>  
> +/**
> + * Download with parental controls enabled.
> + */
> +add_task(function test_download_with_parental_controls_enabled()

test_download_blocked_parental_controls

We also need a similar test for the DownloadLegacy case.

@@ +881,5 @@
> +  function cleanup() {
> +    DownloadIntegration.testMode = false;
> +  }
> +  do_register_cleanup(cleanup);
> +  DownloadIntegration.testMode = true;

We'll just need to set shouldBlockInTest to true here.

@@ +885,5 @@
> +  DownloadIntegration.testMode = true;
> +
> +  let download = yield promiseSimpleDownload();
> +
> +  if ("@mozilla.org/windows-registry-key;1" in Cc) {

Since we don't interact with the operating system, we have no need to do a platform-specific test here.

@@ +892,5 @@
> +      do_throw("The download should have blocked.");
> +    } catch (ex) {
> +      do_check_true(ex instanceof Downloads.Error);
> +      do_check_true(ex.becauseBlocked);
> +      do_check_true(ex.becauseBlockedByParentalControls);

} catch (ex if ex instanceof Downloads.Error && ex.becauseBlocked) {
  do_check_true(ex.becauseBlockedByParentalControls);
}

::: toolkit/components/jsdownloads/test/unit/test_DownloadIntegration.js
@@ +146,5 @@
> +/**
> + * Tests that the shouldBlockForParentalControls returns a boolean indicates
> + * the download should be blocked or not.
> + */
> +add_task(function test_shouldBlockForParentalControls()

With the new testing approach, we don't need this test anymore.

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: v3

(In reply to Paolo Amadini [:paolo] from comment #7)
> Comment on attachment 766609 [details] [diff] [review]
> v2
> 
> Review of attachment 766609 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Raymond Lee [:raymondlee] from comment #6)
> > Yes, I have installed the Family Safety on my Win 7. The back-end API for
> > browser remains the same and return the correct values for
> > |parentalControlsService.parentalControlsEnabled| and
> > |parentalControlsService.blockFileDownloadsEnabled|
> 
> Great! So, this feature has two independent tests to be done, one manual and
> one automatic.
> 
> The automatic test we're implementing checks that the download properties
> are updated correctly in response to a block. Executing the test should
> result in no effect in the system. We do this by enabling test mode for the
> DownloadIntegration module.
> 
> The manual test consists in executing two downloads, blocked and not
> blocked, and seeing the success and failure audits in the system, in
> addition to the actual result. For the moment, we can do this manually from
> a Scratchpad script, or by enabling the browser.download.useJSTransfer
> preference.
> 

I've enabled the browser.download.useJSTransfer preference and tried to download a file which shouldn't block.  However, I see that it throws an Download blocked error but the file is still downloaded.

Am I missing something? 

> ::: toolkit/components/jsdownloads/src/DownloadCore.jsm
> @@ +230,5 @@
> > +    if (DownloadIntegration.shouldBlockForParentalControls()) {
> > +      return Promise.reject(new DownloadError(Cr.NS_ERROR_FAILURE,
> > +                                              "Download blocked.", false,
> > +                                              true));
> > +    }
> 
> For correct state management, this check should be done just before the call
> to "this.saver.execute", throwing a DownloadError if blocked.

Moved

> 
> @@ +488,2 @@
> >   */
> > +function DownloadError(aResult, aMessage, aInferCause, aIsBlocked)
> 
> Let's not add a new constructor parameter for each cause, we can just create
> a new DownloadError(Cr.NS_ERROR_FAILURE, "Download blocked.") and then set
> the appropriate "because" properties before throwing.
> 

Done

> @@ +530,5 @@
> >     */
> >    becauseTargetFailed: false,
> > +
> > +  /**
> > +   * Indicates an error occurred when the download is blocked.
> 
> "Indicates the download failed because it was blocked.  If the reason for
> blocking is known, the corresponding property will be also set."
> 
> @@ +535,5 @@
> > +   */
> > +  becauseBlocked: false,
> > +
> > +  /**
> > +   * Indicates an error occurred the download is blocked by parental controls.
> 
> "Indicates the download was blocked because downloads are globally
> disallowed by the Parental Controls or Family Safety features on Windows."
> 
> ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
> @@ +42,5 @@
> > +    parentalControlsService =
> > +      Cc["@mozilla.org/parental-controls-service;1"]
> > +        .createInstance(Ci.nsIParentalControlsService);
> > +  }
> > +  return parentalControlsService;
> 
> Does getService work here? This code can also be simplified:
> 
> if (... in Cc) {
>   return Cc[...]
>            .getService(...);
> }
> return null;
> 
> @@ +194,5 @@
> > +   * Checks to determine whether to block downloads for parental controls.
> > +   *
> > +   * @return The boolean indicates to block downloads or not.
> > +   */
> > +  shouldBlockForParentalControls: function DI_shouldBlockForParentalControls() {
> 
> We should use this function for auditing also:
> 
> http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/
> nsDownloadManager.cpp#1626
>
> Let's add an aDownload parameter that we can use to get the source URI.
> Also, let's return a promise here, in case the underlying API becomes
> asynchronous.
>

Done
 
> @@ +196,5 @@
> > +   * @return The boolean indicates to block downloads or not.
> > +   */
> > +  shouldBlockForParentalControls: function DI_shouldBlockForParentalControls() {
> > +    let shouldBlock = false;
> > +#ifdef XP_WIN
> 
> No need to preprocess this, we already check if gParentalControlsService is
> null to see if the platform doesn't support it.
> 

Removed

> @@ +200,5 @@
> > +#ifdef XP_WIN
> > +    shouldBlock = (gParentalControlsService &&
> > +                   gParentalControlsService.parentalControlsEnabled &&
> > +                   gParentalControlsService.blockFileDownloadsEnabled) ||
> > +                  this.testMode;
> 
> We need to enable test mode globally, during all the tests, to avoid
> interaction with the operating system.
> 
> This is the same approach as the "dontLoad" property in bug 835885 (by the
> way, I've just finished working on that bug, so this patch will need to be
> rebased on top of it).
> 
> Let's check this property at the very beginning of the function:
> 
> if (this.dontCheckParentalControls) {
>   return Promise.resolve(this.shouldBlockInTest);
> }
> 

Done

> ::: toolkit/components/jsdownloads/test/unit/test_DownloadCore.js
> @@ +875,5 @@
> >  
> > +/**
> > + * Download with parental controls enabled.
> > + */
> > +add_task(function test_download_with_parental_controls_enabled()
> 
> test_download_blocked_parental_controls
> 
> We also need a similar test for the DownloadLegacy case.
> 

Updated

> @@ +881,5 @@
> > +  function cleanup() {
> > +    DownloadIntegration.testMode = false;
> > +  }
> > +  do_register_cleanup(cleanup);
> > +  DownloadIntegration.testMode = true;
> 
> We'll just need to set shouldBlockInTest to true here.
> 

Updated.

> @@ +885,5 @@
> > +  DownloadIntegration.testMode = true;
> > +
> > +  let download = yield promiseSimpleDownload();
> > +
> > +  if ("@mozilla.org/windows-registry-key;1" in Cc) {
> 
> Since we don't interact with the operating system, we have no need to do a
> platform-specific test here.

Removed.

> 
> @@ +892,5 @@
> > +      do_throw("The download should have blocked.");
> > +    } catch (ex) {
> > +      do_check_true(ex instanceof Downloads.Error);
> > +      do_check_true(ex.becauseBlocked);
> > +      do_check_true(ex.becauseBlockedByParentalControls);
> 
> } catch (ex if ex instanceof Downloads.Error && ex.becauseBlocked) {
>   do_check_true(ex.becauseBlockedByParentalControls);
> }
> 

Done

> ::: toolkit/components/jsdownloads/test/unit/test_DownloadIntegration.js
> @@ +146,5 @@
> > +/**
> > + * Tests that the shouldBlockForParentalControls returns a boolean indicates
> > + * the download should be blocked or not.
> > + */
> > +add_task(function test_shouldBlockForParentalControls()
> 
> With the new testing approach, we don't need this test anymore.

Removed

Comment 9

[Raymond Lee [:raymondlee]]

(In reply to Raymond Lee [:raymondlee] from comment #8)
> Created attachment 768213 [details] [diff] [review]
> v3
> 
> (In reply to Paolo Amadini [:paolo] from comment #7)
> > Comment on attachment 766609 [details] [diff] [review]
> > v2
> > 
> > Review of attachment 766609 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > (In reply to Raymond Lee [:raymondlee] from comment #6)
> > > Yes, I have installed the Family Safety on my Win 7. The back-end API for
> > > browser remains the same and return the correct values for
> > > |parentalControlsService.parentalControlsEnabled| and
> > > |parentalControlsService.blockFileDownloadsEnabled|
> > 
> > Great! So, this feature has two independent tests to be done, one manual and
> > one automatic.
> > 
> > The automatic test we're implementing checks that the download properties
> > are updated correctly in response to a block. Executing the test should
> > result in no effect in the system. We do this by enabling test mode for the
> > DownloadIntegration module.
> > 
> > The manual test consists in executing two downloads, blocked and not
> > blocked, and seeing the success and failure audits in the system, in
> > addition to the actual result. For the moment, we can do this manually from
> > a Scratchpad script, or by enabling the browser.download.useJSTransfer
> > preference.
> > 
> 
> I've enabled the browser.download.useJSTransfer preference and tried to
> download a file which shouldn't block.  However, I see that it throws an
> Download blocked error but the file is still downloaded.
> 
> Am I missing something? 

I mean tried to download a file which *should be* blocked.  It throws an Download blocked error but the file is still downloaded.

> 
> > ::: toolkit/components/jsdownloads/src/DownloadCore.jsm
> > @@ +230,5 @@
> > > +    if (DownloadIntegration.shouldBlockForParentalControls()) {
> > > +      return Promise.reject(new DownloadError(Cr.NS_ERROR_FAILURE,
> > > +                                              "Download blocked.", false,
> > > +                                              true));
> > > +    }
> > 
> > For correct state management, this check should be done just before the call
> > to "this.saver.execute", throwing a DownloadError if blocked.
> 
> Moved
> 
> > 
> > @@ +488,2 @@
> > >   */
> > > +function DownloadError(aResult, aMessage, aInferCause, aIsBlocked)
> > 
> > Let's not add a new constructor parameter for each cause, we can just create
> > a new DownloadError(Cr.NS_ERROR_FAILURE, "Download blocked.") and then set
> > the appropriate "because" properties before throwing.
> > 
> 
> Done
> 
> > @@ +530,5 @@
> > >     */
> > >    becauseTargetFailed: false,
> > > +
> > > +  /**
> > > +   * Indicates an error occurred when the download is blocked.
> > 
> > "Indicates the download failed because it was blocked.  If the reason for
> > blocking is known, the corresponding property will be also set."
> > 
> > @@ +535,5 @@
> > > +   */
> > > +  becauseBlocked: false,
> > > +
> > > +  /**
> > > +   * Indicates an error occurred the download is blocked by parental controls.
> > 
> > "Indicates the download was blocked because downloads are globally
> > disallowed by the Parental Controls or Family Safety features on Windows."
> > 
> > ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
> > @@ +42,5 @@
> > > +    parentalControlsService =
> > > +      Cc["@mozilla.org/parental-controls-service;1"]
> > > +        .createInstance(Ci.nsIParentalControlsService);
> > > +  }
> > > +  return parentalControlsService;
> > 
> > Does getService work here? This code can also be simplified:
> > 
> > if (... in Cc) {
> >   return Cc[...]
> >            .getService(...);
> > }
> > return null;
> > 
> > @@ +194,5 @@
> > > +   * Checks to determine whether to block downloads for parental controls.
> > > +   *
> > > +   * @return The boolean indicates to block downloads or not.
> > > +   */
> > > +  shouldBlockForParentalControls: function DI_shouldBlockForParentalControls() {
> > 
> > We should use this function for auditing also:
> > 
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/
> > nsDownloadManager.cpp#1626
> >
> > Let's add an aDownload parameter that we can use to get the source URI.
> > Also, let's return a promise here, in case the underlying API becomes
> > asynchronous.
> >
> 
> Done
>  
> > @@ +196,5 @@
> > > +   * @return The boolean indicates to block downloads or not.
> > > +   */
> > > +  shouldBlockForParentalControls: function DI_shouldBlockForParentalControls() {
> > > +    let shouldBlock = false;
> > > +#ifdef XP_WIN
> > 
> > No need to preprocess this, we already check if gParentalControlsService is
> > null to see if the platform doesn't support it.
> > 
> 
> Removed
> 
> > @@ +200,5 @@
> > > +#ifdef XP_WIN
> > > +    shouldBlock = (gParentalControlsService &&
> > > +                   gParentalControlsService.parentalControlsEnabled &&
> > > +                   gParentalControlsService.blockFileDownloadsEnabled) ||
> > > +                  this.testMode;
> > 
> > We need to enable test mode globally, during all the tests, to avoid
> > interaction with the operating system.
> > 
> > This is the same approach as the "dontLoad" property in bug 835885 (by the
> > way, I've just finished working on that bug, so this patch will need to be
> > rebased on top of it).
> > 
> > Let's check this property at the very beginning of the function:
> > 
> > if (this.dontCheckParentalControls) {
> >   return Promise.resolve(this.shouldBlockInTest);
> > }
> > 
> 
> Done
> 
> > ::: toolkit/components/jsdownloads/test/unit/test_DownloadCore.js
> > @@ +875,5 @@
> > >  
> > > +/**
> > > + * Download with parental controls enabled.
> > > + */
> > > +add_task(function test_download_with_parental_controls_enabled()
> > 
> > test_download_blocked_parental_controls
> > 
> > We also need a similar test for the DownloadLegacy case.
> > 
> 
> Updated
> 
> > @@ +881,5 @@
> > > +  function cleanup() {
> > > +    DownloadIntegration.testMode = false;
> > > +  }
> > > +  do_register_cleanup(cleanup);
> > > +  DownloadIntegration.testMode = true;
> > 
> > We'll just need to set shouldBlockInTest to true here.
> > 
> 
> Updated.
> 
> > @@ +885,5 @@
> > > +  DownloadIntegration.testMode = true;
> > > +
> > > +  let download = yield promiseSimpleDownload();
> > > +
> > > +  if ("@mozilla.org/windows-registry-key;1" in Cc) {
> > 
> > Since we don't interact with the operating system, we have no need to do a
> > platform-specific test here.
> 
> Removed.
> 
> > 
> > @@ +892,5 @@
> > > +      do_throw("The download should have blocked.");
> > > +    } catch (ex) {
> > > +      do_check_true(ex instanceof Downloads.Error);
> > > +      do_check_true(ex.becauseBlocked);
> > > +      do_check_true(ex.becauseBlockedByParentalControls);
> > 
> > } catch (ex if ex instanceof Downloads.Error && ex.becauseBlocked) {
> >   do_check_true(ex.becauseBlockedByParentalControls);
> > }
> > 
> 
> Done
> 
> > ::: toolkit/components/jsdownloads/test/unit/test_DownloadIntegration.js
> > @@ +146,5 @@
> > > +/**
> > > + * Tests that the shouldBlockForParentalControls returns a boolean indicates
> > > + * the download should be blocked or not.
> > > + */
> > > +add_task(function test_shouldBlockForParentalControls()
> > 
> > With the new testing approach, we don't need this test anymore.
> 
> Removed

Comment 10

[:Paolo Amadini]

Comment on attachment 768213 [details] [diff] [review]
v3

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

(In reply to Raymond Lee [:raymondlee] from comment #9)
> I mean tried to download a file which *should be* blocked.  It throws an
> Download blocked error but the file is still downloaded.

It looks you found a bug in the current API, thanks! I've not tested the following code, but replacing the aDownload.start() call in DownloadLegacy.jsm with it should actually help solving the issue:

      // Start the download before allowing it to be controlled.
      aDownload.start().then(null, function () {
        // In case the operation failed, ensure we stop downloading data.
        aDownload.saver.deferCanceled.resolve();
      });

We should definitely add a check to the DownloadLegacy blocking test, to verify that the file is actually removed.

::: toolkit/components/jsdownloads/src/DownloadCore.jsm
@@ +273,5 @@
>  
> +      // Disallow download if parental controls service restricts it.
> +      if (yield DownloadIntegration.shouldBlockForParentalControls(this)) {
> +        throw new DownloadError(Cr.NS_ERROR_FAILURE, "Download blocked.");
> +      }

let error = new DownloadError(Cr.NS_ERROR_FAILURE, "Download blocked.");
error.becauseBlocked = true;
error.becauseBlockedByParentalControls = true;
throw error;

@@ +522,5 @@
>      this.message = aMessage;
> +    if (aMessage == "Download blocked.") {
> +      this.becauseBlocked = true;
> +      this.becauseBlockedByParentalControls = true;
> +    }

(and remove this)

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +63,5 @@
>  this.DownloadIntegration = {
>    // For testing only
>    testMode: false,
>    dontLoad: false,
> +  dontCheckParentalControls: false,

I'd also add "shouldBlockInTest: false" here explicitly.

@@ +247,5 @@
> +
> +    let shouldBlock = (gParentalControlsService &&
> +                       gParentalControlsService.parentalControlsEnabled &&
> +                       gParentalControlsService.blockFileDownloadsEnabled);
> +

We should add auditing, that is we should call the "log" method if loggingEnabled returns true, and specify whether the download was blocked or not.

The log of downloads should be visible during manual testing in the operating system.

::: toolkit/components/jsdownloads/test/unit/test_DownloadLegacy.js
@@ +370,5 @@
> +    yield download.start();
> +    do_throw("The download should have blocked.");
> +  } catch (ex if ex instanceof Downloads.Error && ex.becauseBlocked) {
> +    do_check_true(ex.becauseBlockedByParentalControls);
> +  }

So, we should add a test here that verifies the file doesn't exist.

Because the cancellation process is asynchronous, unfortunately we may have to wait on promiseExecuteSoon a couple of times to ensure the file is actually removed.

Comment 11

[Raymond Lee [:raymondlee]]

Attachment: v4

(In reply to Paolo Amadini [:paolo] from comment #10)
> Comment on attachment 768213 [details] [diff] [review]
> v3
> 
> Review of attachment 768213 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Raymond Lee [:raymondlee] from comment #9)
> > I mean tried to download a file which *should be* blocked.  It throws an
> > Download blocked error but the file is still downloaded.
> 
> It looks you found a bug in the current API, thanks! I've not tested the
> following code, but replacing the aDownload.start() call in
> DownloadLegacy.jsm with it should actually help solving the issue:
> 
>       // Start the download before allowing it to be controlled.
>       aDownload.start().then(null, function () {
>         // In case the operation failed, ensure we stop downloading data.
>         aDownload.saver.deferCanceled.resolve();
>       });
> 

Updated and solved the issue.  However, after doing that, test_error in test_DownloadLegacy.js starts throwing error. I couldn't figure out why. Any ideas?

<<<<<<<
Traceback (most recent call last):
  File "/Users/raymondlee/Documents/appcoast/mozilla-central2/config/pythonpath.py", line 56, in <module>
    main(sys.argv[1:])
  File "/Users/raymondlee/Documents/appcoast/mozilla-central2/config/pythonpath.py", line 48, in main
    execfile(script, frozenglobals)
  File "/Users/raymondlee/Documents/appcoast/mozilla-central2/testing/xpcshell/runxpcshelltests.py", line 1143, in <module>
    main()
  File "/Users/raymondlee/Documents/appcoast/mozilla-central2/testing/xpcshell/runxpcshelltests.py", line 1139, in main
    if not xpcsh.runTests(args[0], testdirs=args[1:], **options.__dict__):
  File "/Users/raymondlee/Documents/appcoast/mozilla-central2/testing/xpcshell/runxpcshelltests.py", line 794, in runTests
    pStderr=pStderr, keep_going=keepGoing)
  File "/Users/raymondlee/Documents/appcoast/mozilla-central2/testing/xpcshell/runxpcshelltests.py", line 971, in run_test
    if mozcrash.check_for_crashes(test_dir, self.symbolsPath, test_name=name):
  File "/Users/raymondlee/Documents/appcoast/mozilla-central2/testing/mozbase/mozcrash/mozcrash/mozcrash.py", line 94, in check_for_crashes
    stderr=subprocess.PIPE)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 711, in __init__
    errread, errwrite)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/subprocess.py", line 1308, in _execute_child
    raise child_exception
OSError: [Errno 13] Permission denied



> We should definitely add a check to the DownloadLegacy blocking test, to
> verify that the file is actually removed.
> 
> ::: toolkit/components/jsdownloads/src/DownloadCore.jsm
> @@ +273,5 @@
> >  
> > +      // Disallow download if parental controls service restricts it.
> > +      if (yield DownloadIntegration.shouldBlockForParentalControls(this)) {
> > +        throw new DownloadError(Cr.NS_ERROR_FAILURE, "Download blocked.");
> > +      }
> 
> let error = new DownloadError(Cr.NS_ERROR_FAILURE, "Download blocked.");
> error.becauseBlocked = true;
> error.becauseBlockedByParentalControls = true;
> throw error;
> 

Updated

> @@ +522,5 @@
> >      this.message = aMessage;
> > +    if (aMessage == "Download blocked.") {
> > +      this.becauseBlocked = true;
> > +      this.becauseBlockedByParentalControls = true;
> > +    }
> 
> (and remove this)
> 

Done

> ::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
> @@ +63,5 @@
> >  this.DownloadIntegration = {
> >    // For testing only
> >    testMode: false,
> >    dontLoad: false,
> > +  dontCheckParentalControls: false,
> 
> I'd also add "shouldBlockInTest: false" here explicitly.

Done

> 
> @@ +247,5 @@
> > +
> > +    let shouldBlock = (gParentalControlsService &&
> > +                       gParentalControlsService.parentalControlsEnabled &&
> > +                       gParentalControlsService.blockFileDownloadsEnabled);
> > +
> 
> We should add auditing, that is we should call the "log" method if
> loggingEnabled returns true, and specify whether the download was blocked or
> not.
> 
> The log of downloads should be visible during manual testing in the
> operating system.

Done. Also, checked with windows 7 Family Safety and it worked fine.

> 
> ::: toolkit/components/jsdownloads/test/unit/test_DownloadLegacy.js
> @@ +370,5 @@
> > +    yield download.start();
> > +    do_throw("The download should have blocked.");
> > +  } catch (ex if ex instanceof Downloads.Error && ex.becauseBlocked) {
> > +    do_check_true(ex.becauseBlockedByParentalControls);
> > +  }
> 
> So, we should add a test here that verifies the file doesn't exist.
> 
> Because the cancellation process is asynchronous, unfortunately we may have
> to wait on promiseExecuteSoon a couple of times to ensure the file is
> actually removed.

Without promiseExecuteSoon, it still works on Mac and Windows 7.

Comment 12

[:Paolo Amadini]

Attachment: Fixing the cancellation issues

(In reply to Raymond Lee [:raymondlee] from comment #11)
> Updated and solved the issue.  However, after doing that, test_error in
> test_DownloadLegacy.js starts throwing error. I couldn't figure out why. Any
> ideas?

It seems that we cannot call aCancelable after we've been notified of failure.
This patch resolves the issue on my machine, please take a look and see if
this works for you too, in which case you can integrate it in your patch.

> Without promiseExecuteSoon, it still works on Mac and Windows 7.

Good!

Comment 13

[:Paolo Amadini]

Comment on attachment 768795 [details] [diff] [review]
v4

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

Looks good! r+ with the changes from the additional patch, if it works for you.

::: toolkit/components/jsdownloads/src/DownloadIntegration.jsm
@@ +244,5 @@
> +  shouldBlockForParentalControls: function DI_shouldBlockForParentalControls(aDownload) {
> +    if (this.dontCheckParentalControls) {
> +      return Promise.resolve(this.shouldBlockInTest);
> +    }
> +    

nit: whitespace

Comment 14

[Raymond Lee [:raymondlee]]

(In reply to Paolo Amadini [:paolo] from comment #13)
> Comment on attachment 768795 [details] [diff] [review]
> v4
> 
> Review of attachment 768795 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good! r+ with the changes from the additional patch, if it works for
> you.
> 

Yes, that works for me.

Comment 15

[Raymond Lee [:raymondlee]]

Attachment: v5

Merged two patches

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/caf93078fbdb

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/caf93078fbdb