Refactored the IdCard services list to fix new bugs and (hopefully) prevent even...

diff --git a/src/moonshot-id.vala b/src/moonshot-id.vala
index 45f3a83..ec0866c 100644 (file)
--- a/src/moonshot-id.vala
+++ b/src/moonshot-id.vala
@@ -29,6 +29,9 @@
  * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE.
 */
+
+using Gee;
+
 public class TrustAnchor : Object
 {
     public string ca_cert {get; set; default = "";}
@@ -64,6 +67,8 @@ public struct Rule
 
 public class IdCard : Object
 {
+    static MoonshotLogger logger = get_logger("IdCard");
+
     public const string NO_IDENTITY = "No Identity";
 
     private string _nai;
@@ -98,12 +103,60 @@ public class IdCard : Object
         internal set {_rules = value ?? new Rule[0] ;}
     }
 
-    private string[] _services = new string[0];
-    public string[] services {
-        get {return _services;}
-        internal set {_services = value ?? new string[0] ;}
+    private ArrayList<string> _services = new ArrayList<string>();
+
+    internal ArrayList<string> services {
+         get {return  _services;}
     }
 
+    // Returns the list of services as a string, using the given separator.
+    internal string get_services_string(string sep) {
+        if (_services.is_empty) {
+            return "";
+        }
+
+        // ArrayList.to_array() seems to be unreliable -- it causes segfaults 
+        // semi-randomly. (Possibly because it returns an unowned ref?)
+        // return string.joinv(sep, _services.to_array());
+        // 
+        // This problem may be related to the one noted elsewhere as the
+        // "Centos vala array property bug".
+
+        string[] svcs = new string[_services.size];
+        for (int i = 0; i < _services.size; i++) {
+            svcs[i] = _services[i];
+        }
+
+        return string.joinv(sep, svcs);
+    }
+
+    internal void update_services(string[] services) {
+        _services.clear();
+
+        // Doesn't exist in older versions of libgee:
+        // _services.add_all_array(services);
+
+        if (services != null) {
+            foreach (string s in services) {
+                _services.add(s);
+            }
+        }
+    } 
+
+    internal void update_services_from_list(ArrayList<string> services) {
+        if (services == this._services) {
+            // Don't try to update from self.
+            return;
+        }
+
+        _services.clear();
+
+        if (services != null) {
+            _services.add_all(services);
+        }
+    } 
+
+
     public bool temporary {get; set; default = false; }
 
     public TrustAnchor trust_anchor  { get; set; default = new TrustAnchor (); }
@@ -145,7 +198,7 @@ public class IdCard : Object
         if (CompareRules(this.rules, other.rules)!=0)
             diff |= 1 << DiffFlags.RULES;
 
-        if (CompareStringArray(this.services, other.services)!=0)
+        if (CompareStringArrayList(this._services, other._services)!=0)
             diff |= 1 << DiffFlags.SERVICES;
 
         if (this.trust_anchor.Compare(other.trust_anchor)!=0)
@@ -169,10 +222,6 @@ public class IdCard : Object
     internal void add_rule(Rule rule) {
         _rules += rule;
     }
-
-    internal void add_service(string service) {
-        _services += service;
-    }
 }
 
 public int CompareRules(Rule[] a, Rule[] b)
@@ -189,13 +238,13 @@ public int CompareRules(Rule[] a, Rule[] b)
     return 0;
 }
 
-public int CompareStringArray(string[] a, string [] b)
+public int CompareStringArrayList(ArrayList<string> a, ArrayList<string> b)
 {
-    if (a.length != b.length) {
+    if (a.size != b.size) {
         return 1;
     }
 
-    for (int i = 0; i < a.length; i++) {
+    for (int i = 0; i < a.size; i++) {
         if (a[i] != b[i]) {
             return 1;
         }

diff --git a/src/moonshot-idcard-widget.vala b/src/moonshot-idcard-widget.vala
index 887dbf6..7a34cf2 100644 (file)
--- a/src/moonshot-idcard-widget.vala
+++ b/src/moonshot-idcard-widget.vala
@@ -123,7 +123,7 @@ class IdCardWidget : Box
     {
         // !!TODO: Use a table to format the labels and values
         string services_text = "Services:  ";
-        string service_spacer = "                ";
+        string service_spacer = "\n                ";
 
         var label_text = Markup.printf_escaped("<big>%s</big>", this.id_card.display_name);
 
@@ -132,14 +132,7 @@ class IdCardWidget : Box
             label_text += "\nUsername:  " + id_card.username;
             label_text += "\nRealm:  " + id_card.issuer;
 
-            var sep = "";
-            for (int i = 0; i < id_card.services.length; i++)
-            {
-                services_text += sep;
-                services_text += id_card.services[i];
-
-                sep = "\n" + service_spacer;
-            }
+            services_text += this.id_card.get_services_string(service_spacer);
             label_text += "\n" + services_text;
         }
 

diff --git a/src/moonshot-identities-manager.vala b/src/moonshot-identities-manager.vala
index 1f390cc..8154052 100644 (file)
--- a/src/moonshot-identities-manager.vala
+++ b/src/moonshot-identities-manager.vala
@@ -182,8 +182,10 @@ public class IdentityManagerModel : Object {
     }
 
     public void add_card(IdCard card, bool force_flat_file_store) {
-        if (card.temporary)
+        if (card.temporary) {
+            logger.trace("add_card: card is temporary; returning.");
             return;
+        }
 
         string candidate;
         IIdentityCardStore.StoreType saved_store_type = get_store_type();
@@ -200,12 +202,18 @@ public class IdentityManagerModel : Object {
 
         if (!card.store_password)
             password_table.CachePassword(card, store);
+
+        logger.trace("add_card: Storing card '%s' with services: '%s'"
+                     .printf(card.display_name, card.get_services_string("; ")));
+
         store.add_card(card);
         set_store_type(saved_store_type);
         card_list_changed();
     }
 
     public IdCard update_card(IdCard card) {
+        logger.trace("update_card");
+
         IdCard retval;
         if (card.temporary) {
             retval = card;
@@ -261,7 +269,7 @@ public class IdentityManagerModel : Object {
             // The 'NoIdentity' card is non-trivial if it has services or rules.
             // All other cards are automatically non-trivial.
             if ((!card.is_no_identity()) || 
-                (card.services.length > 0) ||
+                (card.services.size > 0) ||
                 (card.rules.length > 0)) {
                 return true;
             }

diff --git a/src/moonshot-identity-dialog.vala b/src/moonshot-identity-dialog.vala
index be4914e..fab8f3d 100644 (file)
--- a/src/moonshot-identity-dialog.vala
+++ b/src/moonshot-identity-dialog.vala
@@ -29,6 +29,8 @@
  * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE.
 */
+
+using Gee;
 using Gtk;
 
 
@@ -87,7 +89,7 @@ class IdentityDialog : Dialog
         get { return remember_checkbutton.active; }
     }
 
-    internal string[] get_services()
+    internal ArrayList<string> get_services()
     {
         return card.services;
     }
@@ -300,7 +302,7 @@ class IdentityDialog : Dialog
         remove_button.set_sensitive(false);
 
 
-        var services_table = new Table(card.services.length, 1, false);
+        var services_table = new Table(card.services.size, 1, false);
         services_table.set_row_spacings(1);
         services_table.set_col_spacings(0);
         services_table.modify_bg(StateType.NORMAL, white);
@@ -383,20 +385,7 @@ class IdentityDialog : Dialog
                 if (result)
                 {
                     if (card != null) {
-                        SList<string> services = new SList<string>();
-
-                        foreach (string srv in card.services)
-                        {
-                            if (srv != selected_item.label)
-                                services.append(srv);
-                        }
-
-                        card.services = new string[services.length()];
-                        for (int j = 0; j < card.services.length; j++)
-                        {
-                            card.services[j] = services.nth_data(j);
-                        }
-
+                        card.services.remove(selected_item.label);
                         services_table.remove(selected_item.parent);
                         selected_item = null;
                         remove_button.set_sensitive(false);

diff --git a/src/moonshot-identity-management-view.vala b/src/moonshot-identity-management-view.vala
index 35dcd5f..cea963b 100644 (file)
--- a/src/moonshot-identity-management-view.vala
+++ b/src/moonshot-identity-management-view.vala
@@ -151,7 +151,7 @@ public class IdentityManagerView : Window {
                     return true;
             }
             
-            if (id_card.services.length > 0)
+            if (id_card.services.size > 0)
             {
                 foreach (string service in id_card.services)
                 {
@@ -226,6 +226,7 @@ public class IdentityManagerView : Window {
         }
 
         foreach (IdCard id_card in card_list) {
+            logger.trace(@"load_id_cards: Adding card with display name '$(id_card.display_name)'");
             add_id_card_data(id_card);
             IdCardWidget id_card_widget = add_id_card_widget(id_card);
             if (id_card_widget.id_card.nai == current_idcard_nai) {
@@ -242,7 +243,8 @@ public class IdentityManagerView : Window {
         id_card.username = dialog.username;
         id_card.password = dialog.password;
         id_card.store_password = dialog.store_password;
-        id_card.services = dialog.get_services();
+
+        id_card.update_services_from_list(dialog.get_services());
 
         return id_card;
     }
@@ -322,7 +324,8 @@ public class IdentityManagerView : Window {
         #else
         Gtk.MessageDialog dialog;
         IdCard? prev_id = identities_manager.find_id_card(id_card.nai, force_flat_file_store);
-        logger.trace("add_identity: find_id_card returned " + (prev_id != null ? "non-null" : "null"));
+        logger.trace("add_identity(flat=%s, card='%s'): find_id_card returned %s"
+                     .printf(force_flat_file_store.to_string(), id_card.display_name, (prev_id != null ? "non-null" : "null")));
         if (prev_id!=null) {
             int flags = prev_id.Compare(id_card);
             logger.trace("add_identity: compare returned " + flags.to_string());

diff --git a/src/moonshot-identity-manager-app.vala b/src/moonshot-identity-manager-app.vala
index 4ac6354..39d84f5 100644 (file)
--- a/src/moonshot-identity-manager-app.vala
+++ b/src/moonshot-identity-manager-app.vala
@@ -125,7 +125,13 @@ public class IdentityManagerApp {
     }
 
     public bool add_identity(IdCard id, bool force_flat_file_store) {
-        if (view != null) return view.add_identity(id, force_flat_file_store);
+        if (view != null) 
+        {
+            logger.trace("add_identity: calling view.add_identity");
+            return view.add_identity(id, force_flat_file_store);
+        }
+
+        logger.trace("add_identity: calling model.add_card");
         model.add_card(id, force_flat_file_store);
         return true;
     }
@@ -158,13 +164,8 @@ public class IdentityManagerApp {
                 /* If any service matches we add id card to the candidate list */
                 if (has_srv)
                 {
-                    foreach (string srv in id.services)
-                    {
-                        if (request.service == srv)
-                        {
-                            request.candidates.append(id);
-                            continue;
-                        }
+                    if (id.services.contains(request.service)) {
+                        request.candidates.append(id);
                     }
                 }
             }
@@ -174,37 +175,7 @@ public class IdentityManagerApp {
             {
                 foreach (IdCard id in request.candidates)
                 {
-                    int i = 0;
-                    SList<string> services_list = null;
-                    bool has_service = false;
-
-                    foreach (string srv in id.services)
-                    {
-                        if (srv == request.service)
-                        {
-                            has_service = true;
-                            continue;
-                        }
-                        services_list.append(srv);
-                    }
-                    
-                    if (!has_service)
-                        continue;
-
-                    if (services_list.length() == 0)
-                    {
-                        id.services = {};
-                        continue;
-                    }
-
-                    string[] services = new string[services_list.length()];
-                    foreach (string srv in services_list)
-                    {
-                        services[i] = srv;
-                        i++;
-                    }
-
-                    id.services = services;
+                    id.services.remove(request.service);
                 }
             }
 

diff --git a/src/moonshot-identity-request.vala b/src/moonshot-identity-request.vala
index 2e1c132..2e7ceed 100644 (file)
--- a/src/moonshot-identity-request.vala
+++ b/src/moonshot-identity-request.vala
@@ -88,17 +88,13 @@ public class IdentityRequest : Object {
         /* update id_card service list */
         if (update_card && id_card != null && this.service != null && this.service != "")
         {
-            bool duplicate_service = false;
-
-            foreach (string service in id_card.services)
-            {
-                if (service == this.service)
-                    duplicate_service = true;
-            }
+            bool duplicate_service = id_card.services.contains(this.service);
             logger.trace("return_identity: duplicate_service=" + duplicate_service.to_string());
             if (duplicate_service == false)
             {
-                id_card.add_service(this.service);
+                logger.trace("return_identity: calling add_service");
+                id_card.services.add(this.service);
+                logger.trace("return_identity: back from add_service");
 
                 this.id_card = this.parent_app.model.update_card(id_card);
             }

diff --git a/src/moonshot-keyring-store.vala b/src/moonshot-keyring-store.vala
index 6b36022..9045c95 100644 (file)
--- a/src/moonshot-keyring-store.vala
+++ b/src/moonshot-keyring-store.vala
@@ -33,19 +33,27 @@ using Gee;
 
 #if GNOME_KEYRING
 public class KeyringStore : Object, IIdentityCardStore {
+    static MoonshotLogger logger = get_logger("KeyringStore");
+
     private LinkedList<IdCard> id_card_list;
     private const string keyring_store_attribute = "Moonshot";
     private const string keyring_store_version = "1.0";
     private const GnomeKeyring.ItemType item_type = GnomeKeyring.ItemType.GENERIC_SECRET;
 
     public void add_card(IdCard card) {
+        logger.trace("add_card: Adding card '%s' with services: '%s'"
+                     .printf(card.display_name, card.get_services_string("; ")));
+
         id_card_list.add(card);
         store_id_cards();
     }
 
     public IdCard? update_card(IdCard card) {
+        logger.trace("update_card");
+
         id_card_list.remove(card);
         id_card_list.add(card);
+
         store_id_cards();
         foreach (IdCard idcard in id_card_list) {
             if (idcard.display_name == card.display_name) {
@@ -107,7 +115,7 @@ public class KeyringStore : Object, IIdentityCardStore {
                 } else if (attribute.name == "DisplayName") {
                     id_card.display_name = value;
                 } else if (attribute.name == "Services") {
-                    id_card.services = value.split(";");
+                    id_card.update_services(value.split(";"));
                 } else if (attribute.name == "Rules-Pattern") {
                     rules_patterns_index = i;
                 } else if (attribute.name == "Rules-AlwaysConfirm") {
@@ -153,11 +161,11 @@ public class KeyringStore : Object, IIdentityCardStore {
     }
 
     public void store_id_cards() {
+        logger.trace("store_id_cards");
         clear_keyring();
         foreach (IdCard id_card in this.id_card_list) {
             /* workaround for Centos vala array property bug: use temp array */
             var rules = id_card.rules;
-            var services_array = id_card.services;
             string[] rules_patterns = new string[rules.length];
             string[] rules_always_conf = new string[rules.length];
             
@@ -167,7 +175,7 @@ public class KeyringStore : Object, IIdentityCardStore {
             }
             string patterns = string.joinv(";", rules_patterns);
             string always_conf = string.joinv(";", rules_always_conf);
-            string services = string.joinv(";", services_array);
+            string services = id_card.get_services_string(";");
             GnomeKeyring.AttributeList attributes = new GnomeKeyring.AttributeList();
             uint32 item_id;
             attributes.append_string(keyring_store_attribute, keyring_store_version);

diff --git a/src/moonshot-local-flat-file-store.vala b/src/moonshot-local-flat-file-store.vala
index 8281d9d..c129072 100644 (file)
--- a/src/moonshot-local-flat-file-store.vala
+++ b/src/moonshot-local-flat-file-store.vala
@@ -88,7 +88,7 @@ public class LocalFlatFileStore : Object, IIdentityCardStore {
                 id_card.issuer = key_file.get_string(identity, "Issuer");
                 id_card.username = key_file.get_string(identity, "Username");
                 id_card.password = key_file.get_string(identity, "Password");
-                id_card.services = key_file.get_string_list(identity, "Services");
+                id_card.update_services(key_file.get_string_list(identity, "Services"));
                 id_card.display_name = key_file.get_string(identity, "DisplayName");
                 if (key_file.has_key(identity, "StorePassword")) {
                     id_card.store_password = (key_file.get_string(identity, "StorePassword") == "yes");
@@ -140,7 +140,6 @@ public class LocalFlatFileStore : Object, IIdentityCardStore {
         foreach (IdCard id_card in this.id_card_list) {
             /* workaround for Centos vala array property bug: use temp arrays */
             var rules = id_card.rules;
-            var services = id_card.services;
             string[] empty = {};
             string[] rules_patterns = new string[rules.length];
             string[] rules_always_conf = new string[rules.length];
@@ -157,7 +156,15 @@ public class LocalFlatFileStore : Object, IIdentityCardStore {
                 key_file.set_string(id_card.display_name, "Password", id_card.password);
             else
                 key_file.set_string(id_card.display_name, "Password", "");
-            key_file.set_string_list(id_card.display_name, "Services", services ?? empty);
+
+            // Using id_card.services.to_array() seems to cause a crash, possibly due to
+            // an unowned reference to the array.
+            string[] svcs = new string[id_card.services.size];
+            for (int i = 0; i < id_card.services.size; i++) {
+                svcs[i] = id_card.services[i];
+            }
+
+            key_file.set_string_list(id_card.display_name, "Services", svcs);
 
             if (rules.length > 0) {
                 key_file.set_string_list(id_card.display_name, "Rules-Patterns", rules_patterns);

diff --git a/src/moonshot-provisioning-common.vala b/src/moonshot-provisioning-common.vala
index 10e2366..87d7630 100644 (file)
--- a/src/moonshot-provisioning-common.vala
+++ b/src/moonshot-provisioning-common.vala
@@ -169,7 +169,7 @@ namespace WebProvisioning
                 }
                 else if (stack.nth_data(0) == "service")
                 {
-                    card.add_service(text);
+                    card.services.add(text);
                 }
 
                 /* Rules */

diff --git a/src/moonshot-server.vala b/src/moonshot-server.vala
index b1cc9ba..fb68864 100644 (file)
--- a/src/moonshot-server.vala
+++ b/src/moonshot-server.vala
@@ -197,12 +197,15 @@ public class MoonshotServer : Object {
         if ((password != null) && (password != ""))
             idcard.store_password = true;
         idcard.issuer = realm;
-        idcard.services = services;
+        idcard.update_services(services);
         idcard.trust_anchor.ca_cert = ca_cert;
         idcard.trust_anchor.subject = subject;
         idcard.trust_anchor.subject_alt = subject_alt;
         idcard.trust_anchor.server_cert = server_cert;
 
+        logger.trace("install_id_card: Card '%s' has services: '%s'"
+                     .printf(idcard.display_name, idcard.get_services_string("; ")));
+
         if (rules_patterns.length == rules_always_confirm.length)
         {
             /* workaround Centos vala array property bug: use temp array */
@@ -245,13 +248,25 @@ public class MoonshotServer : Object {
                 }
             } 
 
+
+            // prevent a crash by holding the reference to otherwise
+            // unowned array(?)
+
+            // string[] svcs = card.services.to_array();
+            // string[] svcs = card.services.to_array()[:];
+            string[] svcs = new string[card.services.size];
+            for (int i = 0; i < card.services.size; i++) {
+                svcs[i] = card.services[i];
+            }
+
+            logger.trace(@"install_from_file: Adding card with display name '$(card.display_name)'");
             result = install_id_card(card.display_name,
                                      card.username,
                                      card.password,
                                      card.issuer,
                                      rules_patterns,
                                      rules_always_confirm,
-                                     card.services,
+                                     svcs,
                                      card.trust_anchor.ca_cert,
                                      card.trust_anchor.subject,
                                      card.trust_anchor.subject_alt,

diff --git a/src/moonshot-webp-parser.vala b/src/moonshot-webp-parser.vala
index 90b5452..46ba9c6 100644 (file)
--- a/src/moonshot-webp-parser.vala
+++ b/src/moonshot-webp-parser.vala
@@ -90,7 +90,11 @@ namespace WebProvisioning
         
             /* use temp arrays to workaround centos array property bug */
             var rules = card.rules;
-            var services = card.services;
+            string[] svcs = new string[card.services.size];
+            for (int i = 0; i < card.services.size; i++) {
+                svcs[i] = card.services[i];
+            }
+
             if (rules.length > 0)
             {
                 int i = 0;
@@ -111,7 +115,7 @@ namespace WebProvisioning
                                      card.issuer,
                                      rules_patterns,
                                      rules_always_confirm,
-                                     services,
+                                     svcs,
                                      card.trust_anchor.ca_cert,
                                      card.trust_anchor.subject,
                                      card.trust_anchor.subject_alt,