Closed
Bug 874197
Opened 12 years ago
Closed 12 years ago
Allow session-based permissions to expire at a fixed time
Categories
(Core :: Permission Manager, defect, P2)
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.
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #752182 -
Flags: superreview?(mounir)
Attachment #752182 -
Flags: review?(josh)
Comment 2•12 years ago
|
||
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+
Assignee | ||
Comment 3•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #756183 -
Flags: review?(josh)
Assignee | ||
Updated•12 years ago
|
Attachment #752182 -
Flags: review?(josh)
Comment 4•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #756183 -
Flags: review?(josh) → review+
Assignee | ||
Updated•12 years ago
|
Attachment #752182 -
Attachment is obsolete: true
Assignee | ||
Comment 5•12 years ago
|
||
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
Comment 6•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•