Added author detection for Npm and Nuget Component #51

RushabhBhansali · 2022-01-24T19:13:30Z

Added author Detection for NpmComponents and NugetComponent

Executive Order requirements 4. e.x. and 4.e.vi. require provenance (origination) information of both open-source software and software code and components.

This change enables component detection to extract author information for NPM packages from package.json files and for Nuget Component from .nuspec file.

github-actions · 2022-01-25T00:32:48Z

👋 Hi! It looks like you modified some files in the Detectors folder.
You may need to bump the detector versions if any of the following scenarios apply:

The detector detects more or fewer components than before
The detector generates different parent/child graph relationships than before
The detector generates different devDependencies values than before

If none of the above scenarios apply, feel free to ignore this comment 🙂

cobya · 2022-01-25T20:22:23Z

src/Microsoft.ComponentDetection.Detectors/npm/NpmComponentDetector.cs

+            // for e.g. "author": "John Doe <johnDoe@outlook.com> (https://jd.com)"
+            else
+            {
+                bool isEmailPresent = authorString.Contains("<");


I would be wary of such a simple validation here. Expecting the data to be well formed may not always be accurate and trying to exact match may lead to unexpected errors getting thrown.

introduced JSON and regex-based verification of author.

cobya · 2022-01-25T20:24:34Z

src/Microsoft.ComponentDetection.Detectors/nuget/NuGetComponentDetector.cs


                string name = metadataNode["id"].InnerText;
                string version = metadataNode["version"].InnerText;
+                string[] authors = metadataNode["authors"]?.InnerText.Split(",").Select(email => email.Trim()).ToArray();                


nit: not an email form your .Select

cobya · 2022-01-25T20:25:53Z

src/Microsoft.ComponentDetection.Detectors/npm/NpmComponentDetector.cs

            }

-            var detectedComponent = new DetectedComponent(new NpmComponent(name, version));
+            var npmComponent = new NpmComponent(name, version);


Can you update the class to have a constructor that takes in an optional NpmAuthor parameter? Having to create it and then do .Author on the next line seems unnecessary.

Alternatively, for an optional parameter, something like

var npmComponent = new NpmComponent(name, version) { Author = GetAuthor(authorToken, name, filePath), };

But it's a style choice between those two options

going with Coby's recommendation of introducing optional parameter in constructors, since I see that being used elsewhere in the code base.

cobya · 2022-01-25T20:27:22Z

test/Microsoft.ComponentDetection.Detectors.Tests/NpmTestUtilities.cs

            return ("package.json", packageJsonTemplate, Path.Combine(Path.GetTempPath(), "package.json"));
        }

+        public static (string, string, string) GetPackageJsonNoDependenciesForNameAndVersion(string packageName, string packageVersion)


General testing comment: try to introduce some cases with malformed data that shouldn't exist, but we should cover this scenario without throwing exceptions up the stack

added test for malformed values.

src/Microsoft.ComponentDetection.Detectors/npm/NpmComponentDetector.cs

src/Microsoft.ComponentDetection.Detectors/nuget/NuGetComponentDetector.cs

src/Microsoft.ComponentDetection.Detectors/npm/NpmComponentDetector.cs

Added author detection for npmComponents and NugetComponents

JamieMagee · 2022-01-27T21:36:41Z

src/Microsoft.ComponentDetection.Detectors/npm/NpmComponentDetector.cs

+            } else if (authorMatch.Success)
+            {


Suggested change

} else if (authorMatch.Success)

{

}

else if (authorMatch.Success)

{

JamieMagee · 2022-01-27T21:36:54Z

src/Microsoft.ComponentDetection.Detectors/npm/NpmComponentDetector.cs

+            } else
+            {


Suggested change

} else

{

}

else

