Repository sign packages owned by users with "malformed" usernames (#686)

Repository sign packages that are owned by usernames with "malformed" usernames.

Addresses https://github.com/NuGet/Engineering/issues/1582 and https://github.com/NuGet/Engineering/issues/1592.

Author: loic-sharma

src/NuGet.Services.Validation.Orchestrator/NuGet.Services.Validation.Orchestrator.csproj

@@ -69,7 +69,6 @@
     <Compile Include="Symbols\SymbolsIngester.cs" />
     <Compile Include="Symbols\SymbolsValidator.cs" />
     <Compile Include="Symbols\SymbolsValidationConfiguration.cs" />
-    <Compile Include="UsernameHelper.cs" />
     <Compile Include="ValidatingEntitites\IValidatingEntity.cs" />
     <Compile Include="IValidationOutcomeProcessor.cs" />
     <Compile Include="IValidationPackageFileService.cs" />

src/NuGet.Services.Validation.Orchestrator/PackageSigning/ProcessSignature/PackageSignatureValidator.cs

@@ -93,20 +93,6 @@ private IValidationResult Validate(IValidationRequest request, IValidationResult
                     return ValidationResult.Succeeded;
                 }
 
-                // TODO: Remove this.
-                // See: https://github.com/NuGet/Engineering/issues/1592
-                if (HasOwnerWithInvalidUsername(request))
-                {
-                    _logger.LogWarning(
-                        "Ignoring invalid validation result in package signature validator as the package has an owner with an invalid username. " +
-                        "Status = {ValidationStatus}, Nupkg URL = {NupkgUrl}, validation issues = {Issues}",
-                        result.Status,
-                        result.NupkgUrl,
-                        result.Issues.Select(i => i.IssueCode));
-
-                    return ValidationResult.Succeeded;
-                }
-
                 _logger.LogCritical(
                     "Unexpected validation result in package signature validator. This may be caused by an invalid repository " +
                     "signature. Throwing an exception to force this validation to dead-letter. " +
@@ -133,32 +119,5 @@ private IValidationResult Validate(IValidationRequest request, IValidationResult
 
             return result;
         }
-
-        private bool HasOwnerWithInvalidUsername(IValidationRequest request)
-        {
-            var registration = _packages.FindPackageRegistrationById(request.PackageId);
-
-            if (registration == null)
-            {
-                _logger.LogError("Attempted to validate package that has no package registration");
-
-                throw new InvalidOperationException($"Registration for package id {request.PackageId} does not exist");
-            }
-
-            var owners = registration.Owners.Select(o => o.Username).ToList();
-
-            if (owners.Any(UsernameHelper.IsInvalid))
-            {
-                _logger.LogWarning(
-                    "Package {PackageId} {PackageVersion} has an owner with an invalid username. {Owners}",
-                    request.PackageId,
-                    request.PackageVersion,
-                    owners);
-
-                return true;
-            }
-
-            return false;
-        }
     }
 }

src/NuGet.Services.Validation.Orchestrator/PackageSigning/ScanAndSign/ScanAndSignProcessor.cs

@@ -134,7 +134,7 @@ public async Task<IValidationResult> StartAsync(IValidationRequest request)
 
             var owners = FindPackageOwners(request);
 
-            if (await ShouldRepositorySignAsync(request, owners))
+            if (await ShouldRepositorySignAsync(request))
             {
                 _logger.LogInformation(
                     "Repository signing {PackageId} {PackageVersion} with {ServiceIndex} and {Owners}",
@@ -198,7 +198,7 @@ private bool ShouldSkipScan(IValidationRequest request)
             return false;
         }
 
-        private async Task<bool> ShouldRepositorySignAsync(IValidationRequest request, List<string> owners)
+        private async Task<bool> ShouldRepositorySignAsync(IValidationRequest request)
         {
             var hasRepositorySignature = await _validationContext
                 .PackageSignatures
@@ -216,19 +216,6 @@ private async Task<bool> ShouldRepositorySignAsync(IValidationRequest request, L
                 return false;
             }
 
-            // TODO: Remove this check.
-            // See: https://github.com/NuGet/Engineering/issues/1582
-            if (owners.Any(UsernameHelper.IsInvalid))
-            {
-                _logger.LogWarning(
-                    "Package {PackageId} {PackageVersion} has an owner with an invalid username. Scanning instead of signing. {Owners}",
-                    request.PackageId,
-                    request.PackageVersion,
-                    owners);
-
-                return false;
-            }
-
             return true;
         }
 

src/NuGet.Services.Validation.Orchestrator/UsernameHelper.cs

@@ -153,37 +153,6 @@ public async Task WhenRepositorySigningEnabled_ThrowsExceptionIfPackageIsModifie
                 Assert.Equal("Package signature validator has an unexpected validation result", e.Message);
             }
 
-            [Fact]
-            public async Task WhenRepositorySigningEnabled_IgnoresFailedValidationIfOwnerHasMalformedUsername()
-            {
-                // Arrange
-                _config.RepositorySigningEnabled = true;
-
-                _packages
-                    .Setup(p => p.FindPackageRegistrationById(_validationRequest.Object.PackageId))
-                    .Returns(_packageRegistrationWithBadUsername);
-
-                _validatorStateService
-                    .Setup(x => x.GetStatusAsync(It.IsAny<IValidationRequest>()))
-                    .ReturnsAsync(new ValidatorStatus
-                    {
-                        ValidationId = ValidationId,
-                        PackageKey = PackageKey,
-                        ValidatorName = ValidatorName.PackageSignatureProcessor,
-                        State = ValidationStatus.Failed,
-                        ValidatorIssues = new List<ValidatorIssue>(),
-                    });
-
-                // Act
-                var result = await _target.GetResultAsync(_validationRequest.Object);
-
-                // Assert
-                Assert.Null(result.NupkgUrl);
-                Assert.Empty(result.Issues);
-                Assert.Equal(ValidationStatus.Succeeded, result.Status);
-
-            }
-
             public static IEnumerable<object[]> PossibleValidationStatuses => possibleValidationStatuses.Select(s => new object[] { s });
         }
 
@@ -489,7 +458,6 @@ public abstract class FactsBase
 
             protected readonly ScanAndSignConfiguration _config;
             protected readonly PackageRegistration _packageRegistration;
-            protected readonly PackageRegistration _packageRegistrationWithBadUsername;
 
             public FactsBase(ITestOutputHelper output)
             {
@@ -520,14 +488,6 @@ public FactsBase(ITestOutputHelper output)
                     }
                 };
 
-                _packageRegistrationWithBadUsername = new PackageRegistration
-                {
-                    Owners = new[]
-                    {
-                        new User { Username = "Bad Username" }
-                    }
-                };
-
                 _target = new PackageSignatureValidator(
                     _validatorStateService.Object,
                     _packageSignatureVerifier.Object,

tests/NuGet.Services.Validation.Orchestrator.Tests/PackageSigning/ProcessSignature/PackageSignatureValidatorFacts.cs

@@ -266,30 +266,6 @@ public async Task EnqueuesScanAndSignIfPackageHasNoRepositorySignature()
                 .Verify(v => v.TryAddValidatorStatusAsync(It.IsAny<IValidationRequest>(), It.IsAny<ValidatorStatus>(), It.IsAny<ValidationStatus>()), Times.Once);
         }
 
-        [Fact]
-        public async Task WhenUsernameInvalid_SkipsScanAndSign()
-        {
-            _config.RepositorySigningEnabled = true;
-
-            _validationContext.Mock();
-            _packageServiceMock
-                .Setup(p => p.FindPackageRegistrationById(_request.PackageId))
-                .Returns(_packageRegistrationWithInvalidUser);
-
-            var result = await _target.StartAsync(_request);
-
-            _packageServiceMock
-                .Verify(p => p.FindPackageRegistrationById(_request.PackageId), Times.Once);
-
-            _enqueuerMock
-                .Verify(e => e.EnqueueScanAsync(_request.ValidationId, _request.NupkgUrl), Times.Once);
-
-            _validatorStateServiceMock
-                .Verify(v => v.TryAddValidatorStatusAsync(_request, _status, ValidationStatus.Incomplete), Times.Once);
-            _validatorStateServiceMock
-                .Verify(v => v.TryAddValidatorStatusAsync(It.IsAny<IValidationRequest>(), It.IsAny<ValidatorStatus>(), It.IsAny<ValidationStatus>()), Times.Once);
-        }
-
         [Fact]
         public async Task EnqueuesScanAndSignEvenIfRepositorySigningIsDisabled()
         {
@@ -455,15 +431,6 @@ public async Task WhenPackageFitsCriteriaAndIsNotRepositorySigned_DoesNotSkipSca
             }
         };
 
-        private PackageRegistration _packageRegistrationWithInvalidUser = new PackageRegistration
-        {
-            Owners = new List<User>
-            {
-                new User("Billy"),
-                new User("Satan Claus"),
-            }
-        };
-
         public TheStartAsyncMethod()
         {
             _request = new ValidationRequest(Guid.NewGuid(), 42, "somepackage", "somversion", "https://example.com/package.nupkg");