[auth] Validate usernames upfront #14808
Conversation
| @@ -151,6 +151,16 @@ async def check_valid_new_user(tx: Transaction, username, login_id, is_developer | |||
| return None | |||
There was a problem hiding this comment.
Here we seem to only allow alphanumeric usernames. Do you know if that's true, or is this not being executed for some reason? Either way, it would be nice to define username validity in one place.
There was a problem hiding this comment.
Oh, interesting, I somehow missed this. The original error report was caused by uppercase characters, which I guess is why it got past this check originally. Usernames which don't conform to RFC1123 was where I started seeing downstream problems with user creation, but since "(lowercase) alphanumeric" is a subset of RFC1123, I'm inclined to make that our new rule, instead of expanding the options up to include all of RFC1123.
And I like the idea of moving that check into this function, instead of adding a new validation. I'll do that.
There was a problem hiding this comment.
In fact, RFC1123 would be a fine validation for the credentials_secret_name that we allow as a development environment option, and is currently unvalidated until it potentially blows up the user creation process downstream. So I'll repurpose the original change into a token name validator, and make username validation stricter and happening here
| @@ -128,7 +128,7 @@ async def check_valid_new_user(tx: Transaction, username, login_id, is_developer | |||
| raise MultipleUserTypes(username) | |||
| if not is_service_account and not login_id: | |||
| raise EmptyLoginID(username) | |||
| if not username or not all(c for c in username if c.isalnum()): | |||
| if not username or not (username.isalnum() and username.islower()): | |||
There was a problem hiding this comment.
Am I missing an important reason why this was being done character by character previously?
Change Description
Moves validation of usernames to the front-end, so invalid username requests never get into the database and block driver processing of new user requests.
Security Assessment
Impact Description
Low because we are removing a path to a possible error state.