Trim username
-
is there any reason you can think of, why the username post-var of CP isn't trimmed?
I'm running CP with AD/radius & highschool students are entering leading or trailing spaces in their usernames. this results in 'invalid credentials'.
==> i get the complaint, have to browse the radius logs to find out why & it's almost always a whitespace at the beginning or end of the username.If nobody can think of a reason why trim shouldn't be used, then i'll (attempt to) create a PR.
i guess it would only take a few modifications around here:
https://github.com/pfsense/pfsense/blob/master/src/usr/local/captiveportal/index.php#L185$user = trim($_POST['auth_user']); //same for auth_user2at first glance,i think code change above would probably suffice.
looking forward to your feedback
-
@heper said in Trim username:
is there any reason you can think of, why the username post-var of CP isn't trimmed?
Yes.
It's very important that people learn, if needed, the hard way, that on electronic system white spaces are characters.
Added to that : double clicking on a word (with the mouse) will select only that word excluding white space characters (and often other signs like dashes on the outside of the word'). It's important to know that what you copy is really what you want to copy (and paste).The subject is know, and people that are subjected to those "login screen that won't pass them through" will find this issue with most Internet sites.
And a white space can be a valid character in a password.
But ...
@heper said in Trim username:
$user = trim($_POST['auth_user']); //same for auth_user2
of course you are right. I even did exactly the same thing, some years ago. Not because I discovered a bug in pfSense, but I discovered that there are people that write email addresses in all capitals (not strictly wrong, simply not done), write SMS's even even letters (real snail mail !!!) in all capitals (no kidding)..
These are the same people that just discovered the use of "select and copy" by using the mouse, left click pressed.
I decided that that "issue" wasn't mine and started to help them, by removing the trim() ... -
No reason I know of to not trim the username there. I'm not aware of any system where a space would be significant before or after the username, and IIRC LDAP will ignore the spaces in that context anyhow.
Open an issue and submit a PR and we can look it over. There may be some other reason I'm forgetting.
-
issue #9274 created
-
@heper
I think it's only logical to have a trim( .. ) there.
After all, there is a trim applied to auth_voucher POST-variable, a few lines above the one you mentioned.
https://github.com/pfsense/pfsense/blob/master/src/usr/local/captiveportal/index.php#L159 -
hi @heper
could you have a look to the comment on your pull request on github ? https://github.com/pfsense/pfsense/pull/4037#discussion_r250025108 -
@free4 you might have a point but It's clear the devs have better things to do then look at this minor detail.
Whoever of the staff that finds the time to look at the pr, can change it as they see fit.
-
The point of a PR is so that devs don't have to make the changes necessary for the submitted code to work properly. The PR needs to be fixed, then merged. Not the other way around.
-
I think everyone can agree that pr's should save time. Making comments on this forum uses valuable time. Probably more time then it would take a skilled coder to fix a couple of characters of code in some crappy pr that a random amateur created
