333894 - flesh out details interfaces for managing black/white lists

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect)

Description

[Tony Chang (Google)]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

Safe browsing includes interfaces for managing tables of urls (nsIProtectionTable) and for managing multiple tables (nsIProtectionListManager).  We want to have an interface that allows for reuse in other areas, e.g., thunderbird.

Reproducible: Always

Comment 1

[Tony Chang (Google)]

The interfaces will be in toolkit/components/protection/public (https://bugzilla.mozilla.org/show_bug.cgi?id=333062).

Feedback from mscott via email:
> #include "nsISupports.idl"                                                                  
>                                                                                             
> // A map that contains a string keys mapped to string values.                               
> // TODO: maybe nsISerializeable to handle reading/writing from disk?                        
>                                                                                             
> [scriptable, uuid(fd1f8334-1859-472d-b01f-4ac6b1121ce4)]                                    
> interface nsIProtectionTable : nsISupports                                                  
> {                                                                                           
>     readonly attribute long size;                                                           
>                                                                                             
is this the number of entries in the protection table? maybe we should call it count instead of size                                                                               

>     // Set to false if we don't want to update this table.                                  
>     attribute boolean needsUpdate;                                                          
>                                                                                             
>     // In the simple case, find just looks up the string in the                             
>     // table.  However, it could do something more complex (e.g.,                           
>     // canonicalize the url).                                                               
>     string find(in string key);                                                             
>                                                                                             
find just does the hash table lookup for the url key right? should we return a boolean instead of a string then? Maybe: boolean exists (in string key).                              

>     // In the simple case, key is a url and value is 1.  However,                           
>     // value could be more complicated (e.g., hashed value based                            
>     // on site domain).                                                                     
>     void insert(in string key, in string value);                                            
>                                                                                             
>     boolean erase(in string key);                                                           
>                                                                                             
what does erase do? If it removes the key from the protection table, should we call it remove instead? What is the boolean for, whether we found it in the table?

> };                                                                                          
>                                                                                             
> ===                                                                                         
>                                                                                             
> #include "nsISupports.idl"                                                                  
>                                                                                             
> // Manages updates for all tables.                                                          
>                                                                                             
> interface nsIFile;                                                                          
> interface nsIProtectionTable;                                                               
>                                                                                             
> [scriptable, uuid(e1a80418-1bf9-4bd7-a40d-94d549c24955)]                                    
> interface nsIProtectionListManager : nsISupports                                            
> {                                                                                           
>     // directory to save tables in                                                          
>     void setAppDir(in nsIFile appDir);                                                      
>                                                                                             
>     // Add a table to the list of tables we are managing.                                   
>     boolean registerTable(in nsIProtectionTable table);                                     
>                                                                                             
what is the boolean value used for in registerTable                                           

>     // For each table that is enabled, check for updates during                             
>     // during the scheduled interval.                                                       
>     void startUpdateChecker();                                                              
>                                                                                             
>     // Stop checking for all updates.                                                       
>     void stopUpdateChecker();                                                               
> };                                                                                          
>                                                                                             
> ===                                                                                         
>
> #include "nsISupports.idl"                                                                  
>                                                                                             
> // Manages a set of blacklists and a set whitelists.  For example, the                      
> // PhishingWarden would have an instance of this.                                           
>                                                                                             
> interface nsIProtectionTable;                                                               
> interface nsIProtectionListManager;                                                         
>                                                                                             
> [scriptable, uuid(da2d4893-3cf2-4162-b981-2efe9605aee5)]                                    
> interface nsIProtectionListWarden : nsISupports                                             
> {                                                                                           
>     void setListManager(in nsIProtectionListManager listManager);                           
>                                                                                             
>     void registerBlackTable(in nsIProtectionTable table);                                   
>     void registerWhiteTable(in nsIProtectionTable table);                                   
>                                                                                             
>     // Notify the listmanager that we want to update these tables.                          
>     void enableTableUpdates();                                                              
>                                                                                             
>     // Notify the listmanager that we no longer want to update these                        
>     // tables.                                                                              
>     void disableTableUpdates();                                                             
>                                                                                             
>     // If enhanced protection is on, check the url against a remote                         
>     // server,                                                                              
>     // ow, check against local lists.                                                       
>     boolean checkUrl(in string url);                                                        
>                                                                                             
>     // Should these be exposed?                                                             
>     //bool checkUrlLocal(in string url);                                                    
>     //bool checkUrlRemote(in string url);                                                   
> };                                                                                          
>                                                                                             
Most of my comments about method names came from my assumption that the protection table is a hashtable so I suggested using names/signatures that looked liked existing mozilla hashtable interfaces (nsHashtable.h), but that might not be appropriate, just suggestions from my end.

Comment 2

[Tony Chang (Google)]

Attachment: rename methods as suggested

This patch includes the changes suggested by mscott.

Comment 3

[Tony Chang (Google)]

Comment on attachment 218746 [details] [diff] [review]
rename methods as suggested

I'll recreate the patch after the directory rename (bug 335032),

Comment 4

[Tony Chang (Google)]

Attachment: rename methods as suggested

This patch includes the changes suggested by mscott.  Specifically, it changes the method names in the interface and it changes the string parameters to nsACString.

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 220034 [details] [diff] [review]
rename methods as suggested

r=[e-mail]

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 220034 [details] [diff] [review]
rename methods as suggested

a=[e-mail]

Comment 7

[Brett Wilson]

Fixed on branch and trunk.