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

IBX-8140: Enabled authenticator manager-based security #368

Merged

Conversation

konradoboza
Copy link
Contributor

@konradoboza konradoboza commented May 9, 2024

🎫 Issue IBX-8140

Disclaimer:

This PR allows Ibexa DXP to keep most basic security-related features in-tact with the new mechanism enabled. Most of the features around authorization needs to be at least revisited and in some cases re-implemented. They might stop working after merging this PR and will be addressed per-case-basis.

Technical details:

Main changes needed to handle the new security approach which is relying on authenticators and lack of the anonymous user.

In the nutshell:

  • I get rid of implementations of security.authentication.provider.dao and security.authentication.provider.anonymous providers coming from Symfony which are already removed, so the code from SecurityPass is not executed after enabling enable_authenticator_manager flag,
  • I remove deprecated HttpBasicFactory which doesn't seem to be needed anymore,
  • I suggest removing very old Ibexa\Core\MVC\Symfony\Security\EventListener\SecurityListener which contains some blurry logic that can and imho should be replaced by the new ways of doing security. However, we need to evaluate necessity and possible replacement for Ibexa\Core\MVC\Symfony\Event\InteractiveLoginEvent,
  • I updated the code of src/lib/MVC/Symfony/Security/Authentication/DefaultAuthenticationSuccessHandler.php which extends the built-in Symfony handler. It proves to be useful especially due to logic needed for proper redirection coming from parent Symfony\Component\Security\Http\Authentication\DefaultAuthenticationSuccessHandler::onAuthenticationSuccess method. I supplied it with proper setting repository user and emitting DetermineTargetUrlEvent which is needed e.g. for Dashboard redirection after successful authorization to the Back Office,
  • I implemented basic src/lib/MVC/Symfony/Security/Authentication/EventSubscriber/AccessDeniedListener.php according to https://symfony.com/doc/5.x/security/access_denied_handler.html#customizing-all-access-denied-responses. The is still an option to implement some more custom logic with usage of firewall options: entry_point and access_denied_handler for forbidden resources,
  • I added PasswordAuthenticatedUserInterface implementation to the src/lib/MVC/Symfony/Security/User class to meet the Symfony requirement of enabling password hashers:
password_hashers:
    Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface: 'auto'

it just boils down to adding a strict type to public function getPassword(): ?string which doesn't seem to have any impact whatsoever,

  • I added some failsafe to src/lib/MVC/Symfony/SiteAccess/Router.php::matchByName since its toString method produces some noise when accessing siteaccess name. I will leave it as-is for now until some more visibility on this topic is there,
  • I added some strict type on the occassion of modifying some of the files as well as removed some class_alias occurrences,
  • I adjusted tests/lib/MVC/Symfony/Security/Authentication/RememberMeRepositoryAuthenticationProviderTest.php to not rely on the deprecated method getUsername. The whole "Remember Me" feature however, needs to be also rewritten to rely on the new authenticator mechanisms, since the old fashion is mostly deprecated already.

Known issues:

  • targetting admin/dashboard directly without being logged-in produces an error (possibly, missing access check in the ibexa/dashboard package,
  • constant time authentication might be broken,
  • checking https://haveibeenpwned.com/ leaks should be re-implemented,
  • "Remember Me" and GuardRepositoryAuthenticationProvider needs to be re-implemented,
  • some CI failing scenarios need to be fixed (wasn't able to reproduce them at this point).

Needs revisiting:

  • HttpBasicFactory,
  • login throttling,
  • assess removing InteractiveLoginEvent,
  • how AccessDeniedListener behaves when permissions are unsufficient,
  • check what if there is no user/login permissions.

Related PRs:

Description:

For QA:

Documentation:

@konradoboza konradoboza force-pushed the ibx-8140-enable-authorization-with-new-authenticator-mechanisms branch from 0f2c44c to 261b681 Compare May 9, 2024 13:15
@konradoboza konradoboza force-pushed the ibx-8140-enable-authorization-with-new-authenticator-mechanisms branch from 261b681 to 1eaa9f0 Compare May 10, 2024 07:58
@konradoboza konradoboza marked this pull request as ready for review May 14, 2024 12:27
@konradoboza konradoboza added Feature New feature request Ready for review labels May 14, 2024
@konradoboza konradoboza requested a review from a team May 14, 2024 13:51
@konradoboza konradoboza changed the title IBX-8140: Enabled authenticator manager-based security [REMOVE DEV DEPENDENCY] IBX-8140: Enabled authenticator manager-based security May 20, 2024
@konradoboza konradoboza force-pushed the ibx-8140-enable-authorization-with-new-authenticator-mechanisms branch from d79da4b to edce935 Compare May 20, 2024 07:48
@konradoboza
Copy link
Contributor Author

@Steveb-p resolved via ceceb3f. Since the original listener definition was overridden within src/bundle/Core/DependencyInjection/Compiler/SecurityPass.php and it was marked as abstract by Symfony itself, I needed to change the configuration of our service accordingly. I also removed tests/lib/MVC/Symfony/Security/Authentication/DefaultAuthenticationSuccessHandlerTest.php entirely since it brings close to zero value in the current shape.

There is a slight change we can stop decorating the original listener in the next iterations. However, til this point I didn't find a way to hook into our SA resolving mechanism in the context of authorization using custom authenticator.

@konradoboza konradoboza requested review from Steveb-p and a team May 20, 2024 08:27
@konradoboza konradoboza force-pushed the ibx-8140-enable-authorization-with-new-authenticator-mechanisms branch from 0b61623 to b65af9a Compare May 20, 2024 09:03
@konradoboza konradoboza force-pushed the ibx-8140-enable-authorization-with-new-authenticator-mechanisms branch from b4ef1c3 to 8093540 Compare May 20, 2024 10:21
@konradoboza
Copy link
Contributor Author

After internal sync with @Steveb-p I needed to revert decorating DefaultAuthenticationSuccessHandler as it's not sufficient in our scenario. I also tried to follow dedicated access handler doc page but we need to basically override Symfony\Component\Security\Http\Authentication\DefaultAuthenticationSuccessHandler as we did til this point. The reason for that is necessity to take advantage of all the firewall options which is crucial to perform proper redirections having SA awareness in the back of our heads.

@konradoboza konradoboza requested review from Steveb-p and a team May 21, 2024 09:39
@alongosz alongosz requested a review from a team May 21, 2024 10:28
@konradoboza konradoboza requested review from adamwojs, alongosz and a team May 21, 2024 11:35
@konradoboza konradoboza requested a review from a team May 22, 2024 06:10
@konradoboza konradoboza changed the title [REMOVE DEV DEPENDENCY] IBX-8140: Enabled authenticator manager-based security IBX-8140: Enabled authenticator manager-based security May 23, 2024
Copy link

sonarcloud bot commented May 23, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@konradoboza konradoboza merged commit eeb08b4 into main May 23, 2024
14 checks passed
@konradoboza konradoboza deleted the ibx-8140-enable-authorization-with-new-authenticator-mechanisms branch May 23, 2024 12:45
@konradoboza konradoboza added the Doc needed The changes require some documentation label Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Doc needed The changes require some documentation Feature New feature request Ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants