Extend LUCI_CONTEXT["local_auth"] protocol to understand accounts.

Extend LUCI_CONTEXT["local_auth"] protocol to understand accounts.

The client side of the protocol ("common/auth/internal/luci_ctx") is still
backward compatible with old version of the protocol. This will be removed once
Swarming is updated to provide 'accounts' and 'default_account_id'.

R=iannucci@chromium.org
BUG=730878
Review-Url: https://codereview.chromium.org/2951553002
Committed: https://github.com/luci/luci-go/commit/b782c74dd7d045d41ae85e6a90680efa61d24cfa

[Vadim Sh., 2017-06-19 20:16:29]

PTAL

https://codereview.chromium.org/2951553002/diff/1/client/authcli/authcli.go
File client/authcli/authcli.go (right):

https://codereview.chromium.org/2951553002/diff/1/client/authcli/authcli.go#n...
client/authcli/authcli.go:506: // We currently always setup a context with one
account (which is also
it will be possible to extend authutil to selective replace some accounts, but
leave other accounts intact

but I'm not sure it is needed.... So I did minimal change for now.

https://codereview.chromium.org/2951553002/diff/1/common/auth/internal/luci_c...
File common/auth/internal/luci_ctx.go (right):

https://codereview.chromium.org/2951553002/diff/1/common/auth/internal/luci_c...
common/auth/internal/luci_ctx.go:43: // TODO(vadimsh): This method currently
supports both "old" auth server that
I think we currently run cipd in a LUCI_CONTEXT provided by Swarming (on some
bots that use LUCI_CONTEXT already, eg Skia). So we need some backward
compatibility here.

https://codereview.chromium.org/2951553002/diff/1/common/auth/localauth/serve...
File common/auth/localauth/server_test.go (right):

https://codereview.chromium.org/2951553002/diff/1/common/auth/localauth/serve...
common/auth/localauth/server_test.go:94: testGen := func(ctx context.Context,
scopes []string, lifetime time.Duration) (*oauth2.Token, error) {
no changes here, just moved

[Vadim Sh., 2017-06-19 20:24:44]

The CQ bit was checked by vadimsh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-19 20:24:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-19 20:30:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-19 20:30:23]

Dry run: Try jobs failed on following builders:
  Luci-go Linux Trusty 32-on-64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/36dad9259bb74d10)
  Luci-go Linux Trusty 64 Tester on luci.infra.try (JOB_FAILED,
https://luci-milo.appspot.com/swarming/task/36dad925a6d74510)

[iannucci, 2017-06-19 20:31:29]

lgtm, that wasn't as ugly as I was expecting (assuming we complete this
transition and all the TODO's get TODONE).

https://codereview.chromium.org/2951553002/diff/1/client/authcli/authcli.go
File client/authcli/authcli.go (right):

https://codereview.chromium.org/2951553002/diff/1/client/authcli/authcli.go#n...
client/authcli/authcli.go:514: DefaultAccountID: "authutil",
should we document these names somewhere maybe?

https://codereview.chromium.org/2951553002/diff/1/common/auth/internal/luci_c...
File common/auth/internal/luci_ctx.go (right):

https://codereview.chromium.org/2951553002/diff/1/common/auth/internal/luci_c...
common/auth/internal/luci_ctx.go:52: return nil, fmt.Errorf(`no
"default_account_id" in LUCI_CONTEXT["local_auth"]`)
IIUC, this would only happen if the server emits the new-style LUCI_CONTEXT
blob, but has an empty/missing default_account_id field?

https://codereview.chromium.org/2951553002/diff/1/common/auth/localauth/rpcs/...
File common/auth/localauth/rpcs/getoauthtoken.go (right):

https://codereview.chromium.org/2951553002/diff/1/common/auth/localauth/rpcs/...
common/auth/localauth/rpcs/getoauthtoken.go:22: return fmt.Errorf(`field
"scopes" is required`)
may want to use errors.New so we get a stacktrace from here.

https://codereview.chromium.org/2951553002/diff/1/lucictx/local_auth.go
File lucictx/local_auth.go (right):

https://codereview.chromium.org/2951553002/diff/1/lucictx/local_auth.go#newco...
lucictx/local_auth.go:35: // TODO(vadimsh): Remove this method once all servers
provide 'accounts'.
will we still need it for the DefaultAccountID != "" bit? Or should callers just
check that directly?

[Vadim Sh., 2017-06-19 20:41:36]

https://codereview.chromium.org/2951553002/diff/1/client/authcli/authcli.go
File client/authcli/authcli.go (right):

https://codereview.chromium.org/2951553002/diff/1/client/authcli/authcli.go#n...
client/authcli/authcli.go:514: DefaultAccountID: "authutil",
On 2017/06/19 20:31:28, iannucci wrote:
> should we document these names somewhere maybe?

Maybe.. This particular name shouldn't be hardcoded anywhere though. I think
most of our code should not rely on any particular account names (it should just
use "default_account_id"). The only place where account names matter is when
switching default account to be something else. I believe we'll need to do it
only in Swarming, Kitchen and, maybe, some recipe guts.

We'll see how it goes.

https://codereview.chromium.org/2951553002/diff/1/common/auth/internal/luci_c...
File common/auth/internal/luci_ctx.go (right):

https://codereview.chromium.org/2951553002/diff/1/common/auth/internal/luci_c...
common/auth/internal/luci_ctx.go:52: return nil, fmt.Errorf(`no
"default_account_id" in LUCI_CONTEXT["local_auth"]`)
On 2017/06/19 20:31:28, iannucci wrote:
> IIUC, this would only happen if the server emits the new-style LUCI_CONTEXT
> blob, but has an empty/missing default_account_id field?

Correct. Once old servers are removed, CanUseByDefault will be just replaced
with local_auth.default_account_id != ""

https://codereview.chromium.org/2951553002/diff/1/common/auth/localauth/rpcs/...
File common/auth/localauth/rpcs/getoauthtoken.go (right):

https://codereview.chromium.org/2951553002/diff/1/common/auth/localauth/rpcs/...
common/auth/localauth/rpcs/getoauthtoken.go:22: return fmt.Errorf(`field
"scopes" is required`)
On 2017/06/19 20:31:28, iannucci wrote:
> may want to use errors.New so we get a stacktrace from here.

Done, though I don't really see a point in doing that.

https://codereview.chromium.org/2951553002/diff/1/lucictx/local_auth.go
File lucictx/local_auth.go (right):

https://codereview.chromium.org/2951553002/diff/1/lucictx/local_auth.go#newco...
lucictx/local_auth.go:35: // TODO(vadimsh): Remove this method once all servers
provide 'accounts'.
On 2017/06/19 20:31:28, iannucci wrote:
> will we still need it for the DefaultAccountID != "" bit? Or should callers
just
> check that directly?

Callers will check it directly.

[Vadim Sh., 2017-06-19 20:47:05]

The CQ bit was checked by vadimsh@chromium.org

[Vadim Sh., 2017-06-19 20:47:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org
Link to the patchset: https://codereview.chromium.org/2951553002/#ps60001
(title: "few more tests")

[commit-bot: I haz the power, 2017-06-19 20:47:12]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-19 21:01:24]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497905225202920,
"parent_rev": "7872856a4eb583a8eb851baa170403eba1c37396", "commit_rev":
"b782c74dd7d045d41ae85e6a90680efa61d24cfa"}

[commit-bot: I haz the power, 2017-06-19 21:01:28]

Description was changed from

==========
Extend LUCI_CONTEXT["local_auth"] protocol to understand accounts.

The client side of the protocol ("common/auth/internal/luci_ctx") is still
backward compatible with old version of the protocol. This will be removed once
Swarming is updated to provide 'accounts' and 'default_account_id'.

R=iannucci@chromium.org
BUG=730878
==========

to

==========
Extend LUCI_CONTEXT["local_auth"] protocol to understand accounts.

The client side of the protocol ("common/auth/internal/luci_ctx") is still
backward compatible with old version of the protocol. This will be removed once
Swarming is updated to provide 'accounts' and 'default_account_id'.

R=iannucci@chromium.org
BUG=730878

Review-Url: https://codereview.chromium.org/2951553002
Committed:
https://github.com/luci/luci-go/commit/b782c74dd7d045d41ae85e6a90680efa61d24cfa
==========

[commit-bot: I haz the power, 2017-06-19 21:01:29]

Committed patchset #4 (id:60001) as
https://github.com/luci/luci-go/commit/b782c74dd7d045d41ae85e6a90680efa61d24cfa