744212 - Add blob (or text) field to permissions in permissions manager

Status: RESOLVED WONTFIX

(Core :: Networking, enhancement)

Description

[Camilo Viecco (:cviecco)]

For some type of permissions such as certificate pinning (Bug 744204 (https://bugzilla.mozilla.org/show_bug.cgi?id=744204)) it would be useful to store a blog (or string) to keep the values of the pinned keys.

We should extend the permissions manager interface to support an optional string to the back-end nsPermissionManager.{cpp,h}.


This is similar to (https://bugzilla.mozilla.org/show_bug.cgi?id=519263)

Comment 1

[Camilo Viecco (:cviecco)]

So I started working on the patch, and as first step I modified the IDLs for both permissionmanager and permissions. I found one issue that I do not like:

I changed the IDL for add (in permission manager) to have an optional parameter: dataString, but this makes a C++ signuature with a required dataString parameter. So I am wondering how can I two Add declared so that the C++ can call either call (polymorphic idl?).  Or am I just doing all this thing wrong?

Comment 2

[Camilo Viecco (:cviecco)]

Attachment: option 2 : add a new method in the idl.

An other option would be to add a new method to the IDL as this patch does.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Camilo Viecco from comment #1)
> I changed the IDL for add (in permission manager) to have an optional
> parameter: dataString, but this makes a C++ signuature with a required
> dataString parameter. So I am wondering how can I two Add declared so that
> the C++ can call either call (polymorphic idl?).  Or am I just doing all
> this thing wrong?

In C++ there are no optional arguments. There are only arguments with default values. And, in C++ only primitive-typed arguments (int, char, etc.) can have default values. And, that is purely a syntactic construct.

Generally, we maintain backward compatibility for JS extensions as much as possible, but we require native extensions to be recompiled for every release. I don't know if/when that will change, but AFAICT it is OK to use your first patch that adds the optional argument.

Regardless of which style of change you make, whenever you change an interface, you must generate a new UUID for it.

Comment 4

[Camilo Viecco (:cviecco)]

Attachment: common-patch-no-matter-implementation

Comment 5

[Camilo Viecco (:cviecco)]

Attachment: with-new-method-part1 (just idl stuff)

assumes common patch is already in place

Comment 6

[Camilo Viecco (:cviecco)]

Attachment: with-new-method-part2 (implementation)

This is the actual implementaiton of the idl with new method idea.

Comment 7

[Camilo Viecco (:cviecco)]

Attachment: with-new-method-part1 (just idl stuff)

Just IDL declarations

Comment 8

[Ian Melven :imelven]

(In reply to Camilo Viecco from comment #6)
> Created attachment 623187 [details]
> with-new-method-part2 (implementation)
> 
> This is the actual implementaiton of the idl with new method idea.

@@ -300,17 +300,27 @@ nsPermissionManager::InitDB(bool aRemove

+        rv = mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING,
+              "ALTER TABLE moz_hosts ADD dataString TEXT"));

does this set a default value for the new field ? if not you might want to set "" for this new field.

+    //cviecco -> I am wondering of the copying of this local val...
+    //  who frees the copy? (hopefully someone else!).
+    const nsCString localval(aDataString);
     IPC::Permission permission((aHost),
                                (aType),
-                               aPermission, aExpireType, aExpireTime);
+                               aPermission, aExpireType, aExpireTime,localval);

as i understand it, this gets assigned to the mDataString member of the permission object, that copy (which is the one you care about probably) should get freed when the permission object is destructed. 

otherwise, my only comment is that dataString is a pretty non-descript name - i suppose this could end up being used for a few things so you are trying to keep it generic... 

there's a few typos and whitespace issues, but the overall idea in the attached patches looks ok to me, fwiw :)

Comment 9

[Camilo Viecco (:cviecco)]

Attachment: Implementaiton (forgot the a section in test-permission)

Comment 10

[Gervase Markham [:gerv]]

cviecco: he may jump in to prove me wrong, but I think dwitte is no longer working on Mozilla stuff very much...

Gerv

Comment 11

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 623184 [details] [diff] [review]
common-patch-no-matter-implementation

You're right gerv.  We had a chat with mconnor about this instead... and we may not actually fix this bug.

Comment 12

[Camilo Viecco (:cviecco)]

Storage will no be done this way.