enhancement: new gpliinventory under 10.0.8 not supportet yet if enforce plugin is active

walkerprog commented 8 months ago

First of all: very great plugin, thank you very much for this.

additional to existing code

        if (class_exists('PluginFusioninventoryCommunication') && strpos($_SERVER['HTTP_USER_AGENT'], 'FusionInventory-Agent_') !== false){ 
            if(strpos($_SERVER['REQUEST_URI'], '/plugins/fusioninventory/') !== false || strpos($_SERVER['REQUEST_URI'], '\plugins\fusioninventory/') !== false){
                return;
            }
        }

I added

        if (class_exists('PluginGlpiinventoryCommunication') && strpos($_SERVER['HTTP_USER_AGENT'], 'GLPI-Agent_') !== false){ 
            if(strpos($_SERVER['REQUEST_URI'], '/plugins/glpiinventory/') !== false || strpos($_SERVER['REQUEST_URI'], '\plugins\glpiinventory/') !== false){
                return;
            }
        }

After that the enforcement of the plugin can be used again and the agents are able to connect again. Maybe this can be integrated into the next version?

DonutsNL commented 6 months ago

Added exclusion in latest version https://github.com/DonutsNL/phpsaml

loyolajavi commented 6 months ago

Hi @DonutsNL, Whit the latest version, in my environment the problem persisted. I fixed it by adding the following line of code in phpsaml/inc/phpsaml.class.php: private const EXCLUDED_USERAGENTS = ['FusionInventory-Agent' => '/plugins/fusioninventory/', 'FusionInventory-Agent' => '/marketplace/fusioninventory/', 'FusionInventory-Agent' => '/plugins/glpiinventory/', 'FusionInventory-Agent_v2.6' => '/plugins/glpiinventory/', 'FusionInventory-Agent' => '/marketplace/glpiinventory/', 'GLPI-Agent' => '/', 'GLPI-Injector' => '/'];

I have agents "fusioninventory" still running, pointing to the url glpi.domain/plugins/glpiinventory/. Glpi 10.0.10 and plugin glpiinventory latest release

This line of logs/access-glpi.log help me to fix it: "POST /plugins/glpiinventory/ HTTP/1.1" 302 1065 "-" "FusionInventory-Agent_v2.6"

DonutsNL commented 6 months ago

Hi @loyolajavi, @walkerprog

I can realy use your help with testing.

I added dropdowns in which you can now manually configure the excluded paths. You are able to configure a path, optionally check the useragent, and configure what saml should do, either enforce auth or bypass it.

Its available in my repo at: https://github.com/DonutsNL/phpsaml

Additional stuff implemented

Add menu button in config menu making it more accessible
Disable login button if no valid config is found
Disable enforce/login if no valid configuration is found
Simplify installation logic hopefully reducing issues with the saml database
implement the GLPI migration logic for future installation and update action moving from 1.3.0 forward.
Addressed all linting issues in the sourcecode

walkerprog commented 6 months ago

Hi @DonutsNL I am on vacation right now and will not change anything during my absence for plain reason and really bad internet connection. Will be back next week and do the update and testing. You should have my feedback by the end of next week.

Sorry for the inconvinience.

DonutsNL commented 6 months ago

Im not in a hurry, happy hollidays 😉😎

loyolajavi commented 6 months ago

Hi @DonutsNL, of course, i will test this tomorrow for a feedback. Thanks!

DonutsNL commented 6 months ago

That would be great thx 👍🏻💪🏻

loyolajavi commented 6 months ago

Hi @DonutsNL, how are you? I tried to install the v1.3.0, and it threw an error:

Duplicate entry '1' for key 'PRIMARY'

In the files/_log/sql-errors.log this is the error message:

glpisqllog.ERROR: DBmysql::query() in /var/www/html/glpi/src/DBmysql.php line 379 *** MySQL query error: SQL: INSERT INTO glpi_plugin_phpsaml_configs (id, version, enforced, proxied, strict, debug, jit, saml_sp_certificate, saml_sp_certificate_key, saml_sp_nameid_format, saml_idp_entity_id, saml_idp_single_sign_on_service, saml_idp_single_logout_service, saml_idp_certificate, requested_authn_context, requested_authn_context_comparison, saml_security_nameidencrypted, saml_security_authnrequestssigned, saml_security_logoutrequestsigned, saml_security_logoutresponsesigned, saml_configuration_name) VALUES('1', '1.3.0', '0', '0', '1', '0', '0', '', '', '', '', '', '', '', '', '', '0', '0', '0', '0', 'default'); Error: Duplicate entry '1' for key 'PRIMARY' Backtrace : plugins/phpsaml/inc/config.class.php:1279
plugins/phpsaml/hook.php:59 PluginPhpsamlConfig::install() src/Plugin.php:915 plugin_phpsaml_install() front/plugin.form.php:51 Plugin->install() public/index.php:82 require()

