-
Notifications
You must be signed in to change notification settings - Fork 37
Fix usernames being case sensitive #123
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
base: main
Are you sure you want to change the base?
Fix usernames being case sensitive #123
Conversation
|
So, John.doe and john.doe are same person? |
|
@david-labs-ca, there's discussion on the original ticket. I think the world at large usually assumes usernames to be case-insensitive. It's also just plum broken on several case-insensitive database engines in main, so if the decision were to make them case sensitive, a change would still be required. |
I get angry any time I encounter a system with case-sensitive usernames. I expect to always be able to type my username in all lowercase, regardless of how the admin added it to the system. |
|
suggestion: don't assume anything about which charset, collation, or locale are in use I think it would be a good idea to use |
|
@tonygermano Re: existing users, I've added a fallback to match case-sensitive if multiple users match. Adding a new user with a different casing fails with |
@mgaffigan I don't think the PERSON table is expected to get very large where a scan would be a problem, and this seems evident because there isn't an index on USERNAME. In this case, I think it's better to be safe and use the standard SQL function without making assumptions about the environment rather than going for performance. LOWER should work across all databases respecting their current collation, character encoding, and locale. This is completely up to you, but if you want to address this issue, too, it's related nextgenhealthcare/connect#3386 |
6d64ab2 to
4f8bcdd
Compare
|
@tonygermano, I still think COLLATE is the correct way to do this - but I've updated it to use |
|
@mgaffigan for completeness, what are your thoughts on updating |
If it will get the PR merged, I'm unopposed. I do not think it is called for, but I do not like From a maintenance perspective I think it is counterproductive to keep the different database server scripts in sync: I've never found SQL to be an actually portable language at scale (DDL aside, the query engines are too different). The appropriate interface for changing persistence is at the DAL - not at SQL. This also allows in memory stores for unit tests, no-SQL, etc. |
Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
Signed-off-by: Mitch Gaffigan <mitch.gaffigan@comcast.net>
4f8bcdd to
1ff71d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes an issue where usernames were treated as case-sensitive, making it inconsistent and potentially confusing for users. The implementation makes username comparisons case-insensitive across all supported database systems while preserving the original casing stored in the database and preferring exact case matches when multiple users exist with different casing (for backward compatibility).
Key changes:
- Modified database queries to use case-insensitive username comparison via
LOWER()function - Updated
getUser()method to handle potential multiple results and prefer exact case matches - Enhanced
LoginStatusto return the normalized username from the database to the client
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
server/src/com/mirth/connect/server/controllers/DefaultUserController.java |
Updated getUser() to return lists and prefer case-sensitive matches; updated authorizeUser() to pass normalized username to LoginStatus |
server/dbconf/postgres/postgres-user.xml |
Added case-insensitive username comparison using LOWER() function |
server/dbconf/sqlserver/sqlserver-user.xml |
Added case-insensitive username comparison using LOWER() function |
server/dbconf/oracle/oracle-user.xml |
Added case-insensitive username comparison using LOWER() function |
server/dbconf/mysql/mysql-user.xml |
Added case-insensitive username comparison using LOWER() function |
server/dbconf/derby/derby-user.xml |
Added case-insensitive username comparison using LOWER() function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
Closes #122 / original #5569 by:
Implementation Notes:
LOWER()in pgsql instead of collation to avoid having to rely upon system defaults or database defined collation. Presumably table is not so large that sargability is a concern (since we're loading the full table to the client on each login)LOWER()