From c5146bd6f859f22753dacf244369581de9564e80 Mon Sep 17 00:00:00 2001 From: Eddy Moulton Date: Wed, 3 Jun 2026 14:21:25 +1000 Subject: [PATCH 1/3] Upfront errors when using the wrong Git creds for PRs --- .../AuthenticatingRepositoryFactoryTests.cs | 29 ++++--- .../ArgoCD/Git/GitConnectionFactoryTests.cs | 79 +++++++++++++++++++ .../Git/AuthenticatingRepositoryFactory.cs | 35 ++------ source/Calamari/ArgoCD/Git/GitConnection.cs | 7 +- .../ArgoCD/Git/GitConnectionFactory.cs | 66 ++++++++++++++++ .../Calamari/ArgoCD/Git/RepositoryAdapter.cs | 2 +- .../Calamari/ArgoCD/Git/RepositoryUpdater.cs | 2 + .../CommitToGit/CommitToGitConfigFactory.cs | 7 +- 8 files changed, 181 insertions(+), 46 deletions(-) create mode 100644 source/Calamari.Tests/ArgoCD/Git/GitConnectionFactoryTests.cs create mode 100644 source/Calamari/ArgoCD/Git/GitConnectionFactory.cs diff --git a/source/Calamari.Tests/ArgoCD/Git/AuthenticatingRepositoryFactoryTests.cs b/source/Calamari.Tests/ArgoCD/Git/AuthenticatingRepositoryFactoryTests.cs index 5d559298fc..0eccc1f24a 100644 --- a/source/Calamari.Tests/ArgoCD/Git/AuthenticatingRepositoryFactoryTests.cs +++ b/source/Calamari.Tests/ArgoCD/Git/AuthenticatingRepositoryFactoryTests.cs @@ -58,7 +58,7 @@ [new GitCredentialDto(httpsUrl, "", "")], repositoryFactory, log); - using var wrapper = factory.CloneRepository(httpsUrl, branchName.ToFriendlyName()); + using var wrapper = factory.CloneRepository(httpsUrl, branchName.ToFriendlyName(), createsPr: false); wrapper.Should().NotBeNull(); } @@ -71,10 +71,23 @@ public void AnonymousCloneWhenNoCredentialsMatch() repositoryFactory, log); - using var wrapper = factory.CloneRepository(originUrl, branchName.ToFriendlyName()); + using var wrapper = factory.CloneRepository(originUrl, branchName.ToFriendlyName(), createsPr: false); wrapper.Should().NotBeNull(); log.Messages.Should().Contain(m => m.FormattedMessage.Contains("No Git credentials found")); } + + [Test] + public void LogsAnInformationalMessageWhenCloningAnonymously() + { + var mockRepoFactory = Substitute.For(); + var factory = new AuthenticatingRepositoryFactory([], mockRepoFactory, log); + + factory.CloneRepository("https://github.com/org/repo.git", "main", createsPr: false); + + mockRepoFactory.Received() + .CloneRepository(Arg.Any(), Arg.Is(c => c is AnonymousGitConnection)); + log.Messages.Should().Contain(m => m.FormattedMessage.Contains("No Git credentials found")); + } } [TestFixture] @@ -95,12 +108,10 @@ [new SshKeyGitCredentialDto(sshUrl, "git", "private-key", [])], mockRepoFactory, log); - factory.CloneRepository(sshUrl, branchName.ToFriendlyName()); + factory.CloneRepository(sshUrl, branchName.ToFriendlyName(), createsPr: false); mockRepoFactory.Received() - .CloneRepository( - Arg.Any(), - Arg.Is(c => c is SshKeyGitConnection)); + .CloneRepository(Arg.Any(), Arg.Is(c => c is SshKeyGitConnection)); } [Test] @@ -127,7 +138,7 @@ [new SshKeyGitCredentialDto(sshUrl, "git", "private-key", knownHosts)], mockRepoFactory, log); - factory.CloneRepository(sshUrl, branchName.ToFriendlyName()); + factory.CloneRepository(sshUrl, branchName.ToFriendlyName(), createsPr: false); mockRepoFactory.Received() .CloneRepository( @@ -159,7 +170,7 @@ [new GitCredentialDto(httpsUrl, "user", "pass")], // This will fail to clone (no real repo at this URL) but we can verify it // falls through to anonymous because the SCP URL doesn't match the HTTPS URL - var act = () => factory.CloneRepository(scpUrl, "main"); + var act = () => factory.CloneRepository(scpUrl, "main", createsPr: false); act.Should().Throw(); // clone failure expected log.Messages.Should().Contain(m => m.FormattedMessage.Contains("No Git credentials found")); } @@ -184,7 +195,7 @@ protected void AssertHttpsCredentialTakesPriorityOverSsh(string url) var factory = new AuthenticatingRepositoryFactory(rawCredentials, mockRepoFactory, log); - factory.CloneRepository(url, "main"); + factory.CloneRepository(url, "main", createsPr: false); mockRepoFactory.Received() .CloneRepository( diff --git a/source/Calamari.Tests/ArgoCD/Git/GitConnectionFactoryTests.cs b/source/Calamari.Tests/ArgoCD/Git/GitConnectionFactoryTests.cs new file mode 100644 index 0000000000..0ce25209d1 --- /dev/null +++ b/source/Calamari.Tests/ArgoCD/Git/GitConnectionFactoryTests.cs @@ -0,0 +1,79 @@ +using System; +using Calamari.ArgoCD.Git; +using Calamari.Common.Commands; +using FluentAssertions; +using NUnit.Framework; +using ArgoCdPasswordCredential = Octopus.Calamari.Contracts.ArgoCD.GitCredentialDto; +using ArgoCdSshKeyCredential = Octopus.Calamari.Contracts.ArgoCD.SshKeyGitCredentialDto; +using GitCredential = Octopus.Calamari.Contracts.Git.IGitCredentialDto; +using GitPasswordCredential = Octopus.Calamari.Contracts.Git.UsernamePasswordGitCredentialDto; +using GitSshKeyCredential = Octopus.Calamari.Contracts.Git.SshKeyGitCredentialDto; + +namespace Calamari.Tests.ArgoCD.Git; + +[TestFixture] +public class GitConnectionFactoryTests +{ + static readonly GitReference Reference = GitBranchName.CreateFromFriendlyName("main"); + const string HttpsUrl = "https://github.com/org/repo.git"; + const string SshUrl = "ssh://git@github.com/org/repo.git"; + + // The ArgoCD overload maps Argo credentials onto the common credential type and delegates to the + // overload below, so we only validate the mapping here; the behaviour is covered by the common tests. + + [Test] + public void ArgoCdSshKeyCredentialIsMappedToSshKeyGitConnection() + { + var connection = GitConnectionFactory.Create(new ArgoCdSshKeyCredential(SshUrl, "git", "private-key", []), SshUrl, Reference, createsPr: false); + + connection.Should().BeOfType(); + } + + [Test] + public void ArgoCdUsernamePasswordCredentialIsMappedToUsernamePasswordGitConnection() + { + var connection = GitConnectionFactory.Create(new ArgoCdPasswordCredential(HttpsUrl, "user", "password"), HttpsUrl, Reference, createsPr: false); + + connection.Should().BeOfType(); + } + + [Test] + public void SshKeyCredentialThrowsWhenCreatingAPullRequest() + { + Action action = () => GitConnectionFactory.Create(new GitSshKeyCredential("cred", SshUrl, "git", "private-key", []), SshUrl, Reference, createsPr: true); + + action.Should().Throw().And.Message.Should().Contain("SSH key authentication"); + } + + [Test] + public void SshKeyCredentialSucceedsWhenNotCreatingAPullRequest() + { + var connection = GitConnectionFactory.Create(new GitSshKeyCredential("cred", SshUrl, "git", "private-key", []), SshUrl, Reference, createsPr: false); + + connection.Should().BeOfType(); + } + + [Test] + public void UsernamePasswordCredentialSucceedsWhenCreatingAPullRequest() + { + var connection = GitConnectionFactory.Create(new GitPasswordCredential("cred", HttpsUrl, "user", "password"), HttpsUrl, Reference, createsPr: true); + + connection.Should().BeOfType(); + } + + [Test] + public void NoCredentialThrowsWhenCreatingAPullRequest() + { + Action action = () => GitConnectionFactory.Create((GitCredential)null, HttpsUrl, Reference, createsPr: true); + + action.Should().Throw().And.Message.Should().Contain("requires Git repository credentials"); + } + + [Test] + public void NoCredentialIsAnonymousWhenNotCreatingAPullRequest() + { + var connection = GitConnectionFactory.Create((GitCredential)null, HttpsUrl, Reference, createsPr: false); + + connection.Should().BeOfType(); + } +} diff --git a/source/Calamari/ArgoCD/Git/AuthenticatingRepositoryFactory.cs b/source/Calamari/ArgoCD/Git/AuthenticatingRepositoryFactory.cs index d24b3ee2c1..65fb9c7a3b 100644 --- a/source/Calamari/ArgoCD/Git/AuthenticatingRepositoryFactory.cs +++ b/source/Calamari/ArgoCD/Git/AuthenticatingRepositoryFactory.cs @@ -26,39 +26,16 @@ public AuthenticatingRepositoryFactory( this.log = log; } - public RepositoryWrapper CloneRepository(string requestedUrl, string targetRevision) + public RepositoryWrapper CloneRepository(string requestedUrl, string targetRevision, bool createsPr) { var gitCredential = gitCredentials.GetValueOrDefault(requestedUrl); - switch (gitCredential) + var gitConnection = GitConnectionFactory.Create(gitCredential, requestedUrl, GitReference.CreateFromString(targetRevision), createsPr); + + if (gitConnection is AnonymousGitConnection) { - case GitCredentialDto passwordCredential: - { - var gitConnection = new HttpsGitConnection(passwordCredential.Username, passwordCredential.Password, GitCloneSafeUrl.ConvertToUriString(requestedUrl), GitReference.CreateFromString(targetRevision)); - return repositoryFactory.CloneRepository(UniqueRepoNameGenerator.Generate(), gitConnection); - } - case SshKeyGitCredentialDto sshCredential: - { - var sshConnection = new SshKeyGitConnection( - sshCredential.Username, - sshCredential.PrivateKey, - requestedUrl, - GitReference.CreateFromString(targetRevision), - sshCredential.KnownHosts.Select(kh => new SshKnownHost(kh.Host, kh.PublicKey)).ToArray()); - return repositoryFactory.CloneRepository(UniqueRepoNameGenerator.Generate(), sshConnection); - } - case null: - { - log.Info($"No Git credentials found for: '{requestedUrl}', will attempt to clone repository anonymously."); - break; - } - default: - { - log.Warn($"An unrecognised credential type '{gitCredential.GetType().Name}' was found for '{requestedUrl}'. Ignoring the credentials and attempting an anonymous clone."); - break; - } + log.Info($"No Git credentials found for: '{requestedUrl}', will attempt to clone repository anonymously."); } - var anonGitConnection = new HttpsGitConnection(null, null, GitCloneSafeUrl.ConvertToUriString(requestedUrl), GitReference.CreateFromString(targetRevision)); - return repositoryFactory.CloneRepository(UniqueRepoNameGenerator.Generate(), anonGitConnection); + return repositoryFactory.CloneRepository(UniqueRepoNameGenerator.Generate(), gitConnection); } } \ No newline at end of file diff --git a/source/Calamari/ArgoCD/Git/GitConnection.cs b/source/Calamari/ArgoCD/Git/GitConnection.cs index a7af69e1f3..5297524214 100644 --- a/source/Calamari/ArgoCD/Git/GitConnection.cs +++ b/source/Calamari/ArgoCD/Git/GitConnection.cs @@ -2,7 +2,6 @@ using System; using System.Collections.Generic; -using System.Linq; using Calamari.Common.Commands; namespace Calamari.ArgoCD.Git @@ -59,6 +58,12 @@ static Uri ParseAsHttpsUri(string repositoryUrl) } } + public class AnonymousGitConnection(string url, GitReference gitReference) + : HttpsGitConnection(null, null, url, gitReference); + + public class UsernamePasswordGitConnection(string username, string password, string url, GitReference gitReference) + : HttpsGitConnection(username, password, url, gitReference); + public record SshKeyGitConnection( string? Username, string PrivateKey, diff --git a/source/Calamari/ArgoCD/Git/GitConnectionFactory.cs b/source/Calamari/ArgoCD/Git/GitConnectionFactory.cs new file mode 100644 index 0000000000..f51ab501e7 --- /dev/null +++ b/source/Calamari/ArgoCD/Git/GitConnectionFactory.cs @@ -0,0 +1,66 @@ +#nullable enable + +using System; +using System.Linq; +using Calamari.Common.Commands; +using Octopus.Calamari.Contracts.Git; +using ArgoCdIGitCredentialDto = Octopus.Calamari.Contracts.ArgoCD.IGitCredentialDto; +using ArgoCdGitCredentialDto = Octopus.Calamari.Contracts.ArgoCD.GitCredentialDto; +using ArgoCdSshKeyGitCredentialDto = Octopus.Calamari.Contracts.ArgoCD.SshKeyGitCredentialDto; + +namespace Calamari.ArgoCD.Git; + +public static class GitConnectionFactory +{ + // Converts Argo credentials into the common type + // ideally we migrate the Argo code path to use the common type from server and we can delete this + public static IGitConnection Create(ArgoCdIGitCredentialDto? credential, string url, GitReference gitReference, bool createsPr) + { + return credential switch + { + ArgoCdGitCredentialDto password => Create( + new UsernamePasswordGitCredentialDto(string.Empty, password.Url, password.Username, password.Password), + url, + gitReference, + createsPr), + ArgoCdSshKeyGitCredentialDto ssh => Create( + new SshKeyGitCredentialDto( + string.Empty, + ssh.Url, + ssh.Username, + ssh.PrivateKey, + ssh.KnownHosts.Select(kh => new SshKnownHostDto(kh.Host, kh.PublicKey)).ToArray()), + url, + gitReference, + createsPr), + _ => Create((IGitCredentialDto?)null, url, gitReference, createsPr), + }; + } + + public static IGitConnection Create(IGitCredentialDto? credential, string url, GitReference gitReference, bool createsPr) + { + return credential switch + { + UsernamePasswordGitCredentialDto password + => new UsernamePasswordGitConnection(password.Username, password.Password, url, gitReference), + + SshKeyGitCredentialDto when createsPr + => throw new CommandException("Creating PRs is not possible when using SSH key authentication, please use a username and password instead"), + + SshKeyGitCredentialDto ssh + => new SshKeyGitConnection( + ssh.Username, + ssh.PrivateKey, + url, + gitReference, + ssh.KnownHosts.Select(kh => new SshKnownHost(kh.Host, kh.PublicKey)).ToArray()), + + null when createsPr + => throw new CommandException("Creating a pull request requires Git repository credentials, but none were provided. Please configure a username and password."), + + null => new AnonymousGitConnection(url, gitReference), + + _ => throw new NotSupportedException($"An unrecognised credential type '{credential.GetType().Name}' was found for '{url}'") + }; + } +} \ No newline at end of file diff --git a/source/Calamari/ArgoCD/Git/RepositoryAdapter.cs b/source/Calamari/ArgoCD/Git/RepositoryAdapter.cs index b8abeddc99..53962d26e4 100644 --- a/source/Calamari/ArgoCD/Git/RepositoryAdapter.cs +++ b/source/Calamari/ArgoCD/Git/RepositoryAdapter.cs @@ -19,7 +19,7 @@ public RepositoryAdapter(AuthenticatingRepositoryFactory repositoryFactory, public SourceUpdateResult Process(ApplicationSourceWithMetadata sourceWithMetadata, ISourceUpdater updater) { - using var repository = repositoryFactory.CloneRepository(sourceWithMetadata.Source.OriginalRepoUrl, sourceWithMetadata.Source.TargetRevision); + using var repository = repositoryFactory.CloneRepository(sourceWithMetadata.Source.OriginalRepoUrl, sourceWithMetadata.Source.TargetRevision, repositoryUpdater.RequiresPr); var filesUpdated = updater.Process(sourceWithMetadata, repository.WorkingDirectory); if (filesUpdated.HasChanges()) diff --git a/source/Calamari/ArgoCD/Git/RepositoryUpdater.cs b/source/Calamari/ArgoCD/Git/RepositoryUpdater.cs index 7f1bfe003c..72398633d2 100644 --- a/source/Calamari/ArgoCD/Git/RepositoryUpdater.cs +++ b/source/Calamari/ArgoCD/Git/RepositoryUpdater.cs @@ -17,6 +17,8 @@ public RepositoryUpdater(GitCommitParameters commitParameters, ILog log, ICommit this.log = log; this.commitMessageGenerator = commitMessageGenerator; } + + public bool RequiresPr => commitParameters.RequiresPr; public PushResult? PushToRemote( RepositoryWrapper repository, diff --git a/source/Calamari/CommitToGit/CommitToGitConfigFactory.cs b/source/Calamari/CommitToGit/CommitToGitConfigFactory.cs index 43bafc535a..eb1a9fc36a 100644 --- a/source/Calamari/CommitToGit/CommitToGitConfigFactory.cs +++ b/source/Calamari/CommitToGit/CommitToGitConfigFactory.cs @@ -37,12 +37,7 @@ public CommitToGitRepositorySettings CreateRepositoryConfig(RunningDeployment de var properties = customPropertiesLoader.Load(); - IGitConnection connection = properties.GitCredential switch - { - UsernamePasswordGitCredentialDto usernamePassword => new HttpsGitConnection(usernamePassword.Username, usernamePassword.Password, uriAsString, GitReference.CreateFromString(gitReferenceAsString)), - SshKeyGitCredentialDto ssh => new SshKeyGitConnection(ssh.Username, ssh.PrivateKey, uriAsString, GitReference.CreateFromString(gitReferenceAsString), ssh.KnownHosts.Select(kh => new SshKnownHost(kh.Host, kh.PublicKey)).ToArray()), - _ => throw new NotSupportedException($"An unrecognised credential type '{properties.GitCredential.GetType().Name}' was found for '{uriAsString}'"), - }; + var connection = GitConnectionFactory.Create(properties.GitCredential, uriAsString, GitReference.CreateFromString(gitReferenceAsString), requiresPullRequest); //Note: Octopus server removes variables containing empty strings, thus a missing property should default to an empty string. return new CommitToGitRepositorySettings(connection, From 4590a7832480c9660fe342f4de9b13bd6918697d Mon Sep 17 00:00:00 2001 From: Eddy Moulton Date: Wed, 3 Jun 2026 15:25:27 +1000 Subject: [PATCH 2/3] Don't pass createPr to factory --- .../AuthenticatingRepositoryFactoryTests.cs | 42 ++++++++++++--- .../ArgoCD/Git/GitConnectionFactoryTests.cs | 52 +++++++------------ .../CommitToGitConfigFactoryTests.cs | 28 ++++++++++ .../Git/AuthenticatingRepositoryFactory.cs | 16 +++++- .../ArgoCD/Git/GitConnectionFactory.cs | 27 ++++------ .../CommitToGit/CommitToGitConfigFactory.cs | 23 +++++--- 6 files changed, 121 insertions(+), 67 deletions(-) diff --git a/source/Calamari.Tests/ArgoCD/Git/AuthenticatingRepositoryFactoryTests.cs b/source/Calamari.Tests/ArgoCD/Git/AuthenticatingRepositoryFactoryTests.cs index 0eccc1f24a..0a95fbc9b5 100644 --- a/source/Calamari.Tests/ArgoCD/Git/AuthenticatingRepositoryFactoryTests.cs +++ b/source/Calamari.Tests/ArgoCD/Git/AuthenticatingRepositoryFactoryTests.cs @@ -2,6 +2,7 @@ using System.IO; using Calamari.ArgoCD.Git; using Calamari.ArgoCD.Git.PullRequests; +using Calamari.Common.Commands; using Calamari.Common.Plumbing.FileSystem; using Calamari.Integration.Time; using Calamari.Testing.Helpers; @@ -58,7 +59,7 @@ [new GitCredentialDto(httpsUrl, "", "")], repositoryFactory, log); - using var wrapper = factory.CloneRepository(httpsUrl, branchName.ToFriendlyName(), createsPr: false); + using var wrapper = factory.CloneRepository(httpsUrl, branchName.ToFriendlyName(), requiresPullRequest: false); wrapper.Should().NotBeNull(); } @@ -71,7 +72,7 @@ public void AnonymousCloneWhenNoCredentialsMatch() repositoryFactory, log); - using var wrapper = factory.CloneRepository(originUrl, branchName.ToFriendlyName(), createsPr: false); + using var wrapper = factory.CloneRepository(originUrl, branchName.ToFriendlyName(), requiresPullRequest: false); wrapper.Should().NotBeNull(); log.Messages.Should().Contain(m => m.FormattedMessage.Contains("No Git credentials found")); } @@ -82,12 +83,23 @@ public void LogsAnInformationalMessageWhenCloningAnonymously() var mockRepoFactory = Substitute.For(); var factory = new AuthenticatingRepositoryFactory([], mockRepoFactory, log); - factory.CloneRepository("https://github.com/org/repo.git", "main", createsPr: false); + factory.CloneRepository("https://github.com/org/repo.git", "main", requiresPullRequest: false); mockRepoFactory.Received() .CloneRepository(Arg.Any(), Arg.Is(c => c is AnonymousGitConnection)); log.Messages.Should().Contain(m => m.FormattedMessage.Contains("No Git credentials found")); } + + [Test] + public void ThrowsWhenCreatingAPullRequestWithoutCredentials() + { + var mockRepoFactory = Substitute.For(); + var factory = new AuthenticatingRepositoryFactory([], mockRepoFactory, log); + + var act = () => factory.CloneRepository("https://github.com/org/repo.git", "main", requiresPullRequest: true); + + act.Should().Throw().And.Message.Should().Contain("requires Git repository credentials"); + } } [TestFixture] @@ -108,12 +120,28 @@ [new SshKeyGitCredentialDto(sshUrl, "git", "private-key", [])], mockRepoFactory, log); - factory.CloneRepository(sshUrl, branchName.ToFriendlyName(), createsPr: false); + factory.CloneRepository(sshUrl, branchName.ToFriendlyName(), requiresPullRequest: false); mockRepoFactory.Received() .CloneRepository(Arg.Any(), Arg.Is(c => c is SshKeyGitConnection)); } + [Test] + public void ThrowsWhenCreatingAPullRequestWithAnSshKeyCredential() + { + const string sshUrl = "ssh://git@github.com/org/repo.git"; + var mockRepoFactory = Substitute.For(); + + var factory = new AuthenticatingRepositoryFactory( + [new SshKeyGitCredentialDto(sshUrl, "git", "private-key", [])], + mockRepoFactory, + log); + + var act = () => factory.CloneRepository(sshUrl, branchName.ToFriendlyName(), requiresPullRequest: true); + + act.Should().Throw().And.Message.Should().Contain("SSH key authentication"); + } + [Test] public void HttpsCredentialTakesPriorityOverSshWhenBothMatchAnSshUrl() { @@ -138,7 +166,7 @@ [new SshKeyGitCredentialDto(sshUrl, "git", "private-key", knownHosts)], mockRepoFactory, log); - factory.CloneRepository(sshUrl, branchName.ToFriendlyName(), createsPr: false); + factory.CloneRepository(sshUrl, branchName.ToFriendlyName(), requiresPullRequest: false); mockRepoFactory.Received() .CloneRepository( @@ -170,7 +198,7 @@ [new GitCredentialDto(httpsUrl, "user", "pass")], // This will fail to clone (no real repo at this URL) but we can verify it // falls through to anonymous because the SCP URL doesn't match the HTTPS URL - var act = () => factory.CloneRepository(scpUrl, "main", createsPr: false); + var act = () => factory.CloneRepository(scpUrl, "main", requiresPullRequest: false); act.Should().Throw(); // clone failure expected log.Messages.Should().Contain(m => m.FormattedMessage.Contains("No Git credentials found")); } @@ -195,7 +223,7 @@ protected void AssertHttpsCredentialTakesPriorityOverSsh(string url) var factory = new AuthenticatingRepositoryFactory(rawCredentials, mockRepoFactory, log); - factory.CloneRepository(url, "main", createsPr: false); + factory.CloneRepository(url, "main", requiresPullRequest: false); mockRepoFactory.Received() .CloneRepository( diff --git a/source/Calamari.Tests/ArgoCD/Git/GitConnectionFactoryTests.cs b/source/Calamari.Tests/ArgoCD/Git/GitConnectionFactoryTests.cs index 0ce25209d1..76c996757d 100644 --- a/source/Calamari.Tests/ArgoCD/Git/GitConnectionFactoryTests.cs +++ b/source/Calamari.Tests/ArgoCD/Git/GitConnectionFactoryTests.cs @@ -1,6 +1,4 @@ -using System; using Calamari.ArgoCD.Git; -using Calamari.Common.Commands; using FluentAssertions; using NUnit.Framework; using ArgoCdPasswordCredential = Octopus.Calamari.Contracts.ArgoCD.GitCredentialDto; @@ -18,62 +16,50 @@ public class GitConnectionFactoryTests const string HttpsUrl = "https://github.com/org/repo.git"; const string SshUrl = "ssh://git@github.com/org/repo.git"; - // The ArgoCD overload maps Argo credentials onto the common credential type and delegates to the - // overload below, so we only validate the mapping here; the behaviour is covered by the common tests. + // The factory only maps a credential onto the matching connection type, carrying its fields across. + // The HTTPS connection types expose a Lazy derived from Url, so it is excluded from the + // equivalence checks (Url itself is still asserted). + + // The ArgoCD overload maps Argo credentials onto the common credential type before delegating, so + // we confirm each Argo credential lands on the right connection type with its fields mapped across. [Test] public void ArgoCdSshKeyCredentialIsMappedToSshKeyGitConnection() { - var connection = GitConnectionFactory.Create(new ArgoCdSshKeyCredential(SshUrl, "git", "private-key", []), SshUrl, Reference, createsPr: false); + var connection = GitConnectionFactory.Create(new ArgoCdSshKeyCredential(SshUrl, "git", "private-key", []), SshUrl, Reference); - connection.Should().BeOfType(); + connection.Should().BeEquivalentTo(new SshKeyGitConnection("git", "private-key", SshUrl, Reference, [])); } [Test] public void ArgoCdUsernamePasswordCredentialIsMappedToUsernamePasswordGitConnection() { - var connection = GitConnectionFactory.Create(new ArgoCdPasswordCredential(HttpsUrl, "user", "password"), HttpsUrl, Reference, createsPr: false); - - connection.Should().BeOfType(); - } - - [Test] - public void SshKeyCredentialThrowsWhenCreatingAPullRequest() - { - Action action = () => GitConnectionFactory.Create(new GitSshKeyCredential("cred", SshUrl, "git", "private-key", []), SshUrl, Reference, createsPr: true); - - action.Should().Throw().And.Message.Should().Contain("SSH key authentication"); - } - - [Test] - public void SshKeyCredentialSucceedsWhenNotCreatingAPullRequest() - { - var connection = GitConnectionFactory.Create(new GitSshKeyCredential("cred", SshUrl, "git", "private-key", []), SshUrl, Reference, createsPr: false); + var connection = GitConnectionFactory.Create(new ArgoCdPasswordCredential(HttpsUrl, "user", "password"), HttpsUrl, Reference); - connection.Should().BeOfType(); + connection.Should().BeEquivalentTo(new UsernamePasswordGitConnection("user", "password", HttpsUrl, Reference), options => options.Excluding(c => c.Uri)); } [Test] - public void UsernamePasswordCredentialSucceedsWhenCreatingAPullRequest() + public void SshKeyCredentialIsMappedToSshKeyGitConnection() { - var connection = GitConnectionFactory.Create(new GitPasswordCredential("cred", HttpsUrl, "user", "password"), HttpsUrl, Reference, createsPr: true); + var connection = GitConnectionFactory.Create(new GitSshKeyCredential("cred", SshUrl, "git", "private-key", []), SshUrl, Reference); - connection.Should().BeOfType(); + connection.Should().BeEquivalentTo(new SshKeyGitConnection("git", "private-key", SshUrl, Reference, [])); } [Test] - public void NoCredentialThrowsWhenCreatingAPullRequest() + public void UsernamePasswordCredentialIsMappedToUsernamePasswordGitConnection() { - Action action = () => GitConnectionFactory.Create((GitCredential)null, HttpsUrl, Reference, createsPr: true); + var connection = GitConnectionFactory.Create(new GitPasswordCredential("cred", HttpsUrl, "user", "password"), HttpsUrl, Reference); - action.Should().Throw().And.Message.Should().Contain("requires Git repository credentials"); + connection.Should().BeEquivalentTo(new UsernamePasswordGitConnection("user", "password", HttpsUrl, Reference), options => options.Excluding(c => c.Uri)); } [Test] - public void NoCredentialIsAnonymousWhenNotCreatingAPullRequest() + public void NoCredentialIsMappedToAnonymousGitConnection() { - var connection = GitConnectionFactory.Create((GitCredential)null, HttpsUrl, Reference, createsPr: false); + var connection = GitConnectionFactory.Create((GitCredential)null, HttpsUrl, Reference); - connection.Should().BeOfType(); + connection.Should().BeEquivalentTo(new AnonymousGitConnection(HttpsUrl, Reference), options => options.Excluding(c => c.Uri)); } } diff --git a/source/Calamari.Tests/CommitToGit/CommitToGitConfigFactoryTests.cs b/source/Calamari.Tests/CommitToGit/CommitToGitConfigFactoryTests.cs index 86a525badb..9aac6d9b37 100644 --- a/source/Calamari.Tests/CommitToGit/CommitToGitConfigFactoryTests.cs +++ b/source/Calamari.Tests/CommitToGit/CommitToGitConfigFactoryTests.cs @@ -53,6 +53,34 @@ public void CreateRepositoryConfig_UsesUsernameAndPasswordFromLoadedProperties() httpsGitConnection.Uri.Value.Should().Be(new Uri("https://example.invalid/repo.git")); } + [Test] + public void CreateRepositoryConfig_WhenCreatingPullRequestWithSshKeyCredential_Throws() + { + variables.GetFlag(SpecialVariables.Action.Git.PullRequest.Create).Returns(true); + loader.Load() + .Returns(new CommitToGitCustomPropertiesDto(new SshKeyGitCredentialDto("MyCred", "https://example.invalid/repo.git", "git", "private-key", []))); + + var deployment = new RunningDeployment(null, variables); + + var act = () => factory.CreateRepositoryConfig(deployment, loader); + + act.Should().Throw().And.Message.Should().Contain("SSH key authentication"); + } + + [Test] + public void CreateRepositoryConfig_WhenCreatingPullRequestWithoutCredentials_Throws() + { + variables.GetFlag(SpecialVariables.Action.Git.PullRequest.Create).Returns(true); + loader.Load() + .Returns(new CommitToGitCustomPropertiesDto(null)); + + var deployment = new RunningDeployment(null, variables); + + var act = () => factory.CreateRepositoryConfig(deployment, loader); + + act.Should().Throw().And.Message.Should().Contain("requires Git repository credentials"); + } + [Test] public void CreateRepositoryConfig_WhenDestinationPathIsMissing_DefaultsToEmptyString() { diff --git a/source/Calamari/ArgoCD/Git/AuthenticatingRepositoryFactory.cs b/source/Calamari/ArgoCD/Git/AuthenticatingRepositoryFactory.cs index 65fb9c7a3b..647315ccf4 100644 --- a/source/Calamari/ArgoCD/Git/AuthenticatingRepositoryFactory.cs +++ b/source/Calamari/ArgoCD/Git/AuthenticatingRepositoryFactory.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Linq; +using Calamari.Common.Commands; using Calamari.Common.Plumbing.Logging; using Octopus.Calamari.Contracts.ArgoCD; @@ -26,10 +27,21 @@ public AuthenticatingRepositoryFactory( this.log = log; } - public RepositoryWrapper CloneRepository(string requestedUrl, string targetRevision, bool createsPr) + public RepositoryWrapper CloneRepository(string requestedUrl, string targetRevision, bool requiresPullRequest) { var gitCredential = gitCredentials.GetValueOrDefault(requestedUrl); - var gitConnection = GitConnectionFactory.Create(gitCredential, requestedUrl, GitReference.CreateFromString(targetRevision), createsPr); + var gitConnection = GitConnectionFactory.Create(gitCredential, requestedUrl, GitReference.CreateFromString(targetRevision)); + + if (requiresPullRequest) + { + switch (gitConnection) + { + case SshKeyGitConnection: + throw new CommandException("Creating PRs is not possible when using SSH key authentication, please use a username and password instead"); + case AnonymousGitConnection: + throw new CommandException("Creating a pull request requires Git repository credentials, but none were provided. Please configure a username and password."); + } + } if (gitConnection is AnonymousGitConnection) { diff --git a/source/Calamari/ArgoCD/Git/GitConnectionFactory.cs b/source/Calamari/ArgoCD/Git/GitConnectionFactory.cs index f51ab501e7..d78b926761 100644 --- a/source/Calamari/ArgoCD/Git/GitConnectionFactory.cs +++ b/source/Calamari/ArgoCD/Git/GitConnectionFactory.cs @@ -2,7 +2,6 @@ using System; using System.Linq; -using Calamari.Common.Commands; using Octopus.Calamari.Contracts.Git; using ArgoCdIGitCredentialDto = Octopus.Calamari.Contracts.ArgoCD.IGitCredentialDto; using ArgoCdGitCredentialDto = Octopus.Calamari.Contracts.ArgoCD.GitCredentialDto; @@ -14,15 +13,15 @@ public static class GitConnectionFactory { // Converts Argo credentials into the common type // ideally we migrate the Argo code path to use the common type from server and we can delete this - public static IGitConnection Create(ArgoCdIGitCredentialDto? credential, string url, GitReference gitReference, bool createsPr) + public static IGitConnection Create(ArgoCdIGitCredentialDto? credential, string url, GitReference gitReference) { return credential switch { ArgoCdGitCredentialDto password => Create( new UsernamePasswordGitCredentialDto(string.Empty, password.Url, password.Username, password.Password), url, - gitReference, - createsPr), + gitReference + ), ArgoCdSshKeyGitCredentialDto ssh => Create( new SshKeyGitCredentialDto( string.Empty, @@ -31,35 +30,27 @@ public static IGitConnection Create(ArgoCdIGitCredentialDto? credential, string ssh.PrivateKey, ssh.KnownHosts.Select(kh => new SshKnownHostDto(kh.Host, kh.PublicKey)).ToArray()), url, - gitReference, - createsPr), - _ => Create((IGitCredentialDto?)null, url, gitReference, createsPr), + gitReference + ), + _ => Create((IGitCredentialDto?)null, url, gitReference), }; } - public static IGitConnection Create(IGitCredentialDto? credential, string url, GitReference gitReference, bool createsPr) + public static IGitConnection Create(IGitCredentialDto? credential, string url, GitReference gitReference) { return credential switch { UsernamePasswordGitCredentialDto password => new UsernamePasswordGitConnection(password.Username, password.Password, url, gitReference), - - SshKeyGitCredentialDto when createsPr - => throw new CommandException("Creating PRs is not possible when using SSH key authentication, please use a username and password instead"), - SshKeyGitCredentialDto ssh => new SshKeyGitConnection( ssh.Username, ssh.PrivateKey, url, gitReference, - ssh.KnownHosts.Select(kh => new SshKnownHost(kh.Host, kh.PublicKey)).ToArray()), - - null when createsPr - => throw new CommandException("Creating a pull request requires Git repository credentials, but none were provided. Please configure a username and password."), - + ssh.KnownHosts.Select(kh => new SshKnownHost(kh.Host, kh.PublicKey)).ToArray() + ), null => new AnonymousGitConnection(url, gitReference), - _ => throw new NotSupportedException($"An unrecognised credential type '{credential.GetType().Name}' was found for '{url}'") }; } diff --git a/source/Calamari/CommitToGit/CommitToGitConfigFactory.cs b/source/Calamari/CommitToGit/CommitToGitConfigFactory.cs index eb1a9fc36a..f55e5e9b91 100644 --- a/source/Calamari/CommitToGit/CommitToGitConfigFactory.cs +++ b/source/Calamari/CommitToGit/CommitToGitConfigFactory.cs @@ -1,13 +1,10 @@ using System; -using System.Linq; -using Amazon.ECS.Model; using Calamari.ArgoCD.Conventions; using Calamari.ArgoCD.Git; using Calamari.Common.Commands; using Calamari.Common.Plumbing.Variables; using Calamari.Deployment; using Octopus.Calamari.Contracts.CommitToGit; -using Octopus.Calamari.Contracts.Git; namespace Calamari.CommitToGit { @@ -37,12 +34,24 @@ public CommitToGitRepositorySettings CreateRepositoryConfig(RunningDeployment de var properties = customPropertiesLoader.Load(); - var connection = GitConnectionFactory.Create(properties.GitCredential, uriAsString, GitReference.CreateFromString(gitReferenceAsString), requiresPullRequest); + var connection = GitConnectionFactory.Create(properties.GitCredential, uriAsString, GitReference.CreateFromString(gitReferenceAsString)); + + if (requiresPullRequest) + { + switch (connection) + { + case SshKeyGitConnection: + throw new CommandException("Creating PRs is not possible when using SSH key authentication, please use a username and password instead"); + case AnonymousGitConnection: + throw new CommandException("Creating a pull request requires Git repository credentials, but none were provided. Please configure a username and password."); + } + } //Note: Octopus server removes variables containing empty strings, thus a missing property should default to an empty string. - return new CommitToGitRepositorySettings(connection, - commitParameters, - variables.Get(SpecialVariables.Action.Git.DestinationPath) ?? string.Empty); + return new CommitToGitRepositorySettings( + connection, + commitParameters, + variables.Get(SpecialVariables.Action.Git.DestinationPath) ?? string.Empty); } string EvaluateNonsensitiveExpression(string expression) From 4bda1a6c6799f7c84f5acd29dbb90b6747269abc Mon Sep 17 00:00:00 2001 From: Eddy Moulton Date: Wed, 3 Jun 2026 16:30:23 +1000 Subject: [PATCH 3/3] review --- .../AuthenticatingRepositoryFactoryTests.cs | 24 +++++++++++++++++++ .../ArgoCD/Git/GitConnectionFactoryTests.cs | 23 +++++++++++------- .../Git/AuthenticatingRepositoryFactory.cs | 5 ++-- .../ArgoCD/Git/GitConnectionFactory.cs | 10 +++++--- .../Calamari/ArgoCD/Git/RepositoryFactory.cs | 2 +- .../CommitToGit/CommitToGitConfigFactory.cs | 2 +- 6 files changed, 50 insertions(+), 16 deletions(-) diff --git a/source/Calamari.Tests/ArgoCD/Git/AuthenticatingRepositoryFactoryTests.cs b/source/Calamari.Tests/ArgoCD/Git/AuthenticatingRepositoryFactoryTests.cs index 0a95fbc9b5..3c06b56d57 100644 --- a/source/Calamari.Tests/ArgoCD/Git/AuthenticatingRepositoryFactoryTests.cs +++ b/source/Calamari.Tests/ArgoCD/Git/AuthenticatingRepositoryFactoryTests.cs @@ -230,4 +230,28 @@ protected void AssertHttpsCredentialTakesPriorityOverSsh(string url) Arg.Any(), Arg.Is(c => c is HttpsGitConnection)); } + + [TestFixture] + public class UrlNormalizationTests : AuthenticatingRepositoryFactoryTestBase + { + [Test] + public void SchemelessUrlIsNormalizedWithOciSchemeForNonSshCredential() + { + const string schemelessUrl = "registry.example.com/charts/myapp"; + const string expectedNormalizedUrl = "oci://registry.example.com/charts/myapp"; + + var mockRepoFactory = Substitute.For(); + var factory = new AuthenticatingRepositoryFactory( + [new GitCredentialDto(schemelessUrl, "user", "password")], + mockRepoFactory, + log); + + factory.CloneRepository(schemelessUrl, "main", requiresPullRequest: false); + + mockRepoFactory.Received() + .CloneRepository( + Arg.Any(), + Arg.Is(c => c.Url == expectedNormalizedUrl)); + } + } } \ No newline at end of file diff --git a/source/Calamari.Tests/ArgoCD/Git/GitConnectionFactoryTests.cs b/source/Calamari.Tests/ArgoCD/Git/GitConnectionFactoryTests.cs index 76c996757d..b8b1418f56 100644 --- a/source/Calamari.Tests/ArgoCD/Git/GitConnectionFactoryTests.cs +++ b/source/Calamari.Tests/ArgoCD/Git/GitConnectionFactoryTests.cs @@ -6,6 +6,7 @@ using GitCredential = Octopus.Calamari.Contracts.Git.IGitCredentialDto; using GitPasswordCredential = Octopus.Calamari.Contracts.Git.UsernamePasswordGitCredentialDto; using GitSshKeyCredential = Octopus.Calamari.Contracts.Git.SshKeyGitCredentialDto; +using GitSshKnownHostDto = Octopus.Calamari.Contracts.Git.SshKnownHostDto; namespace Calamari.Tests.ArgoCD.Git; @@ -16,13 +17,6 @@ public class GitConnectionFactoryTests const string HttpsUrl = "https://github.com/org/repo.git"; const string SshUrl = "ssh://git@github.com/org/repo.git"; - // The factory only maps a credential onto the matching connection type, carrying its fields across. - // The HTTPS connection types expose a Lazy derived from Url, so it is excluded from the - // equivalence checks (Url itself is still asserted). - - // The ArgoCD overload maps Argo credentials onto the common credential type before delegating, so - // we confirm each Argo credential lands on the right connection type with its fields mapped across. - [Test] public void ArgoCdSshKeyCredentialIsMappedToSshKeyGitConnection() { @@ -42,9 +36,20 @@ public void ArgoCdUsernamePasswordCredentialIsMappedToUsernamePasswordGitConnect [Test] public void SshKeyCredentialIsMappedToSshKeyGitConnection() { - var connection = GitConnectionFactory.Create(new GitSshKeyCredential("cred", SshUrl, "git", "private-key", []), SshUrl, Reference); + GitSshKnownHostDto[] knownHosts = + [ + new ("github.com", "AAAAB3NzaC1yc2EAAAADAQABAAABAQ=="), + new ("bitbucket.org", "AAAAC3NzaC1lZDI1NTE5AAAAIA=="), + ]; + var connection = GitConnectionFactory.Create(new GitSshKeyCredential("cred", SshUrl, "git", "private-key", knownHosts), SshUrl, Reference); - connection.Should().BeEquivalentTo(new SshKeyGitConnection("git", "private-key", SshUrl, Reference, [])); + + SshKnownHost[] expectedKnownHosts = + [ + new("github.com", "AAAAB3NzaC1yc2EAAAADAQABAAABAQ"), + new("bitbucket.org", "AAAAC3NzaC1lZDI1NTE5AAAAIA=="), + ]; + connection.Should().BeEquivalentTo(new SshKeyGitConnection("git", "private-key", SshUrl, Reference, expectedKnownHosts)); } [Test] diff --git a/source/Calamari/ArgoCD/Git/AuthenticatingRepositoryFactory.cs b/source/Calamari/ArgoCD/Git/AuthenticatingRepositoryFactory.cs index 647315ccf4..022b299fc1 100644 --- a/source/Calamari/ArgoCD/Git/AuthenticatingRepositoryFactory.cs +++ b/source/Calamari/ArgoCD/Git/AuthenticatingRepositoryFactory.cs @@ -30,7 +30,8 @@ public AuthenticatingRepositoryFactory( public RepositoryWrapper CloneRepository(string requestedUrl, string targetRevision, bool requiresPullRequest) { var gitCredential = gitCredentials.GetValueOrDefault(requestedUrl); - var gitConnection = GitConnectionFactory.Create(gitCredential, requestedUrl, GitReference.CreateFromString(targetRevision)); + var cloneUrl = gitCredential is SshKeyGitCredentialDto ? requestedUrl : GitCloneSafeUrl.ConvertToUriString(requestedUrl); + var gitConnection = GitConnectionFactory.Create(gitCredential, cloneUrl, GitReference.CreateFromString(targetRevision)); if (requiresPullRequest) { @@ -50,4 +51,4 @@ public RepositoryWrapper CloneRepository(string requestedUrl, string targetRevis return repositoryFactory.CloneRepository(UniqueRepoNameGenerator.Generate(), gitConnection); } -} \ No newline at end of file +} diff --git a/source/Calamari/ArgoCD/Git/GitConnectionFactory.cs b/source/Calamari/ArgoCD/Git/GitConnectionFactory.cs index d78b926761..4f9820c2c6 100644 --- a/source/Calamari/ArgoCD/Git/GitConnectionFactory.cs +++ b/source/Calamari/ArgoCD/Git/GitConnectionFactory.cs @@ -2,6 +2,7 @@ using System; using System.Linq; +using Calamari.Common.Commands; using Octopus.Calamari.Contracts.Git; using ArgoCdIGitCredentialDto = Octopus.Calamari.Contracts.ArgoCD.IGitCredentialDto; using ArgoCdGitCredentialDto = Octopus.Calamari.Contracts.ArgoCD.GitCredentialDto; @@ -18,12 +19,14 @@ public static IGitConnection Create(ArgoCdIGitCredentialDto? credential, string return credential switch { ArgoCdGitCredentialDto password => Create( + // ArgoCD credentials don't pass a name new UsernamePasswordGitCredentialDto(string.Empty, password.Url, password.Username, password.Password), url, gitReference ), ArgoCdSshKeyGitCredentialDto ssh => Create( new SshKeyGitCredentialDto( + // ArgoCD credentials don't pass a name string.Empty, ssh.Url, ssh.Username, @@ -32,7 +35,8 @@ public static IGitConnection Create(ArgoCdIGitCredentialDto? credential, string url, gitReference ), - _ => Create((IGitCredentialDto?)null, url, gitReference), + null => Create((IGitCredentialDto?)null, url, gitReference), + _ => throw new CommandException($"An unrecognised credential type '{credential.GetType().Name}' was found for '{url}'"), }; } @@ -51,7 +55,7 @@ SshKeyGitCredentialDto ssh ssh.KnownHosts.Select(kh => new SshKnownHost(kh.Host, kh.PublicKey)).ToArray() ), null => new AnonymousGitConnection(url, gitReference), - _ => throw new NotSupportedException($"An unrecognised credential type '{credential.GetType().Name}' was found for '{url}'") + _ => throw new CommandException($"An unrecognised credential type '{credential.GetType().Name}' was found for '{url}'") }; } -} \ No newline at end of file +} diff --git a/source/Calamari/ArgoCD/Git/RepositoryFactory.cs b/source/Calamari/ArgoCD/Git/RepositoryFactory.cs index 771c9b83b0..cec0593494 100644 --- a/source/Calamari/ArgoCD/Git/RepositoryFactory.cs +++ b/source/Calamari/ArgoCD/Git/RepositoryFactory.cs @@ -114,7 +114,7 @@ RepositoryWrapper CheckoutGitRepository(IGitConnection gitConnection, string che } //TODO(tmm): Make this function (and all callers async). - var gitVendorApiAdapter = gitConnection is HttpsGitConnection httpsGitConnection + var gitVendorApiAdapter = gitConnection is UsernamePasswordGitConnection httpsGitConnection ? gitVendorPullRequestClientResolver.TryResolve(httpsGitConnection, log, CancellationToken.None).Result : null; diff --git a/source/Calamari/CommitToGit/CommitToGitConfigFactory.cs b/source/Calamari/CommitToGit/CommitToGitConfigFactory.cs index f55e5e9b91..018bbfa7ae 100644 --- a/source/Calamari/CommitToGit/CommitToGitConfigFactory.cs +++ b/source/Calamari/CommitToGit/CommitToGitConfigFactory.cs @@ -67,4 +67,4 @@ string EvaluateNonsensitiveExpression(string expression) return result; } } -} \ No newline at end of file +}