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

[SecurityBundle] Don't normalize username of in-memory users #21718

Merged
merged 1 commit into from Feb 22, 2017

Conversation

Projects
None yet
4 participants
@chalasr
Copy link
Member

commented Feb 22, 2017

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

It's common to have e.g. emails as keys in security.providers.in_memory.users since keys are username. Actually they are normalized so foo-bar@gmail.com becomes foo_bar@gmail.com and authentication fails unexpectedly.

@fabpot

This comment has been minimized.

Copy link
Member

commented Feb 22, 2017

That's a BC break (probably just for tests in end-users apps, but still). I would document the change and merge it on master.

@chalasr chalasr force-pushed the chalasr:in-memory-normalizekeys branch from 1b4800d to 0e378ef Feb 22, 2017

@chalasr chalasr changed the base branch from 2.7 to master Feb 22, 2017

@chalasr

This comment has been minimized.

Copy link
Member Author

commented Feb 22, 2017

Change documented and rebased on master.

@stof

This comment has been minimized.

Copy link
Member

commented Feb 22, 2017

@fabpot ideally, we should skip normalized for all prototyped nodes (and even remove the explicit config), but this is a BC break.
Transforming keys selected by the user is generally a WTF moment, especially if they need to use these keys elsewhere.
The normalization was added to allow writing XML config files feeling native, but keys for prototyped nodes are inside attribute values, and so don't need to use dashes to feel native.

I don't see a way to easily disable key normalization for prototyped nodes in an fully BC way though (even though it would break BC only for people who faced the WTF moment and decided to keep a WTF config files forever rather than using an underscore explicitly)

@chalasr chalasr force-pushed the chalasr:in-memory-normalizekeys branch 2 times, most recently from cdf1908 to 313e6e8 Feb 22, 2017

@chalasr chalasr force-pushed the chalasr:in-memory-normalizekeys branch from 313e6e8 to 8d03332 Feb 22, 2017

@fabpot

This comment has been minimized.

Copy link
Member

commented Feb 22, 2017

Thank you @chalasr.

@fabpot fabpot merged commit 8d03332 into symfony:master Feb 22, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

fabpot added a commit that referenced this pull request Feb 22, 2017

feature #21718 [SecurityBundle] Don't normalize username of in-memory…
… users (chalasr)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[SecurityBundle] Don't normalize username of in-memory users

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

It's common to have e.g. emails as keys in `security.providers.in_memory.users` since keys are username. Actually they are normalized so `foo-bar@gmail.com` becomes `foo_bar@gmail.com` and authentication fails unexpectedly.

Commits
-------

8d03332 [SecurityBundle] Don't normalize keys of in-memory users

@chalasr chalasr deleted the chalasr:in-memory-normalizekeys branch Feb 22, 2017

@fabpot fabpot referenced this pull request May 1, 2017

Merged

Release v3.3.0-BETA1 #22603

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.