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: allow stricter masking #35

Merged
merged 1 commit into from
Aug 12, 2024
Merged

Conversation

deborahgu
Copy link
Member

For some applications, we want to mask all text and images, because non-input text sometimes contains PII. Allows setting this by environment variable.

For some applications, we want to mask all text and images, because
non-input text sometimes contains PII. Allows setting this by
environment variable.
@jsnwesson
Copy link
Member

Oh neat! To clarify, all this does is include an environment variable, but each MFE would have to include their own declaration in edx-internal, right? Could you link here the options that Datadog allows for masking?

@jsnwesson
Copy link
Member

Or maybe include it in frontend-logging's README?

Copy link
Member

@adamstankiewicz adamstankiewicz left a comment

Choose a reason for hiding this comment

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

I think this approach makes sense to extend DatadogLoggingService to allow consumers to override the base defaultPrivacyLevel. However, I do wonder if opting to mask all text/copy in a given MFE removes some of the utility/benefits of session replays in terms of debugging and product analytics.

It'd be great if we could mask specific elements (like the username in the header) known to contain PII (e.g., like the username in the MFE's header). See this Datadog documentation for example of using data-dd-privacy attributes and/or dd-privacy-* CSS class names. This is similar to how various PII elements are masked for other tools like Hotjar throughout Open edX currently, e.g., via a data-hj-suppress HTML attribute (examples). In fact, the frontend-app-learner-dashboard MFE currently masks the username from Hotjar via the data-hj-suppress attribute today (source).

That said, including tool/vendor specific HTML attributes or class names like this is not really open-source friendly for Open edX since it introduces attributes that are specific to our 2U instance (i.e., not everyone uses Hotjar/Datadog like we do at 2U).

tl;dr; I think supporting the ability for MFEs to override defaultPrivacyLevel here makes sense, but it might also be worthwhile to explore how we could extend the Open edX MFE framework to support vendor-specific attributes like data-hj-suppress and data-dd-privacy in an open-source friendly way so we don't need to mask everything within an MFE just to remove PII from a fairly narrow subset of UI elements, which removes some of the utility of having the session replays from a debugging / product analytics perspective.

@adamstankiewicz
Copy link
Member

Or maybe include it in frontend-logging's README?

Good suggestion, @jsnwesson... +1 to updating the README to include how DatadogLoggingService may be extended/configured, though these updates could probably be split into a more documentation-focused PR as a follow-up.

@deborahgu deborahgu merged commit 3f84826 into master Aug 12, 2024
5 checks passed
@adamstankiewicz adamstankiewicz deleted the dkaplan1/allow-stricter-masking branch August 12, 2024 16:46
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.

3 participants