New test failure in AdminSystemConfigurationExampleSessionID.t

[bschmalhofer]

Since 2023-05-18, that is the first test run with Cpanel::JSON::XS, the test scripts/test/Selenium/Agent/Admin/AdminSystemConfigurationExampleSessionID.t reports an error. Here is the relevant snippet.

103         # Verify the deploy link click.
104         $Selenium->WaitFor(
105             ElementExists => '//a[contains(@href,"Subaction=Deployment")]',
106         );
107         $Selenium->find_element('//a[contains(@href,"Subaction=Deployment")]')->VerifiedClick();
108 $DB::single = 1;
109         $Self->Is(
110             $Selenium->execute_script("return \$('#DeploymentStart').length > 0"),
111             "1",
112             "The deployment link not redirecting to login.",
113         );

It is not obvious what the correct behavior is. The test script does:

• turns off the session cookie
• log in with credentials
• change a sample SysConfig setting (all in JS presumably)
• checks whether the alert for the deployment shows up
• clicks on the link in the alert
• expects that there is no redirect to the login page

The last assertion fails, there is a redirect to the login page. This makes sense, as the session cookie is turned off.

TODO:

• investigate why it worked before
• fix the code or adapt the test suite

[bschmalhofer]

Boiled it down to a single commit. The error occurs in the commit a5e3233 but not in a5e3233^. The difference is not really apparent. In both cases the test script should click on the notification for dirty SysConfig, which is the link https://localhost:1484/otobo/index.pl?Action=AdminSystemConfigurationDeployment;Subaction=Deployment . But only a5e3233^ then redirects to the login page.

On possibility is that in the working case there is some kind of JavaScript magic that makes no request to the server. The next step could be a comparison of the HTML that shows the notification.

See #399.

[bschmalhofer]

Might be related to OTRS/otrs@bd974e5#diff-8e6b4bce22cf7471e867d1b768cb1d147df1263aec09298ace8ae4cdccec2298. The session id should be added to the deployment link.

[bschmalhofer]

Looks like the broken behavior is due to bugs that are fixed in Cpanel::JSON::XS but not in JSON.pm. In var/httpd/htdocs/js/Core.SystemConfiguration.js there is a check of SessionUseCookie from Core.Config.

 498                     if (Core.Config.Get('SessionUseCookie') === '0') {
 499                         LinkURL += ';' + Core.Config.Get('SessionName') + '=' + Core.Config.Get('SessionID');
 500                     }

Things are fine when the comparision with the single letter string '0' succeeds. Core.Config is set up in Kernel/Output/HTML/Layout/Template.pm by JSON serializing a hash. This hash config the value for SessionUseCookie from the SysConfig. But this value is the number 0, not the string '0'.

otobo@ad98e2ba4b0f:~$ grep "'SessionUseCookie'" Kernel/Config/Files/ZZZAAuto.pm 
$Self->{'SessionUseCookie'} =  0;

Thus the comparison should fail. It did not fail because JSON.pm serializes the number 0 incorrenctly. Cpanel::JSON::XS serializes correctly but breaks the comparison.

otobo@ad98e2ba4b0f:~$ cp JSON_old_but_working.pm Kernel/System/JSON.pm 
otobo@ad98e2ba4b0f:~$ bin/otobo.Console.pl Maint::Config::Dump SessionUseCookie
"0"
otobo@ad98e2ba4b0f:~$ cp JSON_new_but_broken.pm Kernel/System/JSON.pm 
otobo@ad98e2ba4b0f:~$ bin/otobo.Console.pl Maint::Config::Dump SessionUseCookie
0
otobo@ad98e2ba4b0f:~$ 

This could be fixed by fiddling with Kernel/Output/HTLM/Layout/Template.pm . But this is confusing. I propose to fix the JS instead. There don't seem to be many occurrences.

