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-5705: Added support for authentication via GuardAuthenticationProvider #244

Merged
merged 4 commits into from
Jul 3, 2023

Conversation

Nattfarinn
Copy link
Contributor

Question Answer
JIRA issue IBX-5705
Type improvement
Target Ibexa version v4.6
BC breaks no

Required by OAuth2 Authentication. Without it even if we successfully authenticate user with JWT token on Symfony security layer, we need to provide user for our PermissionResolver.

Checklist:

  • Provided PR description.
  • Tested the solution manually.
  • Provided automated test coverage.
  • Checked that target branch is set correctly (main for features, the oldest supported for bugs).
  • Ran PHP CS Fixer for new PHP code (use $ composer fix-cs).
  • Asked for a review (ping @ibexa/engineering).

@@ -73,6 +74,13 @@ public function process(ContainerBuilder $container)
[$permissionResolverRef]
);

$guardAuthenticationProviderDef = $container->findDefinition('security.authentication.provider.guard');
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you can possibly decorate this service instead of extending it.

Copy link
Contributor Author

@Nattfarinn Nattfarinn Jun 30, 2023

Choose a reason for hiding this comment

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

This is the Way of the Core. Or we can consider it as a tribute to Andre.

But sure, we could. WDYT @alongosz ?

As for my opinion, I think it is not perfect but it is good to have all security services defined in one place. Less prone to error that could otherwise be potentially catastrophic.

Copy link
Member

Choose a reason for hiding this comment

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

This is the Way of the Core. Or we can consider it as a tribute to Andre.

This is the way of legacy.

But sure, we could. WDYT @alongosz ?

As for my opinion, I think it is not perfect but it is good to have all security services defined in one place. Less prone to error that could otherwise be potentially catastrophic.

The issue is that the way it works now is a technical debt. If you can decorate it, please do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Nattfarinn Nattfarinn requested a review from webhdx July 3, 2023 10:04
@sonarcloud
Copy link

sonarcloud bot commented Jul 3, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@alongosz alongosz merged commit be03c8e into main Jul 3, 2023
22 checks passed
@alongosz alongosz deleted the ibx-5705-guard-authentication-provider branch July 3, 2023 12:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants