Closed Bug 744212 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Networking, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: cviecco, Assigned: cviecco)

Details

Attachments

(3 files, 3 obsolete files)

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)
Blocks: 744204
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?
An other option would be to add a new method to the IDL as this patch does.
(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.
Attachment #623184 - Flags: feedback?(dwitte)
assumes common patch is already in place
Attachment #621706 - Attachment is obsolete: true
Attachment #623186 - Flags: feedback?(dwitte)
Attached file with-new-method-part2 (implementation) (obsolete) —
This is the actual implementaiton of the idl with new method idea.
Attachment #623187 - Flags: feedback?(dwitte)
Just IDL declarations
Attachment #623186 - Attachment is obsolete: true
Attachment #623186 - Flags: feedback?(dwitte)
Attachment #623189 - Flags: feedback?(dwitte)
(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 :)
Attachment #623187 - Attachment is obsolete: true
Attachment #623187 - Flags: feedback?(dwitte)
Attachment #631914 - Flags: feedback?(dwitte)
cviecco: he may jump in to prove me wrong, but I think dwitte is no longer working on Mozilla stuff very much...

Gerv
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.
Attachment #623184 - Flags: feedback?(dwitte)
Attachment #623189 - Flags: feedback?(dwitte)
Attachment #631914 - Flags: feedback?(dwitte)
No longer blocks: 744204
Storage will no be done this way.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.