Fix/saml integration #13742

mollux · 2024-05-12T08:30:33Z

Q	A
Bug fix? (use the a.b branch)	[x]
New feature/enhancement? (use the a.x branch)	[ ]
Deprecations?	[ ]
BC breaks? (use the c.x branch)	[ ]
Automated tests included?	[ ]
Related user documentation PR URL	mautic/mautic-documentation#...
Related developer documentation PR URL	mautic/developer-documentation#...
Issue(s) addressed	Fixes #13361

Description:

The SAML integration is broken, due to an incompatibility with the expected arguments in the LightSaml\SpBundle DefaultController.
This is addressed in mautic/SpBundle#2, and this PR is the proof that this works.

This PR should not be merged, but is a way to test this out.

Steps to test this PR:

This PR should be tested locally as it requires an usable IDP to test the SAML auth flow.

checkout this branch and run composer update --lock to get the changed lightsaml/sp-bundle dependency.
Run a Mock simplesamlphp instance to be able to test the flow
Replace <LOCAL_MAUTIC_URL> with your local url, e.g. http://mautic.localhost
change the ports if needed if they conflict with your local setup.
```
docker run -p 8080:8080 \
-e SIMPLESAMLPHP_SP_ENTITY_ID=<LOCAL_MAUTIC_URL> \
-e SIMPLESAMLPHP_SP_ASSERTION_CONSUMER_SERVICE=<LOCAL_MAUTIC_URL>/s/saml/login_check \
-d kenchan0130/simplesamlphp
```
go to http://localhost:8080/simplesaml/saml2/idp/metadata.php?output=xhtml and copy the metadata in xml format to a temp file (e.g. mautic_saml_test_metatada.xml).
go to <LOCAL_MAUTIC_URL>/s/config/edit?tab=userconfig and select your entity ID, upload the metadata.xml file, select a role to create users, and fill in email in the fields Email, First Name and Last name (we are testing, so no problem that the actual values are not correct). Click save.
go in an anonymous tab to <LOCAL_MAUTIC_URL> and you should be redirected to the simplesaml page (if you get there, that proofs this patch works)
login with username user1 and password password
you're redirected to Mautic and logged in as user1

codecov · 2024-05-12T08:38:01Z

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 61.93%. Comparing base (aa4ac3c) to head (7e0e600).

Additional details and impacted files

@@             Coverage Diff              @@
##                5.x   #13742      +/-   ##
============================================
- Coverage     61.93%   61.93%   -0.01%     
  Complexity    34196    34196              
============================================
  Files          2249     2249              
  Lines        102267   102268       +1     
============================================
- Hits          63337    63336       -1     
- Misses        38930    38932       +2

Files	Coverage Δ
...les/CoreBundle/EventListener/ExceptionListener.php	`40.62% <0.00%> (-1.32%)`	⬇️

... and 1 file with indirect coverage changes

mallezie · 2024-05-24T13:52:38Z

PHPStan still points out an issue. No need for the elseif, it will always be like that, you can just change the else condition. (If PHPstan is correct that is offcourse ;-) -

LordRembo · 2024-05-24T14:13:36Z

Okay, so I tried to follow the steps but maybe I did something wrong. Had 2 issues:

step 2: Run a Mock simplesamlphp instance -> I pasted that multi-line command in my Terminal and gave this output (note the warning at the top):

Unable to find image 'kenchan0130/simplesamlphp:latest' locally
latest: Pulling from kenchan0130/simplesamlphp
13808c22b207: Pull complete 
8ea9cef6db5a: Pull complete 
ff65b997523e: Pull complete 
46d87a00aaae: Pull complete 
6b61c519885f: Pull complete 
4833ee2f6ebe: Pull complete 
ef4edbe797ac: Pull complete 
b89ff01946fc: Pull complete 
bf2a981fb28a: Pull complete 
532fd16ffa9e: Pull complete 
25a973e7be21: Pull complete 
38c2947a515d: Pull complete 
13e7ae825cb0: Pull complete 
bad6c4ae6412: Pull complete 
65b1f01d81cc: Pull complete 
0da35eb28861: Pull complete 
6fdbda77f1ff: Pull complete 
0c42ecb45daf: Pull complete 
8f75a07b0544: Pull complete 
04f669127e35: Pull complete 
f25ff63fbd3b: Pull complete 
51ccb185ce26: Pull complete 
8001d0120cc9: Pull complete 
a75c8d24dd23: Pull complete 
f9a1db9326c5: Pull complete 
1001e85cecac: Pull complete 
4f4fb700ef54: Pull complete 
Digest: sha256:3c6ed4b8c6bdc86bb131c802ddc35f6311cb91df7f8412f50b8f159c4c19fab0
Status: Downloaded newer image for kenchan0130/simplesamlphp:latest
310effb603ea06019dd606541a5a2d95ce87d698856f42d6f1b37ad686d14e08

Then I did the rest of the steps, which all seemed to work, until step 5: 'go in an anonymous tab to local url'. That redirected me to the SAML login page and gave me a server error. So I couldn't complete the rest:

RCheesley · 2024-05-28T14:39:28Z

@LordRembo could you please post the logs for that error so we might be able to track what's going on there? Thanks!

RCheesley · 2024-05-29T08:13:26Z

@mollux looks like we have a PHPSTAN issue to fix here and some feedback, could you take a look please? This is blocking the 5.1 release so we need to get it over the line asap.

LordRembo · 2024-05-31T06:42:04Z

Don't know if it's the full log, but here's the Mautic logs I got from that day:

[2024-05-24T14:08:53.774741+00:00] php.CRITICAL: Uncaught Error: LightSaml\SpBundle\Controller\DefaultController::__construct(): Argument #1 ($metadataProfileBuilder) not passed {"exception":"[object] (ArgumentCountError(code: 0): LightSaml\\SpBundle\\Controller\\DefaultController::__construct(): Argument #1 ($metadataProfileBuilder) not passed at /var/www/html/vendor/lightsaml/sp-bundle/src/LightSaml/SpBundle/Controller/DefaultController.php:29)"} {"hostname":"mautic-web","pid":2461}
[2024-05-24T14:11:59.256257+00:00] php.CRITICAL: Uncaught Error: LightSaml\SpBundle\Controller\DefaultController::__construct(): Argument #1 ($metadataProfileBuilder) not passed {"exception":"[object] (ArgumentCountError(code: 0): LightSaml\\SpBundle\\Controller\\DefaultController::__construct(): Argument #1 ($metadataProfileBuilder) not passed at /var/www/html/vendor/lightsaml/sp-bundle/src/LightSaml/SpBundle/Controller/DefaultController.php:29)"} {"hostname":"mautic-web","pid":2461}
[2024-05-24T14:16:57.590913+00:00] php.CRITICAL: Uncaught Error: LightSaml\SpBundle\Controller\DefaultController::__construct(): Argument #1 ($metadataProfileBuilder) not passed {"exception":"[object] (ArgumentCountError(code: 0): LightSaml\\SpBundle\\Controller\\DefaultController::__construct(): Argument #1 ($metadataProfileBuilder) not passed at /var/www/html/vendor/lightsaml/sp-bundle/src/LightSaml/SpBundle/Controller/DefaultController.php:29)"} {"hostname":"mautic-web","pid":2462}
[2024-05-24T14:18:41.300521+00:00] php.CRITICAL: Uncaught Error: LightSaml\SpBundle\Controller\DefaultController::__construct(): Argument #1 ($metadataProfileBuilder) not passed {"exception":"[object] (ArgumentCountError(code: 0): LightSaml\\SpBundle\\Controller\\DefaultController::__construct(): Argument #1 ($metadataProfileBuilder) not passed at /var/www/html/vendor/lightsaml/sp-bundle/src/LightSaml/SpBundle/Controller/DefaultController.php:29)"} {"hostname":"mautic-web","pid":2461}
[2024-05-24T14:20:07.071170+00:00] php.CRITICAL: Uncaught Error: LightSaml\SpBundle\Controller\DefaultController::__construct(): Argument #1 ($metadataProfileBuilder) not passed {"exception":"[object] (ArgumentCountError(code: 0): LightSaml\\SpBundle\\Controller\\DefaultController::__construct(): Argument #1 ($metadataProfileBuilder) not passed at /var/www/html/vendor/lightsaml/sp-bundle/src/LightSaml/SpBundle/Controller/DefaultController.php:29)"} {"hostname":"mautic-web","pid":2460}
[2024-05-24T14:20:15.221356+00:00] php.CRITICAL: Uncaught Error: LightSaml\SpBundle\Controller\DefaultController::__construct(): Argument #1 ($metadataProfileBuilder) not passed {"exception":"[object] (ArgumentCountError(code: 0): LightSaml\\SpBundle\\Controller\\DefaultController::__construct(): Argument #1 ($metadataProfileBuilder) not passed at /var/www/html/vendor/lightsaml/sp-bundle/src/LightSaml/SpBundle/Controller/DefaultController.php:29)"} {"hostname":"mautic-web","pid":2461}

RCheesley

I got to the login bit with the SAML screen:

but then had an error:

Unknown InResponseTo '_46768a6bfe387a488cccaf9c43b1ac33c1d9637919'

It seemed like it wasn't logged in, but then when I went back using the back button on the browser, it was actually logged in.

Now I can't reproduce that ... so it's ... a bit weird!

RCheesley · 2024-05-31T12:57:49Z

screencast-bpconcjcammlapcogcnnelfmaeghhagj-2024.05.31-13_56_24.webm

Here's a screencast. I get to the Mautic login screen with the error, then I use the back button (on my mouse) and it goes back to the SAML (URL is http://localhost:8080/simplesaml/saml2/idp/SSOService.php?spentityid=https%3A%2F%2Fmautic.ddev.site&cookieTime=1717160195) and then logs into Mautic.

…n be passed in a session

mollux · 2024-05-31T13:13:09Z

@RCheesley that's another SAML issue, not related to the broken M5 support (see also InResponseTo in the forums)
I agree it's an issue, but let's make it a separate issue.

This PR fixes the code compatibility with M5 and the lightsaml dependency

RCheesley

Based on the 'InResponseTo' error being a known existing bug then this works as expected!

mollux · 2024-05-31T13:23:15Z

next step is to merge Spbundle, update composer dependencies, and adapt this PR, doing that now

…s been merged

nick-vanpraet

The code makes sense, uses a specific AuthenticationException that does not serialize unneeded data. Looks good to me!

mollux · 2024-05-31T13:43:50Z

re-running failed codecov report test

RCheesley

Retested with latest commits, works as expected (with the existing error earlier reported)

RCheesley · 2024-05-31T16:27:55Z

8.1 test failure was due to rate limits on codecov - passing now.

switch lightsaml/sp-bundle to temp branch to be able to test out patch

6b518d4

RCheesley mentioned this pull request May 12, 2024

SAML Authentication not working #13361

Open

1 task

RCheesley added this to the 5.1.0 milestone May 28, 2024

RCheesley added T3 Hard difficulty to fix (issue) or test (PR) bug Issues or PR's relating to bugs configuration Anything related to the Mautic configuration section labels May 28, 2024

RCheesley requested a review from fedys May 28, 2024 14:45

RCheesley reviewed May 31, 2024

View reviewed changes

Convert LightSamlException to AuthenticationException to ensure it ca…

57a28cf

…n be passed in a session

mollux force-pushed the fix/saml-integration branch from 0490b6a to 57a28cf Compare May 31, 2024 13:10

RCheesley previously approved these changes May 31, 2024

View reviewed changes

RCheesley added pending-test-confirmation PR's that require one test before they can be merged code-review-needed PR's that require a code review before merging labels May 31, 2024

change lightsaml/sp-bundle back to original version now the change ha…

767d090

…s been merged

mollux dismissed RCheesley’s stale review via 767d090 May 31, 2024 13:28

nick-vanpraet approved these changes May 31, 2024

View reviewed changes

mollux added code-review-passed PRs which have passed code review and removed code-review-needed PR's that require a code review before merging labels May 31, 2024

RCheesley approved these changes May 31, 2024

View reviewed changes

Merge branch '5.x' into fix/saml-integration

7e0e600

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/saml integration #13742

Fix/saml integration #13742

mollux commented May 12, 2024

codecov bot commented May 12, 2024 •

edited

mallezie commented May 24, 2024

LordRembo commented May 24, 2024 •

edited

RCheesley commented May 28, 2024

RCheesley commented May 29, 2024

LordRembo commented May 31, 2024

RCheesley left a comment

RCheesley commented May 31, 2024

mollux commented May 31, 2024

RCheesley left a comment

mollux commented May 31, 2024

nick-vanpraet left a comment

mollux commented May 31, 2024

RCheesley left a comment

RCheesley commented May 31, 2024 •

edited

Fix/saml integration #13742

Are you sure you want to change the base?

Fix/saml integration #13742

Conversation

mollux commented May 12, 2024

Description:

Steps to test this PR:

codecov bot commented May 12, 2024 • edited

Codecov Report

mallezie commented May 24, 2024

LordRembo commented May 24, 2024 • edited

RCheesley commented May 28, 2024

RCheesley commented May 29, 2024

LordRembo commented May 31, 2024

RCheesley left a comment

Choose a reason for hiding this comment

RCheesley commented May 31, 2024

mollux commented May 31, 2024

RCheesley left a comment

Choose a reason for hiding this comment

mollux commented May 31, 2024

nick-vanpraet left a comment

Choose a reason for hiding this comment

mollux commented May 31, 2024

RCheesley left a comment

Choose a reason for hiding this comment

RCheesley commented May 31, 2024 • edited

codecov bot commented May 12, 2024 •

edited

LordRembo commented May 24, 2024 •

edited

RCheesley commented May 31, 2024 •

edited