derricksmith / phpsaml

GLPI Plugin - SAML integration using the Onelogin SAML Library
MIT License
32 stars 24 forks source link

Geuest Account #135

Open StellarDriftLabs opened 1 year ago

StellarDriftLabs commented 1 year ago

@DonutsNL Hi i have downloaded the plugins from your repository. it works for the account on my domain but i can't login using a guest account on my domain. i get blocked on the this empty page. do you know if this is really achievable i mean login as a guest account ?

avec SAML_invité here is the error: Undefined array key "saml_configuration_name" in /var/www/html/glpi/plugins/phpsaml/setup.php on line 213 thanks in advance for your reply!

DonutsNL commented 1 year ago

Hi Jayamin,

Thank you for using our plugin.

First some background : As far as the PHPSAML is concerned the plugin 'does not care' who tries to login as long as the SAML information returned by the configured Identity provider (IdP) for example Azure AD is valid. In other words, the actual validation and decision to allow any account (guest or not) to login is made by the identity provider (IdP) not by our plugin. If the user is allowed the IdP should return the user browser with valid SAML body including at the very least name and email claims back tot PHPSAML that is than being processed and should login the user (given that JIT is enabled assuming these accounts are not prepopulated in GLPI).

The white screen The white screen usually is caused by an error that we did not anticipate. Could you please review the GLPI errorlogs and share any relevant (errors reported by acs) in this feed for me to review?

undefined array key Is reported by the config.php and is harmless. For this we will add an additional validation to make the error goes away.

Thanks!

DonutsNL commented 1 year ago

Added a 1 min fix for the undefined property message.

DonutsNL commented 1 year ago

Proxied should never be empty its a prefilled database field. Please make sure your database has been updated properly or fix it by reinstalling the last update from my repo. Does the config report you got less then expected fields?

StellarDriftLabs commented 1 year ago

Hey @DonutsNL hope you're doing well. proxied

This is proxied? Look, this is how it displays on my screen. It doesn't allow me to put/select anything. Also i get this message at the top of the configuration page "Phpsaml expected 21 configuration items but got 19 items instead" Thanks in advance for your reply.

DonutsNL commented 1 year ago

Hi @jayamin-10

This is usually caused by the update proces not finishing succesfully. Currently i did not write corrective measures when a different columncount is detected in the config. Its only reported. The error, expected XYZ but got less message is an indicator the database table isnt updated properly. This could be fixed by uninstalling and reinstalling the plugin, or try the manual approach.

For the manual approach: These statements should have been executed during the update process and should be validated and if found missing be added to the phpsaml table.

https://github.com/DonutsNL/phpsaml/blob/b537e3bdf1a5bc4381ed8fba0b1ca1f0994c3dd0/install/update.class.php#L280 https://github.com/DonutsNL/phpsaml/blob/b537e3bdf1a5bc4381ed8fba0b1ca1f0994c3dd0/install/update.class.php#L283 https://github.com/DonutsNL/phpsaml/blob/b537e3bdf1a5bc4381ed8fba0b1ca1f0994c3dd0/install/update.class.php#LL293C5-L293C5

StellarDriftLabs commented 1 year ago

@DonutsNL Thanks a lot for your help. the manual approach solved the proxied problem. now i can select 'yes' or 'no' options. But couldn't yet login with a guest account. The same blank screen. Here are the errors i got from glpi error log : PHP Warning: Undefined array key "http://schemas.xmlsoap.org/xx/xxxx/xx/identity/claims/surname" in /glpi/plugins/phpsaml/inc/phpsaml.class.php on line 348, referer: https://login.microsoftonline.com/ PHP Warning: Trying to access array offset on value of type null in /glpi/plugins/phpsaml/inc/phpsaml.class.php on line 349, referer: https://login.microsoftonline.com/ PHP Warning: Undefined array key "http://schemas.xmlsoap.org/xx/xxxx/xx/identity/claims/givenname" in /glpi/plugins/phpsaml/inc/phpsaml.class.php on line 350, referer: https://login.microsoftonline.com/ PHP Warning: Trying to access array offset on value of type null in /glpi/plugins/phpsaml/inc/phpsaml.class.php on line 350, referer: https://login.microsoftonline.com/ PHP Warning: Undefined variable $error in /glpi/plugins/phpsaml/inc/phpsaml.class.php on line 320, referer: https://login.microsoftonline.com/

DonutsNL commented 1 year ago

The required claims are missing in the SAML response from the idp. Are these claims added in the response?

https://learn.microsoft.com/en-us/azure/active-directory/develop/saml-claims-customization

StellarDriftLabs commented 1 year ago

@DonutsNL Yes they were provided. These surname and givenname errors are gone apart from this error: PHP Warning: Undefined variable $error in /glpi/plugins/phpsaml/inc/phpsaml.class.php on line 320, referer: https://login.microsoftonline.com/ PHP Warning: Undefined variable $error in /glpi/plugins/phpsaml/inc/phpsaml.class.php on line 320, referer: https://login.microsoftonline.com/

DonutsNL commented 1 year ago

Hi @jayamin-10

I still expect that not all required claims are provided and is causing the JIT user->add to fail.

I added:

Please download and install the latest version from my repo and tell me what the messages/log say now.

StellarDriftLabs commented 1 year ago

@DonutsNL It redirects back to the login page and does not register any error on glpi error log file for a guest user. But like before it works with a user of the domain. The cmails provided are : image

DonutsNL commented 1 year ago

let me quickly write a procedure to dump the provided Saml post to a local file so we can review that.

DonutsNL commented 1 year ago

Changed the front/acs.php it should now dump the headers in the /front/ directory for you to review.

Please logon using both users and review the dumps. Then please tell me what is different. Dont share that file anywhere for obvious reasons :-)

DonutsNL commented 1 year ago

Completely refactored the acs.php file and added a huge number of validations and error messages also implemented the ability to dump responses when debug is enabled.

StellarDriftLabs commented 1 year ago

@DonutsNL after transforming the nameid attribute claim with user.mail (test.saml@yahoo.com) instead of user.principalname (test.saml_yahoo.com#EXT#@domain.onmicrosoft.com) we can now sign in with the guest account. I think glpi doesn't accept ".com#EXT#" as ID Thanks a lot. you have been a great help. For me the plugin worked with a guest id.

DonutsNL commented 1 year ago

Glad it is working. Ill add an additional check in the acs and write a warning pointing out to the MS documentation

DonutsNL commented 1 year ago

I added an additional check to detect a guest user without transformations, could validate its working for me?

StellarDriftLabs commented 1 year ago

Hey @DonutsNL i will let you know tomorrow about the detection of a guest user without trasformation of nameid claim. Btw it would be really kind of you, if you could fix the bug for proxied where we had to pass three sql query (https://github.com/derricksmith/phpsaml/issues/135#issuecomment-1597287927) manually to alter a table. A lot of people would benefit from it. on that note i haven't quite get the point of proxied because i tested with proxied yes and no but didn't notice any difference.

DonutsNL commented 1 year ago

Hey @jayamin-10 ,

Yeah its something I need to dive into. Its also the reason why I added the fieldcheck in the config page. This to allow someone to correct it if the DB is corrupted for whatever reason. On the proxy my understanding is that it breaks the URLs used for redirect and validation in some cases because the plugin is looking at the wrong HTTP headers. There is a param for this in the PHP saml plugin i need to config.

StellarDriftLabs commented 1 year ago

Hey again @DonutsNL,

I did the test. this is the warning message we get. image

I think this is good,. People will have an idea how to solve the issue. If you could just add in the warning "Solution : use the value user.mail for the the claims 'nameid' and 'name' instead of using the default value user.principalname on your Azure sso application"

here is how i transformed the the claims which works : image

DonutsNL commented 1 year ago

Hi @jayamin-10,

Thank you for validating my check. Good it is working as intended. I updated the error message with your suggestion.

DonutsNL commented 1 year ago

@derricksmith, i think some merges will prove fruitfull. Also this issue can be closed.