Skip to content

Commit

Permalink
fix(kubernetes-client-api): TokenRefreshInterceptor doesn't overwrite…
Browse files Browse the repository at this point in the history 
… existing OAuth token with empty string

Signed-off-by: Marc Nuri <[email protected]>

(cherry picked from commit ad7c825)
Signed-off-by: Marc Nuri <[email protected]>
  • Loading branch information
@manusa
manusa committed Sep 29, 2022
1 parent cb915d0 commit d83af67
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 6 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
* * Fix #4206: KubernetesDeserializer can now handle any valid object. If the object lacks type information, it will be deserialized as a GenericKubernetesResource.
* Fix #4365: backport of stopped future for informers to obtain the termination exception
* Fix #4383: bump snakeyaml from 1.28 to 1.33
* Fix #4442: TokenRefreshInterceptor doesn't overwrite existing OAuth token with empty string

#### _**Note**_: Behavior changes
* Fix #4206: The Serialization utility class will throw an Exception, instead of returning null, if an untyped unmarshall method is used on something that lacks type information
Expand Down
2 changes: 1 addition & 1 deletion ...etes-client/src/main/java/io/fabric8/kubernetes/client/utils/TokenRefreshInterceptor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ private boolean refreshToken(BasicBuilder headerBuilder) {
newAccessToken = newestConfig.getOauthToken();
}

if (newAccessToken != null) {
if (Utils.isNotNullOrEmpty(newAccessToken)) {
// Delete old Authorization header and append new one
headerBuilder
.setHeader("Authorization", "Bearer " + newAccessToken);
Expand Down
69 changes: 64 additions & 5 deletions ...-client/src/test/java/io/fabric8/kubernetes/client/utils/TokenRefreshInterceptorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,14 @@
package io.fabric8.kubernetes.client.utils;

import io.fabric8.kubernetes.client.Config;
import io.fabric8.kubernetes.client.ConfigBuilder;
import io.fabric8.kubernetes.client.http.BasicBuilder;
import io.fabric8.kubernetes.client.http.HttpClient;
import io.fabric8.kubernetes.client.http.HttpRequest;
import io.fabric8.kubernetes.client.http.HttpResponse;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.mockito.MockedStatic;
import org.mockito.Mockito;

import java.io.File;
Expand All @@ -35,7 +40,13 @@
import static io.fabric8.kubernetes.client.Config.KUBERNETES_AUTH_TRYKUBECONFIG_SYSTEM_PROPERTY;
import static io.fabric8.kubernetes.client.Config.KUBERNETES_KUBECONFIG_FILE;
import static io.fabric8.kubernetes.client.MockHttpClientUtils.buildResponse;
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.CALLS_REAL_METHODS;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.mockStatic;
import static org.mockito.Mockito.when;

/**
* Ignoring for now - the token refresh should be based upon the java 11 client or the provided client library and not okhttp
Expand All @@ -50,7 +61,7 @@ public void shouldAutoconfigureAfter401() throws IOException {
Files.copy(Objects.requireNonNull(getClass().getResourceAsStream("/test-kubeconfig-tokeninterceptor")), Paths.get(tempFile.getPath()), StandardCopyOption.REPLACE_EXISTING);
System.setProperty(KUBERNETES_KUBECONFIG_FILE, tempFile.getAbsolutePath());

HttpRequest.Builder builder = Mockito.mock(HttpRequest.Builder.class, Mockito.RETURNS_SELF);
HttpRequest.Builder builder = mock(HttpRequest.Builder.class, Mockito.RETURNS_SELF);

// Call
boolean reissue = new TokenRefreshInterceptor(Config.autoConfigure(null), null).afterFailure(builder, buildResponse(HttpURLConnection.HTTP_UNAUTHORIZED, "foo"));
Expand All @@ -71,7 +82,7 @@ void shouldAutoconfigureAfter1Minute() throws Exception {
Paths.get(tempFile.getPath()), StandardCopyOption.REPLACE_EXISTING);
System.setProperty(KUBERNETES_KUBECONFIG_FILE, tempFile.getAbsolutePath());

HttpRequest.Builder builder = Mockito.mock(HttpRequest.Builder.class, Mockito.RETURNS_SELF);
HttpRequest.Builder builder = mock(HttpRequest.Builder.class, Mockito.RETURNS_SELF);

// Call
TokenRefreshInterceptor tokenRefreshInterceptor = new TokenRefreshInterceptor(Config.autoConfigure(null), null);
Expand All @@ -87,6 +98,54 @@ void shouldAutoconfigureAfter1Minute() throws Exception {
}
}

@Test
@DisplayName("#4442 token auto refresh should not overwrite existing token when not applicable")
void refreshShouldNotOverwriteExistingToken() throws Exception {
try (MockedStatic<Config> configMock = mockStatic(Config.class, CALLS_REAL_METHODS)) {
// Given
final Config autoConfig = new ConfigBuilder(Config.empty())
.withOauthToken("") // empty token
.build();
configMock.when(() -> Config.autoConfigure(any())).thenReturn(autoConfig);
final Config config = new ConfigBuilder(Config.empty())
.withOauthToken("existing-token")
.build();
final TokenRefreshInterceptor tokenRefreshInterceptor = new TokenRefreshInterceptor(config, null);
tokenRefreshInterceptor.setLastRefresh(Instant.now().minusSeconds(61));
final HttpResponse<String> response = mock(HttpResponse.class);
when(response.code()).thenReturn(401);
// When
final boolean result = tokenRefreshInterceptor.afterFailure(mock(BasicBuilder.class), response);
// Then
assertThat(result).isFalse();
assertThat(config).hasFieldOrPropertyWithValue("oauthToken", "existing-token");
}
}

@Test
@DisplayName("#4442 token auto refresh should overwrite existing token when applicable")
void refreshShouldOverwriteExistingToken() throws Exception {
try (MockedStatic<Config> configMock = mockStatic(Config.class, CALLS_REAL_METHODS)) {
// Given
final Config autoConfig = new ConfigBuilder(Config.empty())
.withOauthToken("new-token")
.build();
configMock.when(() -> Config.autoConfigure(any())).thenReturn(autoConfig);
final Config config = new ConfigBuilder(Config.empty())
.withOauthToken("existing-token")
.build();
final TokenRefreshInterceptor tokenRefreshInterceptor = new TokenRefreshInterceptor(config, null);
tokenRefreshInterceptor.setLastRefresh(Instant.now().minusSeconds(61));
final HttpResponse<String> response = mock(HttpResponse.class);
when(response.code()).thenReturn(401);
// When
final boolean result = tokenRefreshInterceptor.afterFailure(mock(BasicBuilder.class), response);
// Then
assertThat(result).isTrue();
assertThat(config).hasFieldOrPropertyWithValue("oauthToken", "new-token");
}
}

@Test
void shouldReloadInClusterServiceAccount() throws IOException {
try {
Expand All @@ -97,7 +156,7 @@ void shouldReloadInClusterServiceAccount() throws IOException {
System.setProperty(KUBERNETES_AUTH_SERVICEACCOUNT_TOKEN_FILE_SYSTEM_PROPERTY, tokenFile.getAbsolutePath());
System.setProperty(KUBERNETES_AUTH_TRYKUBECONFIG_SYSTEM_PROPERTY, "false");

HttpRequest.Builder builder = Mockito.mock(HttpRequest.Builder.class, Mockito.RETURNS_SELF);
HttpRequest.Builder builder = mock(HttpRequest.Builder.class, Mockito.RETURNS_SELF);

// The expired token will be read at auto configure.
TokenRefreshInterceptor interceptor = new TokenRefreshInterceptor(Config.autoConfigure(null), null);
Expand Down Expand Up @@ -126,7 +185,7 @@ void shouldRefreshOIDCToken() throws IOException {
System.setProperty(KUBERNETES_KUBECONFIG_FILE, tempFile.getAbsolutePath());

// Prepare HTTP call that will fail with 401 Unauthorized to trigger OIDC token renewal.
HttpRequest.Builder builder = Mockito.mock(HttpRequest.Builder.class, Mockito.RETURNS_SELF);
HttpRequest.Builder builder = mock(HttpRequest.Builder.class, Mockito.RETURNS_SELF);

// Loads the initial kubeconfig, including initial token value.
Config config = Config.autoConfigure(null);
Expand All @@ -140,7 +199,7 @@ void shouldRefreshOIDCToken() throws IOException {
Files.copy(Objects.requireNonNull(getClass().getResourceAsStream("/test-kubeconfig-tokeninterceptor-oidc")),
Paths.get(tempFile.getPath()), StandardCopyOption.REPLACE_EXISTING);

TokenRefreshInterceptor interceptor = new TokenRefreshInterceptor(config, Mockito.mock(HttpClient.Factory.class));
TokenRefreshInterceptor interceptor = new TokenRefreshInterceptor(config, mock(HttpClient.Factory.class));
boolean reissue = interceptor.afterFailure(builder, buildResponse(HttpURLConnection.HTTP_UNAUTHORIZED, "foo"));

// Make the call and check that renewed token was read at 401 Unauthorized.
Expand Down

0 comments on commit d83af67

Please sign in to comment.