diff --git a/src/Tgstation.Server.Api/Models/TestMergeParameters.cs b/src/Tgstation.Server.Api/Models/TestMergeParameters.cs index ed27e6bb08..7c84c05cdb 100644 --- a/src/Tgstation.Server.Api/Models/TestMergeParameters.cs +++ b/src/Tgstation.Server.Api/Models/TestMergeParameters.cs @@ -10,10 +10,10 @@ namespace Tgstation.Server.Api.Models /// /// The number of the pull request /// - public int Number { get; set; } + public int? Number { get; set; } /// - /// The sha of the pull request revision to merge + /// The sha of the pull request revision to merge. If not specified, the latest commit shall be used (semi-unsafe) /// [Required] public string PullRequestRevision { get; set; } diff --git a/src/Tgstation.Server.Host/Components/Repository/IRepository.cs b/src/Tgstation.Server.Host/Components/Repository/IRepository.cs index 16a64142f1..3ca1a934bc 100644 --- a/src/Tgstation.Server.Host/Components/Repository/IRepository.cs +++ b/src/Tgstation.Server.Host/Components/Repository/IRepository.cs @@ -1,6 +1,7 @@ using System; using System.Threading; using System.Threading.Tasks; +using Tgstation.Server.Api.Models; namespace Tgstation.Server.Host.Components.Repository { @@ -55,17 +56,15 @@ namespace Tgstation.Server.Host.Components.Repository /// /// Attempt to merge a GitHub pull request into HEAD /// - /// The pull request number on the remote repository - /// The commit in the pull request to merge + /// The of the pull request /// The name of the merge committer /// The e-mail of the merge committer - /// The commit message /// The username to fetch from the origin repository /// The password to fetch from the origin repository /// The for the operation /// Optional function to report 0-100 progress of the clone /// A resulting in a representing the merge result that is after a fast forward or up to date, on a merge, on a conflict - Task AddTestMerge(int pullRequestNumber, string targetCommit, string committerName, string committerEmail, string commitMessage, string username, string password, Action progressReporter, CancellationToken cancellationToken); + Task AddTestMerge(TestMergeParameters testMergeParameters, string committerName, string committerEmail, string username, string password, Action progressReporter, CancellationToken cancellationToken); /// /// Fetch commits from the origin repository diff --git a/src/Tgstation.Server.Host/Components/Repository/Repository.cs b/src/Tgstation.Server.Host/Components/Repository/Repository.cs index b04a0efa63..4c9c6ce9cc 100644 --- a/src/Tgstation.Server.Host/Components/Repository/Repository.cs +++ b/src/Tgstation.Server.Host/Components/Repository/Repository.cs @@ -5,6 +5,7 @@ using System.Globalization; using System.Linq; using System.Threading; using System.Threading.Tasks; +using Tgstation.Server.Api.Models; using Tgstation.Server.Host.IO; namespace Tgstation.Server.Host.Components.Repository @@ -121,17 +122,27 @@ namespace Tgstation.Server.Host.Components.Repository } /// - public async Task AddTestMerge(int pullRequestNumber, string targetCommit, string committerName, string committerEmail, string commitMessage, string username, string password, Action progressReporter, CancellationToken cancellationToken) + public async Task AddTestMerge(TestMergeParameters testMergeParameters, string committerName, string committerEmail, string username, string password, Action progressReporter, CancellationToken cancellationToken) { + if (testMergeParameters == null) + throw new ArgumentNullException(nameof(testMergeParameters)); + + if (committerName == null) + throw new ArgumentNullException(nameof(committerName)); + if (committerEmail == null) + throw new ArgumentNullException(nameof(committerEmail)); if (!IsGitHubRepository) throw new InvalidOperationException("Test merging is only available on GitHub hosted origin repositories!"); - var Refspec = new List(); - var prBranchName = String.Format(CultureInfo.InvariantCulture, "pr-{0}", pullRequestNumber); - var localBranchName = String.Format(CultureInfo.InvariantCulture, "pull/{0}/headrefs/heads/{1}", pullRequestNumber, prBranchName); - Refspec.Add(String.Format(CultureInfo.InvariantCulture, "pull/{0}/head:{1}", pullRequestNumber, prBranchName)); - var logMessage = String.Format(CultureInfo.InvariantCulture, "Merge remote pull request #{0}", pullRequestNumber); + var commitMessage = String.Format(CultureInfo.InvariantCulture, "Test merge of pull request #{0}{1}{2}", testMergeParameters.Number.Value, testMergeParameters.Comment != null ? Environment.NewLine : String.Empty, testMergeParameters.Comment ?? String.Empty); + + + var prBranchName = String.Format(CultureInfo.InvariantCulture, "pr-{0}", testMergeParameters.Number); + var localBranchName = String.Format(CultureInfo.InvariantCulture, "pull/{0}/headrefs/heads/{1}", testMergeParameters.Number, prBranchName); + + var Refspec = new List { String.Format(CultureInfo.InvariantCulture, "pull/{0}/head:{1}", testMergeParameters.Number, prBranchName) }; + var logMessage = String.Format(CultureInfo.InvariantCulture, "Merge remote pull request #{0}", testMergeParameters.Number); var originalCommit = repository.Head; @@ -142,39 +153,47 @@ namespace Tgstation.Server.Host.Components.Repository { try { - var remote = repository.Network.Remotes.First(); - Commands.Fetch((LibGit2Sharp.Repository)repository, remote.Name, Refspec, new FetchOptions + try { - Prune = true, - OnProgress = (a) => !cancellationToken.IsCancellationRequested, - OnTransferProgress = (a) => + var remote = repository.Network.Remotes.First(); + Commands.Fetch((LibGit2Sharp.Repository)repository, remote.Name, Refspec, new FetchOptions { - var percentage = 100 * (((float)a.IndexedObjects + a.ReceivedObjects) / (a.TotalObjects * 2)); - progressReporter?.Invoke((int)percentage); - return !cancellationToken.IsCancellationRequested; - }, - OnUpdateTips = (a, b, c) => !cancellationToken.IsCancellationRequested, - CredentialsProvider = (a, b, c) => username != null ? (Credentials)new UsernamePasswordCredentials - { - Username = username, - Password = password - } : new DefaultCredentials() - }, logMessage); - } - catch (UserCancelledException) - { + Prune = true, + OnProgress = (a) => !cancellationToken.IsCancellationRequested, + OnTransferProgress = (a) => + { + var percentage = 100 * (((float)a.IndexedObjects + a.ReceivedObjects) / (a.TotalObjects * 2)); + progressReporter?.Invoke((int)percentage); + return !cancellationToken.IsCancellationRequested; + }, + OnUpdateTips = (a, b, c) => !cancellationToken.IsCancellationRequested, + CredentialsProvider = (a, b, c) => username != null ? (Credentials)new UsernamePasswordCredentials + { + Username = username, + Password = password + } : new DefaultCredentials() + }, logMessage); + } + catch (UserCancelledException) { } + cancellationToken.ThrowIfCancellationRequested(); + + testMergeParameters.PullRequestRevision = repository.Lookup(testMergeParameters.PullRequestRevision ?? localBranchName).Sha; + + cancellationToken.ThrowIfCancellationRequested(); + + result = repository.Merge(testMergeParameters.PullRequestRevision, sig, new MergeOptions + { + CommitOnSuccess = commitMessage == null, + FailOnConflict = true, + FastForwardStrategy = FastForwardStrategy.NoFastForward, + SkipReuc = true + }); } - - cancellationToken.ThrowIfCancellationRequested(); - - result = repository.Merge(targetCommit, sig, new MergeOptions + finally { - CommitOnSuccess = commitMessage == null, - FailOnConflict = true, - FastForwardStrategy = FastForwardStrategy.NoFastForward, - SkipReuc = true - }); + repository.Branches.Remove(localBranchName); + } cancellationToken.ThrowIfCancellationRequested(); @@ -189,7 +208,7 @@ namespace Tgstation.Server.Host.Components.Repository if (result.Status == MergeStatus.Conflicts) { - await eventConsumer.HandleEvent(EventType.RepoMergeConflict, new List { originalCommit.Tip.Sha, targetCommit, originalCommit.FriendlyName ?? UnknownReference, prBranchName }, cancellationToken).ConfigureAwait(false); + await eventConsumer.HandleEvent(EventType.RepoMergeConflict, new List { originalCommit.Tip.Sha, testMergeParameters.PullRequestRevision, originalCommit.FriendlyName ?? UnknownReference, prBranchName }, cancellationToken).ConfigureAwait(false); return false; } diff --git a/src/Tgstation.Server.Host/Controllers/RepositoryController.cs b/src/Tgstation.Server.Host/Controllers/RepositoryController.cs index cd69498335..50bcc733a6 100644 --- a/src/Tgstation.Server.Host/Controllers/RepositoryController.cs +++ b/src/Tgstation.Server.Host/Controllers/RepositoryController.cs @@ -243,6 +243,9 @@ namespace Tgstation.Server.Host.Controllers if (model.Origin != null) return BadRequest(new ErrorMessage { Message = "origin cannot be modified without deleting the repository!" }); + if(model.NewTestMerges.Any(x => !x.Number.HasValue)) + return BadRequest(new ErrorMessage { Message = "All new test merges must provide a number!" }); + var newTestMerges = model.NewTestMerges != null && model.NewTestMerges.Count > 0; var userRights = (RepositoryRights)AuthenticationContext.GetRight(RightsType.Repository); if (newTestMerges && !userRights.HasFlag(RepositoryRights.MergePullRequest)) @@ -396,11 +399,13 @@ namespace Tgstation.Server.Host.Controllers I.PullRequestRevision = I.PullRequestRevision?.ToLowerInvariant(); #pragma warning restore CA1308 // Normalize strings to uppercase - revInfoWereLookingFor = await databaseContext.RevisionInformations.Where( - x => x.OriginCommitSha == lastRevisionInfo.OriginCommitSha - && x.ActiveTestMerges.Count == model.NewTestMerges.Count) + revInfoWereLookingFor = await databaseContext.RevisionInformations + .Where(x => x.OriginCommitSha == lastRevisionInfo.OriginCommitSha && x.ActiveTestMerges.Count == model.NewTestMerges.Count) //split here cause this bit probably has to be done locally - .Where(x => x.ActiveTestMerges.Select(y => y.TestMerge).All(y => model.NewTestMerges.Any(z => y.Number == z.Number && y.PullRequestRevision.StartsWith(z.PullRequestRevision, StringComparison.Ordinal)))).FirstOrDefaultAsync(ct).ConfigureAwait(false); + .Where(x => x.ActiveTestMerges.Select(y => y.TestMerge) + .All(y => model.NewTestMerges.Any(z => y.Number == z.Number && y.PullRequestRevision.StartsWith(z.PullRequestRevision, StringComparison.Ordinal)))) + .Include(x => x.ActiveTestMerges).ThenInclude(x => x.TestMerge) + .FirstOrDefaultAsync(ct).ConfigureAwait(false); } @@ -423,7 +428,7 @@ namespace Tgstation.Server.Host.Controllers string errorMessage = null; try { - pr = await gitHubClient.PullRequest.Get(repoOwner, repoName, I.Number).ConfigureAwait(false); + pr = await gitHubClient.PullRequest.Get(repoOwner, repoName, I.Number.Value).ConfigureAwait(false); } catch (Octokit.RateLimitExceededException) { @@ -432,11 +437,15 @@ namespace Tgstation.Server.Host.Controllers } catch (Octokit.NotFoundException) { - //you look at your shithub access and sigh + //you look at your shithub and sigh errorMessage = "P.R.E. NOT FOUND"; } - var mergeResult = await repo.AddTestMerge(I.Number, I.PullRequestRevision, committerName, currentModel.CommitterEmail, String.Format(CultureInfo.InvariantCulture, "Test merge of pull request #{0}{1}{2}", I.Number, I.Comment != null ? Environment.NewLine : null, I.Comment), currentModel.AccessUser, currentModel.AccessToken, x => progressReporter((x + 100 * doneFetches) / numFetches), ct).ConfigureAwait(false); + //we want to take the earliest truth possible to prevent RCEs, if this fails AddTestMerge will set it + if (I.PullRequestRevision == null && pr != null) + I.PullRequestRevision = pr.Head.Sha; + + var mergeResult = await repo.AddTestMerge(I, committerName, currentModel.CommitterEmail, currentModel.AccessUser, currentModel.AccessToken, x => progressReporter((x + 100 * doneFetches) / numFetches), ct).ConfigureAwait(false); if (!mergeResult.HasValue) //conflict, we don't care, dd already knows continue; @@ -469,7 +478,8 @@ namespace Tgstation.Server.Host.Controllers } } - if (startSha != repo.Head) + //never synchronize with test merges + if (startSha != repo.Head && lastRevisionInfo.ActiveTestMerges.Count == 0) { await repo.Sychronize(currentModel.AccessUser, currentModel.AccessToken, false, ct).ConfigureAwait(false); await UpdateRevInfo().ConfigureAwait(false);