Last Comment Bug 777072 - Update Permission Manager database to handle apps and mozbrowser
: Update Permission Manager database to handle apps and mozbrowser
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: Mounir Lamouri (:mounir)
: Jason Smith [:jsmith]
Mentors:
Depends on: 785161 785276 787717 814554
Blocks: app-data-jars basecamp-permissions 619236 785631
  Show dependency treegraph
 
Reported: 2012-07-24 14:28 PDT by Mounir Lamouri (:mounir)
Modified: 2012-11-26 03:17 PST (History)
6 users (show)
mounir: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Part 1 - Move the logic from methods taking URIs to methods taking principals (9.93 KB, patch)
2012-07-24 14:29 PDT, Mounir Lamouri (:mounir)
jonas: review+
Details | Diff | Splinter Review
Part 2 - AddInternal() and CommonTestPermission() should use nsIPrincipal (16.83 KB, patch)
2012-07-24 14:30 PDT, Mounir Lamouri (:mounir)
jonas: review+
Details | Diff | Splinter Review
Part 3 - Update nsHostTable/nsHostEntry to make them aware of the new security model (25.18 KB, patch)
2012-08-17 07:28 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part 3 - Update nsHostTable/nsHostEntry to make them aware of the new security model (25.25 KB, patch)
2012-08-17 07:42 PDT, Mounir Lamouri (:mounir)
jonas: review+
Details | Diff | Splinter Review
Part 3 - Update nsHostTable/nsHostEntry to make them aware of the new security model (r=sicking) (25.32 KB, patch)
2012-08-21 10:05 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Part 4 - Update nsPermission to use appid/inbrowserelement (10.24 KB, patch)
2012-08-21 10:06 PDT, Mounir Lamouri (:mounir)
jonas: review+
Details | Diff | Splinter Review
Part 5 - Update IPC::Permission to use appid/inbrowserelement (7.64 KB, patch)
2012-08-21 10:06 PDT, Mounir Lamouri (:mounir)
jonas: review+
Details | Diff | Splinter Review
Part 6 - Update the database (19.66 KB, patch)
2012-08-21 11:36 PDT, Mounir Lamouri (:mounir)
jonas: review+
Details | Diff | Splinter Review
Part 6b - Update profile creation scripts to use the new DB schema (2.57 KB, patch)
2012-08-21 14:15 PDT, Mounir Lamouri (:mounir)
jonas: review+
Details | Diff | Splinter Review
Part 7 - Tests (5.89 KB, patch)
2012-08-21 20:24 PDT, Mounir Lamouri (:mounir)
jonas: review+
Details | Diff | Splinter Review
Part 3 - Update nsHostTable/nsHostEntry to make them aware of the new security model (r=sicking) (30.53 KB, patch)
2012-08-23 11:48 PDT, Mounir Lamouri (:mounir)
justin.lebar+bug: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2012-07-24 14:28:05 PDT
Permissions should not be given per-origin but per { origin, app, mozbrowser }.
Comment 1 Mounir Lamouri (:mounir) 2012-07-24 14:29:23 PDT
Created attachment 645495 [details] [diff] [review]
Part 1 - Move the logic from methods taking URIs to methods taking principals
Comment 2 Mounir Lamouri (:mounir) 2012-07-24 14:30:30 PDT
Created attachment 645496 [details] [diff] [review]
Part 2 - AddInternal() and CommonTestPermission() should use nsIPrincipal
Comment 3 Jonas Sicking (:sicking) PTO Until July 5th 2012-07-25 19:07:01 PDT
Comment on attachment 645496 [details] [diff] [review]
Part 2 - AddInternal() and CommonTestPermission() should use nsIPrincipal

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

I'll try to fix this up.

::: extensions/cookie/nsPermissionManager.cpp
@@ +304,5 @@
>        const IPC::Permission &perm = perms[i];
> +
> +      nsCOMPtr<nsIPrincipal> principal;
> +      nsresult rv = GetPrincipalForHost(perm.host, getter_AddRefs(principal));
> +      NS_ENSURE_SUCCESS(rv, rv);

This doesn't seem right. We should receive data from the parent process which includes appId+isInBrowserElement
Comment 4 Mounir Lamouri (:mounir) 2012-07-25 21:41:08 PDT
(In reply to Jonas Sicking (:sicking) from comment #3)
> Comment on attachment 645496 [details] [diff] [review]
> Part 2 - AddInternal() and CommonTestPermission() should use nsIPrincipal
> 
> Review of attachment 645496 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'll try to fix this up.
> 
> ::: extensions/cookie/nsPermissionManager.cpp
> @@ +304,5 @@
> >        const IPC::Permission &perm = perms[i];
> > +
> > +      nsCOMPtr<nsIPrincipal> principal;
> > +      nsresult rv = GetPrincipalForHost(perm.host, getter_AddRefs(principal));
> > +      NS_ENSURE_SUCCESS(rv, rv);
> 
> This doesn't seem right. We should receive data from the parent process
> which includes appId+isInBrowserElement

The idea was to fix that in another part actually.
Comment 5 Mounir Lamouri (:mounir) 2012-08-17 07:28:32 PDT
Created attachment 652761 [details] [diff] [review]
Part 3 - Update nsHostTable/nsHostEntry to make them aware of the new security model
Comment 6 Mounir Lamouri (:mounir) 2012-08-17 07:29:51 PDT
I think next steps are updating nsPermission, IPC::Permission and finally the database.
Comment 7 Mounir Lamouri (:mounir) 2012-08-17 07:42:40 PDT
Created attachment 652765 [details] [diff] [review]
Part 3 - Update nsHostTable/nsHostEntry to make them aware of the new security model

I introduced syntax errors with last minute changes.
Comment 8 Jonas Sicking (:sicking) PTO Until July 5th 2012-08-21 01:21:23 PDT
Comment on attachment 652765 [details] [diff] [review]
Part 3 - Update nsHostTable/nsHostEntry to make them aware of the new security model

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

::: extensions/cookie/nsPermissionManager.h
@@ +53,5 @@
> +  /**
> +   * PermissionKey is the key used by PermissionHashKey hash table.
> +   *
> +   * NOTE: It could be implementing nsIHashable but there is no reason to worry
> +   * with XPCOM interfaces while we don't need to.

"when we don't need to."
Comment 9 Mounir Lamouri (:mounir) 2012-08-21 10:05:35 PDT
Created attachment 653810 [details] [diff] [review]
Part 3 - Update nsHostTable/nsHostEntry to make them aware of the new security model (r=sicking)

With leak/test failures being fixed.
Comment 10 Mounir Lamouri (:mounir) 2012-08-21 10:06:07 PDT
Created attachment 653811 [details] [diff] [review]
Part 4 - Update nsPermission to use appid/inbrowserelement
Comment 11 Mounir Lamouri (:mounir) 2012-08-21 10:06:46 PDT
Created attachment 653812 [details] [diff] [review]
Part 5 - Update IPC::Permission to use appid/inbrowserelement
Comment 12 Mounir Lamouri (:mounir) 2012-08-21 11:36:25 PDT
Created attachment 653870 [details] [diff] [review]
Part 6 - Update the database
Comment 13 Mounir Lamouri (:mounir) 2012-08-21 11:37:23 PDT
Now, I have to write tests and fix a leak I haven't been able to reproduce locally yet.
Comment 14 Mounir Lamouri (:mounir) 2012-08-21 14:15:07 PDT
Created attachment 653942 [details] [diff] [review]
Part 6b - Update profile creation scripts to use the new DB schema

I will merge this with Part 6 before pushing.
Comment 15 Jonas Sicking (:sicking) PTO Until July 5th 2012-08-21 14:17:11 PDT
Hmm... do you have code to deal with upgrading existing databases in the user's profile? We don't want to lose any user data when the user installs the next version of firefox.
Comment 16 Mounir Lamouri (:mounir) 2012-08-21 14:36:38 PDT
(In reply to Jonas Sicking (:sicking) from comment #15)
> Hmm... do you have code to deal with upgrading existing databases in the
> user's profile? We don't want to lose any user data when the user installs
> the next version of firefox.

This is done in "Part 6". You reviewed that patch 2 or 3 hours ago ;)
Comment 17 Mounir Lamouri (:mounir) 2012-08-21 20:24:34 PDT
Created attachment 654064 [details] [diff] [review]
Part 7 - Tests
Comment 18 Mounir Lamouri (:mounir) 2012-08-23 11:48:49 PDT
Created attachment 654716 [details] [diff] [review]
Part 3 - Update nsHostTable/nsHostEntry to make them aware of the new security model (r=sicking)

Justin, could you review the tests changes. Everything else has been reviewed by Jonas.
Comment 19 Justin Lebar (not reading bugmail) 2012-08-23 13:43:59 PDT
Comment on attachment 654716 [details] [diff] [review]
Part 3 - Update nsHostTable/nsHostEntry to make them aware of the new security model (r=sicking)

Yeesh.
Comment 21 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-23 22:33:47 PDT
Backed out, sorry

https://hg.mozilla.org/mozilla-central/rev/eccef9b3f1f1
Comment 23 Jason Smith [:jsmith] 2012-09-01 07:48:13 PDT
Can't tell if something is testable from an end-user perspective or not.
Comment 24 Mounir Lamouri (:mounir) 2012-09-02 08:00:42 PDT
It is testable from an end-user perspective but we have automated tests for that so I think it's not worth doing manual testings.
Comment 25 Jason Smith [:jsmith] 2012-09-02 08:02:06 PDT
(In reply to Mounir Lamouri (:mounir) from comment #24)
> It is testable from an end-user perspective but we have automated tests for
> that so I think it's not worth doing manual testings.

Okay sounds good.

Note You need to log in before you can comment on or make changes to this bug.