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

Allow password reset with token alone #1295

Merged

Conversation

jkeen
Copy link
Contributor

@jkeen jkeen commented Jun 2, 2019

This is an attempt to resurrect the stagnant PR #1072 💀, which allowed for a reset password flow without needing the user to visit the API directly.

  1. user fills out password reset request form
  2. user is sent email
  3. user clicks confirmation link
  4. link leads to the client (instead of the API) with a reset_password_token
  5. user submits password along with reset_password_token
  6. user is now authorized and has a new password

This PR took the existing work of #1072, rebased master on it, fixed a couple of bugs, and updated some tests.

@jkeen jkeen force-pushed the forgot_pass_with_reset_password_token branch from f726795 to 18c1c77 Compare June 2, 2019 21:18
@MaicolBen
Copy link
Collaborator

@jkeen Awesome! Do you know why the specs are failing?

@jkeen
Copy link
Contributor Author

jkeen commented Jun 4, 2019

Looks like after rebasing onto master some of the changes to #create_token now returning an object instead of an array need to get updated in this branch. I'm on it

@jkeen
Copy link
Contributor Author

jkeen commented Jun 5, 2019

Now I don't know why the specs are failing.

@jkeen jkeen force-pushed the forgot_pass_with_reset_password_token branch 3 times, most recently from fe63b54 to d232301 Compare August 13, 2019 23:15
@jkeen
Copy link
Contributor Author

jkeen commented Aug 14, 2019

Finally figured out the intermittent test failure! So tests pass now @MaicolBen

@MaicolBen
Copy link
Collaborator

Awesome! It'd be great to fix Code Climate but not mandatory, I need to release a version before merging this

@jkeen jkeen force-pushed the forgot_pass_with_reset_password_token branch from c337593 to c93de06 Compare August 26, 2019 17:20
@@ -184,5 +194,23 @@ def validate_redirect_url_param
return render_create_error_missing_redirect_url unless @redirect_url
return render_error_not_allowed_redirect_url if blacklisted_redirect_url?
end

def build_callback_url(reset_password_token)
Copy link
Collaborator

Choose a reason for hiding this comment

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

can't you use DeviseTokenAuth::Url.generate for this?

setup { DatabaseCleaner.start }
teardown { DatabaseCleaner.clean }


Copy link
Collaborator

Choose a reason for hiding this comment

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

you can remove these unless it has a purpose

@@ -39,9 +39,12 @@ class ActiveSupport::TestCase
strategies = { active_record: :transaction,
mongoid: :truncation }
DatabaseCleaner.strategy = strategies[DEVISE_TOKEN_AUTH_ORM]
DatabaseCleaner.clean_with(:truncation)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was an attempt to combat the mysterious database constraint violations with duplicate emails while testing.

That validation problem I don't think is related to the changes on this branch, but I'm still trying to resolve it. I just made a change to ensure unique emails, so hopefully that fixes it

@jkeen
Copy link
Contributor Author

jkeen commented Sep 4, 2019

@MaicolBen updated!

@MaicolBen
Copy link
Collaborator

@jkeen Thank you!

@MaicolBen MaicolBen merged commit b8620bb into lynndylanhurley:master Sep 12, 2019
@lucasm-iRonin
Copy link

@jkeen thank you!

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.

3 participants