rt icon indicating copy to clipboard operation
rt copied to clipboard

Fix problematic case-sensitivity in ExternalAuth::DBI.

Open Elemecca opened this issue 7 years ago • 4 comments

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 avatar Oct 22 '16 02:10 Elemecca

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.

Elemecca avatar Oct 22 '16 05:10 Elemecca

Hi Sam,

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

Best, Shawn

sartak avatar Nov 09 '16 20:11 sartak

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?

Elemecca avatar Jan 13 '19 20:01 Elemecca

Hi Sam

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

-sunnavy

sunnavy avatar Jan 15 '19 16:01 sunnavy