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

Implement proper account recovery #62

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from
Draft

Conversation

claudiodekker
Copy link
Owner

@claudiodekker claudiodekker commented Aug 5, 2023

This PR aims to solve #3

Closes #3

@Jubeki
Copy link
Contributor

Jubeki commented Aug 6, 2023

Questions in regarding the recover process:

  1. Should it be possible to Recover your account with your username?
  2. If not (1), should it be possible to recover your email address from the username?

@claudiodekker
Copy link
Owner Author

claudiodekker commented Aug 6, 2023

Good points. I was going to say "no" to both, but since emails aren't guaranteed to be unique this would pose a problem for username-based accounts. As such, I'm going to have to modify this to support username-based recovery requests. Thanks!

@claudiodekker
Copy link
Owner Author

Alright, apart from some minor things that are now temporarily failing, I believe this should account for the issue you brought up @Jubeki. Thanks again!

Comment on lines +48 to +51

Route::middleware([])->group(function () {
Route::get('/auth/reset', ['foo', 'bar'])->name('recover-account.reset');
});
Copy link
Contributor

@Jubeki Jubeki Aug 7, 2023

Choose a reason for hiding this comment

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

Suggested change
Route::middleware([])->group(function () {
Route::get('/auth/reset', ['foo', 'bar'])->name('recover-account.reset');
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay it seems the route is needed, but there is no Controller associated and an empty middleware array.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah, this is on purpose - hence the PR still being a draft. The actual 'reset' part is still TODO

@shane-smith
Copy link

Any updates on this one?

@claudiodekker
Copy link
Owner Author

claudiodekker commented Oct 12, 2023

@shane-smith Definitely! Work might not appear active on the repo at the moment, but I can ensure you it's very active in a separate (temporary) one 👍

@Jubeki and I have had extensive discussions about some of the problems we ran into with the approach we've taken so far, specifically surrounding account recovery with username-only accounts, which lead to essentially a rewrite of the current repository. It's 90-95% identical, but does a few things fundamentally different (e.g. no "username-based" or "email-based" accounts, both are possible now), and simultaneously addresses a couple of other issues that we ran into (and that are still TODO here in this "current version" of the repository).

Right now I'd say it's about 70% done, and I expect work to complete (and be PR'd here) within a week or two. Once thoroughly tested, I expect to also tag v1.0.0 as a result.

I'm genuinely super excited for it, as what we're working on so far feels so much better and more robust already. Can't wait to share the whole thing! But to give a bit of an impression; you can expect it to work almost identical to how Github's email & security management works :)

@shane-smith
Copy link

Your excitement is palpable, I look forward to it :)

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.

Recovery: No way to actually reset a password or passkey
3 participants