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

User-Facing Forgot & Reset Password Pages #89

Closed
rafecolton opened this issue Feb 8, 2016 · 18 comments

Comments

@rafecolton
Copy link
Contributor

commented Feb 8, 2016

Along with hiding the wordpress admin bar and wp-admin for certain user types, this would make for a completely user-facing login and registration workflow.

I would need to add settings to make hiding the admin bar and wp-admin optional (it's hardcoded in ours), and the code could probably use some feedback on organization, but the core functionality is already done and tested for our site.

Is this a feature you would be interested in?

@ericnicolaas

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2016

@rafecolton Absolutely!

@ericnicolaas

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2016

Just following this up, I'm going to suggest we work on merging this in as part of the 1.4 release.

@ericnicolaas ericnicolaas modified the milestone: 1.4.0 Feb 8, 2016

@rafecolton

This comment has been minimized.

Copy link
Contributor Author

commented Jun 17, 2016

Sorry for the huge delay, we had to shift priorities away from this project for a bit. Back on it and working on this feature right now.

@ericnicolaas

This comment has been minimized.

Copy link
Contributor

commented Jun 20, 2016

No problems @rafecolton! Great to hear from you again; it'll be great to have some help on this part of the plugin.

@ericnicolaas

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2016

Thanks for the PR @rafecolton! Really appreciate your contribution.

I have quite a few thoughts on this; sorry if they end up a bit jumbled below.

First, one issue that's preventing me from testing this out locally is that there are a whole bunch of notices & warnings:

[30-Jun-2016 06:08:41 UTC] PHP Notice:  Undefined index: view_wp_admin_bar in /vagrant/content/plugins/charitable/includes/user-management/class-charitable-user-management.php on line 187
[30-Jun-2016 06:08:41 UTC] PHP Warning:  array_merge(): Argument #1 is not an array in /vagrant/content/plugins/charitable/includes/user-management/class-charitable-user-management.php on line 187
[30-Jun-2016 06:08:41 UTC] PHP Warning:  array_diff(): Argument #2 is not an array in /vagrant/content/plugins/charitable/includes/user-management/class-charitable-user-management.php on line 188
[30-Jun-2016 06:08:41 UTC] PHP Warning:  array_intersect(): Argument #1 is not an array in /vagrant/content/plugins/charitable/includes/user-management/class-charitable-user-management.php on line 209
[30-Jun-2016 06:08:41 UTC] PHP Warning:  array_intersect(): Argument #1 is not an array in /vagrant/content/plugins/charitable/includes/user-management/class-charitable-user-management.php on line 210
[30-Jun-2016 06:09:46 UTC] PHP Notice:  Undefined index: view_wp_admin_bar in /vagrant/content/plugins/charitable/includes/user-management/class-charitable-user-management.php on line 187
[30-Jun-2016 06:09:46 UTC] PHP Warning:  array_merge(): Argument #1 is not an array in /vagrant/content/plugins/charitable/includes/user-management/class-charitable-user-management.php on line 187
[30-Jun-2016 06:09:46 UTC] PHP Warning:  array_diff(): Argument #2 is not an array in /vagrant/content/plugins/charitable/includes/user-management/class-charitable-user-management.php on line 188
[30-Jun-2016 06:09:46 UTC] PHP Warning:  array_intersect(): Argument #1 is not an array in /vagrant/content/plugins/charitable/includes/user-management/class-charitable-user-management.php on line 209
[30-Jun-2016 06:09:46 UTC] PHP Warning:  array_intersect(): Argument #1 is not an array in /vagrant/content/plugins/charitable/includes/user-management/class-charitable-user-management.php on line 210

Mostly this appears to be because of missing settings. I think if you test this on a completely fresh install you should be able to replicate this.

One thing I should mention is that the current master branch includes password fields in the Profile form. Sorry for not mentioning this before; I'd been thinking mostly about the Forgot Password flow. I guess though that the Reset Password form you have here is designed instead for resetting of the password after running through the Forgot Password routine -- is that right?

A bigger question at the moment for me is whether to go down the track of adding the Forgot Password through a separate shortcode. Your work here has made me reconsider how all the account-related functionality is Charitable is organized. I've posted an epic set of thoughts on that in #144. But in terms of how to build in the Forgot/Reset Password into this round, I'm thinking that we could build this out as part of the login form; so instead of requiring a separate shortcode, clicking the link takes you to a new page that might be something like domain.com/login/forgot-password/.

@ericnicolaas

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2016

On a separate note, could you re-push the PR against the issue/89 branch instead of master? That way, we can work on this in a separate branch and then merge it into master when it's ready.

@rafecolton

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2016

It looks like you re-pushed against pr/143 in d76a8a1 - is that right?

I will look at the notices and fix it against that branch.

I noticed the password reset fields in the Profile form, which I think is a great addition. Both the reset and forgot password pages I have added replace the default WordPress ones. The idea is that somebody can use the site with an entirely user facing login/registration/password workflow and never reach /wp-admin or /wp-login.php.

I think the solution here is to integrate these changes along with the existing login and registration shortcodes and then to refactor them with the approach you discuss in #144. I will leave more thoughts on that issue when I have a minute.

@ericnicolaas

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2016

Hmm, looks like the push to that branch was a mistake. issue/89 would be the one to use for this issue.

Spot on with the approach. My thinking is that these could both be added as endpoints, and then the content is dynamically generated without any need for shortcodes.

@ericnicolaas

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2016

OK, I've had a chance to play around with your changes in PR #150. Thanks for fixing up all the notices; much easier to use it now :)

I think the user management functionality is probably a little more advanced than we need in Charitable. There are plugins out there that allow the per-role locking down of wp-admin and the admin bar. For Charitable's purposes, I think we should find a sane default and auto-enable it for users.

Based on that, I would suggest leaving the settings area basically as it is now in master, without the User Management tab for now (I can see a use-case for that when we implement #144). Instead of all the settings, here is what I would propose:

  • Forgot Password Page - Auto-generate this URL as an endpoint based on the login page. If the login page setting is using the default WP login, we do not do anything here. If the login page is set to a specific page in WordPress (i.e. with the shortcode), the forgot password page URL becomes /login/forgot-password/. (Note: I'm making the assumption that the login page's slug is login. If it's hotdog, the forgot password page is at hotdog/forgot-password/.
  • Reset Password Page - Same logic as the Forgot Password Page. When using a specific page for the login, the reset password page URL is /login/reset-password/.
  • View Admin Bar - Default to removing this for any user that does not have either of the edit_posts or manage_charitable_settings caps.
  • View WP Admin - Default to redirecting any user that does not have either of the edit_posts or manage_charitable_settings caps.
  • 404 Page - I would suggest that the Profile page would be a good place for users to be redirected to, particularly since that will become increasingly central when we build out #144. We can make this value filterable so that developers can override it, but my feeling is that the vast majority of users won't need the fine-grained control.
  • Hide Default WP Login Page - I'm a bit unsure about including this at all. I don't like the idea of hiding wp-login by default if you're using a custom login page, because I can see people getting really confused by that (read: I can see me getting lots of support questions). I understand the motivation behind it, but at this point I don't think it's something that Charitable needs to do; if our login process already provides reset & forgot password options, then from an end user perspective they would not stumble across this page unless they're looking for it.

Does all that make sense?

I'm going to begin implementing the endpoint/routing for the Forgot & Reset Password pages; will push any changes I make back to this branch.

@ericnicolaas

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2016

I started fleshing this out a bit in 94a4103. For now I'm not touching the backend; just focusing on the frontend experience.

What my changes do so far:

  • Added forgot_password as an endpoint, and a rewrite rule so that it matches the routing I described above.
  • Refactored charitable_get_forgot_password_page_permalink(). It checks against the login_page URL and if that's custom, returns a dynamic URL that matches the routing for the forgot_password endpoint. If the login page is the standard WP login, the default forgot password page link is used.
  • Added the link above to the Login form, so it sends people to the right place to reset their password.
  • Added charitable_is_forgot_password_page().
  • In Charitable_Templates, check whether it's the forgot password page, and if so, create a Ghost Page with just the page title. Required to prevent 404 errors.
  • Added charitable_template_forgot_password_content(), which is run on the forgot password page's the_content filter, thereby displaying the FP form.

Next is the reset password page, but this should basically give you a pretty clear idea of the direction I want to move in.

ericnicolaas added a commit that referenced this issue Jul 13, 2016
Added our own password retrieval function so that we can send a Chari…
…table email, instead of using the default email. #89
@ericnicolaas

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2016

I have fleshed out the email that is fired off after the Forgot Password form is submitted. Instead of using the default WP email, I have created a new Charitable email so that we have a bit more control over this and it looks consistent with other emails the user would receive.

Also set up the /reset-password/ endpoint. I haven't been able to successfully reset a password yet. Always end up with the "Your password reset link appears to be invalid. Please request a new link below" error. Will dig into this later, but let me know if you see why this might not be working.

@ericnicolaas

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2016

Just pushed up another commit: bed3428

I have been able to get the password reset working. @rafecolton, I ended up shuffling around parts of your code and disabling other parts (for now). The password reset link works a little differently now. It provides a link with the key & login appended as query strings, which is intercepted by Charitable in Charitable_User_Management::maybe_redirect_to_password_reset(). This then creates a cookie using the key & login and then forwards the user to the reset page without the query string.

This is based on how WooCommerce handles this, which in turn is based on how wp-login works itself.

The actual password reset happens in Charitable_Reset_Password_Form now (instead of Charitable_Reset_Password). I copied what you had in Charitable_Reset_Password::handle_password_reset() and refactored that a bit to keep it consistent with the rest of the code base. One thing to note is that you can use charitable_get_notices()->add_error() to record errors, and these will automatically be prepended to forms (provided the form has the charitable_form_before_fields hook). If you are doing a redirect, you can persist those notices to the session like this: charitable_get_session()->add_notices().

This all still needs a whole bunch of testing, but I'm fairly happy with the state of the forgot & reset functionality at the moment.

I'll post back later with some more thoughts about the rest of the functionality, like whether to keep the shortcodes and what to do about the wp-login.php stuff.

@ericnicolaas

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2016

In a65ab76:

  • Removed admin bar & admin access for users without edit_posts or manage_charitable_settings caps. There are three filters that can be used to change this behaviour (detailed below).
  • Redirect unauthorized users accessing wp-admin to the profile page by default, or the homepage if no profile page is set. There is a filter for this too.
  • Provide an option for all requests to wp-login.php to be redirected to the Charitable login. This is disabled by default and requires a filter to override.
  • Instead of redirecting users to the login page after filling out the forgot password form, redirect them to the same page but display a message instead of the form.

New filters:

  • charitable_disable_admin_bar - Boolean. true by default. Return false to enable the admin bar for everyone.
  • charitable_disable_admin_access - Boolean. true by default. Return false to enable the admin bar for everyone.
  • charitable_admin_redirect_url - String. Any returned URL must be one the same domain, since we're using wp_safe_redirect() for the redirection.
  • charitable_user_has_admin_access - Boolean. true by default for users with edit_posts or manage_charitable_settings caps.
  • charitable_disable_wp_login - Boolean. false by default. Return true to remove wp-login access and forward people to the Charitable login. Note: This will be ignored if no Charitable login page has been set.
@ericnicolaas

This comment has been minimized.

Copy link
Contributor

commented Jul 14, 2016

@rafecolton: From your point of view, with the above filters and the changes I have made over the past few days, does this allow you to achieve everything you want on your site? I think it should, but I just want to make sure I'm not overlooking anything. I think that with the above filters, Charitable is providing the framework for you to disable admin bar, wp-admin and wp-login access for a set of users of your choosing.

@rafecolton

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2016

@ericnicolaas, thanks for being so thorough in your approach to this. I have several thoughts, which I have divided into 👍 (things I agree with), questions, and objections.

Overall, this seems like a good way to integrate the features we need while still keeping things simple and consistent with the rest of Charitable.

👍

  • Using auto-generated URLs for forgot and reset password per #144 is just fine and simpler than the user having to create a page for each
  • It is better to use the session to store key/login when redirecting to the reset password page than to keep key/login in url like WP does.
  • I added notices to the session because charitable_get_notices()->add_error() doesn't work with redirects. Using charitable_get_session()->add_notices() would be better. I can't remember if I didn't use it because it didn't work or I simply didn't know about it. Using that would allow for a good chunk of code to be removed.
  • Your choice for charitable_user_has_admin_access being users without edit_posts or manage_charitable_settings seems fine. It's simpler and does a better job of handling the case where new user permissions are added. (On that note, is there a permission set I could give users so they can only manage campaigns, donations, and charitable settings but nothing else?)

Questions

  • It looks like you haven't edited the settings yet from the ones I added. I assume this is intentional?
  • This is assuming that a user will want to use a custom forgot/reset password page if they are using a custom login page. For me, this assumption is fine. Can you think of a case where someone might object?
  • The redirect_to_charitable_login filter doesn't filter by request type - will this cause a problem with POST requests (e.g. logout)?

Objections

  • For this feature:

    Instead of redirecting users to the login page after filling out the forgot password form, redirect them to the same page but display a message instead of the form.

    The default behavior of wordpress is to redirect to the login form, which enables the user to login with the new password they just created. This behavior is strongly preferable, as it has less friction for users.

@ericnicolaas

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2016

It looks like you haven't edited the settings yet from the ones I added. I assume this is intentional?

Yep, just hadn't really focused on touching that yet.

This is assuming that a user will want to use a custom forgot/reset password page if they are using a custom login page. For me, this assumption is fine. Can you think of a case where someone might object?

I'm not 100% sure what you mean here. My intention at the moment is to remove the option for people to set a custom page for the forgot/reset password page. It should always just be the login page + endpoint for now. On that note, I will also remove the shortcodes.

The redirect_to_charitable_login filter doesn't filter by request type - will this cause a problem with POST requests (e.g. logout)?

Good question. I will test this out.

The default behavior of wordpress is to redirect to the login form, which enables the user to login with the new password they just created. This behavior is strongly preferable, as it has less friction for users.

OK, you have a point. I'll revert to that.

@ericnicolaas

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2016

@rafecolton Merged! Thanks for all your work on this. Really happy to have this included in Charitable.

@rafecolton rafecolton referenced this issue Jul 19, 2016
6 of 6 tasks complete
@ericnicolaas

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2017

This needs documentation. Particularly for the filters outlined in #89 (comment)

@ericnicolaas ericnicolaas removed the need docs label Nov 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.