Bug 223441 - Port, fix and test objecttreeparser
Summary: Port, fix and test objecttreeparser
Status: RESOLVED FIXED
Alias: None
Product: kmail
Classification: Applications
Component: messageviewer (show other bugs)
Version: unspecified
Platform: Compiled Sources Linux
: HI normal
Target Milestone: ---
Assignee: András Manţia
URL:
Keywords: akonadi-ports-regression
Depends on: 226408
Blocks: 223438
  Show dependency tree / graph
 
Reported: 2010-01-19 19:41 UTC by Thomas McGuire
Modified: 2010-05-07 20:19 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:

Attachments
Example mailman message (31.75 KB, application/mbox)
2010-04-06 11:11 UTC, Thomas McGuire 		Details
View All Add an attachment

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas McGuire 2010-01-19 19:41:36 UTC 
Version:           4.5 (using Devel)
OS:                Linux
Installed from:    Compiled sources

The objecttreeparser is still not completely ported.

Some bugs:
- It creates strange mime structures during parsing of embedded messages
- trueFromAddress() got lost in the port, now signatures and photos in embedded messages will not work anymore


Ideally, the objectreeparser should have more unit tests, especially for parsing crypto messages correctly. Those have been proven to be brittle in the past.

Possible test combinations:
- signed, encrypted, signed + encrypted
- inline, detached, opaque
- broken signatures
- chiasmus
- old-style mailman messages
- embedded messages
- embedded messages with signatures
- ...
Comment 1 Thomas McGuire 2010-01-22 20:08:58 UTC 
> It creates strange mime structures during parsing of embedded messages

That can be seen very good when using the mime part tree viewer, the tree grows and grows and grows...
Comment 2 Torgny Nyblom 2010-02-12 07:29:10 UTC 
*** Bug 226408 has been marked as a duplicate of this bug. ***
Comment 3 Thomas McGuire 2010-04-06 10:37:37 UTC 
> It creates strange mime structures during parsing of embedded messages

I fixed that one by adding special support for encapsulated messages to KMime, the OTP now doesn't modify the MIME structure anymore, for encapsulated messages.
Comment 4 Thomas McGuire 2010-04-06 11:02:51 UTC 
Here are the results/TODO list after an IRC discussion:

1) Make sure that saving the message as mbox saves the original encrypted 
   message, not the modified one
2) What should happen when deleting/edit an attachment that is part of the 
   encrypted message? Check what KMail 1 does in this case.
3) Be careful when saving back the message (for some other reason than 
   deleting/editing attachments), it should save back the original message, not 
   the modified one
4) Viewing encrypted/signed messages has two problems right now, see 
   http://imagebin.org/91863:
   a) In the viewer, the original encrypted nodes are shown as attachments in 
      the viewer. Didn't happen in KMail 1.
   b) The signature is marked as invalid, even though it is valid
   I created the test message with KMail 1, in the following way:
   a) In the composer, add an attachment and mark the message as signed and 
      encrypted. Either send the message to yourself or save as draft, if the 
      "Never sign/encrypt when saving as draft" option is disabled
   b) Right-click the message in the message list and save as mbox
   c) In KMail 2, import the message using File->Import Messages
5) There is a special option to always save the decrypted form of the message 
   once it is displayed.
   This option is storeDisplayedMessagesUnencrypted().
   This is done in ViewerPrivate::parseMsg(), which calls 
   objectTreeToDecryptedMsg().
   This does currently not work, as some TODO comments indicate.
   Note that the decrypted message is _not_ the same as the modified message 
   the OTP creates when decrypting a message. The OTP still leaves the old 
   encrypted nodes around, while objectTreeToDecryptedMsg() removes them, so 
   that the message is truely decrypted and viewable by everyone.
   objectTreeToDecryptedMsg() should be unit-tested.
6) Make sure insertAndParseChildNode() works, and check all if all callers 
   work. One example are old-style mailman messages, but I think there are more
7) trueFromAddress() was incorrectly ported. Check the KMail 1 sources for it.
   trueFromAddress() is used when displaying encapsulated messages, for example 
   when fetching the contact photo or when verifying the signature of 
   encapsulated messages. This was ported to always use the From address of the 
   top-level node, so it doesn't work anymore for encapsulated messages.
8) As this stuff is complicated, especially the crypto stuff and the parts that 
   modify the message, unit tests are needed.
   To get the crypto stuff unit tested, one needs special unit test keys, and 
   set the gnupg home to the unit test dir. Have a look at how the 
   messagecomposer does it, in kdepim/messagecomposer/tests/gnupg_home and the 
   setenv() calls in ComposerTestUtil::getKeys()
Comment 5 Thomas McGuire 2010-04-06 11:11:49 UTC 
Created attachment 42523 [details]
Example mailman message

insertAndParseNewChildNode() is used by the crypto stuff and by processMailmanMessage().

I don't see why processMailmanMessage() needs to modify the message, it could be changed to not use insertAndParseNewChildNode(), probably. Or it could be ported and made to work.

Anyway, I attached a test mailman message here. Can be also found at the kdabtest1 account in the "test mails" folder.
Comment 6 András Manţia 2010-04-08 13:56:03 UTC 
SVN commit 1112524 by amantia:

1) clean up and rename insertAndParseNewChildNode to reflect what it is really doing. From now on the mime tree is not modified when a message is decrypted or a mailman message is processed.
2) fix signature verification for messages with attachments
3) fix displaying of mailman messages
4) don't show the encrypted data as attachments for decrypted messages
5) introduce a helper class to get the from() as a QString from a message

CCBUG: 223441


 M  +20 -1     nodehelper.cpp  
 M  +4 -0      nodehelper.h  
 M  +37 -75    objecttreeparser.cpp  
 M  +3 -8      objecttreeparser.h  


WebSVN link: http://websvn.kde.org/?view=rev&revision=1112524
Comment 7 Thomas McGuire 2010-04-09 13:39:04 UTC 
Adding some notes after a short IRC discussion:

Right now, createAndParseTempNode() deletes the node after calling parseObjectTree() on it. This means all state attached to the node gets lost, i.e. the mementos and all the maps in the NodeHelper, like mEncryptionState.
The OTP is called multiple times on the same root node, for async crypto stuff and also for things like expanded/collapsed quote marks, so the state should not get lost at all. Probably the temp node should be stored in NodeHelper instead.

The attachment quick list in the fancy header right now does not work for attachments in encrypted messages. Same with the mimetree viewer, ideally it would only show the decrypted parts, and not the original encrypted one, once it has been decrypted. Clicking on nodes in the mimetree viewer should also work for stuff inside encrypted nodes.

mStoredMessagePayload can now be removed, and probably also the "processed source" tab in the source viewer. Or maybe the "processed source" tab should show the source of the temp nodes.

Note to self: Update the OTP documentation about insertAndParseChildNode, since it has changed so much now.
Comment 8 Thomas McGuire 2010-04-09 13:41:05 UTC 
Another point: Attachment actions, like saving them, should also work in encrypted nodes, no idea if that works currently. Before, it worked, since the original MIME tree was modified.
Comment 9 Thomas McGuire 2010-04-18 15:07:38 UTC 
> 1) Make sure that saving the message as mbox saves the original encrypted 
>    message, not the modified one

Fixed, as the OTP doesn't modify the original message anymore.
mStoredMessagePayload can also be removed again.

> 2) What should happen when deleting/edit an attachment that is part of the 
>    encrypted message? Check what KMail 1 does in this case.

This is buggy in KMail 1, attachment deletion and editing inside of decrypted nodes does not work. Easiest way around this is to just disable those actions for those temp nodes.

> 3) Be careful when saving back the message (for some other reason than 
>    deleting/editing attachments), it should save back the original message, 
>    not the modified one

Fixed as well, same reason as 1).

> a) In the viewer, the original encrypted nodes are shown as attachments in 
>    the viewer. Didn't happen in KMail 1.

Fixed according to Andras.

> b) The signature is marked as invalid, even though it is valid

Fixed as well.

> storeDisplayedMessagesUnencrypted().

For all of the store unencrypted message stuff, we discussed that it is probably best to rewrite objectTreeToDecryptedMsg() and base it on the temporary nodes createAndParseChildNodes() creates.

> 6) Make sure insertAndParseChildNode() works, and check all if all callers 
>    work. One example are old-style mailman messages, but I think there are more

Old style mailman messages are fixed, and crypto stuff probably as well.

> 7) trueFromAddress() was incorrectly ported

This is fixed now I guess, after processRfc822Subtype was changed. I might need to double-check this though.

> Right now, createAndParseTempNode() deletes the node after calling
> parseObjectTree() on it.

We decided on store the newly created nodes in the NodeHelper so that the nodes can be found again when the OTP is run again on the same tree, in async mode.

> The attachment quick list in the fancy header right now does not work 

Probably the easiest way to fix this it to change the AttachmentUrlHander to take attachments inside of temp nodes into account. This would probably also fix attachments actions such as save as and others.
It might be an idea to make the mime tree model aware of temp nodes as well, since we need to be able to see the decrypted nodes in the mime tree viewer.