777072 - Update Permission Manager database to handle apps and mozbrowser

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Mounir Lamouri (:mounir)]

Permissions should not be given per-origin but per { origin, app, mozbrowser }.

Comment 1

[Mounir Lamouri (:mounir)]

Attachment: Part 1 - Move the logic from methods taking URIs to methods taking principals

Comment 2

[Mounir Lamouri (:mounir)]

Attachment: Part 2 - AddInternal() and CommonTestPermission() should use nsIPrincipal

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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)]

(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)]

Attachment: Part 3 - Update nsHostTable/nsHostEntry to make them aware of the new security model

Comment 6

[Mounir Lamouri (:mounir)]

I think next steps are updating nsPermission, IPC::Permission and finally the database.

Comment 7

[Mounir Lamouri (:mounir)]

Attachment: 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) No longer reading bugmail consistently]

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)]

Attachment: 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)]

Attachment: Part 4 - Update nsPermission to use appid/inbrowserelement

Comment 11

[Mounir Lamouri (:mounir)]

Attachment: Part 5 - Update IPC::Permission to use appid/inbrowserelement

Comment 12

[Mounir Lamouri (:mounir)]

Attachment: Part 6 - Update the database

Comment 13

[Mounir Lamouri (:mounir)]

Now, I have to write tests and fix a leak I haven't been able to reproduce locally yet.

Comment 14

[Mounir Lamouri (:mounir)]

Attachment: 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) No longer reading bugmail consistently]

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)]

(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)]

Attachment: Part 7 - Tests

Comment 18

[Mounir Lamouri (:mounir)]

Attachment: 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)]

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 20

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/4e0bd449dcb1
https://hg.mozilla.org/mozilla-central/rev/13c586b3c010
https://hg.mozilla.org/mozilla-central/rev/8700d3b9054d
https://hg.mozilla.org/mozilla-central/rev/0e6ed173961e
https://hg.mozilla.org/mozilla-central/rev/a050027d1520
https://hg.mozilla.org/mozilla-central/rev/212b861d4d22
https://hg.mozilla.org/mozilla-central/rev/41f9eeefbee7

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Backed out, sorry

https://hg.mozilla.org/mozilla-central/rev/eccef9b3f1f1

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2b4f2729b401
https://hg.mozilla.org/mozilla-central/rev/0e1c51a8387d
https://hg.mozilla.org/mozilla-central/rev/ef0744b50b61
https://hg.mozilla.org/mozilla-central/rev/789055abc89d
https://hg.mozilla.org/mozilla-central/rev/346a4ed09e06
https://hg.mozilla.org/mozilla-central/rev/961258980236
https://hg.mozilla.org/mozilla-central/rev/5e035bffbad5

Comment 23

[Jason Smith [:jsmith]]

Can't tell if something is testable from an end-user perspective or not.

Comment 24

[Mounir Lamouri (:mounir)]

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]]

(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.