niklasfemerstrand / rc_openpgpjs

OpenPGP for Roundcube via JavaScript
GNU General Public License v2.0
108 stars 46 forks source link

Preferences for defaults for sign and encrypt are not stored #87

Closed sidamos closed 11 years ago

sidamos commented 11 years ago

I disabled "Always encrypt messages" and "Always sign messages" in the Roundtable (0.9.2) prefs/composing screen and clicked "Save" but it does not save these prefs.

niklasfemerstrand commented 11 years ago

Works for me, which browser and OS are you using? What's your cache setting?

sidamos commented 11 years ago

Chromium on Linux. The other Roundtable settings are being saved. You mean cache settings of the browser? I did not change anything there, I don't think anything can be changed there regarding cache.

niklasfemerstrand commented 11 years ago

Works for me with Chromium on Linux, try purging your cache. If that doesn't work check your Roundcube, DB and PHP logs for clues.

sidamos commented 11 years ago

I purged Chromium cache and also tried Firefox 17 and same problem. I see no errors in MySQL logs and no errors in Roundcube error log. I don't now where to look for PHP logs.

But I enabled SQL logging in Roundcube and this is the update statement: [21-Jul-2013 12:08:31 +0200]: [4] UPDATE users SET preferences = 'a:7:{s:8:\"timezone\";s:4:\"auto\";s:11:\"date_format\";s:5:\"d.m.Y\";s:9:\"date_long\";s:9:\"d.m.Y H:i\";s:16:\"message_sort_col\";s:7:\"arrival\";s:17:\"message_threading\";a:0:{}s:17:\"flag_for_deletion\";b:1;s:12:\"preview_pane\";b:1;}', language = 'en_US' WHERE user_id = '1';

So, it looks like it does not update the plugin settings in the database.

niklasfemerstrand commented 11 years ago

Elevating to bug even though I can't confirm it since some guys had this issue earlier as #80.

sidamos commented 11 years ago

How can I help to find the problem?

BTW, I am using PHP 5.3 and the rest of Roundcube and the plugin seem to work fine.

bogde commented 11 years ago

rc_openpgpjs.php -> function preferences_save($p)

i think the following code overwrites all the preferences for the compose page (instead of adding the settings for encrypt and sign):

  $p['prefs'] = array(
    'encrypt'     => get_input_value('_encrypt', RCUBE_INPUT_POST) ? true : false,
    'sign'     => get_input_value('_sign', RCUBE_INPUT_POST) ? true : false,
  );

something like this works and settings are saved:

$p['prefs']['encrypt'] = get_input_value('_encrypt', RCUBE_INPUT_POST) ? true : false; $p['prefs']['sign'] = get_input_value('_sign', RCUBE_INPUT_POST) ? true : false;

(tested with the latest version of roundcube).

mysql log output:

before: UPDATE users SET preferences = 'a:5:{s:12:\"preview_pane\";b:0;s:17:\"check_all_folders\";b:0;s:8:\"timezone\";s:4:\"auto\";s:16:\"refresh_interval\";i:300;s:17:\"message_threading\";a:0:{}}', language = 'en_US' WHERE user_id = '1';

after the mentioned change:

UPDATE users SET preferences = 'a:7:{s:7:\"encrypt\";b:1;s:4:\"sign\";b:0;s:12:\"preview_pane\";b:0;s:17:\"check_all_folders\";b:0;s:8:\"timezone\";s:4:\"auto\";s:16:\"refresh_interval\";i:300;s:17:\"message_threading\";a:0:{}}', language = 'en_US' WHERE user_id = '1';

niklasfemerstrand commented 11 years ago

Thanks a lot bogde, I pushed it to master.

sidamos: Please verify the latest commit.

sidamos commented 11 years ago

This did not fix it for me. Still get the same behavior and UDPATE statement. It would have surprised me if it would have, because I do not have the effect that it only saves its own settings and overwrites all others. In my case, it saves all standard settings but not those from the plugin. It looks to me, as if this function is never called in my case. How can I add some debug output? Roundcube only writes "errors" and "sendmail" and "sql" log files here, although I am using debug level 8.

bogde commented 11 years ago

