Last Comment Bug 775377 - nsIContentPermissionRequest should use nsIPrincipal instead of URI
: nsIContentPermissionRequest should use nsIPrincipal instead of URI
Status: RESOLVED FIXED
[qa-]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: William Chen [:wchen]
:
Mentors:
Depends on:
Blocks: app-data-jars 781011
  Show dependency treegraph
 
Reported: 2012-07-18 17:38 PDT by Mounir Lamouri (:mounir)
Modified: 2012-09-01 08:00 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI. (34.37 KB, patch)
2012-07-30 08:31 PDT, William Chen [:wchen]
doug.turner: review-
Details | Diff | Splinter Review
Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI. v2 (35.36 KB, patch)
2012-08-01 17:02 PDT, William Chen [:wchen]
doug.turner: review+
Details | Diff | Splinter Review
v1 diff v2 (3.82 KB, patch)
2012-08-01 17:03 PDT, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI. v3 (38.79 KB, patch)
2012-08-07 20:04 PDT, William Chen [:wchen]
doug.turner: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2012-07-18 17:38:45 PDT
This is using the permission manager to check for the permission and that should use a principal instead of an URI.

Doug, Jonas told me you wrote that code. Do you think you could change that?
Comment 1 Doug Turner (:dougt) 2012-07-18 20:28:29 PDT
I could, sure.  Won't happen this week though.  Feel free to reassign if you want me to look at it.  wchen could also take a look too.
Comment 2 Mounir Lamouri (:mounir) 2012-07-20 08:43:54 PDT
wchen, can you look at this? We need that to happen ASAP. It's a major blocker to switch the permission manager to be "app-aware".
Comment 3 William Chen [:wchen] 2012-07-20 09:01:10 PDT
Yeah, I'll take a look at this. It was bugging me that we were using URIs for permissions.
Comment 4 William Chen [:wchen] 2012-07-30 08:31:53 PDT
Created attachment 647186 [details] [diff] [review]
Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI.
Comment 5 Doug Turner (:dougt) 2012-07-30 12:15:04 PDT
Comment on attachment 647186 [details] [diff] [review]
Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI.

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

chris, can you also review?

::: browser/components/nsBrowserGlue.js
@@ +1560,4 @@
>          return;
>      }
>  
> +    var requestingPrincipal = request.principal;

Can a request here have a null principal?

::: dom/base/nsContentPermissionHelper.cpp
@@ +69,1 @@
>    if (mParent == nullptr)

style nit:

if (..) {
  stmt;
}

@@ +69,4 @@
>    if (mParent == nullptr)
>      return NS_ERROR_FAILURE;
>  
> +  NS_ADDREF(*aRequestingPrincipal = mParent->mPrincipal);

is mParent->mPrincipal always non-null?

::: dom/ipc/PermissionMessageUtils.cpp
@@ +18,5 @@
> +  }
> +
> +  bool isSerialized = false;
> +  nsCString principalString;
> +  nsCOMPtr<nsISerializable> serializable = do_QueryInterface(aParam.mPrincipal);

We should assert/die if we find a principal that doesn't serialize.

@@ +29,5 @@
> +  WriteParam(aMsg, isSerialized);
> +
> +  if (isSerialized) {
> +    WriteParam(aMsg, principalString);
> +  } else {

no need for else here.

if (isSerialized) {
  WriteParam(aMsg, principalString);
}

::: dom/ipc/PermissionMessageUtils.h
@@ +21,5 @@
> +  operator nsIPrincipal*() const { return mPrincipal.get(); }
> +
> +private:
> +  // Unimplemented
> +  Principal& operator=(Principal&);

are you worried about assignment?
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-08-01 13:53:44 PDT
Comment on attachment 647186 [details] [diff] [review]
Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI.


>diff -r 1cb91ed25dd0 -r 8abeec1f1787 b2g/components/ContentPermissionPrompt.js

>       "type": "permission-prompt",
>       "permission": request.type,
>       "id": requestId,
>-      "url": request.uri.spec
>+      "url": request.principal.URI.spec

This needs to be changed too.  Please file a followup on sending this
information to the system app too.

>diff -r 1cb91ed25dd0 -r 8abeec1f1787 dom/base/nsContentPermissionHelper.cpp

> nsresult
> nsContentPermissionRequestProxy::Init(const nsACString & type,
>-				                      mozilla::dom::ContentPermissionRequestParent* parent)
>+                                      mozilla::dom::ContentPermissionRequestParent* parent)

