NuGet needs a nicer error message when dealing with encrypted credentials

[blackdwarf]

As per dotnet/cli#1286.

/cc @mgravell

[yishaigalatzer]

I'm thinking this would be quite a common scenario, so lets prioritize it higher.

Pri 0 - Throw a good error message, saying what went wrong in what file and in what line/char
Bonus points - See if we can delay load the credentials, or only load them for active feeds. We should discuss which of the two is best.

This also looks similar to the bug on your plate where a password as a * char in it

[sklivvz]

Also, it shouldn't really blow up at all if the credentials are not needed or used -- at the moment they merely need to be in the global config, and it will be an issue even if the local config has a "clear" and therefore should not read them.

[yishaigalatzer]

@sklivvz not sure what you are adding here? Can you clarify that compared to my comment above?

See if we can delay load the credentials, or only load them for active feeds. We should discuss which of the two is best.

[sklivvz]

This is the default configuration which is generated by dotnet new:

  <packageSources>
    <!--To inherit the global NuGet package sources remove the <clear/> line below -->
    <clear />
    <add key="dotnet-core" value="https://dotnet.myget.org/F/dotnet-core/api/v3/index.json" />
    <add key="api.nuget.org" value="https://api.nuget.org/v3/index.json" />
  </packageSources>

Notice the comment. Other sources should not be inherited. Not inherited sources should actually be ignored, not delay loaded.

[yishaigalatzer]

Duh :)

[blackdwarf]

Soooo...is this like another bug or something? The thing?

[sklivvz]

@blackdwarf after giving a look at the code base and little thought. I think it's three bugs (in the sense that it's probably going to take three patches):

Bug 1: encrypted passwords should be supported and not throw

Bug 2: the caller method chain should handle any exceptions when loading the package sources (including the one from Bug 1) and give appropriate feedback when loading fails

Bug 3: package sources "cleared" in the configuration should not be loaded or parsed

Fixing Bug 1 or Bug 3 would fix the issue as originally reported. Fixing Bug 2 would not, as the command line calls would still fail with a valid configuration. However I think it's highly appropriate that a decent error message is thrown to the end user if shit goes ill anyways.

[mgravell]

Bug 4: disabled global options should be parsed

On Sat, 27 Feb 2016 10:36 Marco Cecconi, notifications@github.com wrote:

@blackdwarf https://github.com/blackdwarf after giving a look at the
code base and little thought. I think it's three bugs (in the sense that
it's probably going to take three patches):

Bug 1: encrypted passwords should be supported and not throw

Bug 2: the caller method chain should handle any exceptions when
loading the package sources (including the one from Bug 1) and give
appropriate feedback when loading fails

Bug 3: package sources "cleared" in the configuration
#2197 (comment) should
not be loaded or parsed

Fixing Bug 1 or Bug 3 would fix the issue as originally reported. Fixing
Bug 2 would not, as the command line calls would still fail with a valid
configuration. However I think it's highly appropriate that a decent error
message is thrown to the end user if shit goes ill anyways.

—
Reply to this email directly or view it on GitHub
#2197 (comment).

[yishaigalatzer]

Except coreclr does not support the encryption on Windows at the moment. So this works well on desktop CLR but not one core (at least yet).

Agree with the rest