in rc_openpgpjs.php, in function preferences_save($p), after if ($p['section'] == 'compose') { you can add a line like echo "save prefs called
";

this should output a line on the settings page, just above all the settings, as soon as you click save.

you could add more debug output here like print_r($p); just above the return statement. it should show what settings its trying to save. it should do no harm, it should just look uglier.

i'm very interested in how this goes.

bogde commented 11 years ago

also, if you look at the sql you posted you'll see this: ...;a:0:{}s:17..., this looks almost like what i used to get before the update: ...;a:0:{}}',...

do you have any other plugins enabled? can you try with those de-activated? can you post the mysql log output again.

sidamos commented 11 years ago

This is the debug output: save prefs called Array ( [prefs] => Array ( [compose_extwin] => 0 [htmleditor] => 0 [draft_autosave] => 300 [mime_param_folding] => 1 [force_7bit] => [mdn_default] => [dsn_default] => [reply_same_folder] => [spellcheck_before_send] => [spellcheck_ignore_syms] => [spellcheck_ignore_nums] => [spellcheck_ignore_caps] => [show_sig] => 1 [reply_mode] => 1 [strip_existing_sig] => 1 [default_font] => [forward_attachment] => [encrypt] => [sign] => ) [section] => compose [abort] => )

And this is the current SQL: UPDATE users SET preferences = 'a:7:{s:8:\"timezone\";s:4:\"auto\";s:11:\"date_format\";s:5:\"d.m.Y\";s:9:\"date_long\";s:9:\"d.m.Y H:i\";s:16:\"message_sort_col\";s:7:\"arrival\";s:17:\"message_threading\";a:0:{}s:17:\"flag_for_deletion\";b:1;s:12:\"preview_pane\";b:1;}', language = 'en_US' WHERE user_id = '1';

The code in the function looks like it should after the latest commit.

I do not have any other plugins enabled. I enabled the plugin like this: $rcmail_config['plugins'] = array('rc_openpgpjs');

sidamos commented 11 years ago

Got it! After once saving as enabled, it then worked saving as disabled:

UPDATE users SET preferences = 'a:9:{s:7:\"encrypt\";b:1;s:4:\"sign\";b:1;s:8:\"timezone\";s:4:\"auto\";s:11:\"date_format\";s:5:\"d.m.Y\";s:9:\"date_long\";s:9:\"d.m.Y H:i\";s:16:\"message_sort_col\";s:7:\"arrival\";s:17:\"message_threading\";a:0:{}s:17:\"flag_for_deletion\";b:1;s:12:\"preview_pane\";b:1;}', language = 'en_US' WHERE user_id = '1';

UPDATE users SET preferences = 'a:9:{s:7:\"encrypt\";b:0;s:4:\"sign\";b:0;s:8:\"timezone\";s:4:\"auto\";s:11:\"date_format\";s:5:\"d.m.Y\";s:9:\"date_long\";s:9:\"d.m.Y H:i\";s:16:\"message_sort_col\";s:7:\"arrival\";s:17:\"message_threading\";a:0:{}s:17:\"flag_for_deletion\";b:1;s:12:\"preview_pane\";b:1;}', language = 'en_US' WHERE user_id = '1';

PoGo606 commented 11 years ago

I cloned the repository today, and I have the same problem with RC 0.9.2, both options stays checked...

bogde commented 11 years ago

can you save any other settings? can you post mysql logs?

PoGo606 commented 11 years ago

Other settings from other plugins can be saved fine (couldn't see other settings for this plugin)

save prefs called Array ( [prefs] => Array ( [compose_extwin] => 0 [htmleditor] => 1 [draft_autosave] => 60 [mime_param_folding] => 1 [force_7bit] => [mdn_default] => [dsn_default] => [reply_same_folder] => 1 [spellcheck_before_send] => [spellcheck_ignore_syms] => [spellcheck_ignore_nums] => [spellcheck_ignore_caps] => [show_sig] => 1 [reply_mode] => 1 [strip_existing_sig] => 1 [default_font] => Arial [forward_attachment] => [encrypt] => [sign] => 1 ) [section] => compose [abort] => )

SQL Log UPDATE users SET preferences = 'a:73:{s:10:\"htmleditor\";i:1;s:11:\"dsn_default\";b:0;s:17:\"reply_same_folder\";b:1;s:10:\"reply_mode\";i:1;s:12:\"default_font\";s:5:\"Arial\";s:7:\"encrypt\";b:0;s:4:\"sign\";b:1;s:6:\"tzname\";s:13:\"Europe/Berlin\";[......]

bogde commented 11 years ago

Everything looks in order. What browser are you using? Can you clear cache and try again.

nemorath commented 11 years ago

Can only confirm this bug using Chromium on Ubuntu, Firefox on Ubuntu, Chorme on OS X, Firefox on OS X Always sign preference is saved though but not always encrypt. Roundcube 0.9.2 PHP 5.3.18

Glad to help in any way I can

ghost commented 11 years ago

I too confirm it. Saving the preferences works fine, but they are not applied to the "Compose Mail" view. The checkboxes are always checked although preferences state they are disabled.

EDIT: preferences are not saved at all, sorry for confusing you.

quemquis commented 11 years ago

I can also confirm this bug using Chrome on Win7. Using latest git.

quemquis commented 11 years ago

Just tested with Safari on Win7. Bug confirmed on that platform too.

unamedplayer commented 11 years ago

Same problem last build Chrome & Firefox on Mac.

cyberdaemon commented 11 years ago

Just a question: I see that you can change the behaviour for encryption/signing from RC settings menu and in the compose message. The default for this options are checked. How can I change this?

AdriVillaB commented 11 years ago

I used a proxy interceptor (Burp) and when I click in the Save button, the browser didn't send the parameters _encrypt and _sign. I modified the request with the proxy, adding the parameters but the problem persist.

quemquis commented 11 years ago

Has anyone managed to make any headway on this issue? As it currently stands it's very frustrating for users since they have to click on the arrow next to the From field in the Compose tab and manually uncheck the Encrypt and Sign boxes before sending an email.

J3ffr3y commented 11 years ago

I am having this same issue. I can work around it but it would be nice for this bug to get worked out.

ghost commented 11 years ago

If you can work around it, you might consider making a pull request if you have solved this issue. I too would be interested in a fix.

J3ffr3y commented 11 years ago

My work around is to simply manually uncheck when not sending an encrypted message. Unfortunately I don't have a coded solution that I can share.

bogde commented 11 years ago

try this and let me know if it works:

  1. go to settings -> preferences -> composing messages
  2. make sure Always encrypt messages is CHECKED and Always sign messages is UNCHECKED
  3. click Save
  4. go to mail -> compose and verify if the above settings were saved
  5. repeat steps 1 - 4, in step 2 make sure both options are UNchecked
bogde commented 11 years ago

also, can you confirm that you are dealing with a fresh roundcube installation or new user? can you try saving some other options first (not related to this plugin) and then go back and alter the plugin settings. i was only able to reproduce the behavior once, with a fresh install and new user (with no other settings saved).

bogde commented 11 years ago

just made one more test with a new user: saving both options unchecked first fails. saving other setting first (like compose in a new window) and then saving both encrypt and sign options works.

bogde commented 11 years ago

seems to also work: save first with both options checked. save again with both unchecked.

krautsource commented 11 years ago

Yes, saving with both checked and then saving with both unchecked indeed does the trick. This is because in /program/lib/Roundcube/rcube_user.php, in the save_prefs() function, the new settings for "encrypt" and "sign" are compared with the already-stored values; when the settings have never been saved before, both (old and new value) are found to be unset, so these settings are not written to the database. That's okay per se.

The actual issue is that the checkboxes in the browser are ticked even though their corresponding settings are not (yet) present in the database. That's because the plugin code incorrectly assumes "true" when the settings for "encrypt" and "sign" have actually never been saved to the database yet:

$this->rc->config->get('encrypt', true)

IMO, this should be changed to

$this->rc->config->get('encrypt', false)

and

$this->rc->config->get('sign', false)

respectively (in all 4 places within rc_openpgpjs.php). That way the checkboxes will no longer be ticked by default, which makes sense since the corresponding settings are not yet present in the database.

If OTOH you really want to "always encrypt" and "always sign" by default, you'd have two options:

HTH :-)

bogde commented 11 years ago

thanks for confirming. the two guys complaining about this probably died because they didn't reply, one way or another.

i concluded the same analysis as you did when i added my comments above, and basically came to the same conclusion, however i think Niklas is the one who should take this decision. although i understand the reasons for the options being assumed as 'checked', i'm more in favor of the 'usual' way of doing things in roundcube, that is having them both unchecked until the user checks them and saves the options. that doubled with a few lines in the docs describing how one can activate the plugin might do the trick.

your 'silently set options on first run' probably implies having a third 'option' in the database - 'first_run' true or false.

krautsource commented 11 years ago

Yes of course it's up to Niklas to decide on this, I was merely making suggestions :-)

That said, I agree with your preference not to make encrypt/sign a default for all messages. Both options require the presence of your own keypair and (in case of encrypt) the recipient's public key. If they aren't present in localStorage, sending the message fails and you have to manually un-tick the checkboxes to be able to send the email at all. Unfortunately, the receiver not being aware of PGP and therefore not having a public key is probably the rule rather than the exception. The same goes for the average Roundcube user in a multi-user installation.

unamedplayer commented 11 years ago

thanks for confirming. the two guys complaining about this probably died because they didn't reply, one way or another.

Still here, just waiting for a patch, for a broken checkbox, for months.

niklasfemerstrand commented 11 years ago

"Still here, just waiting for a patch, for a broken checkbox, for months."

Not to be rude or anything, but reading something like this isn't very motivational.

I run this project on my spare time. The project is open source meaning that there is no boring legal stuff stopping you from doing (almost) what you want with the project. You can fork it and you can submit patches instead of, for example, whining about that they are coming too slowly from somebody doing this voluntarily.

Patch coming after I got some food, that's a promise. In the meantime please take a second and reflect on your comment above and how it feels for a volunteer reading that.

But, indeed, this project is lagging heavily on my end. The reason for that is economical. Open source doesn't always pay and I have deadlines to meet in order to have food on the table and bills covered. It's the circle of life, admittedly it's unfortunate.

unamedplayer commented 11 years ago

Fair enough it was rude on my part; I have a few projects I take care of myself and have no time to fixing other's code not to mention how inefficient it is if you're not familiar with the code. That's why seeing so much time & discussion spent for a broken checkbox bugged me.

Your project is well implemented and adds a much needed piece of functionality to Roundcube. Sorry for being a dick :)

niklasfemerstrand commented 11 years ago

It's all history now :-)

Cheers guys.