-
Notifications
You must be signed in to change notification settings - Fork 111
Password change page #33
Comments
Hmm, I'm not really sure about this. My reasoning is that:
|
Alright, if it's not something you think should go into the lib, we'll implement it right away in our app then. :) |
Hmm, I'm not sure. I'll leave this open and think on it for a bit. Thanks for the suggestion! |
No problem, I was thinking that express-stormpath could use a page like this since I feel the whole concept of stormpath is to be some sort of drop in solution to manage users and a standalone password change page could be enough for a lot of people with small apps. Maybe I'm mistaken on the philosophy, but this is how I see it... a way for developpers to focus on anything other than the generic user management problem. |
+1 for this, having e-mail be a dependency for password resets feels clunky |
An email address is only required if you want the user to go through the password reset email workflow. I think @lemieux was suggesting adding a new default view for the use case: "I already know my current password, but I want to change it to a new value". Randall was questioning providing a default view for this because it appears to be functionality that is usually embedded in some other view location rather than being a whole page by itself. @sholladay are you advocating for a new default view? (I'm just trying to understand what people want) |
Yes I was suggesting a new view.
|
Yes, I am advocating for a new view. "Forgot password" and "change password" are never that far apart, so it seems to me like we should either have both or neither, as far as long-term feature justification goes. Another reason is that, as a new Stormpath user, if I'm only using the Express middleware, it's not obvious how to implement this on my side. |
I've come up with a possible fix; may I submit all code changes as a single commit or separately as multiple commits? |
Hey @denim2x, it'd be great if you could submit a PR to the develop branch in one big PR. I'll review things. Right now I'm still against providing this, as it doesn't make much sense to me. Here's my full reasoning (wall of text follows): For users who 'forget' their password, our existing password reset workflow is really the ideal way to allow a user to reset their password. We get the user to enter their username or email address, and then we send them an email with a link to click that takes them back to your site, and we provide a page there that allows them to reset their password. This makes perfect sense. This github issue is talking about providing a page so that users who are already logged in, and inside of a dashboard (presumably), can view a page to JUST change their own password to something else. Usually something like this is 100% dependent on the application a user is building. For instance, with password reset stuff, it's easy to provide that as we're providing top-level URLs for an application, eg: /forgot, /reset, etc. With a password change page, we'd have to start doing odd things that don't make sense for many applications (by default), eg: /users/password, or /accounts/password, or something similar. This means we'd be expecting the developer to make a /users/ or /accounts page, and have an entire sort of user profile management type thing in their site. Take github for instance, here's how the github 'password change page' looks: As you can see, there's a TON of context-specific information around this. Unlike a login page, registration page, or password reset page -- a password change page has TONS of specific site stuff, and doesn't make sense unless it's super duper customized. Because of the above reasoning, and also because of how easy this is to implement on your own, I don't think it's really appropriate to have this sort of feature in this library. If you wanted to make your own password change page, you could implement your own page (however you like) that just accepts a new password. On the backend, using express-stormpath, you can then update a user's password (without triggering an email or anything) by simply saying: req.user.password = 'newpassword';
req.user.save(); That's it! I'm still definitely open to this idea, but hopefully this helps explain why it's a hard thing to do and why I'm not crazy about it currently ^^ |
Also, for what it's worth, I had another idea related to this a while back which I think would be pretty cool: Another library that sits on TOP of express-stormpath which provides an entire CRM for user management. Something like a fully complete user dashboard. It could allow users to do things like:
|
Personally, I think both ideas are great. For me, using Stormpath comes down to the plug-and-play experience. Having an even more abstract package would be useful in many scenarios, especially for greenfield projects. But I definitely need a password change page, regardless of which module it comes from. It's unusual to have the time, skill, knowledge, and resources to build part of an application layer like this and not touch the rest of it. I hate to put forward the "all or nothing" argument, but the reality for most companies and projects I work with is that they either can't do this themselves for various reasons or they are building a custom, in-house security layer deeper in their stack. |
I'd like to know whether the bounty on this issue is still applicable or not. If anyone believes it would be more appropriate to implement this feature as a separate package, please let me know. |
As long as it becomes an official patch in some form or another (plugin, dependent package, etc) and is supported by Stormpath, I will accept it. Seems a bit harder than I thought when I first posted the bounty, so sorry about that. But it is still valid. I think the best path forward is a proof of concept (PR or separate repo) that @rdegges can get his hands on and discuss. |
I'm totally happy to review all branches / etc. with this stuff in there. I try to keep a really open mind to any contributions! <333 |
I've set up a minimal demo here. Just login using batman@mailinator.com:Passw0rd and you will be redirected to the password change page; here you should enter the old password first and then the new password: Batm0bile. After submitting you will be prompted to login; now you can verify whether the new password actually works. Easy as pie. @sholladay I was wondering if you could also review my price proposal and let me know what you think about it; also i'd like to thank you for your support. |
Looks good to me, @denim2x. This is pretty much exactly how I envisioned it. Could you please link to a repo or pull request with the proposed solution? I will consider the bounty details again after we figure out what is going to happen here. @rdegges if you had to boil down your concerns into a few words, would you say they are more UX related or architecture related? I hear your points on both. But I think the demo proves that a simple top-level URL could be a good, if not perfect, user experience. One which could be iterated upon in the future. And there doesn't seem to be any roadblock to factoring the code any way you desire. |
Created a new view, two new controllers, some new settings and a new test.
@sholladay I've set up two solutions: a fork of stormpath-express and stormpath-changer. |
Closing this issue, we may implement something like this as a plugin (aka another module) but don't intend to put this into the this library. |
As discussed in #20, express-stormpath should also provide a password change page. This should be almost the same as the password reset one, but instead of verifying a token, the current password should be provided. This page should only be accessible by authenticated users. I think it should also be possible to configure whether or not you should sign in again after the password change. Either way, all other sessions (or all sessions in case of a log out after the change) should be nuked to prevent unauthorized access from another computer/browser.
We are most likely going to tackle this problem in our app in the next two weeks and since this is a generic thing, I think it should go in express-stormpath.
Thoughts?
The text was updated successfully, but these errors were encountered: