New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Install blocks if a single source fails authorization #2034

Closed
zjrunner opened this Issue Feb 2, 2016 · 9 comments

Comments

Projects
None yet
4 participants
@zjrunner

zjrunner commented Feb 2, 2016

This may be multiple related but distinct bugs:

  • I expect 'nuget install' to not be blocked if one of many sources fails authorization
  • I expect an authz failure to not be retried

In the below example, the first two sources in my list have the package. The third one does not and I have no permissions to this specific feed. I do have credentials returned by the CredentialProvider.VSS.exe that authenticates my user, but when nuget takes those creds and calls the server I do not have authorization. Nuget blocks and prevents install due to this one bad feed, and gets in an auth loop where it keeps calling the CredentialProvider over and over, forcing the pop-up over and over (isRetry=true), The only way out of this loop is to wait 5min for it to timeout and end the failure cycle, or to close the pop-up without providing credentials.

(aside : I'm going to move to restore instead of install -- it fits my needs and doesn't have the first blocking problem. I'm not yet sure if it has the second retry problem)

** Error ** Nuget failure : install DevConsole.Commandline -Version 5.1.2 -NoCache -ExcludeVersion -PackageSaveMode nuspec -Verbosity detailed -ConfigFile C:\Users\zackrun\AppData\Local\devconsole\nuget\BoazvU6lVFkykMuMVoLPtQ\nuget.config -Source \co2adcvm446\corext\corext -Source \co2wttvm0008\corext\corext -Source https://codesharing-su0.pkgs.visualstudio.com/DefaultCollection/_packaging/NoZackRun/nuget/v2 -OutputDirectory C:\Users\zackrun\AppData\Local\devconsole\packages\DevConsole.Commandline\5.1.2.tmp
Feeds used:
\co2adcvm446\corext\corext
\co2wttvm0008\corext\corext
https://codesharing-su0.pkgs.visualstudio.com/DefaultCollection/_packaging/NoZackRun/nuget/v2

Attempting to gather dependencies information for package 'DevConsole.Commandline.5.1.2' with respect to project 'C:\Users\zackrun\AppData\Local\devconsole\packages\DevConsole.Commandline\5.1.2.tmp', targeting 'Any,Version=v0.0'

System.InvalidOperationException: One or more errors occurred. ---> System.AggregateException: One or more errors occurred. ---> NuGet.Credentials.PluginException: Credential plugin C:\Users\zackrun\AppData\Local\devconsole\nuget\BoazvU6lVFkykMuMVoLPtQ\CredentialProvider.VSS.exe handles this request, but is unable to provide credentials. Credentials request has been cancelled by the user.

@zjrunner

This comment has been minimized.

Show comment
Hide comment
@zjrunner

zjrunner Feb 3, 2016

(verified that restore does not have the blocking issue - whether or not it retries authz failures is less important to me currently since I don't depend on the failing feed in question)

zjrunner commented Feb 3, 2016

(verified that restore does not have the blocking issue - whether or not it retries authz failures is less important to me currently since I don't depend on the failing feed in question)

@yishaigalatzer

This comment has been minimized.

Show comment
Hide comment
@yishaigalatzer

yishaigalatzer Feb 3, 2016

This is a feature your team developed. I assumed you plan to submit a PR?

I'm OK with fixing the second item (be able to ignore authz issue and not retry them), we need to discuss separately should an operation succeed if over feed fails, this is not something that should work generally and it can lead to incorrect results when restoring for project.json

yishaigalatzer commented Feb 3, 2016

This is a feature your team developed. I assumed you plan to submit a PR?

I'm OK with fixing the second item (be able to ignore authz issue and not retry them), we need to discuss separately should an operation succeed if over feed fails, this is not something that should work generally and it can lead to incorrect results when restoring for project.json

@zjrunner

This comment has been minimized.

Show comment
Hide comment
@zjrunner

zjrunner Feb 3, 2016

That would be the nice thing to do if it is from this team's code, but it probably won't land immediately from this side. I thought it might be outside this team's code because authn is happening just fine, getting back a PAT, it is getting back to the nuget process as a cred, which is sent to the server. At this point I thought it would fall within the standard authz flow nuget had previously for other feeds but I didn't look far enough to know if this was true or not.

So you are saying no retry on authz is definitely something to go in as a fix. I didn't get the second half -- how is restore impacted if a single feed can't be auth'd but the packages in question can come from other feeds? Is it a concern that the package ranges can differ (I would expect anyone who cares about this should have a .lock in place which would enforce exact versions anyway)

zjrunner commented Feb 3, 2016

That would be the nice thing to do if it is from this team's code, but it probably won't land immediately from this side. I thought it might be outside this team's code because authn is happening just fine, getting back a PAT, it is getting back to the nuget process as a cred, which is sent to the server. At this point I thought it would fall within the standard authz flow nuget had previously for other feeds but I didn't look far enough to know if this was true or not.

So you are saying no retry on authz is definitely something to go in as a fix. I didn't get the second half -- how is restore impacted if a single feed can't be auth'd but the packages in question can come from other feeds? Is it a concern that the package ranges can differ (I would expect anyone who cares about this should have a .lock in place which would enforce exact versions anyway)

@yishaigalatzer

This comment has been minimized.

Show comment
Hide comment
@yishaigalatzer

yishaigalatzer Feb 9, 2016

I expect it will fail the restore (or at least warn), when I meant your team's code. Your team actually did the authentication flow, and retry makes sense to ask for different creds and a user should be able to cancel, and fail the restore.

yishaigalatzer commented Feb 9, 2016

I expect it will fail the restore (or at least warn), when I meant your team's code. Your team actually did the authentication flow, and retry makes sense to ask for different creds and a user should be able to cancel, and fail the restore.

@yishaigalatzer yishaigalatzer added this to the 3.4 milestone Feb 9, 2016

@yishaigalatzer

This comment has been minimized.

Show comment
Hide comment
@yishaigalatzer

yishaigalatzer Feb 9, 2016

Assigning to Joel who already works on restructuring the retry pattern

yishaigalatzer commented Feb 9, 2016

Assigning to Joel who already works on restructuring the retry pattern

@yishaigalatzer

This comment has been minimized.

Show comment
Hide comment
@yishaigalatzer

yishaigalatzer Feb 9, 2016

@zjrunner re-read the original comment, install should fail if a source is missing, because it might get a different set of packages and we care about consistency more than anything, it is pretty much impossible to track down failed installs like that.

Restore from packages.config should not fail if all entries are found, but restore from project.json could fail if exact references are not found or should fail if star notation is used.

So unless we really get this down all the way in all the scenarios it is probably better to fail.

yishaigalatzer commented Feb 9, 2016

@zjrunner re-read the original comment, install should fail if a source is missing, because it might get a different set of packages and we care about consistency more than anything, it is pretty much impossible to track down failed installs like that.

Restore from packages.config should not fail if all entries are found, but restore from project.json could fail if exact references are not found or should fail if star notation is used.

So unless we really get this down all the way in all the scenarios it is probably better to fail.

@joelverhagen

This comment has been minimized.

Show comment
Hide comment
@joelverhagen

joelverhagen Feb 11, 2016

Member

I tinkered with the current implementation on dev (NuGet/NuGet.Client@4a602cf).

First off, credentials are only prompted for a maxmum of 3 times when the server returns 401 Unauthorized. The user can still cancel out of these prompts (@zjrunner mentioned this). In short, our authN prompting flow seems correct. We do not, however, handle authZ failures (e.g. 403 Forbidden).

Concerning install, the operation fails if any of the sources are unavailable (down or auth failed).

Concerning restore, the current functionality for restoring packages.config or project.json is that we can complete successfully even if a source is unavailable. One caveat is that project.json restore fails if there are floating versions (that is, using the asterisk, e.g. 8.0.*) and a source is unavailable.

Questions:

  1. Should we handle 403 Forbidden? My proposal is that we treat it the same as 401 Unauthorized. That is, if an HTTP response comes back with 401 or 403, we prompt the user for credentials.
  2. Is the current functionality of allowing restore on unavailable servers correct? If not, one option is to never succeed when a source is unavailable but provide a switch like DNU has --ignore-failed-sources (thanks for the heads up @emgarten).

Thoughts?

Member

joelverhagen commented Feb 11, 2016

I tinkered with the current implementation on dev (NuGet/NuGet.Client@4a602cf).

First off, credentials are only prompted for a maxmum of 3 times when the server returns 401 Unauthorized. The user can still cancel out of these prompts (@zjrunner mentioned this). In short, our authN prompting flow seems correct. We do not, however, handle authZ failures (e.g. 403 Forbidden).

Concerning install, the operation fails if any of the sources are unavailable (down or auth failed).

Concerning restore, the current functionality for restoring packages.config or project.json is that we can complete successfully even if a source is unavailable. One caveat is that project.json restore fails if there are floating versions (that is, using the asterisk, e.g. 8.0.*) and a source is unavailable.

Questions:

  1. Should we handle 403 Forbidden? My proposal is that we treat it the same as 401 Unauthorized. That is, if an HTTP response comes back with 401 or 403, we prompt the user for credentials.
  2. Is the current functionality of allowing restore on unavailable servers correct? If not, one option is to never succeed when a source is unavailable but provide a switch like DNU has --ignore-failed-sources (thanks for the heads up @emgarten).

Thoughts?

@yishaigalatzer

This comment has been minimized.

Show comment
Hide comment
@yishaigalatzer

yishaigalatzer Feb 11, 2016

403 - retry but have a good message to the user, just a prompt will be confusing.

Restore for project.json should still fail unless it found exact matches. That's an edge case but can still lead to inconsistent results. The flag sound right to me

yishaigalatzer commented Feb 11, 2016

403 - retry but have a good message to the user, just a prompt will be confusing.

Restore for project.json should still fail unless it found exact matches. That's an edge case but can still lead to inconsistent results. The flag sound right to me

@joelverhagen

This comment has been minimized.

Show comment
Hide comment
@joelverhagen

joelverhagen May 3, 2016

Member

Addressed action items with NuGet/NuGet.Client#498.

Member

joelverhagen commented May 3, 2016

Addressed action items with NuGet/NuGet.Client#498.

@zhili1208 zhili1208 modified the milestones: 3.5 Beta, 3.6 Beta, 3.5 Beta2 Jun 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment