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-8138: Refactored deprecated loadUserByUsername method in custom providers #400

Merged
merged 9 commits into from
Jul 23, 2024

Conversation

alongosz
Copy link
Member

@alongosz alongosz commented Jul 4, 2024

🎫 Issue IBX-8138

Related PRs:

Description:

Seems Symfony deprecated \Symfony\Component\Security\Core\User\UserProviderInterface::loadUserByUsername method in favor of loadUserByIdentifier.
A fix for that is not a straightforward rector (though there's one for that in Symfony 6.0 sets) because we have our own custom providers which would need to implement that method.

Added it here. Also seems there was custom handling of anonymous user which I beliveve changed and should be already fixed. I needed to drop that code because of Symfony interface parameter type mismatch on loadUserByUsername method (it cannot be an object).

I've also fixed some redundant WET (not DRY ;-) ) test cases.

Changelog:
  • [Tests] Dropped redundancy in EmailProvider and UsernameProvider tests
  • Aligned Username and Email providers with Symfony interface
  • [Tests] Dropped obsolete test case
  • Implemented loadUserByIdentifier in custom User providers
  • [Tests] Dropped usage of deprecated loadUserByUsername method
  • [PHPDoc] Added missing throw annotations to UserService::loadUserBy*
  • [PHPStan] Aligned baseline with the changes
  • [Tests] Reduced code duplication in custom providers test cases]

For QA:

Probably the same approach as in case of other security layers PRs.

These providers can be configured via: config/packages/security.yaml

security:
    providers:
        ibexa:
            id: ibexa.security.user_provider

By default it's ibexa.security.user_provider.username, but you can use ibexa.security.user_provider.email to test the second provider.

Regression run: ibexa/commerce#911

Documentation:

Seems no changes are required. Those are exposed as service names configurable via Symfony security bundle and documented by us here https://doc.ibexa.co/en/master/users/login_methods/. This behavior doesn't change.

@alongosz alongosz force-pushed the ibx-8138/refactor-deprecated-loadUserByUsername branch from f3108e5 to 5c5f441 Compare July 5, 2024 08:50
@alongosz alongosz marked this pull request as ready for review July 5, 2024 09:10
@Steveb-p Steveb-p requested a review from a team July 5, 2024 10:03
Co-authored-by: Paweł Niedzielski <[email protected]>
Copy link

sonarqubecloud bot commented Jul 5, 2024

@alongosz alongosz requested a review from konradoboza July 5, 2024 10:16
@konradoboza konradoboza requested a review from a team July 5, 2024 10:21
@tomaszszopinski tomaszszopinski self-assigned this Jul 22, 2024
Copy link

@tomaszszopinski tomaszszopinski left a comment

Choose a reason for hiding this comment

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

QA approved on IbexaDXP commerce 5.0

@alongosz alongosz merged commit da23f87 into main Jul 23, 2024
15 checks passed
@alongosz alongosz deleted the ibx-8138/refactor-deprecated-loadUserByUsername branch July 23, 2024 07:55
barw4 pushed a commit that referenced this pull request Oct 17, 2024
For more details see https://issues.ibexa.co/browse/IBX-8138 and #400

Key changes:

* Implemented `loadUserByIdentifier` in custom User providers

* Aligned Username and Email providers with Symfony interface

* [Tests] Dropped usage of deprecated `loadUserByUsername` method

* [Tests] Dropped redundancy in EmailProvider and UsernameProvider tests

* [PHPDoc] Added missing throw annotations to `UserService::loadUserBy*` methods

* [PHPStan] Aligned baseline with the changes

---------

Co-Authored-By: Paweł Niedzielski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants