Skip to content
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

feat(text)!: add Masked to display secure data #168

Merged
merged 1 commit into from
May 9, 2023

Conversation

joshka
Copy link
Member

@joshka joshka commented May 7, 2023

Adds a new type Masked that can mask data with a mask character, and can be used anywhere we expect Cow<'a, str> or Text<'a>. E.g. Paragraph, ListItem, Table Cells etc.

BREAKING CHANGE:
Because Masked implements From for Text<'a>, code that binds Into<Text<'a>> without type annotations may no longer compile (e.g. Paragraph::new("".as_ref()))
To fix this, annotate or call to_string() / to_owned() / as_str()

This is an alternative to #32

asciicast

Copy link
Member

@mindoodoo mindoodoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome stuff, this looks great. Breaking change is relatively small and isn't an issue IMHO. I'd also add I much prefer this as opposed to #32, although it has the one of advantage of not having a breaking change.

@joshka
Copy link
Member Author

joshka commented May 7, 2023

Awesome stuff, this looks great. Breaking change is relatively small and isn't an issue IMHO. I'd also add I much prefer this as opposed to #32, although it has the one of advantage of not having a breaking change.

Yeah, I'm not sure I'd have noticed the breaking change if it wasn't for the two places where we have as_ref() calls. The impact is only really felt when there is some large string that's being copied (and the largest string that can be reasonably rendered on a screen isn't likely to ever be a perf issue, so meh). For this change I wouldn't bother with a major version bump ever. I only called it out as it's useful to have notes on what to fix when something breaks (or when assumptions about impact are wrong).

@mindoodoo
Copy link
Member

For this change I wouldn't bother with a major version bump ever. I only called it out as it's useful to have notes on what to fix when something breaks (or when assumptions about impact are wrong).

I think the next release will most likely have breaking changes anyways so might as well slip it in with it.

@mindoodoo
Copy link
Member

@joshka Regarding the information about the breaking change, did you find a way to be able to keep the commit footer and body on github ?

@joshka
Copy link
Member Author

joshka commented May 7, 2023

@joshka Regarding the information about the breaking change, did you find a way to be able to keep the commit footer and body on github ?

Yep merge / push changes via the cli rather than github. Rebase on main, test, push. Done. :)

Copy link
Contributor

@sophacles sophacles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it. Nice feature.

Idea for expansion of it in the future:
Have an option to mask only a subset of the text, for example:

Choose how you'd like to receive your authorization code:
 Email: a*****@gmail.com
 Phone: ***-***-1234

Etc.

Maybe in the form of

// constructor or setter on Masked
// args to masker: input txt, mask_char
fn masker(self, masker: Fn(&str, &str) -> &str) -> Self {...}

@joshka
Copy link
Member Author

joshka commented May 9, 2023

Idea for expansion of it in the future:
Have an option to mask only a subset of the text, for example:

The main benefit of having this as part of the library is that it is useful in its simple form, but I think once you go beyond a certain complexity on this, it's probably more the user's responsibility to be the one that handles their own input / output, and this isn't all that difficult. An example of keeping that idea super simple:

struct MaskedCreditCard<'a> {
    card_number: &'a str,
}

impl<'a> MaskedCreditCard<'a> {
    pub fn new(card_number: &'a str) -> Self {
        // in a real app, should never have anything more than the last 4 digit available
        // but for the sake of an example
        MaskedCreditCard { card_number }
    }
}

impl Default for MaskedCreditCard<'_> {
    fn default() -> Self {
        MaskedCreditCard::new("0000-0000-0000-0000")
    }
}

impl<'a> From<MaskedCreditCard<'a>> for Cow<'a, str> {
    fn from(value: MaskedCreditCard) -> Self {
        ("****-****-****-".to_string() + value.card_number.rsplit_once('-').unwrap().1).into()
    }
}

Adds a new type Masked that can mask data with a mask character, and can
be used anywhere we expect Cow<'a, str> or Text<'a>. E.g. Paragraph,
ListItem, Table Cells etc.

BREAKING CHANGE:
Because Masked implements From for Text<'a>, code that binds
Into<Text<'a>> without type annotations may no longer compile
(e.g. `Paragraph::new("".as_ref())`)

To fix this, annotate or call to_string() / to_owned() / as_str()
@joshka
Copy link
Member Author

joshka commented May 9, 2023

Rebased on main

@orhun orhun changed the title feat(text)!: add Masked to display secure data feat(text)!: add Masked to display secure data May 9, 2023
@orhun orhun merged commit 2f0d549 into ratatui:main May 9, 2023
@sophacles
Copy link
Contributor

sophacles commented May 9, 2023

Sure, not looking to make it complex, just was thinking it would allow the suer to handle the masking functionality for their own cases that aren't "all the text":

fn masked_credit_card(ccn: &str) -> Masked<'a> {
    Masked::new(ccn, "*").masker(|txt, mask| {
        let (to_mask, plain) = txt.rsplit_once('-').unwrap();
        let mut s = to_mask.replace(['0', '1', '2', '3', '4', '5', '6', '7', '8', '9'], mask);
        s.push_str("-");
        s.push_str(plain);
    })
}

and the user could call it in something like:

Paragraph::new(masked_credit_card(ccn).mask_char("!"))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants