Check that username contains no uppercase letters #2000

Kradyz · 2021-12-15T02:20:56Z

Addresses #1955 and #1986 for future users

Removed uppercase letters from the names passed to test_valid_actor_name()

dessalines · 2021-12-15T15:17:21Z

We've been wanting to add lowercase-forced names for a while, so this might as well do the rest too.

This will also need a migration to:

Delete any to_lowercase dupes for the person.name column, keeping the first one. There are examples of this in the migration folder.
Convert all existing names to lower case.
Check if any actor_ids contain upper case chars ( i don't think they do, but it'd be good to check )
Add a lowercase enforcement check to the name column.

Password test needs lowercase user

Kradyz · 2021-12-16T14:23:24Z

Adapted the remove duplicates code from this migration. Since "actor_id" now contains the information about both username and instance, I think that group by lower(actor_id) should be enough to identify duplicates.
Implemented by transforming person.name, person.actor_id, and person.inbox_url to lowercase, as these three columns contain the username. I also created "old_variable" columns so that down.sql command is able to recover the old names. I am not sure about how to undo the "remove duplicates" command with down.sq.
person.actor_ids do contain upper case chars when the username does, as the actor_id contains the username.
Adding this lowercase enforcement check will block new entries if they contain uppercase characters, rather than re-writing. A post from another instance is not fetched if the name of the author is capitalized in the other instance, so this will have backwards-compatibility issues with instances that have not migrated.
activity.data contains the username under the "actor" key, so it might be necessary to update this one too. However, the data also contains some text that should be capitalized, so applying lower(data) is probably not the right solution.

dessalines · 2021-12-16T20:59:36Z

Great, this is looking good so far. A few things:

You need to generate this folder using diesel migration generate ... , and test that it works by doing:

export DATABASE_URL=postgres://lemmy:password@localhost:5432/lemmy
diesel migration run
diesel migration redo

Run redo a few times.

dessalines · 2021-12-16T21:00:39Z

migrations/2021-12-16-003905_names_to_lowercase/up.sql

+alter table person add constraint person_name_lowercase_ck check (name = lower(name));
+alter table person add constraint person_actor_id_lowercase_ck check (actor_id = lower(actor_id));
+alter table person add constraint person_inbox_url_lowercase_ck check (inbox_url = lower(inbox_url));


Preface these all with idx_ like the other indexes / constraints.

Ok. But shouldn't the prefix be ck_ or chk_, since this is a check constraint instead of a unique index constraints?

dessalines · 2021-12-16T21:01:14Z

migrations/2021-12-16-003905_names_to_lowercase/up.sql

+-- Add a lowecase enforcement check to these three columns
+
+alter table person add constraint person_name_lowercase_ck check (name = lower(name));
+alter table person add constraint person_actor_id_lowercase_ck check (actor_id = lower(actor_id));


I think there's a idx_group lower index, but you should probably remove that old one, as this new one is better.

Currently, there is a lower_actor_id unique index constraint.

Indexes:
"person__pkey" PRIMARY KEY, btree (id)
"idx_person_actor_id" UNIQUE CONSTRAINT, btree (actor_id)
"idx_person_inbox_url" UNIQUE CONSTRAINT, btree (inbox_url)
"idx_person_lower_actor_id" UNIQUE, btree (lower(actor_id::text))
"idx_person_published" btree (published DESC)

dessalines · 2021-12-16T21:01:58Z

migrations/2021-12-16-003905_names_to_lowercase/up.sql

+delete
+from person
+where id not in (
+  select min(id)
+  from person
+  group by lower(actor_id)
+);


I had to learn the hard way, that doing an in query can take days sometimes, especially on large tables.

Use this one for dupes: https://github.com/LemmyNet/lemmy/blob/main/migrations/2021-11-22-135324_add_activity_ap_id_index/up.sql#L9

dessalines · 2021-12-16T21:03:57Z

migrations/2021-12-16-003905_names_to_lowercase/up.sql

+-- Store the old values so that down.sql can recover them
+alter table person add column old_name varchar(255);
+update person set old_name = name;
+
+alter table person add column old_actor_id varchar(255);
+update person set old_actor_id = actor_id;
+
+alter table person add column old_inbox_url varchar(255);
+update person set old_inbox_url = inbox_url;


I wouldn't worry about saving these, and it also might mess up some things with diesel if we add columns but don't do it also in the code. Don't worry, before we merge this and run it in production, I will test with our lemmy.ml data to make sure it works correctly. But thanks for thinking of this.