id 1 already exists in the table "glpi_plugin_phpsaml_configs" it contains the data of v1.2.2. Or it was not deleted, or the instruction should be an update instead of an insert.

DonutsNL commented 6 months ago

Thanks for the feedback.

I know what the problem is and didnt get to fix it yet. Im implementing the GLPI migration object to manage this for us. I think ill address this issue this evening.

DonutsNL commented 6 months ago

hi @loyolajavi

I updated the migration logic. It will now also create simple backups of the configuration by copying the config table prior to updating and deleting the plugin. Could you retry your test?

loyolajavi commented 6 months ago

I tested with the new code. After the update, the table glpi_plugin_phpsaml_configs was not recreated So, there is no config now:

glpisqllog.ERROR: DBmysql::query() in /var/www/html/glpi/src/DBmysql.php line 379 *** MySQL query error: SQL: SHOW COLUMNS FROM glpi_plugin_phpsaml_configs Error: Table 'glpi.glpi_plugin_phpsaml_configs' doesn't exist Backtrace : plugins/phpsaml/inc/config.class.php:259
plugins/phpsaml/inc/phpsaml.class.php:473 PluginPhpsamlConfig->getConfig() plugins/phpsaml/inc/phpsaml.class.php:143 PluginPhpsamlPhpsaml::getSettings() plugins/phpsaml/inc/phpsaml.class.php:132 PluginPhpsamlPhpsaml::init() plugins/phpsaml/setup.php:174 PluginPhpsamlPhpsaml->__construct() src/Plugin.php:1681 pluginPhpsamlPostInit() src/Plugin.php:275 Plugin::doHook() inc/includes.php:95 Plugin->init() ajax/notifications_ajax.php:36 include() public/index.php:82

DonutsNL commented 6 months ago

Please use a plugin reinstall to recover from any inconsistant state.

loyolajavi commented 6 months ago

Oh ok, with the reinstall the table was created Ok, I will test and tell you how works

DonutsNL commented 6 months ago

Thx 💪🏻😎

loyolajavi commented 6 months ago

Good morning, It works great! I tested the inventory with both fusion and glpi agents, it works Ok and I didn't have to add any path to the exclude.

Also works Ok the menu button.

On the other hand, the disable login button if no valid config is found, i think is not working.. I deleted the idp id and it didn't disable the login button.

Finally, I think the enforce mode is not working, it redirects infinitely. In logs I see this: I wait for a minute since click on login, and in that minute this line repeats many times:

[10/Jan/2024:10:16:00] "POST /plugins/phpsaml/front/acs.php HTTP/1.1" 302 988 "https://URLOfMySSOService"

This message also appears (only once)

Usage of signed integers in primary or foreign keys is discouraged, please use unsigned integers instead in glpi_plugin_phpsaml_configs.id. in /var/www/html/glpi/src/DBmysql.php on line 2138, referer: https://URLGlpi/front/plugin.php

DonutsNL commented 6 months ago

Hi @loyolajavi,

Thanks for testing.

I deleted the idp id and it didn't disable the login button. Yeah the check was somewhat limited :) thats corrected.
Finally, I think the enforce mode is not working, it redirects infinitely. I have the same effect. Im diving into that one because somehow a redirect loop is created with the enforced setting. Thanks for being so precise 👍

walkerprog commented 6 months ago

Hi @DonutsNL

I do think for the redirect-loop a new version of the plugin will be necessasary? As SAML is very crucial for me I would wait for the new version to test all in once.

DonutsNL commented 6 months ago

I have been debugging and found and corrected the issue. The excludes need more testing as it was clearly not excluding acs.php. As a fix I removed acs.php from the excludes and hardcoded this exclude in the sourcecode because removing it manually from the dropdowns would obviously break the plugin.

I pushed the change to the repo

DonutsNL commented 6 months ago

There was an logic issue in the excludes.php logic. This issue caused excludes to only evaluate 1 exclude from the config and would never itterate. As a result acs.php was never evaluated causing the loop issue. This also showed that acs.php should not be configurable as it could be accidently deleted (causing the loop). I corrected both issues in the latest version inside my repo.

loyolajavi commented 6 months ago

Hi @DonutsNL, how are you today? I've reinstalled the plugin and now the login button works Ok and the loop redirection does not occur anymore. I have a question.. If you logout from the Glpi logout button (in enforced mode), works Ok? I really dont know if Glpi or my Idp service is not working as I expected (doesn't return to login page)

On the agents' side, I have to add this exclude path, with that, fusion and glpiAgent through /plugins/glpiinventory (I have GlpiInventory plugin installed) works Ok:

