Skip to content

[Security] Add an easier way to get the current firewall configuration #46066

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 1 commit into from
Jun 7, 2022
Merged

[Security] Add an easier way to get the current firewall configuration #46066

merged 1 commit into from
Jun 7, 2022

Conversation

Kocal
Copy link
Member

@Kocal Kocal commented Apr 16, 2022 •
edited
Loading

Q A
Branch? 6.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets Fix #46015
License MIT
Doc PR symfony/symfony-docs#...

Added a new method Security#getFirewallConfig($request) to easily get the firewall configuration associated to the Request.

The firewall name can be accessed through $security->getFirewallConfig($request)?->getName().

@Kocal Kocal requested review from wouterj and chalasr as code owners April 16, 2022 14:41
@carsonbot carsonbot added this to the 6.1 milestone Apr 16, 2022
@Kocal
Copy link
Member Author

Kocal commented Apr 16, 2022

What's the best way to make the low-deps check passing?

@Kocal Kocal requested a review from chalasr April 16, 2022 21:03
@wouterj
Copy link
Member

wouterj commented Apr 16, 2022 •
edited
Loading

What's the best way to make the low-deps check passing?

By requiring ^6.2 in the composer.json (or skipping the test on lower versions).

However, this does signal the design flaw of the current implementation (which is a flaw with the Security class as a whole, but highlighted more by this change): We now have a class in Security\Core that depends on a FirewallMap and FirewallConfig class from SecurityBundle, which depend on a FirewallMap from Security\Http. Imho, that sadly is a no go, so 👎 for the moment.

Please note that this is purely a -1 for the implementation, I think the DX problem highlighted in the issue is worth fixing.

@chalasr this is the second time we have a nice DX improvement that introduces lots of coupling through the Security helper class (ref #41274 (comment)). What do you think about deprecating this helper class in Security\Core and moving it completely to the SecurityBundle? That directly resolves coupling issues, and only reduces DX for standalone users (all methods in the class are little helpers to retrieve information more easily, none add missing functionality), which - to be honest - already suffer from a bad DX. And given the currently coupling to SecurityBundle service names in the class, I don't expect standalone users are using the class at the moment.

@Kocal
Copy link
Member Author

Kocal commented Apr 16, 2022 •
edited
Loading

Having the Security class in the SecurityBundle makes more sense to me, I was dubitative when working on the feature.

@chalasr
Copy link
Member

chalasr commented Apr 16, 2022

Big 👍 for moving the helper to SecurityBundle.

@chalasr chalasr modified the milestones: 6.1, 6.2 Apr 17, 2022
@chalasr chalasr mentioned this pull request Apr 17, 2022
Merged
@fabpot fabpot closed this in 0140746 Jun 5, 2022
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request Jun 5, 2022
@fabpot
feature #46094 [Security][SecurityBundle] Move the Security helper …
bd17d92 
…to SecurityBundle (chalasr)

This PR was merged into the 6.2 branch.

Discussion
----------

[Security][SecurityBundle] Move the `Security` helper to SecurityBundle

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | yes
| Tickets       | Fixes symfony/symfony#46066 (comment)
| License       | MIT
| Doc PR        | todo

The `Security` helper is a high-level service providing an easy access to commonly-needed features coming from various low-level abstractions. Basically, it's a facade.
Based on this, it makes sense to me to make it available only via the full-stack framework, as proposed by Wouter in symfony/symfony#46066 (comment).

This unlocks #46066, symfony/symfony#41274 and symfony/symfony#41406.
/cc @wouterj @johnkrovitch @Kocal

Commits
-------

7b91bcb068 [Security] Move the `Security` helper to SecurityBundle
@wouterj wouterj reopened this Jun 5, 2022
@wouterj
Copy link
Member

wouterj commented Jun 5, 2022

The merge tool was a bit trigger happy closing this PR when #46094 got merged, reopening.

@Kocal do you have time to rebash and finish this PR for 6.2, now the helper is moved to the security bundle?

@Kocal
Copy link
Member Author

Kocal commented Jun 5, 2022 •
edited
Loading

I'm glad that #46094 has been merged, nice work @chalasr :)

@wouterj the PR has been rebased and modified in consequence.

wouterj
wouterj approved these changes Jun 6, 2022
View reviewed changes
Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

I like it, thanks @Kocal!

@fabpot
Copy link
Member

fabpot commented Jun 7, 2022

Thank you @Kocal.

@fabpot fabpot mentioned this pull request Jun 7, 2022
Closed
@fabpot fabpot merged commit 1e85fba into symfony:6.2 Jun 7, 2022
@alexander-schranz
Copy link
Contributor

@Kocal Nice work. Thx for implementing this 👍

chalasr
chalasr reviewed Jun 7, 2022
View reviewed changes
src/Symfony/Bundle/SecurityBundle/Tests/Security/SecurityTest.php

$container = $this->createContainer('security.firewall.map', $firewallMap);

$security = new \Symfony\Component\Security\Core\Security($container);
Copy link
Member

Choose a reason for hiding this comment

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

Bad rebase :) See #46619

Copy link
Member Author

Choose a reason for hiding this comment

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

Thx!

@Kocal Kocal deleted the feat/GH-46015 branch June 8, 2022 09:11
@fabpot fabpot mentioned this pull request Oct 24, 2022
Merged
symfony-splitter pushed a commit to symfony/framework-bundle that referenced this pull request Jul 28, 2023
@fabpot
feature #46094 [Security][SecurityBundle] Move the Security helper …
c3dfdd2 
…to SecurityBundle (chalasr)

This PR was merged into the 6.2 branch.

Discussion
----------

[Security][SecurityBundle] Move the `Security` helper to SecurityBundle

| Q             | A
| ------------- | ---
| Branch?       | 6.2
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | yes
| Tickets       | Fixes symfony/symfony#46066 (comment)
| License       | MIT
| Doc PR        | todo

The `Security` helper is a high-level service providing an easy access to commonly-needed features coming from various low-level abstractions. Basically, it's a facade.
Based on this, it makes sense to me to make it available only via the full-stack framework, as proposed by Wouter in symfony/symfony#46066 (comment).

This unlocks #46066, symfony/symfony#41274 and symfony/symfony#41406.
/cc @wouterj @johnkrovitch @Kocal

Commits
-------

7b91bcb068 [Security] Move the `Security` helper to SecurityBundle
@Hanmac Hanmac mentioned this pull request Apr 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chalasr chalasr chalasr left review comments

@fabpot fabpot fabpot approved these changes

@wouterj wouterj wouterj approved these changes

Assignees
No one assigned
Labels
Feature Security Status: Reviewed
Projects
None yet
Milestone
6.2
Development

Successfully merging this pull request may close these issues.

[Security] Add an easier way to get the current firewall name
6 participants
@Kocal @wouterj @chalasr @fabpot @alexander-schranz @carsonbot