Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/saml integration #13742

Open
wants to merge 2 commits into
base: 5.x
Choose a base branch
from
Open

Fix/saml integration #13742

wants to merge 2 commits into from

Conversation

mollux
Copy link
Contributor

@mollux mollux commented May 12, 2024

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.

  1. checkout this branch and run composer update --lock to get the changed lightsaml/sp-bundle dependency.

  2. 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
    
  3. 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).

  4. 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.

  5. 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)

  6. login with username user1 and password password

  7. you're redirected to Mautic and logged in as user1

Copy link

codecov bot commented May 12, 2024

Codecov Report

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

Project coverage is 61.51%. Comparing base (7f141f6) to head (0490b6a).
Report is 16 commits behind head on 5.x.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                5.x   #13742      +/-   ##
============================================
- Coverage     61.51%   61.51%   -0.01%     
- Complexity    34068    34069       +1     
============================================
  Files          2241     2241              
  Lines        101852   101854       +2     
============================================
  Hits          62651    62651              
- Misses        39201    39203       +2     
Files Coverage Δ
...ndles/UserBundle/Controller/SecurityController.php 42.55% <0.00%> (-1.90%) ⬇️

@mallezie
Copy link
Contributor

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
Copy link
Contributor

LordRembo commented May 24, 2024

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:
    Screenshot 2024-05-24 at 16 09 04
    Screenshot 2024-05-24 at 16 09 17

@RCheesley
Copy link
Sponsor Member

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

@RCheesley RCheesley added this to the 5.1.0 milestone May 28, 2024
@RCheesley 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 RCheesley requested a review from fedys May 28, 2024 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs configuration Anything related to the Mautic configuration section T3 Hard difficulty to fix (issue) or test (PR)
Projects
Status: 🦸🏻 Needs 2 tests
Development

Successfully merging this pull request may close these issues.

SAML Authentication not working
4 participants