Bug 128749 - Macro security level not displayed correctly in non-gtk3 backends
Summary: Macro security level not displayed correctly in non-gtk3 backends
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: UI (show other bugs)
Version:
(earliest affected)
6.4.0.0.alpha0+
Hardware: All All
: medium normal
Assignee: Not Assigned
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-11-12 14:15 UTC by Jan-Marek Glogowski
Modified: 2019-11-14 15:46 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:
Regression By:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jan-Marek Glogowski 2019-11-12 14:15:01 UTC
Description:
My original bug started as the macro security user dialog, when opening a signed document with invalid signature, to be different on Windows and Linux on security level "Very High", even with the same macro security level in the GUI. It was a longer way to find the problem, then expected initially ;-)

Real problem: when opening the macro security level dialog (Tools => Options... => LibreOffice => Security => Macro Security), the radio button doesn't represent the current value.

Just open the dialog with a new profile, like

$ SAL_USE_VCLPLUGIN=gtk3 ./inst*/program/soffice -env:UserInstallation=file:///tmp/test

which (correctly) shows the macro security level as "High". With any other backend, like:

$ SAL_USE_VCLPLUGIN=gen ./inst*/program/soffice -env:UserInstallation=file:///tmp/test

the default security level is displayed as "Very High".

During my initial investigation I found bug 125609 but I did miss the "see also" bugs.

My original bisecting pointed at commit 9127763db1fc ("weld MacroSecurity cluster") in the 6.3 development repository, but that might just be a false flag. I also noted that it didn't happen with any releases and a 
"git log --pretty= online origin/libreoffice-6-3 vcl/" had commit 4109dfff009f (Revert "tdf#108687 vcl: always enable tabstop on radio buttons"), which is a "see also" of 125609.

Also reverting commit 4109dfff009f on master fixes the problem for me.


Steps to Reproduce:
1. SAL_USE_VCLPLUGIN=gtk3 ./inst*/program/soffice -env:UserInstallation=file:///tmp/test
2. Tools => Options... => LibreOffice => Security => Macro Security
3. Register the default security level of "High"

Any non-gtk3 backend, like:

4. SAL_USE_VCLPLUGIN=gen ./inst*/program/soffice -env:UserInstallation=file:///tmp/test
5. Tools => Options... => LibreOffice => Security => Macro Security
6. Register the default security level of "Very High"


Actual Results:
The displayed macro security level is wrong.

Expected Results:
The displayed macro security level is correct.


Reproducible: Always


User Profile Reset: No



Additional Info:
Comment 1 Julien Nabet 2019-11-12 14:28:26 UTC
On Win10 with master sources updated today, here what I got:
by default, macro security is at very high level.
If I try to put it to high level, close LO and relaunch, I see it very high level again.
However, no pb if I put medium or low level.

Also I noticed this log each time I access security macro dialog:
error : Unknown IO error
Comment 2 Jan-Marek Glogowski 2019-11-12 14:37:55 UTC
I've added the same revert to Gerrit for master as: https://gerrit.libreoffice.org/82523

This side effect is more problematic then the one mentioned in the commit message of the original revert. I don't think that the state of bug 108687 is really correct, as it claims it's fixed in target:6.3.0, but the last comments contain these reverts, which IMHO should reopen bug 108687.
Comment 3 Justin L 2019-11-13 06:55:55 UTC
The problem in this scenario is something like this:
1.) VeryHigh is the last item to be processed. High (which IsChecked()) has already been processed and has ImplUncheckAllOther() which turns off tabstop as well.
2.) A label change for VeryHigh initiated a statechange, which re-initializes the button, turning tabstop on again.
3.) MacroSecurityLevelTP correctly identifes "High" as the mnCurLevel, and sets it active. However, since High is already marked as IsChecked, no state change occurs, and so nothing triggers the initialized tabstop to be removed from VeryHigh.  (Although ImplUncheckAllOther() has already run many times, it still needs to be run this time, after all the initalization has been finalized.)

A very ugly work-around would be for MacroSecurityLevelTP to:
    if (pCheck)
    {   //ensure state is fully toggled...
        pCheck->set_active(false);
        pCheck->set_active(true);
    }
Comment 4 Jan-Marek Glogowski 2019-11-13 18:53:49 UTC
(In reply to Justin L from comment #3)
> A very ugly work-around would be for MacroSecurityLevelTP to:
>     if (pCheck)
>     {   //ensure state is fully toggled...
>         pCheck->set_active(false);
>         pCheck->set_active(true);
>     }

You really can't do this, as this would need fixes for all users of radio items. This is just the first case found, which is now broken in master.

While looking into this problem I eventually came up for a fix for bug 108687 in Gerrit as: https://gerrit.libreoffice.org/#/c/82608/
It's smaller as it looks, as it changes quite some indention and refactors some code by dropping the bool parameters of GetRadioButtonGroup, which could / should be done in a separate patch. I've written quite a few comments into the commit message, but maybe this should be moved into bug 108687 altogether?
Comment 5 Jan-Marek Glogowski 2019-11-14 15:46:03 UTC
Since the fix for bug 108687 is now reverted in all branches, we can close this bug.