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: support ability for consumers to add custom props / HTML attributes defined via configuration to components #723

Closed
wants to merge 1 commit into from

Conversation

adamstankiewicz
Copy link
Member

@adamstankiewicz adamstankiewicz commented Aug 9, 2024

Description

Context

Open edX instances (e.g., 2U) rely on third-party tools for logging and analytics (e.g., Datadog, Hotjar). Some of these tools end up ingesting PII into things such as session replays, etc. that should otherwise be masked.

Example vendor-specific approaches to masking UI elements from their ingested data:

  • Datadog. Supports data-dd-privacy="allow" | "mask" | "hidden" | "mask-user-input".
  • Hotjar. Supports data-hj-suppress and data-hj-allow.
  • Fullstory Supports .fs-mask, .fs-unmask, .fs-exclude, etc. class names.
  • Other third-party vendors, with their own specific HTML attributes/values/classes.

Currently, such attributes for Hotjar are hardcoded throughout the platform (search results), despite not all instances of Open edX using Hotjar. While these attributes are fairly benign, the current approach does not support extending the masking of PII to other vendors (e.g., Datadog).

Related, there are other vendor-specific HTML attributes and class names unrelated to PII this approach would also be applicable to. As such, the following implementation is generic to support "custom props" on annotated elements, regardless of whether it's for PII masking or otherwise.

Solution

Introduces a React hook (useComponentPropOverrides) and a Higher-Order-Component (withComponentPropOverrides ) as a mechanism to allow consumers to add one or more custom props (e.g., to mask PII within session replays) to any component that accepts prop spreading (e.g., ...props).

Configuration

May be configured by env.config.js or the MFE runtime configuration with componentPropOverrides (open to other config property names):

{
  componentPropOverrides: {
    targets: {
      username: {
        'data-dd-privacy': 'mask', // Custom `data-*` attribute (e.g., Datadog)
        'data-hj-suppress': '', // Custom `data-*` attribute (e.g., Hotjar)
        className: 'fs-mask', // Custom `className` attribute (e.g., Fullstory)
      },
    },
  },
}

Usage

Consider the following JSX containing PII (i.e., username):

<p>Hi there, {authenticatedUser.username}.</p>

useComponentPropOverrides (hook)

function UsernameWithPropOverrides({ children, ...rest }) {
  const propOverrides = useComponentPropOverrides('username', rest);
  return <span {...propOverrides}>{children}</span>;
}
UsernameWithPropOverrides.propTypes = {
  children: PropTypes.node.isRequired,
};

export default function AuthenticatedPage() {
  const { authenticatedUser, config } = useContext(AppContext);
  return (
      <p>Hi there, <UsernameWithPropOverrides>{authenticatedUser.username}</UsernameWithPropOverrides>.</p>
  );
}

withComponentPropOverrides (HOC)

function Username({ children, ...rest }) {
  return <span {...rest}>{children}</span>;
}
Username.propTypes = {
  children: PropTypes.node.isRequired,
};
const UsernameWithPropOverrides = withComponentPropOverrides('username')(Username);

export default function AuthenticatedPage() {
  const { authenticatedUser, config } = useContext(AppContext);
  return (
      <p>Hi there, <UsernameWithPropOverrides>{authenticatedUser.username}</UsernameWithPropOverrides>.</p>
  );
}

By using useComponentPropOverrides (hook) or withComponentPropOverrides (HOC) to mask the username from third-party tools with the above componentPropOverrides configuration, the resulting HTML would render as the following:

<p>Hi there, <span data-dd-privacy="mask" data-hj-suppress="" class="fs-mask">edx</span>.</p>

It does this by attempting to find a match between the component's specified selector and the componentPropOverrides.targets.* defined via configuration. If the specified selector matches one of the selectors defined via configuration, any configured props and their values be returned.

Special cases

By default, components supporting configurable prop overrides only works with the prop className and any prop prefixed with data- (e.g., data-dd-privacy). Any other configuration prop name is ignored and not applied during rendering.

In certain cases, components may opt-in to supporting overrides of explicitly named prop names (e.g., onClick). By making the special cases like function handlers or style opt-in within the MFE's base code, it provides opportunity to discuss/review which prop overrides are officially supported in any given component beyond the default data-* attributes or className prop.

className

