Update Permission Manager database to handle apps and mozbrowser

RESOLVED FIXED in mozilla17

Status

()

Core
General
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla17
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [qa-])

Attachments

(8 attachments, 3 obsolete attachments)

9.93 KB, patch
sicking
: review+
Details | Diff | Splinter Review
16.83 KB, patch
sicking
: review+
Details | Diff | Splinter Review
10.24 KB, patch
sicking
: review+
Details | Diff | Splinter Review
7.64 KB, patch
sicking
: review+
Details | Diff | Splinter Review
19.66 KB, patch
sicking
: review+
Details | Diff | Splinter Review
2.57 KB, patch
sicking
: review+
Details | Diff | Splinter Review
5.89 KB, patch
sicking
: review+
Details | Diff | Splinter Review
30.53 KB, patch
Justin Lebar (not reading bugmail)
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
Permissions should not be given per-origin but per { origin, app, mozbrowser }.
(Assignee)

Comment 1

5 years ago
Created attachment 645495 [details] [diff] [review]
Part 1 - Move the logic from methods taking URIs to methods taking principals
Assignee: nobody → mounir
Status: NEW → ASSIGNED
Attachment #645495 - Flags: review?(jonas)
(Assignee)

Comment 2

5 years ago
Created attachment 645496 [details] [diff] [review]
Part 2 - AddInternal() and CommonTestPermission() should use nsIPrincipal
Attachment #645496 - Flags: review?(jonas)
Attachment #645495 - Flags: review?(jonas) → review+
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
(Assignee)

Comment 4

5 years ago
(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.
blocking-basecamp: ? → +
Blocks: 781870
(Assignee)

Comment 5

5 years ago
Created attachment 652761 [details] [diff] [review]
Part 3 - Update nsHostTable/nsHostEntry to make them aware of the new security model
Attachment #652761 - Flags: review?(jonas)
(Assignee)

Comment 6

5 years ago
I think next steps are updating nsPermission, IPC::Permission and finally the database.
(Assignee)

Comment 7

5 years ago
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.
Attachment #652761 - Attachment is obsolete: true
Attachment #652761 - Flags: review?(jonas)
Attachment #652765 - Flags: review?(jonas)
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."
Attachment #652765 - Flags: review?(jonas) → review+
(Assignee)

Comment 9

5 years ago
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.
Attachment #652765 - Attachment is obsolete: true
(Assignee)

Comment 10

5 years ago
Created attachment 653811 [details] [diff] [review]
Part 4 - Update nsPermission to use appid/inbrowserelement
Attachment #653811 - Flags: review?(jonas)
(Assignee)

Comment 11

5 years ago
Created attachment 653812 [details] [diff] [review]
Part 5 - Update IPC::Permission to use appid/inbrowserelement
Attachment #653812 - Flags: review?(jonas)
Attachment #653811 - Flags: review?(jonas) → review+
Attachment #653812 - Flags: review?(jonas) → review+
(Assignee)

Comment 12

5 years ago
Created attachment 653870 [details] [diff] [review]
Part 6 - Update the database
Attachment #653870 - Flags: review?(jonas)
(Assignee)

Comment 13

5 years ago
Now, I have to write tests and fix a leak I haven't been able to reproduce locally yet.
Attachment #645496 - Flags: review?(jonas) → review+
Attachment #653870 - Flags: review?(jonas) → review+
(Assignee)

Comment 14

5 years ago
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.
Attachment #653942 - Flags: review?(jonas)
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.
(Assignee)

Updated

5 years ago
Blocks: 619236
(Assignee)

Comment 16

5 years ago
(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 ;)
Attachment #653942 - Flags: review?(jonas) → review+
(Assignee)

Comment 17

5 years ago
Created attachment 654064 [details] [diff] [review]
Part 7 - Tests
Attachment #654064 - Flags: review?(jonas)
Attachment #654064 - Flags: review?(jonas) → review+
(Assignee)

Updated

5 years ago
Depends on: 785161
(Assignee)

Comment 18

5 years ago
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.
Attachment #653810 - Attachment is obsolete: true
Attachment #654716 - Flags: review?(justin.lebar+bug)
Comment on attachment 654716 [details] [diff] [review]
Part 3 - Update nsHostTable/nsHostEntry to make them aware of the new security model (r=sicking)

Yeesh.
Attachment #654716 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 20

5 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Depends on: 785276
Backed out, sorry

https://hg.mozilla.org/mozilla-central/rev/eccef9b3f1f1
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

5 years ago
Blocks: 785631
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
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Can't tell if something is testable from an end-user perspective or not.
Whiteboard: [qa?]

Updated

5 years ago
QA Contact: jsmith
(Assignee)

Comment 24

5 years ago
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.
(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.
Whiteboard: [qa?] → [qa-]
(Assignee)

Updated

5 years ago
Depends on: 787717

Updated

5 years ago
Depends on: 814554
You need to log in before you can comment on or make changes to this bug.