Closed Bug 519263 Opened 11 years ago Closed 11 years ago

Session and time-based expiration for permissions in permissions manager

Categories

(Core :: Networking, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: geekboy, Assigned: geekboy)

References

Details

Attachments

(1 file, 4 obsolete files)

For some types of permissions kept by permissions manager (like Force-TLS, Bug 495115) it would be useful to have permissions expire after a specified period of time.  Additionally, it may be convenient to have "session permissions" that expire like session cookies.

We should extend the permissions manager interface to support an optional expiry timestamp and a boolean "forSessionOnly" flag, and modify the back-end nsPermissionManager.{cpp,h} to auto-expire and purge permissions at the appropriate times.
Attached patch proposed patch (obsolete) — Splinter Review
Adds an "expiretime" field to the database, and an expireType (NEVER, SESSION, TIME) flag and time to the IDLs.
Attachment #404147 - Flags: review?(dwitte)
Comment on attachment 404147 [details] [diff] [review]
proposed patch

>diff -r ab3ee8fc063d extensions/cookie/nsCookiePermission.cpp
>-  return mPermMgr->Add(aURI, kPermissionType, aAccess);
>+  return mPermMgr->Add(aURI, kPermissionType, aAccess, nsIPermissionManager::EXPIRE_NEVER, nsnull);

Null is for pointers - s/nsnull/0/ for the PRTime arg, please - ditto for all the other consumer changes, and within nsPermissionManager itself.

>diff -r ab3ee8fc063d extensions/cookie/nsPermissionManager.cpp
>@@ -301,7 +305,13 @@
>-  return AddInternal(host, nsDependentCString(aType), aPermission, 0, eNotify, eWriteToDB);
>+  if (!aExpireType) {
>+    aExpireType = nsIPermissionManager::EXPIRE_NEVER;
>+    aExpireTime = PRInt64(0);
>+  }
>+
>+  return AddInternal(host, nsDependentCString(aType), aPermission, 0, 
>+                     aExpireType, aExpireTime, eNotify, eWriteToDB);

Hmm, so, not sure I like the overloading of expireTime == 0 with EXPIRE_NEVER. Some day, someone is going to think that passing 0 for expiry into nsIPermissionManager.add or nsIPermissionManager.remove is the best idea ever - and what should be a nop will become quite the antinop. There are a few solutions:

1) Add the expireType column to the db and be done with it. 64-bit ints are cheap, and this isn't places - we don't need to pennypinch for space. (Could init expireTime to 0 when expireType == EXPIRE_NEVER, but I wouldn't specify that in the idl.)
2) Use expireTime == LL_MAX to mean "never expire". Still overloading, but better than 0.
3) Bitmask the expireType into the type column. (type is only 32 bits, and sqlite stores INTEGERs as 64 bits, so we could technically do this. But it will suck for anyone trying to read the db themselves.)

I'd take 1 or 2, with a preference for 1.

Also, in future please produce patches with -p -U 8 -- more context ftw! :)

>@@ -534,7 +554,11 @@
>-      if (entry->GetPermission(aType) != nsIPermissionManager::UNKNOWN_ACTION)
>+
>+      // if we remove the entry, keep looking for others.
>+      if (entry->IsExpired(aType))
>+        Remove(aHost, mTypeArray[aType].get());
>+      else if (entry->GetPermission(aType) != nsIPermissionManager::UNKNOWN_ACTION)
>         break;

Extra newline after |if|.

>@@ -712,7 +742,15 @@
>-    rv = AddInternal(host, type, permission, id, eDontNotify, eNoDBOperation);
>+    // convert into PRTime value (microseconds, not milliseconds)
>+    expiretime = stmt->AsInt64(4) * 1000;
>+
>+    expiretype = expiretime > 0 ? nsIPermissionManager::EXPIRE_TIME : 
>+                                  nsIPermissionManager::EXPIRE_NEVER;
>+
>+
>+    rv = AddInternal(host, type, permission, id, expiretype, expiretime,
>+                     eDontNotify, eNoDBOperation);