Custom class name(s) will be concatenated with any existing className prop values that might already be passed to the component.

style

Custom style properties will be shallow merged with any existing style properties that might already exist for the component. If the style properties overlap, the custom style's value takes precedence.

Functions (e.g., onClick)

If a custom prop is defined as a function (i.e., supported when customComponentProps is defined via env.config) but the component itself already has an onClick function handler, the component's onClick function will be executed first before calling the custom onClick handler. Both functions otherwise receive the same functions args.

Merge checklist:

  • Consider running your code modifications in the included example app within frontend-platform. This can be done by running npm start and opening http://localhost:8080.
  • Consider testing your code modifications in another local micro-frontend using local aliases configured via the module.config.js file in frontend-build.
  • Verify your commit title/body conforms to the conventional commits format (e.g., fix, feat) and is appropriate for your code change. Consider whether your code is a breaking change, and modify your commit accordingly.

Post merge:

  • After the build finishes for the merged commit, verify the new release has been pushed to NPM.

Copy link

codecov bot commented Aug 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.89%. Comparing base (ba3ff7e) to head (5b53e4c).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #723      +/-   ##
==========================================
+ Coverage   83.41%   83.89%   +0.48%     
==========================================
  Files          40       41       +1     
  Lines        1073     1105      +32     
  Branches      197      210      +13     
==========================================
+ Hits          895      927      +32     
  Misses        166      166              
  Partials       12       12              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@adamstankiewicz adamstankiewicz marked this pull request as draft August 9, 2024 23:19
@adamstankiewicz adamstankiewicz force-pushed the ags/pii-masking branch 12 times, most recently from e5e6bcc to be3418f Compare August 10, 2024 17:21
<h3><code>withPii</code> (HOC)</h3>
<p>
<i>Username:</i>{' '}
<UsernameWithPii2 ref={usernameRef}>
Copy link
Member Author

Choose a reason for hiding this comment

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

[inform] Shows example passing a ref that should be forwarded.

@adamstankiewicz adamstankiewicz marked this pull request as ready for review August 11, 2024 23:30
@adamstankiewicz adamstankiewicz force-pushed the ags/pii-masking branch 3 times, most recently from d3a37c4 to 148934c Compare August 13, 2024 14:36
@adamstankiewicz adamstankiewicz changed the title feat: support ability for consumers to add vendor-specific HTML attributes to suppress PII feat: support ability for consumers to add vendor-specific HTML attributes Aug 14, 2024
@adamstankiewicz adamstankiewicz changed the title feat: support ability for consumers to add vendor-specific HTML attributes feat: support ability for consumers to add custom HTML attributes defined via configuration Aug 14, 2024
@adamstankiewicz adamstankiewicz changed the title feat: support ability for consumers to add custom HTML attributes defined via configuration feat: support ability for consumers to add custom HTML attributes defined via configuration to components Aug 15, 2024
@adamstankiewicz adamstankiewicz force-pushed the ags/pii-masking branch 3 times, most recently from 8bfc4ea to 263841c Compare August 15, 2024 12:50
@adamstankiewicz adamstankiewicz changed the title feat: support ability for consumers to add custom HTML attributes defined via configuration to components feat: support ability for consumers to add custom props / HTML attributes defined via configuration to components Aug 19, 2024
@@ -629,7 +629,7 @@ exports.publish = (memberData, opts, tutorials) => {
generateSourceFiles(sourceFiles, opts.encoding);
}

// if (members.globals.length) { generate('Global', [{kind: 'globalobj'}], globalUrl); }
if (members.globals.length) { generate('Global', [{kind: 'globalobj'}], globalUrl); }
Copy link
Member Author

@adamstankiewicz adamstankiewicz Aug 19, 2024

Choose a reason for hiding this comment

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

[inform] generates the global.html file which is used in links for custom @typedef types used within the React module. without this, the generated JSDoc link results in a 404. that said, the globals still do not show in the sidebar navigation.

@adamstankiewicz adamstankiewicz marked this pull request as draft August 27, 2024 21:23
@adamstankiewicz
Copy link
Member Author

Closed in favor of openedx/frontend-plugin-framework#84

@adamstankiewicz adamstankiewicz deleted the ags/pii-masking branch August 27, 2024 21:24
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.

1 participant