Closed Bug 874197 Opened 9 years ago Closed 9 years ago

Allow session-based permissions to expire at a fixed time

Categories

(Core :: Permission Manager, defect, P2)

x86_64
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

Attachments

(1 file, 1 obsolete file)

Currently persistent permissions can be set to expire at a particular time using EXPIRE_TIME. But session permissions can not be set to expire at a particular time. For the plugin click-to-activate UI, the current UX spec is that the short permission will expire when the sessions ends *or* an hour of inactivity.

I managed to do this merely by redefining the meaning of TYPE_SESSION so that if expireTime is 0 (the default) then it behaves as it does currently. But if expireTime is a real number, then we expire it.
Attachment #752182 - Flags: superreview?(mounir)
Attachment #752182 - Flags: review?(josh)
Comment on attachment 752182 [details] [diff] [review]
Allow EXPIRE_SESSION to also expire by time, rev. 1

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

Another approach would have been to consider that EXPIRE_TIME permissions expire also when the session is terminated. Otherwise, adding a new permission expire type called EXPIRE_TIME_SESSION. FWIW, I'm fine with the current approach as much as I would have been okay with those two (though, the former could have created regressions assuming some consumers relied n the behaviour).

r/sr=me, with the comments applied and the requested tests added.

::: extensions/cookie/nsPermissionManager.cpp
@@ +625,5 @@
>    }
>  
> +  if (aExpireTime == nsIPermissionManager::EXPIRE_SESSION &&
> +      aExpireTime != 0 &&
> +      aExpireTime <= (PR_Now() / 1000)) {

I guess we can merge this condition with the one above:
  if ((aExpireType == nsIPermissionManager::EXPIRE_TIME ||
       (aExpireType == nsIPermissionManager::EXPIRE_SESSION &&
        aExpireTime != 0) &&
      aExpireTime <= (PR_Now() / 1000)) {

You should probably explain in a comment what this is actually doing though.

Also, I guess you probably had a bug here because there is a typo in this code. Could you add a test to test that behaviour? Just add some  EXPIRE_SESSION and EXPIRE_TIME permissions that have an expireTime before PR_Now() and check that when enumerating all permissions, you haven't any new permissions. Counting them should do.

@@ +1152,5 @@
> +    if ((permEntry.mExpireType == nsIPermissionManager::EXPIRE_TIME &&
> +         permEntry.mExpireTime <= (PR_Now() / 1000)) ||
> +        (permEntry.mExpireType == nsIPermissionManager::EXPIRE_SESSION &&
> +         permEntry.mExpireTime != 0 &&
> +         permEntry.mExpireTime <= (PR_Now() / 1000))) {

nit: you could also merge them as above but either is fine.

::: extensions/cookie/nsPermissionManager.h
@@ +54,4 @@
>      int64_t  mExpireTime;
>      uint32_t mNonSessionPermission;
>      uint32_t mNonSessionExpireType;
> +    uint32_t mNonSessionExpireTime;

You should modify the ctor too which is going to require modifying ctor calls.

::: extensions/cookie/test/unit/test_permmanager_expiration.js
@@ +60,5 @@
> +  // Check that .getPermission returns a matching result
> +  do_check_null(pm.getPermission(principal, "test/expiration-perm-exp", false));
> +  do_check_null(pm.getPermission(principal, "test/expiration-session-exp", false));
> +  do_check_null(pm.getPermission(principal, "test/expiration-perm-exp2", false));
> +  do_check_null(pm.getPermission(principal, "test/expiration-session-exp", false));

There is a typo here, I think you meant to check "test/expiration-session-exp2".
Attachment #752182 - Flags: superreview?(mounir) → superreview+
Attachment #756183 - Flags: review?(josh)
Attachment #752182 - Flags: review?(josh)
Comment on attachment 752182 [details] [diff] [review]
Allow EXPIRE_SESSION to also expire by time, rev. 1

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

What Mounir said.
Attachment #752182 - Flags: review+
Attachment #756183 - Flags: review?(josh) → review+
Attachment #752182 - Attachment is obsolete: true
There was a typo in this patch ("== 0" should be "!= 0" in nsPermissionManager::GetPermissionHashKey) which caused tests to fail (yay). Pushed with that corrected.

https://hg.mozilla.org/integration/mozilla-inbound/rev/15df9914a1ad
Target Milestone: --- → mozilla24
https://hg.mozilla.org/mozilla-central/rev/15df9914a1ad
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Blocks: 880735
You need to log in before you can comment on or make changes to this bug.