1433271 - Both "allow" and "block" fields are required to set a "block" policy

Status: VERIFIED FIXED

(Firefox :: Enterprise Policies, defect)

Description

[Abe - QA (:Abe_LV)]

1. Set your policy as:
{
  "policies": {
    "install_addons": {  
      "block": [            
           "https://duckduckgo.com",      
      ]
    }
  }
}
 
2. Open browser console and check for warnings

Actual Result:
Throws warnings-
PoliciesValidator.jsm:Array expected but not received  PoliciesValidator.jsm:56
Enterprise Policies:Invalid parameters specified for install_addons.  EnterprisePolicies.js:120

Expected Result:
Users should not include an empty array for "allow" if they only want to set a block policy.

For the above policy to work, it should be re-written as:

{
  "policies": {
    "install_addons": {
      "allow": [      
     
      ],
      "block": [            
           "https://duckduckgo.com",
      
      ]
    }
  }
}

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Bug 1433271 - Don't fail policy validation if an array inside a object is missing.

A top-level array (i.e., if a policy directly requires an array) won't be affected by this problem, because if the array is missing, that means that that policy is not present, so it will be ignored.

However, this can affect an array that is expected inside another object, for more complex policy types (like the popups permission which accepts both an 'allow' and a 'block' array of URLs.

In the future, we should implement the 'required' property as defined by the JSON-Schema standard, but there's not a strong use case for it yet, so let's do the simple solution for now

Review commit: https://reviewboard.mozilla.org/r/218260/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/218260/

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Note: I started implementing the "required" part of the JSON-Schema spec to fix this bug, but it was getting more complicated than initially thought, so I decided not to do it here and just go for the simple fix, since this problem is getting in the way of the GPO work.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8948866 [details]
Bug 1433271 - Don't fail policy validation if an array inside a object is missing.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/218260/diff/1-2/

Comment 4

[Code Review Bot [:reviewbot]]

Comment on attachment 8948866 [details]
Bug 1433271 - Don't fail policy validation if an array inside a object is missing.

https://reviewboard.mozilla.org/r/218260/#review224050


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/enterprisepolicies/tests/browser/browser_policies_validate_and_parse_API.js:259
(Diff revision 1)
> +    allow: [true, true, true]
> +  }, schema);
> +
> +  ok(valid, "Object is valid");
> +  is(parsed.allow.length, 3, "Allow array is correct.");
> +  is(parsed.block, undefined, "Block array is undefined, as expected.")

Error: Missing semicolon. [eslint: semi]

Comment 5

[:Felipe Gomes (needinfo for replies!)]

(In reply to Code Review Bot [:reviewbot] from comment #4)
> Error: Missing semicolon. [eslint: semi]

I fixed this in the updated review request (comment 3)

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 8948866 [details]
Bug 1433271 - Don't fail policy validation if an array inside a object is missing.

https://reviewboard.mozilla.org/r/218260/#review224346

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8130dd208393
Don't fail policy validation if an array inside a object is missing. r=Mossop

Comment 8

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/8130dd208393

Comment 9

[Abe - QA (:Abe_LV)]

We tested this fix on Windows-10x64, Windows-7x86, Ubuntu 16.04 and OSX 10.13. We verified this as fixed.
Test cases and runs are here-https://testrail.stage.mozaws.net/index.php?/plans/view/7734