dessalines · 2021-12-16T21:04:26Z

migrations/2021-12-16-003905_names_to_lowercase/down.sql

+-- Recover the old values
+update person set name = old_name;
+update person set actor_id = old_actor_id;
+update person set inbox_url = old_inbox_url;
+
+-- Delete the columns with the "old" values
+alter table person drop column old_name;
+alter table person drop column old_actor_id;
+alter table person drop column old_inbox_url;


Kradyz · 2021-12-18T17:20:38Z

I have implemented the changes and tested the migration using

diesel migration run
diesel migration redo

I had a bit of trouble implementing the exact same syntax for the deletion of dupes, so I used what I think would be the equivalent minimal syntax from this postgresql tutorial. This works with my small database so hopefully it will be efficient with your large database.

The lowercase check will make it such that a migrated instance won't be able to fetch data from capitalized usernames in non-migrated instances. I think that it should be possible to make this migration backwards-compatible by applying to_lowercase() when processing the person information in lemmy/crates/apub/src/objects/person.rs. I tried to implement this myself as a test, but I don't quite understand how to transform the "inbox_url.clone().into()" to lowercase.

Nutomic · 2021-12-18T20:54:26Z

The lowercase check will make it such that a migrated instance won't be able to fetch data from capitalized usernames in non-migrated instances. I think that it should be possible to make this migration backwards-compatible by applying to_lowercase() when processing the person information in lemmy/crates/apub/src/objects/person.rs. I tried to implement this myself as a test, but I don't quite understand how to transform the "inbox_url.clone().into()" to lowercase.

That sounds like it would not allow Lemmy to federate properly with other software which allows uppercase in usernames. Not sure if there is such software as usernames seem to be generally lowercase, but anyway it might not be good to make the db constraints too strict.

Also, did you check if the same problem happens with uppercase community names?

Kradyz · 2021-12-19T01:09:06Z

After diving a bit deeper into it...

To address #1955 and #1986, it is enough to change the webfinger regex in lemmy/crates/utils/src/settings/mod.rs, which currently excludes A-Z.

static WEBFINGER_REGEX: Lazy<Regex> = Lazy::new(|| {
  Regex::new(&format!(
    "^acct:([a-z0-9_]{{3,}})@{}$",
    Settings::get().hostname
  ))
  .expect("compile webfinger regex")
});

I have applied this change to my instance, and you can now access the user URL without problem:
http://lemmy.ml/u/Sal@mander.xyz

I also tested Mastodon, and the user is also fetched without issue.

Is using lowercase names actually necessary? If other projects prefer interacting with lowercase actor_id, why not just apply a to_lowercase() function call in generate_local_apub_endpoint() such that the actor_ids and the inbox_urls are set to lowercase, while the username remains catpitalized?

@Nutomic With the current webfinger regex, uppercase community names would also be rejected.

Nutomic · 2021-12-19T02:01:42Z

Good find, fixing the webfinger regex definitely seems like the better solution then. And as you say that Mastodon handles uppercase without problems, I dont see any reason for us to convert to lowercase.

Honestly this simply looks like an oversight because we only ever tested with lowercase names because its easier to type. While fixing this, you could change a username in the api_tests folder to contain uppercase, so that this gets tested for (but not sure if any of them use webfinger).

dessalines · 2021-12-19T16:44:57Z

Along with the other PR, I'd still like to keep / do this one, and enforce lowercase lemmy usernames. Its something we've needed to do for a while, and its worthwhile to not have usernames like MyUser, myUser, etc. Holidays got me busy so it might be a bit before I can test this one.

Nutomic · 2021-12-19T20:09:11Z

@dessalines For that it would be enough to add a constraint that lowercase(username) is unique.

Kradyz · 2021-12-19T20:41:02Z

@Nutomic The unique index for lower(actor_id) called "idx_person_lower_actor_id" is already enforcing that no two people can have the same lower(username) in an instance. I think that this issue was dealt with during this migration, and the username duplicates were removed.

If you would like enforce lowercase usernames, I would suggest performing the migration and enforcing the creation of new lowercase usernames via the UI and via the VALID_ACTOR_NAME_REGEX, but not enforcing the lowercase database checks, as those checks will prevent interacting with uppercase usernames in other projects.

Alternatively, db checks can be enforced and external usernames could be changed to lowercase when they interact with lemmy.

dessalines · 2021-12-20T20:44:37Z

The other reason I can think of, to enforce lowercase usernames: people would stop getting confused when trying to log in and not using the correct capital letters.

…ion to check

