From f85b6c43636709ea5f8212370bfbf936f3c17978 Mon Sep 17 00:00:00 2001 From: James Oakley Date: Tue, 27 Feb 2024 13:39:53 -0500 Subject: [PATCH] Support development dependencies for the Gradle detector (#878) * Support development dependencies for the Gradle detector Lack of development dependency detection for Gradle is a problem for Android teams, especially in the context of Component Governance alerts. Unfortunately Gradle doesn't provide enough information to definitively identify dev dependencies in all cases, so manual configuration is required. This change adds dev dependency classification through two mechanisms 1. `buildscript-gradle.lockfile` and `settings-gradle.lockfile` contain only build-system dependencies, so always classify these as development dependencies. 2. Processing based on two new environment variables: `GRADLE_PROD_CONFIGURATIONS_REGEX` and `GRADLE_DEV_CONFIGURATIONS_REGEX`. Gradle lockfiles indicate which Gradle configuration(s) each dependency is required by. `GRADLE_PROD_CONFIGURATIONS_REGEX` allows specifying production configurations explicitly. All other configurations are considered development. Alternately, dev configurations may be specified in `GRADLE_DEV_CONFIGURATIONS_REGEX` and all others are considered production. * Changes based on meeting prior to the holidays * fluent assertions * Visual studio recommendations * More fluent assertsions * Fix test to be cross-platform * Fix the cross-platform test fix * Fix code coverage by removing dead code check * Address code review comments --- docs/environment-variables.md | 17 ++ docs/feature-overview.md | 2 +- .../EnvironmentVariableService.cs | 4 + .../IEnvironmentVariableService.cs | 10 ++ .../gradle/GradleComponentDetector.cs | 57 +++++- .../EnvironmentVariableServiceTests.cs | 43 ++++- .../GradleComponentDetectorTests.cs | 167 +++++++++++++++++- 7 files changed, 295 insertions(+), 5 deletions(-) diff --git a/docs/environment-variables.md b/docs/environment-variables.md index 74f600e21..4c6ccc584 100644 --- a/docs/environment-variables.md +++ b/docs/environment-variables.md @@ -18,4 +18,21 @@ When set to any value, enables detector experiments, a feature to compare the re same ecosystem. The available experiments are found in the [`Experiments\Config`](../src/Microsoft.ComponentDetection.Orchestrator/Experiments/Configs) folder. +## `CD_GRADLE_DEV_LOCKFILES` + +Enables dev-dependency categorization for the Gradle +detector. Comma-separated list of Gradle lockfiles which contain only +development dependencies. Dependencies connected to Gradle +configurations matching the given regex are considered development +dependencies. If a lockfile will contain a mix of development and +production dependencies, see `CD_GRADLE_DEV_CONFIGURATIONS` below. + +## `CD_GRADLE_DEV_CONFIGURATIONS` + +Enables dev-dependency categorization for the Gradle +detector. Comma-separated list of Gradle configurations which refer to development dependencies. +Dependencies connected to Gradle configurations matching +the given configurations are considered development dependencies. +If an entire lockfile will contain only dev dependencies, see `CD_GRADLE_DEV_LOCKFILES` above. + [1]: https://go.dev/ref/mod#go-mod-graph diff --git a/docs/feature-overview.md b/docs/feature-overview.md index a56b6c51b..aabee8bf1 100644 --- a/docs/feature-overview.md +++ b/docs/feature-overview.md @@ -5,7 +5,7 @@ | CocoaPods | | - | ❌ | - | | Conda (Python) | | - | ❌ | ✔ | | Linux (Debian, Alpine, Rhel, Centos, Fedora, Ubuntu)| | - | - | - | - | -| Gradle | | | ❌ | ❌ | +| Gradle | | | ✔ (requires env var configuration for full effect) | ❌ | | Go | Fallback
| | ❌ | ✔ (root idenditication only for fallback) | | Maven | | | ✔ (test dependency scope) | ✔ | | NPM | | - | ✔ (dev-dependencies in package.json, dev flag in package-lock.json) | ✔ | diff --git a/src/Microsoft.ComponentDetection.Common/EnvironmentVariableService.cs b/src/Microsoft.ComponentDetection.Common/EnvironmentVariableService.cs index 355779149..7ce1bed86 100644 --- a/src/Microsoft.ComponentDetection.Common/EnvironmentVariableService.cs +++ b/src/Microsoft.ComponentDetection.Common/EnvironmentVariableService.cs @@ -1,6 +1,7 @@ namespace Microsoft.ComponentDetection.Common; using System; +using System.Collections.Generic; using System.Linq; using Microsoft.ComponentDetection.Contracts; @@ -23,6 +24,9 @@ public string GetEnvironmentVariable(string name) return caseInsensitiveName != null ? Environment.GetEnvironmentVariable(caseInsensitiveName) : null; } + public List GetListEnvironmentVariable(string name, string delimiter) + => (this.GetEnvironmentVariable(name) ?? string.Empty).Split(delimiter, StringSplitOptions.RemoveEmptyEntries).ToList(); + public bool IsEnvironmentVariableValueTrue(string name) { _ = bool.TryParse(this.GetEnvironmentVariable(name), out var result); diff --git a/src/Microsoft.ComponentDetection.Contracts/IEnvironmentVariableService.cs b/src/Microsoft.ComponentDetection.Contracts/IEnvironmentVariableService.cs index 055fc5831..36918258d 100644 --- a/src/Microsoft.ComponentDetection.Contracts/IEnvironmentVariableService.cs +++ b/src/Microsoft.ComponentDetection.Contracts/IEnvironmentVariableService.cs @@ -1,5 +1,7 @@ namespace Microsoft.ComponentDetection.Contracts; +using System.Collections.Generic; + /// /// Wraps some common environment variable operations for easier testability. /// @@ -19,6 +21,14 @@ public interface IEnvironmentVariableService /// Returns a string of the environment variable value. string GetEnvironmentVariable(string name); + /// + /// Returns the value of an environment variable which is formatted as a delimited list. + /// + /// Name of the environment variable. + /// Delimiter separating the items in the list. + /// Returns she parsed environment variable value. + List GetListEnvironmentVariable(string name, string delimiter = ","); + /// /// Returns true if the environment variable value is true. /// diff --git a/src/Microsoft.ComponentDetection.Detectors/gradle/GradleComponentDetector.cs b/src/Microsoft.ComponentDetection.Detectors/gradle/GradleComponentDetector.cs index 397207a39..a3478b1dd 100644 --- a/src/Microsoft.ComponentDetection.Detectors/gradle/GradleComponentDetector.cs +++ b/src/Microsoft.ComponentDetection.Detectors/gradle/GradleComponentDetector.cs @@ -3,6 +3,7 @@ namespace Microsoft.ComponentDetection.Detectors.Gradle; using System; using System.Collections.Generic; using System.IO; +using System.Linq; using System.Text.RegularExpressions; using System.Threading.Tasks; using Microsoft.ComponentDetection.Contracts; @@ -12,16 +13,27 @@ namespace Microsoft.ComponentDetection.Detectors.Gradle; public class GradleComponentDetector : FileComponentDetector, IComponentDetector { + private const string DevConfigurationsEnvVar = "CD_GRADLE_DEV_CONFIGURATIONS"; + private const string DevLockfilesEnvVar = "CD_GRADLE_DEV_LOCKFILES"; private static readonly Regex StartsWithLetterRegex = new Regex("^[A-Za-z]", RegexOptions.Compiled); + private readonly List devConfigurations; + private readonly List devLockfiles; + public GradleComponentDetector( IComponentStreamEnumerableFactory componentStreamEnumerableFactory, IObservableDirectoryWalkerFactory walkerFactory, + IEnvironmentVariableService envVarService, ILogger logger) { this.ComponentStreamEnumerableFactory = componentStreamEnumerableFactory; this.Scanner = walkerFactory; this.Logger = logger; + + this.devLockfiles = envVarService.GetListEnvironmentVariable(DevLockfilesEnvVar) ?? new List(); + this.devConfigurations = envVarService.GetListEnvironmentVariable(DevConfigurationsEnvVar) ?? new List(); + this.Logger.LogDebug("Gradle dev-only lockfiles {Lockfiles}", string.Join(", ", this.devLockfiles)); + this.Logger.LogDebug("Gradle dev-only configurations {Configurations}", string.Join(", ", this.devConfigurations)); } public override string Id { get; } = "Gradle"; @@ -32,7 +44,7 @@ public GradleComponentDetector( public override IEnumerable SupportedComponentTypes { get; } = new[] { ComponentType.Maven }; - public override int Version { get; } = 2; + public override int Version { get; } = 3; protected override Task OnFileFoundAsync(ProcessRequest processRequest, IDictionary detectorArgs) { @@ -68,7 +80,8 @@ private void ParseLockfile(ISingleFileComponentRecorder singleFileComponentRecor if (line.Split(":").Length == 3) { var detectedMavenComponent = new DetectedComponent(this.CreateMavenComponentFromFileLine(line)); - singleFileComponentRecorder.RegisterUsage(detectedMavenComponent); + var devDependency = this.IsDevDependencyByLockfile(file) || this.IsDevDependencyByConfigurations(line); + singleFileComponentRecorder.RegisterUsage(detectedMavenComponent, isDevelopmentDependency: devDependency); } } } @@ -87,4 +100,44 @@ private MavenComponent CreateMavenComponentFromFileLine(string line) } private bool StartsWithLetter(string input) => StartsWithLetterRegex.IsMatch(input); + + private bool IsDevDependencyByConfigurations(string line) + { + var equalsSeparatorIndex = line.IndexOf('='); + if (equalsSeparatorIndex == -1) + { + // We can't parse out the configuration. Maybe the project is using the one-lockfile-per-configuration format but + // this is deprecated in Gradle so we don't support it here, projects should upgrade to one-lockfile-per-project. + return false; + } + + var configurations = line[(equalsSeparatorIndex + 1)..].Split(","); + return configurations.All(this.IsDevDependencyByConfigurationName); + } + + private bool IsDevDependencyByConfigurationName(string configurationName) + { + return this.devConfigurations.Contains(configurationName); + } + + private bool IsDevDependencyByLockfile(IComponentStream file) + { + // Buildscript and Settings lockfiles are always development dependencies + var lockfileName = Path.GetFileName(file.Location); + var lockfileRelativePath = Path.GetRelativePath(this.CurrentScanRequest.SourceDirectory.FullName, file.Location); + var dev = lockfileName == "buildscript-gradle.lockfile" + || lockfileName == "settings-gradle.lockfile" + || this.devLockfiles.Contains(lockfileRelativePath); + + if (dev) + { + this.Logger.LogDebug("Gradle lockfile {Location} contains dev dependencies only", lockfileRelativePath); + } + else + { + this.Logger.LogDebug("Gradle lockfile {Location} contains at least some production dependencies", lockfileRelativePath); + } + + return dev; + } } diff --git a/test/Microsoft.ComponentDetection.Common.Tests/EnvironmentVariableServiceTests.cs b/test/Microsoft.ComponentDetection.Common.Tests/EnvironmentVariableServiceTests.cs index b9be1482a..ef7ad8ba9 100644 --- a/test/Microsoft.ComponentDetection.Common.Tests/EnvironmentVariableServiceTests.cs +++ b/test/Microsoft.ComponentDetection.Common.Tests/EnvironmentVariableServiceTests.cs @@ -1,4 +1,4 @@ -namespace Microsoft.ComponentDetection.Common.Tests; +namespace Microsoft.ComponentDetection.Common.Tests; using System; using FluentAssertions; @@ -93,4 +93,45 @@ public void IsEnvironmentVariableValueTrue_returnsFalseForInvalidAndNull() result2.Should().BeFalse(); Environment.SetEnvironmentVariable(envVariableKey1, null); } + + [TestMethod] + public void GetListEnvironmentVariable_returnEmptyIfVariableDoesNotExist() + { + this.testSubject.GetListEnvironmentVariable("NonExistentVar", ",").Should().BeEmpty(); + } + + [TestMethod] + public void GetListEnvironmentVariable_emptyListIfEmptyVar() + { + var key = "foo"; + Environment.SetEnvironmentVariable(key, string.Empty); + var result = this.testSubject.GetListEnvironmentVariable(key, ","); + result.Should().NotBeNull(); + result.Should().BeEmpty(); + Environment.SetEnvironmentVariable(key, null); + } + + [TestMethod] + public void GetListEnvironmentVariable_singleItem() + { + var key = "foo"; + Environment.SetEnvironmentVariable(key, "bar"); + var result = this.testSubject.GetListEnvironmentVariable(key, ","); + result.Should().ContainSingle(); + result.Should().Contain("bar"); + Environment.SetEnvironmentVariable(key, null); + } + + [TestMethod] + public void GetListEnvironmentVariable_multipleItems() + { + var key = "foo"; + Environment.SetEnvironmentVariable(key, "bar,baz,qux"); + var result = this.testSubject.GetListEnvironmentVariable(key, ","); + result.Should().HaveCount(3); + result.Should().Contain("bar"); + result.Should().Contain("baz"); + result.Should().Contain("qux"); + Environment.SetEnvironmentVariable(key, null); + } } diff --git a/test/Microsoft.ComponentDetection.Detectors.Tests/GradleComponentDetectorTests.cs b/test/Microsoft.ComponentDetection.Detectors.Tests/GradleComponentDetectorTests.cs index 7ab6fb7f8..dbe7b1752 100644 --- a/test/Microsoft.ComponentDetection.Detectors.Tests/GradleComponentDetectorTests.cs +++ b/test/Microsoft.ComponentDetection.Detectors.Tests/GradleComponentDetectorTests.cs @@ -1,6 +1,8 @@ -namespace Microsoft.ComponentDetection.Detectors.Tests; +namespace Microsoft.ComponentDetection.Detectors.Tests; +using System; using System.Collections.Generic; +using System.IO; using System.Linq; using System.Threading.Tasks; using FluentAssertions; @@ -10,12 +12,21 @@ using Microsoft.ComponentDetection.Detectors.Tests.Utilities; using Microsoft.ComponentDetection.TestsUtilities; using Microsoft.VisualStudio.TestTools.UnitTesting; +using Moq; [TestClass] [TestCategory("Governance/All")] [TestCategory("Governance/ComponentDetection")] public class GradleComponentDetectorTests : BaseDetectorTest { + private readonly Mock envVarService; + + public GradleComponentDetectorTests() + { + this.envVarService = new Mock(); + this.DetectorTestUtility.AddServiceMock(this.envVarService); + } + [TestMethod] public async Task TestGradleDetectorWithNoFiles_ReturnsSuccessfullyAsync() { @@ -216,4 +227,158 @@ four score and seven bugs ago component.Should().NotBeNull(); } } + + [TestMethod] + public async Task TestGradleDetector_DevDependenciesByLockfileNameAsync() + { + var regularLockfile = + @"org.springframework:spring-beans:5.0.5.RELEASE +org.springframework:spring-core:5.0.5.RELEASE"; + + var devLockfile1 = @"org.hamcrest:hamcrest-core:2.2 +org.springframework:spring-core:5.0.5.RELEASE"; + + var devLockfile2 = @"org.jacoco:org.jacoco.agent:0.8.8"; + + var (scanResult, componentRecorder) = await this.DetectorTestUtility + .WithFile("settings-gradle.lockfile", devLockfile1) + .WithFile("buildscript-gradle.lockfile", devLockfile2) + .WithFile("gradle.lockfile", regularLockfile) + .ExecuteDetectorAsync(); + + scanResult.ResultCode.Should().Be(ProcessingResultCode.Success); + + var discoveredComponents = componentRecorder.GetDetectedComponents().Select(c => (MavenComponent)c.Component).OrderBy(c => c.ArtifactId).ToList(); + var dependencyGraphs = componentRecorder.GetDependencyGraphsByLocation(); + var gradleLockfileGraph = dependencyGraphs[dependencyGraphs.Keys.First(k => k.EndsWith(Path.DirectorySeparatorChar + "gradle.lockfile"))]; + var settingsGradleLockfileGraph = dependencyGraphs[dependencyGraphs.Keys.First(k => k.EndsWith("settings-gradle.lockfile"))]; + var buildscriptGradleLockfileGraph = dependencyGraphs[dependencyGraphs.Keys.First(k => k.EndsWith("buildscript-gradle.lockfile"))]; + + discoveredComponents.Should().HaveCount(4); + + // Dev dependency listed only in settings-gradle.lockfile + var component = discoveredComponents[0]; + component.GroupId.Should().Be("org.hamcrest"); + component.ArtifactId.Should().Be("hamcrest-core"); + settingsGradleLockfileGraph.IsDevelopmentDependency(component.Id).Should().BeTrue(); + + // Dev dependency listed only in buildscript-gradle.lockfile + component = discoveredComponents[1]; + component.GroupId.Should().Be("org.jacoco"); + component.ArtifactId.Should().Be("org.jacoco.agent"); + buildscriptGradleLockfileGraph.IsDevelopmentDependency(component.Id).Should().BeTrue(); + + // This should be purely a prod dependency, just a basic confidence test + component = discoveredComponents[2]; + component.GroupId.Should().Be("org.springframework"); + component.ArtifactId.Should().Be("spring-beans"); + gradleLockfileGraph.IsDevelopmentDependency(component.Id).Should().BeFalse(); + + // This is listed as both a prod and a dev dependency in different files + component = discoveredComponents[3]; + component.GroupId.Should().Be("org.springframework"); + component.ArtifactId.Should().Be("spring-core"); + gradleLockfileGraph.IsDevelopmentDependency(component.Id).Should().BeFalse(); + settingsGradleLockfileGraph.IsDevelopmentDependency(component.Id).Should().BeTrue(); + } + + [TestMethod] + public async Task TestGradleDetector_DevDependenciesByDevLockfileEnvironmentAsync() + { + var regularLockfile = + @"org.springframework:spring-beans:5.0.5.RELEASE +org.springframework:spring-core:5.0.5.RELEASE"; + + var devLockfile1 = @"org.hamcrest:hamcrest-core:2.2 +org.springframework:spring-core:5.0.5.RELEASE"; + + var devLockfile2 = @"org.jacoco:org.jacoco.agent:0.8.8"; + + this.envVarService.Setup(x => x.GetListEnvironmentVariable("CD_GRADLE_DEV_LOCKFILES", ",")).Returns(new List { "dev1\\gradle.lockfile", "dev2\\gradle.lockfile" }); + + var (scanResult, componentRecorder) = await this.DetectorTestUtility + .WithFile("dev1\\gradle.lockfile", devLockfile1) + .WithFile("dev2\\gradle.lockfile", devLockfile2) + .WithFile("prod\\gradle.lockfile", regularLockfile) + .ExecuteDetectorAsync(); + + scanResult.ResultCode.Should().Be(ProcessingResultCode.Success); + + var discoveredComponents = componentRecorder.GetDetectedComponents().Select(c => (MavenComponent)c.Component).OrderBy(c => c.ArtifactId).ToList(); + var dependencyGraphs = componentRecorder.GetDependencyGraphsByLocation(); + var gradleLockfileGraph = dependencyGraphs[dependencyGraphs.Keys.First(k => k.EndsWith("prod\\gradle.lockfile"))]; + var dev1GradleLockfileGraph = dependencyGraphs[dependencyGraphs.Keys.First(k => k.EndsWith("dev1\\gradle.lockfile"))]; + var dev2GradleLockfileGraph = dependencyGraphs[dependencyGraphs.Keys.First(k => k.EndsWith("dev2\\gradle.lockfile"))]; + + discoveredComponents.Should().HaveCount(4); + + // Dev dependency listed only in dev1\gradle.lockfile + var component = discoveredComponents[0]; + component.GroupId.Should().Be("org.hamcrest"); + component.ArtifactId.Should().Be("hamcrest-core"); + dev1GradleLockfileGraph.IsDevelopmentDependency(component.Id).Should().BeTrue(); + + // Dev dependency listed only in dev2\gradle.lockfile + component = discoveredComponents[1]; + component.GroupId.Should().Be("org.jacoco"); + component.ArtifactId.Should().Be("org.jacoco.agent"); + dev2GradleLockfileGraph.IsDevelopmentDependency(component.Id).Should().BeTrue(); + + // This should be purely a prod dependency, just a basic confidence test + component = discoveredComponents[2]; + component.GroupId.Should().Be("org.springframework"); + component.ArtifactId.Should().Be("spring-beans"); + gradleLockfileGraph.IsDevelopmentDependency(component.Id).Should().BeFalse(); + + // This is listed as both a prod and a dev dependency in different files + component = discoveredComponents[3]; + component.GroupId.Should().Be("org.springframework"); + component.ArtifactId.Should().Be("spring-core"); + gradleLockfileGraph.IsDevelopmentDependency(component.Id).Should().BeFalse(); + + dev1GradleLockfileGraph.IsDevelopmentDependency(component.Id).Should().BeTrue(); + } + + [TestMethod] + public async Task TestGradleDetector_DevDependenciesByDevConfigurationEnvironmentAsync() + { + var lockfile = + @"org.springframework:spring-beans:5.0.5.RELEASE=assembleRelease +org.springframework:spring-core:5.0.5.RELEASE=assembleRelease,testDebugUnitTest +org.hamcrest:hamcrest-core:2.2=testReleaseUnitTest"; + + this.envVarService.Setup(x => x.GetListEnvironmentVariable("CD_GRADLE_DEV_CONFIGURATIONS", ",")).Returns(new List { "testDebugUnitTest", "testReleaseUnitTest" }); + + var (scanResult, componentRecorder) = await this.DetectorTestUtility + .WithFile("gradle.lockfile", lockfile) + .ExecuteDetectorAsync(); + + scanResult.ResultCode.Should().Be(ProcessingResultCode.Success); + + var discoveredComponents = componentRecorder.GetDetectedComponents().Select(c => (MavenComponent)c.Component).OrderBy(c => c.ArtifactId).ToList(); + var dependencyGraph = componentRecorder.GetDependencyGraphsByLocation().Values.First(); + + discoveredComponents.Should().HaveCount(3); + + var component = discoveredComponents[0]; + component.GroupId.Should().Be("org.hamcrest"); + component.ArtifactId.Should().Be("hamcrest-core"); + + // Purely a dev dependency, only present in a test configuration + dependencyGraph.IsDevelopmentDependency(component.Id).Should().BeTrue(); + + component = discoveredComponents[1]; + component.GroupId.Should().Be("org.springframework"); + component.ArtifactId.Should().Be("spring-beans"); + + // Purely a prod dependency, only present in a prod configuration + dependencyGraph.IsDevelopmentDependency(component.Id).Should().BeFalse(); + + component = discoveredComponents[2]; + component.GroupId.Should().Be("org.springframework"); + component.ArtifactId.Should().Be("spring-core"); + + // Present in both dev and prod configurations, prod should win + dependencyGraph.IsDevelopmentDependency(component.Id).Should().BeFalse(); + } }