Page MenuHomePhabricator

[identity] fix issues with capitalized usernames in users and reserved usernames tables
ClosedPublic

Authored by varun on Jul 9 2024, 9:34 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Oct 14, 8:43 PM
Unknown Object (File)
Mon, Oct 14, 8:43 PM
Unknown Object (File)
Mon, Oct 14, 8:43 PM
Unknown Object (File)
Wed, Sep 25, 8:25 PM
Unknown Object (File)
Wed, Sep 25, 8:25 PM
Unknown Object (File)
Sep 8 2024, 4:25 PM
Unknown Object (File)
Sep 8 2024, 4:25 PM
Unknown Object (File)
Sep 8 2024, 4:25 PM
Subscribers

Details

Summary

we have to store usernames in their original form and in their lowercase form in these two dynamodb tables because we need case-insensitive uniqueness checks and case-sensitive retrievals. if a user registered with the username "ashoat," we should allow that user to log in with "Ashoat," too. however, we should not allow a different user to register the username "Ashoat."

searched for all places where we use USERS_TABLE_USERNAME_ATTRIBUTE or RESERVED_USERNAMES_TABLE_PARTITION_KEY and checked whether we were doing a retrieval or uniqueness check
if we were doing a uniqueness check, i swapped out the attribute with the lowercase equivalent. i made sure we always write the lowercase username to DDB along with the original username.

would normally add @bartek here but he's on vacation

Test Plan
  1. registered username pamela
  2. tried logging in with capitalized Pamela -> succeeded
  3. tried registering a new user Pamela -> failed
  4. registered a new user KidRock directly with local keyserver
  5. tried to log in with usingCSAT=true and username kidrock -> succeeded
  6. registered a new user tommylee directly with local keyserver
  7. tried to log in with usingCSAT=true and username TommyLee -> succeeded

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

varun published this revision for review.Jul 9 2024, 9:40 AM

I don't feel qualified to review this diff... @will, could you maybe take a look?

will accepted this revision.EditedJul 10 2024, 9:34 AM

Directly synced with Varun. Looks good. Before landing make sure to remove need for unnecessary partition key check and add a comment for changes starting from line 340 in services/identity/src/database.rs

This revision is now accepted and ready to land.Jul 10 2024, 9:34 AM
This revision was landed with ongoing or failed builds.Jul 10 2024, 11:48 PM
This revision was automatically updated to reflect the committed changes.
In D12704#360102, @will wrote:

Directly synced with Varun. Looks good. Before landing make sure to remove need for unnecessary partition key check and add a comment for changes starting from line 340 in services/identity/src/database.rs

ended up rewriting this code so there was no need for the code comment

This diff caused https://linear.app/comm/issue/ENG-8810/unable-to-login - we're reading from the new index but we didn't create records in it for existing users.