MaterialDesignInXamlToolkit icon indicating copy to clipboard operation
MaterialDesignInXamlToolkit copied to clipboard

Add a revealed PasswordBox style

Open Drise13 opened this issue 3 years ago • 11 comments

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.

image

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)

Drise13 avatar Aug 10 '22 17:08 Drise13

@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.

nicolaihenriksen avatar Aug 20 '22 19:08 nicolaihenriksen

I think it is reasonable to implement this within the library. A few details on what I would expect for the implementation.

  • I really like how MahApps did it by using a behavior to sync between the plain 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.
  • I would create a separate style for this. It should be something that consumers have to opt-in for, rather than the default.
  • I would add properties 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.
  • As for validation, that should "just work" with regular binding validation once there is a simple string attached property to bind to.

Keboo avatar Aug 22 '22 23:08 Keboo

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.

Drise13 avatar Aug 23 '22 01:08 Drise13

  • 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:

  • We can reuse the existing 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.
  • We can expose the 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:

  • I am not sure how this works with Test Automation; I guess we may need to override OnCreateAutomationPeer() and return a PasswordBoxAutomationPeer passing in the encapsulated PasswordBox in the constructor - hopefully that works, but I don't know for sure.
  • In order for us to have the 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...

nicolaihenriksen avatar Aug 23 '22 07:08 nicolaihenriksen

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 image image

<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 avatar Sep 16 '22 06:09 HavenDV

@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.

nicolaihenriksen avatar Sep 16 '22 07:09 nicolaihenriksen

@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.

nicolaihenriksen avatar Sep 16 '22 13:09 nicolaihenriksen

@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: image image

HavenDV avatar Sep 19 '22 05:09 HavenDV

@HavenDV Wow, that is surprising to me. When I paste your example from above into the demo tool, I get the following:

image

When inspecting with Snoop, it is also clear that the two buttons are placed adjacent to each other in the grid:

image image image

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.

nicolaihenriksen avatar Sep 19 '22 07:09 nicolaihenriksen

Could I ask you to write up a simple repro sample?

material-ui-password-box-reveal.zip

HavenDV avatar Sep 20 '22 04:09 HavenDV

@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>

nicolaihenriksen avatar Sep 20 '22 08:09 nicolaihenriksen

This appears to be complete.

Keboo avatar Sep 26 '22 04:09 Keboo