claudiodekker/laravel-auth

Rich authentication logic for your Laravel applications.

laravel-auth's Issues

Recovery Code must be exactly 10 characters

This is a very minor bug, but essentially when you enter a recovery code with a dash separator (as it is displayed during code generation), the code will be 11 characters long instead of 10, while the recovery code input itself only allows 10 characters exactly.

The obvious workaround is to simply enter the code without the separator, but the problem there is that those who store their recovery codes in a password manager, will likely copy-paste the code as-is with the separator, in which case the browser likely truncates the input field, making the code one character too short, causing the code to be considered invalid.

To fix this, we'll want to keep the "min" length as-is, but make the "max" length 11.

Recovery: Skip the recovery code challenge when no codes exist

Initially, I considered recovery codes to be mandatory:

• You get a warning when you're signed in, and if you don't generate and store them somewhere, you get locked out.
• When you request a recovery link, the recovery code challenge page would always be shown, regardless of whether you have generated codes or not. It'd just always fail any code if you didn't have any.
• Through all the fired events, as well as user activity on the platform, the person could then reach out to your company/application's support person, and through a chat conversation and those data points (e.g. the last authenticated event's IP address, or most common one, as well as the IP address of the person who's requested recovery) could have support reset their account for them.

After talking to @valorin and thinking things over, I've changed my position on this slightly. This issue serves to track as a reminder to myself, as well as a specification as to what I intend to change and to what.

First of all, I will adjust the recovery code flow, so the system will 'skip' the recovery code challenge in the situation that you did not generate recovery codes, similar to what would happen when you did not configure 2FA and try to authenticate. This puts the responsibility as to generate recovery codes with the user.

Secondly, I will add a "account security" indicator class, with the following levels:

	No 2FA	With 2FA
No recovery	Weak & at-risk (RED)	At-risk (ORANGE)
With recovery	Weak (RED)	Strong (GREEN)

The colors (red, orange, green) serve as the actual levels, and can be checked against in-code. You can feed it the user object, call a check method, and get the level-code back. By default, the goal is to use this in the UI of the adapter packages, as to display a "security alert" on both the dashboard ("home") as well as the account security overview page.

Stricter Typesystem

Hey @claudiodekker,

I'd like to help a little bit with the package and thought about how I could contribute the best for now.

So I'd like to propose to use a stricter type system which would mean:

• use declare(strict_types=1) at the top of all files
• using types instead of phpdoc annotations

Why is that change important in my opinion?
A stricter type system would mean catching more unexpected bugs / type conversions. There are a lot of cases were an integer is passed but a string is expected. Because for Authentication Security is an important aspect this should be an explicit instead of an implicit type conversion.

If you want I would work on these things in multiple smaller iterations so that it would be better to review:

1. In multiple iterations I would add declare(strict_types=1) to all files (maybe 10 - 20 files per iteration) and fix the type conversions
2. Once all files have this declare, I would start with replacing phpdoc annotations with php types. (also in multiple iterations)

Note
If I am talking about all files I mean currently only internal files and not the stubs

What is your opinion on this?
(I could also work on this in one large PR, but this would make it quite hard to review)

Passkey registration: "Claimed" accounts do not get removed

Right now, whenever you register an account using a passkey, the first step is to "claim" the user by creating it in the database. Once the user record exists, the system can use this record to generate the passkey / WebAuthn "creation options" (challenge), which once confirmed essentially gets added to that record in order to "complete" registration.

However, when an user does not confirm this, you end up with an account that does not have a valid authentication method. To solve this, I have come up with two solutions:

1. While the user is still in the registration state, and they cancel out of passkey-based registration (e.g. to use password-based registration), we can send a DELETE request to the register endpoint which can then delete the "claimed" account immediately. This way the user can directly submit a password-based registration, without getting stuck with an "account already exists" error. This is mostly an adapter/UI-feature, but within laravel-auth it means adding a new route.
2. When the user does not cancel out (and instead leaves the page / the session expires etc.), we can still clean these accounts periodically using Laravel's prunable method: https://laravel.com/docs/9.x/eloquent#pruning-models

Add additional test coverage for Str::transliterate / Str::lower

The library currently implements (amongst others) per-username rate limiting. While traditionally rate limiting an IP-address would have been mostly sufficient, these days it is fairly trivial for attackers to proxy their requests and to simply switch to 'fresh' IP address once a rate limit / block has been reached. As such, we also want to rate-limit on the username itself, making this 'brute force' attack significantly less effective.

Now, we already do have tests that ensure an username gets rate limited, and we currently also already do make sure that usernames and emails such as [email protected], [email protected] (casing) and fσσ@example.com (transliteration) are treated the same, but we don't specifically have tests that cover these edge-cases.

As such, if an user decides to override and remove/change this functionality, they might expose themselves to risk, and it is for this reason that we want to ensure that two additional tests exist.

Consider adding Larastan on Level 0

Then I would say we leave the stricter type system open for now. (Maybe use larastan on level 0 for now to already find smaller problems and then later on improve incrementally the type system.)

Originally posted by @Jubeki in #25 (comment)

Error After Adding OTP

Hi,

I've come across a re-accruing bug I wanted to bring to your attention.

I'll explain the steps below on how to get to it.

1. I've created a User, regular with email auth
2. I add a OTP
3. I logout and log back in
4. I enter the OTP and am hit with the bug

Here is the error in question

When I hit back and refresh, I am logged in.

This has happened to me on Safari and Brave. I'm on a Mac if this helps -- unlikely.

Recovery: No way to actually reset a password or passkey

Due to an oversight, it's currently only possible to use recovery codes / account reset links to regain access to the account, but it not to reset your credentials once authenticated (as changing your password at that point requires your current password to be provided)

To solve this, I'll change the way account recovery works. Instead of directly authenticating the user and sending them to the account settings page, I will make it so that the user enters a "confirmed recovery state" (similar to the 2FA challenges) where the user can then (depending on the account type) either register a new passkey, or choose a new password.

Once a new credential has been registered and the recovery mode cleared, the user will be returned to the login page.