feat(PassthroughParameters): Make it possible to pass through parameters to the SAML library

[mickenordin]

OneLogin/Auth->login can accept an array of parameters to be passed through to the IdP.

This patch set makes it possible to specify an array of parameters to be passed through to the IdP, in the admin settings of the app

[blizzz]

Thanks for this addition!

I left a change request there. Also wonder whether there is anything that shibboleth can be tested against, so we have it covered by integration tests. It's simple enough though, so I am not worried to much about this – if problems arise then probably through misconfiguration or unexpected values.

[mickenordin]

Now the setting has been moved to the optional idp settings, does that look ok @blizzz ?

[mickenordin]

I am not sure how the test framwork works, or how to run it locally, but I did try to add an integration test...

[mickenordin]

I was able to run the integration tests localy, and the scenario that I added passes (as do the others):

19 scenarios (19 passed)
500 steps (500 passed)
2m31.85s (9.77Mb)

[mickenordin]

Fixed the code formatting issue, sorry about that

[blizzz]

One more thing, that i found when having a second look in the target url validation. The redirection checks uses parseUrl, but does not check the query at the moment:

user_saml/tests/integration/features/bootstrap/FeatureContext.php

Lines 204 to 208 in 4ea5899

	$paramsToCheck = [
	'scheme',
	'host',
	'path',
	];

Adding 'query' would be necessary. And i would think it would break elsewhere, for the SAML parameters are usually added in some places. Perhaps a second, extended method could check the specified parameters are actually in the redirect URL.

[mickenordin]

Perhaps a second, extended method could check the specified parameters are actually in the redirect URL.

I seem to have problems setting the config value required in the test. I have this:

                                     # FeatureContext::theSettingIsSetTo()
    And The setting "idp-passthroughParameters" is set to "idp_hint,test_param"

But when I test for it I get:

                                     # FeatureContext::theSettingIsCurrently()
      Config value for idp-passthroughParameters is , but expected was idp_hint,test_param (UnexpectedValueException)

Any help here would be apprechiated

[blizzz]

Perhaps a second, extended method could check the specified parameters are actually in the redirect URL.

I seem to have problems setting the config value required in the test. I have this:

                                     # FeatureContext::theSettingIsSetTo()
    And The setting "idp-passthroughParameters" is set to "idp_hint,test_param"

But when I test for it I get:

                                     # FeatureContext::theSettingIsCurrently()
      Config value for idp-passthroughParameters is , but expected was idp_hint,test_param (UnexpectedValueException)

Any help here would be apprechiated

I can reproduce, now trying to understand it 😅

[mickenordin]

I see this now that I try a very reduced case with just setting that:

    And The setting "passthroughParameters" is set to "idp_hint,test_param"                                                   # FeatureContext::theSettingIsSetTo()

                                                        
  The "--passthroughParameters" option does not exist.  

Edit: yeah, I changed it to idp-passthroughParameters now, same issue when using the correct name though

[mickenordin]

                                                 
  Not enough arguments (missing: "providerId").  
                                                 

saml:config:set [--general-idp0_display_name GENERAL-IDP0_DISPLAY_NAME] [--general-uid_mapping GENERAL-UID_MAPPING] [--idp-entityId IDP-ENTITYID] [--idp-singleLogoutService.responseUrl IDP-SINGLELOGOUTSERVICE.RESPONSEURL] [--idp-singleLogoutService.url IDP-SINGLELOGOUTSERVICE.URL] [--idp-singleSignOnService.url IDP-SINGLESIGNONSERVICE.URL] [--idp-passthroughParameters IDP-PASSTHROUGHPARAMETERS] [--idp-x509cert IDP-X509CERT] [--security-authnRequestsSigned SECURITY-AUTHNREQUESTSSIGNED] [--security-general SECURITY-GENERAL] [--security-logoutRequestSigned SECURITY-LOGOUTREQUESTSIGNED] [--security-logoutResponseSigned SECURITY-LOGOUTRESPONSESIGNED] [--security-lowercaseUrlencoding SECURITY-LOWERCASEURLENCODING] [--security-nameIdEncrypted SECURITY-NAMEIDENCRYPTED] [--security-offer SECURITY-OFFER] [--security-required SECURITY-REQUIRED] [--security-signatureAlgorithm SECURITY-SIGNATUREALGORITHM] [--security-signMetadata SECURITY-SIGNMETADATA] [--security-sloWebServerDecode SECURITY-SLOWEBSERVERDECODE] [--security-wantAssertionsEncrypted SECURITY-WANTASSERTIONSENCRYPTED] [--security-wantAssertionsSigned SECURITY-WANTASSERTIONSSIGNED] [--security-wantMessagesSigned SECURITY-WANTMESSAGESSIGNED] [--security-wantNameId SECURITY-WANTNAMEID] [--security-wantNameIdEncrypted SECURITY-WANTNAMEIDENCRYPTED] [--security-wantXMLValidation SECURITY-WANTXMLVALIDATION] [--saml-attribute-mapping-displayName_mapping SAML-ATTRIBUTE-MAPPING-DISPLAYNAME_MAPPING] [--saml-attribute-mapping-email_mapping SAML-ATTRIBUTE-MAPPING-EMAIL_MAPPING] [--saml-attribute-mapping-group_mapping SAML-ATTRIBUTE-MAPPING-GROUP_MAPPING] [--saml-attribute-mapping-home_mapping SAML-ATTRIBUTE-MAPPING-HOME_MAPPING] [--saml-attribute-mapping-quota_mapping SAML-ATTRIBUTE-MAPPING-QUOTA_MAPPING] [--saml-attribute-mapping-mfa_mapping SAML-ATTRIBUTE-MAPPING-MFA_MAPPING] [--saml-attribute-mapping-group_mapping_prefix SAML-ATTRIBUTE-MAPPING-GROUP_MAPPING_PREFIX] [--saml-user-filter-reject_groups SAML-USER-FILTER-REJECT_GROUPS] [--saml-user-filter-require_groups SAML-USER-FILTER-REQUIRE_GROUPS] [--sp-x509cert SP-X509CERT] [--sp-name-id-format SP-NAME-ID-FORMAT] [--sp-privateKey SP-PRIVATEKEY] [--output [OUTPUT]] [--] <providerId>

[mickenordin]

Ok, I have a fix I think. Give me a sec :)

[blizzz]

Ok, I have a fix I think. Give me a sec :)

I am curious. I saw same stuff like yours and am doubting either the Symfony Console or something in the docker shell argument passing. For locally and manually i could not reproduce this, only with the integration tests.

[mickenordin]

It is working now, the test passes AND actually does something :)

[blizzz]

8.4 fails, due to a deprecation in php-saml. Let's ignore for now, also out of scope here.

[github-actions]

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)