dessalines · 2022-01-03T16:06:24Z

@Nutomic what do you think? Should we disregard this now that we support uppercase user names?

Nutomic · 2022-01-04T14:53:38Z

I dont think there is any reason to prevent uppercase in usernames. Only argument I could think of would be confusion between some characters like I / l, but thats something admins can take care of if necessary.

dessalines · 2022-01-06T16:29:19Z

In that case I'll close, @Kradyz other merged PR should be good enough.

Kradyz · 2022-01-06T16:45:45Z

@dessalines Ok! Just a reminder that LemmyNet/lemmy-ui/#525 was merged, preventing users from inputting uppercase usernames when creating a user. You might want to revert this too.

Kradyz added 2 commits December 15, 2021 03:17

Check that username contains no uppercase letters

b6a1774

Update test.rs

d2c19aa

Removed uppercase letters from the names passed to test_valid_actor_name()

Kradyz and others added 10 commits December 16, 2021 00:19

Merge branch 'LemmyNet:main' into main

cf05d19

Create up.sql

7be3236

Create down.sql

7bc2ce5

Update up.sql

f401464

Update up.sql

b24f4aa

Update down.sql

9e6a391

Migrate usernames to lowecase

8084fa5

Update up.sql

e85fe88

Update lib.rs

af76961

Password test needs lowercase user

Update down.sql

1133669

dessalines reviewed Dec 16, 2021

View reviewed changes

Username to lower migration

fd2dd0a

Kradyz mentioned this pull request Dec 19, 2021

Case-insensitive webfinger response. Fixes #1955 & #1986 #2005

Merged

Kradyz and others added 3 commits December 21, 2021 12:25

Merge branch 'LemmyNet:main' into main

6b793d0

Remove A-Z from VALID_ACTOR_NAME_REGEX instead of calling a new funct…

a98abf6

…ion to check

Remove uppercase API tests if uppercase if disallowed

5f4cfc5

dessalines requested a review from Nutomic January 3, 2022 16:05

dessalines closed this Jan 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check that username contains no uppercase letters #2000

Check that username contains no uppercase letters #2000

Kradyz commented Dec 15, 2021

dessalines commented Dec 15, 2021

Kradyz commented Dec 16, 2021

dessalines commented Dec 16, 2021

dessalines Dec 16, 2021

Kradyz Dec 17, 2021

dessalines Dec 16, 2021

Kradyz Dec 17, 2021

dessalines Dec 16, 2021

dessalines Dec 16, 2021

dessalines Dec 16, 2021

Kradyz commented Dec 18, 2021

Nutomic commented Dec 18, 2021

Kradyz commented Dec 19, 2021 •

edited

Nutomic commented Dec 19, 2021 •

edited

dessalines commented Dec 19, 2021 •

edited

Nutomic commented Dec 19, 2021

Kradyz commented Dec 19, 2021

dessalines commented Dec 20, 2021

dessalines commented Jan 3, 2022

Nutomic commented Jan 4, 2022

dessalines commented Jan 6, 2022

Kradyz commented Jan 6, 2022

Check that username contains no uppercase letters #2000

Check that username contains no uppercase letters #2000

Conversation

Kradyz commented Dec 15, 2021

dessalines commented Dec 15, 2021

Kradyz commented Dec 16, 2021

dessalines commented Dec 16, 2021

dessalines Dec 16, 2021

Choose a reason for hiding this comment

Kradyz Dec 17, 2021

Choose a reason for hiding this comment

dessalines Dec 16, 2021

Choose a reason for hiding this comment

Kradyz Dec 17, 2021

Choose a reason for hiding this comment

dessalines Dec 16, 2021

Choose a reason for hiding this comment

dessalines Dec 16, 2021

Choose a reason for hiding this comment

dessalines Dec 16, 2021

Choose a reason for hiding this comment

Kradyz commented Dec 18, 2021

Nutomic commented Dec 18, 2021

Kradyz commented Dec 19, 2021 • edited

Nutomic commented Dec 19, 2021 • edited

dessalines commented Dec 19, 2021 • edited

Nutomic commented Dec 19, 2021

Kradyz commented Dec 19, 2021

dessalines commented Dec 20, 2021

dessalines commented Jan 3, 2022

Nutomic commented Jan 4, 2022

dessalines commented Jan 6, 2022

Kradyz commented Jan 6, 2022

Kradyz commented Dec 19, 2021 •

edited

Nutomic commented Dec 19, 2021 •

edited

dessalines commented Dec 19, 2021 •

edited