fixes NuGet/Home #11210 `CredentialService` incorrectly sets the value of `isRetry` #4636

vlada-shubina · 2022-05-19T11:00:24Z

Bug

fixes NuGet/Home#11210
Related to: dotnet/templating#4278

Description

Fixed setting of isRetry value in CredentialService, isRetry should be set only when previous request fails.

This affects the UX of .NET SDK as users need to auth multiple times when doing operations with private NuGet feed.
isRetry should not be set on subsequent call unless the credentials are invalid.

PR Checklist

PR has a meaningful title
PR has a linked issue.
Described changes
Tests
- Automated tests added
Documentation
- https://github.com/NuGet/docs.microsoft.com-nuget/issues/2715

…e of `isRetry`

zivkan · 2022-05-19T14:32:25Z

src/NuGet.Core/NuGet.Credentials/CredentialService.cs

@@ -90,7 +90,7 @@ public CredentialService(AsyncLazy<IEnumerable<ICredentialProvider>> providers,
                cancellationToken.ThrowIfCancellationRequested();

                var retryKey = RetryCacheKey(uri, type, provider);
-                var isRetry = _retryCache.ContainsKey(retryKey);
+                var isRetry = (type == CredentialRequestType.Forbidden) && _retryCache.ContainsKey(retryKey);


Could you help me understand why Forbidden and not Unauthorized?

Forbidden suggests the username&password or PAT is valid, so no amounts of retries are going to succeed until permissions are fixed server side. The edge case is when the customer has two or more accounts, and they entered the wrong one.

Unauthorized suggests the username& password or PAT is invalid, so valid credentials need to be supplied in the retry.

Honestly, my intuition is that both attempts are a kind of retry, but only unauthorized responses make sense to retry. Why shouldn't incorrect credentials being supplied be treated as a retry?

My understanding of the problem, from reading the link issues, is that NuGet doesn't attempt to pass the credentials on the first HTTP request (same as how web browsers (usually?) don't, but NuGet incorrectly detects the first attempt at sending credentials as a retry. My gut feeling is that we need a more sophisticated fix, rather than treating all unauthorized responses as not-retries.

My understanding of the problem, from reading the link issues, is that NuGet doesn't attempt to pass the credentials on the first HTTP request (same as how web browsers (usually?) don't, but NuGet incorrectly detects the first attempt at sending credentials as a retry.

The credential service should only come in play with a challenge, so I don't imagine we have an unneeded isRetry.

nkolev92

I'm having a tough time figuring out the effects of this change here.

In particular I was reading:

dotnet/templating#4278 (comment)

Which suggests NuGet tries to talk to the provider after it's obtained successful credentials, which seems to contradict what

NuGet.Client/src/NuGet.Core/NuGet.Protocol/HttpSource/HttpSourceAuthenticationHandler.cs

Line 67 in 5ac4851

    
           protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)

does.

If NuGet retries for successful credentials then this wouldn't fix it correct?

Can you maybe explain step by step what happens here currently and what should be happen with a focus on what the customer sees.

nkolev92 · 2022-05-19T16:13:46Z

test/NuGet.Core.Tests/NuGet.Credentials.Test/CredentialServiceTests.cs

+        }
+
+        [Fact]
+        public async Task GetCredentialsAsync_SecondCallHasRetryTrue_WhenTypeIsForbidden()


I'd flip the 2nd and 3rd part of the name here. https://github.com/NuGet/NuGet.Client/blob/dev/docs/coding-guidelines.md#unit-test-method-naming

nkolev92 · 2022-05-19T16:45:59Z

src/NuGet.Core/NuGet.Credentials/CredentialService.cs

@@ -90,7 +90,7 @@ public CredentialService(AsyncLazy<IEnumerable<ICredentialProvider>> providers,
                cancellationToken.ThrowIfCancellationRequested();

                var retryKey = RetryCacheKey(uri, type, provider);
-                var isRetry = _retryCache.ContainsKey(retryKey);
+                var isRetry = (type == CredentialRequestType.Forbidden) && _retryCache.ContainsKey(retryKey);


My understanding of the problem, from reading the link issues, is that NuGet doesn't attempt to pass the credentials on the first HTTP request (same as how web browsers (usually?) don't, but NuGet incorrectly detects the first attempt at sending credentials as a retry.

The credential service should only come in play with a challenge, so I don't imagine we have an unneeded isRetry.

vlada-shubina · 2022-05-19T19:57:58Z

I'm having a tough time figuring out the effects of this change here.

In particular I was reading:

dotnet/templating#4278 (comment)

Which suggests NuGet tries to talk to the provider after it's obtained successful credentials, which seems to contradict what

NuGet.Client/src/NuGet.Core/NuGet.Protocol/HttpSource/HttpSourceAuthenticationHandler.cs

Line 67 in 5ac4851

protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)

does.
If NuGet retries for successful credentials then this wouldn't fix it correct?

Can you maybe explain step by step what happens here currently and what should be happen with a focus on what the customer sees.

I attempted to do fix explained in NuGet/Home#11210
I'm still trying to debug this change in dotnet new, will provide more results tomorrow. Logs in #4278 is for different case.

My understanding of what is happening:

dotnet new using multiple resources from NuGet.Protocol at same uri
at accessing the first resource, on SendAsync default credentials is used, it fails for private feed; isRetry is not set` due to no cache, the user completes authentication and it gets cached; consequent call succeeds
at accessing the second resource, on SendAsync default credentials is used, it fails; since cache is already available, isRetry is set`, cache is removed, and the user need to complete authentication again.

As the result, the user need to authenticate twice on installing / updating template packages with dotnet new. This is very unfortunate behavior.
The fix may work if first call to the second resource completes with code other than Unauthorized, but I doubt so at this point.

Proper fix might be:

in HttpSourceAuthenticationHandler and possible other Handlers implement the following logic
- 1st try - with default credentials (as now)
- 2nd try - with cached credentials from CredentialService (if any)
- 3rd try - with cleaning the cache and re-authentication (basically current isRetry behavior)

This will avoid the need of re-authentication. But this will need changes in API, I cannot see any way how 2nd try is possible with existing definition of GetCredentialsAsync.

Another way is to share Credentials between resource, but again not sure if it's possible and reasonable.

zivkan · 2022-05-19T20:17:40Z

A problem that the dotnet workload team experienced is that they were creating multiple instances of NuGet.Protocol.SourceRepository, one for each file they were downloading. NuGet caches the credential (for all resources) within that instance, but by creating a different instance per file, it caused the credential to be re-obtained each time (although in that scenario, isRetry should be false on the first message to the credential provider).

Is it possible that dotnet new is creating multiple instances of SourceRepository (probably though Repository.Factory.CreateCoreV3(url)) for the same URL, rather than re-using/sharing a single instance?

vlada-shubina · 2022-05-19T21:23:43Z

A problem that the dotnet workload team experienced is that they were creating multiple instances of NuGet.Protocol.SourceRepository, one for each file they were downloading. NuGet caches the credential (for all resources) within that instance, but by creating a different instance per file, it caused the credential to be re-obtained each time (in which case the change in this PR wouldn't help).

Is it possible that dotnet new is creating multiple instances of SourceRepository (probably though Repository.Factory.CreateCoreV3(url)) for the same URL, rather than re-using/sharing a single instance?

Yes, it appears it does, thank you for pointing it out - dotnet new creates it twice - once for downloading available package metadata, and second time for actual download of requested package. I will change it tomorrow to see if it helps to avoid authenticating twice, it should not be difficult to cache created source repositories. If so, this issue will no longer be that important for dotnet new.

nkolev92 · 2022-05-19T23:53:03Z

We recently fixed a similar issue with list package: NuGet/Home#11169.

vlada-shubina · 2022-05-20T11:25:16Z

A problem that the dotnet workload team experienced is that they were creating multiple instances of NuGet.Protocol.SourceRepository, one for each file they were downloading. NuGet caches the credential (for all resources) within that instance, but by creating a different instance per file, it caused the credential to be re-obtained each time (although in that scenario, isRetry should be false on the first message to the credential provider).

Is it possible that dotnet new is creating multiple instances of SourceRepository (probably though Repository.Factory.CreateCoreV3(url)) for the same URL, rather than re-using/sharing a single instance?

So this worked perfectly, so dotnet/templating issue will be fixed in dotnet/templating#4764.
I'm closing this PR.

I don't have enough context to see if NuGet/Home#11210 should be closed too. It's clear that fix suggested in that issue won't work.

fixes NuGet/Home #11210 CredentialService incorrectly sets the valu…

02588a7

…e of `isRetry`

vlada-shubina requested a review from a team as a code owner May 19, 2022 11:00

ghost added the Community PRs created by someone not in the NuGet team label May 19, 2022

tests adjustments

c72ffc4

zivkan reviewed May 19, 2022

View reviewed changes

nkolev92 reviewed May 19, 2022

View reviewed changes

vlada-shubina closed this May 20, 2022

kartheekp-ms mentioned this pull request Sep 14, 2022

while executing list package command, use SourceRepository from cache to avoid clearing valid sessions tokens from credential provider cache #4794

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes NuGet/Home #11210 `CredentialService` incorrectly sets the value of `isRetry` #4636

fixes NuGet/Home #11210 `CredentialService` incorrectly sets the value of `isRetry` #4636

vlada-shubina commented May 19, 2022 •

edited

Loading

zivkan May 19, 2022

nkolev92 May 19, 2022

nkolev92 left a comment

nkolev92 May 19, 2022

nkolev92 May 19, 2022

vlada-shubina commented May 19, 2022

zivkan commented May 19, 2022 •

edited

Loading

vlada-shubina commented May 19, 2022

nkolev92 commented May 19, 2022

vlada-shubina commented May 20, 2022

fixes NuGet/Home #11210 CredentialService incorrectly sets the value of isRetry #4636

fixes NuGet/Home #11210 CredentialService incorrectly sets the value of isRetry #4636

Conversation

vlada-shubina commented May 19, 2022 • edited Loading

Bug

Description

PR Checklist

zivkan May 19, 2022

Choose a reason for hiding this comment

nkolev92 May 19, 2022

Choose a reason for hiding this comment

nkolev92 left a comment

Choose a reason for hiding this comment

nkolev92 May 19, 2022

Choose a reason for hiding this comment

nkolev92 May 19, 2022

Choose a reason for hiding this comment

vlada-shubina commented May 19, 2022

zivkan commented May 19, 2022 • edited Loading

vlada-shubina commented May 19, 2022

nkolev92 commented May 19, 2022

vlada-shubina commented May 20, 2022

fixes NuGet/Home #11210 `CredentialService` incorrectly sets the value of `isRetry` #4636

fixes NuGet/Home #11210 `CredentialService` incorrectly sets the value of `isRetry` #4636

vlada-shubina commented May 19, 2022 •

edited

Loading

zivkan commented May 19, 2022 •

edited

Loading