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

Fix problematic case-sensitivity in ExternalAuth::DBI. #200

Open
wants to merge 2 commits into
base: 4.4-trunk
Choose a base branch
from

Conversation

Elemecca
Copy link

@Elemecca Elemecca commented Oct 22, 2016

Users expect their username and email address to be case-insensitive. RT's users are case-insensitive, but RT::Authen::ExternalAuth::DBI had a bug that caused it to fail if its backing database was case-insensitive.

It used DBI::selectall_hashref keyed on the username or email address from the backing database (whichever was used to look up the account), and then retrieved the record from the resulting hash based on the value it had been passed. If the backing database was case-insenitive but case-preserving (as most are) and the case in the database did not match the case used for the query, then the case used to create the hash key would not match the case used to retrieve it. Since Perl hash keys are case-sensitive, the hash dereference would fail and the method would return undef, resulting in very strange error messages.

This changes it to use DBI::selectall_arrayref and simply use the first record returned. Since the code already checks that exactly one result is returned that change should have no effect on the intended behavior.

I feel like this should have regression tests, but there don't seem to be any tests yet for ExternalAuth::DBI and I'm not sure how to mock the external database, so I'm submitting without tests for now. I'll keep looking into how to do it, but I'd appreciate advice if people have any.

@Elemecca
Copy link
Author

Well, I found t/externalauth/sqlite.t and added a few tests there to make sure all the code I changed is covered and that the case-insensitivity of usernames and email addresses is asserted. Still, I'll be happy to make them better if people have suggestions.

Users expect their username and email address to be case-insensitive.
RT's users are case-insensitive, but RT::Authen::ExternalAuth::DBI had a
bug that caused it to fail if its backing database was case-insensitive.

It used DBI::selectall_hashref keyed on the username or email address
from the backing database (whichever was used to look up the account),
and then retrieved the record from the resulting hash based on the value
it had been passed. If the backing database was case-insenitive but
case-preserving (as most are) and the case in the database did not match
the case used for the query, then the case used to create the hash key
would not match the case used to retrieve it. Since Perl hash keys are
case-sensitive, the hash dereference would fail and the method would
return undef, resulting in very strange error messages.

This changes it to use DBI::selectall_arrayref and simply use the first
record returned. Since the code already checks that exactly one result
is returned that change should have no effect on the intended behavior.
@sartak
Copy link
Contributor

sartak commented Nov 9, 2016

Hi Sam,

Thanks for your patch. It's on our list to review and merge. :)

Best,
Shawn

@Elemecca
Copy link
Author

Elemecca commented Jan 13, 2019

Why was this closed? It doesn't appear to have been merged, nor has an equivalent change been made on master, and the bug still exists. Has this change been rejected? Do I need to rebase it for a new branch?

@sunnavy
Copy link
Member

sunnavy commented Jan 15, 2019

Hi Sam

It seems an unintentional action, sorry for the confusion. I re-opened it.

-sunnavy

@sunnavy sunnavy reopened this Jan 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants