Skip to content

Use new authenticator system. #14219

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Dec 5, 2024
Merged

Use new authenticator system. #14219

merged 11 commits into from
Dec 5, 2024

Conversation

biozshock
Copy link
Contributor

@biozshock biozshock commented Oct 23, 2024 •
edited by RCheesley
Loading

Q A
Bug fix? (use the a.b branch) red
New feature/enhancement? (use the a.x branch) new
Deprecations? yes
BC breaks? (use the c.x branch) yes
Automated tests included? not for new functionality

Description

Remove deprecated authentication of Symfony 5. Use other forks to address the issues with current and next Symfony versions.

📋 Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Test general login
  3. Test SAML solgin - instructions for setting up a local SAML testing facility can be found here: Fix/saml integration #13742 (comment)
  4. Test API login (mainly done by tests)
  5. Test SSO login
  6. Enable saml, and check if regular login page is available. Remove a todo in \Mautic\UserBundle\Security\EntryPoint\MainEntryPoint::start

@biozshock
Copy link
Contributor Author

This one still lacks a unit test love. But you can manually test points from the description.

@codecov Codecov
Copy link

codecov bot commented Oct 23, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 83.77660% with 61 lines in your changes missing coverage. Please review.

Project coverage is 63.35%. Comparing base (38af77b) to head (60c27ac).
Report is 1 commits behind head on 6.x.

Files with missing lines Patch % Lines
...Bundle/Security/Authenticator/SsoAuthenticator.php 79.72% 15 Missing ⚠️
...le/EventListener/PreAuthorizationEventListener.php 0.00% 8 Missing ⚠️
...hentication/Token/Permissions/TokenPermissions.php 79.41% 7 Missing ⚠️
...ncyInjection/Firewall/Factory/MauticSsoFactory.php 82.85% 6 Missing ⚠️
.../UserBundle/Security/EntryPoint/MainEntryPoint.php 16.66% 5 Missing ⚠️
...piBundle/Controller/oAuth2/AuthorizeController.php 50.00% 4 Missing ⚠️
...dle/Security/Authenticator/PluginAuthenticator.php 94.52% 4 Missing ⚠️
...endencyInjection/Compiler/SsoAuthenticatorPass.php 76.92% 3 Missing ⚠️
...ndencyInjection/Firewall/Factory/PluginFactory.php 76.92% 3 Missing ⚠️
...urity/Authenticator/Passport/Badge/PluginBadge.php 77.77% 2 Missing ⚠️
... and 4 more
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                6.x   #14219      +/-   ##
============================================
+ Coverage     63.26%   63.35%   +0.09%     
- Complexity    34604    34730     +126     
============================================
  Files          2270     2282      +12     
  Lines        103503   103822     +319     
============================================
+ Hits          65485    65781     +296     
- Misses        38018    38041      +23
Files with missing lines Coverage Δ
app/bundles/ApiBundle/Entity/oAuth2/Client.php 78.20% <ø> (ø)
app/bundles/ApiBundle/MauticApiBundle.php 100.00% <ø> (ø)
...bundles/CoreBundle/Controller/CommonController.php 84.71% <100.00%> (ø)
.../bundles/CoreBundle/Loader/EnvVars/SAMLEnvVars.php 100.00% <100.00%> (ø)
...ndles/UserBundle/Controller/SecurityController.php 55.88% <100.00%> (ø)
...Bundle/DependencyInjection/MauticUserExtension.php 100.00% <100.00%> (ø)
...p/bundles/UserBundle/Event/AuthenticationEvent.php 63.79% <100.00%> (+63.79%) ⬆️
...les/UserBundle/EventListener/ApiUserSubscriber.php 100.00% <100.00%> (ø)
...undle/EventListener/PasswordStrengthSubscriber.php 100.00% <100.00%> (ø)
...undles/UserBundle/EventListener/SAMLSubscriber.php 84.61% <ø> (-15.39%) ⬇️
... and 21 more

... and 1 file with indirect coverage changes

@biozshock biozshock force-pushed the authentication-deprecations branch 2 times, most recently from c34cffe to 9bdfbd3 Compare October 23, 2024 23:58
@RCheesley RCheesley added ready-to-test PR's that are ready to test bc-break A BC break PR for major release milestones only deprecation Includes deprecations dependencies Pull requests that update a dependency file mautic-6 labels Oct 24, 2024
@RCheesley RCheesley added this to the 6.0 milestone Oct 24, 2024
@RCheesley RCheesley added essential This must be done to close the milestone authentication code-review-needed PR's that require a code review before merging needs-automated-tests PR's that need automated tests before they can be merged labels Oct 24, 2024
escopecz
escopecz reviewed Oct 24, 2024
View reviewed changes
@escopecz
Copy link
Member

I ran the API Library tests on this PR and there is only 1 test failing. That is pretty good!
https://github.com/mautic/api-library/actions/runs/11494369288/job/31991951131

@biozshock biozshock force-pushed the authentication-deprecations branch from 9bdfbd3 to 2486821 Compare October 24, 2024 21:20
@biozshock
Copy link
Contributor Author

@escopecz Seems i have found the issue. Please re-run API tests with updated code.

@biozshock biozshock force-pushed the authentication-deprecations branch 2 times, most recently from 64a6d71 to 2f192d7 Compare October 24, 2024 23:27
@escopecz
Copy link
Member

Looks the same: https://github.com/mautic/api-library/actions/runs/11513222648/job/32049535379

@biozshock biozshock force-pushed the authentication-deprecations branch 2 times, most recently from 2fdfe12 to 4d5f986 Compare October 30, 2024 00:31
@biozshock
Copy link
Contributor Author

@escopecz Please run api tests again. Thanks.

@RCheesley RCheesley added the hacktoberfest-accepted PR's that have been accepted for the purposes of Hacktoberfest label Oct 30, 2024
@escopecz
Copy link
Member

@biozshock the API Library tests are passing now 🎉 https://github.com/mautic/api-library/actions/runs/11608868885

@biozshock biozshock force-pushed the authentication-deprecations branch 4 times, most recently from 146356a to 7dcad27 Compare November 3, 2024 01:26
@RCheesley
Copy link
Member

Thanks so much @markusVJH for testing this PR and for the detailed screenshots/videos! 🚀

@RCheesley RCheesley added pending-test-confirmation PR's that require one test before they can be merged and removed ready-to-test PR's that are ready to test labels Nov 19, 2024
@RCheesley RCheesley mentioned this pull request Nov 19, 2024
Closed
@mautibot
Copy link
Contributor

This pull request has been mentioned on Mautic Forums. There might be relevant details there:

https://forum.mautic.org/t/your-opportunity-to-give-something-back-to-mautic-open-source-friday/34200/1

@escopecz
Copy link
Member

escopecz commented Dec 1, 2024

@biozshock could you please resolve the conflict? Also rect to the comment where the small issue in SAML is shown on the screen recording?

@biozshock biozshock force-pushed the authentication-deprecations branch from 4ef3cdd to 1a1ca53 Compare December 4, 2024 21:52
@biozshock biozshock force-pushed the authentication-deprecations branch from 1a1ca53 to 53005b6 Compare December 4, 2024 22:05
@biozshock biozshock force-pushed the authentication-deprecations branch from 53005b6 to 60c27ac Compare December 4, 2024 22:08
@biozshock
Copy link
Contributor Author

@escopecz The conflict is fixed.
I'm not really sure what i should address in the comment you mentioned.
If that's a known issue #13808 then i'm working on it next.

@escopecz escopecz requested a review from fedys December 5, 2024 08:28
escopecz
escopecz approved these changes Dec 5, 2024
View reviewed changes
Copy link
Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some error visible in one of the videos but it's not blocking. I'm merging this to move forward. We can fix that later on.

Thank you for this!

@escopecz escopecz added code-review-passed PRs which have passed code review and removed code-review-needed PR's that require a code review before merging pending-test-confirmation PR's that require one test before they can be merged labels Dec 5, 2024
@escopecz escopecz merged commit e7a1ae5 into mautic:6.x Dec 5, 2024
16 of 17 checks passed
@mautibot
Copy link
Contributor

This pull request has been mentioned on Mautic Forums. There might be relevant details there:

https://forum.mautic.org/t/mautic-6-is-here-alpha-release-is-ready-for-you-to-start-testing/34874/1

@mautibot
Copy link
Contributor

This pull request has been mentioned on Mautic Forums. There might be relevant details there:

https://forum.mautic.org/t/mautic-6-is-here-alpha-release-is-ready-for-you-to-start-testing/34900/1

@escopecz escopecz mentioned this pull request Apr 30, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@escopecz escopecz escopecz approved these changes

@fedys fedys Awaiting requested review from fedys

Assignees
No one assigned
Labels
authentication bc-break A BC break PR for major release milestones only code-review-passed PRs which have passed code review dependencies Pull requests that update a dependency file deprecation Includes deprecations essential This must be done to close the milestone hacktoberfest-accepted PR's that have been accepted for the purposes of Hacktoberfest mautic-6
Projects
Open Source Fridays
Archived in project
Milestone
6.0.0-alpha
Development

Successfully merging this pull request may close these issues.
6 participants
@biozshock @escopecz @RCheesley @markusVJH @mautibot @fedys