otobo@ad98e2ba4b0f:~$ grep -r "=== '0'" var/httpd/htdocs/js/
var/httpd/htdocs/js/Core.SystemConfiguration.js:                    if (Core.Config.Get('SessionUseCookie') === '0') {
var/httpd/htdocs/js/Core.SystemConfiguration.js:                        if (Core.Config.Get('SessionUseCookie') === '0') {
var/httpd/htdocs/js/thirdparty/ckeditor-4.16.0/plugins/image2/dialogs/image2.js:                if ( value === '0' )
var/httpd/htdocs/js/Core.UI.js:            if (Core.Config.Get('SessionUseCookie') === '0') {
otobo@ad98e2ba4b0f:~$ grep -r "!== '0'" var/httpd/htdocs/js/
var/httpd/htdocs/js/Core.Agent.Overview.js:            if ($(this).val() !== '0') {
var/httpd/htdocs/js/thirdparty/qunit-2.18.2/qunit.js:    enabled: !NODE_DISABLE_COLORS && NO_COLOR == null && TERM !== 'dumb' && (FORCE_COLOR != null && FORCE_COLOR !== '0' || isTTY),
var/httpd/htdocs/js/Core.UI.InputFields.js:        if ($SelectObj.data('filtered') && $SelectObj.data('filtered') !== '0') {
var/httpd/htdocs/js/Core.UI.InputFields.js:                        && $SelectObj.data('filtered') !== '0'
var/httpd/htdocs/js/Core.UI.InputFields.js:                        } else if ($SelectObj.data('filtered') !== '0') {
var/httpd/htdocs/js/Core.UI.InputFields.js:                            && $SelectObj.data('expand-filters') !== '0'
var/httpd/htdocs/js/Core.UI.InputFields.js:                            && $SelectObj.data('filtered') !== '0'
var/httpd/htdocs/js/Core.Agent.DynamicFieldContactWDSearch.js:                AutoCompleteActive = AutoComplete.DynamicFieldContactWD.AutoCompleteActive !== '0'
otobo@ad98e2ba4b0f:~$ 

[bschmalhofer]

Fixing this problem by making the following change:

-                        if (Core.Config.Get('SessionUseCookie') === '0') {
+                        // The untyped comparison with '==' works when SessionUseCookie is either the string '0' or the number 0.
+                        if ( ( Core.Config.Get('SessionUseCookie') ?? 'not configured' ) == '0') {

The condition holds when SessionUseCookie is 0 or '0', which is the intended new behavior.
The condition does not hold for SessionUseCookie being null, undefined, '', 'any other string', or any other number. This is the old behavior that should not be changed.

[bschmalhofer]

The previous fix fixed only half of the problem. The JavaScript method `Core.Config.Get(Key, DefaultValue) did not retrieve the value 0 for SessionUseCookie. Instead the undefined DefaultValue was retrieved. This broke the check whether SessionUseCookie has been disabled.

Fixed Core.Config.Get() with:

-                    return ConfigLevel[ConfigPrefix + Keys[KeyToken]] || DefaultValue;
+                    return ConfigLevel[ConfigPrefix + Keys[KeyToken]] ?? DefaultValue;

[bschmalhofer]

Test failures are almost gone now, but there is a new failure:

/opt/otobo/ scripts/test/Selenium/Agent/AgentAppointmentCalendarOverview.t                                 (Wstat: 257 (exited 1) Tests: 165 Failed: 1)
  Failed test:  165
  Non-zero exit status: 1

Looks like some kind of Layout issue, likely not related to the JSON changes. Let's handle this in a new issue.

[bschmalhofer]

Closing as tests look fine.

[bschmalhofer]

The change

•                return ConfigLevel[ConfigPrefix + Keys[KeyToken]] || DefaultValue;
  

•                return ConfigLevel[ConfigPrefix + Keys[KeyToken]] ?? DefaultValue;
  

must be rolled back as there were instances where GUI broke. One example is the PictureUploadAction in Kernel/Output/HTML/Layout.pm . Reopening this issue.

[bschmalhofer]

That the change has to be reverted is clear. However there are five new test failures in the test suite, including the original errorAdminSystemConfigurationExampleSessionID.t. I merged anyways and accept that there are some intermediary failures.

Here is the analysis part. It was already stated above that the problem can be highlighted by dumping a numeric Sysconfig setting one time with JSON and the other time Cpanel::JSON::XS.

otobo@ad98e2ba4b0f:$ cp JSON_old_but_working.pm Kernel/System/JSON.pm
otobo@ad98eis2ba4b0f:$ bin/otobo.Console.pl Maint::Config::Dump SessionUseCookie
"0"
otobo@ad98e2ba4b0f:$ cp JSON_new_but_broken.pm Kernel/System/JSON.pm
otobo@ad98e2ba4b0f:$ bin/otobo.Console.pl Maint::Config::Dump SessionUseCookie
0
otobo@ad98e2ba4b0f:~$

Further debugging showed that the versions of the JSON libraries do not matter.

The explanation why SessionUseCookie is considered one time as a string and the other time as number is not obvious. But the relevant hint is right in the documentation of Cpanel::JSON::XS: https://metacpan.org/pod/Cpanel::JSON::XS#simple-scalars .

Non-deterministic behavior is following: scalars that have last been used in a string context before encoding as JSON strings, and anything else as number value:

For SessionUseCookie this checks out. The number is created by running ZZZAAuto,pm and never meddled with, e.g. printed, before converted to JSON.

JSON::XS follows more or less the same strategy but apparently there are differences in details.

The take home message is that the decision why a (numeric) value is represented as JSON as a string might be deterministic, but not transparent. So the next best idea is to force transparency. This can be done by setting $json->type_all_string(1) for the values that are passed into the JavaScript World. This is a simple rule, that forces stringiness on the numbers. It seems to be in line with the current implementation:

otobo$ grep -r Core.Config.Get var/httpd/htdocs/js/ | grep parseInt | wc -l
27

[bschmalhofer]

This is fixed with the changes done for #2595 .