And just as a comment, I see this in files/_log/php-errors.log:

[2024-01-11 10:42:01] glpiphplog.WARNING: *** PHP Warning (2): Undefined array key "valid_id" in .../plugins/phpsaml/inc/phpsaml.class.php at line 365 Backtrace : plugins/phpsaml/inc/phpsaml.class.php:417 PluginPhpsamlPhpsaml::glpiLogout() plugins/phpsaml/inc/phpsaml.class.php:198 PluginPhpsamlPhpsaml::sloRequest() plugins/phpsaml/setup.php:204 PluginPhpsamlPhpsaml->processUserLogin() src/Plugin.php:1681 pluginPhpsamlPostInit() src/Plugin.php:275 Plugin::doHook() inc/includes.php:95 Plugin->init() front/logout.php:42 include() public/index.php:82 require()

DonutsNL commented 6 months ago

If you logout from the Glpi logout button (in enforced mode), works Ok? I really dont know if Glpi or my Idp service is not working as I expected (doesn't return to login page)

The flow would be something like this: This causes GLPI to flush all session auth stuff and redirect you to the configured idp Service LogOff location also clearing the session stuff from the idp. The idp will redirect you back to glpi mainpage (this now should be the main page because I removed the front/slo.php that wasnt doing anything but break stuff and confuse people). After redirect the login is enforced and you are redirected back to the (hopefully reset) idp asking you to relogin again. If successfull the samlresponse will be redirected back to the acs.php and if correct you are logged in again.

Troubleshooting the loop i have tested this flow multiple times without an issue. It might be problematic if the IDP does not provision an logoff url or the logoff url was misconfigured. This would prob cause an inconsistant state that most prob needs to be solved by resetting the idp session by closing the browser or something.

To help configuration i added the correct ACS and Redirect URLs on the config page. Maybe this helps:

DonutsNL commented 6 months ago

I see this in files/_log/php-errors.log:

I have not evaluated the original plugins session handling yet (and dont fully understand why it is implemented in the way it is) . I expect its an old GLPI version remanent. When implementing the option to bypass the enforced login (https://glpi.url/?nosso) I found that GLPI overwrites the $_SESSION superglobal. In effect this behaviour will wipe all custom properties stored in the $_SESSION. I expect the error you are seeing is caused by this GLPI behaviour.

In some obscure development thread i found that usage of the _SESSION was actually discouraged and developers should revert to using the GLPI cache instead. This is badly documented and i found the solution for the bypass option (that also needs to remember it is bypassing phpsaml during browsing glpi) by reviewing the GLPI sourcecode ($GLPI_CACHE->set(KEY, VALUE) and $GLPI_CACHE->get(KEY);

This needs to be addressed at some point in time.

walkerprog commented 6 months ago

I have tested it today

At first I reverted all my manual changes checked settings: Plugin enforce is enabled

SAML-Login is working as expected Logout redirects to GLPI-startpage and triggers immediate relogin - I think I should deep dive into the idp settings at this point

as expected XML delivery from GLPI-agent was broken again the same second i reverted my changes - as far as good, original state recovered

next update of the plugin to newest version (deleted all files in the plugins folder and uploaded new files) configuration is completely broken after update, all fields on the settings page are empty, no dropdown is working...

reverted to last functional state to copy the old, functional settings

reinstalled update via uninstall and reinstall, now the settings page is working again, but configuration was not transfered reconfigured plugin new menu point ist working fine and I really like it

SAML is now working again no noticable change in login or logoff behaviour

but inventory via GLPI-agent still NOT working :/

added new exclusion via Dropdowns: name: Bypass GLPI agent Agent contains: GLPI-Agent_ Url contains path or file: plugins/glpiinventory/index.php Bypass SAML auth: Yes

and THERE we go, NOW it is working login and logoff of user still no change noticeable but glpi agent can now drop the xml with the client data

maybe this could be added in the source code?

additionaly, on the settings page:

it states "No valid Ipd certificate details provided or available" - but certificate is provided and saml is working?? wrong check??
it states "A different version of Phpsaml is marked latest. Version 1.2.1 was found in the repository, you are running 1.3.0" - just for the eye, but could maybe be fixed in future releases?

@DonutsNL thank you very much for your work, appreciate it

for production use: what exactly is the plan now? Will the changes be written back to the repo from derricksmith? will your fork be official?

DonutsNL commented 6 months ago

Im not sure. Working from my dev branch surely is not ideal. @derricksmith has been busy merging. Im not sure when he will be able to merge the latest version.

derricksmith / phpsaml

enhancement: new gpliinventory under 10.0.8 not supportet yet if enforce plugin is active #152