Skip to content

[SecurityBundle] Comma separated ips for security.access_control #38149

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

Merged
merged 1 commit into from
Sep 12, 2020

Conversation

a-menshchikov
Copy link
Contributor

@a-menshchikov a-menshchikov commented Sep 10, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets
License MIT
Doc PR symfony/symfony-docs#14219

There is currently no way to use env vars to configure security.access_control ips with multiple values. Ability to use comma separated ips make it able.

@nicolas-grekas nicolas-grekas added this to the next milestone Sep 11, 2020
@nicolas-grekas nicolas-grekas changed the title Comma separated ips for security.access_control [SecurityBundle] Comma separated ips for security.access_control Sep 11, 2020
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Looks good to me, I don't think there is any downside to this.

$ips = null !== $ips ? (array) $ips : [];

$this->ips = array_reduce($ips, static function (array $ips, string $ip) {
return array_merge($ips, preg_split('/\s*,\s*/', $ip));
Copy link
Member

Choose a reason for hiding this comment

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

for next reviewers, this is the core of the PR

Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

With minor comment

@fabpot fabpot force-pushed the comma_separated_ips branch from a3e3e40 to 0412e91 Compare September 12, 2020 05:06
@fabpot
Copy link
Member

fabpot commented Sep 12, 2020

Thank you @a-menshchikov.

@fabpot fabpot merged commit 11b1429 into symfony:master Sep 12, 2020
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
@a-menshchikov a-menshchikov deleted the comma_separated_ips branch March 4, 2021 15:51
nicolas-grekas added a commit that referenced this pull request May 7, 2021
… comma-separated string (edefimov)

This PR was merged into the 5.2 branch.

Discussion
----------

[Security] Allow ips parameter in access_control to accept comma-separated string

| Q             | A
| ------------- | ---
| Branch?       | 5.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | #40881, #40864, #40865
| License       | MIT

PR #38149 introduced a new feature to accept a comma-separated string in ip adresses setting in `access_control` configuration  section of security bundle.

However the feature works in inconsistent manner: comma-separated string can be successfully passed via environment variable, but can not be passed as plain string. This PR changes this inconsistent behavior by allowing validation pass if comma-separated list of ip addresses is given in plain string.

More detailed explanation about the inconsistent behavior can be found [here](#40881 (comment))

Commits
-------

8947482 [SecurityBundle] Allow ips parameter in access_control accept comma-separated string
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.

5 participants