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

Add firewall routes configuration #31485

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@MatTheCat
Copy link
Contributor

commented May 12, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #31362
License MIT
Doc PR no doc PR yet

Implementation of #31362 (comment)

@MatTheCat MatTheCat force-pushed the MatTheCat:ticket_31362 branch from 870c693 to 669a814 May 12, 2019

@MatTheCat MatTheCat force-pushed the MatTheCat:ticket_31362 branch 4 times, most recently from 9f62b42 to 48a788e May 13, 2019

@MatTheCat MatTheCat marked this pull request as ready for review May 13, 2019

@MatTheCat MatTheCat force-pushed the MatTheCat:ticket_31362 branch from 48a788e to 19c238f May 13, 2019

@MatTheCat

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

Uh https://travis-ci.org/symfony/symfony/jobs/531801914 fails because assertResponseStatusCodeSame does not exist. Am I supposed not to use it?

@linaori

This comment has been minimized.

Copy link
Contributor

commented May 14, 2019

I can see the reason as of why this configuration method can be preferred over url matching. I'm a bit afraid that this list might end up rather big and that the security.yaml will have the majority of lines containing this particular config, making it harder to maintain.

Would it be an idea to add pattern matching for route names? For example: app.foo.* would match everything starting with app.foo.. This is a crude example and might be a bit difficult to properly realize with regexes because of the . in names being allowed.

@MatTheCat

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

I was afraid of that. Plus changing a route name would mean changing it in these lists too. Are you sure we cannot have the firewall configured in the route instead? 😄

@nicolas-grekas

This comment has been minimized.

Copy link
Member

commented May 20, 2019

What about matching a route parameter instead of its name?

@MatTheCat

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

@nicolas-grekas you mean I could implement #31362 (comment) like

firewalls:
    test:
        route_params:
            _firewall: test

?

@MatTheCat MatTheCat force-pushed the MatTheCat:ticket_31362 branch from 19c238f to 8879179 May 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.