Third party hosted pub registry authentication

[themisir]

Closes #1381

This PR adds support to pub CLI to authenticate with third party hosted pub servers. Before that only official pub servers: pub.dev (aka: pub.dartlang.org) could be authenticated and the authentication was only applied when publishing packages. We usually need to share some pieces of code across company projects. In other ecosystems like npm, nuget, docker this could be done using private repositories / servers. Private repositories does needs authentication for both pulling and pushing data to it - unlike pub's current behavior which only does authenticates requests for pushing new packages.

CLI Interface

dart pub login                      # Will initiate login-flow for pub.dev
dart pub logout                     # Will delete credential.json

dart pub token list                # Will list servers for which a token exists
dart pub token add <hosted_url>    # Will prompt for a token for <hosted_url> and store both
dart pub token remove <hosted_url> # Will remove token for <hosted_url>
dart pub token remove --all        # Will delete all tokens stored

[sigurdm]

I think this starts looking good.
LGTM assuming the last comments are addressed

[themisir]

fyi @sigurdm adding limitation to use OAuth credentials only for pub.dev official servers caused some tests to fail (probably because they're using localhost server to test the feature).

Should I revert those changes back or what would you suggest?

Maybe we could override "official pub server" url using environment variable for the sake of tests. Any suggestions?

[jonasfj]

Just a few nits... the part about truncatedMessage is probably important.

An attacker could otherwise make some very confusing output in the terminal 🙈

[jonasfj]

Let's throw a FormatException if token == null, then we can remove the nullability...

And if we throw FormatException here, that's fine, because in List<Credential> _loadCredentials() { we will appropriately log: Failed to load credentials for ${element['url']}:

[jonasfj]

Let's make this a format exception when loading. Then it'll get logged as a warning every time the user uses an old pub version with a newer config file.

Hmm, I can see that being less smart, but I also don't like dragging around nullability.

@sigurdm any thoughts here?

[themisir]

The issue is current saving mechanism will remove entries that doesn't contains 'token' property because it'll not be able to deserialize it. So they'll not added to the credential list and when saving those entries will be removed.

[themisir]

I wanted to encapsulate json serialization part to Credential class itself to reduce code duplication for serialization & validation, otherwise the code would look like much confusing I think.

[sigurdm]

fyi @sigurdm adding limitation to use OAuth credentials only for pub.dev official servers caused some tests to fail (probably because they're using localhost server to test the feature).

Should I revert those changes back or what would you suggest?

Maybe we could override "official pub server" url using environment variable for the sake of tests. Any suggestions?

Yes - I think that is the way to go!
We have runningFromTest in lib/src/io.dart - I suggest if that is true then we consider the host of PUB_HOSTED_URL to be the "official" host that gets the credentials.

(PS When landing: https://github.com/dart-lang/pub/pull/3092this will be more safe, because runningFromTest will no longer only depend on the environment.)

[themisir]

We have runningFromTest in lib/src/io.dart - I suggest if that is true then we consider the host of PUB_HOSTED_URL to be the "official" host that gets the credentials.

Yes, used this method - it works. In future when we create integration tests for using token authentication for publishing we might want to declare another environment variable to provide whether given PUB_HOSTED_URL should use oauth2 authentication (or: considered as an official pub server) or uses token authentication. But for now it works and aside from testing I don't think this might cause any issue for production.

[themisir]

Do I have to write integration tests for server integration or we could do that in a separate PR later?

[sigurdm]

Thanks @themisir for all the hard work on this!