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.

Sign up for GitHub

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

[auth] Validate usernames upfront #14808

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[auth] Validate usernames upfront #14808

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fe1420a
validate usernames
cjllanwarne Jan 30, 2025
7cd31cd
log decision making
cjllanwarne Jan 31, 2025
f0cc17a
more instructive error message
cjllanwarne Jan 31, 2025
8353aad
review feedback
cjllanwarne Jan 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions auth/auth/auth.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,16 @@ async def check_valid_new_user(tx: Transaction, username, login_id, is_developer
return None
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@cjllanwarne cjllanwarne Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



def validate_username(username):
log.info(f'validating username {username}')
regex = re.compile(r'^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$')
if not regex.match(username):
raise AuthUserError(
f'invalid username {username}, must match RFC1123 (lowercase alphanumeric plus "." and "-", start and end with alphanumeric)',
'error',
)


async def insert_new_user(
db: Database,
username: str,
Expand Down Expand Up @@ -183,6 +193,7 @@ async def _insert(tx):
),
)

validate_username(username)
await _insert()
return True

Expand Down