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

NuGet.exe push 403 handling - Incorrectly prompting for credentials #2910

Closed
maartenba opened this Issue Jun 6, 2016 · 7 comments

Comments

Projects
None yet
4 participants
@maartenba
Contributor

maartenba commented Jun 6, 2016

NuGet.exe is incorrectly prompting for credentials on push that results in 403 Unauthorized. I tested this against local gallery but verified it has the same behaviour against NuGet.org.

Repro steps

  • Push a package using a non-existing API key (so that authentication will fail)
    NuGet.exe push <package> <random-guid-api-key> -Source https://www.nuget.org/api/v2/package

Expected

NuGet.exe 2.x, 3.2.0, 3.3.0 show a 403 Forbidden with the error message.

1

## Actual

NuGet.exe 3.4.3, 3.4.4 and 3.5.0-beta prompt for credentials.

2

After 3 failed attempts, the 403 Forbidden is shown.

3

@joelverhagen

This comment has been minimized.

Show comment
Hide comment
@joelverhagen

joelverhagen Jun 6, 2016

Member

We intentionally added prompting on 403. The reasoning was that if the server is returned 403, the user should be given a chance to enter different credentials. 403 generally means the server is saying "okay, I know who you are but you don't have permissions to access that resource". Therefore, a different set of credentials from the user could be used.

See PRs:
NuGet/NuGet.Client#487
NuGet/NuGet.Client#498

Member

joelverhagen commented Jun 6, 2016

We intentionally added prompting on 403. The reasoning was that if the server is returned 403, the user should be given a chance to enter different credentials. 403 generally means the server is saying "okay, I know who you are but you don't have permissions to access that resource". Therefore, a different set of credentials from the user could be used.

See PRs:
NuGet/NuGet.Client#487
NuGet/NuGet.Client#498

@maartenba

This comment has been minimized.

Show comment
Hide comment
@maartenba

maartenba Jun 6, 2016

Contributor

That is somewhat annoying, for two reasons even 😄

  1. NuGet.org does not do basic auth. This means the username/password entered will never be considered valid during this flow (which you can see in the "after 3 failed attempts" where I did enter valid username/password).
  2. If we decide to expire API keys, say in 90 days, I expect lots of build around the world waiting for input (and thus hanging) instead of failing fast, with fall-out to our support inbox.
Contributor

maartenba commented Jun 6, 2016

That is somewhat annoying, for two reasons even 😄

  1. NuGet.org does not do basic auth. This means the username/password entered will never be considered valid during this flow (which you can see in the "after 3 failed attempts" where I did enter valid username/password).
  2. If we decide to expire API keys, say in 90 days, I expect lots of build around the world waiting for input (and thus hanging) instead of failing fast, with fall-out to our support inbox.
@joelverhagen

This comment has been minimized.

Show comment
Hide comment
@joelverhagen

joelverhagen Jun 6, 2016

Member

Yup seems reasonable. I think we have two options (spoke offline with @rrelyea and @yishaigalatzer):

  1. Only prompt for 403 if the user has gotten 401 (or provided credentials of some kind) already for this source.
  2. Only prompt for 403 if an API key has NOT been provided. This will take some plumbing.
Member

joelverhagen commented Jun 6, 2016

Yup seems reasonable. I think we have two options (spoke offline with @rrelyea and @yishaigalatzer):

  1. Only prompt for 403 if the user has gotten 401 (or provided credentials of some kind) already for this source.
  2. Only prompt for 403 if an API key has NOT been provided. This will take some plumbing.
@maartenba

This comment has been minimized.

Show comment
Hide comment
@maartenba

maartenba Jun 6, 2016

Contributor

2 seems sleekest imo, but 1 may work as well.

Contributor

maartenba commented Jun 6, 2016

2 seems sleekest imo, but 1 may work as well.

@joelverhagen joelverhagen added this to the 3.4.5 milestone Jun 6, 2016

@joelverhagen joelverhagen self-assigned this Jun 6, 2016

@joelverhagen

This comment has been minimized.

Show comment
Hide comment
@joelverhagen

joelverhagen Jun 6, 2016

Member

Time permitting, we will try to pull this in for 3.4.5.

Member

joelverhagen commented Jun 6, 2016

Time permitting, we will try to pull this in for 3.4.5.

@joelverhagen

This comment has been minimized.

Show comment
Hide comment
@joelverhagen

joelverhagen Jun 7, 2016

Member

This fix has been committed to dev and 3.5.0-beta2 so far. I am working on porting the change to 3.4.5.

Member

joelverhagen commented Jun 7, 2016

This fix has been committed to dev and 3.5.0-beta2 so far. I am working on porting the change to 3.4.5.

@joelverhagen

This comment has been minimized.

Show comment
Hide comment
@joelverhagen

joelverhagen Jun 8, 2016

Member

This change has now been ported to all destination branches:
dev: NuGet/NuGet.Client@1ddcb74
3.5.0-beta2: NuGet/NuGet.Client@efb2590
3.4.5: NuGet/NuGet.Client@b0ef84e

Member

joelverhagen commented Jun 8, 2016

This change has now been ported to all destination branches:
dev: NuGet/NuGet.Client@1ddcb74
3.5.0-beta2: NuGet/NuGet.Client@efb2590
3.4.5: NuGet/NuGet.Client@b0ef84e

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

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