Extra newline.

>@@ -746,10 +784,11 @@
>   /* format is:
>-   * matchtype \t type \t permission \t host
>+   * matchtype \t type \t permission \t host \t expiretime

This method (Import(), I think) is for importing pre-sqlite permission files - we don't need to change the format there, so you can drop this stuff...

>-      rv = AddInternal(lineArray[3], lineArray[1], permission, 0, eDontNotify, eWriteToDB);
>+      rv = AddInternal(lineArray[3], lineArray[1], permission, 0, 
>+                       expiretype, expiretime, eDontNotify, eWriteToDB);

... and just assume EXPIRE_NEVER here.

>diff -r ab3ee8fc063d extensions/cookie/nsPermissionManager.h
>@@ -89,6 +95,16 @@
>+  inline PRBool IsExpired(PRUint32 aType) const
>+  {
>+    for (PRUint32 i = 0; i < mPermissions.Length(); ++i)
>+      if (mPermissions[i].mType == aType) {
>+        // compare time stored (milliseconds) to now (microseconds / 1000)
>+        return  mPermissions[i].mExpireType == nsIPermissionManager::EXPIRE_TIME  &&
>+                mPermissions[i].mExpireTime < PR_Now() / 1000;

Extra space after |return| and before |&&|.

>diff -r ab3ee8fc063d extensions/cookie/test/unit/test_permmanager_expiration.js
>+  // ... and that the others didn't
>+  do_check_eq(1, pm.testPermission(permURI, "test/expiration-perm-exp2"));
>+  do_check_eq(1, pm.testPermission(permURI, "test/expiration-perm-nexp"));

You could also test that the future expiration one actually expires at the right time - maybe make the expiration delta 100ms, and use a setTimeout() with a 200ms delay?

>diff -r ab3ee8fc063d netwerk/base/public/nsIPermission.idl
>@@ -66,4 +66,16 @@
>+    /**
>+     * The expiration time of the permission (in unix time).
>+     * Constants are EXPIRE_*, defined in nsIPermissionManager.
>+     * @see nsIPermissionManager
>+     */
>+    readonly attribute PRUint32 expireType;
>+
>+    /**
>+     * The expiration time of the permission (in unix time).
>+     */
>+    readonly attribute PRTime expireTime;

(see below)

diff -r ab3ee8fc063d netwerk/base/public/nsIPermissionManager.idl
@@ -81,6 +81,15 @@
   /**
+   * Predefined expiration types for permissions.  Permissions can be permanant
+   * (never expire), expire at the end of the session, or expire at a specified
+   * time.
+   */

s/permanant/permanent/

@@ -98,10 +107,18 @@
+   * @param expiretype  a constant defining whether this permission should
+   *                    never expire (EXPIRE_NEVER), expire at the end of the
+   *                    session (EXPIRE_SESSION), or expire at a specified time
+   *                    (EXPIRE_TIME).
+   * @param expiretime  an integer representation of when this permission
+   *                    should be forgotten (microseconds since Jan 1 1970 0:00:00). 

This idl says expireTime is microseconds, nsIPermission.idl says it's unix time (seconds), and the code says it's milliseconds. :)

Let's split the middle and go with milliseconds.

Also, it'd be nice to see some already-expired checks in nsPermissionManager::Add() and Read(). (We probably don't want to add something to the db that's already expired, and notify listeners about it, etc. And reading in the db is a really good place to do expiry checks for cheap.)

I'd like to see another patch, but otherwise looks good!
Attachment #404147 - Flags: review?(dwitte) → review-
Attached patch proposed patch (obsolete) — Splinter Review
(In reply to comment #2)
> 1) Add the expireType column to the db and be done with it. 64-bit ints are
> cheap, and this isn't places - we don't need to pennypinch for space. (Could
> init expireTime to 0 when expireType == EXPIRE_NEVER, but I wouldn't specify
> that in the idl.)
[snip]
> I'd take 1 or 2, with a preference for 1.

Implemented 1.

> >@@ -746,10 +784,11 @@
> >   /* format is:
> >-   * matchtype \t type \t permission \t host
> >+   * matchtype \t type \t permission \t host \t expiretime
> 
> This method (Import(), I think) is for importing pre-sqlite permission files -
> we don't need to change the format there, so you can drop this stuff...

Great, dropped.

> You could also test that the future expiration one actually expires at the
> right time - maybe make the expiration delta 100ms, and use a setTimeout() with
> a 200ms delay?

Added something equivalent with do_test_pending and do_timeout.

> This idl says expireTime is microseconds, nsIPermission.idl says it's unix time
> (seconds), and the code says it's milliseconds. :)
> 
> Let's split the middle and go with milliseconds.

Yeah, that was my intention, and how it was implemented, but I must've missed those comments with conflicting units.  Fixed.

> Also, it'd be nice to see some already-expired checks in
> nsPermissionManager::Add() and Read(). (We probably don't want to add something
> to the db that's already expired, and notify listeners about it, etc. And
> reading in the db is a really good place to do expiry checks for cheap.)

Good call.  Done.
Attachment #404147 - Attachment is obsolete: true
Attachment #405932 - Flags: review?(dwitte)
Comment on attachment 405932 [details] [diff] [review]
proposed patch

>@@ -243,28 +244,29 @@ nsPermissionManager::InitDB(PRBool aRemo
>   // cache frequently used statements (for insertion, deletion, and updating)
>   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>     "INSERT INTO moz_hosts "
>-    "(id, host, type, permission) "
>-    "VALUES (?1, ?2, ?3, ?4)"), getter_AddRefs(mStmtInsert));
>+    "(id, host, type, permission, expiretype, expiretime) "
>+    "VALUES (?1, ?2, ?3, ?4, ?5, ?6)"), getter_AddRefs(mStmtInsert));

Nit: can we call the columns expireType and expireTime, resp.?

Also, you need to write a bit more migration code here. See nsCookieService::InitDB() for an example. (In the switch statement, you'll need to add an upgrade case, and modify the default case.) Sorry I didn't catch this before.

>@@ -368,65 +382,72 @@ nsPermissionManager::AddInternal(const n
>       if (aNotifyOperation == eNotify) {
>         NotifyObserversWithPermission(aHost,
>                                       mTypeArray[typeIndex],
>                                       oldPermission,
>+                                      aExpireType,
>+                                      aExpireTime,
>                                       NS_LITERAL_STRING("deleted").get());

When notifying listeners about a deleted permission, we should include the expireType and expireTime of the permission being deleted - not the arbitrary values passed in.

Can you add tests for the permmgr notifications (added, deleted, changed)? Sorta not your job, but you are changing what gets provided in that notification. And they're easy to test. :)

>@@ -437,16 +458,18 @@ nsPermissionManager::Remove(const nsACSt
>   // AddInternal() handles removal, just let it do the work
>   return AddInternal(PromiseFlatCString(aHost),
>                      nsDependentCString(aType),
>                      nsIPermissionManager::UNKNOWN_ACTION,
>+                     nsIPermissionManager::EXPIRE_TIME,
>+                     0,

Not that it matters, but EXPIRE_NEVER seems slightly less arbitrary than EXPIRE_TIME w/ 0.

>@@ -529,17 +552,20 @@ nsPermissionManager::GetHostEntry(const 
>   do {
>     entry = mHostTable.GetEntry(aHost.get() + offset);
>     if (entry) {
>-      if (entry->GetPermission(aType) != nsIPermissionManager::UNKNOWN_ACTION)
>+      // if we remove the entry, keep looking for others.
>+      if (entry->IsExpired(aType))
>+        Remove(aHost, mTypeArray[aType].get());
>+      else if (entry->GetPermission(aType) != nsIPermissionManager::UNKNOWN_ACTION)

The abstraction here is nice, but we do a bunch more work than we need to:
1) We loop over the nsHostEntry::mPermissions array twice
2) We call PR_Now() once for each subdomain level.

I suggest modifying nsHostEntry::GetPermission() to return an nsPermissionEntry, and writing out the logic directly.

>@@ -648,25 +676,27 @@ nsPermissionManager::GetTypeIndex(const 
>-// wrapper function for mangling (host,type,perm) triplet into an nsIPermission.
>+// wrapper function for mangling (host,type,perm,expire) set into an nsIPermission.

s/expire/expireType,expireTime/

>@@ -684,40 +714,52 @@ nsPermissionManager::NotifyObservers(nsI
>-    rv = AddInternal(host, type, permission, id, eDontNotify, eNoDBOperation);
>+    // convert into PRTime value (microseconds, not milliseconds)
>+    expiretime = stmt->AsInt64(5) * 1000;
>+
>+    // don't add expired stuff
>+    if (expiretype == nsIPermissionManager::EXPIRE_TIME &&
>+        expiretime < PR_Now() / 1000)
>+      continue;

Looks like expiretime is in microseconds, but you're comparing against milliseconds? Let's go with milliseconds everywhere. Further, since PRTime kindasorta implies microseconds, can you s/PRTime/PRInt64/ everywhere? (Most especially the interface.)

Also, pull the PR_Now() out of the loop? It's expensive on some plats, and we don't need to do it a bunch of times.

Finally, we don't really clean out the db of expired entries anywhere. It's not super great to do it on startup, but let's just do that for now... take a look at nsCookieService::Read() for an example.

>@@ -760,31 +802,32 @@ nsPermissionManager::Import()
>     }
> 
>     nsTArray<nsCString> lineArray;
> 
>     // Split the line at tabs
>     ParseString(buffer, '\t', lineArray);
>     
>     if (lineArray[0].EqualsLiteral(kMatchTypeHost) &&
>-        lineArray.Length() == 4) {
>+        lineArray.Length() == 5) {

Looks like this snuck in.

>@@ -839,31 +884,43 @@ nsPermissionManager::UpdateDB(OperationT
>   case eOperationChanging:
>     {
>       rv = aStmt->BindInt64Parameter(0, aID);
>       if (NS_FAILED(rv)) break;
> 
>       rv = aStmt->BindInt32Parameter(1, aPermission);
>+      if (NS_FAILED(rv)) break;
>+
>+      rv = aStmt->BindInt32Parameter(4, aExpireType);
>+      if (NS_FAILED(rv)) break;
>+
>+      rv = aStmt->BindInt64Parameter(5, aExpireTime);

Indexes should be 2 and 3 here.

>diff -r ab3ee8fc063d netwerk/base/public/nsIPermission.idl
>+    /**
>+     * The expiration time of the permission (in unix time, milliseconds).
>+     */
>+    readonly attribute PRTime expireTime;

I'd rather the definition be explicitly called out here, just like you do in nsIPermissionManager.idl:

>+   * @param expiretime  an integer representation of when this permission
>+   *                    should be forgotten (milliseconds since Jan 1 1970 0:00:00).
Attachment #405932 - Flags: review?(dwitte) → review-
Attached patch proposed patch (round 3) (obsolete) — Splinter Review
Okay, here's another patch.  I fixed everything the review comments requested, and set it up so that when nsIPermissionManager:add() is called on a permission that exists, but with a different expireTime or expireType than previously recorded, the new expiration data gets saved.

There are also observer notification tests for "perm-changed" add, remove, change, and clear events.
Attachment #405932 - Attachment is obsolete: true
Attachment #406081 - Flags: review?(dwitte)
Comment on attachment 406081 [details] [diff] [review]
proposed patch (round 3)

>diff -r ab3ee8fc063d extensions/cookie/nsPermissionManager.cpp
> NS_IMETHODIMP
> nsPermissionManager::Add(nsIURI     *aURI,
>                          const char *aType,
>-                         PRUint32    aPermission)
>+                         PRUint32    aPermission,
>+                         PRUint32    aExpireType,
>+                         PRInt64     aExpireTime)
> {
>   NS_ENSURE_ARG_POINTER(aURI);
>   NS_ENSURE_ARG_POINTER(aType);

Would be nice to see a check for a valid aExpireType here.

> nsresult
> nsPermissionManager::Read()
> {
>   nsresult rv;
> 
>+  // delete expired permissions before we read in the db
>+  {
>+    // this deletion has its own scope so the write lock is released when done.
>+    nsCOMPtr<mozIStorageStatement> stmtDeleteExpired;
>+    rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>+          "DELETE FROM moz_hosts WHERE expireType = ?1 AND expireTime <= ?2"),
>+          getter_AddRefs(stmtDeleteExpired));

The "<=" comparison here is inconsistent with your other expiry checks.

>-    rv = AddInternal(host, type, permission, id, eDontNotify, eNoDBOperation);
>+    // convert into PRInt64 value (milliseconds)
>+    expireTime = stmt->AsInt64(5);
>+
>+    // don't add expired stuff
>+    if (expireType == nsIPermissionManager::EXPIRE_TIME &&
>+        expireTime < PR_Now() / 1000)
>+      continue;

Don't need this bit anymore.

Wonderful! r=me with fixes. :)
Attachment #406081 - Flags: review?(dwitte) → review+
Attached patch fix (obsolete) — Splinter Review
applied above mentioned minor fixes to approved patch.
Attachment #406081 - Attachment is obsolete: true
Attachment #406250 - Flags: review+
Comment on attachment 406250 [details] [diff] [review]
fix

Could use an API sr here - thanks!
Attachment #406250 - Flags: superreview?(mrbkap)
Comment on attachment 406250 [details] [diff] [review]
fix

>diff -r ab3ee8fc063d netwerk/base/public/nsIPermission.idl
>@@ -61,9 +61,22 @@ interface nsIPermission : nsISupports
>      * @see nsIPermissionManager
>      */
>     readonly attribute ACString type;
> 
>     /**
>      * The permission (see nsIPermissionManager.idl for allowed values)
>      */
>     readonly attribute PRUint32 capability;
>+
>+    /**
>+     * The expiration type of the permission (session, time-based or none).
>+     * Constants are EXPIRE_*, defined in nsIPermissionManager.
>+     * @see nsIPermissionManager
>+     */
>+    readonly attribute PRUint32 expireType;
>+
>+    /**
>+     * The expiration time of the permission (milliseconds since Jan 1 1970
>+     * 0:00:00).
>+     */
>+    readonly attribute PRInt64 expireTime;

Need to bump the iid here.

>diff -r ab3ee8fc063d netwerk/base/public/nsIPermissionManager.idl
>@@ -76,16 +76,25 @@ interface nsIPermissionManager : nsISupp
>    * default permission when no entry is found for a host, and
>    * should not be used by consumers to indicate otherwise.
>    */
>   const PRUint32 UNKNOWN_ACTION = 0;
>   const PRUint32 ALLOW_ACTION = 1;
>   const PRUint32 DENY_ACTION = 2;
> 
>   /**
>+   * Predefined expiration types for permissions.  Permissions can be permanent
>+   * (never expire), expire at the end of the session, or expire at a specified
>+   * time.
>+   */
>+  const PRUint32 EXPIRE_NEVER = 0;
>+  const PRUint32 EXPIRE_SESSION = 1;
>+  const PRUint32 EXPIRE_TIME = 2;
>+

And here too.
Attachment #406250 - Flags: superreview?(mrbkap) → superreview+
Attached patch fixSplinter Review
inserted new iids.
Attachment #406250 - Attachment is obsolete: true
Attachment #406689 - Flags: superreview+
Attachment #406689 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/c3f243110dc2
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Blocks: 527144
You need to log in before you can comment on or make changes to this bug.