✨ [RUMF-998] introduce the initialPrivacyLevel configuration option #1004

BenoitZugmeyer · 2021-08-19T15:47:46Z

Motivation

Allow the user to set a default privacy level for session replay

Changes

This PR is changing a bit the design of the Privacy by Default implementation #914 .

Mainly, it removes the confusing distinct types NodePrivacyLevel and NodePrivacyLevelInternal, to allow simpler inheritance of the privacy level.

It also make the privacy level a require argument for main recording functions, to allow passing an initial privacy level.

Finally, it implements the new configuration option publicly.

Testing

I have gone over the contributing documentation.

The goal is to simplify the Privacy Level typings by removing the distinction between "Internal" and non-"Internal" levels. This is a first step: it introduces a new function that focuses on handling the "MASK_FORMS_ONLY" level. This way, we don't have to worry whether "MASK" or "MASK_FORMS_ONLY" is transmitted to children.

This commit removes the "remap" function and associated. The `getInternalNodePrivacyLevel` becomes `getNodePrivacyLevel` and can return any privacy level. This makes typings a bit less safe, but following commits will improve the situation.

Now that associated functions have been removed, we can have a single type for all privacy levels.

This change is in preparation of the initialPrivacyLevel configuration option. It changes the `getNodePrivacyLevel` paradigm a bit: instead of receiving the direct parent privacy level, it receives a default privacy level to be used if no level was found. Thus, this function now always recurse over the node ancestors. During serialization, where we know the parent privacy level, we can use the `getNodeSelfPrivacyLevel` function to avoid this behavior.

codecov-commenter · 2021-08-19T16:26:27Z

Codecov Report

Merging #1004 (bd54d8b) into main (fdcb5f9) will decrease coverage by 0.12%.
The diff coverage is 87.50%.

@@            Coverage Diff             @@
##             main    #1004      +/-   ##
==========================================
- Coverage   89.04%   88.91%   -0.13%     
==========================================
  Files          86       86              
  Lines        4115     4105      -10     
  Branches      946      945       -1     
==========================================
- Hits         3664     3650      -14     
- Misses        451      455       +4

Impacted Files	Coverage Δ
packages/rum-core/src/boot/rumPublicApi.ts	`97.91% <ø> (ø)`
packages/rum/src/boot/startRecording.ts	`100.00% <ø> (ø)`
packages/rum/src/constants.ts	`100.00% <ø> (ø)`
packages/rum/src/domain/record/record.ts	`59.52% <ø> (ø)`
packages/rum/src/domain/record/types.ts	`100.00% <ø> (ø)`
packages/rum/src/domain/record/observer.ts	`72.22% <59.09%> (-3.15%)`	⬇️
packages/rum/src/domain/record/mutationObserver.ts	`96.29% <91.66%> (-0.68%)`	⬇️
packages/rum/src/domain/record/privacy.ts	`92.25% <95.45%> (-0.06%)`	⬇️
packages/core/src/domain/configuration.ts	`94.28% <100.00%> (+7.61%)`	⬆️
packages/core/src/tools/utils.ts	`87.08% <100.00%> (-0.07%)`	⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fdcb5f9...bd54d8b. Read the comment docs.

ThibautGeriz

I don't think I can either approve or refuse this PR but it looks good.

packages/core/src/domain/configuration.ts

This reduce corner cases a bit by leveraging the type to restrict some API usages.

Same as previous commit, this allows to reduce the scope of some functions by passing explicit subsets of privacy levels

To make sure the initial privacy level is correctly set everywhere, make it required on `getNodePrivacyLevel` and `serializeDocument` (and associated subfunctions).

packages/rum/src/constants.ts

packages/rum/src/domain/record/serialize.ts

* More explicit function naming * Improved typings inference

amortemousque · 2021-08-25T09:40:20Z

For the overall understanding of the recorder, it might be interesting to split the observer.ts file like so for instance:

/initObservers.ts
/observers
  /mutationObserver.ts
  /moveObserver.ts
  /scrollObserver.ts
  ...

Same logic could be applied for serialize.ts:

/serializers
  /serializeDocumentNode.ts
  /serializeElementNode.ts
  /serializeAttribute.ts

...
Of course, it should be done in another PR :)
What do you think?

packages/rum/src/domain/record/privacy.ts

amortemousque · 2021-08-25T10:57:31Z

packages/rum/src/domain/record/privacy.ts

    switch (privAttr) {
      case PRIVACY_ATTR_VALUE_ALLOW:
-        return NodePrivacyLevelInternal.ALLOW
+        return NodePrivacyLevel.ALLOW
      case PRIVACY_ATTR_VALUE_MASK:
-        return NodePrivacyLevelInternal.MASK
+        return NodePrivacyLevel.MASK
      case PRIVACY_ATTR_VALUE_MASK_FORMS_ONLY:
      case PRIVACY_ATTR_VALUE_INPUT_IGNORED: // Deprecated, now aliased
      case PRIVACY_ATTR_VALUE_INPUT_MASKED: // Deprecated, now aliased
-        return NodePrivacyLevelInternal.MASK_FORMS_ONLY
+        return NodePrivacyLevel.MASK_FORMS_ONLY
      case PRIVACY_ATTR_VALUE_HIDDEN:
-        return NodePrivacyLevelInternal.HIDDEN
+        return NodePrivacyLevel.HIDDEN
    }

    // But we also need to support class based privacy tagging for certain frameworks
    if (elNode.classList.contains(PRIVACY_CLASS_ALLOW)) {
-      return NodePrivacyLevelInternal.ALLOW
+      return NodePrivacyLevel.ALLOW
    } else if (elNode.classList.contains(PRIVACY_CLASS_MASK)) {
-      return NodePrivacyLevelInternal.MASK
+      return NodePrivacyLevel.MASK
    } else if (elNode.classList.contains(PRIVACY_CLASS_HIDDEN)) {
-      return NodePrivacyLevelInternal.HIDDEN
+      return NodePrivacyLevel.HIDDEN
    } else if (
      elNode.classList.contains(PRIVACY_CLASS_MASK_FORMS_ONLY) ||
      elNode.classList.contains(PRIVACY_CLASS_INPUT_MASKED) || // Deprecated, now aliased
      elNode.classList.contains(PRIVACY_CLASS_INPUT_IGNORED) // Deprecated, now aliased
    ) {
-      return NodePrivacyLevelInternal.MASK_FORMS_ONLY
+      return NodePrivacyLevel.MASK_FORMS_ONLY
    } else if (shouldIgnoreElement(elNode)) {
      // such as for scripts
-      return NodePrivacyLevelInternal.IGNORE
+      return NodePrivacyLevel.IGNORE
    }


It's the same conditions for HTML attributes and CSS classes privacy. It could be nice not to repeat ourselves.

I agree, and we would have the same priority for both classes and attributes. For example, an element with a hidden class and a mask attribute should be considered as hidden (higher privacy guarantees). I'll change this in a separate PR.

amortemousque · 2021-08-25T11:13:17Z

packages/rum/src/domain/record/observer.ts

@@ -182,7 +194,7 @@ export function initInputObserver(cb: InputCallback): ListenerHandler {

    let inputState: InputState
    if (type === 'radio' || type === 'checkbox') {
-      if (nodePrivacyLevel === NodePrivacyLevel.MASK) {
+      if (shouldMaskNode(target, nodePrivacyLevel)) {


I don't really inderstand why we need shouldMaskNode(), if we already have getNodePrivacyLevel()
There is probably a confusion between the Mask term in shouldMaskNode() and the NodePrivacyLevel.MASK

I'll add a comment about this, but:

shouldMaskNode is a helper mostly aiming to unify mask-forms-only and mask privacy levels. In the mask case, it is trivial: we should mask the element. In the mask-forms-only case, we should mask the element only if it is a "form" element (or the direct parent is a form element for text nodes). Other shouldMaskNode cases are edge cases that should not matter too much (ex: should we mask a node if it is ignored or hidden? it doesn't matter since it won't be serialized, but the case is handled anyway to be exhaustive).

It has been introduced to remove the need to have two different NodePrivacyLevel types (Internal and not Internal), which was a bit obscur for me and made introducing a new type (InitialPrivacyLevel) even more confusing. The NodePrivacyLevel (not Internal) was designed to unify the mask and mask-forms-only into a single mask variant.

I get the improvement that is legitimate. But I still find it a bit confusing especially in the case bellow, where we check the hidden level and then the shouldMaskNode() which also checks the hidden level.

browser-sdk/packages/rum/src/domain/record/privacy.ts

Lines 261 to 264 in 6837279

} else if (nodePrivacyLevel === NodePrivacyLevel.HIDDEN) {

// Should never occur, but just in case, we set to CENSORED_MARK.

textContent = CENSORED_STRING_MARK

} else if (shouldMaskNode(textNode, nodePrivacyLevel)) {

However, I can consider it as good enough ;)

Also, leverage the typegard and use `node` as an element directly

BenoitZugmeyer · 2021-08-27T08:05:27Z

For the overall understanding of the recorder, it might be interesting to split the observer.ts file like so for instance:
/initObservers.ts
/observers
  /mutationObserver.ts
  /moveObserver.ts
  /scrollObserver.ts
  ...
Same logic could be applied for serialize.ts:
/serializers
  /serializeDocumentNode.ts
  /serializeElementNode.ts
  /serializeAttribute.ts
...
Of course, it should be done in another PR :)
What do you think?

I agree, we can split things up in another PR.

amortemousque · 2021-08-27T08:09:59Z

Small code improvement, instead of:

browser-sdk/packages/rum/src/domain/record/privacy.ts

Lines 258 to 263 in 6837279

    
           if (isScript) { 
        
             // For perf reasons, we don't record script (heuristic) 
        
             textContent = CENSORED_STRING_MARK 
        
           } else if (nodePrivacyLevel === NodePrivacyLevel.HIDDEN) { 
        
             // Should never occur, but just in case, we set to CENSORED_MARK. 
        
             textContent = CENSORED_STRING_MARK

if (
    isScript || // For perf reasons, we don't record script (heuristic)
    nodePrivacyLevel === NodePrivacyLevel.HIDDEN // Should never occur, but just in case, we set to CENSORED_MARK.
) {
  textContent = CENSORED_STRING_MARK
}

jagracey · 2021-09-07T11:22:39Z

For the overall understanding of the recorder, it might be interesting to split the observer.ts file like so for instance:
/initObservers.ts
/observers
  /mutationObserver.ts
  /moveObserver.ts
  /scrollObserver.ts
  ...
Same logic could be applied for serialize.ts:
/serializers
  /serializeDocumentNode.ts
  /serializeElementNode.ts
  /serializeAttribute.ts
...
Of course, it should be done in another PR :)
What do you think?
I agree, we can split things up in another PR.

I'd agree with the first case, observer, particularly mutationObserver, however the serialisers (document, element, attribute) to me represent code that should be kept together.

BenoitZugmeyer added 4 commits August 19, 2021 17:01

[RUMF-998] remove remap function

e4539c5

This commit removes the "remap" function and associated. The `getInternalNodePrivacyLevel` becomes `getNodePrivacyLevel` and can return any privacy level. This makes typings a bit less safe, but following commits will improve the situation.

[RUMF-998] rename NodePrivacyLevelInternal to NodePrivacyLevel

cd0d20a

Now that associated functions have been removed, we can have a single type for all privacy levels.

BenoitZugmeyer requested a review from a team as a code owner August 19, 2021 15:47

ThibautGeriz reviewed Aug 20, 2021

View reviewed changes

packages/core/src/domain/configuration.ts Show resolved Hide resolved

packages/core/src/domain/configuration.ts Outdated Show resolved Hide resolved

BenoitZugmeyer added 7 commits August 20, 2021 15:36

[RUMF-998] remove the "NOT_SET" privacy level variant

0a92809

This reduce corner cases a bit by leveraging the type to restrict some API usages.

[RUMF-998] ensure only a subset of privacy levels can be inherited

e9724b7

Same as previous commit, this allows to reduce the scope of some functions by passing explicit subsets of privacy levels

[RUMF-998] make initial privacy level arguments required

540324a

To make sure the initial privacy level is correctly set everywhere, make it required on `getNodePrivacyLevel` and `serializeDocument` (and associated subfunctions).

[RUMF-998] rename NodePrivacyLevel values to match public configuration

e557515

✨ [RUMF-998] introduce the initialPrivacyLevel configuration option

a541c0a

[RUMF-998] adjust some comments and typos

279e300

👌 [RUMF-998] add a few tests to ensure the setting is correctly applied

7c0a10a

BenoitZugmeyer force-pushed the benoit/implement-initial-privacy-level-option branch from ed2e242 to 7c0a10a Compare August 20, 2021 15:13

webNeat reviewed Aug 23, 2021

View reviewed changes

packages/rum/src/constants.ts Show resolved Hide resolved

webNeat reviewed Aug 23, 2021

View reviewed changes

packages/rum/src/domain/record/serialize.ts Show resolved Hide resolved

BenoitZugmeyer added 3 commits August 24, 2021 15:13

👌 improve configuration value check

c4f6ea9

* More explicit function naming * Improved typings inference

👌 use the InitialPrivacyLevel to declare the NodePrivacyLevel constants

8a2a611

[RUMF-998] export the InitialPrivacyLevel constants

5c5e764

webNeat approved these changes Aug 24, 2021

View reviewed changes

🚩 [RUMF-998] put the option behind a feature flag

bd54d8b

amortemousque reviewed Aug 25, 2021

View reviewed changes

BenoitZugmeyer added 4 commits August 26, 2021 17:37

👌 rename derivePrivacyLevelGivenParent to reducePrivacyLevel

664f4ac

👌 early return in getNodePrivacyLevel

07b8ebd

Also, leverage the typegard and use `node` as an element directly

📝👌 adjust comments and doc

5659520

Merge branch 'main' into benoit/implement-initial-privacy-level-option

6837279

BenoitZugmeyer merged commit fe1f62b into main Aug 27, 2021

BenoitZugmeyer deleted the benoit/implement-initial-privacy-level-option branch August 27, 2021 08:05

BenoitZugmeyer mentioned this pull request Aug 27, 2021

[REPLAY-336] Rescope privacy init JS config #1003

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ [RUMF-998] introduce the initialPrivacyLevel configuration option #1004

✨ [RUMF-998] introduce the initialPrivacyLevel configuration option #1004

BenoitZugmeyer commented Aug 19, 2021

codecov-commenter commented Aug 19, 2021 •

edited

Loading

ThibautGeriz left a comment

amortemousque commented Aug 25, 2021 •

edited

Loading

amortemousque Aug 25, 2021

BenoitZugmeyer Aug 26, 2021

amortemousque Aug 25, 2021

BenoitZugmeyer Aug 26, 2021

amortemousque Aug 27, 2021

BenoitZugmeyer commented Aug 27, 2021

amortemousque commented Aug 27, 2021

jagracey commented Sep 7, 2021

	} else if (nodePrivacyLevel === NodePrivacyLevel.HIDDEN) {
	// Should never occur, but just in case, we set to CENSORED_MARK.
	textContent = CENSORED_STRING_MARK
	} else if (shouldMaskNode(textNode, nodePrivacyLevel)) {

✨ [RUMF-998] introduce the initialPrivacyLevel configuration option #1004

✨ [RUMF-998] introduce the initialPrivacyLevel configuration option #1004

Conversation

BenoitZugmeyer commented Aug 19, 2021

Motivation

Changes

Testing

codecov-commenter commented Aug 19, 2021 • edited Loading

Codecov Report

ThibautGeriz left a comment

Choose a reason for hiding this comment

amortemousque commented Aug 25, 2021 • edited Loading

amortemousque Aug 25, 2021

Choose a reason for hiding this comment

BenoitZugmeyer Aug 26, 2021

Choose a reason for hiding this comment

amortemousque Aug 25, 2021

Choose a reason for hiding this comment

BenoitZugmeyer Aug 26, 2021

Choose a reason for hiding this comment

amortemousque Aug 27, 2021

Choose a reason for hiding this comment

BenoitZugmeyer commented Aug 27, 2021

amortemousque commented Aug 27, 2021

jagracey commented Sep 7, 2021

codecov-commenter commented Aug 19, 2021 •

edited

Loading

amortemousque commented Aug 25, 2021 •

edited

Loading