🐛 Allow setting of itop_env $_SESSION variable through switch_env parameter if $_SESSION is still empty (SF1976) (support API and CRON)

[jbostoen]

• https://sourceforge.net/p/itop/tickets/1976/

This adds missing logic (I've put it on a separate line). The startup process allows the user to explicitly use the switch_env parameter to change the environment if the configuration file exists and if it's not the same as what's already in the $_SESSION['itop_env'] variable.

Original logic (relevant part on new line) checks whether the $_SESSION['itop_env'] is set, probably even just to avoid a warning in the comparison part if the $_SESSION['itop_env'] has NOT been set yet.

if (($sSwitchEnv != null) && (file_exists(APPCONF.$sSwitchEnv.'/'.ITOP_CONFIG_FILE)) && 
	(isset($_SESSION['itop_env']) && ($_SESSION['itop_env'] !== $sSwitchEnv))
)

But if people use the iTop REST/JSON API, definitely on the first call the $_SESSION['itop_env'] has not been set yet. So you can not override the environment when using the API.

The fix is easy: allow this override if the variable has not been set yet (= also 'not equal to' the value of the variable).

if (($sSwitchEnv != null) && (file_exists(APPCONF.$sSwitchEnv.'/'.ITOP_CONFIG_FILE)) && 
	((isset($_SESSION['itop_env']) && ($_SESSION['itop_env'] !== $sSwitchEnv)) || !isset($_SESSION['itop_env']))
)

Implementation risks are on the extremely low side: it will only change things for people/processes explicitly using the switch_env parameter and if the $_SESSION['itop_env'] has not been set yet.

The benefit is mostly for people who run multiple environments on one iTop "installation" (by installation, I mean: you can quickly grab all the code/installation package; keep all extensions you want in the web/extensions folder and run different environments on it (using the unattended install AND while still retaining the benefit of the API features).

This might be a faster way for people (probably mostly developers but definitely also regular clients who use the API) to have multiple iTop (test) instances without having to set up dedicated Docker containers or similar solutions.

[Molkobain]

Correct me if I'm wrong but another way to summarize the purpose of this PR is "fix the usage of the switch_env parameter in the REST/API" right? I think this way it will be way easier to sell it to the product team than "will benefits some devs" 😁🙊

[jbostoen]

It does benefit anyone who uses both the API and the switch_env parameter 😁

[jbostoen]

The above fix seems to work for the API.

For my use case, I was able to only put the iTop application files once on my computer and run multiple environments and connect to them with the API. Only using the switch_env URL parameter.

That alone is already a big step forward in maintaining all my iTop (demo/development) instances.

---

I do notice that cron jobs also lack this kind of support. There would at least need to be an additional change in the startup file so it also allows input from the CLI (switch_env). Some other refactoring is needed as well - see latest commit. It checks whether the relevant config-itop.php file exists (filename now determined by calling the startup earlier).

[Molkobain]

Hello Jeffrey, this was reviewed today.

The technical team is open to approve it with the small following changes. This does not mean that it will be accepted during the functional review but it's a step forward.

1. Regarding the startup change, we would like you to add a unit test for the REST/API testing several environments to ensure that it works. You can check existing REST/API unit tests in iTop, as for creating a secondary environment you can check the methods used by the toolkit for example (don't use the unattended install script).

2. Regarding the CRON it seems alright, no changes needed, we will need to test it manually. Not great but we don't have tests for that yet.

Cheers, Guillaume

[jbostoen]

Haven't made these kind of tests before, I'll need to look into it.

Not sure at all how to write a (for Combodo) suitable test; my personal test was actually just using different environments and adjusting the requests to include the switch_env parameter.

Also not very clear to me how different the environments should be (same test DB? but then how will you see the difference?)

https://github.com/Combodo/iTop/blob/develop/test/webservices/RestTest.php https://github.com/Combodo/itop-toolkit-community/blob/master/ajax.toolkit.php

[Molkobain]

Having a dedicated DB would be better as it is own it's used today by Combodo with the ITSM Designer and how you seem to be using it as well. No need to duplicate the BD content though, just create it empty if not already present. With the ITSM Designer we create a DB with the same name and a "_<ENV_NAME>" suffix.

[jbostoen]

I really have no clue how to write those tests and add the custom database I'm afraid :|

[Hipska]

I have the same here: #197 It would really help a lot if the GitHub PR did already the necessary CI checks, so you immediately see if your PR can be accepted.

[piRGoif]

It would really help a lot if the GitHub PR did already the necessary CI checks, so you immediately see if your PR can be accepted.

For any PHPUnit test that needs to have the datamodel loaded, I fear we cannot do much :/ And for now running any PR automatically in our test infrastructure would be risky (security)...

[Hipska]

And for now running any PR automatically in our test infrastructure would be risky (security)...

GitHub has options for that where you need to approve to run the CI for untrusted contributors. I'm using this on other repositories..

[jbostoen]

Unfixed (just discovered): iTop's email action relies on the ITOP_DEFAULT_CONFIG_FILE parameter, which only seems to be used here:

• https://github.com/Combodo/iTop/blob/7c7386afc71a9191f5fdd66888fe3ec8248ceaf0/core/email.class.inc.php#L48
• https://github.com/Combodo/iTop/blob/5f575d524a7bcc7567cbc8b904204416e9b445e3/sources/Application/Status/Status.php#L86

Why should email (and status) rely on default config rather than environment config?

Should this either be overruled, or taken care of in a different way (perhaps a ITOP_ENV_CONFIG_FILE or being able to pass the name of the config file or by default first call MetaModel::GetConfig() if it's available )? The initial email process will always take its settings from the default config now

---

Quick test, this could work (if acceptable):

	public function LoadConfig($sConfigFile = ITOP_DEFAULT_CONFIG_FILE)
	{
	
		if (is_null(self::$m_oConfig)) {

			// Prefer environment config if available
			if(class_exists('MetaModel') == true) {
				$oConfig = MetaModel::GetConfig();
				if($oConfig !== null) { 
					self::$m_oConfig = $oConfig;
                                        return;
				}
			}
			
			self::$m_oConfig = new Config($sConfigFile);
			
		}
	}

[Hipska]

I would deprecate the Email::LoadConfig method and replace this line: https://github.com/Combodo/iTop/blob/7c7386afc71a9191f5fdd66888fe3ec8248ceaf0/core/email.class.inc.php#L163

With either self::$m_oConfig = MetaModel::GetConfig(); or even $oConfig = MetaModel::GetConfig();. It's even more remarkable that the constructor does use the meta model config: https://github.com/Combodo/iTop/blob/7c7386afc71a9191f5fdd66888fe3ec8248ceaf0/core/email.class.inc.php#L58-L63

For application status, I would leave it as is because of the nature of why/how it is used..

[Molkobain]

Hum not sure why this was done, it might be a very old piece of code that wasn't updated to retrieve the config correctly. Pierre might be able to tell us more about this.

[jbostoen]

I would deprecate the Email::LoadConfig method and replace this line:

https://github.com/Combodo/iTop/blob/7c7386afc71a9191f5fdd66888fe3ec8248ceaf0/core/email.class.inc.php#L163

With either self::$m_oConfig = MetaModel::GetConfig(); or even $oConfig = MetaModel::GetConfig();. It's even more remarkable that the constructor does use the meta model config:

https://github.com/Combodo/iTop/blob/7c7386afc71a9191f5fdd66888fe3ec8248ceaf0/core/email.class.inc.php#L58-L63

For application status, I would leave it as is because of the nature of why/how it is used..

You're right, so wouldn't even need to check if MetaModel is already available. Perhaps there's cases where Email is constructed, but the "send" (and hence initial load config) only happens at a later point. You could argue the changes get picked up more instantly, but I doubt there will be use cases where this comes into play.

If you want to rely on self::$m_oConfig, it could indeed be set in the constructor.

[piRGoif]

Hello,

Good catch Jeffrey, thanks !

Indeed this is old code : \EMail::LoadConfig was last modified in 87bf0999, some... 10 years ago !

We had quite a mess with retrieving the config in the whole application and extensions... To solve most of the problems, we introduce in 2.7.0 lots of changes in \utils::GetConfig : since then this is THE method that should be used every time to get the config.

Indeed \MetaModel::GetConfig would only work if the datamodel is loaded... Whereas \utils::GetConfig has some fallbacks detailed in its phpdoc (see https://github.com/Combodo/iTop/blob/809ea2eb49bc00c6c3cad0e4544581947cf9f3e3/application/utils.inc.php#L769) Also, if \utils::GetConfig needs to open the file directly from disk, it will use the current env, not a hardcoded one.

I've added N°4947 in Combodo internal DB, and will add a PR for this fix.

[jbostoen]

Thanks for the very quick fix!

[jbostoen]

Now that the mail issue has been addressed... Any ideas on how to get this approved? Basically environments are somewhat supported in iTop; this PR addresses some flaws in their implementation?

[Hipska]

I would fix the merge conflicts first 😉

[jbostoen]

I would fix the merge conflicts first 😉

I'm going to wait on the feedback first before I spend time doing so. 😂

[piRGoif]

Jeffrey:

Now that the mail issue has been addressed...

Do you mean #278 ? It isn't merged yet... The decision was to merge it for 2.7.8. We are currently working on 2.7.7, so it won't be merged before second half of the year I think ?

[Hipska]

That PR has been replaced with #331 and is merged already, so could @jbostoen get some feedback? Or does he need to fix the conflicts first? (as the PR is in "Pending contributor update" column)

[Molkobain]

That PR has been replaced with #331 and is merged already, so could @jbostoen get some feedback? Or does he need to fix the conflicts first? (as the PR is in "Pending contributor update" column)

To me we are still in the "pending contributor" column waiting for the tests

Hello Jeffrey, this was reviewed today.

The technical team is open to approve it with the small following changes. This does not mean that it will be accepted during the functional review but it's a step forward.

1. Regarding the startup change, we would like you to add a unit test for the REST/API testing several environments to ensure that it works. You can check existing REST/API unit tests in iTop, as for creating a secondary environment you can check the methods used by the toolkit for example (don't use the unattended install script).

2. Regarding the CRON it seems alright, no changes needed, we will need to test it manually. Not great but we don't have tests for that yet.

Cheers, Guillaume

[jbostoen]

Thanks for the feedback! I simply have no idea how to create that kind of functional test.

[Molkobain]

Thanks for the feedback! I simply have no idea how to create that kind of functional test.

Maybe we could open a dedicated thread on the slack to bootstrap you, it would avoid to flood this PR as we might have many messages.

[Molkobain]

Discussion started here

[jbostoen]

The proposed bug fix above seems to need some additional work in iTop 3.1. It's the first thing I apply after downloading iTop for the last couple of releases. It always worked, but now it seems there's a dictionary-related issue when using the unattended install script (which seems to be unchanged for about a year in the Combodo GitHub repository).

Initially I assumed it was a regression in iTop 3.1, but that turns out not to be the case. I can mimic it with iTop 3.0.2 as well, where I simply already had an env-production in place. I'll need to find out how to integrate the Session::Set() call during an unattended install; as at some point it also relies on this info.

PHP Warning:  require_once(C:\xampp64\htdocs\iTop3.1\web/env-production/dictionaries/en-us.dict.php): Failed to open stream: No such file or directory in C:\xampp64\htdocs\iTop3.1\web\core\dict.class.inc.php on line 261

Warning: require_once(C:\xampp64\htdocs\iTop3.1\web/env-production/dictionaries/en-us.dict.php): Failed to open stream: No such file or directory in C:\xampp64\htdocs\iTop3.1\web\core\dict.class.inc.php on line 261
load event servicePHP Fatal error:  Uncaught Error: Failed opening required 'C:\xampp64\htdocs\iTop3.1\web/env-production/dictionaries/en-us.dict.php' (include_path='C:\xampp64\htdocs\iTop3.1\web\lib/pear/archive_tar;C:\xampp64\htdocs\iTop3.1\web\lib/pear/console_getopt;C:\xampp64\htdocs\iTop3.1\web\lib/pear/pear-core-minimal/src;C:\xampp64\htdocs\iTop3.1\web\lib/pear/pear_exception;C:\xampp64\php\PEAR') in C:\xampp64\htdocs\iTop3.1\web\core\dict.class.inc.php:261
Stack trace:
#0 C:\xampp64\htdocs\iTop3.1\web\core\dict.class.inc.php(122): Dict::InitLangIfNeeded('EN US')
#1 C:\xampp64\htdocs\iTop3.1\web\core\contexttag.class.inc.php(139): Dict::S('Core:Context=RE...')
#2 C:\xampp64\htdocs\iTop3.1\web\core\trigger.class.inc.php(52): ContextTag::GetTags()
#3 C:\xampp64\htdocs\iTop3.1\web\core\metamodel.class.php(2953): Trigger::Init()
#4 C:\xampp64\htdocs\iTop3.1\web\core\metamodel.class.php(6486): MetaModel::InitClasses('')
#5 C:\xampp64\htdocs\iTop3.1\web\core\metamodel.class.php(6322): MetaModel::LoadConfig(Object(Config), false)
#6 C:\xampp64\htdocs\iTop3.1\web\setup\runtimeenv.class.inc.php(131): MetaModel::Startup(Object(Config), true, false, false, 'demo-authentica...')
#7 C:\xampp64\htdocs\iTop3.1\web\setup\applicationinstaller.class.inc.php(709): RunTimeEnvironment->InitDataModel(Object(Config), true)
#8 C:\xampp64\htdocs\iTop3.1\web\setup\applicationinstaller.class.inc.php(313): ApplicationInstaller::DoUpdateDBSchema(Array, 'env-demo-authen...', Array, 'demo-authentica...', '', 'https://127.0.0...')
#9 C:\xampp64\htdocs\iTop3.1\web\setup\applicationinstaller.class.inc.php(117): ApplicationInstaller->ExecuteStep('db-schema', NULL)
#10 C:\xampp64\htdocs\iTop3.1\web\toolkit\unattended_install.php(174): ApplicationInstaller->ExecuteAllSteps()
#11 {main}
  thrown in C:\xampp64\htdocs\iTop3.1\web\core\dict.class.inc.php on line 261

Note: this can be resolved by fixing the unattended install script. It doesn't need more changes in iTop. The issue with the unattended install script is that it needs to set the session variable (itop_env) before sending any output. Once that's changed; it works like a charm.

Update: there actually is a regression in my use case. I used Session::Start() and Session::Set() in the unattended install script to fix my oversight, which happens when there is no "production" environment in place already.

However, iTop 3.1 brings another change (vs 3.0.2) which results in sessions not even being started anymore for CLI scripts. It also needs the line Session::$bAllowCLI = true; now.

[jbostoen]

Full disclosure: all of the above works very well.

But when signing out, it seems iTop somehow takes some of the config of the alternative environment (in this case, I'm logging out from an environment named "demo-mfa"); and then tries to load files from "env-production".

Fatal error: Uncaught Twig\Error\LoaderError: The "C:\Personal\htdocs\iTop3.1\web/env-production/jb-login-authenticator/templates/" directory does not exist ("C:\Personal\htdocs\iTop3.1\web/env-production/jb-login-authenticator/templates/"). in C:\Personal\htdocs\iTop3.1\web\lib\twig\twig\src\Loader\FilesystemLoader.php:92 Stack trace: #0 C:\Personal\htdocs\iTop3.1\web\lib\twig\twig\src\Loader\FilesystemLoader.php(78): Twig\Loader\FilesystemLoader->addPath('C:\\Personal\\htd...', '__main__') #1 C:\Personal\htdocs\iTop3.1\web\application\logintwig.class.inc.php(224): Twig\Loader\FilesystemLoader->setPaths(Array) #2 C:\Personal\htdocs\iTop3.1\web\application\loginwebpage.class.inc.php(377): LoginTwigRenderer->__construct() #3 C:\Personal\htdocs\iTop3.1\web\pages\logoff.php(107): LoginWebPage->DisplayLogoutPage(false) #4 {main} thrown in C:\Personal\htdocs\iTop3.1\web\lib\twig\twig\src\Loader\FilesystemLoader.php on line 92

iTop: An error occurred, check server error log for more information.

It seems the environment variable is removed too soon somewhere (perhaps the entire session or sth gets removed rather than just user credentials). IMHO, it's a product bug when using "iTop environment". When logging out, the logoff page should still consider the settings from the iTop you're logging off from. But the session gets reset before the page is outputted. (ResetSession method). While this is okay for most variables, the itop_env variable should only be unset AFTER logging off.

[jbostoen]

The REST client from the data collector would also need a small adjustment to support environments.