Closed Bug 1433513 Opened 6 years ago Closed 6 years ago

Permissions panel needs to handle new expire type (EXPIRE_POLICY) correctly

Categories

(Firefox :: Site Identity, defect, P1)

Product:
Firefox
Component:
Site Identity
Version:
60 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: Felipe, Assigned: prathiksha)

References

Details

Attachments

(1 file)

Bug 1433513 - Correctly handle new expire type (EXPIRE_POLICY) in the Site Identity panel and Page Info window.
59 bytes, text/x-review-board-request
johannh
: review+
Details 
In bug 1428922, a new expire type for permissions were added, called EXPIRE_POLICY. It works the same way as a EXPIRE_SESSION permission, in that it's not written to disk. However, once set, this permission can't be changed anymore for the duration of the session.

You can still try to change the permission through the dropdown, but it won't have any effect.

I think the dropdown should show as disabled, or maybe just plain-text (no dropdown).


(I thought bug 1433511 was also caused by this new permission type, but I was able to reproduce it with a normal permission too, so I filed it separately).


STR:
Services.perms.add(Services.io.newURI("http://www.popuptest.com"), "popup", Ci.nsIPermissionManager.ALLOW_ACTION, Ci.nsIPermissionManager.EXPIRE_POLICY);

Visit http://www.popuptest.com

Click the Site Identity icon
Blocks: 1433173
We should resolve this before policy engine gets introduced in 60. It should not be very hard to implement. Prathiksha, if you have time to finish this before 60 (beginning of March), feel free to take it. Otherwise I can do it, maybe next week.
Priority: -- → P1
Assignee: nobody → prathikshaprasadsuman
Status: NEW → ASSIGNED
Attachment #8950678 - Flags: feedback?(jhofmann)
Comment on attachment 8950678 [details]
Bug 1433513 - Correctly handle new expire type (EXPIRE_POLICY) in the Site Identity panel and Page Info window.

https://reviewboard.mozilla.org/r/219934/#review226426


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/base/content/test/permissions/browser_permissions.js:231
(Diff revision 2)
>  });
> +
> +add_task(async function testPolicyPermission() {
> +  // This function tests the control center UI when policy permissions are set.
> +  await BrowserTestUtils.withNewTab(PERMISSIONS_PAGE, async function() {
> +    let permissionsList = document.getElementById("identity-popup-permission-list");

Error: 'permissionslist' is assigned a value but never used. allowed unused vars must match /^cc|ci|cu|cr|exported_symbols/. [eslint: no-unused-vars]
Comment on attachment 8950678 [details]
Bug 1433513 - Correctly handle new expire type (EXPIRE_POLICY) in the Site Identity panel and Page Info window.

This looks pretty good already, but I don't think we should say "Allowed Temporarily" for policy permissions. Maybe you can fix up the getCurrentStateLabel function[0] to return e.g. "Allowed" for policy scope, if Felipe doesn't have another idea of what it should say...

[0] https://searchfox.org/mozilla-central/rev/9011be0a172711bc243e50dfca16d42e877bf4ec/browser/modules/SitePermissions.jsm#544

How is page info handling the new permission scope?
Attachment #8950678 - Flags: feedback+
Felipe, the latest revision of the patch has a test for the policy permissions UI. Is there a way to remove a policy permission that's once set in the test file so I can clean up the test when it reaches the end? It looks like I can't use Services.perms.remove() to remove a EXPIRE_POLICY permission.
Flags: needinfo?(felipc)
(In reply to Johann Hofmann [:johannh] from comment #5)

> How is page info handling the new permission scope?

Currently, the patch doesn't handle the UI in page info. I will disable the radio buttons and the checkbox for policy permissions in page info before review.
(In reply to :prathiksha from comment #7)
> Felipe, the latest revision of the patch has a test for the policy
> permissions UI. Is there a way to remove a policy permission that's once set
> in the test file so I can clean up the test when it reaches the end? It
> looks like I can't use Services.perms.remove() to remove a EXPIRE_POLICY
> permission.

No, once a permission has been set with EXPIRE_POLICY, there's no way to remove it. But it's not saved anywhere, so it only remains for the duration of the session. If it's not causing any problems with follow-up tests, you don't need to worry about cleaning it up.
Flags: needinfo?(felipc)
(In reply to Johann Hofmann [:johannh] from comment #5)
> Comment on attachment 8950678 [details]
> Bug 1433513 - Correctly handle new expire type (EXPIRE_POLICY) in the Site
> Identity panel.
> 
> This looks pretty good already, but I don't think we should say "Allowed
> Temporarily" for policy permissions. Maybe you can fix up the
> getCurrentStateLabel function[0] to return e.g. "Allowed" for policy scope,
> if Felipe doesn't have another idea of what it should say...

Yeah I agree that it's best to use the permanent labels of Allowed/Blocked for the policy scope
This patch currently does not handle the flash plugin permission in page info. I'll be working on Bug 1192927 next to remove plugin specific code and initialize flash just like the other permissions in browser/base/content/pageinfo/permissions.js and it should automatically handle policy permission state for flash in page info.
(In reply to :prathiksha from comment #13)
> This patch currently does not handle the flash plugin permission in page
> info. I'll be working on Bug 1192927 next to remove plugin specific code and
> initialize flash just like the other permissions in
> browser/base/content/pageinfo/permissions.js and it should automatically
> handle policy permission state for flash in page info.

Ah that's great! There's so much complexity in the plugin code that can be removed right now since we only support Flash and not other plugins.
Comment on attachment 8950678 [details]
Bug 1433513 - Correctly handle new expire type (EXPIRE_POLICY) in the Site Identity panel and Page Info window.

https://reviewboard.mozilla.org/r/219934/#review227790

This looks great, thank you!

::: browser/base/content/browser.js:8069
(Diff revision 5)
>  
> +    container.appendChild(img);
> +    container.appendChild(nameLabel);
> +    container.appendChild(stateLabel);
> +
> +    if (isPolicyPermission) {

Can you add a comment why we're doing this?

::: browser/base/content/test/permissions/browser_permissions.js:232
(Diff revision 5)
> +
> +// Test the control center UI when policy permissions are set.
> +add_task(async function testPolicyPermission() {
> +  await BrowserTestUtils.withNewTab(PERMISSIONS_PAGE, async function() {
> +    await SpecialPowers.pushPrefEnv({set: [
> +    ["dom.disable_open_during_load", true],

Nit: indent this

::: browser/base/content/test/permissions/browser_permissions.js:262
(Diff revision 5)
> +    // The menulist is specific to the "popup" permission.
> +    let menulist = document.getElementById("identity-popup-popup-menulist");
> +    ok(menulist == null, "The popup permission menulist is not visible");
> +
> +    let removeButton = permissionsList.querySelectorAll(".identity-popup-permission-remove-button");
> +    ok(removeButton != null, "The permission remove button is not visible");

I don't understand this assertion. Shouldn't it be null?
Attachment #8950678 - Flags: review?(jhofmann) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6cd2792d5ebc
Correctly handle new expire type (EXPIRE_POLICY) in the Site Identity panel and Page Info window. r=johannh
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6cd2792d5ebc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: