Closed Drise13 closed 1 year ago
@Drise13 This seems like a valid request, but I am wondering whether there are some security concerns to be aware of here. The PasswordBox
in WPF stores the entered password in a SecureString
(whether that is secure or not is another discussion). Opting to reveal the password would require pulling the password out of the SecureString
and into a clear-text string which naturally will be stored in memory. This can be picked up by a process dump and memory inspection. @Keboo thoughts?
To address the issue you have with the awkwardly placed reveal button, I think you could solve that quite easily. I would do something akin to this:
<Grid>
<PasswordBox Padding="0,0,30,0" />
<TextBox Padding="0,0,30,0" />
<Button Width="24" HorizontalAlignment="Right" />
</Grid>
The Padding
on the PasswordBox
/TextBox
ensure there is room for the button "inside" the PasswordBox
. Padding
and Width
of course need to be adjusted to the values that match your button.
If the above approach is not feasible in your control, you could also apply the Padding
on the PasswordBox
/TextBox
like above, and then simply use a negative Margin.Left
property on your button.
I think it is reasonable to implement this within the library. A few details on what I would expect for the implementation.
string
and SecureString
properties. Because the normal password is not a dependency property, registering for events is needed to manually manage synchronization between them. I would put the attached property within a new PasswordBoxAssist
class.PasswordBoxAssist
class with several attached properties, a boolean indicating if we are in a revealed state or not, and one for the reveal icon (PackIconKind
enum). For now, the icon can simply default to the standard "eye" (like in the screenshots above). But having this property allows people to change it, or toggle it based on revealed state if that is desired.For my specific use case, we rarely need a proper SecureString
. And generally, PasswordBox
(at least in my experience of it) breaks the MVVM paradigm and generally makes my life more difficult for some vague notion of "security". Like @nicolaihenriksen mentioned whether that is secure or not is another discussion. (As a side note, Dotnet even recommends against using SecureString
since it doesn't do anything except implement IDisposable
on anything but Framework
).
My latest "hack" to do what I want with as little effort as possible is to use a single textbox and swap between a password and standard font. A password font is one that replaces all codepoints with a ●
character and that seems to do what I want. I really don't think for most of my needs I really need to visually hide the password, but that seems to be the common interface Google uses. In their Home app to setup the Wifi network, they mask it by default but don't require any authentication to unmask the password.
This "hack" lets me using a single binding and its 100% MVVM compatible. I don't know if it would be within the MDIX purview to add a non-password "password box" (UnsecurePasswordBox
maybe?) that explicitly makes the choice not to use the SecureString
scheme that PasswordBox
uses and be more MVVM friendly. Just a thought.
- I would create a separate style for this. It should be something that consumers have to opt-in for, rather than the default.
Sorry about the long response below :-)
@Keboo I agree to the opt-in part of this. What nags me a bit about this approach is that we have 4 different PasswordBox
styles today: default, floating-hint, filled, and outlined (the three latter are derived styles). We would need to replicate these 4 in a "reveal flavor". And since ControlTemplates
cannot be extended/modified (but only overridden) in a style, we would effectively create a copy of the default PasswordBox
style and add in the reveal button (and TextBox
for editing the revealed password). This means we would need to ensure these stay in sync.
Another approach could be to wrap the PasswordBox
inside of a custom control (e.g. UnsecurePasswordBox
as @Drise13 suggests). I see this as having (at least) 2 advantages:
PasswordBox
styles for the encapsulated PasswordBox
and thus not need to keep 2 styles in sync. We would still need to extend the default PasswordBox
style with the ability for a "reveal button", but that could be done using an internal attached property. This is potentially also a disadvantage; see below.UnsecurePasswordBox.Password
string as a DependencyProperty
which lends itself nicely to the MVVM paradigm. Other properties/methods may need to be delegated into the encapsulated PasswordBox
directly.However I also see some disadvantages, including:
OnCreateAutomationPeer()
and return a PasswordBoxAutomationPeer
passing in the encapsulated PasswordBox
in the constructor - hopefully that works, but I don't know for sure.ControlTemplate
include a TextBox
in order to edit the revealed password, we would need to modify the default PasswordBox
style to include this, which sort of defeats the opt-in functionality because the default style would now include a TextBox
with the password in clear text although it may be hidden (or otherwise removed from the Visual Tree).Perhaps having 4 dedicated "copies" of the original PasswordBox
styles is the safer approach after all...
Trying to use the new MaterialDesignFloatingHintRevealPasswordBox
style. There seem to be problems here in some cases, it might be worth repeating the behavior of TextFieldAssist.HasClearButton
<PasswordBox
Style="{StaticResource MaterialDesignFloatingHintRevealPasswordBox}"
Margin="0"
FontSize="18"
Foreground="Black"
material:ValidationAssist.HorizontalAlignment="Right"
material:ValidationAssist.Background="Transparent"
material:TextFieldAssist.HasClearButton="True"
material:PasswordBoxAssist.Password="123"
>
<material:HintAssist.Hint>
<StackPanel Orientation="Horizontal">
<material:PackIcon Kind="Key" VerticalAlignment="Center"/>
<TextBlock Text="Password" />
</StackPanel>
</material:HintAssist.Hint>
</PasswordBox>
@HavenDV I'll look into it when I have some time - hopefully sooner rather than later.
For your particular issue, could the cause of the issue be the fact that you set an explicit FontSize
? If you leave that out, does it look correct? Anyways, your suggestion about following the TextFieldAssist.HasClearButton
approach seems reasonable, I will look into that.
@HavenDV I added a PR #2864 which hopefully should resolve the issue. Could you try it out once there is a nightly build available? Thanks.
I tested with your example (and variants thereof) in the demo app, and it seems to work as expected.
@HavenDV I added a PR #2864 which hopefully should resolve the issue. Could you try it out once there is a nightly build available? Thanks.
I tested with your example (and variants thereof) in the demo app, and it seems to work as expected.
I try 4.7.0-ci306
and see the following:
@HavenDV Wow, that is surprising to me. When I paste your example from above into the demo tool, I get the following:
When inspecting with Snoop, it is also clear that the two buttons are placed adjacent to each other in the grid:
It almost seems like you have a Style
that kicks in and changes the VerticalAlignment
property of the reveal button (or possibly the clear button)? Could I ask you to write up a simple repro sample?
Also, tracking the elements with Snoop like I have shown above often gives some good hints as to what is going on.
Could I ask you to write up a simple repro sample?
@HavenDV Thanks a lot. So it turns out it was not your code that was overriding a style, it was the demo tool. I had not noticed this, but Fields.xaml
includes this:
<Style TargetType="{x:Type materialDesign:PackIcon}" BasedOn="{StaticResource {x:Type materialDesign:PackIcon}}">
<Setter Property="VerticalAlignment" Value="Center"/>
<Setter Property="Margin" Value="4 0 4 0"/>
</Style>
Which effectively sets the VerticalAlignment
of the PackIcon
to Center
. I have created PR #2873 to fix the alignment directly on the "reveal button".
In the mean time, you could just override a style in your code, by inserting something like this in a resource dictionary (here I add it directly to the resource dictionary of the PasswordBox
itself):
<PasswordBox.Resources>
<Style TargetType="material:PackIcon" BasedOn="{StaticResource {x:Type material:PackIcon}}">
<Setter Property="VerticalAlignment" Value="Center" />
</Style>
</PasswordBox.Resources>
This appears to be complete.
Describe the solution you'd like Looking for a toggle-based password box reveal style. Something similar to MahApp's revealed password box. MahApps seems to require mouse-down to reveal. I believe the correct Material interaction would be to toggle states.
Describe alternatives you've considered We currently have a custom control that just switches between a textbox and password box, but the toggle button is separate from the input control and seems awkwardly placed.
Additional context Possibly additional nice to haves would be able to directly control the icons used for each state. Also would be nice if it supported validation rules. (Think WIFI's 8-63 character requirement)