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-8356: Removed Ibexa\Core\MVC\Symfony\Security\Authentication\AuthenticatorInterface to be replaced with Symfony-based authentication #375

Merged
merged 3 commits into from
Jul 1, 2024

Conversation

konradoboza
Copy link
Contributor

@konradoboza konradoboza commented Jun 4, 2024

🎫 Issue IBX-8356

Description:

Should be tested together with:
ibexa/recipes-dev#124
ibexa/graphql#67

Mentioned interface relies on deprecated security concepts ergo can be safely removed. The only places where it's built upon is JWT for REST and GraphQL. Since both of those authentication mechanisms were reworked, we can proceed with ibexa/core changes.

Besides simple removal of the interface I added src/lib/MVC/Symfony/Security/Authentication/EventSubscriber/OnAuthenticationTokenCreatedRepositoryUserSubscriber.php which strips redundancy whenever we need to have repository user set (stateless calls validating JWT for GraphQL, JWT for REST, RestAuthenticator and coming from ibexa/core - DefaultSuccessHandler).

TODO:

  • provide subscriber unit test.

For QA:

Sanities are basically all we need to track down potential usages I might be missing at this point.

Documentation:

Removal of AuthenticatorInterface should be mentioned in the doc.

@konradoboza konradoboza force-pushed the ibx-8356-removed-authenticatorinterface branch from 89441ae to 05dc0dd Compare June 4, 2024 13:12
@konradoboza konradoboza changed the title IBX-8356: Removed Ibexa\Core\MVC\Symfony\Security\Authentication\AuthenticatorInterface to be replaced with Symfony-based authorization IBX-8356: Removed Ibexa\Core\MVC\Symfony\Security\Authentication\AuthenticatorInterface to be replaced with Symfony-based authentication Jun 4, 2024
@konradoboza konradoboza marked this pull request as ready for review June 4, 2024 13:50
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

@konradoboza
Copy link
Contributor Author

konradoboza commented Jun 5, 2024

I'm assuming this can be merged after or with merging

@alongosz have you heard what they say about assuming? 😉

This PR is a next step of our authentication-switch journey which is about re-assessing and reworking all the code relying on the mentioned interface (see attached ticket). I opened this PR just to check if ibexa/core has any more usages that I missed and see if CI here needs any further adjustments. I am perfectly aware of the other repositories that are still in use of AuthenticatorInterface...

I am a bit surprised you already reviewed this one - marking it as work in progress to avoid more premature reviews.

@konradoboza konradoboza changed the title IBX-8356: Removed Ibexa\Core\MVC\Symfony\Security\Authentication\AuthenticatorInterface to be replaced with Symfony-based authentication [WiP] IBX-8356: Removed Ibexa\Core\MVC\Symfony\Security\Authentication\AuthenticatorInterface to be replaced with Symfony-based authentication Jun 5, 2024
@konradoboza konradoboza marked this pull request as draft June 5, 2024 06:16
@konradoboza konradoboza added the Doc needed The changes require some documentation label Jun 5, 2024
@konradoboza konradoboza marked this pull request as ready for review June 13, 2024 06:37
@konradoboza konradoboza changed the title [WiP] IBX-8356: Removed Ibexa\Core\MVC\Symfony\Security\Authentication\AuthenticatorInterface to be replaced with Symfony-based authentication IBX-8356: Removed Ibexa\Core\MVC\Symfony\Security\Authentication\AuthenticatorInterface to be replaced with Symfony-based authentication Jun 13, 2024
Copy link
Contributor Author

@konradoboza konradoboza left a comment

Choose a reason for hiding this comment

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

Note to myself: this interface needs to be marked as deprecated in 4.6.
#387

…henticatorInterface` to be replaced with Symfony-based authorization
@konradoboza konradoboza force-pushed the ibx-8356-removed-authenticatorinterface branch from 05dc0dd to 6572a91 Compare June 19, 2024 11:45
Copy link
Member

@adamwojs adamwojs left a comment

Choose a reason for hiding this comment

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

Please remember to cover this change in upgrade guide.

@konradoboza konradoboza requested review from adamwojs, alongosz and a team June 25, 2024 13:35
@konradoboza konradoboza requested review from Steveb-p and webhdx June 25, 2024 14:05
Copy link

@webhdx webhdx requested a review from a team June 26, 2024 06:14
@micszo micszo self-assigned this Jun 26, 2024
@alongosz alongosz requested a review from a team June 26, 2024 08:45
@micszo micszo removed their assignment Jul 1, 2024
@Steveb-p Steveb-p merged commit 62e04b2 into main Jul 1, 2024
14 checks passed
@Steveb-p Steveb-p deleted the ibx-8356-removed-authenticatorinterface branch July 1, 2024 11:05
barw4 pushed a commit that referenced this pull request Oct 17, 2024
…henticatorInterface` to be replaced with Symfony-based authentication

#375
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 QA approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants