auth: Fix very unlikely race condition (sort of) when refreshing token.

common/auth/auth.go

 // Copyright 2015 The LUCI Authors. All rights reserved.
 // Use of this source code is governed under the Apache License, Version 2.0
 // that can be found in the LICENSE file.
 
 // Package auth implements a wrapper around golang.org/x/oauth2.
 //
 // Its main improvement is the on-disk cache for OAuth tokens, which is
 // especially important for 3-legged interactive OAuth flows: its usage
 // eliminates annoying login prompts each time a program is used (because the
 // refresh token can now be reused). The cache also allows to reduce unnecessary
@@ skipping 543 matching lines @@
                 return nil, err
         case useAuth:
                 return perRPCCreds{a}, nil // token-injecting PerRPCCredentials
         default:
                 return perRPCCreds{}, nil // noop PerRPCCredentials
         }
 }
 
 // GetAccessToken returns a valid access token with specified minimum lifetime.
 //
-//
 // Does not interact with the user. May return ErrLoginRequired.
 func (a *Authenticator) GetAccessToken(lifetime time.Duration) (*oauth2.Token, error) {
         tok, err := a.currentToken()
         if err != nil {
                 return nil, err
         }
         if tok == nil || internal.TokenExpiresInRnd(a.ctx, tok, lifetime) {
+                // Give 5 sec extra to make sure callers definitely receive a token that
+                // has at least 'lifetime' seconds of life left. Without this margin, we
+                // can get into an unlucky situation where the token is valid here, but
+                // no longer valid (has fewer than 'lifetime' life left) up the stack, due
+                // to the passage of time.
                 var err error
-                tok, err = a.refreshToken(tok, lifetime)
+                tok, err = a.refreshToken(tok, lifetime+5*time.Second)

[iannucci, 2017/06/23 00:06:18]

would it make sense to tie this to the context deadline?

[Vadim Sh., 2017/06/23 00:11:59]

I don't think if will provide any value. By existing contract, the tokens we
return are allowed to have lifetime that exceeds context deadline (it's just how
the token cache works: if we have a token that lives for 1h, we use it, no
matter what the context deadline is).

So tying min lifetime to the context deadline when refreshing the token will
just be confusing, I think: most of the time returned tokens will greatly
outlive the deadline anyhow.

                 if err != nil {
                         return nil, err
                 }
                 // Note: no randomization here. It is a sanity check that verifies
                 // refreshToken did its job.
                 if internal.TokenExpiresIn(a.ctx, tok, lifetime) {
                         return nil, fmt.Errorf("auth: failed to refresh the token")
                 }
         }
         return tok, nil
@@ skipping 407 matching lines @@
         })
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Transport implementation.
 
 // authTokenInjector injects an authentication token into request headers.
 //
 // Used as a callback for NewModifyingTransport.
 func (a *Authenticator) authTokenInjector(req *http.Request) error {
-        // Grab a currently known token or fail if 'ensureInitialized' failed.
-        tok, err := a.currentToken()
-        if err != nil {
+        switch tok, err := a.GetAccessToken(minAcceptedLifetime); {
+        case err == ErrLoginRequired && a.effectiveLoginMode() == OptionalLogin:
+                return nil // skip auth, no need for modifications
+        case err != nil:
                 return err
+        default:
+                tok.SetAuthHeader(req)
+                return nil
         }
-
-        // Attempt to refresh the token, if required. Revert to non-authed call if
-        // token can't be refreshed and running in OptionalLogin mode.
-        if tok == nil || internal.TokenExpiresInRnd(a.ctx, tok, minAcceptedLifetime) {
-                var err error
-                tok, err = a.refreshToken(tok, minAcceptedLifetime)
-                switch {
-                case err == ErrLoginRequired && a.effectiveLoginMode() == OptionalLogin:
-                        return nil // skip auth, no need for modifications
-                case err != nil:
-                        return err
-                // Note: no randomization here. It is a sanity check that verifies
-                // refreshToken did its job.
-                case internal.TokenExpiresIn(a.ctx, tok, minAcceptedLifetime):
-                        return fmt.Errorf("auth: failed to refresh the token")
-                }
-        }
-        tok.SetAuthHeader(req)
-        return nil
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // tokenWithProvider implementation.
 
 // tokenWithProvider wraps a token with provider that can update it and a cache
 // that stores it.
 type tokenWithProvider struct {
         token    *oauth2.Token          // in-memory cache of the token
         provider internal.TokenProvider // knows how to generate 'token'
@@ skipping 238 matching lines @@
 func makeIAMTokenProvider(ctx context.Context, opts *Options) (internal.TokenProvider, error) {
         if opts.testingIAMTokenProvider != nil {
                 return opts.testingIAMTokenProvider, nil
         }
         return internal.NewIAMTokenProvider(
                 ctx,
                 opts.ActAsServiceAccount,
                 opts.Scopes,
                 opts.Transport)
 }