rt
rt copied to clipboard
Fix problematic case-sensitivity in ExternalAuth::DBI.
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.
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.
Hi Sam,
Thanks for your patch. It's on our list to review and merge. :)
Best, Shawn
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?
Hi Sam
It seems an unintentional action, sorry for the confusion. I re-opened it.
-sunnavy