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
fix: quotaProjectId should be applied for cached getRequestMetadata(URI, Executor, RequestMetadataCallback)
#509
Changes from all commits
885ebf8
8665dfd
8f6b1fc
eaa8ee5
1b4174f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,7 @@ | |
import com.google.common.annotations.VisibleForTesting; | ||
import com.google.common.base.MoreObjects; | ||
import com.google.common.base.Preconditions; | ||
import com.google.common.collect.ImmutableMap; | ||
import com.google.common.collect.Iterables; | ||
import java.io.IOException; | ||
import java.io.ObjectInputStream; | ||
|
@@ -56,6 +57,7 @@ public class OAuth2Credentials extends Credentials { | |
|
||
private static final long serialVersionUID = 4556936364828217687L; | ||
private static final long MINIMUM_TOKEN_MILLISECONDS = 60000L * 5L; | ||
private static final Map<String, List<String>> EMPTY_EXTRA_HEADERS = Collections.emptyMap(); | ||
|
||
// byte[] is serializable, so the lock variable can be final | ||
private final Object lock = new byte[0]; | ||
|
@@ -89,7 +91,7 @@ protected OAuth2Credentials() { | |
*/ | ||
protected OAuth2Credentials(AccessToken accessToken) { | ||
if (accessToken != null) { | ||
useAccessToken(accessToken); | ||
useAccessToken(accessToken, EMPTY_EXTRA_HEADERS); | ||
} | ||
} | ||
|
||
|
@@ -154,7 +156,9 @@ public void refresh() throws IOException { | |
synchronized (lock) { | ||
requestMetadata = null; | ||
temporaryAccess = null; | ||
useAccessToken(Preconditions.checkNotNull(refreshAccessToken(), "new access token")); | ||
useAccessToken( | ||
Preconditions.checkNotNull(refreshAccessToken(), "new access token"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a very strange and possibly inappropriate use of Preconditions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, it's been there for years. Filed #510 to track. |
||
getAdditionalHeaders()); | ||
if (changeListeners != null) { | ||
for (CredentialsChangedListener listener : changeListeners) { | ||
listener.onChanged(this); | ||
|
@@ -163,6 +167,15 @@ public void refresh() throws IOException { | |
} | ||
} | ||
|
||
/** | ||
* Provide additional headers to return as request metadata. | ||
* | ||
* @return additional headers | ||
*/ | ||
protected Map<String, List<String>> getAdditionalHeaders() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nested types (Map of List) suggest there's a missed opportunity for a more semantic class. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. HTTP headers behave as maps of Note: |
||
return EMPTY_EXTRA_HEADERS; | ||
} | ||
|
||
/** | ||
* Refresh these credentials only if they have expired or are expiring imminently. | ||
* | ||
|
@@ -177,12 +190,15 @@ public void refreshIfExpired() throws IOException { | |
} | ||
|
||
// Must be called under lock | ||
private void useAccessToken(AccessToken token) { | ||
private void useAccessToken(AccessToken token, Map<String, List<String>> additionalHeaders) { | ||
this.temporaryAccess = token; | ||
this.requestMetadata = | ||
Collections.singletonMap( | ||
AuthHttpConstants.AUTHORIZATION, | ||
Collections.singletonList(OAuth2Utils.BEARER_PREFIX + token.getTokenValue())); | ||
ImmutableMap.<String, List<String>>builder() | ||
.put( | ||
AuthHttpConstants.AUTHORIZATION, | ||
Collections.singletonList(OAuth2Utils.BEARER_PREFIX + token.getTokenValue())) | ||
.putAll(additionalHeaders) | ||
.build(); | ||
} | ||
|
||
// Must be called under lock | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will this be added to by subclasses? I'm not quite following but it looks like it might be; but this is immutable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addQuotaProjectIdToRequestMetadata
returns a newImmutableMap
so nothing should be added to this (nor do we want anyone to).