[Security] Fix SwitchUser functionality to work with non-latin usernames #2908

Closed
wants to merge 1 commit into
from

Projects

None yet

3 participants

@stloyd
Contributor
stloyd commented Dec 17, 2011

Bug fix: yes
Feature addition: no
Backwards compatibility break: no
Symfony2 tests pass: yes
Fixes the following tickets: #2899

@stof stof and 2 others commented on an outdated diff Dec 17, 2011
...mponent/Security/Http/Firewall/SwitchUserListener.php
@@ -116,7 +116,12 @@ private function attemptSwitchUser(Request $request)
throw new AccessDeniedException();
}
- $username = $request->get($this->usernameParameter);
+ $username = urldecode($request->get($this->usernameParameter));
+ if (function_exists('mb_convert_encoding')) {
+ $username = mb_convert_encoding($username, 'UTF-8');
+ } else if (function_exists('iconv')) {
@stof
stof Dec 17, 2011 Member

should be elseif without space

@stloyd
stloyd Dec 17, 2011 Contributor

Not really, please check other files, also I don't find anything about this in Symfony2 CS.

@stof
stof Dec 17, 2011 Member

then this file does not follow the CS properly. But the review on previous pull requests asked to use elseif.

And some other things may be missing from this page too. It needs to be updated

@stloyd
stloyd Dec 17, 2011 Contributor

Then for me there is no issue, as there is no info about this in publicly accessible documentation.

Let's skip this until we have an public full CS which contributors will follow, then we will update check_cs file.

@stof
stof Dec 17, 2011 Member

the check_cs file is not meant to validate all CS anyway as it would make it far too complex. Opensky has a PHPCS configuration matching most of the Symfony CS (not sure it is exactly all rules though)

@stloyd
stloyd Dec 17, 2011 Contributor

I know what check_cs do, also know about PHPCS file which follow official documentation.

@stof
stof Dec 17, 2011 Member

last time I looked at the phpcs configuration, it was not following it exactly.

@fabpot
fabpot Dec 18, 2011 Member

I've updated the check_cs script to enforce usage of elseif (see 8d7a6e5).

@stloyd
Contributor
stloyd commented Dec 19, 2011

@fabpot @stof Rebased and CS fixed. Is there any other problem with this before merge ?

@fabpot
Member
fabpot commented Jan 2, 2012

I don't understand why we need to convert the encoding. And anyway, we cannot assume that the current encoding is UTF-8.

@stloyd stloyd closed this Mar 30, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment