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

PasswordChar support for TextBox #1544

Merged
merged 16 commits into from May 15, 2018

Conversation

Projects
None yet
4 participants
@danwalmsley
Member

danwalmsley commented May 3, 2018

  • What does the pull request do?
    Adds password char to Textbox similar to WinForms textbox.

  • What is the current behavior?
    N/A

  • What is the updated/expected behavior with this PR?
    When password char is set to anything but '\0' then the textbox displays those chars instead of the text.
    Also Copy, Cut, is disabled.

Everything else work as normal so you can databind.

Some people may have concerns about security.

Checklist:

@danwalmsley

This comment has been minimized.

Member

danwalmsley commented May 3, 2018

Ok here is an initial insecure version, if people are happy with the implementation, ill add any unit tests / documentation necessary.

@danwalmsley danwalmsley changed the title from WIP: Add a PasswordBox control. to WIP: PasswordChar support for TextBox May 3, 2018

@@ -30,7 +30,10 @@ public class TextBox : TemplatedControl, UndoRedoHelper<TextBox.UndoRedoState>.I
nameof(CaretIndex),
o => o.CaretIndex,
(o, v) => o.CaretIndex = v);
public static readonly StyledProperty<char> PasswordCharProperty =

This comment has been minimized.

@grokys

grokys May 4, 2018

Member

Warning: OCD trigger! The rest of these property definitions are alphabetically ordered.

This comment has been minimized.

@danwalmsley

danwalmsley May 9, 2018

Member

Reordered :)

@@ -208,6 +232,12 @@ public string Text
}
}
public string DisplayText

This comment has been minimized.

@grokys

grokys May 4, 2018

Member

I think there's probably an easier way to do this:

  • Pass the text to the TextPresenter like normal
  • Add a PasswordChar property to TextPresenter and bind it
  • In TextPresenter.CreateFormattedText, if PasswordChar is set, simply replace the text for display there

Would that work?

This comment has been minimized.

@danwalmsley

danwalmsley May 6, 2018

Member

Sure, will get this done

This comment has been minimized.

@danwalmsley

danwalmsley May 9, 2018

Member

Implemented as requested.

@Karnah

This comment has been minimized.

Contributor

Karnah commented May 8, 2018

Good day. Can i ask question? Why passwordbox insert into textbox? I think that passwordbox must be a separate sealed class with SecureString property. Yes, like in WPF. It seems to me this is the mostly secure case.
If you create a separate control now - in the future it will be easier to modified. And there will be no need to maintain backward compatibility in TextBox.
But, as i remember, in Xamarin using one control too. But may be exists some reason there?

@danwalmsley

This comment has been minimized.

Member

danwalmsley commented May 9, 2018

For now we wanted a quick way to get password input, for 99% use case where people are not worried about securestring. A secure PasswordBox control could indeed be implemented seperatly in future.

@Karnah

This comment has been minimized.

Contributor

Karnah commented May 9, 2018

In the first commit you created separate control. Why did you change your mind?
Sorry if i'm asking goofy questions and wasting your time, but i'm very curious.

@danwalmsley

This comment has been minimized.

Member

danwalmsley commented May 9, 2018

That was following a discussion with @grokys, and after the realisation that binding couldnt be supported in this scenario.

@danwalmsley danwalmsley changed the title from WIP: PasswordChar support for TextBox to PasswordChar support for TextBox May 9, 2018

@danwalmsley

changes submitted.

@@ -38,12 +38,13 @@
Text="{TemplateBinding Watermark}"
IsVisible="{TemplateBinding Path=Text, Converter={x:Static StringConverters.NullOrEmpty}}"/>
<TextPresenter Name="PART_TextPresenter"
Text="{TemplateBinding DisplayText, Mode=TwoWay}"
Text="{TemplateBinding Text, Mode=TwoWay}"

This comment has been minimized.

@Karnah

Karnah May 10, 2018

Contributor

May be OneWay? Otherwise TextPresenter override TextProperty in TextBox like '******'
Sample for testing:

<TextBox Name="TextBox" Width="200" 
         Watermark="Password Box"
         UseFloatingWatermark="True"
         PasswordChar="*"
         Text="Password" />
<TextBox Text="{Binding ElementName=TextBox, Path=Text}" />
@Karnah

This comment has been minimized.

Contributor

Karnah commented May 10, 2018

Yes, in WPF i allways create binding using AttachedProperty.
Can i try explore this question in details and create separate control with binding supporting? I'd like time until Monday. If I disappear this will mean that I overestimated my strength :)

@grokys

