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

[1.x] Return recovery errors under the recovery_code key #401

Merged
merged 1 commit into from
Aug 15, 2022

Conversation

jessarcher
Copy link
Member

This might be considered a breaking change...

Currently when the user sends a recovery_code, any basic validation errors will be returned under the recovery_code key. But if the provided recovery code is incorrect, then the error is returned under the code key instead.

It's not a big deal if all errors are displayed at the top of the page. But if the errors are displayed alongside the specific fields (like this Jetstream PR), then the developer really needs to look in two places for recovery code errors.

This PR returns all recovery code errors under the recovery_code key.

The only time this may be a problem is if the user has worked around this quirk by only checking the code key for recovery_code errors. If this is considered a breaking change, then I can target master instead.

@driesvints
Copy link
Member

Won't this break Laravel apps that update Fortify but not Jetstream? Also, we indeed had complaints in the past from people implementing Fortify themselves when we changed some error code keys. I feel it might be better to target master for this.

@jessarcher
Copy link
Member Author

I don't believe this will break existing Jetstream installs because they display all validation errors at the top of the page. The key isn't used afaik.

To me, this feels more like a bug fix. But, as you say, people may be depending on the code key for their recovery_code requests.

Note that this won't change the code key for standard 2FA logins. It's just recovery logins that send a bad code that may not see an error, depending on their customisations.

@sebastiaanluca
Copy link

sebastiaanluca commented Aug 18, 2022

But, as you say, people may be depending on the code key for their recovery_code requests.

Since Fortify only returned errors in the code key, we checked it like so 😅 So breaking change for us, but luckily our tests caught it.

I'm probably always the one complaining about it (since that one project uses custom views instead of Jetstream), so could breaking changes like this be avoided if we just use Jetstream + Fortify? They're probably kept in sync (in terms of front and back-end functionality and changes)?

Though perhaps these breaking changes, even potential ones, can be marked as such in the release notes and maybe part of a new major release? Or at least a minor, not a patch. I'm sure there are more people that use it as the back-end for their own auth views (as that's its use as described in the docs — https://laravel.com/docs/9.x/fortify), but they might not even know their auth flow is broken if they don't have (very specific) tests (that are basically duplicates of Fortify's).

@driesvints
Copy link
Member

@sebastiaanluca sorry about that... I was afraid you'd get caught again 😞

so could breaking changes like this be avoided if we just use Jetstream/Breeze + Fortify

Not really since Jetstream is a scaffold and existing views in apps don't get updated.

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.

None yet

4 participants