diff --git a/GitLfsCache.Tests/Integration/LockListCachingTests.cs b/GitLfsCache.Tests/Integration/LockListCachingTests.cs index a4987d8..f1e87c5 100644 --- a/GitLfsCache.Tests/Integration/LockListCachingTests.cs +++ b/GitLfsCache.Tests/Integration/LockListCachingTests.cs @@ -215,6 +215,41 @@ public async Task CreatingALock_InvalidatesTheSnapshot() Assert.HasCount(2, (await ListLocksAsync(fixture))["locks"]!.AsArray()); } + [TestMethod] + [DataRow("?refspec=refs/heads/main", "refspec=refs%2Fheads%2Fmain", DisplayName = "With a refspec")] + [DataRow("", null, DisplayName = "Without a refspec")] + public async Task CachedListing_ForwardsTheClientsRefspecOnTheWalkAndTheProbe(string query, string? forwarded) + { + // The snapshot is keyed by ref because upstream may answer differently per ref, so what fills it + // and what admits a caller to it have to ask upstream about that same ref. + await using ProxyFixture fixture = await ProxyFixture.StartAsync(); + fixture.Upstream.Locks.Add("a"); + + await ListLocksAsync(fixture, query, Credential); + await ListLocksAsync(fixture, query, OtherCredential); + + StubUpstream.RecordedRequest[] listings = + [ + .. fixture.Upstream.Requests.Where(request => request.Path.EndsWith("/locks", StringComparison.Ordinal)), + ]; + + // The first caller's walk and the second caller's probe. + Assert.HasCount(2, listings); + Assert.Contains("limit=1", listings[1].Query, "The second request should be the one-page probe."); + + foreach (StubUpstream.RecordedRequest listing in listings) + { + if (forwarded is null) + { + Assert.DoesNotContain("refspec", listing.Query); + } + else + { + Assert.Contains(forwarded, listing.Query); + } + } + } + [TestMethod] [DataRow("/locks", "{\"path\":\"b\",\"ref\":{\"name\":\"refs/heads/main\"}}", 2, DisplayName = "Create")] [DataRow("/locks/1/unlock", "{\"ref\":{\"name\":\"refs/heads/main\"}}", 0, DisplayName = "Unlock")] diff --git a/GitLfsCache.Tests/Integration/StubUpstream.cs b/GitLfsCache.Tests/Integration/StubUpstream.cs index 6f3562f..4bcb8e6 100644 --- a/GitLfsCache.Tests/Integration/StubUpstream.cs +++ b/GitLfsCache.Tests/Integration/StubUpstream.cs @@ -91,7 +91,8 @@ protected override async Task SendAsync( request.Headers.TryGetValues("Authorization", out IEnumerable? authorization) ? string.Join(",", authorization) : null, - request.Headers.Range?.ToString())); + request.Headers.Range?.ToString(), + request.RequestUri!.Query)); } if (path.EndsWith("/unlock", StringComparison.Ordinal)) @@ -431,5 +432,6 @@ private HttpResponseMessage BuildUploadResponse( /// The absolute path. /// The Authorization header, or null when absent. /// The Range header, or null when absent. - internal sealed record RecordedRequest(string Method, string Path, string? Authorization, string? Range); + /// The query string, including its leading ?, or empty when absent. + internal sealed record RecordedRequest(string Method, string Path, string? Authorization, string? Range, string Query); } diff --git a/GitLfsCache.Tests/Locks/CredentialAdmissionTests.cs b/GitLfsCache.Tests/Locks/CredentialAdmissionTests.cs index 403fb7d..552b108 100644 --- a/GitLfsCache.Tests/Locks/CredentialAdmissionTests.cs +++ b/GitLfsCache.Tests/Locks/CredentialAdmissionTests.cs @@ -33,7 +33,7 @@ public void IsAdmitted_BeforeAnyUpstreamSuccess_IsFalse() // The whole point: nothing is admitted until upstream actually said yes. (CredentialAdmission admission, _) = Build(); - Assert.IsFalse(admission.IsAdmitted("github", Repository, Credential)); + Assert.IsFalse(admission.IsAdmitted("github", Repository, null, Credential)); } [TestMethod] @@ -41,9 +41,9 @@ public void IsAdmitted_AfterAdmit_IsTrue() { (CredentialAdmission admission, _) = Build(); - admission.Admit("github", Repository, Credential); + admission.Admit("github", Repository, null, Credential); - Assert.IsTrue(admission.IsAdmitted("github", Repository, Credential)); + Assert.IsTrue(admission.IsAdmitted("github", Repository, null, Credential)); } [TestMethod] @@ -51,12 +51,12 @@ public void IsAdmitted_AfterTheTtl_IsFalseAgain() { (CredentialAdmission admission, FakeTimeProvider time) = Build(TimeSpan.FromMinutes(1)); - admission.Admit("github", Repository, Credential); + admission.Admit("github", Repository, null, Credential); time.Advance(TimeSpan.FromMinutes(1)); // This is the window in which a credential revoked upstream still reads listings. It has to // actually close. - Assert.IsFalse(admission.IsAdmitted("github", Repository, Credential)); + Assert.IsFalse(admission.IsAdmitted("github", Repository, null, Credential)); } [TestMethod] @@ -64,10 +64,10 @@ public void IsAdmitted_JustBeforeTheTtl_IsStillTrue() { (CredentialAdmission admission, FakeTimeProvider time) = Build(TimeSpan.FromMinutes(1)); - admission.Admit("github", Repository, Credential); + admission.Admit("github", Repository, null, Credential); time.Advance(TimeSpan.FromSeconds(59)); - Assert.IsTrue(admission.IsAdmitted("github", Repository, Credential)); + Assert.IsTrue(admission.IsAdmitted("github", Repository, null, Credential)); } [TestMethod] @@ -75,9 +75,9 @@ public void IsAdmitted_ADifferentCredential_IsNotAdmitted() { (CredentialAdmission admission, _) = Build(); - admission.Admit("github", Repository, Credential); + admission.Admit("github", Repository, null, Credential); - Assert.IsFalse(admission.IsAdmitted("github", Repository, "Basic c29tZW9uZTplbHNl")); + Assert.IsFalse(admission.IsAdmitted("github", Repository, null, "Basic c29tZW9uZTplbHNl")); } [TestMethod] @@ -86,9 +86,9 @@ public void IsAdmitted_ADifferentRepository_IsNotAdmitted() // Admission is per repository. Read access to one proves nothing about another. (CredentialAdmission admission, _) = Build(); - admission.Admit("github", Repository, Credential); + admission.Admit("github", Repository, null, Credential); - Assert.IsFalse(admission.IsAdmitted("github", "owner/other.git/info/lfs", Credential)); + Assert.IsFalse(admission.IsAdmitted("github", "owner/other.git/info/lfs", null, Credential)); } [TestMethod] @@ -96,9 +96,25 @@ public void IsAdmitted_ADifferentUpstream_IsNotAdmitted() { (CredentialAdmission admission, _) = Build(); - admission.Admit("github", Repository, Credential); + admission.Admit("github", Repository, null, Credential); - Assert.IsFalse(admission.IsAdmitted("ado", Repository, Credential)); + Assert.IsFalse(admission.IsAdmitted("ado", Repository, null, Credential)); + } + + [TestMethod] + [DataRow("refs/heads/feature", DisplayName = "Another ref")] + [DataRow(null, DisplayName = "No ref")] + [DataRow("", DisplayName = "An empty ref")] + public void IsAdmitted_ADifferentRef_IsNotAdmitted(string? reference) + { + // The locking API treats the ref as an authentication input, so upstream accepting a + // credential under one ref says nothing about another. + (CredentialAdmission admission, _) = Build(); + + admission.Admit("github", Repository, "refs/heads/main", Credential); + + Assert.IsTrue(admission.IsAdmitted("github", Repository, "refs/heads/main", Credential)); + Assert.IsFalse(admission.IsAdmitted("github", Repository, reference, Credential)); } [TestMethod] @@ -108,9 +124,9 @@ public void Key_CannotBeConfusedAcrossFields() // "b" would hash identically and one repository's admission would serve another's. (CredentialAdmission admission, _) = Build(); - admission.Admit("github", "a/b", Credential); + admission.Admit("github", "a/b", null, Credential); - Assert.IsFalse(admission.IsAdmitted("github/a", "b", Credential)); + Assert.IsFalse(admission.IsAdmitted("github/a", "b", null, Credential)); } [TestMethod] @@ -121,9 +137,9 @@ public void IsAdmitted_NoCredential_IsNeverAdmitted(string? authorization) // Admitting an anonymous caller would mean serving a listing to someone who proved nothing. (CredentialAdmission admission, _) = Build(); - admission.Admit("github", Repository, authorization); + admission.Admit("github", Repository, null, authorization); - Assert.IsFalse(admission.IsAdmitted("github", Repository, authorization)); + Assert.IsFalse(admission.IsAdmitted("github", Repository, null, authorization)); } [TestMethod] @@ -131,11 +147,11 @@ public void Admit_Twice_ExtendsTheWindowFromTheSecondTime() { (CredentialAdmission admission, FakeTimeProvider time) = Build(TimeSpan.FromMinutes(1)); - admission.Admit("github", Repository, Credential); + admission.Admit("github", Repository, null, Credential); time.Advance(TimeSpan.FromSeconds(50)); - admission.Admit("github", Repository, Credential); + admission.Admit("github", Repository, null, Credential); time.Advance(TimeSpan.FromSeconds(50)); - Assert.IsTrue(admission.IsAdmitted("github", Repository, Credential)); + Assert.IsTrue(admission.IsAdmitted("github", Repository, null, Credential)); } } diff --git a/GitLfsCache/Locks/CredentialAdmission.cs b/GitLfsCache/Locks/CredentialAdmission.cs index 3b6468b..f18a65a 100644 --- a/GitLfsCache/Locks/CredentialAdmission.cs +++ b/GitLfsCache/Locks/CredentialAdmission.cs @@ -3,6 +3,7 @@ namespace ktsu.GitLfsCache.Locks; using System.Collections.Concurrent; +using System.Globalization; using System.Security.Cryptography; using System.Text; using ktsu.GitLfsCache.Configuration; @@ -42,7 +43,7 @@ public sealed class CredentialAdmission( private readonly Lock _hashGate = new(); /// - public bool IsAdmitted(string upstream, string repositoryPath, string? authorization) + public bool IsAdmitted(string upstream, string repositoryPath, string? reference, string? authorization) { // An anonymous caller is never admitted. Upstream would refuse it, and admitting it here would // mean a listing served to someone who never proved anything. @@ -51,7 +52,7 @@ public bool IsAdmitted(string upstream, string repositoryPath, string? authoriza return false; } - string key = Key(upstream, repositoryPath, authorization); + string key = Key(upstream, repositoryPath, reference, authorization); if (!_admitted.TryGetValue(key, out DateTimeOffset expiry)) { @@ -70,14 +71,14 @@ public bool IsAdmitted(string upstream, string repositoryPath, string? authoriza } /// - public void Admit(string upstream, string repositoryPath, string? authorization) + public void Admit(string upstream, string repositoryPath, string? reference, string? authorization) { if (string.IsNullOrEmpty(authorization)) { return; } - _admitted[Key(upstream, repositoryPath, authorization)] = + _admitted[Key(upstream, repositoryPath, reference, authorization)] = timeProvider.GetUtcNow() + options.Value.Locks.AdmissionTtl; if (_admitted.Count > SweepThreshold) @@ -106,13 +107,18 @@ private void Sweep() /// Derives the entry key for one credential and repository. /// /// - /// The three parts are separated by a character that cannot appear in an upstream key, so no two - /// different triples can produce the same input. Without that, an upstream and repository could be + /// The parts are separated by a character that cannot appear in an upstream key, so no two + /// different combinations can produce the same input. Without that, an upstream and repository could be /// re-split to match a different pair. /// - private string Key(string upstream, string repositoryPath, string authorization) + private string Key(string upstream, string repositoryPath, string? reference, string authorization) { - byte[] input = Encoding.UTF8.GetBytes($"{upstream}\n{repositoryPath}\n{authorization}"); + // The ref goes last and length-prefixed, since unlike the other parts it comes straight from a + // query string and may contain the separator. -1 keeps "no ref" apart from an empty one. + string scope = reference is null + ? "-1:" + : string.Create(CultureInfo.InvariantCulture, $"{reference.Length}:{reference}"); + byte[] input = Encoding.UTF8.GetBytes($"{upstream}\n{repositoryPath}\n{authorization}\n{scope}"); // HMACSHA256 holds mutable state across ComputeHash, so one shared instance needs a gate. The // alternative, an instance per call, allocates on a path taken on every lock request. diff --git a/GitLfsCache/Locks/ICredentialAdmission.cs b/GitLfsCache/Locks/ICredentialAdmission.cs index 0e66b58..e8ddd28 100644 --- a/GitLfsCache/Locks/ICredentialAdmission.cs +++ b/GitLfsCache/Locks/ICredentialAdmission.cs @@ -23,9 +23,13 @@ public interface ICredentialAdmission /// /// The upstream key. /// The repository the credential was accepted for. + /// + /// The ref the credential was presented with, or null for none. Upstream may accept a credential + /// for one ref and refuse it for another, so an admission for one does not carry over. + /// /// The client's Authorization header, exactly as sent. /// when an unexpired admission exists. - public bool IsAdmitted(string upstream, string repositoryPath, string? authorization); + public bool IsAdmitted(string upstream, string repositoryPath, string? reference, string? authorization); /// /// Records that upstream accepted this credential for this repository. @@ -36,6 +40,10 @@ public interface ICredentialAdmission /// /// The upstream key. /// The repository the credential was accepted for. + /// + /// The ref the credential was presented with, or null for none. Upstream may accept a credential + /// for one ref and refuse it for another, so an admission for one does not carry over. + /// /// The client's Authorization header, exactly as sent. - public void Admit(string upstream, string repositoryPath, string? authorization); + public void Admit(string upstream, string repositoryPath, string? reference, string? authorization); } diff --git a/GitLfsCache/Locks/LockListRefresher.cs b/GitLfsCache/Locks/LockListRefresher.cs index 5077d75..0567d3c 100644 --- a/GitLfsCache/Locks/LockListRefresher.cs +++ b/GitLfsCache/Locks/LockListRefresher.cs @@ -52,6 +52,7 @@ public async Task ProbeAsync( using HttpRequestMessage request = UpstreamRequests.BuildLockListRequest( upstreamBase, key.RepositoryPath, + key.Ref, cursor: null, limit: 1, authorization); @@ -83,6 +84,7 @@ public async Task RefreshAsync( using HttpRequestMessage request = UpstreamRequests.BuildLockListRequest( upstreamBase, key.RepositoryPath, + key.Ref, cursor, limit: null, authorization); diff --git a/GitLfsCache/Locks/LockListService.cs b/GitLfsCache/Locks/LockListService.cs index b287f85..8929ded 100644 --- a/GitLfsCache/Locks/LockListService.cs +++ b/GitLfsCache/Locks/LockListService.cs @@ -65,7 +65,7 @@ public async Task ResolveAsync( // A caller already admitted, with a snapshot still inside its lifetime, is the steady state and // costs upstream nothing at all. This is the whole point of the subsystem. - if (usable && admission.IsAdmitted(key.Upstream, key.RepositoryPath, authorization)) + if (usable && admission.IsAdmitted(key.Upstream, key.RepositoryPath, key.Ref, authorization)) { metrics.RecordLockListHit(key.Upstream); return LockListOutcome.Serve(current!); @@ -110,7 +110,7 @@ private async Task ProbeAsync( return LockListOutcome.Relay(); } - admission.Admit(key.Upstream, key.RepositoryPath, authorization); + admission.Admit(key.Upstream, key.RepositoryPath, key.Ref, authorization); return LockListOutcome.Serve(current); } @@ -155,7 +155,7 @@ private async Task RefreshAsync( snapshots.Publish(key, result.Snapshot!); // The walk succeeding is itself upstream's answer that this caller may read these locks. - admission.Admit(key.Upstream, key.RepositoryPath, authorization); + admission.Admit(key.Upstream, key.RepositoryPath, key.Ref, authorization); ticket.Complete(published: true); return LockListOutcome.Serve(result.Snapshot!); diff --git a/GitLfsCache/Upstreams/UpstreamRequests.cs b/GitLfsCache/Upstreams/UpstreamRequests.cs index 9d08680..944fc13 100644 --- a/GitLfsCache/Upstreams/UpstreamRequests.cs +++ b/GitLfsCache/Upstreams/UpstreamRequests.cs @@ -84,6 +84,11 @@ public static HttpRequestMessage BuildBatchRequest( /// /// The configured upstream base URL. /// The path between the upstream key and /locks. + /// + /// The ref the client listed under, forwarded as refspec, or null when it sent none. The + /// locking API treats the ref as an authentication input, so a walk or probe made on a client's + /// behalf has to present the same one the client did. + /// /// Upstream's cursor for the page to fetch, or null for the first. /// A page size to request, or null to let upstream choose. /// The client's Authorization header, forwarded unchanged. @@ -91,6 +96,7 @@ public static HttpRequestMessage BuildBatchRequest( public static HttpRequestMessage BuildLockListRequest( Uri upstreamBase, string repositoryPath, + string? refspec, string? cursor, int? limit, string? authorization) @@ -100,6 +106,11 @@ public static HttpRequestMessage BuildLockListRequest( List query = []; + if (refspec is not null) + { + query.Add($"refspec={Uri.EscapeDataString(refspec)}"); + } + if (!string.IsNullOrEmpty(cursor)) { query.Add($"cursor={Uri.EscapeDataString(cursor)}");