{

JamieMagee · 2022-01-27T21:39:06Z

src/Microsoft.ComponentDetection.Detectors/npm/NpmComponentDetector.cs

+                authorEmail = authorMatch.Groups["email"].ToString().Trim();
+            } else
+            {
+                Logger.LogWarning("Unable to parse author:[{authorString}] for package:[{packageName}] found at path:[{filePath}]. This may indicate an invalid npm package author, and author will not be registered.");


This needs $ to make sure the strings are interpolated.

Suggested change

Logger.LogWarning("Unable to parse author:[{authorString}] for package:[{packageName}] found at path:[{filePath}]. This may indicate an invalid npm package author, and author will not be registered.");

Logger.LogWarning($"Unable to parse author:[{authorString}] for package:[{packageName}] found at path:[{filePath}]. This may indicate an invalid npm package author, and author will not be registered.");

JamieMagee · 2022-01-27T21:39:21Z

src/Microsoft.ComponentDetection.Detectors/npm/NpmComponentDetector.cs

+
+            if (string.IsNullOrEmpty(authorName))
+            {
+                Logger.LogWarning("Unable to parse author:[{authorString}] for package:[{packageName}] found at path:[{filePath}]. This may indicate an invalid npm package author, and author will not be registered.");


Same here

Suggested change

Logger.LogWarning("Unable to parse author:[{authorString}] for package:[{packageName}] found at path:[{filePath}]. This may indicate an invalid npm package author, and author will not be registered.");

Logger.LogWarning($"Unable to parse author:[{authorString}] for package:[{packageName}] found at path:[{filePath}]. This may indicate an invalid npm package author, and author will not be registered.");

JamieMagee

Couple of small comments. Fixes provided inline. Approving so as not to block.

Added author detection for NpmComponents

c5c0cf5

RushabhBhansali requested a review from a team as a code owner January 24, 2022 19:13

RushabhBhansali requested a review from grvillic January 24, 2022 19:13

Detecting author from .nuspec file in nuget component Detector.

4c2a8d8

RushabhBhansali changed the title ~~Added author detection for NpmComponents~~ Added author detection for Npm and Nuget Component Jan 25, 2022

cobya reviewed Jan 25, 2022

View reviewed changes

JamieMagee reviewed Jan 25, 2022

View reviewed changes

src/Microsoft.ComponentDetection.Detectors/npm/NpmComponentDetector.cs Outdated Show resolved Hide resolved

JamieMagee reviewed Jan 25, 2022

View reviewed changes

src/Microsoft.ComponentDetection.Detectors/npm/NpmComponentDetector.cs Outdated Show resolved Hide resolved

JamieMagee reviewed Jan 25, 2022

View reviewed changes

src/Microsoft.ComponentDetection.Detectors/npm/NpmComponentDetector.cs Outdated Show resolved Hide resolved

JamieMagee reviewed Jan 25, 2022

View reviewed changes

src/Microsoft.ComponentDetection.Detectors/npm/NpmComponentDetector.cs Outdated Show resolved Hide resolved

JamieMagee reviewed Jan 25, 2022

View reviewed changes

src/Microsoft.ComponentDetection.Detectors/npm/NpmComponentDetector.cs Outdated Show resolved Hide resolved

JamieMagee reviewed Jan 25, 2022

View reviewed changes

src/Microsoft.ComponentDetection.Detectors/nuget/NuGetComponentDetector.cs Show resolved Hide resolved

RushabhBhansali requested review from JamieMagee and cobya January 26, 2022 19:12

JamieMagee reviewed Jan 26, 2022

View reviewed changes

src/Microsoft.ComponentDetection.Detectors/npm/NpmComponentDetector.cs Outdated Show resolved Hide resolved

JamieMagee added detector:npm The npm detector detector:nuget The NuGet detector type:feature Feature (new functionality) labels Jan 26, 2022

RushabhBhansali force-pushed the users/rbhansali/provenance-npmComponent branch from 91fd379 to ab4b156 Compare January 26, 2022 21:57

Detecting author from .nuspec file in nuget component Detector.

8f68abf

Added author detection for npmComponents and NugetComponents

RushabhBhansali force-pushed the users/rbhansali/provenance-npmComponent branch from ab4b156 to 8f68abf Compare January 26, 2022 22:19

RushabhBhansali enabled auto-merge (squash) January 26, 2022 23:47

RushabhBhansali requested a review from JamieMagee January 26, 2022 23:50

Merge branch 'main' into users/rbhansali/provenance-npmComponent

efcb7a7

JamieMagee reviewed Jan 27, 2022

View reviewed changes

JamieMagee approved these changes Jan 27, 2022

View reviewed changes

RushabhBhansali merged commit c05d165 into main Jan 27, 2022

RushabhBhansali deleted the users/rbhansali/provenance-npmComponent branch January 27, 2022 21:42

-            } else
-            {
+            }
+            else
+            {

	Logger.LogWarning("Unable to parse author:[{authorString}] for package:[{packageName}] found at path:[{filePath}]. This may indicate an invalid npm package author, and author will not be registered.");
	Logger.LogWarning($"Unable to parse author:[{authorString}] for package:[{packageName}] found at path:[{filePath}]. This may indicate an invalid npm package author, and author will not be registered.");

Added author detection for Npm and Nuget Component #51

Added author detection for Npm and Nuget Component #51

Uh oh!

Conversation

RushabhBhansali commented Jan 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 25, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RushabhBhansali Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JamieMagee left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

RushabhBhansali commented Jan 24, 2022 •

edited

Loading

RushabhBhansali Jan 26, 2022 •

edited

Loading