daveaglick / FluentBootstrap

Provides extensions, helper classes, model binding, and other goodies to help you use the Bootstrap CSS framework from .NET code.
http://www.fluentbootstrap.com
MIT License
200 stars 76 forks source link

InputFor for FormInputType.Password repopulates the field with submitted password (unlike Html.PasswordFor) when returning the submitted model to the view back. #29

Closed ryanelian closed 9 years ago

ryanelian commented 9 years ago

Quoting this stackoverflow page: http://stackoverflow.com/questions/5642849/asp-net-mvc3-html-passwordfor-does-not-populate

"This is as designed. Passwords are not filled to prevent accidental resubmits, and to prevent the page from containing unencrypted passwords. Obviously the password was wrong to begin with if you're posting back the credentials."

EDIT: whoops. email field in the picture was using my real email. removed.

daveaglick commented 9 years ago

The first issue that I see is that there is a password set in the model to fill the password box in the first place. Ideally, using .InputFor() would result in an empty text box in both Razor and FluentBootstrap, even with current behavior, because the model property being referenced is either null or empty. The fact that something is getting placed in the text box suggests that a password was passed to the view via the model. However, this particular issue isn't really the domain of a front-end library and is more an matter of following best practices for page generation.

That said, Razor appears to try and protect the developer from accidentally populating the password box. If you want to create a pre-populated password box in Razor, it looks like you actually need to call Html.TextboxFor() and then manually set the input type, or just use HTML directly. In other words, Razor takes the default position that a password box should be empty and the developer needs to do extra work to populate it.

FluentBootstrap takes a different default stance. Since it's intended to be a wrapper around the Bootstrap components, everything is designed to work like it would if you were writing straight Bootstrap HTML code (the exception is DisplayFor() and EditorFor() since those need to use the ASP.NET MVC template mechanism). In other words, FluentBootstrap assumes the developer knows that they're doing and doesn't try to "protect them" from mistakes. There may be good reasons why a developer wants to consciously populate a password box. If you do want to clear the password, you can set the model object to null or string.Empty before calling .EditorFor().

Does all this make sense? While my preference is to keep the current behavior since it's closer to what happens when you make a raw HTML text box with a password type, I'd be open to changing it to the Razor style of stripping password content if there's a strong desire to do so. Any other project watchers have input on this?

ryanelian commented 9 years ago

Ah, I see. What you said makes sense.

Although I honestly prefer the "safe" way to be the default and the "unsafe" way to be made explicit (Just to protect me from negligence from failing to strip password fields prior to returning to views and considering there's little reason to returning a pre-populated password field), I'll leave the decision up to you.

:+1:

daveaglick commented 9 years ago

Yeah, I started to question my stance as soon as I submitted the comment. Given that Razor does it this way, perhaps it does make sense to replicate that. Or even better, make a new PasswordFor() helper that matches the Razor behavior and leave the InputFor() as-is. Does that seem like a good compromise?

ryanelian commented 9 years ago

Adding PasswordFor that replicates Razor's behavior would be a godsend!

Although there might be arising questions about the differences between InputFor(, FormInputType.Password) vs PasswordFor for people who are new to the library.

But eh, as long as the difference is properly documented I guess it won't hurt.

:+1:

daveaglick commented 9 years ago

Okay, that's what I'll do then. Might take a few days find a chance, but I'll let you know when it's in the repo.

daveaglick commented 9 years ago

The new PasswordFor extension is in the develop branch. I hope to push it out to NuGet (along with some other changes) later today or tomorrow.