After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 659075 - IndividualAggregator shouldn't set the trust level for PersonaStores
IndividualAggregator shouldn't set the trust level for PersonaStores
Status: RESOLVED FIXED
Product: folks
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: folks-0.6.3
Assigned To: folks-maint
folks-maint
Depends on:
Blocks: 657141 659040
 
 
Reported: 2011-09-14 16:49 UTC by Raul Gutierrez Segales
Modified: 2011-09-16 19:45 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch (4.57 KB, patch)
2011-09-15 13:18 UTC, Raul Gutierrez Segales
needs-work Details | Review
patch (5.65 KB, patch)
2011-09-16 09:32 UTC, Raul Gutierrez Segales
accepted-commit_now Details | Review

Description Raul Gutierrez Segales 2011-09-14 16:49:06 UTC
PersonaStore.trust-level is changed to be determined entirely by
   the store.

   This is needed so that we link personas from different stores by
   their linkable properties, which (correctly) requires that we fully
   trust those stores. It doesn't make sense, conceptually, to tie
   trust into our choice of primary store — instead it should be
   determined by how much control the user has over the store, which
   the store itself is in a better position to determine than the
   IndividualAggregator.
Comment 1 Raul Gutierrez Segales 2011-09-14 16:52:07 UTC
To follow-up on the description (which was posted accidentally - sorry), this is a follow up of Philip's email about tidying up writeability and primary persona stores.
Comment 2 Raul Gutierrez Segales 2011-09-15 13:18:03 UTC
Created attachment 196625 [details] [review]
patch
Comment 3 Philip Withnall 2011-09-15 17:19:01 UTC
Review of attachment 196625 [details] [review]:

The Tracker backend also needs its trust level setting (to FULL, I think).

::: backends/eds/lib/edsf-persona-store.vala
@@ +798,3 @@
           if (do_initial_query == false && this._is_quiescent == false)
             {
+              this.trust_level = PersonaStoreTrust.PARTIAL;

What's the reason behind adding this one? If it's to make LDAP stores only partially-trusted, I think we need to more explicitly check that the address book's actually coming from LDAP (e.g. by checking the type of the address book or something similar).

If not, it needs a comment.

@@ +1769,3 @@
+
+  private void _update_trust_level ()
+    {

Stick a comment in here along the lines of:

This isn't perfect, since we want to base our trust of the address book on whether *other people* can write to it (and potentially maliciously affect the linking our aggregator does). However, since we can't know that, we assume that if we can write to the address book we're probably in full control of it. If we can't, either nobody/a sysadmin is (e.g. LDAP) or somebody else (who we can't trust) is (e.g. a read-only view of someone else's WebDAV address book).
Comment 4 Raul Gutierrez Segales 2011-09-16 09:32:13 UTC
Created attachment 196702 [details] [review]
patch

Updated version.
Comment 5 Philip Withnall 2011-09-16 19:19:39 UTC
Review of attachment 196702 [details] [review]:

A couple of small issues to fix, then please commit!

::: backends/eds/lib/edsf-persona-store.vala
@@ +1777,3 @@
+  private void _update_trust_level ()
+    {
+      unowned SourceGroup? group = (SourceGroup) this._source.peek_group ();

Should the case be to (SourceGroup?) instead?

@@ +1781,3 @@
+        {
+          var base_uri = group.peek_base_uri ();
+          if (Regex.match_simple ("^ldap", base_uri))

Might it be faster to do base_uri.has_prefix("ldap")?

Would be useful to have a comment showing an example of the kind of base URI we expect to see/match.
Comment 6 Raul Gutierrez Segales 2011-09-16 19:45:02 UTC
Merged:

commit 44f9a8316082e5f86ac96e5018f11713767ecb9d
Author: Raul Gutierrez Segales <rgs@collabora.co.uk>
Date:   Wed Sep 14 20:48:47 2011 +0100

    Let each PersonaStore manage its trust-level
    
    Previously, the IndividualAggregator was in charge of
    downgrading and upgrading the trust-level of each
    PersonaStore. From now on, we let each Store manage its
    trust-level since it's the natural way to do it.
    
    Fixes: https://bugzilla.gnome.org/show_bug.cgi?id=659075