From 572671b58ee83d9c00de042b4f9c79ddc4c46b1a Mon Sep 17 00:00:00 2001 From: Teo Voinea <58236992+tevoinea@users.noreply.github.com> Date: Thu, 16 Feb 2023 13:08:30 -0500 Subject: [PATCH] Sematically validate notification configs (#2850) * Add new command * Update remaining jinja templates and references to use scriban * Add ado template validation * Validate ado and github templates * Remove unnecessary function * Update src/ApiService/ApiService/OneFuzzTypes/Model.cs Co-authored-by: Cheick Keita --------- Co-authored-by: Cheick Keita --- src/ApiService/ApiService/FeatureFlags.cs | 1 + .../ApiService/OneFuzzTypes/Model.cs | 23 +++++-- .../onefuzzlib/NotificationOperations.cs | 8 ++- .../onefuzzlib/notifications/Ado.cs | 66 +++++++++++++++---- .../onefuzzlib/notifications/GithubIssues.cs | 37 +++++++++-- .../bicep-templates/feature-flags.bicep | 13 ++++ 6 files changed, 126 insertions(+), 22 deletions(-) diff --git a/src/ApiService/ApiService/FeatureFlags.cs b/src/ApiService/ApiService/FeatureFlags.cs index 97fdfcad1..7533b66c3 100644 --- a/src/ApiService/ApiService/FeatureFlags.cs +++ b/src/ApiService/ApiService/FeatureFlags.cs @@ -3,4 +3,5 @@ public static class FeatureFlagConstants { public const string EnableScribanOnly = "EnableScribanOnly"; public const string EnableNodeDecommissionStrategy = "EnableNodeDecommissionStrategy"; + public const string EnableValidateNotificationConfigSemantics = "EnableValidateNotificationConfigSemantics"; } diff --git a/src/ApiService/ApiService/OneFuzzTypes/Model.cs b/src/ApiService/ApiService/OneFuzzTypes/Model.cs index a2f4416bc..7e1b08954 100644 --- a/src/ApiService/ApiService/OneFuzzTypes/Model.cs +++ b/src/ApiService/ApiService/OneFuzzTypes/Model.cs @@ -1,6 +1,7 @@ using System.Reflection; using System.Text.Json; using System.Text.Json.Serialization; +using System.Threading.Tasks; using Microsoft.OneFuzz.Service.OneFuzzLib.Orm; using Endpoint = System.String; using GroupId = System.Guid; @@ -536,6 +537,7 @@ public record RegressionReport( #pragma warning disable CA1715 public interface NotificationTemplate { #pragma warning restore CA1715 + Async.Task Validate(); } @@ -637,10 +639,19 @@ public record AdoTemplate( Dictionary AdoFields, ADODuplicateTemplate OnDuplicate, string? Comment = null - ) : NotificationTemplate; - -public record TeamsTemplate(SecretData Url) : NotificationTemplate; + ) : NotificationTemplate { + public async Task Validate() { + return await Ado.Validate(this); + } +} +public record TeamsTemplate(SecretData Url) : NotificationTemplate { + public Task Validate() { + // The only way we can validate in the current state is to send a test webhook + // Maybe there's a teams nuget package we can pull in to help validate + return Async.Task.FromResult(OneFuzzResultVoid.Ok); + } +} public record GithubAuth(string User, string PersonalAccessToken); @@ -668,7 +679,11 @@ public record GithubIssuesTemplate( List Assignees, List Labels, GithubIssueDuplicate OnDuplicate - ) : NotificationTemplate; + ) : NotificationTemplate { + public async Task Validate() { + return await GithubIssues.Validate(this); + } +} public record Repro( [PartitionKey][RowKey] Guid VmId, diff --git a/src/ApiService/ApiService/onefuzzlib/NotificationOperations.cs b/src/ApiService/ApiService/onefuzzlib/NotificationOperations.cs index fa79284c9..98828d539 100644 --- a/src/ApiService/ApiService/onefuzzlib/NotificationOperations.cs +++ b/src/ApiService/ApiService/onefuzzlib/NotificationOperations.cs @@ -96,6 +96,13 @@ public class NotificationOperations : Orm, INotificationOperations return OneFuzzResult.Error(ErrorCode.INVALID_REQUEST, "The notification config is not a valid scriban template"); } + if (await _context.FeatureManagerSnapshot.IsEnabledAsync(FeatureFlagConstants.EnableValidateNotificationConfigSemantics)) { + var validConfig = await config.Validate(); + if (!validConfig.IsOk) { + return OneFuzzResult.Error(validConfig.ErrorV); + } + } + if (replaceExisting) { var existing = this.SearchByRowKeys(new[] { container.String }); await foreach (var existingEntry in existing) { @@ -138,7 +145,6 @@ public class NotificationOperations : Orm, INotificationOperations public async Async.Task GetRegressionReportTask(RegressionReport report) { if (report.CrashTestResult.CrashReport != null) { - return await _context.TaskOperations.GetByJobIdAndTaskId(report.CrashTestResult.CrashReport.JobId, report.CrashTestResult.CrashReport.TaskId); } if (report.CrashTestResult.NoReproReport != null) { diff --git a/src/ApiService/ApiService/onefuzzlib/notifications/Ado.cs b/src/ApiService/ApiService/onefuzzlib/notifications/Ado.cs index 2b6b5ac2e..d9c801401 100644 --- a/src/ApiService/ApiService/onefuzzlib/notifications/Ado.cs +++ b/src/ApiService/ApiService/onefuzzlib/notifications/Ado.cs @@ -2,13 +2,13 @@ using Microsoft.TeamFoundation.WorkItemTracking.WebApi; using Microsoft.TeamFoundation.WorkItemTracking.WebApi.Models; using Microsoft.VisualStudio.Services.Common; +using Microsoft.VisualStudio.Services.WebApi; using Microsoft.VisualStudio.Services.WebApi.Patch.Json; namespace Microsoft.OneFuzz.Service; public interface IAdo { public Async.Task NotifyAdo(AdoTemplate config, Container container, string filename, IReport reportable, bool isLastRetryAttempt, Guid notificationId); - } public class Ado : NotificationsBase, IAdo { @@ -61,6 +61,55 @@ public class Ado : NotificationsBase, IAdo { return errorCodes.Any(code => errorStr.Contains(code)); } + public static async Async.Task Validate(AdoTemplate config) { + // Validate PAT is valid for the base url + VssConnection connection; + if (config.AuthToken.Secret is SecretValue token) { + try { + connection = new VssConnection(config.BaseUrl, new VssBasicCredential(string.Empty, token.Value)); + await connection.ConnectAsync(); + } catch { + return OneFuzzResultVoid.Error(ErrorCode.INVALID_CONFIGURATION, $"Failed to connect to {config.BaseUrl} using the provided token"); + } + } else { + return OneFuzzResultVoid.Error(ErrorCode.INVALID_CONFIGURATION, "Auth token is missing or invalid"); + } + + try { + // Validate unique_fields are part of the project's valid fields + var witClient = await connection.GetClientAsync(); + + // The set of valid fields for this project according to ADO + var projectValidFields = await GetValidFields(witClient, config.Project); + + var configFields = config.UniqueFields.Select(field => field.ToLowerInvariant()).ToHashSet(); + var validConfigFields = configFields.Intersect(projectValidFields.Keys).ToHashSet(); + + if (!validConfigFields.SetEquals(configFields)) { + var invalidFields = configFields.Except(validConfigFields); + return OneFuzzResultVoid.Error(ErrorCode.INVALID_CONFIGURATION, new[] + { + $"The following unique fields are not valid fields for this project: {string.Join(',', invalidFields)}", + "You can find the valid fields for your project by following these steps: https://learn.microsoft.com/en-us/azure/devops/boards/work-items/work-item-fields?view=azure-devops#review-fields" + } + ); + } + } catch { + return OneFuzzResultVoid.Error(ErrorCode.INVALID_CONFIGURATION, "Failed to query and compare the valid fields for this project"); + } + + return OneFuzzResultVoid.Ok; + } + + private static WorkItemTrackingHttpClient GetAdoClient(Uri baseUrl, string token) { + return new WorkItemTrackingHttpClient(baseUrl, new VssBasicCredential("PAT", token)); + } + + private static async Async.Task> GetValidFields(WorkItemTrackingHttpClient client, string? project) { + return (await client.GetFieldsAsync(project, expand: GetFieldsExpand.ExtensionFields)) + .ToDictionary(field => field.ReferenceName.ToLowerInvariant()); + } + sealed class AdoConnector { private readonly AdoTemplate _config; private readonly Renderer _renderer; @@ -75,13 +124,11 @@ public class Ado : NotificationsBase, IAdo { var authToken = await context.SecretsOperations.GetSecretStringValue(config.AuthToken); var client = GetAdoClient(config.BaseUrl, authToken!); - return new AdoConnector(container, filename, config, report, renderer, project!, client, instanceUrl, logTracer); + return new AdoConnector(config, renderer, project!, client, instanceUrl, logTracer); } - private static WorkItemTrackingHttpClient GetAdoClient(Uri baseUrl, string token) { - return new WorkItemTrackingHttpClient(baseUrl, new VssBasicCredential("PAT", token)); - } - public AdoConnector(Container container, string filename, AdoTemplate config, Report report, Renderer renderer, string project, WorkItemTrackingHttpClient client, Uri instanceUrl, ILogTracer logTracer) { + + public AdoConnector(AdoTemplate config, Renderer renderer, string project, WorkItemTrackingHttpClient client, Uri instanceUrl, ILogTracer logTracer) { _config = config; _renderer = renderer; _project = project; @@ -112,7 +159,7 @@ public class Ado : NotificationsBase, IAdo { } var project = filters.TryGetValue("system.teamproject", out var value) ? value : null; - var validFields = await GetValidFields(project); + var validFields = await GetValidFields(_client, project); var postQueryFilter = new Dictionary(); /* @@ -235,11 +282,6 @@ public class Ado : NotificationsBase, IAdo { return stateUpdated; } - private async Async.Task> GetValidFields(string? project) { - return (await _client.GetFieldsAsync(project, expand: GetFieldsExpand.ExtensionFields)) - .ToDictionary(field => field.ReferenceName.ToLowerInvariant()); - } - private async Async.Task CreateNew() { var (taskType, document) = await RenderNew(); var entry = await _client.CreateWorkItemAsync(document, _project, taskType); diff --git a/src/ApiService/ApiService/onefuzzlib/notifications/GithubIssues.cs b/src/ApiService/ApiService/onefuzzlib/notifications/GithubIssues.cs index 0a8dc696c..88020d014 100644 --- a/src/ApiService/ApiService/onefuzzlib/notifications/GithubIssues.cs +++ b/src/ApiService/ApiService/onefuzzlib/notifications/GithubIssues.cs @@ -30,6 +30,35 @@ public class GithubIssues : NotificationsBase, IGithubIssues { } } + public static async Async.Task Validate(GithubIssuesTemplate config) { + // Validate PAT is valid + GitHubClient gh; + if (config.Auth.Secret is SecretValue auth) { + try { + gh = GetGitHubClient(auth.Value.User, auth.Value.PersonalAccessToken); + var _ = await gh.User.Get(auth.Value.User); + } catch { + return OneFuzzResultVoid.Error(ErrorCode.INVALID_CONFIGURATION, $"Failed to login to github.com with user {auth.Value.User} and the provided Personal Access Token"); + } + } else { + return OneFuzzResultVoid.Error(ErrorCode.INVALID_CONFIGURATION, $"GithubAuth is missing or invalid"); + } + + try { + var _ = await gh.Repository.Get(config.Organization, config.Repository); + } catch { + return OneFuzzResultVoid.Error(ErrorCode.INVALID_CONFIGURATION, $"Failed to access repository: {config.Organization}/{config.Repository}"); + } + + return OneFuzzResultVoid.Ok; + } + + private static GitHubClient GetGitHubClient(string user, string pat) { + return new GitHubClient(new ProductHeaderValue("OneFuzz")) { + Credentials = new Credentials(user, pat) + }; + } + private async Async.Task Process(GithubIssuesTemplate config, Container container, string filename, Report report) { var renderer = await Renderer.ConstructRenderer(_context, container, filename, report, _logTracer); var handler = await GithubConnnector.GithubConnnectorCreator(config, container, filename, renderer, _context.Creds.GetInstanceUrl(), _context, _logTracer); @@ -48,14 +77,12 @@ public class GithubIssues : NotificationsBase, IGithubIssues { SecretValue sv => sv.Value, _ => throw new ArgumentException($"Unexpected secret type {config.Auth.Secret.GetType()}") }; - return new GithubConnnector(config, container, filename, renderer, instanceUrl, auth!, logTracer); + return new GithubConnnector(config, renderer, instanceUrl, auth!, logTracer); } - public GithubConnnector(GithubIssuesTemplate config, Container container, string filename, Renderer renderer, Uri instanceUrl, GithubAuth auth, ILogTracer logTracer) { + public GithubConnnector(GithubIssuesTemplate config, Renderer renderer, Uri instanceUrl, GithubAuth auth, ILogTracer logTracer) { _config = config; - _gh = new GitHubClient(new ProductHeaderValue("OneFuzz")) { - Credentials = new Credentials(auth.User, auth.PersonalAccessToken) - }; + _gh = GetGitHubClient(auth.User, auth.PersonalAccessToken); _renderer = renderer; _instanceUrl = instanceUrl; _logTracer = logTracer; diff --git a/src/deployment/bicep-templates/feature-flags.bicep b/src/deployment/bicep-templates/feature-flags.bicep index bb676b1d0..7c8810982 100644 --- a/src/deployment/bicep-templates/feature-flags.bicep +++ b/src/deployment/bicep-templates/feature-flags.bicep @@ -24,4 +24,17 @@ resource configStoreFeatureflag 'Microsoft.AppConfiguration/configurationStores/ } } +resource validateNotificationConfigSemantics 'Microsoft.AppConfiguration/configurationStores/keyValues@2021-10-01-preview' = { + parent: featureFlags + name: '.appconfig.featureflag~2FEnableValidateNotificationConfigSemantics' + properties: { + value: string({ + id: 'EnableScribanOnly' + description: 'Check notification configs for valid PATs and fields' + enabled: true + }) + contentType: 'application/vnd.microsoft.appconfig.ff+json;charset=utf-8' + } +} + output AppConfigEndpoint string = 'https://${appConfigName}.azconfig.io'