This comment has been minimized.

Member

grokys commented May 10, 2018

@Karnah I'm interested in how you use the WPF PasswordBox with SecureString.

What do you do with the SecureString? Do you store it on disk or send it to the network? As far as I know to do this, it needs to be converted to a string, which completely negates the use of SecureString anyway.

My feeling is that if an attacker has access to your process you're pretty much screwed anyway. Add to this the fact you can't do anything with a SecureString without converting it to a string then PasswordBox seems completely pointless to me. However I'm not a security expert so happy to be proved wrong.

@Karnah

This comment has been minimized.

Contributor

Karnah commented May 10, 2018

@grokys, default i use SecureString. When i need original value, i convert to unsafe string, use it, and than clean unsafe string like this:

unsafe {
    fixed (char* ps = input) {
        for (int i = 0; i < input.Length; ++i) {
            ps[i] = ' ';
        }
    }
}

This method may have problem with 3rd party libraries and intern strings, but it seems to me it more secure :)
Yes, i understand that this can be my frills, but paranoid inside me sadness, when i can't use SecureString instead string

@danwalmsley

This comment has been minimized.

Member

danwalmsley commented May 10, 2018

A PR for a securestring version would be greatly received. 😀

@danwalmsley

This comment has been minimized.

Member

danwalmsley commented May 10, 2018

@zii-dmg

This comment has been minimized.

Contributor

zii-dmg commented May 11, 2018

SecureString is hard to use in 'secure' way and from docs - 'Because of this platform dependency, SecureString does not encrypt the internal storage on non-Windows platform'. Is it worth it?

@@ -204,6 +213,11 @@ internal void CaretIndexChanged(int caretIndex)
protected override FormattedText CreateFormattedText(Size constraint)
{
if (PasswordChar != default(char))
{
Text = new string(PasswordChar, Text.Length);

This comment has been minimized.

@grokys

grokys May 11, 2018

Member

You don't want to set the Text property here. This is two-way bound with TextBox.Text! If you look in the devtools you'll see that TextBox now has "*****" as its text. This isn't what we want. Just use a local variable.

This comment has been minimized.

@danwalmsley

danwalmsley May 14, 2018

Member

Ok with a little refactoring to CreateFormattedText in the base class TextBlock so that it accepts the string to be rendered this issue is now resolved.

@grokys

This comment has been minimized.

Member

grokys commented May 11, 2018

SecureString is hard to use in 'secure' way and from docs - 'Because of this platform dependency, SecureString does not encrypt the internal storage on non-Windows platform'. Is it worth it?

Personally, I don't think so.

@Karnah

This comment has been minimized.

Contributor

Karnah commented May 12, 2018

@zii-dmg, wow, I didn't know this! This is very interesting! And this is pity. I think SecureString must be standard for some methods/classes in .Net.
@grokys, now there are a less benefits for the separate control. But i still dream about it. WPF realization of PasswordBox is more harder and interesting than I thinked. And I don't know how implement this in Avalonia. If I can create it, I'd like you reviewing.
Thanks a lot for discussing! It was very interesting!

danwalmsley added some commits May 14, 2018

@grokys

grokys approved these changes May 14, 2018

LGTM except for a small matter of a non-gramatical doc comment!

@@ -348,14 +348,15 @@ public override void Render(DrawingContext context)
/// Creates the <see cref="FormattedText"/> used to render the text.
/// </summary>
/// <param name="constraint">The constraint of the text.</param>
/// <param name="text">The text to generated the <see cref="FormattedText"/> for.</param>

This comment has been minimized.

@grokys

grokys May 14, 2018

Member

This comment doesn't make sense :) "The text to format." (or something similar) might be clearer

@@ -82,17 +82,11 @@ public override void Render(DrawingContext context)
/// Creates the <see cref="FormattedText"/> used to render the text.
/// </summary>
/// <param name="constraint">The constraint of the text.</param>
/// <param name="text">The text to generated the <see cref="FormattedText"/> for.</param>

This comment has been minimized.

@grokys

grokys May 14, 2018

Member

Non-gramatical comment here too. You can use /// <inheirtdoc/> here to prevent copy-pasta errors.

grokys added some commits May 15, 2018

@grokys

This comment has been minimized.

Member

grokys commented May 15, 2018

@danwalmsley fixed those doc comments myself instead of waiting for the back-and-forth. Hope that's OK, will merge when CI passes.

Thanks for implementing this!

@grokys grokys merged commit ffbc42d into master May 15, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@grokys grokys deleted the feature/password-box branch May 15, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment