DYN-6146 Removing PII data from a JSON workspace #14471

jesusalvino · 2023-10-06T04:54:36Z

Purpose

Removing the PII data from a JSON workspace object to implement the task https://jira.autodesk.com/browse/DYN-6146

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.
This PR modifies some build requirements and the readme is updated
This PR contains no files larger than 50 MB

Reviewers

@QilongTang
@reddyashish

FYIs

@RobertGlobant20
@Enzo707

aparajit-pratap · 2023-10-13T17:04:48Z

src/DynamoUtilities/PIIDetector.cs

+        static string creditCardPattern = @"(\d{4}[-, ]\d{4})";
+        static string ssnPattern = @"\d{3}[- ]\d{2}[- ]\d{4}";
+        static string ipPattern = @"((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)";
+        static string datePattern = @"(3[01]|[12][0-9]|0?[1-9])(\/|-)(1[0-2]|0?[1-9])\2([0-9]{2})?[0-9]{2}";


Does this account for different data formats like DDMMYY, DDMMYYYY, MMDDYY, etc.

Does this account for different data formats like DDMMYY, DDMMYYYY, MMDDYY, etc.

Done, I have added a Unit Test too for validate more cases.

aparajit-pratap

Isn't this amounting to modifying the original graph authored by a user? What if users deliberately want to insert such data in their graphs for certain specific workflows, without which their graphs might not be useful?
In other words, do we need to care about removing PII data that users are deliberately of their own volition introducing in Dynamo?

QilongTang · 2023-10-13T19:03:53Z

Isn't this amounting to modifying the original graph authored by a user? What if users deliberately want to insert such data in their graphs for certain specific workflows, without which their graphs might not be useful? In other words, do we need to care about removing PII data that users are deliberately of their own volition introducing in Dynamo?

Hi @aparajit-pratap The change to save function itself is unintended, we intend to instrument graphs without PII as the purpose. @jesusalvino I would suggest not injecting the function as part of the save process for clarity.

mjkkirschner · 2023-10-17T17:08:38Z

src/DynamoUtilities/PIIDetector.cs

+            return new Tuple<string, JObject>(uuid, jObjectResult);
+        }
+
+        static string emailPattern = @"\w+([-+.]\w+)*@\w+([-.]\w+)*\.\w+([-.]\w+)*";


is this best practice? where are these regexes pulled from? I don't know how others feel, but this seems very hard to understand, reason about, or maintain - especially because there are no tests in this PR.

of note
https://docs.aws.amazon.com/comprehend/latest/dg/how-pii.html
https://microsoft.github.io/presidio/analyzer/

of note https://docs.aws.amazon.com/comprehend/latest/dg/how-pii.html https://microsoft.github.io/presidio/analyzer/

I have added an Unit Test for it so We can discuss in detail the used regexes and their scope / granularity, I have already used them in the past for others projects (As we know this feature is cross project/company).

About your references I have already considered similar options in the Spike from this task : https://jira.autodesk.com/browse/DYN-5964 . I considered the PII Helper class as the main option but its opened to be extended or changed as the Team decide.

jesusalvino · 2023-10-19T14:47:29Z

Isn't this amounting to modifying the original graph authored by a user? What if users deliberately want to insert such data in their graphs for certain specific workflows, without which their graphs might not be useful? In other words, do we need to care about removing PII data that users are deliberately of their own volition introducing in Dynamo?

Hi @aparajit-pratap The change to save function itself is unintended, we intend to instrument graphs without PII as the purpose. @jesusalvino I would suggest not injecting the function as part of the save process for clarity.

I have removed the functionality from the Save method and create a Unit Test instead of.

mjkkirschner · 2023-10-24T16:55:29Z

src/DynamoUtilities/PIIDetector.cs

+    /// <summary>
+    /// Helper Class for removing PII Data from a JSON workspace
+    /// </summary>
+    public static class PIIDetector


keep this stuff internal, this should not be an API IMO.

mjkkirschner · 2023-10-24T16:55:59Z

src/DynamoUtilities/PIIDetector.cs

+            return property.Value;
+        }
+
+        public static bool ContainsEmail(string value) { return new Regex(emailPattern).Match(value).Success; }


be conservative as possible with access modifiers, none of this should be something we are providing as APIs - right?

mjkkirschner · 2023-10-31T14:11:18Z

src/DynamoUtilities/PIIDetector.cs

+        /// </summary>
+        /// <param name="jsonObject"></param>
+        /// <returns></returns>
+        public static JObject RemovePIIData(JObject jsonObject)


the code in here seems kind of fragile to changes - it uses a bunch of hardcoded string names which could easily break at runtime.

I think for the null checking if possible you should move to using the null coalescing operator for ease of readability:
https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/null-coalescing-operator

or for properties

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/member-access-operators#null-conditional-operators--and-

In addition I don't see this function used, who will use it? Will it be called often?

I would add a try catch around this entire call internally with a catch that reports the error, or return a bool if the removal fails so downstream consumers can decide if they still wish to use the possibly tainted file.

I would probably also take all the strings out and make them consts - ideally we would use NameOf but I understand you want this in a project that should probably not reference the types you would need to do that. I think?

@mjkkirschner I have applied your suggestions, the function will be used to get a serialized workspace without PII data (https://jira.autodesk.com/browse/DYN-5964), based on a well formatted and confident json object, it will be called when an user close a workspace / graph (potentially) after checking if it's valid for send to Forge. Yes, since the project doesn't have reference to the types, the comparisons are performed to explicit values.

mjkkirschner · 2023-11-02T15:04:28Z

@jesusalvino it looks good, but the build checks are failing, any idea if thats a fluke or something real?

QilongTang · 2023-11-02T15:26:44Z

@jesusalvino Please double check this branch for building, I always trust the checks. It could be that updating this branch with master branch caused side effects or errors.

QilongTang

LGTM. Thanks @mjkkirschner and @aparajit-pratap for the review. One thing I was not thrilled about was the hardcoded Workspace related property names, e.g. const string Nodes = "Nodes";. I later realized that these need to be hard coded because the code is in Utility class with a recursive implementation, I do not see any work around yet other than us moving these code back so we can use nameof. But I think for now, since these properties are always going to be valid and we have been stable about our file format, I am OK with that.

jesusalvino · 2023-11-02T20:27:10Z

@jesusalvino it looks good, but the build checks are failing, any idea if thats a fluke or something real?

I missed commit the unit test / merged with the latest master, all should be clean now

QilongTang · 2023-11-02T20:28:17Z

@jesusalvino it looks good, but the build checks are failing, any idea if thats a fluke or something real?

I missed commit the unit test / merged with the latest master, all should be clean now

sure, waiting for PR checks to pass

QilongTang · 2023-11-06T15:57:39Z

The regression should be fixed by #14566 already. Merging

Removing PII data from a JSON workspace

c24745f

QilongTang requested a review from varvaratou October 11, 2023 03:53

QilongTang added this to the 3.0 milestone Oct 11, 2023

QilongTang requested a review from reddyashish October 11, 2023 03:53

aparajit-pratap reviewed Oct 13, 2023

View reviewed changes

mjkkirschner reviewed Oct 17, 2023

View reviewed changes

avidit changed the title ~~Removing PII data from a JSON workspace~~ DYN-6146 Removing PII data from a JSON workspace Oct 17, 2023

Refactoring and Adding a Unit Test

dfbeeb3

jesusalvino closed this Oct 19, 2023

jesusalvino reopened this Oct 19, 2023

QilongTang requested a review from mjkkirschner October 20, 2023 15:52

mjkkirschner reviewed Oct 24, 2023

View reviewed changes

Restricting the access level

033ffc4

mjkkirschner reviewed Oct 31, 2023

View reviewed changes

Jesus Alfredo Alviño and others added 2 commits November 2, 2023 07:57

PII Detector refactored

8bb6593

Merge branch 'master' into DYN-6146-RemovePIIData

26e0585

mjkkirschner approved these changes Nov 2, 2023

View reviewed changes

Updating the Unit Test

73d5917

QilongTang approved these changes Nov 2, 2023

View reviewed changes

QilongTang merged commit 4e0cf2e into DynamoDS:master Nov 6, 2023
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DYN-6146 Removing PII data from a JSON workspace #14471

DYN-6146 Removing PII data from a JSON workspace #14471

jesusalvino commented Oct 6, 2023

aparajit-pratap Oct 13, 2023

jesusalvino Oct 19, 2023

aparajit-pratap left a comment

QilongTang commented Oct 13, 2023 •

edited

Loading

mjkkirschner Oct 17, 2023 •

edited

Loading

mjkkirschner Oct 17, 2023

jesusalvino Oct 19, 2023

jesusalvino commented Oct 19, 2023

mjkkirschner Oct 24, 2023

jesusalvino Oct 25, 2023

mjkkirschner Oct 24, 2023

jesusalvino Oct 25, 2023

mjkkirschner Oct 31, 2023

jesusalvino Nov 2, 2023

mjkkirschner commented Nov 2, 2023

QilongTang commented Nov 2, 2023

QilongTang left a comment

jesusalvino commented Nov 2, 2023

QilongTang commented Nov 2, 2023

QilongTang commented Nov 6, 2023

DYN-6146 Removing PII data from a JSON workspace #14471

DYN-6146 Removing PII data from a JSON workspace #14471

Conversation

jesusalvino commented Oct 6, 2023

Purpose

Declarations

Reviewers

FYIs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aparajit-pratap left a comment

Choose a reason for hiding this comment

QilongTang commented Oct 13, 2023 • edited Loading

mjkkirschner Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesusalvino commented Oct 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner commented Nov 2, 2023

QilongTang commented Nov 2, 2023

QilongTang left a comment

Choose a reason for hiding this comment

jesusalvino commented Nov 2, 2023

QilongTang commented Nov 2, 2023

QilongTang commented Nov 6, 2023

QilongTang commented Oct 13, 2023 •

edited

Loading

mjkkirschner Oct 17, 2023 •

edited

Loading