|using namespace mozilla::dom| and remove the qualification here.

>diff -r 1cb91ed25dd0 -r 8abeec1f1787 dom/devicestorage/nsDeviceStorage.cpp

I didn't look at this.

>diff -r 1cb91ed25dd0 -r 8abeec1f1787 dom/ipc/PBrowser.ipdl

>+    PContentPermissionRequest(nsCString aType, Principal principal);

We need to document these things.  Explain that this initiates an
asynchronous request asking for permission |aType| for the object
|principal|.  In the parent process, |principal| is "untrusted", and
requests are restricted to the principals that can live in the content
process.

>diff -r 1cb91ed25dd0 -r 8abeec1f1787 dom/ipc/PermissionMessageUtils.cpp

>+  nsCOMPtr<nsISerializable> serializable = do_QueryInterface(aParam.mPrincipal);
>+  if (serializable) {

If we try to serialize an unserializable URI, then the content process
will just end up being killed, but confusingly because it'll be
asynchronously killed after the parent realizes it didn't get a URI.

Let's just preemptively NS_RUNTIMEABORT() here.

>+  if (isSerialized) {

Won't need the if/else here.

>+bool
>+ParamTraits<Principal>::Read(const Message* aMsg, void** aIter, paramType* aResult)
>+{
>+  bool isNull;
>+  if (!ReadParam(aMsg, aIter, &isNull)) {

What's the meaning of a null URI in this context?  I don't know.

We don't need to resolve that here, since I think the old URI
serialization did this too, but please file a followup and CC bz /
sicking / jlebar / mounir to comment on this.

>+  bool isSerialized;

We won't need this logic now.

I'm not a good reviewer for the non-IPC parts.

Would like to see the next one.
Comment 7 William Chen [:wchen] 2012-08-01 17:01:09 PDT
(In reply to Doug Turner (:dougt) from comment #5)
> Comment on attachment 647186 [details] [diff] [review]
> Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI.
> 
> Review of attachment 647186 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> chris, can you also review?
> 
> ::: browser/components/nsBrowserGlue.js
> @@ +1560,4 @@
> >          return;
> >      }
> >  
> > +    var requestingPrincipal = request.principal;
> 
> Can a request here have a null principal?
A request should not have a null principal. I have looked through the callers and this should be the case.
> 
> ::: dom/base/nsContentPermissionHelper.cpp
> @@ +69,1 @@
> >    if (mParent == nullptr)
> 
> style nit:
> 
> if (..) {
>   stmt;
> }
> 
done.
> @@ +69,4 @@
> >    if (mParent == nullptr)
> >      return NS_ERROR_FAILURE;
> >  
> > +  NS_ADDREF(*aRequestingPrincipal = mParent->mPrincipal);
> 
> is mParent->mPrincipal always non-null?
>
Yes
> ::: dom/ipc/PermissionMessageUtils.cpp
> @@ +18,5 @@
> > +  }
> > +
> > +  bool isSerialized = false;
> > +  nsCString principalString;
> > +  nsCOMPtr<nsISerializable> serializable = do_QueryInterface(aParam.mPrincipal);
> 
> We should assert/die if we find a principal that doesn't serialize.
> 
done.
> @@ +29,5 @@
> > +  WriteParam(aMsg, isSerialized);
> > +
> > +  if (isSerialized) {
> > +    WriteParam(aMsg, principalString);
> > +  } else {
> 
> no need for else here.
> 
done
> if (isSerialized) {
>   WriteParam(aMsg, principalString);
> }
> 
> ::: dom/ipc/PermissionMessageUtils.h
> @@ +21,5 @@
> > +  operator nsIPrincipal*() const { return mPrincipal.get(); }
> > +
> > +private:
> > +  // Unimplemented
> > +  Principal& operator=(Principal&);
> 
> are you worried about assignment?
No, but the serialization wrapper for URI (IPC::URI) did this so I am guessing there is a reason. The class should only be used for IPC and I don't see a reason to allow users to do much with IPC::Principal.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> Comment on attachment 647186 [details] [diff] [review]
> Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI.
> 
> 
> >diff -r 1cb91ed25dd0 -r 8abeec1f1787 b2g/components/ContentPermissionPrompt.js
> 
> >       "type": "permission-prompt",
> >       "permission": request.type,
> >       "id": requestId,
> >-      "url": request.uri.spec
> >+      "url": request.principal.URI.spec
> 
> This needs to be changed too.  Please file a followup on sending this
> information to the system app too.
> 
ok
> >diff -r 1cb91ed25dd0 -r 8abeec1f1787 dom/base/nsContentPermissionHelper.cpp
> 
> > nsresult
> > nsContentPermissionRequestProxy::Init(const nsACString & type,
> >-				                      mozilla::dom::ContentPermissionRequestParent* parent)
> >+                                      mozilla::dom::ContentPermissionRequestParent* parent)
> 
> |using namespace mozilla::dom| and remove the qualification here.
> 
done
> >diff -r 1cb91ed25dd0 -r 8abeec1f1787 dom/devicestorage/nsDeviceStorage.cpp
> 
> I didn't look at this.
> 
> >diff -r 1cb91ed25dd0 -r 8abeec1f1787 dom/ipc/PBrowser.ipdl
> 
> >+    PContentPermissionRequest(nsCString aType, Principal principal);
> 
> We need to document these things.  Explain that this initiates an
> asynchronous request asking for permission |aType| for the object
> |principal|.  In the parent process, |principal| is "untrusted", and
> requests are restricted to the principals that can live in the content
> process.
> 
done
> >diff -r 1cb91ed25dd0 -r 8abeec1f1787 dom/ipc/PermissionMessageUtils.cpp
> 
> >+  nsCOMPtr<nsISerializable> serializable = do_QueryInterface(aParam.mPrincipal);
> >+  if (serializable) {
> 
> If we try to serialize an unserializable URI, then the content process
> will just end up being killed, but confusingly because it'll be
> asynchronously killed after the parent realizes it didn't get a URI.
> 
> Let's just preemptively NS_RUNTIMEABORT() here.
> 
done
> >+  if (isSerialized) {
> 
> Won't need the if/else here.
> 
done
> >+bool
> >+ParamTraits<Principal>::Read(const Message* aMsg, void** aIter, paramType* aResult)
> >+{
> >+  bool isNull;
> >+  if (!ReadParam(aMsg, aIter, &isNull)) {
> 
> What's the meaning of a null URI in this context?  I don't know.
> 
> We don't need to resolve that here, since I think the old URI
> serialization did this too, but please file a followup and CC bz /
> sicking / jlebar / mounir to comment on this.
> 
I think it is reasonable that we are able to serialize null pointers for the sake of completeness and leave it up to the consumers to decide what they want to do with a null pointer. I'm not sure I see the problem, but sure, I'll cc someone to comment.
> >+  bool isSerialized;
> 
> We won't need this logic now.
> 
done.
> I'm not a good reviewer for the non-IPC parts.
> 
> Would like to see the next one.
Comment 8 William Chen [:wchen] 2012-08-01 17:02:44 PDT
Created attachment 648149 [details] [diff] [review]
Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI. v2
Comment 9 William Chen [:wchen] 2012-08-01 17:03:25 PDT
Created attachment 648150 [details] [diff] [review]
v1 diff v2
Comment 10 Doug Turner (:dougt) 2012-08-07 00:07:55 PDT
What are the two follow up bugs (one for sending the request.principal information to the system app, serialize null uri pointers)?
Comment 11 Doug Turner (:dougt) 2012-08-07 00:09:44 PDT
Comment on attachment 648149 [details] [diff] [review]
Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI. v2

You'll have to rebase this patch as it doesn't apply cleanly. Please post a new patch.  Also, please send this new patch to try so that there are no surprises.
Comment 12 William Chen [:wchen] 2012-08-07 20:04:54 PDT
Created attachment 649940 [details] [diff] [review]
Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI. v3

Rebased. https://tbpl.mozilla.org/?tree=Try&rev=5efc48cfb7fd

I added one of the followup bugs (system app) to this one. The other follow up (uri serialization) is bug 781028.
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-08-08 18:24:24 PDT
https://hg.mozilla.org/mozilla-central/rev/37694e6f8df0
Comment 15 Doug Turner (:dougt) 2012-08-09 09:51:10 PDT
Comment on attachment 649940 [details] [diff] [review]
Modify nsIContentPermissionRequest to use nsIPrincipal instead of nsIURI. v3

this should have been r+'ed when I pushed.

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