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: [M3-8719] - Add a 'Mask Sensitive Data' setting to Cloud Manager #11143

Merged

Conversation

mjac0bs
Copy link
Contributor

@mjac0bs mjac0bs commented Oct 22, 2024

Description 📝

There are several places in the Cloud Manager UI where sensitive data is displayed, and we currently lack a way to obscure this data in situations where users may want it to be present, but not visible in the UI.

We would like to offer a solution to this by adding a profile setting to mask sensitive data app-wide using the commonly used pattern of dots (such as in a password field) and a visibility (eye) icon to toggle visibility of masked fields individually. Sensitive data includes: IP addresses, contact/billing information (including usernames and email addresses), and security questions.

This toggle-able setting will be available to users via the Profile > Settings page.

Changes 🔄

  • A user can toggle on/off masking sensitive data via their profile settings.
  • When the Mask Sensitive Data setting is ON, sensitive data throughout the app is masked with placeholder text.
    • The data can still be copied when it is masked, if the original field is copyable (e.g. IP Address).
    • The visibility of individual fields can be toggled on/off with an icon next to the field.
    • The length of the masked placeholder text is has a default length or can be customized via prop.
  • When the Mask Sensitive Data setting is OFF, the UI displays as prod does today - all sensitive data is visible.
  • Updated the IPAddress and CopyTooltip components to be able to mask IP addresses and copy the unmasked IP address while displaying masked text.
  • Created a two new components (with accompanying tests): MaskableText and VisibilityTooltip (and a story)
  • Created a new util: createMaskedText to convert a string to masked placeholder text (the dots).

Target release date 🗓️

11/12/24

Preview 📷

Screenshots

Page Screenshot
Profile Settings MaskedPreferenceOn MaskedPreferenceOff
Linode Landing MaskedLinodeLanding
Linode Details MaskedLinodeDetailsSummary MaskedLinodeDetailsNetworkIPs
NodeBalancer Landing MaskedNodeBalancerLanding
NodeBalancer Details MaskedNodeBalancerDetailsIPs
Account - Billing MaskedBillingDetails
Account - Users MaskedUsersLanding MaskedUserProfileDetails
Account - Login History MaskedLoginHistory
Profile - Login & Auth MaskedProfileLoginAuth
IPAddress with showAll MaskedMultipleIPs UnmaskedShowAllIPs
IPAddress with showMore MaskedShowSingleIP) UnmaskedShowSingleIP

How to test 🧪

Prerequisites

(How to setup test environment)

Verification steps

(How to verify changes)

yarn test MaskableText VisibilityTooltip createMaskedText IPAddress CopyTooltip
  • Let me know if there's anywhere I missed that you think should be masked!

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description (😄)
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

@mjac0bs mjac0bs self-assigned this Oct 22, 2024
@mjac0bs mjac0bs force-pushed the M3-8719-add-mask-sensitive-data-setting branch from 8ec6f6b to 8765249 Compare October 24, 2024 14:13
@mjac0bs
Copy link
Contributor Author

mjac0bs commented Oct 30, 2024

I'm sorry to be leaving my review so late but I had a couple notes as well as a number of places where we may want to consider deploying this tooltip.

@hkhalil-akamai Super helpful thoughts - thank you for the review! I'm looking into masking those places (except with VPC private IP, I think you're correct) now. As long as they play nicely with MaskableText, I think those are reasonable additions to make to this PR even though it will grow in size. If there are spots that become more complicated, I might punt them to a 'part 2' PR and get it into the same release.

One possible consideration is that the length of important data can be sensitive and should be obscured too. For example, it may be possible to determine which username is being hidden if the attacker posses a list of possible usernames and their lengths. We could instead default to a set length when the text is hidden, or even allow this fixed length to be customized via a prop.

I have conflicting thoughts here. I understand the scenario you present, it's valid. Defaulting to a set length also results in more abrupt changes in the UI when a user goes to toggle the visibility of the data, which was some feedback the team had when viewing the POC. And if we were to offer a default length, what length would be best - something about the length of an IP?

@@ -11,7 +11,6 @@ export const StyledTableRow = styled(TableRow, {
})(({ theme }) => ({
'& svg': {
height: `12px`,
opacity: 0,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this, because without doing so, the masked text visibility toggle icon wasn't visible. And it didn't seem to actually have a purpose.

@hkhalil-akamai
Copy link
Contributor

I have conflicting thoughts here. I understand the scenario you present, it's valid. Defaulting to a set length also results in more abrupt changes in the UI when a user goes to toggle the visibility of the data, which was some feedback the team had when viewing the POC. And if we were to offer a default length, what length would be best - something about the length of an IP?

I don't mind going in either direction, but I would like to hear the thoughts of other members. The length of an IP is a reasonable default, but we should make it configurable via a prop so it can be adjusted based on what kind of information it's masking.

@mjac0bs
Copy link
Contributor Author

mjac0bs commented Oct 31, 2024

Things that have changed recently:

  • Removed the MaskableText story from this PR - was having issues with msw mocking of an endpoint and want to troubleshoot in a follow up (M3-8819).
  • Added more masking in the places Hussain pointed out above. Expand the Screenshots accordion at the bottom of this comment for pictures.
  • Created an optional length prop on MaskableText and revised createMaskedText.
    • We can use a few defaults for the lengths of maskable text fields (allows us to limit some of the jumpiness in the UI) and we also avoid revealing the length of the plaintext in most places.
      • Exceptions:
        • Kube API Endpoints (too jumpy otherwise)
        • Billing Contact Info (it looked very weird to have uniform rows of data... open to feedback here; in my opinion, we should improve the display of info here overall (with labels) in another ticket and revisit masking then).
  • Added a maskedTextLength prop to CopyTooltip - it was necessary for the length issue above.
  • Updated unit tests accordingly, especially createMaskedText.test.tsx.
Screenshots

Screenshot 2024-10-31 at 4 58 02 PM

Screenshot 2024-10-31 at 4 58 16 PM

Screenshot 2024-10-31 at 4 58 26 PM

Screenshot 2024-10-31 at 4 58 44 PM

NOTE: IP addresses are not toggleable here because of the nature of the drag-and-droppable rows. If we unmask the IPs for one rule, but then move the rule around, the unmasking remains a property of the index of the original row instead of the moved row. (e.g. Unmasked row 2, move it to position 3 in list, row 2 has another rule that is now unmasked.)
Screenshot 2024-10-31 at 4 58 57 PM

Screenshot 2024-10-31 at 4 59 15 PM

Screenshot 2024-10-31 at 5 01 54 PM

Note: CI test failure is unrelated (database-resize) and fixed in develop.

Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

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

Thanks for considering feedback! Verified all sensitive areas have been covered. The maskedTextLength is a sensible and elegant approach to handling the length and gives excellent flexibility to developers.

One thought that didn't occur to me before -- should we update our documentation to mention using MaskableText when appropriate? It may be forgotten by developers, especially if they haven't turned on the feature for themselves. This mention could possibly go under "Component Library" or "Accessibility".

Overall, excellent work on this feature and I'm sure this many users will find this addition extremely useful! 🎉

Copy link
Contributor

@hana-akamai hana-akamai left a comment

Choose a reason for hiding this comment

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

Nice work! Looking forward to easier screenshots and videos 🚀

@hana-akamai hana-akamai added the Approved Multiple approvals and ready to merge! label Nov 1, 2024
@mjac0bs
Copy link
Contributor Author

mjac0bs commented Nov 1, 2024

Thanks for the reviews - I'm going to merge this once CI finishes. 🎉

@hkhalil-akamai Great callout.

I mentioned this feature in a few places that seemed appropriate:

  1. Contribution guidelines under submitting a PR request (number 7) - we point new contributors here often
    Screenshot 2024-11-01 at 10 42 48 AM

  2. Actual PR template under the screenshots section - we all see this every day
    Screenshot 2024-11-01 at 10 45 32 AM

  3. Component Structure page of the developer docs - to help remind devs that this feature exists when they're developing new components
    Screenshot 2024-11-01 at 10 44 47 AM

I don't think this is overkill, since we want to actually make use of it!

@mjac0bs mjac0bs merged commit 42f6cc2 into linode:develop Nov 1, 2024
22 of 23 checks passed
Copy link

cypress bot commented Nov 1, 2024

Cloud Manager E2E    Run #6772

Run Properties:  status check passed Passed #6772  •  git commit 42f6cc20cd: feat: [M3-8719] - Add a 'Mask Sensitive Data' setting to Cloud Manager (#11143)
Project Cloud Manager E2E
Branch Review develop
Run status status check passed Passed #6772
Run duration 27m 17s
Commit git commit 42f6cc20cd: feat: [M3-8719] - Add a 'Mask Sensitive Data' setting to Cloud Manager (#11143)
Committer Mariah Jacobs
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 3
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 445
View all changes introduced in this branch ↗︎

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants