Page MenuHomekolab.org

Fix S/MIME handling in syncroton (T1229)
ClosedPublic

Authored by rbrunhuber on Apr 28 2016, 10:09 AM.

Diff Detail

Repository
rS syncroton
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

rbrunhuber updated this revision to Diff 298.Apr 28 2016, 10:09 AM
rbrunhuber retitled this revision from to Fix S/MIME handling in syncroton (T1229).
rbrunhuber updated this object.
rbrunhuber edited the test plan for this revision. (Show Details)

Please, fix indentation (use spaces).

lib/kolab_sync_data_email.php
322–330

To exclude PGP messages this check need to be a little bit more specific. See https://github.com/roundcube/roundcubemail/blob/master/plugins/enigma/lib/enigma_engine.php#L499

rbrunhuber updated this revision to Diff 304.Apr 28 2016, 11:32 PM
  • Fixed indentation, replaced tabs with spaces
  • Fixed condition to exclude PGP
machniak accepted this revision.Apr 29 2016, 7:25 AM
machniak added a reviewer: machniak.

Still not perfect with code style, but other than that looks good.

This revision is now accepted and ready to land.Apr 29 2016, 7:25 AM
This revision was automatically updated to reflect the committed changes.
rbrunhuber reopened this revision.May 4 2016, 2:25 PM

I just found out that the test for smime-signatures does not work as expected: $struct is always null and so a signed only mail is not recognized.
For me there a two possibilities to fix this:

  1. Change test so it loops over all attachments and parts and checks if at least one has a mimetype of 'application/pkcs7-signature' then use the 'IPM.Note.SMIME.MultipartSigned' messageClass
  1. Implement a new method has_smime_sig in rcube_message class analogue to has_html_part() and check the result in the test.

Solution one looks like a good thing because it is completely "local" change but also may cause the check for smime signatures to be implemented multiple times if anybody else needs it. Solution feels more like the "right" way because ultimately the message (represented by the rcube_message class) should "know" it's features. Also the rcube_message class would be the central point to implement this only once for everyone to consume.

Please advise how to proceed and which solution you would prefer.

This revision is now accepted and ready to land.May 4 2016, 2:25 PM
machniak closed this revision.May 5 2016, 8:51 AM

According to RFC5751 the signed message has only two parts, i.e. one attachment. So, I think my fix (3ecd3f2df1b1) is good enough.