6.0.0-git
2021-05-07

[#14197] Nag_Driver_Kolab stores parent_id strings base64 encoded (should be: unencoded)
Summary Nag_Driver_Kolab stores parent_id strings base64 encoded (should be: unencoded)
Queue Kolab
Type Bug
State Resolved
Priority 1. Low
Owners mrubinsk (at) horde (dot) org
Requester mike.gabriel (at) das-netzwerkteam (dot) de
Created 2015-12-15 (1970 days ago)
Due
Updated 2015-12-29 (1956 days ago)
Assigned 2015-12-29 (1956 days ago)
Resolved 2015-12-29 (1956 days ago)
Milestone
Patch No

History
2015-12-29 17:45:19 mike (dot) gabriel (at) das-netzwerkteam (dot) de Comment #13 Reply to this comment
Either way, we can't use the internal ID of the task as the parent 
property. It MUST be the UID. If the regex isn't enough to catch the 
incorrectly set properties, maybe we can perform a look up of the 
UID to ensure it exists? That would be alot more expensive, so if we 
do need to do that, maybe we'd have to move this to a login task 
after all...
Looking up the UID is not needed and not useful, I guess.

Please note that when looking at all tasks (complete and 
non-completed), parenting of tasks should always work (unless a parent 
task got removed).

But when looking at e.g. completed tasks, the parenting is broken 
anyway, because often sub-tasks are marked as completed whereas parent 
tasks are still set to not-completed.

If the parent UID lacks from the list of tasks to be shown, the 
sub-task is simply shown at top level. I would not put too much effort 
into solving this.

/me wonders how other groupware / tasklist tools handle that 
(parenting of completed and non-completed) tasks.
2015-12-29 16:35:18 Jan Schneider Comment #12 Reply to this comment

[Show Quoted Text - 18 lines]
I think there was some duplication of ID usage which was kind of 
normalized to UIDs across the code. It's well possible that 
accidentally the task parent IDs got somehow disconnected along the way.
2015-12-29 15:27:39 Michael Rubinsky Comment #11 Reply to this comment
The format of the Kolab UID's is not fixed and can contain ":" and
other funky characters.
I remember vaguely that's one of the reasons Jan changed the internal IDs
when we reported that several backends were broken when using a Kolab driver
+ "special" UIDs that needed proper URI-encoding.
Wouldn't/shouldn't the xml parser/handler take care of the normal 
escaping for these characters, like it does for other string fields?

Either way, we can't use the internal ID of the task as the parent 
property. It MUST be the UID. If the regex isn't enough to catch the 
incorrectly set properties, maybe we can perform a look up of the UID 
to ensure it exists? That would be alot more expensive, so if we do 
need to do that, maybe we'd have to move this to a login task after 
all...
@Jan: Do you remember anything about this?
2015-12-29 14:53:31 Thomas Jarosch Comment #10 Reply to this comment
Regarding this new detection logic in nag/lib/Driver/Kolab.php

  +        /* Check if the decoded string resembles a uid hash...
+         */
+        if (!preg_match('/^[a-zA-Z0-9-]+$/', $decoded)) {
+            return false;
+        }


The format of the Kolab UID's is not fixed and can contain ":" and
other funky characters.

I remember vaguely that's one of the reasons Jan changed the internal IDs
when we reported that several backends were broken when using a Kolab driver
+ "special" UIDs that needed proper URI-encoding.

@Jan: Do you remember anything about this?

2015-12-29 05:52:49 Michael Rubinsky State ⇒ Resolved
 
2015-12-29 05:52:19 Git Commit Comment #9 Reply to this comment
Changes have been made in Git (master):

commit 74c5fe33e4b0ad36ed2564ac2cf98d2715565e18
Author: Mike Gabriel <mike.gabriel@das-netzwerkteam.de>
Date:   Tue Dec 29 00:32:16 2015 -0500

     Convert parent field to the parent's UID and not the parent's id 
when using Kolab.

     See Bug #14197. Kolab expects the parent field to be the parent's UID,
     and not some arbitrary internal-to-horde id.

     Signed-off-by: Michael J Rubinsky <mrubinsk@horde.org>

  nag/lib/Driver/Kolab.php |   56 ++++++++++++++++++++++++++++++++++++++++++++-
  1 files changed, 54 insertions(+), 2 deletions(-)

http://github.com/horde/horde/commit/74c5fe33e4b0ad36ed2564ac2cf98d2715565e18
2015-12-29 05:49:54 Git Commit Comment #8 Reply to this comment
Changes have been made in Git (FRAMEWORK_5_2):

commit 93d3d84a7cc7020d46b4acefa0fa57bad8b30236
Author: Mike Gabriel <mike.gabriel@das-netzwerkteam.de>
Date:   Tue Dec 29 00:32:16 2015 -0500

     Convert parent field to the parent's UID and not the parent's id 
when using Kolab.

     See Bug #14197. Kolab expects the parent field to be the parent's UID,
     and not some arbitrary internal-to-horde id.

     Signed-off-by: Michael J Rubinsky <mrubinsk@horde.org>

  nag/lib/Driver/Kolab.php |   56 ++++++++++++++++++++++++++++++++++++++++++++-
  1 files changed, 54 insertions(+), 2 deletions(-)

http://github.com/horde/horde/commit/93d3d84a7cc7020d46b4acefa0fa57bad8b30236
2015-12-29 05:29:08 Michael Rubinsky Comment #7
Assigned to Michael Rubinsky
Taken from Michael J Rubinsky <mrubinsk@horde.org>
Taken from Thomas Jarosch
State ⇒ Assigned
Reply to this comment
Yup. This regression was introduced back in 2012 by:

https://github.com/horde/horde/commit/54173622945ca87ee48905bceb8b436c2e71d0af 
and is the result of Nag using internal ids as parent properties and 
not uids.

The patch looks good. Generally I would argue that code to perform the 
correction of the broken user data should be done in a login task. 
However, since it's conceivable that multiple Horde installs could be 
working against the same Kolab backend this is probably the safest way 
to handle it - with the caveat that the workaround will be removed in 
Horde 6.
2015-12-17 15:41:10 Michael Rubinsky Comment #6
Assigned to Michael J Rubinsky <mrubinsk@horde.org>
Assigned to Thomas Jarosch
Reply to this comment
The patch also applies cleanly to the master branch using this command:

patch -p1 < PATCHFILE
Yes. I was just verifying where you are seeing all these errors since 
the ticket was created under the master branch.
Any chance that *lot* of work done can be viewed (e.g. on a separate 
Git branch or such)?
The work was initially done on a topic branch, but has since been 
merged to master. You can view the history in the Git repo. The 
majority of the work was done directly in Kolab_Storage, but some work 
was also done to the application drivers.
2015-12-17 05:03:45 mike (dot) gabriel (at) das-netzwerkteam (dot) de Comment #5 Reply to this comment
This patch is not against Nag's master branch. The ticket was 
created for the master branch - is this where you see the issue?
I see the issue in php-horde-nag 4.2.6-1 uploaded to Debian unstable 
on Oct 24th, 2015. As there have been no potential fixes on the master 
branch of github.com/horde/horde, I assume this issue is also still 
present on the master branch.

The patch also applies cleanly to the master branch using this command:

patch -p1 < PATCHFILE

"""
[mike@minobo horde.upstream (master)]$ patch -p1 < 
../my-horde-kolab-patches/horde-instance/patches/1004_nag-kolab-fix-storing-parent-id.patch
patching file nag/lib/Driver/Kolab.php
"""
Also, there has been a *lot* of work and testing of Kolab against 
the master branch fairly recently (though I don't remember seeing 
this issue come up).
Any chance that *lot* of work done can be viewed (e.g. on a separate 
Git branch or such)?

Thanks,
Mike
2015-12-16 23:17:17 Michael Rubinsky Comment #4
Version ⇒
Queue ⇒ Kolab
Reply to this comment
Moving to Kolab queue.
2015-12-16 23:02:34 Michael Rubinsky Comment #3
State ⇒ Feedback
Priority ⇒ 1. Low
Reply to this comment
This patch is not against Nag's master branch. The ticket was created 
for the master branch - is this where you see the issue?

Also, there has been a *lot* of work and testing of Kolab against the 
master branch fairly recently (though I don't remember seeing this 
issue come up).
2015-12-16 12:59:35 mike (dot) gabriel (at) das-netzwerkteam (dot) de Comment #2
New Attachment: 1004_nag-kolab-fix-storing-parent-id.patch Download
Reply to this comment
I am working on a patch...
Patch is attached. Works well with Kolab v2 backend.

(I plan to start working on Kolab v3, but first focus should be fixing 
what used to work / should have used to work in the past, that is 
Kolab v2).

Request for feedback...
2015-12-15 20:45:40 mike (dot) gabriel (at) das-netzwerkteam (dot) de Comment #1
Type ⇒ Bug
State ⇒ Unconfirmed
Priority ⇒ 3. High
Summary ⇒ Nag_Driver_Kolab stores parent_id strings base64 encoded (should be: unencoded)
Queue ⇒ Nag
Milestone ⇒
Patch ⇒ No
Reply to this comment
The Nag_Driver_Kolab breaks the Kolab format specification concerning 
the parent parameter.

The parent id of a task object should be stored as a UID hash when 
viewing the kolab.xml attachment of the Kolab storage object (i.e., a 
mail with one .xml file as attachment).

However, I see the UID hash stored base64 encoded:

"""
   <uid>5256723d-68a8-46b4-b349-07e24e2ecc62</uid>
   <body></body>
   <categories></categories>
   <creation-date>2015-12-15T19:38:42Z</creation-date>
   <last-modification-date>2015-12-15T19:38:42Z</last-modification-date>
   <sensitivity>public</sensitivity>
   <product-id>Horde_Kolab_Format_Xml-2.0.7 (api version: 1)</product-id>
   <summary>PALMER: Spracheinstellungen (deutsche Tastatur)</summary>
   <organizer>
     <display-name></display-name>
     <smtp-address></smtp-address>
     <uid></uid>
   </organizer>
   <alarm>0</alarm>
   <priority>2</priority>
   <completed>100</completed>
   <status>completed</status>
   vvvvvvvvvvvvvvvvvvvvv
   <parent>NTI0OTc1MTMtYjc1NC00OTMwLWE2YTktNGUwOTRlMmVjYzYy</parent>
   ^^^^^^^^^^^^^^^^^^^^
   <horde-estimate>0.00</horde-estimate>
   <horde-alarm-methods>a:0:{}</horde-alarm-methods>
"""

The challenge now is:

   * storing parent ids as non-base64 encoded hash (7-bit encoding, sort of)
   * recognize parent ids that have been stored with base64 encoding, 
decode them properly
   * store base64-encoded parent ids as non-base64-encoded UID hash 
with next save operation

I am working on a patch...


Kolab Format v3 (Site currently down):
https://wiki.kolab.org/Kolab_3.0_Storage_Format

Kolab Format v2:
https://kolab.org/history/kolabformat/c312

Saved Queries