Skip to content
This repository has been archived by the owner on Jul 30, 2024. It is now read-only.
/ NuGet.Jobs Public archive

Commit

Permalink
Repository sign packages owned by users with "malformed" usernames (#686
Browse files Browse the repository at this point in the history 
)

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.
  • Loading branch information
@loic-sharma
loic-sharma authored Nov 29, 2018
1 parent b09cd63 commit 3f59f20
Show file tree
Hide file tree
Showing 6 changed files with 2 additions and 151 deletions.
1 change: 0 additions & 1 deletion src/NuGet.Services.Validation.Orchestrator/NuGet.Services.Validation.Orchestrator.csproj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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" />
Expand Down
41 changes: 0 additions & 41 deletions ...ices.Validation.Orchestrator/PackageSigning/ProcessSignature/PackageSignatureValidator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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. " +
Expand All @@ -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;
}
}
}
17 changes: 2 additions & 15 deletions ...NuGet.Services.Validation.Orchestrator/PackageSigning/ScanAndSign/ScanAndSignProcessor.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}",
Expand Down Expand Up @@ -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
Expand All @@ -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;
}

Expand Down
21 changes: 0 additions & 21 deletions src/NuGet.Services.Validation.Orchestrator/UsernameHelper.cs
View file

This file was deleted.

40 changes: 0 additions & 40 deletions ...tion.Orchestrator.Tests/PackageSigning/ProcessSignature/PackageSignatureValidatorFacts.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
}

Expand Down Expand Up @@ -489,7 +458,6 @@ public abstract class FactsBase

protected readonly ScanAndSignConfiguration _config;
protected readonly PackageRegistration _packageRegistration;
protected readonly PackageRegistration _packageRegistrationWithBadUsername;

public FactsBase(ITestOutputHelper output)
{
Expand Down Expand Up @@ -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,
Expand Down
33 changes: 0 additions & 33 deletions tests/Validation.PackageSigning.ScanAndSign.Tests/ScanAndSignProcessorFacts.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand Down Expand Up @@ -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");
Expand Down

0 comments on commit 3f59f20

Please sign in to comment.