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

Usernames with uppercase chars can no longer log in #2073

Closed
bones-was-here opened this issue Dec 16, 2021 · 13 comments
Closed

Usernames with uppercase chars can no longer log in #2073

bones-was-here opened this issue Dec 16, 2021 · 13 comments
Labels
C-Client-API F-Registration good first issue Want to help with Dendrite? These are the issues to start with! T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.

Comments

@bones-was-here
Copy link

Background information

  • Dendrite version or git SHA: 002c3e0
  • Monolith or Polylith?: Monolith
  • SQLite3 or Postgres?: Postgres
  • Running in Docker?: no
  • go version: 1.17.5

Description

If a user has created an account with create-account and their username contains a capital letter, they can no longer log in.

Steps to reproduce

  • create-account --username "Test" --password "SomePassword"
  • try to log in with version 388d7a1 or later

We have had such an account for a long time now and were unaware it was not compliant with the spec. Is this going to cause federation problems for the rooms? How can we recover from the situation?

@kegsay
Copy link
Member

kegsay commented Jan 21, 2022

It shouldn't cause federation problems as there are existing matrix user IDs on matrix.org which also have upper-case letters.

@kegsay kegsay added bug good first issue Want to help with Dendrite? These are the issues to start with! labels Jan 21, 2022
@hoernschen
Copy link
Contributor

I would like to get into developing for this project and wanted to start with this issue.
But I am not completely sure what the expected behaviour should be here. Should the login also support old uppercase usernames or be completely not case sensitive?

@kegsay
Copy link
Member

kegsay commented Jan 31, 2022

It should be completely not case-sensitive.

@DefaultUser
Copy link

While it was possible to log in with a username with uppercase characters after 1d5fd99, it is again no longer possible. I assume it was this change:
c36e454#diff-62b3ac909bbc9d2680fa82b61540fd0ef7ad94417d9d07582c898655dd6c6d4cR65

@bones-was-here
Copy link
Author

"Fixed" by adding a line to the build script...
sed -i 's/username := strings.ToLower(r.Username())/username := r.Username()/' clientapi/auth/password.go

It should be completely not case-sensitive.

Forcing to lower case is not the same as case insensitivity...

@avdb13
Copy link

avdb13 commented Mar 13, 2022

Forcing to lower case is not the same as case insensitivity...

This is indeed true but if you want to have case insensitivity you'd have to change it all the way down to the database level which might break compatibility with a lot of other parts of the code. As for as I know SQLite supports case-insensitive comparisons for ASCII characters but for PostgreSQL the citext extension would be required.

@bones-was-here
Copy link
Author

Forcing to lower case is not the same as case insensitivity...

This is indeed true but if you want to have case insensitivity you'd have to change it all the way down to the database level which might break compatibility with a lot of other parts of the code. As for as I know SQLite supports case-insensitive comparisons for ASCII characters but for PostgreSQL the citext extension would be required.

No, how it's supposed to work (at least according to MSC threads) is:

  1. "under the hood" (in the DB, in federation traffic, in JSON) the usernames are fully case sensitive and case is always preserved
  2. Authentication (logging in) is supposed to be case insensitive
  3. Account creation should fail when there's an existing username that differs from the new one only by case

Dendrite succeeds at 1 and fails at 2 and 3.

@avdb13
Copy link

avdb13 commented Mar 15, 2022

I see, how would 2 and 3 be accomplished without just converting the username to lowercase during authentication and registration?

@bones-was-here
Copy link
Author

I see, how would 2 and 3 be accomplished without just converting the username to lowercase during authentication and registration?

New accounts are easy: just squash to lowercase or refuse to create the account unless it's all lowercase. This needs to apply to the create-account binary as well as the online registration methods.
Unfortunately there's probably some cases where the Application Service needs to be able to create multiple accounts that differ only by case, for bridging to networks that allow it.

Authentication requires something like https://pkg.go.dev/strings#EqualFold

@spaetz
Copy link
Contributor

spaetz commented Mar 15, 2022

This would let you create account 'admin' when there is already an 'Admin' account.

@bones-was-here
Copy link
Author

Oh true, so I guess it would have to use case folding when checking if the account already exists.

@paride
Copy link

paride commented Jul 19, 2022

I'm a user stuck with a username with an uppercase letter. I'd definitely appreciate a migration path to a compliant username (e.g. creation of the lowercase username with deactivation of the non-compliant username).

@kegsay kegsay added T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. F-Registration and removed bug labels Dec 5, 2022
@bones-was-here
Copy link
Author

This appears to have been fixed at some point, on 0.10.8 I can log in to accounts with capitals provided I exactly match the case of the localpart column in userapi_accounts. The sed patch above is no longer needed or applicable.

S7evinK added a commit that referenced this issue Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Client-API F-Registration good first issue Want to help with Dendrite? These are the issues to start with! T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants