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

Added case-sensitivity option for usernames #485

Merged

Conversation

Radiergummi
Copy link
Contributor

Why
Using Postgres (and possibly other DBMS), comparisons are case-sensitive as opposed to MySQL.
This causes issues where users would enter e.g. Taylor@laravel.com on logging in but registered with taylor@laravel.com, and receive a bad credentials error. This bites every single installation of Fortify using Postgres.

By storing the usernames in lowercase, and converting them before passing it to user-defined authentication logic, this problem just disappears.

Backwards compatibility
I took care to not break BC anywhere:

  • The option defaults to true for now, meaning usernames are case-sensitive by default. That way, nothing breaks for existing applications, including those with custom workarounds. It should probably be set to false in the next major revision.
  • The case change happens before the input is passed to user-defined actions (e.g. CreateNewUser), so no changes to existing implementations are required.
  • The new CanonicalizeUsername action can just be dropped in existing users' pipelines, if desired.

Introduced a new configuration option 'case_sensitive_usernames'. This feature allows to decide whether usernames should be treated as case-sensitive. The default configuration value is true to prevent backwards-compatibility breaks. If it is set to false, the system will convert all usernames to lowercase before storing or comparing them. This prevents issues with case-sensitive database collations, where users would not enter their email address in the exact variation used at registration. Accompanying tests and implementation have been updated or added accordingly.
Copy link
Member

@driesvints driesvints left a comment

Choose a reason for hiding this comment

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

I'm not 100% sure if a setting is the right solution here but let's see what @taylorotwell says.

@Radiergummi
Copy link
Contributor Author

I'm not 100% sure if a setting is the right solution here but let's see what @taylorotwell says.

I don't really see another option which doesn't interfere with existing applications, but I'm open for any suggestions!

@taylorotwell
Copy link
Member

Made a few changes.

Renamed configuration option to lowercase_usernames. Made default value in fallback config false (to preserve BC), but updated stub value to true so all new applications will get the more correct and expected behavior IMO of lowercasing the username.

@taylorotwell taylorotwell merged commit 74cd344 into laravel:1.x Sep 6, 2023
@martinbean
Copy link

This is an amazing PR, it’s something I have to deal with (and remember to deal with) in applications using Postgres. However, should the lower-casing not be done before storing? Otherwise validations will fail.

For example, say I have [email protected] in my users table. If I try and register with [email protected] then the validation is going to pass because it's different casing. But when the controller comes to actually insert the database record, Fortify is going to down-case the submitted value, and the database is going to throw a unique constraint error (if the column does indeed have a unique constraint).

@driesvints
Copy link
Member

@martinbean good question actually. Can you weigh in here @Radiergummi?

@Radiergummi
Copy link
Contributor Author

Radiergummi commented Sep 28, 2023

In the RegisteredUserController, the given username should be lowercased if the setting is enabled:

public function store(Request $request,
                          CreatesNewUsers $creator): RegisterResponse
{
    if (config('fortify.lowercase_usernames')) {
        $request->merge([
            Fortify::username() => Str::lower($request->{Fortify::username()}),
        ]);
    }

    event(new Registered($user = $creator->create($request->all())));

    $this->guard->login($user);
    return app(RegisterResponse::class);
}

The idea here is to pass the correctly cased value to any user creator implementation already present - that should work well, unless the implementation retrieves the value from the request via any other means or changes the formatting somehow (but I don't think that is very likely).

Have I overlooked something here? Does that error actually crop up?

Edit: @martinbean reading your question again, you were wondering whether the Rule::unique('email') in CreateNewUser could fail if it checks a differently cased variant to the already one stored in the database, right?
This shouldn't be possible as the lowercase value is passed to the implementation, which both validates the input and creates a user from it. So whatever the casing used by the user, we only store lowercase usernames and validate lowercase values against them.

@martinbean
Copy link

Sorry, didn’t see there were replies to this.

@Radiergummi, the issue isn’t the persisting of the usernames. Yes, they’re normalised as all-lowercase in the controller. But my issue is, the validation happens before then.

So, if I have [email protected] in my users table. The issue is if someone then tries to sign up but submits the same email address in a different case, i.e. [email protected]. The validation rule is essentially going to do a where email = '[email protected], find no matches, and carry on to the controller, which is then going to lowercase, try and insert the value, and throw a database exception because it’s a duplicate.

@martinbean
Copy link

/cc @driesvints

@driesvints
Copy link
Member

Maybe we should just add lowercase validation to fortify as well like we did in Breeze? laravel/breeze#321

@Radiergummi
Copy link
Contributor Author

@martinbean I'm not sure I understand the problem yet; from what I've seen of the Fortify code, validation happens in the CreatesNewUsers implementation, not before -- where would validation take place otherwise in a controller that uses a base Request instance?

@martinbean
Copy link

martinbean commented Oct 12, 2023

@Radiergummi No normalisation happens in the implementation:

'email' => [
'required',
'string',
'email',
'max:255',
Rule::unique(User::class),
],

So if I try and register using [email protected] then that will pass validation, even if the all-lowercase equivalent ([email protected]) exists in the database.

@Radiergummi
Copy link
Contributor Author

Radiergummi commented Oct 13, 2023

@martinbean Ah, but we call $request->merge() before passing the request to the CreateNewUser class if lowercase usernames are enabled. This merges the request data in-place, and overrides the given username with its lowercase variant - so the implementation will only ever receive [email protected] in your example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide and Document a secure way to implement case-insensitive "login" and "forgot password" methods
5 participants